feat: add migration checker cmd #1114
Conversation
WalkthroughThe changes enhance the functionality of the migration-checker tool and the trie structures. In Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main
participant HTTP_Server
participant SecureTrie
User->>Main: Run migration-checker
Main->>HTTP_Server: Start on 0.0.0.0:6060
HTTP_Server-->>Main: Log any errors
Main->>SecureTrie: Create SecureTrie instance
SecureTrie-->>Main: Return instance
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (10)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
omerfirmak
force-pushed
the
omerfirmak/checker-cmd
branch
17 times, most recently
from
a9d9968 to
1651970
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
cmd/migration-checker/main.go (7)
21-22: Consider grouping related global variables into a config struct.
Storing global variables likeaccountsDoneandtrieCheckersis convenient, but it can introduce shared state across the entire application. Wrapping them in a configuration or context structure may improve clarity, testability, and modularity.
29-58: Evaluate replacing panics with error returns or exit codes.
While usingpanicOnErroris acceptable in a development tool, panics might complicate automated scripts or higher-level orchestration. Converting these to user-facing error messages with proper exit codes could yield a cleaner CLI experience.
46-49: Check concurrency capacity alignment with real-world usage.
Usingruntime.GOMAXPROCS(0)*4for channel capacity is a good start, but it may need tuning for very large databases. Monitoring resource utilization and adjusting this factor might improve throughput or prevent overconsumption of system resources.
81-83: Add clarity before panicking on leaf count mismatch.
While it’s correct to panic if leaf counts differ, consider logging the specific roots or DB paths for easier troubleshooting.
105-107: Remove unnecessary byte slice conversion.
Static analysis indicates an unnecessary conversion to[]byte(...)formptKeyandpreimageKey, which are already[]byte.- panic(fmt.Sprintf("%s key %s (preimage %s) not found in mpt", label, hex.EncodeToString([]byte(mptKey)), hex.EncodeToString([]byte(preimageKey)))) + panic(fmt.Sprintf("%s key %s (preimage %s) not found in mpt", label, hex.EncodeToString(mptKey), hex.EncodeToString(preimageKey)))🧰 Tools
🪛 GitHub Check: check
[failure] 107-107:
unnecessary conversion (unconvert)
134-146: Handle goroutine panics more gracefully.
Capturing panics in the goroutine and then callingos.Exit(1)is valid, but consider using a more structured error-reporting approach. This might help gather partial results or trigger retries.
217-245: Ensure consistent error handling inloadZkTrie.
panicis used when a preimage is missing. If missing preimages are a frequent partial data scenario, consider logging or returning an error to allow partial inspections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/migration-checker/main.go(1 hunks)trie/secure_trie.go(1 hunks)trie/tracer.go(4 hunks)trie/zk_trie.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: check
cmd/migration-checker/main.go
[failure] 107-107:
unnecessary conversion (unconvert)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (4)
cmd/migration-checker/main.go (2)
69-112: Verify potential mismatch in leaf counts.
The code darts right into apanicif there is a mismatch between MPT leaves and ZK leaves. This strict approach is valid for a verification tool, but confirm whether partial or incremental mismatch logs might be more appropriate for diagnosing large data sets.🧰 Tools
🪛 GitHub Check: check
[failure] 107-107:
unnecessary conversion (unconvert)
167-215: Watch for race conditions on the shared map.
This code uses a mutex for the sharedmptLeafMap. However, the brief locking aroundif _, ok := mptLeafMap[...]andmptLeafMap[...] = ...might leave a small gap if multiple goroutines detect the same key concurrently before any break/return. Confirm whether this is acceptable or if you require a more robust check-and-insert process.trie/tracer.go (1)
63-66: Nil receiver checks look correct.
These addedif t == nil { return }conditions prevent panics whentraceris intentionally set tonil(e.g., viaNewSecureNoTracer). This change maintains robust handling if the tracer is disabled.Also applies to: 74-77, 89-92, 102-105, 114-116, 130-133
trie/secure_trie.go (1)
68-76:NewSecureNoTracerfunction is well-structured.
The approach of callingNewSecureand then settingtrie.tracer = nilis straightforward and aligns with the new nil-check logic intracer.go. Consider documenting any limitations when tracing is disabled (e.g., no record of deleted nodes).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
trie/zk_trie.go (1)
242-248: 🛠️ Refactor suggestionImprove error handling and add documentation.
The method needs better error handling and documentation.
Additionally, fix the pipeline failures by adding the missing
verifyNodeHashesargument:-func (t *ZkTrie) CountLeaves(cb func(key, value []byte), parallel, verifyNodeHashes bool) uint64 { +func (t *ZkTrie) CountLeaves(cb func(key, value []byte), parallel, verifyNodeHashes bool) (uint64, error) { root, err := t.ZkTrie.Tree().Root() if err != nil { - panic("CountLeaves cannot get root") + return 0, fmt.Errorf("failed to get root: %w", err) } - return t.countLeaves(root, cb, 0, parallel, verifyNodeHashes) + return t.countLeaves(root, cb, 0, parallel, verifyNodeHashes), nil }
🧹 Nitpick comments (1)
cmd/migration-checker/main.go (1)
39-42: Consider making levelDB buffer sizes configurable.The hardcoded buffer sizes (1024, 128) for levelDB might not be optimal for all scenarios. Consider making these configurable via command-line flags.
var ( mptDbPath = flag.String("mpt-db", "", "path to the MPT node DB") zkDbPath = flag.String("zk-db", "", "path to the ZK node DB") mptRoot = flag.String("mpt-root", "", "root hash of the MPT node") zkRoot = flag.String("zk-root", "", "root hash of the ZK node") paranoid = flag.Bool("paranoid", false, "verifies all node contents against their expected hash") + cacheSize = flag.Int("cache-size", 1024, "size of the data cache in MB") + handles = flag.Int("handles", 128, "number of file handles") )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/migration-checker/main.go(1 hunks)trie/zk_trie.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: test
trie/zk_trie.go
[failure] 279-279:
not enough arguments in call to leftT.countLeaves
[failure] 282-282:
not enough arguments in call to rightT.countLeaves
[failure] 286-286:
not enough arguments in call to t.countLeaves
🪛 GitHub Check: check
trie/zk_trie.go
[failure] 279-279:
not enough arguments in call to leftT.countLeaves
[failure] 282-282:
not enough arguments in call to rightT.countLeaves
[failure] 286-286:
not enough arguments in call to t.countLeaves
[failure] 279-279:
not enough arguments in call to leftT.countLeaves
[failure] 282-282:
not enough arguments in call to rightT.countLeaves
[failure] 286-286:
not enough arguments in call to t.countLeaves
[failure] 279-279:
not enough arguments in call to leftT.countLeaves
cmd/migration-checker/main.go
[failure] 18-18:
could not import github.com/scroll-tech/go-ethereum/trie (-: # github.com/scroll-tech/go-ethereum/trie
🪛 GitHub Check: build-mock-ccc-geth
trie/zk_trie.go
[failure] 279-279:
not enough arguments in call to leftT.countLeaves
[failure] 282-282:
not enough arguments in call to rightT.countLeaves
[failure] 286-286:
not enough arguments in call to t.countLeaves
🪛 GitHub Actions: CI
trie/zk_trie.go
[error] 279-279: not enough arguments in call to leftT.countLeaves
🪛 golangci-lint (1.62.2)
cmd/migration-checker/main.go
18-18: could not import github.com/scroll-tech/go-ethereum/trie (-: # github.com/scroll-tech/go-ethereum/trie
trie/zk_trie.go:279:62: not enough arguments in call to leftT.countLeaves
have (*zktrie.Hash, func(key []byte, value []byte), int, bool)
want (*zktrie.Hash, func(key []byte, value []byte), int, bool, bool)
trie/zk_trie.go:282:63: not enough arguments in call to rightT.countLeaves
have (*zktrie.Hash, func(key []byte, value []byte), int, bool)
want (*zktrie.Hash, func(key []byte, value []byte), int, bool, bool)
trie/zk_trie.go:286:55: not enough arguments in call to t.countLeaves
have (*zktrie.Hash, func(key []byte, value []byte), int, bool)
want (*zktrie.Hash, func(key []byte, value []byte), int, bool, bool))
(typecheck)
omerfirmak
force-pushed
the
omerfirmak/checker-cmd
branch
from
81ab44f to
12ec3a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
cmd/migration-checker/main.go (2)
61-65: 🛠️ Refactor suggestionConsider returning errors instead of panicking.
Using panic for error handling is not a best practice. Consider returning errors and letting the caller decide how to handle them.
70-113: 🛠️ Refactor suggestionRefactor for better modularity and readability.
The function is doing too many things: creating tries, loading data, comparing data, and handling preimage keys. Consider splitting it into smaller, more focused functions.
🧰 Tools
🪛 GitHub Check: check
[failure] 108-108:
unnecessary conversion (unconvert)trie/zk_trie.go (2)
242-248: 🛠️ Refactor suggestionImprove error handling and add documentation.
The method needs the following improvements:
- Return an error instead of panicking
- Add nil check for the callback function
- Add documentation explaining the purpose and parameters
250-289:⚠️ Potential issueFix missing arguments and improve error handling.
The method has several issues:
- Missing
verifyNodeHashesargument in recursive calls- Using panic for error handling
- No error handling in goroutines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/migration-checker/main.go(1 hunks)trie/secure_trie.go(1 hunks)trie/tracer.go(4 hunks)trie/zk_trie.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- trie/tracer.go
- trie/secure_trie.go
🧰 Additional context used
🪛 GitHub Check: check
cmd/migration-checker/main.go
[failure] 108-108:
unnecessary conversion (unconvert)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
cmd/migration-checker/main.go (3)
67-69: LGTM!The function correctly creates a copy of the input byte slice.
158-166: LGTM!The function correctly compares storage values between ZK and MPT tries.
218-246: LGTM!The function correctly loads ZK trie data with proper synchronization for parallel processing.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/migration-checker/main.go (2)
21-22: Consider encapsulating global variables within a struct.Global variables make testing difficult and can introduce subtle bugs. Consider encapsulating these within a struct:
-var accountsDone atomic.Uint64 -var trieCheckers = make(chan struct{}, runtime.GOMAXPROCS(0)*4) +type migrationChecker struct { + accountsDone atomic.Uint64 + trieCheckers chan struct{} +} + +func newMigrationChecker() *migrationChecker { + maxWorkers := runtime.GOMAXPROCS(0) * 4 + if maxWorkers <= 0 { + maxWorkers = 1 + } + return &migrationChecker{ + trieCheckers: make(chan struct{}, maxWorkers), + } +}
70-113: Optimize memory usage in trie comparison.The function loads all leaves into memory before comparison. For large tries, this could cause out-of-memory issues.
Consider implementing a streaming comparison that processes leaves in batches:
func checkTrieEquality(...) { + const batchSize = 1000 // ... existing setup code ... - mptLeafMap := <-mptLeafCh - zkLeafMap := <-zkLeafCh + for { + mptBatch := make(map[string][]byte, batchSize) + zkBatch := make(map[string][]byte, batchSize) + // ... load and compare batches ... + if done { + break + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/migration-checker/main.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
cmd/migration-checker/main.go (1)
61-65: Replace panic with error returns for better error handling.Using panic for error handling is not a best practice.
See previous suggestion about returning errors instead of panicking.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
cmd/migration-checker/main.go (2)
110-110:⚠️ Potential issueUpdate function signature to include paranoid parameter.
The function signature for
checkAccountEqualityneeds to be updated to include the paranoid parameter, which is passed from checkTrieEquality.-func checkAccountEquality(label string, dbs *dbs, zkAccountBytes, mptAccountBytes []byte) { +func checkAccountEquality(label string, dbs *dbs, zkAccountBytes, mptAccountBytes []byte, paranoid bool) {
35-35:⚠️ Potential issueAdd paranoid mode flag.
Looking at the code, there's a
paranoidparameter in the updatedloadZkTriefunction (line 249-277), but it's not defined in the command-line flags.var ( mptDbPath = flag.String("mpt-db", "", "path to the MPT node DB") zkDbPath = flag.String("zk-db", "", "path to the ZK node DB") mptRoot = flag.String("mpt-root", "", "root hash of the MPT node") zkRoot = flag.String("zk-root", "", "root hash of the ZK node") + paranoid = flag.Bool("paranoid", false, "verifies all node contents against their expected hash") )
🧹 Nitpick comments (1)
cmd/migration-checker/main.go (1)
1-241: Add elapsed time tracking for performance monitoring.The code would benefit from elapsed time tracking to help monitor the performance of the migration checking process.
import ( // ... existing imports + "time" ) func main() { + startTime := time.Now() // ... existing code checkTrieEquality(&dbs{ zkDb: zkDb, mptDb: mptDb, }, zkRootHash, mptRootHash, "", checkAccountEquality, true, *paranoid) + + elapsed := time.Since(startTime) + fmt.Printf("Migration check completed in %s\n", elapsed) } func checkTrieEquality(dbs *dbs, zkRoot, mptRoot common.Hash, label string, leafChecker func(string, *dbs, []byte, []byte, bool), top bool, paranoid bool) { + checkStartTime := time.Now() // ... existing code + if top { + elapsed := time.Since(checkStartTime) + fmt.Printf("Trie equality check completed in %s\n", elapsed) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/migration-checker/main.go(1 hunks)cmd/migration-checker/main.go(1 hunks)cmd/migration-checker/main.go(8 hunks)cmd/migration-checker/main.go(1 hunks)cmd/migration-checker/main.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/migration-checker/main.go
- cmd/migration-checker/main.go
- cmd/migration-checker/main.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (6)
cmd/migration-checker/main.go (6)
29-54: Add validation for required command line flags.The current implementation doesn't validate that required flags are provided before proceeding with execution. Without validation, the program might panic with cryptic errors when trying to use empty values.
func main() { var ( mptDbPath = flag.String("mpt-db", "", "path to the MPT node DB") zkDbPath = flag.String("zk-db", "", "path to the ZK node DB") mptRoot = flag.String("mpt-root", "", "root hash of the MPT node") zkRoot = flag.String("zk-root", "", "root hash of the ZK node") ) flag.Parse() + + if *mptDbPath == "" || *zkDbPath == "" || *mptRoot == "" || *zkRoot == "" { + fmt.Println("Error: All flags (mpt-db, zk-db, mpt-root, zk-root) must be provided") + flag.Usage() + os.Exit(1) + }
56-60: Consider returning errors instead of panicking.Using panic for error handling is not a best practice. Consider returning errors and letting the caller decide how to handle them.
-func panicOnError(err error, label, msg string) { +func handleError(err error, label, msg string) error { if err != nil { - panic(fmt.Sprint(label, " error: ", msg, " ", err)) + return fmt.Errorf("%s error: %s %w", label, msg, err) } + return nil }
134-146: Improve error handling in goroutines.The goroutine error handling could be improved by using a channel to propagate errors from goroutines.
<-trieCheckers +errCh := make(chan error, 1) go func() { defer func() { if p := recover(); p != nil { - fmt.Println(p) - os.Exit(1) + if err, ok := p.(error); ok { + errCh <- fmt.Errorf("%s: %w", label, err) + } else { + errCh <- fmt.Errorf("%s: %v", label, p) + } + return } + errCh <- nil }() checkTrieEquality(dbs, zkRoot, mptRoot, label, checkStorageEquality, false, paranoid) accountsDone.Add(1) fmt.Println("Accounts done:", accountsDone.Load()) trieCheckers <- struct{}{} }() +if err := <-errCh; err != nil { + panic(err) +}
187-190: Fix potential race condition in break condition.The break condition after checking for duplicate keys has a race condition. The mutex is unlocked before breaking, which could lead to inconsistent state.
+var shouldBreak bool if _, ok := mptLeafMap[string(trieIt.Key)]; ok { - mptLeafMutex.Unlock() - break + shouldBreak = true } -mptLeafMap[string(dup(trieIt.Key))] = dup(trieIt.Value) +if !shouldBreak { + mptLeafMap[string(dup(trieIt.Key))] = dup(trieIt.Value) +} if parallel { mptLeafMutex.Unlock() } +if shouldBreak { + break +}
224-232: Fix potential race condition in ZK trie loading.Similar to the MPT loading function, there's a potential race condition when checking for duplicates in the ZK trie loading function.
if parallel { zkLeafMutex.Lock() } +defer func() { + if parallel { + zkLeafMutex.Unlock() + } +}() zkLeafMap[string(dup(preimageKey))] = value - -if parallel { - zkLeafMutex.Unlock() -}
65-108: Refactor for better modularity and readability.The function is doing too many things: creating tries, loading data, comparing data, and handling preimage keys. Consider splitting it into smaller, more focused functions.
A good refactoring would include:
- Extract preimage key handling into a separate function
- Extract trie creation into a separate function
- Extract comparison logic into a separate function
Example refactor for preimage key handling:
+func normalizePreimageKey(preimageKey string, top bool) string { + if top { + if len(preimageKey) > 20 { + for _, b := range []byte(preimageKey)[20:] { + if b != 0 { + panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", + hex.EncodeToString([]byte(preimageKey)))) + } + } + return preimageKey[:20] + } + } else if len(preimageKey) != 32 { + zeroes := make([]byte, 32) + copy(zeroes, []byte(preimageKey)) + return string(zeroes) + } + return preimageKey +}
| trieIt := trie.NewIterator(mptTrie.NodeIterator(startKey)) | ||
|
|
||
| mptWg.Add(1) | ||
| go func() { | ||
| defer mptWg.Done() | ||
| for trieIt.Next() { | ||
| if parallel { | ||
| mptLeafMutex.Lock() | ||
| } | ||
|
|
||
| if _, ok := mptLeafMap[string(trieIt.Key)]; ok { | ||
| mptLeafMutex.Unlock() | ||
| break | ||
| } | ||
|
|
||
| mptLeafMap[string(dup(trieIt.Key))] = dup(trieIt.Value) | ||
|
|
||
| if parallel { | ||
| mptLeafMutex.Unlock() | ||
| } | ||
|
|
||
| if parallel && len(mptLeafMap)%10000 == 0 { | ||
| fmt.Println("MPT Accounts Loaded:", len(mptLeafMap)) | ||
| } | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| respChan := make(chan map[string][]byte) |
There was a problem hiding this comment.
Resolve conflict in loadZkTrie function definitions.
There appears to be two definitions of the loadZkTrie function. The first version (lines 213-241) doesn't have a paranoid parameter, while the second version (lines 249-277) does. You need to resolve this conflict and keep only the second version.
Remove lines 213-241 and keep the version that includes the paranoid parameter (lines 249-277).
Also applies to: 242-277
| } | ||
| }() | ||
| defer close(done) | ||
|
|
There was a problem hiding this comment.
Update function signature to include paranoid parameter.
The function signature for checkTrieEquality needs to be updated to include the paranoid parameter, which is used in the loadZkTrie call.
-func checkTrieEquality(dbs *dbs, zkRoot, mptRoot common.Hash, label string, leafChecker func(string, *dbs, []byte, []byte), top bool) {
+func checkTrieEquality(dbs *dbs, zkRoot, mptRoot common.Hash, label string, leafChecker func(string, *dbs, []byte, []byte, bool), top bool, paranoid bool) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func checkTrieEquality(dbs *dbs, zkRoot, mptRoot common.Hash, label string, leafChecker func(string, *dbs, []byte, []byte, bool), top bool, paranoid bool) { | |
| // existing function implementation | |
| } |
| panicOnError(err, label, "failed to decode zk account") | ||
|
|
||
| if mptAccount.Nonce != zkAccount.Nonce { | ||
| panic(fmt.Sprintf("%s nonce mismatch: zk: %d, mpt: %d", label, zkAccount.Nonce, mptAccount.Nonce)) |
There was a problem hiding this comment.
Update function signature to include paranoid parameter.
The function signature for checkStorageEquality needs to be updated to include the paranoid parameter for consistency.
-func checkStorageEquality(label string, _ *dbs, zkStorageBytes, mptStorageBytes []byte) {
+func checkStorageEquality(label string, _ *dbs, zkStorageBytes, mptStorageBytes []byte, paranoid bool) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| panic(fmt.Sprintf("%s nonce mismatch: zk: %d, mpt: %d", label, zkAccount.Nonce, mptAccount.Nonce)) | |
| func checkStorageEquality(label string, _ *dbs, zkStorageBytes, mptStorageBytes []byte, paranoid bool) { | |
| panic(fmt.Sprintf("%s nonce mismatch: zk: %d, mpt: %d", label, zkAccount.Nonce, mptAccount.Nonce)) | |
| } |
| panic(fmt.Sprintf("%s key %s (preimage %s) not found in mpt", label, hex.EncodeToString(mptKey), hex.EncodeToString([]byte(preimageKey)))) | ||
| } | ||
|
|
||
| leafChecker(fmt.Sprintf("%s key: %s", label, hex.EncodeToString([]byte(preimageKey))), dbs, zkValue, mptVal, paranoid) |
There was a problem hiding this comment.
Update function call to pass paranoid parameter.
The function call to checkTrieEquality within the goroutine needs to include the paranoid parameter.
- checkTrieEquality(dbs, zkRoot, mptRoot, label, checkStorageEquality, false)
+ checkTrieEquality(dbs, zkRoot, mptRoot, label, checkStorageEquality, false, paranoid)Committable suggestion skipped: line range outside the PR's diff.
cmd/migration-checker/main.go
Outdated
| for i := 0; i < runtime.GOMAXPROCS(0)*4; i++ { | ||
| <-trieCheckers |
There was a problem hiding this comment.
Update function calls to pass paranoid parameter.
The calls to loadMPT and loadZkTrie need to be updated to match their new function signatures and pass the paranoid parameter.
- mptLeafCh := loadMPT(mptTrie, top)
- zkLeafCh := loadZkTrie(zkTrie, top)
+ mptLeafCh := loadMPT(mptTrie, top)
+ zkLeafCh := loadZkTrie(zkTrie, top, paranoid)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i := 0; i < runtime.GOMAXPROCS(0)*4; i++ { | |
| <-trieCheckers | |
| mptLeafCh := loadMPT(mptTrie, top) | |
| zkLeafCh := loadZkTrie(zkTrie, top, paranoid) |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
cmd/migration-checker/main.go (1)
242-280:⚠️ Potential issueUnresolved merge conflict and duplicate code.
Lines 242–280 include another partial/unwanted definition or leftover code. This will not compile as-is. Remove or merge the conflicting definitions of
loadZkTrieproperly.-} - } - -respChan := make(chan map[string][]byte) -go func() { - mptWg.Wait() - respChan <- mptLeafMap -}() -return respChan -} -} - -func loadZkTrie(zkTrie *trie.ZkTrie, parallel, paranoid bool) chan map[string][]byte { +func loadZkTrie(zkTrie *trie.ZkTrie, parallel, paranoid bool) chan map[string][]byte { // keep only the most up-to-date definition here // ... }
🧹 Nitpick comments (1)
cmd/migration-checker/main.go (1)
65-108: Consider splitting the function for modularity.
checkTrieEqualityis doing multiple tasks—creating tries, loading leaves, normalizing preimages, and verifying matching leaves. Splitting into smaller functions would improve readability and testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/migration-checker/main.go(1 hunks)cmd/migration-checker/main.go(1 hunks)cmd/migration-checker/main.go(8 hunks)cmd/migration-checker/main.go(1 hunks)cmd/migration-checker/main.go(3 hunks)cmd/migration-checker/main.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/migration-checker/main.go
- cmd/migration-checker/main.go
- cmd/migration-checker/main.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (6)
cmd/migration-checker/main.go (6)
29-36: Validate required command-line flags.Currently, there's no validation for mandatory flags (e.g.,
--mpt-db,--zk-db,--mpt-root,--zk-root). Exiting or showing usage instructions when these flags are not provided would improve user experience and avoid potential runtime failures.
56-60: Consider returning errors instead of panicking.Using
panicOnErrorfor error handling can abruptly terminate the process and limit composability. Allowing callers to handle errors would improve robustness.
110-151: Improve error handling in goroutines.Calling
os.Exit(1)on panic inside the goroutine stops the entire process prematurely. Use error channels to propagate failures gracefully without crashing everything.
153-161: Storage comparison logic looks good.The method correctly extracts the MPT storage content and compares it to the ZK storage hash. No issues found here.
163-211: Fix the race condition when breaking on duplicate keys.Breaking immediately after unlocking leaves the map unprotected if other goroutines continue writing. Use a temporary boolean or a deferred unlock to avoid data races.
213-241: Duplicate definition of loadZkTrie (without paranoid parameter).This appears to be an old version of the function. Retain only one version of
loadZkTrieto avoid conflicts and confusion.
1. Purpose or design rationale of this PR
...
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
SecureTrieinstance without tracing.ZkTriewith optional hash verification and parallel execution support.Bug Fixes