Repository navigation
Fix FlatKV state sync bad-hash scenario. - #4370
Conversation
PR SummaryMedium Risk Overview Splits import paths: Adds regression tests for duplicate-key LtHash wraparound, out-of-order rejection, and trusted any-order import; existing LtHash tests feed nodes via Reviewed by Cursor Bugbot for commit 141cc6d. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4370 +/- ##
==========================================
- Coverage 67.70% 66.93% -0.78%
==========================================
Files 2168 2070 -98
Lines 168283 159290 -8993
==========================================
- Hits 113938 106622 -7316
+ Misses 54336 52659 -1677
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR makes the untrusted FlatKV import path (state sync, including the composite importer's lazy flatkvFactory) reject any physical key that is not strictly greater than the one before it. That rules out duplicate rows that could wrap the LtHash limbs and forge a matching root. The offline memiavl migration moves to an explicit TrustedImporter that accepts keys in any order. The ordering matches what KVExporter/RawGlobalIterator already emit (lexicographic across DBs, with meta keys skipped), so existing snapshots stay importable, and tests cover rejection, the trusted path and the updated call sites. I found nothing blocking.
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
| // the first of the import. | ||
| func checkAscending(prevKey []byte, key []byte) error { | ||
| if prevKey != nil && bytes.Compare(prevKey, key) >= 0 { | ||
| return fmt.Errorf("flatkv import: physical key %x does not strictly follow %x", key, prevKey) |
There was a problem hiding this comment.
how about we have diff error message for dup key vs key out of order.
also wording strictly follow is not very obvious what's order it should follow
| } | ||
|
|
||
| // TrustedImporter returns an importer for data from a trusted source, such as an offline migration of this | ||
| // node's own state. It accepts physical keys in any order. |
There was a problem hiding this comment.
It accepts physical keys in any order but each key must appear at most once.
| return | ||
| } | ||
| if imp.requireAscendingKeys { | ||
| if err := checkAscending(prevKey, kv.Key); err != nil { |
There was a problem hiding this comment.
What if the prev keys ends at previous DB and current is the first key of the next DB?
There was a problem hiding this comment.
Current flatKV state sync already provides a global sorted ordering. i.e. the sender does a merge sort on the data extracted from each of the 4 DBs before sending it.
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-4370-to-release/v6.7
git worktree add --checkout .worktree/backport-4370-to-release/v6.7 backport-4370-to-release/v6.7
cd .worktree/backport-4370-to-release/v6.7
git reset --hard HEAD^
git cherry-pick -x 712fa98e10eb7dd3c245df4d155c4f6cbbd7a7bb
git push --force-with-lease |
) Backport of #4370 to `release/v6.7`. --------- Co-authored-by: Cody Littley <56973212+cody-littley@users.noreply.github.com> Co-authored-by: Cody Littley <cody.littley@seinetwork.io>
4 participants
Fixes scenario where flatKV can compute a bad hash after a state sync. Requires sender to send keys in lexographic order, and refuses duplicate keys.