feat: add migration checker cmd #1114
Conversation
WalkthroughThe changes introduce new functionalities to the migration-checker tool and enhance the trie structures. The 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
✨ 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.
omerfirmak
force-pushed
the
omerfirmak/checker-cmd
branch
2 times, most recently
from
bb6c2d4 to
0eb1fb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
trie/zk_trie.go (1)
250-289: 🛠️ Refactor suggestionAvoid panics in
countLeavesand handle concurrency errors gracefully.The function panics in several places if node retrieval or hash verification fails. This approach prevents any controlled recovery at higher levels and can disrupt in-progress goroutines. Returning an error or feeding the error into a channel would allow callers to respond appropriately.
Example refactor:
func (t *ZkTrie) countLeaves(depth int, root *zkt.Hash, cb func(key, value []byte), spawnWorker func(func()), parallelismCutoffDepth int, verifyNodeHashes bool) (uint64, error) { if root == nil { return 0, nil } rootNode, err := t.ZkTrie.Tree().GetNode(root) if err != nil { - panic("countLeaves cannot get rootNode") + return 0, fmt.Errorf("failed to retrieve node: %w", err) } if rootNode.Type == zktrie.NodeTypeLeaf_New { if verifyNodeHashes { calculatedNodeHash, err := rootNode.NodeHash() if err != nil { - panic("countLeaves cannot get calculatedNodeHash") + return 0, fmt.Errorf("failed to calculate node hash: %w", err) } if *calculatedNodeHash != *root { - panic("countLeaves node hash mismatch") + return 0, fmt.Errorf("node hash mismatch") } } cb(rootNode.NodeKey.Bytes(), rootNode.Data()) - return 1 + return 1, nil } else { if depth < parallelismCutoffDepth { countCh := make(chan uint64, 1) errCh := make(chan error, 1) leftT := t.Copy() rightT := t.Copy() go func() { spawnWorker(func() { - countCh <- leftT.countLeaves(depth+1, rootNode.ChildL, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + ln, lerr := leftT.countLeaves(depth+1, rootNode.ChildL, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + countCh <- ln + errCh <- lerr }) }() - rn := rightT.countLeaves(depth+1, rootNode.ChildR, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) - return rn + <-countCh + rn, rerr := rightT.countLeaves(depth+1, rootNode.ChildR, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + ln := <-countCh + lerr := <-errCh + if rerr != nil { + return 0, rerr + } + if lerr != nil { + return 0, lerr + } + return rn + ln, nil } else { - return t.countLeaves(depth+1, rootNode.ChildL, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + - t.countLeaves(depth+1, rootNode.ChildR, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + leftCount, leftErr := t.countLeaves(depth+1, rootNode.ChildL, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + if leftErr != nil { + return 0, leftErr + } + rightCount, rightErr := t.countLeaves(depth+1, rootNode.ChildR, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + if rightErr != nil { + return 0, rightErr + } + return leftCount + rightCount, nil } } }
🧹 Nitpick comments (4)
cmd/migration-checker/main.go (4)
35-72: Use structured error handling instead of panics inmain().Panic-based error handling within
main()abruptly stops execution. Returning or logging errors could give operators more control and visibility into failures. Also consider logging the HTTP server startup error more robustly.
74-78:panicOnErrorbypasses controlled recovery.By using
panicOnError, non-critical issues can terminate the process. Converting this helper to return or log errors instead would allow callers to decide how to proceed.
144-185: Graceful error handling incheckAccountEquality.This function forcibly exits the process if
recover()catches a panic in the goroutine. Consider returning errors or sending them to a result channel, allowing the main flow to handle them without abruptly stopping the entire program.
187-195:checkStorageEqualityrelies on panics.Consider returning an error if the storage data mismatch, rather than panicking. This change can provide better clarity on partial successes/failures and allow for retry logic.
📜 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
cmd/migration-checker/main.go (2)
80-82: Utility functionduplooks good.This function is straightforward and correctly duplicates the slice. No concerns here.
197-236: Verify partial iteration inloadMPT.When a duplicate key is found, the code breaks from iteration immediately. Confirm that skipping subsequent entries is acceptable. Also, consider handling any iteration errors from
trie.NewIteratorgracefully (e.g., returning them), rather than ignoring them.
trie/zk_trie.go
Outdated
|
|
||
| func (t *ZkTrie) CountLeaves(cb func(key, value []byte), spawnWorker func(func()), parallelismCutoffDepth int, verifyNodeHashes bool) uint64 { | ||
| root, err := t.ZkTrie.Tree().Root() | ||
| if err != nil { | ||
| panic("CountLeaves cannot get root") | ||
| } | ||
| return t.countLeaves(0, root, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace panics with error returns in CountLeaves.
Currently, the function panics if ZkTrie.Tree().Root() fails to load. Using panic can abruptly terminate the application without giving callers a chance to recover. Consider returning an error (or zero count plus an error) to allow graceful handling upstream.
Example refactor:
func (t *ZkTrie) CountLeaves(cb func(key, value []byte), spawnWorker func(func()), parallelismCutoffDepth int, 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(0, root, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes)
+ count := t.countLeaves(0, root, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes)
+ return count, nil
}📝 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 (t *ZkTrie) CountLeaves(cb func(key, value []byte), spawnWorker func(func()), parallelismCutoffDepth int, verifyNodeHashes bool) uint64 { | |
| root, err := t.ZkTrie.Tree().Root() | |
| if err != nil { | |
| panic("CountLeaves cannot get root") | |
| } | |
| return t.countLeaves(0, root, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) | |
| } | |
| func (t *ZkTrie) CountLeaves(cb func(key, value []byte), spawnWorker func(func()), parallelismCutoffDepth int, verifyNodeHashes bool) (uint64, error) { | |
| root, err := t.ZkTrie.Tree().Root() | |
| if err != nil { | |
| return 0, fmt.Errorf("failed to get root: %w", err) | |
| } | |
| count := t.countLeaves(0, root, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) | |
| return count, nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/migration-checker/main.go (1)
35-72: 🛠️ Refactor suggestionValidate user inputs and handle errors more gracefully
Within themainfunction, flag values (mpt-db,zk-db,mpt-root, andzk-root) are assumed valid. If these flags or paths are incorrect, the program panics only after the DB open call fails. Validating flags immediately after parsing can surface errors sooner and avoid partial setup.flag.Parse() +if *mptDbPath == "" || *zkDbPath == "" || *mptRoot == "" || *zkRoot == "" { + fmt.Println("Error: required flags --mpt-db, --zk-db, --mpt-root, --zk-root must be provided.") + os.Exit(1) +} go func() { log.Println(http.ListenAndServe("0.0.0.0:6060", nil)) }()
🧹 Nitpick comments (2)
cmd/migration-checker/main.go (2)
83-142: Consider decomposing this large function for readability and maintainability
checkTrieEqualityhandles setup, concurrency, progress logging, multiple comparisons, and error checking in one place. Splitting these responsibilities into smaller, dedicated functions or modules can improve clarity and testability.
197-236: Propagate a unified cancellation or completion signal in concurrent MPT loading
When a duplicate key is found, one goroutine breaks, but others continue to iterate. For large data sets, this might waste resources. Consider a cancellation context or a shared done channel to stop other goroutines.
📜 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
trie/zk_trie.go (1)
241-248: Replace panics with proper error handling
As previously suggested in other parts of the code, usingpanicfor error handling can make debugging and recovery difficult. Consider returning an error fromCountLeavesinstead, providing callers the opportunity to handle failures gracefully.-func (t *ZkTrie) CountLeaves(cb func(key, value []byte), spawnWorker func(func()), parallelismCutoffDepth int, verifyNodeHashes bool) uint64 { - root, err := t.ZkTrie.Tree().Root() - if err != nil { - panic("CountLeaves cannot get root") - } - return t.countLeaves(0, root, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) -} +func (t *ZkTrie) CountLeaves(cb func(key, value []byte), spawnWorker func(func()), parallelismCutoffDepth int, verifyNodeHashes bool) (uint64, error) { + root, err := t.ZkTrie.Tree().Root() + if err != nil { + return 0, fmt.Errorf("failed to get root: %w", err) + } + count := t.countLeaves(0, root, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + return count, nil +}cmd/migration-checker/main.go (4)
74-78: Avoid using panicOnError for routine errors
This helper enforces a panic whenever an error occurs. That can abruptly stop program execution, leaving no way to recover. Consider returning or logging errors to handle them gracefully.
80-82: No concerns with slice duplication
Thedupfunction is a succinct way to produce a copy of the provided slice. This looks good.
144-185: Replace multiple panics with structured error handling
checkAccountEqualityextensively usespanicto handle unexpected conditions. By returning errors or using a more controlled mechanism (logging, sentinel errors, etc.), you ensure the caller can gracefully handle partial failures and keep the rest of the system operational.
238-269: Remove panic when zk preimage is not found
If the code fails to find a key’s preimage (line 247), it panics immediately. Instead, consider returning an error or using an error channel for concurrency. This aligns better with robust error management practices.
omerfirmak
force-pushed
the
omerfirmak/checker-cmd
branch
2 times, most recently
from
9f8af37 to
324fc7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
trie/zk_trie.go (1)
250-289: 🛠️ Refactor suggestionRefactor concurrency and remove panics for safer error handling.
Within
countLeaves, goroutines are spawned for parallel leaf counting, but there is no mechanism to propagate errors back to the main thread. Panics in the goroutines cannot be intercepted, leading to abrupt termination. Additionally, there is no buffering or wait group to catch the first error should one goroutine fail. Consider:
- Returning
(uint64, error)fromcountLeaves, capturing and merging errors from both subcalls.- Using a buffered channel for counts and errors, or a
sync.WaitGroupplus an error channel, to handle concurrency safely.- Removing or replacing all
paniccalls with proper error returns or logging to allow the caller to handle errors gracefully.Example approach (minimal illustration):
-func (t *ZkTrie) countLeaves(root *zkt.Hash, cb func(key, value []byte), depth int, parallel, verifyNodeHashes bool) uint64 { +func (t *ZkTrie) countLeaves(root *zkt.Hash, cb func(key, value []byte), depth int, parallel, verifyNodeHashes bool) (uint64, error) { if root == nil { - return 0 + return 0, nil } rootNode, err := t.ZkTrie.Tree().GetNode(root) if err != nil { - panic("countLeaves cannot get rootNode") + return 0, fmt.Errorf("failed to get node: %w", err) } // ... }
🧹 Nitpick comments (3)
cmd/migration-checker/main.go (3)
35-76: Validate essential command-line flags before proceeding.The code checks only for errors in opening databases but does not enforce that the user actually provided values for
-mpt-db,-zk-db,-mpt-root, and-zk-root. If these flags are required, validating that they are non-empty and returning an error or usage message would improve user experience.
111-170: Separate concurrency logic from business logic in checkTrieEquality.This function handles concurrency, logs durations, spawns MPT/ZK tries, compares their leaves, and panics on mismatches. Mixing concurrency, I/O, and error handling at this scale complicates nesting and future maintenance. Consider:
- Extracting concurrency-related aspects (channels, goroutines) into a helper function.
- Substituting
paniccalls with structured error returns or logging.- Using a more robust concurrency pattern (e.g., wait groups or buffered channels) to prevent partial loads or half-finished comparisons from leaving the system in an unknown state.
172-213: Use consistent error reporting for account mismatches.Mismatches in nonce, balance, or code hash all trigger a
panic. This approach makes it impossible to gather multiple mismatch details in a single run. Consider returning an aggregated list of errors or logging them, especially if the system needs to continue evaluating other accounts.
📜 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
trie/zk_trie.go (1)
242-248: Consider returning an error instead of panicking in CountLeaves.The function signature returns only
uint64but may encounter errors (e.g., failing to retrieve the root). Relying onpanicprecludes graceful error handling for callers and may abruptly terminate the program. Returning(uint64, error)instead ofuint64would make errors explicit and reduce the risk of unintended crashes.cmd/migration-checker/main.go (2)
102-106: Replace panicOnError with structured error handling.Using
paniccan abruptly terminate the process, making it difficult for upstream callers to recover. Promote better error handling by returning errors or logging them and allowing the caller to decide how to handle unexpected situations.
275-303: Return errors rather than panicking in loadZkTrie.The anonymous goroutine calls
panicon missing preimages, which immediately kills the process. Instead, capture this failure using an error channel or wait group approach that signals back to the caller. This avoids forcing the entire process to crash when a single leaf key is missing.
omerfirmak
force-pushed
the
omerfirmak/checker-cmd
branch
from
324fc7a to
6cd77ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
cmd/migration-checker/main.go (3)
104-108: 🛠️ Refactor suggestionReturn 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 }
227-275:⚠️ Potential issueFix race condition in MPT loading.
There's a potential race condition in the break condition. The mutex is unlocked before breaking, which could lead to inconsistent state.
func loadMPT(mptTrie *trie.SecureTrie, parallel bool) chan map[string][]byte { startKey := make([]byte, 32) workers := 1 << 6 if !parallel { workers = 1 } step := byte(0xFF) / byte(workers) mptLeafMap := make(map[string][]byte, 1000) var mptLeafMutex sync.Mutex var mptWg sync.WaitGroup for i := 0; i < workers; i++ { startKey[0] = byte(i) * step trieIt := trie.NewIterator(mptTrie.NodeIterator(startKey)) mptWg.Add(1) go func() { defer mptWg.Done() for trieIt.Next() { if parallel { mptLeafMutex.Lock() } + 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 + } if parallel && len(mptLeafMap)%10000 == 0 { fmt.Println("MPT Accounts Loaded:", len(mptLeafMap)) } } }() } - respChan := make(chan map[string][]byte) + respChan := make(chan map[string][]byte, 1) go func() { mptWg.Wait() respChan <- mptLeafMap }() return respChan }
277-305: 🛠️ Refactor suggestionFix potential race condition and improve error handling in ZK trie loading.
The current implementation has several issues:
- It does not properly handle errors from
CountLeaves- There is no proper error propagation from the goroutine
- Mutex locking pattern could be improved
func loadZkTrie(zkTrie *trie.ZkTrie, parallel, paranoid bool) chan map[string][]byte { zkLeafMap := make(map[string][]byte, 1000) var zkLeafMutex sync.Mutex - zkDone := make(chan map[string][]byte) + zkDone := make(chan map[string][]byte, 1) + errCh := make(chan error, 1) go func() { - zkTrie.CountLeaves(func(key, value []byte) { + defer func() { + if r := recover(); r != nil { + var err error + if e, ok := r.(error); ok { + err = fmt.Errorf("panic in loadZkTrie: %w", e) + } else { + err = fmt.Errorf("panic in loadZkTrie: %v", r) + } + errCh <- err + } + }() + + count, err := zkTrie.CountLeaves(func(key, value []byte) { preimageKey := zkTrie.GetKey(key) if len(preimageKey) == 0 { - panic(fmt.Sprintf("preimage not found zk trie %s", hex.EncodeToString(key))) + err = fmt.Errorf("preimage not found zk trie %s", hex.EncodeToString(key)) + return } if parallel { zkLeafMutex.Lock() + defer zkLeafMutex.Unlock() } zkLeafMap[string(dup(preimageKey))] = value - if parallel { - zkLeafMutex.Unlock() - } - if parallel && len(zkLeafMap)%10000 == 0 { fmt.Println("ZK Accounts Loaded:", len(zkLeafMap)) } }, parallel, paranoid) + + if err != nil { + errCh <- fmt.Errorf("error counting leaves: %w", err) + return + } + + if len(zkLeafMap) != int(count) { + errCh <- fmt.Errorf("leaf count mismatch: got %d leaves but map has %d entries", count, len(zkLeafMap)) + return + } + zkDone <- zkLeafMap + errCh <- nil }() - return zkDone + + // Return a new channel that includes error handling + resultCh := make(chan map[string][]byte, 1) + go func() { + select { + case result := <-zkDone: + if err := <-errCh; err != nil { + panic(err) // maintain existing panic behavior for now + } + resultCh <- result + case err := <-errCh: + if err != nil { + panic(err) // maintain existing panic behavior for now + } + } + }() + return resultCh }
🧹 Nitpick comments (2)
cmd/migration-checker/main.go (2)
141-143: Improve error message for leaf count mismatch.The current panic message doesn't give enough information to diagnose the issue. Consider enhancing it with more details.
if len(mptLeafMap) != len(zkLeafMap) { - panic(fmt.Sprintf("%s MPT and ZK trie leaf count mismatch: MPT: %d, ZK: %d", label, len(mptLeafMap), len(zkLeafMap))) + panic(fmt.Sprintf("%s MPT and ZK trie leaf count mismatch: MPT: %d, ZK: %d (diff: %d)", + label, len(mptLeafMap), len(zkLeafMap), len(mptLeafMap) - len(zkLeafMap))) }
46-48: Improve HTTP server handling for profiling.The HTTP server for profiling is started in a goroutine with no error recovery mechanism, and errors are only logged.
go func() { - log.Println(http.ListenAndServe("0.0.0.0:6060", nil)) + err := http.ListenAndServe("0.0.0.0:6060", nil) + if err != nil && err != http.ErrServerClosed { + log.Printf("Profiling HTTP server error: %v", err) + // Consider signaling this error to the main thread + } }()
📜 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
cmd/migration-checker/main.go (1)
35-48: Add validation for required command-line flags.The code should validate that required flags are provided before proceeding.
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") paranoid = flag.Bool("paranoid", false, "verifies all node contents against their expected hash") parallelismMultipler = flag.Int("parallelism-multiplier", 4, "multiplier for the number of parallel workers") ) flag.Parse() + + if *mptDbPath == "" || *zkDbPath == "" || *mptRoot == "" || *zkRoot == "" { + fmt.Println("Error: Required flags missing") + flag.Usage() + os.Exit(1) + } go func() { log.Println(http.ListenAndServe("0.0.0.0:6060", nil)) }()
trie/zk_trie.go
Outdated
| func (t *ZkTrie) CountLeaves(cb func(key, value []byte), parallel, verifyNodeHashes bool) uint64 { | ||
| root, err := t.ZkTrie.Tree().Root() | ||
| if err != nil { | ||
| panic("CountLeaves cannot get root") | ||
| } | ||
| return t.countLeaves(root, cb, 0, parallel, verifyNodeHashes) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace panic with proper error handling.
Using panic in a library method can lead to unexpected application crashes. Return errors instead so callers can handle them appropriately.
-func (t *ZkTrie) CountLeaves(cb func(key, value []byte), parallel, verifyNodeHashes bool) uint64 {
+// CountLeaves counts the number of leaf nodes in the trie.
+// It accepts a callback function that is called for each leaf node with its key and value,
+// a parallel flag to enable concurrent counting, and a verification flag to verify node hashes.
+// Returns the total number of leaves and any error encountered.
+func (t *ZkTrie) CountLeaves(cb func(key, value []byte), parallel, verifyNodeHashes bool) (uint64, error) {
+ if cb == nil {
+ return 0, fmt.Errorf("callback function cannot be nil")
+ }
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)
}📝 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 (t *ZkTrie) CountLeaves(cb func(key, value []byte), parallel, verifyNodeHashes bool) uint64 { | |
| root, err := t.ZkTrie.Tree().Root() | |
| if err != nil { | |
| panic("CountLeaves cannot get root") | |
| } | |
| return t.countLeaves(root, cb, 0, parallel, verifyNodeHashes) | |
| } | |
| // CountLeaves counts the number of leaf nodes in the trie. | |
| // It accepts a callback function that is called for each leaf node with its key and value, | |
| // a parallel flag to enable concurrent counting, and a verification flag to verify node hashes. | |
| // Returns the total number of leaves and any error encountered. | |
| func (t *ZkTrie) CountLeaves(cb func(key, value []byte), parallel, verifyNodeHashes bool) (uint64, error) { | |
| if cb == nil { | |
| return 0, fmt.Errorf("callback function cannot be nil") | |
| } | |
| root, err := t.ZkTrie.Tree().Root() | |
| if err != nil { | |
| return 0, fmt.Errorf("failed to get root: %w", err) | |
| } | |
| return t.countLeaves(root, cb, 0, parallel, verifyNodeHashes) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
trie/zk_trie.go (1)
241-248: 🛠️ Refactor suggestionConsider returning errors instead of panicking in
CountLeaves.
By panicking on root retrieval failure at line 245, higher-level code cannot recover from errors gracefully. Converting the panic to an error return fosters safer error propagation and makes your API more robust.-func (t *ZkTrie) CountLeaves(cb func(key, value []byte), spawnWorker func(func()), parallelismCutoffDepth int, verifyNodeHashes bool) uint64 { - root, err := t.ZkTrie.Tree().Root() - if err != nil { - panic("CountLeaves cannot get root") - } - return t.countLeaves(0, root, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) +func (t *ZkTrie) CountLeaves( + cb func(key, value []byte), + spawnWorker func(func()), + parallelismCutoffDepth int, + verifyNodeHashes bool, +) (uint64, error) { + root, err := t.ZkTrie.Tree().Root() + if err != nil { + return 0, fmt.Errorf("failed to retrieve root: %w", err) + } + return t.countLeaves(0, root, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) }
🧹 Nitpick comments (1)
cmd/migration-checker/main.go (1)
248-285: Implement error propagation inloadZkTrieconcurrency.
Lines 254-281 spawn multiple goroutines but rely onpanicfor any critical errors (e.g., missing preimages). This approach prevents other parts of the system from handling or reporting errors gracefully. A dedicated error channel would give more control over concurrency flows.
📜 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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
cmd/migration-checker/main.go (1)
88-92: Replace or complementpanicOnErrorwith error returns to improve reliability.
Panic-based flows abruptly halt execution, making it harder for callers to handle errors gracefully. It is often more maintainable to return an error so that upstream logic can decide how best to recover or log.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
cmd/migration-checker/main.go (4)
88-92: 🛠️ 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.
-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 }
176-187: 🛠️ Refactor suggestionImprove goroutine error handling in account trie checking
The current error handling in the goroutine only prints the panic and exits the process. This is a drastic measure that prevents proper error reporting and recovery.
zkRoot := common.BytesToHash(zkAccount.Root[:]) mptRoot := common.BytesToHash(mptAccount.Root[:]) + errCh := make(chan error, 1) go func() { defer func() { if p := recover(); p != nil { - fmt.Println(p) - os.Exit(1) + var err error + if e, ok := p.(error); ok { + err = fmt.Errorf("%s: %w", label, e) + } else { + err = fmt.Errorf("%s: %v", label, p) + } + errCh <- err + return } + errCh <- nil }() checkTrieEquality(zkRoot, mptRoot, label, checkStorageEquality, false, paranoid) accountsDone.Add(1) fmt.Println("Accounts done:", accountsDone.Load()) }() + // Wait for goroutine to complete or panic + if err := <-errCh; err != nil { + fmt.Fprintf(os.Stderr, "Fatal error: %v\n", err) + os.Exit(1) // Still exit, but with better error reporting + }
204-246: 🛠️ Refactor suggestionFix 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.
for trieIt.Next() { mptLeafMutex.Lock() + 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) + } mptLeafMutex.Unlock() + + if shouldBreak { + break + }
36-86: 🛠️ Refactor suggestionAdd validation for required command-line flags
The code should validate that required flags are provided before proceeding. Without validation, the program may panic later with cryptic errors if any required flag is missing.
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") paranoid = flag.Bool("paranoid", false, "verifies all node contents against their expected hash") parallelismMultipler = flag.Int("parallelism-multiplier", 4, "multiplier for the number of parallel workers") ) flag.Parse() + + // Validate required flags + if *mptDbPath == "" || *zkDbPath == "" || *mptRoot == "" || *zkRoot == "" { + fmt.Println("Error: required flags -mpt-db, -zk-db, -mpt-root, and -zk-root must be provided") + flag.Usage() + os.Exit(1) + }
🧹 Nitpick comments (3)
cmd/migration-checker/main.go (3)
47-49: Improve HTTP server error handlingThe HTTP server is started in a goroutine, but any errors it encounters are only logged and not handled properly. This could lead to silent failures.
go func() { - log.Println(http.ListenAndServe("0.0.0.0:6060", nil)) + err := http.ListenAndServe("0.0.0.0:6060", nil) + if err != nil { + log.Printf("HTTP server error: %v", err) + } }()
97-151: Consider refactoringcheckTrieEqualityfor better maintainabilityThe function is quite long and does multiple things: creating tries, logging progress, comparing data, and handling key formatting. Consider splitting it into smaller, more focused functions for better maintainability.
For example, key handling logic could be extracted:
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 }
57-65: Extract database creation into a helper functionThe database opening code is repeated for both zkDb and mptDb with identical options. Extract this into a helper function to reduce duplication.
+func createReadOnlyDb(path, name string) (*leveldb.Database, error) { + return leveldb.NewCustom(path, name, func(options *opt.Options) { + // Set default options + options.ReadOnly = true + options.BlockCacher = opt.NoCacher + options.BlockCacheCapacity = 0 + options.OpenFilesCacheCapacity = 128 + options.DisableBlockCache = true + }) +} // In main() - zkDb, err := leveldb.NewCustom(*zkDbPath, "zkDb", func(options *opt.Options) { - // Set default options - options.ReadOnly = true - options.BlockCacher = opt.NoCacher - options.BlockCacheCapacity = 0 - options.OpenFilesCacheCapacity = 128 - options.DisableBlockCache = true - }) + zkDb, err := createReadOnlyDb(*zkDbPath, "zkDb") panicOnError(err, "", "failed to open zk db") - mptDb, err := leveldb.NewCustom(*mptDbPath, "mptDb", func(options *opt.Options) { - // Set default options - options.ReadOnly = true - options.BlockCacher = opt.NoCacher - options.BlockCacheCapacity = 0 - options.OpenFilesCacheCapacity = 128 - options.DisableBlockCache = true - }) + mptDb, err := createReadOnlyDb(*mptDbPath, "mptDb") panicOnError(err, "", "failed to open mpt db")Also applies to: 66-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/migration-checker/main.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
cmd/migration-checker/main.go (1)
trie/secure_trie.go (1) (1)
NewSecureNoTracer(68:76)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
omerfirmak
force-pushed
the
omerfirmak/checker-cmd
branch
2 times, most recently
from
07c7893 to
22218a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
trie/zk_trie.go (1)
241-248: 🛠️ Refactor suggestionConsider returning an error instead of panicking in
CountLeaves.
Panic-based error handling can abruptly terminate the application. By returning an error, the caller can decide how to react, improving robustness and maintainability.func (t *ZkTrie) CountLeaves(cb func(key, value []byte), spawnWorker func(func()), parallelismCutoffDepth int, verifyNodeHashes bool) (uint64, error) { root, err := t.ZkTrie.Tree().Root() if err != nil { - panic("CountLeaves cannot get root") + return 0, fmt.Errorf("failed to retrieve trie root: %w", err) } - return t.countLeaves(0, root, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + count := t.countLeaves(0, root, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + return count, nil }
🧹 Nitpick comments (4)
cmd/migration-checker/main.go (4)
35-72: Validate flags and handle DB closure inmain().
Consider validating that all required flags (*mptDbPath,*zkDbPath, etc.) are set before proceeding, and close databases properly before exiting (e.g., usingdefer zkDb.Close()anddefer mptDb.Close()) to release resources.
83-142: Refine concurrency logging incheckTrieEquality.
While the minute-based logging helps track progress, consider a context or cancellation mechanism to stop the ticker when the operation completes. This prevents stray goroutines waiting on timeouts.
187-195: Validate storage data structure incheckStorageEquality.
The logic effectively checks hash mismatches. Consider more descriptive error messages or additional checks if partial data might be encountered.
238-275: Handle potential deadlocks inloadZkTrie.
The 5-minute timeout with a direct call tof()can cause partial concurrency flows iftrieLoadersis unavailable. Consider a more robust approach (e.g., returning an error, or using a default case in aselectto attempt fallback) for consistent concurrency control.
📜 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
cmd/migration-checker/main.go (1)
80-82: Functionduplooks good.
This function correctly returns a copy of the incoming byte slice, preventing unintended modifications.
trie/zk_trie.go
Outdated
| func (t *ZkTrie) countLeaves(depth int, root *zkt.Hash, cb func(key, value []byte), spawnWorker func(func()), parallelismCutoffDepth int, verifyNodeHashes bool) uint64 { | ||
| if root == nil { | ||
| return 0 | ||
| } | ||
|
|
||
| rootNode, err := t.ZkTrie.Tree().GetNode(root) | ||
| if err != nil { | ||
| panic("countLeaves cannot get rootNode") | ||
| } | ||
|
|
||
| if rootNode.Type == zktrie.NodeTypeLeaf_New { | ||
| if verifyNodeHashes { | ||
| calculatedNodeHash, err := rootNode.NodeHash() | ||
| if err != nil { | ||
| panic("countLeaves cannot get calculatedNodeHash") | ||
| } | ||
| if *calculatedNodeHash != *root { | ||
| panic("countLeaves node hash mismatch") | ||
| } | ||
| } | ||
|
|
||
| cb(append([]byte{}, rootNode.NodeKey.Bytes()...), append([]byte{}, rootNode.Data()...)) | ||
| return 1 | ||
| } else { | ||
| if depth < parallelismCutoffDepth { | ||
| count := make(chan uint64, 1) | ||
| leftT := t.Copy() | ||
| spawnWorker(func() { | ||
| count <- leftT.countLeaves(depth+1, rootNode.ChildL, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) | ||
| }) | ||
| return t.countLeaves(depth+1, rootNode.ChildR, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + <-count | ||
| } else { | ||
| return t.countLeaves(depth+1, rootNode.ChildL, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + | ||
| t.countLeaves(depth+1, rootNode.ChildR, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Propagate errors and refine concurrency logic.
In countLeaves, multiple panic calls prevent graceful error handling and can lead to partial data if any error arises. Replacing these panic statements with returned errors would allow the caller to handle failures. Also, consider collecting errors from spawned workers to ensure consistent concurrency behavior.
-func (t *ZkTrie) countLeaves(depth int, root *zkt.Hash, cb func(key, value []byte), spawnWorker func(func()), parallelismCutoffDepth int, verifyNodeHashes bool) uint64 {
+func (t *ZkTrie) countLeaves(depth int, root *zkt.Hash, cb func(key, value []byte), spawnWorker func(func()), parallelismCutoffDepth int, verifyNodeHashes bool) (uint64, error) {
if root == nil {
- return 0
+ return 0, nil
}
rootNode, err := t.ZkTrie.Tree().GetNode(root)
if err != nil {
- panic("countLeaves cannot get rootNode")
+ return 0, fmt.Errorf("failed to get node from the trie: %w", err)
}
if rootNode.Type == zktrie.NodeTypeLeaf_New {
if verifyNodeHashes {
calculatedNodeHash, err := rootNode.NodeHash()
if err != nil {
- panic("countLeaves cannot get calculatedNodeHash")
+ return 0, fmt.Errorf("failed to calculate node hash: %w", err)
}
if *calculatedNodeHash != *root {
- panic("countLeaves node hash mismatch")
+ return 0, fmt.Errorf("node hash mismatch")
}
}
cb(append([]byte{}, rootNode.NodeKey.Bytes()...), append([]byte{}, rootNode.Data()...))
- return 1
+ return 1, nil
} else {
if depth < parallelismCutoffDepth {
countCh := make(chan uint64, 1)
+ errCh := make(chan error, 1)
leftT := t.Copy()
spawnWorker(func() {
- countCh <- leftT.countLeaves(depth+1, rootNode.ChildL, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes)
+ c, err := leftT.countLeaves(depth+1, rootNode.ChildL, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes)
+ if err != nil {
+ errCh <- err
+ return
+ }
+ countCh <- c
+ errCh <- nil
})
- return t.countLeaves(depth+1, rootNode.ChildR, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) + <-countCh
+ rightCount, rightErr := t.countLeaves(depth+1, rootNode.ChildR, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes)
+ if rightErr != nil {
+ return 0, rightErr
+ }
+ leftCount := <-countCh
+ if leftErr := <-errCh; leftErr != nil {
+ return 0, leftErr
+ }
+ return leftCount + rightCount, nil
} else {
- return t.countLeaves(depth+1, rootNode.ChildL, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes) +
- t.countLeaves(depth+1, rootNode.ChildR, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes)
+ leftCount, err := t.countLeaves(depth+1, rootNode.ChildL, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes)
+ if err != nil {
+ return 0, err
+ }
+ rightCount, err := t.countLeaves(depth+1, rootNode.ChildR, cb, spawnWorker, parallelismCutoffDepth, verifyNodeHashes)
+ if err != nil {
+ return 0, err
+ }
+ return leftCount + rightCount, nil
}
}
}Committable suggestion skipped: line range outside the PR's diff.
| func panicOnError(err error, label, msg string) { | ||
| if err != nil { | ||
| panic(fmt.Sprint(label, " error: ", msg, " ", err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer returning errors instead of calling panicOnError.
This helper function can abruptly terminate the program. For production tools or libraries, returning errors often leads to more flexible handling.
cmd/migration-checker/main.go
Outdated
| func loadMPT(mptTrie *trie.SecureTrie, top bool) chan map[string][]byte { | ||
| startKey := make([]byte, 32) | ||
| mptLeafMap := make(map[string][]byte, 1000) | ||
| var mptLeafMutex sync.Mutex | ||
|
|
||
| var mptWg sync.WaitGroup | ||
| for i := 0; i < 255; i++ { | ||
| startKey[0] = byte(i) | ||
| trieIt := trie.NewIterator(mptTrie.NodeIterator(startKey)) | ||
|
|
||
| mptWg.Add(1) | ||
| <-trieLoaders | ||
| go func() { | ||
| defer func() { | ||
| mptWg.Done() | ||
| trieLoaders <- struct{}{} | ||
| }() | ||
| for trieIt.Next() { | ||
| mptLeafMutex.Lock() | ||
| if _, ok := mptLeafMap[string(trieIt.Key)]; ok { | ||
| mptLeafMutex.Unlock() | ||
| break | ||
| } | ||
| mptLeafMap[string(dup(trieIt.Key))] = dup(trieIt.Value) | ||
| mptLeafMutex.Unlock() | ||
|
|
||
| if top && len(mptLeafMap)%10000 == 0 { | ||
| fmt.Println("MPT Accounts Loaded:", len(mptLeafMap)) | ||
| } | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| respChan := make(chan map[string][]byte) | ||
| go func() { | ||
| mptWg.Wait() | ||
| respChan <- mptLeafMap | ||
| }() | ||
| return respChan | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure partial iteration break won't lead to inconsistent state in loadMPT.
When a duplicate key is found, the loop breaks out of one goroutine, but other goroutines may still be iterating. Consider adding coordination (e.g., a shared flag or context cancellation) so other goroutines do not continue reading partial data.
omerfirmak
force-pushed
the
omerfirmak/checker-cmd
branch
from
22218a8 to
6ed543b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
cmd/migration-checker/main.go (3)
30-77: Validate required flags before proceeding.If any required flag (e.g.,
--mpt-db,--zk-db, etc.) is missing, consider printing usage and exiting more cleanly. This will help avoid unexpected panics down the line.flag.Parse() +if *mptDbPath == "" || *zkDbPath == "" || *mptRoot == "" || *zkRoot == "" { + flag.Usage() + os.Exit(1) +}
192-200: Refine error handling for storage mismatch.Panicking on storage mismatch can be replaced with structured error returns, allowing advanced error recovery or logging.
248-272: Allow configurable error handling for missing preimages.Panic at line 256 abruptly ends the process, which might be undesirable in some use cases. Providing an error to the caller could enable more flexible handling or logging.
📜 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
🧬 Code Definitions (2)
trie/zk_trie.go (10)
trie/zktrie_deletionproof.go (5) (5)
t(21-29)t(32-52)t(60-103)t(106-121)t(125-168)trie/secure_trie.go (11) (11)
t(80-86)t(91-93)t(97-99)t(103-114)t(122-126)t(136-144)t(147-151)t(155-159)t(163-171)t(178-192)t(196-198)core/state/state_test.go (1) (1)
value(104-104)core/state/state_object.go (3) (3)
value(252-252)err(165-165)err(207-207)trie/trie_test.go (1) (1)
root(652-652)core/state/state_prove.go (2) (2)
err(63-63)err(80-80)core/blockchain.go (2) (2)
err(270-270)err(813-813)trie/proof.go (2) (2)
err(59-59)err(166-166)core/state/snapshot/snapshot.go (1) (1)
Tree(160-169)trie/zk_trie_test.go (1) (1)
count(237-237)
cmd/migration-checker/main.go (2)
trie/zk_trie.go (2) (2)
NewZkTrie(53-59)ZkTrie(35-38)trie/secure_trie.go (2) (2)
NewSecureNoTracer(68-76)SecureTrie(38-44)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
trie/zk_trie.go (1)
250-294: Replace panics with proper error handling.This function spawns goroutines for parallel leaf counting, but any failure triggers a panic without graceful recovery. A robust approach is to return an error and combine the results in the parent function.
cmd/migration-checker/main.go (2)
79-83: Avoid panics in library-like helper functions.
panicOnErrormay abruptly terminate. Consider returning an error or using structured logging to allow the caller to decide how to handle the failure.
85-87: LGTM for byte slice duplication.This helper function properly returns a copy without issues.
|
|
||
| func (t *ZkTrie) CountLeaves(cb func(key, value []byte), top, verifyNodeHashes bool) uint64 { | ||
| root, err := t.ZkTrie.Tree().Root() | ||
| if err != nil { | ||
| panic("CountLeaves cannot get root") | ||
| } | ||
| return t.countLeaves(root, cb, 0, top, verifyNodeHashes) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning an error instead of panicking.
Relying on panic("CountLeaves cannot get root") makes it hard for callers to handle errors gracefully. Converting this to an error return would allow the calling code to recover or log the failure as needed.
-func (t *ZkTrie) CountLeaves(cb func(key, value []byte), top, verifyNodeHashes bool) uint64 {
+// CountLeaves counts the number of leaf nodes in the trie
+// and returns the total count or an error if it fails
+func (t *ZkTrie) CountLeaves(cb func(key, value []byte), top, verifyNodeHashes bool) (uint64, error) {
root, err := t.ZkTrie.Tree().Root()
if err != nil {
- panic("CountLeaves cannot get root")
+ return 0, fmt.Errorf("CountLeaves failed to retrieve root: %w", err)
}
- return t.countLeaves(root, cb, 0, top, verifyNodeHashes)
+ return t.countLeaves(root, cb, 0, top, verifyNodeHashes)
}📝 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 (t *ZkTrie) CountLeaves(cb func(key, value []byte), top, verifyNodeHashes bool) uint64 { | |
| root, err := t.ZkTrie.Tree().Root() | |
| if err != nil { | |
| panic("CountLeaves cannot get root") | |
| } | |
| return t.countLeaves(root, cb, 0, top, verifyNodeHashes) | |
| } | |
| // CountLeaves counts the number of leaf nodes in the trie | |
| // and returns the total count or an error if it fails | |
| func (t *ZkTrie) CountLeaves(cb func(key, value []byte), top, verifyNodeHashes bool) (uint64, error) { | |
| root, err := t.ZkTrie.Tree().Root() | |
| if err != nil { | |
| return 0, fmt.Errorf("CountLeaves failed to retrieve root: %w", err) | |
| } | |
| return t.countLeaves(root, cb, 0, top, verifyNodeHashes) | |
| } |
cmd/migration-checker/main.go
Outdated
| func checkTrieEquality(dbs *dbs, zkRoot, mptRoot common.Hash, label string, leafChecker func(string, *dbs, []byte, []byte, bool), top, paranoid bool) { | ||
| done := make(chan struct{}) | ||
| start := time.Now() | ||
| if !top { | ||
| go func() { | ||
| for { | ||
| select { | ||
| case <-done: | ||
| return | ||
| case <-time.After(time.Minute): | ||
| fmt.Println("Checking trie", label, "for", time.Since(start)) | ||
| } | ||
| } | ||
| }() | ||
| } | ||
| defer close(done) | ||
|
|
||
| zkTrie, err := trie.NewZkTrie(zkRoot, trie.NewZktrieDatabaseFromTriedb(trie.NewDatabaseWithConfig(dbs.zkDb, &trie.Config{Preimages: true}))) | ||
| panicOnError(err, label, "failed to create zk trie") | ||
| mptTrie, err := trie.NewSecureNoTracer(mptRoot, trie.NewDatabaseWithConfig(dbs.mptDb, &trie.Config{Preimages: true})) | ||
| panicOnError(err, label, "failed to create mpt trie") | ||
|
|
||
| mptLeafCh := loadMPT(mptTrie, top) | ||
| zkLeafCh := loadZkTrie(zkTrie, top, paranoid) | ||
|
|
||
| mptLeafMap := <-mptLeafCh | ||
| zkLeafMap := <-zkLeafCh | ||
|
|
||
| if len(mptLeafMap) != len(zkLeafMap) { | ||
| panic(fmt.Sprintf("%s MPT and ZK trie leaf count mismatch: MPT: %d, ZK: %d", label, len(mptLeafMap), len(zkLeafMap))) | ||
| } | ||
|
|
||
| for preimageKey, zkValue := range zkLeafMap { | ||
| if top { | ||
| // ZkTrie pads preimages with 0s to make them 32 bytes. | ||
| // So we might need to clear those zeroes here since we need 20 byte addresses at top level (ie state trie) | ||
| if len(preimageKey) > 20 { | ||
| for _, b := range []byte(preimageKey)[20:] { | ||
| if b != 0 { | ||
| panic(fmt.Sprintf("%s padded byte is not 0 (preimage %s)", label, hex.EncodeToString([]byte(preimageKey)))) | ||
| } | ||
| } | ||
| preimageKey = preimageKey[:20] | ||
| } | ||
| } else if len(preimageKey) != 32 { | ||
| // storage leafs should have 32 byte keys, pad them if needed | ||
| zeroes := make([]byte, 32) | ||
| copy(zeroes, []byte(preimageKey)) | ||
| preimageKey = string(zeroes) | ||
| } | ||
|
|
||
| mptKey := crypto.Keccak256([]byte(preimageKey)) | ||
| mptVal, ok := mptLeafMap[string(mptKey)] | ||
| if !ok { | ||
| 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.
🛠️ Refactor suggestion
Consider returning an error for leaf mismatch.
Currently, the function panics if the trie leaf counts or data differ. Exiting abruptly may not be ideal for larger tools or services. Providing retries or partial recoveries is often more user-friendly.
cmd/migration-checker/main.go
Outdated
| func loadMPT(mptTrie *trie.SecureTrie, top bool) chan map[string][]byte { | ||
| startKey := make([]byte, 32) | ||
| workers := 1 << 5 | ||
| if !top { | ||
| workers = 1 << 3 | ||
| } | ||
| step := byte(0xFF) / byte(workers) | ||
|
|
||
| mptLeafMap := make(map[string][]byte, 1000) | ||
| var mptLeafMutex sync.Mutex | ||
|
|
||
| var mptWg sync.WaitGroup | ||
| for i := 0; i < workers; i++ { | ||
| startKey[0] = byte(i) * step | ||
| trieIt := trie.NewIterator(mptTrie.NodeIterator(startKey)) | ||
|
|
||
| mptWg.Add(1) | ||
| go func() { | ||
| defer mptWg.Done() | ||
| for trieIt.Next() { | ||
| mptLeafMutex.Lock() | ||
|
|
||
| if _, ok := mptLeafMap[string(trieIt.Key)]; ok { | ||
| mptLeafMutex.Unlock() | ||
| break | ||
| } | ||
|
|
||
| mptLeafMap[string(dup(trieIt.Key))] = dup(trieIt.Value) | ||
|
|
||
| mptLeafMutex.Unlock() | ||
|
|
||
| if top && len(mptLeafMap)%10000 == 0 { | ||
| fmt.Println("MPT Accounts Loaded:", len(mptLeafMap)) | ||
| } | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| respChan := make(chan map[string][]byte) | ||
| go func() { | ||
| mptWg.Wait() | ||
| respChan <- mptLeafMap | ||
| }() | ||
| return respChan | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent partial load after duplicate key detection.
Currently, when one goroutine encounters a duplicate key and breaks, the others continue loading. This can cause partial or inconsistent data. Consider coordinating with a shared flag or context cancellation so all goroutines stop together.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
cmd/migration-checker/main.go (8)
30-39: Validate required command-line flags.The program doesn't validate that required flags (
mpt-db,zk-db,mpt-root,zk-root) are provided before proceeding.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") paranoid = flag.Bool("paranoid", false, "verifies all node contents against their expected hash") parallelismMultipler = flag.Int("parallelism-multiplier", 4, "multiplier for the number of parallel workers") ) flag.Parse() + + if *mptDbPath == "" || *zkDbPath == "" || *mptRoot == "" || *zkRoot == "" { + fmt.Fprintf(os.Stderr, "Error: required flags missing\n") + flag.Usage() + os.Exit(1) + }
57-66: Add HTTP server for profiling.The code mentions stats and periodic reporting, but lacks a proper profiling server. Consider adding one for better monitoring capabilities.
go func() { for { select { case <-done: return case <-time.After(time.Minute): fmt.Println("Active checkers:", totalCheckers-len(trieCheckers)) } } }() defer close(done) + + // Start HTTP server for profiling + go func() { + http.HandleFunc("/progress", func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintf(w, "Accounts processed: %d\nActive checkers: %d\n", + accountsDone.Load(), totalCheckers-len(trieCheckers)) + }) + fmt.Println("Starting profiling server on :6060") + if err := http.ListenAndServe(":6060", nil); err != nil { + fmt.Fprintf(os.Stderr, "Failed to start profiling server: %v\n", err) + } + }()Don't forget to add the required import:
import ( + "net/http" // existing imports )
105-109: Error handling for trie creation is too aggressive.Creating tries shouldn't panic on error. Instead, report the error and exit gracefully.
- zkTrie, err := trie.NewZkTrie(zkRoot, trie.NewZktrieDatabaseFromTriedb(trie.NewDatabaseWithConfig(dbs.zkDb, &trie.Config{Preimages: true}))) - panicOnError(err, label, "failed to create zk trie") - mptTrie, err := trie.NewSecureNoTracer(mptRoot, trie.NewDatabaseWithConfig(dbs.mptDb, &trie.Config{Preimages: true})) - panicOnError(err, label, "failed to create mpt trie") + zkTrie, err := trie.NewZkTrie(zkRoot, trie.NewZktrieDatabaseFromTriedb(trie.NewDatabaseWithConfig(dbs.zkDb, &trie.Config{Preimages: true}))) + if err != nil { + fmt.Fprintf(os.Stderr, "%s error: failed to create zk trie: %v\n", label, err) + os.Exit(1) + } + + mptTrie, err := trie.NewSecureNoTracer(mptRoot, trie.NewDatabaseWithConfig(dbs.mptDb, &trie.Config{Preimages: true})) + if err != nil { + fmt.Fprintf(os.Stderr, "%s error: failed to create mpt trie: %v\n", label, err) + os.Exit(1) + }
116-118: Improve error reporting for leaf count mismatches.Instead of just panicking, provide more detailed diagnostics about the mismatch.
if len(mptLeafs) != len(zkLeafs) { - panic(fmt.Sprintf("%s MPT and ZK trie leaf count mismatch: MPT: %d, ZK: %d", label, len(mptLeafs), len(zkLeafs))) + fmt.Fprintf(os.Stderr, "\n%s MPT and ZK trie leaf count mismatch: MPT: %d, ZK: %d\n", label, len(mptLeafs), len(zkLeafs)) + + // Try to provide more diagnostic information + if len(mptLeafs) > 0 && len(zkLeafs) > 0 { + fmt.Fprintf(os.Stderr, "First few MPT keys: %v\n", formatKeys(mptLeafs[:min(5, len(mptLeafs))])) + fmt.Fprintf(os.Stderr, "First few ZK keys: %v\n", formatKeys(zkLeafs[:min(5, len(zkLeafs))])) + } + + os.Exit(1) }You would need to add these helper functions:
func min(a, b int) int { if a < b { return a } return b } func formatKeys(kvs []kv) []string { keys := make([]string, len(kvs)) for i, kv := range kvs { keys[i] = hex.EncodeToString(kv.key) } return keys }
183-188: Optimize worker count based on trie size.The worker count is hardcoded and doesn't adapt based on trie size. For small tries, this could lead to unnecessary overhead.
func loadMPT(mptTrie *trie.SecureTrie, top bool) chan []kv { startKey := make([]byte, 32) - workers := 1 << 5 + workers := runtime.GOMAXPROCS(0) * 2 // Base worker count on available CPUs if !top { - workers = 1 << 3 + workers = runtime.GOMAXPROCS(0) // Use fewer workers for non-top tries } + // Cap workers to a reasonable maximum + if workers > 32 { + workers = 32 + }
191-193: Prefer a more dynamic initial capacity for mptLeafs.The initial capacity is hardcoded to 1000, which might be too small for large tries and too large for small ones.
- mptLeafs := make([]kv, 0, 1000) + // Estimate an appropriate initial capacity + initialCapacity := 1000 + if stats, err := mptTrie.DB().DiskDB().Stat("state"); err == nil && stats["state/chunks"] > 0 { + // If we have stats, use them to estimate the capacity + initialCapacity = int(stats["state/chunks"]) + } + mptLeafs := make([]kv, 0, initialCapacity)
234-237: Align initial capacity handling in loadZkTrie with loadMPT.For consistency, use the same approach to determine initial capacity in both functions.
func loadZkTrie(zkTrie *trie.ZkTrie, top, paranoid bool) chan []kv { - zkLeafs := make([]kv, 0, 1000) + // Estimate an appropriate initial capacity + initialCapacity := 1000 + if stats, err := zkTrie.Database().DiskDB().Stat("state"); err == nil && stats["state/chunks"] > 0 { + // If we have stats, use them to estimate the capacity + initialCapacity = int(stats["state/chunks"]) + } + zkLeafs := make([]kv, 0, initialCapacity)
258-261: Improve ZkTrie key padding for non-32-byte keys.The current implementation creates a new slice and copies the content. This can be simplified.
- // storage leafs should have 32 byte keys, pad them if needed - zeroes := make([]byte, 32) - copy(zeroes, []byte(preimageKey)) - preimageKey = zeroes + // storage leafs should have 32 byte keys, pad them if needed + if preimageKey != nil { + paddedKey := make([]byte, 32) + copy(paddedKey, preimageKey) + preimageKey = paddedKey + } else { + preimageKey = make([]byte, 32) // Handle nil case + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/migration-checker/main.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
cmd/migration-checker/main.go (2)
trie/zk_trie.go (2) (2)
NewZkTrie(53-59)ZkTrie(35-38)trie/secure_trie.go (2) (2)
NewSecureNoTracer(68-76)SecureTrie(38-44)
🪛 GitHub Check: check
cmd/migration-checker/main.go
[failure] 122-122:
unnecessary conversion (unconvert)
[failure] 249-249:
unnecessary conversion (unconvert)
[failure] 251-251:
unnecessary conversion (unconvert)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
cmd/migration-checker/main.go (3)
41-47: Add graceful error handling for database connections.Instead of panicking on database connection errors, consider returning errors and properly shutting down resources.
- zkDb, err := leveldb.New(*zkDbPath, 1024, 128, "", true) - panicOnError(err, "", "failed to open zk db") - mptDb, err := leveldb.New(*mptDbPath, 1024, 128, "", true) - panicOnError(err, "", "failed to open mpt db") + zkDb, err := leveldb.New(*zkDbPath, 1024, 128, "", true) + if err != nil { + fmt.Fprintf(os.Stderr, "Failed to open zk db: %v\n", err) + os.Exit(1) + } + defer zkDb.Close() + + mptDb, err := leveldb.New(*mptDbPath, 1024, 128, "", true) + if err != nil { + fmt.Fprintf(os.Stderr, "Failed to open mpt db: %v\n", err) + zkDb.Close() + os.Exit(1) + } + defer mptDb.Close()
79-83: Prefer returning errors instead of callingpanicOnError.This helper function abruptly terminates the program. For production tools or libraries, returning errors leads to more flexible handling.
Instead of relying on panics, consider refactoring the code to use proper error handling. This would make the tool more reliable and easier to integrate with other systems.
150-156: Improve goroutine error handling in account trie checking.The current error handling in the goroutine only prints the panic and exits the process. This is a drastic measure that prevents proper error reporting and recovery.
go func() { defer func() { if p := recover(); p != nil { - fmt.Println(p) - os.Exit(1) + var err error + if e, ok := p.(error); ok { + err = fmt.Errorf("%s: %w", label, e) + } else { + err = fmt.Errorf("%s: %v", label, p) + } + fmt.Fprintf(os.Stderr, "Fatal error during storage trie check: %v\n", err) + // Signal other goroutines to terminate gracefully + // Consider using a context with cancel for cleaner termination + os.Exit(1) // If you must exit, provide more context } }()
| panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString([]byte(preimageKey)))) | ||
| } |
There was a problem hiding this comment.
Fix another unnecessary type conversion in error message.
The unnecessary conversion of []byte(preimageKey) in the hex encoding call.
- panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString([]byte(preimageKey))))
+ panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString(preimageKey)))📝 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("padded byte is not 0 (preimage %s)", hex.EncodeToString([]byte(preimageKey)))) | |
| } | |
| panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString(preimageKey))) | |
| } |
🧰 Tools
🪛 GitHub Check: check
[failure] 251-251:
unnecessary conversion (unconvert)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/migration-checker/main.go (2)
31-39:⚠️ Potential issueAdd validation for required command-line flags.
The code should validate that all required flags (
mpt-db,zk-db,mpt-root,zk-root) are provided before proceeding.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") paranoid = flag.Bool("paranoid", false, "verifies all node contents against their expected hash") parallelismMultipler = flag.Int("parallelism-multiplier", 4, "multiplier for the number of parallel workers") ) flag.Parse() + + if *mptDbPath == "" || *zkDbPath == "" || *mptRoot == "" || *zkRoot == "" { + fmt.Fprintf(os.Stderr, "Error: Missing required flags\n") + flag.Usage() + os.Exit(1) + }
149-162: 🛠️ Refactor suggestionImprove goroutine error handling in account trie checking.
The current error handling in the goroutine only prints the panic and exits the process. This is a drastic measure that prevents proper error reporting and recovery.
<-trieCheckers go func() { defer func() { if p := recover(); p != nil { - fmt.Println(p) - os.Exit(1) + var err error + if e, ok := p.(error); ok { + err = fmt.Errorf("%s: %w", label, e) + } else { + err = fmt.Errorf("%s: %v", label, p) + } + fmt.Fprintf(os.Stderr, "Fatal error during storage trie check: %v\n", err) + // Consider using a synchronized error channel to report errors back to the main goroutine + os.Exit(1) // If you must exit, provide more context } }() checkTrieEquality(dbs, zkRoot, mptRoot, label, checkStorageEquality, false, paranoid) accountsDone.Add(1) fmt.Println("Accounts done:", accountsDone.Load()) trieCheckers <- struct{}{} }()
🧹 Nitpick comments (6)
cmd/migration-checker/main.go (6)
79-83: Consider returning errors instead of using panic.Using panic for error handling can make the tool difficult to use as a library or integrate with other systems.
-func panicOnError(err error, label, msg string) { +func panicOnError(err error, label, msg string) { if err != nil { panic(fmt.Sprint(label, " error: ", msg, " ", err)) } }A better approach would be to return errors and handle them gracefully:
func handleError(err error, label, msg string) error { if err != nil { return fmt.Errorf("%s error: %s %w", label, msg, err) } return nil } // Then in your main function: if err := handleError(someOperation(), label, msg); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) }
122-122: Fix unnecessary type conversion.There's an unnecessary conversion of
[]byte(zkKv.key)whenzkKv.keyis already a byte slice.- leafChecker(fmt.Sprintf("%s key: %s", label, hex.EncodeToString([]byte(zkKv.key))), dbs, zkKv.value, mptKv.value, paranoid) + leafChecker(fmt.Sprintf("%s key: %s", label, hex.EncodeToString(zkKv.key)), dbs, zkKv.value, mptKv.value, paranoid)
249-254: Fix unnecessary type conversions in preimage key handling.There are unnecessary conversions of preimageKey to byte slices when it's already a byte slice.
if len(preimageKey) > 20 { - for _, b := range []byte(preimageKey)[20:] { + for _, b := range preimageKey[20:] { if b != 0 { - panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString([]byte(preimageKey)))) + panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString(preimageKey))) } } preimageKey = preimageKey[:20]🧰 Tools
🪛 GitHub Check: check
[failure] 249-249:
unnecessary conversion (unconvert)
[failure] 251-251:
unnecessary conversion (unconvert)
259-259: Fix another unnecessary type conversion.There's an unnecessary conversion of
preimageKeyto byte slice when it's already a byte slice.// storage leafs should have 32 byte keys, pad them if needed zeroes := make([]byte, 32) - copy(zeroes, []byte(preimageKey)) + copy(zeroes, preimageKey) preimageKey = zeroes🧰 Tools
🪛 GitHub Check: check
[failure] 259-259:
unnecessary conversion (unconvert)
183-232: Refactor loadMPT to avoid potential issues with concurrent access.The function is designed to handle partial iteration based on key ranges, but lacks synchronization for cases where multiple goroutines might try to access overlapping key ranges.
Consider adding a context for coordination and better error handling:
-func loadMPT(mptTrie *trie.SecureTrie, top bool) chan []kv { +func loadMPT(ctx context.Context, mptTrie *trie.SecureTrie, top bool) chan []kv { startKey := make([]byte, 32) workers := 1 << 5 if !top { workers = 1 << 3 } step := byte(256 / workers) mptLeafs := make([]kv, 0, 1000) var mptLeafMutex sync.Mutex + errCh := make(chan error, workers) var mptWg sync.WaitGroup for i := 0; i < workers; i++ { startKey[0] = byte(i) * step trieIt := trie.NewIterator(mptTrie.NodeIterator(startKey)) stopKey := (i + 1) * int(step) mptWg.Add(1) go func() { defer mptWg.Done() + defer func() { + if r := recover(); r != nil { + if err, ok := r.(error); ok { + errCh <- fmt.Errorf("panic in loadMPT worker: %w", err) + } else { + errCh <- fmt.Errorf("panic in loadMPT worker: %v", r) + } + } + }() for trieIt.Next() { + select { + case <-ctx.Done(): + return + default: + } if int(trieIt.Key[0]) >= stopKey { break } preimageKey := mptTrie.GetKey(trieIt.Key) if len(preimageKey) == 0 { - panic(fmt.Sprintf("preimage not found mpt trie %s", hex.EncodeToString(trieIt.Key))) + errCh <- fmt.Errorf("preimage not found mpt trie %s", hex.EncodeToString(trieIt.Key)) + return } mptLeafMutex.Lock() mptLeafs = append(mptLeafs, kv{key: preimageKey, value: dup(trieIt.Value)}) mptLeafMutex.Unlock() if top && len(mptLeafs)%10000 == 0 { fmt.Println("MPT Accounts Loaded:", len(mptLeafs)) } } }() } respChan := make(chan []kv) go func() { + // Create a channel to signal cancellation to other goroutines if an error occurs + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + // Check for errors from workers + go func() { + select { + case err := <-errCh: + fmt.Fprintf(os.Stderr, "Error loading MPT: %v\n", err) + cancel() + os.Exit(1) + case <-ctx.Done(): + return + } + }() + mptWg.Wait() sort.Slice(mptLeafs, func(i, j int) bool { return bytes.Compare(mptLeafs[i].key, mptLeafs[j].key) < 0 }) respChan <- mptLeafs }() return respChan }Apply similar changes to
loadZkTrieand update function calls accordingly.
1-20: Add HTTP server for pprof profiling.Since this tool deals with large data structures, adding profiling would help debug performance issues.
package main import ( "bytes" "encoding/hex" "flag" "fmt" + "net/http" + _ "net/http/pprof" "os" "runtime" "sort" "sync" "sync/atomic" "time" "github.com/scroll-tech/go-ethereum/common" "github.com/scroll-tech/go-ethereum/core/types" "github.com/scroll-tech/go-ethereum/ethdb/leveldb" "github.com/scroll-tech/go-ethereum/rlp" "github.com/scroll-tech/go-ethereum/trie" )Then add the HTTP server initialization in the main function:
func main() { // ... existing code ... // Start pprof server pprofPort := flag.String("pprof-port", "6060", "Port for pprof HTTP server") flag.Parse() go func() { fmt.Printf("Starting pprof server on :%s\n", *pprofPort) if err := http.ListenAndServe(":"+*pprofPort, nil); err != nil { fmt.Printf("Failed to start pprof server: %v\n", err) } }() // ... rest of the main function ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/migration-checker/main.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
cmd/migration-checker/main.go (2)
trie/zk_trie.go (2) (2)
NewZkTrie(53-59)ZkTrie(35-38)trie/secure_trie.go (2) (2)
NewSecureNoTracer(68-76)SecureTrie(38-44)
🪛 GitHub Check: check
cmd/migration-checker/main.go
[failure] 249-249:
unnecessary conversion (unconvert)
[failure] 251-251:
unnecessary conversion (unconvert)
[failure] 259-259:
unnecessary conversion (unconvert)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
omerfirmak
force-pushed
the
omerfirmak/checker-cmd
branch
from
b088a58 to
08a049b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/migration-checker/main.go (1)
186-235: 🛠️ Refactor suggestionPrevent potential race condition when breaking.
When a duplicate key is found, the loop breaks but other goroutines might still be iterating, which could lead to inconsistent state. Consider adding coordination to ensure all goroutines stop together.
Add a shared flag or context cancellation to coordinate between goroutines:
func loadMPT(mptTrie *trie.SecureTrie, top bool) chan []kv { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() // Ensure context is cancelled when function returns // Rest of the function remains the same for i := 0; i < workers; i++ { mptWg.Add(1) go func() { defer mptWg.Done() for trieIt.Next() { + select { + case <-ctx.Done(): + // Context was cancelled, stop processing + return + default: + // Continue processing + } if int(trieIt.Key[0]) >= stopKey { break } // Rest of the goroutine logic remains the same // If duplicate found, cancel context to signal other goroutines + if duplicateFound { + cancel() + break + } } }() }
🧹 Nitpick comments (8)
cmd/migration-checker/main.go (8)
42-45: Add defer statements to close database connections.The database connections are opened but never properly closed, which could lead to resource leaks. Add defer statements after opening the databases to ensure they're properly closed when the program exits.
zkDb, err := leveldb.New(*zkDbPath, 1024, 128, "", true) panicOnError(err, "", "failed to open zk db") + defer zkDb.Close() mptDb, err := leveldb.New(*mptDbPath, 1024, 128, "", true) panicOnError(err, "", "failed to open mpt db") + defer mptDb.Close()
80-84: Consider returning errors instead of panicking.The
panicOnErrorfunction causes the program to terminate abruptly, which may not be ideal for a tool that could be used as part of larger workflows. Consider modifying the error handling approach to return errors and allow the caller to decide how to handle them.-func panicOnError(err error, label, msg string) { +func checkError(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 }This would require updating all calling code to handle returned errors appropriately.
125-125: Fix unnecessary type conversion.There's an unnecessary conversion of
[]byte(zkKv.key)whenzkKv.keyis already a byte slice.- leafChecker(fmt.Sprintf("%s key: %s", label, hex.EncodeToString([]byte(zkKv.key))), dbs, zkKv.value, mptKv.value, paranoid) + leafChecker(fmt.Sprintf("%s key: %s", label, hex.EncodeToString(zkKv.key)), dbs, zkKv.value, mptKv.value, paranoid)🧰 Tools
🪛 GitHub Check: check
[failure] 125-125:
unnecessary conversion (unconvert)
153-165: Improve error handling in goroutine.The current error handling in the goroutine only prints the panic and exits the process. This is a drastic measure that prevents proper error reporting and recovery.
go func() { defer func() { if p := recover(); p != nil { - fmt.Println(p) - os.Exit(1) + var err error + if e, ok := p.(error); ok { + err = fmt.Errorf("%s: %w", label, e) + } else { + err = fmt.Errorf("%s: %v", label, p) + } + fmt.Fprintf(os.Stderr, "Fatal error during storage trie check: %v\n", err) + // Consider using a context with cancel for cleaner termination + os.Exit(1) // If you must exit, provide more context } }() checkTrieEquality(dbs, zkRoot, mptRoot, label, checkStorageEquality, false, paranoid) accountsDone.Add(1) fmt.Println("Accounts done:", accountsDone.Load(), "/", totalAccounts) trieCheckers <- struct{}{} }()
252-257: Fix unnecessary type conversions in preimage key handling.There's an unnecessary conversion of
[]byte(preimageKey)when iterating over bytes.- for _, b := range []byte(preimageKey)[20:] { + for _, b := range preimageKey[20:] {Also on line 254:
- panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString([]byte(preimageKey)))) + panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString(preimageKey)))🧰 Tools
🪛 GitHub Check: check
[failure] 252-252:
unnecessary conversion (unconvert)
[failure] 254-254:
unnecessary conversion (unconvert)
31-40: Add validation for required command-line flags.The code should validate that required flags are provided before proceeding to avoid potential issues when accessing 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") paranoid = flag.Bool("paranoid", false, "verifies all node contents against their expected hash") parallelismMultipler = flag.Int("parallelism-multiplier", 4, "multiplier for the number of parallel workers") ) flag.Parse() + + if *mptDbPath == "" || *zkDbPath == "" || *mptRoot == "" || *zkRoot == "" { + flag.Usage() + os.Exit(1) + }
89-127: Consider adding a HTTP pprof endpoint for profiling.Given the CPU and memory-intensive nature of trie comparison, it would be beneficial to add a HTTP pprof endpoint to diagnose performance issues during execution.
Consider adding this near the beginning of the
checkTrieEqualityfunction or in themainfunction:import ( "net/http" _ "net/http/pprof" ) // Add near the beginning of main(): go func() { fmt.Println("Starting pprof server on :6060") http.ListenAndServe("localhost:6060", nil) }()This will allow you to profile the application using standard Go tools:
- CPU profile: http://localhost:6060/debug/pprof/profile
- Memory profile: http://localhost:6060/debug/pprof/heap
- Goroutine blocking profile: http://localhost:6060/debug/pprof/block
🧰 Tools
🪛 GitHub Check: check
[failure] 125-125:
unnecessary conversion (unconvert)
117-121: Provide more detailed error message for trie leaf count mismatch.When there's a mismatch in leaf counts, it would be helpful to have more context about the specific keys that exist in one trie but not the other.
if len(mptLeafs) != len(zkLeafs) { - panic(fmt.Sprintf("%s MPT and ZK trie leaf count mismatch: MPT: %d, ZK: %d", label, len(mptLeafs), len(zkLeafs))) + // Create maps to find unique keys + mptKeys := make(map[string]struct{}, len(mptLeafs)) + zkKeys := make(map[string]struct{}, len(zkLeafs)) + + for _, kv := range mptLeafs { + mptKeys[hex.EncodeToString(kv.key)] = struct{}{} + } + for _, kv := range zkLeafs { + zkKeys[hex.EncodeToString(kv.key)] = struct{}{} + } + + // Find keys in MPT but not in ZK + var mptOnly []string + for k := range mptKeys { + if _, exists := zkKeys[k]; !exists { + mptOnly = append(mptOnly, k) + } + } + + // Find keys in ZK but not in MPT + var zkOnly []string + for k := range zkKeys { + if _, exists := mptKeys[k]; !exists { + zkOnly = append(zkOnly, k) + } + } + + // Limit the number of keys to show in the error message + const maxKeys = 5 + if len(mptOnly) > maxKeys { + mptOnly = mptOnly[:maxKeys] + } + if len(zkOnly) > maxKeys { + zkOnly = zkOnly[:maxKeys] + } + + panic(fmt.Sprintf("%s MPT and ZK trie leaf count mismatch: MPT: %d, ZK: %d\nKeys in MPT only: %v\nKeys in ZK only: %v", + label, len(mptLeafs), len(zkLeafs), mptOnly, zkOnly)) } else if top { totalAccounts = len(mptLeafs) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/migration-checker/main.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: check
cmd/migration-checker/main.go
[failure] 125-125:
unnecessary conversion (unconvert)
[failure] 252-252:
unnecessary conversion (unconvert)
[failure] 254-254:
unnecessary conversion (unconvert)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
cmd/migration-checker/main.go (4)
43-46: 🛠️ Refactor suggestionAdd resource cleanup for database connections.
The databases are opened but never properly closed, which could lead to resource leaks.
zkDb, err := leveldb.New(*zkDbPath, 1024, 128, "", true) panicOnError(err, "", "failed to open zk db") mptDb, err := leveldb.New(*mptDbPath, 1024, 128, "", true) panicOnError(err, "", "failed to open mpt db") + defer zkDb.Close() + defer mptDb.Close()
41-41: 🛠️ Refactor suggestionAdd validation for required command-line flags.
The code should validate that required flags are provided before proceeding.
flag.Parse() + + if *mptDbPath == "" || *zkDbPath == "" || *mptRoot == "" || *zkRoot == "" { + flag.Usage() + os.Exit(1) + }
155-167: 🛠️ Refactor suggestionImprove concurrency error handling.
When comparing account data, the code spawns a goroutine and calls
os.Exit(1)on panic. Consider returning errors or using an error channel to handle partial failures without terminating the entire process.go func() { defer func() { if p := recover(); p != nil { - fmt.Println(p) - os.Exit(1) + var err error + if e, ok := p.(error); ok { + err = fmt.Errorf("%s: %w", label, e) + } else { + err = fmt.Errorf("%s: %v", label, p) + } + fmt.Fprintf(os.Stderr, "Fatal error during storage trie check: %v\n", err) + // Consider using a context with cancel for cleaner termination + os.Exit(1) // If you must exit, provide more context } }() checkTrieEquality(dbs, zkRoot, mptRoot, label, checkStorageEquality, false, paranoid, 0) accountsDone.Add(1) fmt.Println("Accounts done:", accountsDone.Load(), "/", totalAccounts) trieCheckers <- struct{}{} }()
201-225: 🛠️ Refactor suggestionFix potential race condition in break condition.
The break condition in the MPT loading logic could lead to inconsistent state if multiple goroutines are processing concurrently.
mptWg.Add(1) go func() { defer mptWg.Done() for trieIt.Next() { if int(trieIt.Key[0]) >= stopKey { break } preimageKey := mptTrie.GetKey(trieIt.Key) if len(preimageKey) == 0 { panic(fmt.Sprintf("preimage not found mpt trie %s", hex.EncodeToString(trieIt.Key))) } + var shouldAdd bool mptLeafMutex.Lock() - mptLeafs = append(mptLeafs, kv{key: preimageKey, value: dup(trieIt.Value)}) + shouldAdd = true + if shouldAdd { + mptLeafs = append(mptLeafs, kv{key: preimageKey, value: dup(trieIt.Value)}) + } mptLeafMutex.Unlock() + if top && len(mptLeafs)%10000 == 0 { fmt.Println("MPT Accounts Loaded:", len(mptLeafs)) } } }()
🧹 Nitpick comments (4)
cmd/migration-checker/main.go (4)
39-39: Fix typo in flag name.There's a typo in the flag name:
start-formshould bestart-fromto correctly reflect its purpose.- startFrom = flag.Int("start-form", 0, "start checking from account at the given index") + startFrom = flag.Int("start-from", 0, "start checking from account at the given index")
250-266: Refactor key normalization into a separate function.The key normalization logic is complex and would be more maintainable as a separate function.
+func normalizePreimageKey(preimageKey []byte, top bool) []byte { + if top { + // ZkTrie pads preimages with 0s to make them 32 bytes. + // So we might need to clear those zeroes here since we need 20 byte addresses at top level (ie state trie) + if len(preimageKey) > 20 { + for _, b := range preimageKey[20:] { + if b != 0 { + panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString(preimageKey))) + } + } + return preimageKey[:20] + } + } else if len(preimageKey) != 32 { + // storage leafs should have 32 byte keys, pad them if needed + zeroes := make([]byte, 32) + copy(zeroes, preimageKey) + return zeroes + } + return preimageKey +} // In loadZkTrie preimageKey := zkTrie.GetKey(key) if len(preimageKey) == 0 { panic(fmt.Sprintf("preimage not found zk trie %s", hex.EncodeToString(key))) } - if top { - // ZkTrie pads preimages with 0s to make them 32 bytes. - // So we might need to clear those zeroes here since we need 20 byte addresses at top level (ie state trie) - 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)))) - } - } - preimageKey = preimageKey[:20] - } - } else if len(preimageKey) != 32 { - // storage leafs should have 32 byte keys, pad them if needed - zeroes := make([]byte, 32) - copy(zeroes, []byte(preimageKey)) - preimageKey = zeroes - } + preimageKey = normalizePreimageKey(preimageKey, top)🧰 Tools
🪛 GitHub Check: check
[failure] 254-254:
unnecessary conversion (unconvert)
[failure] 256-256:
unnecessary conversion (unconvert)
265-265: Fix unnecessary type conversion in zeroes copy.There's an unnecessary type conversion when copying preimageKey to zeroes.
- copy(zeroes, []byte(preimageKey)) + copy(zeroes, preimageKey)
31-79: Add HTTP server for profiling.To assist with debugging and performance optimization, consider adding an HTTP server for profiling.
import ( "bytes" "encoding/hex" "flag" "fmt" + "net/http" + _ "net/http/pprof" "os" "runtime" "sort" "sync" "sync/atomic" "time" "github.com/scroll-tech/go-ethereum/common" "github.com/scroll-tech/go-ethereum/core/types" "github.com/scroll-tech/go-ethereum/ethdb/leveldb" "github.com/scroll-tech/go-ethereum/rlp" "github.com/scroll-tech/go-ethereum/trie" ) 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") paranoid = flag.Bool("paranoid", false, "verifies all node contents against their expected hash") parallelismMultipler = flag.Int("parallelism-multiplier", 4, "multiplier for the number of parallel workers") startFrom = flag.Int("start-form", 0, "start checking from account at the given index") + pprofAddr = flag.String("pprof", "", "enable pprof HTTP server on given address (e.g., localhost:6060)") ) flag.Parse() + + if *pprofAddr != "" { + go func() { + fmt.Println("Starting pprof server on", *pprofAddr) + if err := http.ListenAndServe(*pprofAddr, nil); err != nil { + fmt.Println("Error starting pprof server:", err) + } + }() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/migration-checker/main.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
cmd/migration-checker/main.go (2)
trie/zk_trie.go (2) (2)
NewZkTrie(53-59)ZkTrie(35-38)trie/secure_trie.go (2) (2)
NewSecureNoTracer(68-76)SecureTrie(38-44)
🪛 GitHub Check: check
cmd/migration-checker/main.go
[failure] 127-127:
unnecessary conversion (unconvert)
[failure] 254-254:
unnecessary conversion (unconvert)
[failure] 256-256:
unnecessary conversion (unconvert)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
cmd/migration-checker/main.go (5)
81-85: Prefer returning errors instead of callingpanicOnError.This helper function abruptly terminates the program. For production tools or libraries, returning errors instead of panicking allows for more graceful error handling.
-func panicOnError(err error, label, msg string) { - if err != nil { - panic(fmt.Sprint(label, " error: ", msg, " ", err)) - } +func handleError(err error, label, msg string) error { + if err != nil { + return fmt.Errorf("%s error: %s %w", label, msg, err) + } + return nil }
118-122: Consider returning an error for leaf mismatch.Currently, the function panics if the trie leaf counts differ. Returning an error would allow for more flexible handling of this condition.
if len(mptLeafs) != len(zkLeafs) { - panic(fmt.Sprintf("%s MPT and ZK trie leaf count mismatch: MPT: %d, ZK: %d", label, len(mptLeafs), len(zkLeafs))) + return fmt.Errorf("%s MPT and ZK trie leaf count mismatch: MPT: %d, ZK: %d", label, len(mptLeafs), len(zkLeafs)) } else if top { totalAccounts = len(mptLeafs) }
127-127: Fix unnecessary type conversion.There's an unnecessary conversion of
[]byte(zkKv.key)whenzkKv.keyis already a byte slice.- leafChecker(fmt.Sprintf("%s key: %s", label, hex.EncodeToString([]byte(zkKv.key))), dbs, zkKv.value, mptKv.value, paranoid) + leafChecker(fmt.Sprintf("%s key: %s", label, hex.EncodeToString(zkKv.key)), dbs, zkKv.value, mptKv.value, paranoid)🧰 Tools
🪛 GitHub Check: check
[failure] 127-127:
unnecessary conversion (unconvert)
254-254: Fix unnecessary type conversions in preimage key handling.There are unnecessary conversions of byte slices that are already in the correct type.
- for _, b := range []byte(preimageKey)[20:] { + for _, b := range preimageKey[20:] { if b != 0 { - panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString([]byte(preimageKey)))) + panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString(preimageKey)))Also applies to: 256-256
🧰 Tools
🪛 GitHub Check: check
[failure] 254-254:
unnecessary conversion (unconvert)
90-129: Refactor for better modularity and readability.The
checkTrieEqualityfunction is doing too many things: creating tries, loading data, comparing data, and handling preimage keys. Consider splitting it into smaller, more focused functions.+func createTries(dbs *dbs, zkRoot, mptRoot common.Hash, label string) (*trie.ZkTrie, *trie.SecureTrie, error) { + zkTrie, err := trie.NewZkTrie(zkRoot, trie.NewZktrieDatabaseFromTriedb(trie.NewDatabaseWithConfig(dbs.zkDb, &trie.Config{Preimages: true}))) + if err != nil { + return nil, nil, fmt.Errorf("%s failed to create zk trie: %w", label, err) + } + + mptTrie, err := trie.NewSecureNoTracer(mptRoot, trie.NewDatabaseWithConfig(dbs.mptDb, &trie.Config{Preimages: true})) + if err != nil { + return nil, nil, fmt.Errorf("%s failed to create mpt trie: %w", label, err) + } + + return zkTrie, mptTrie, nil +} +func compareLeafCounts(zkLeafs, mptLeafs []kv, label string, top bool) error { + if len(mptLeafs) != len(zkLeafs) { + return fmt.Errorf("%s MPT and ZK trie leaf count mismatch: MPT: %d, ZK: %d", label, len(mptLeafs), len(zkLeafs)) + } else if top { + totalAccounts = len(mptLeafs) + } + return nil +}🧰 Tools
🪛 GitHub Check: check
[failure] 127-127:
unnecessary conversion (unconvert)
omerfirmak
force-pushed
the
omerfirmak/checker-cmd
branch
from
59951b3 to
8e0b29c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/migration-checker/main.go (3)
39-39: Typo in flag name
The flag is declared as"start-form"instead of"start-from". Consider fixing the name to match the usage context.- startFrom = flag.Int("start-form", 0, "start checking from account at the given index") + startFrom = flag.Int("start-from", 0, "start checking from account at the given index")
134-134: Remove unnecessary conversion
zkKv.keyis already a[]byte. You can directly pass it tohex.EncodeToString()without casting.- leafChecker(fmt.Sprintf("%s key: %s", label, hex.EncodeToString([]byte(zkKv.key))), dbs, zkKv.value, mptKv.value, paranoid) + leafChecker(fmt.Sprintf("%s key: %s", label, hex.EncodeToString(zkKv.key)), dbs, zkKv.value, mptKv.value, paranoid)
260-260: Remove unnecessary conversions
There’s no need to castpreimageKeyto[]bytesince it’s already a byte slice:-for _, b := range []byte(preimageKey)[20:] { +for _, b := range preimageKey[20:] { - panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString([]byte(preimageKey)))) + panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString(preimageKey))) - copy(zeroes, []byte(preimageKey)) + copy(zeroes, preimageKey)Also applies to: 262-262, 270-270
🧰 Tools
🪛 GitHub Check: check
[failure] 260-260:
unnecessary conversion (unconvert)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/migration-checker/main.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
cmd/migration-checker/main.go (2)
trie/zk_trie.go (2) (2)
NewZkTrie(53-59)ZkTrie(35-38)trie/secure_trie.go (2) (2)
NewSecureNoTracer(68-76)SecureTrie(38-44)
🪛 GitHub Check: check
cmd/migration-checker/main.go
[failure] 260-260:
unnecessary conversion (unconvert)
[failure] 262-262:
unnecessary conversion (unconvert)
[failure] 270-270:
unnecessary conversion (unconvert)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
cmd/migration-checker/main.go (5)
31-41: Add validation for required flags
All four flags are required for the script to run properly, but there's no code to confirm that these flags are set. This can lead to cryptic errors if the user forgets to provide them.
43-46: Close database connections to prevent resource leaks
You open the databases but never close them. This can cause file locks or resource leaks.// after opening the DBs successfully defer zkDb.Close() defer mptDb.Close()
96-135: Refactor for better modularity and readability
This function handles multiple responsibilities, including setting up the trie, loading leaves, sorting them, and verifying their consistency. Splitting it into smaller helpers can improve clarity and testability.
130-135: Verify partial iteration logic
Starting from an arbitrary index could misalign comparison if either leaf list has changed or doesn’t match in sorted order. Ensure your partial iteration logic won’t skip mismatched keys.
163-165: Refine concurrency error handling
Panicking in the goroutine combined withos.Exit(1)can abruptly terminate the entire process, losing context for debugging. Prefer returning an error or using an error channel for more graceful handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
cmd/migration-checker/main.go (3)
41-41: 🛠️ Refactor suggestionAdd validation for required command-line flags.
The code should validate that required flags are provided before proceeding.
flag.Parse() + + if *mptDbPath == "" || *zkDbPath == "" || *mptRoot == "" || *zkRoot == "" { + flag.Usage() + os.Exit(1) + }
31-71:⚠️ Potential issueAdd proper database resource cleanup.
The databases opened at lines 43-46 are never closed, which could lead to resource leaks. Ensure proper cleanup with
deferstatements after opening the databases.zkDb, err := leveldb.New(*zkDbPath, 1024, 128, "", true) panicOnError(err, "", "failed to open zk db") mptDb, err := leveldb.New(*mptDbPath, 1024, 128, "", true) panicOnError(err, "", "failed to open mpt db") + defer zkDb.Close() + defer mptDb.Close()
147-159: 🛠️ Refactor suggestionImprove error handling in the goroutine.
The current implementation panics and exits the process on any error, which prevents proper cleanup and error reporting. Consider using a more graceful approach.
go func() { defer func() { if p := recover(); p != nil { - fmt.Println(p) - os.Exit(1) + var err error + if e, ok := p.(error); ok { + err = fmt.Errorf("%s: %w", label, e) + } else { + err = fmt.Errorf("%s: %v", label, p) + } + fmt.Fprintf(os.Stderr, "Fatal error during storage trie check: %v\n", err) + // Signal other goroutines to terminate gracefully + // Consider using a context with cancel for cleaner termination + os.Exit(1) // If you must exit, provide more context } }()
🧹 Nitpick comments (5)
cmd/migration-checker/main.go (5)
119-119: Fix unnecessary type conversion at line 119.The expression
[]byte(zkKv.key)is unnecessary aszkKv.keyis already a byte slice.- leafChecker(fmt.Sprintf("%s key: %s", label, hex.EncodeToString([]byte(zkKv.key))), dbs, zkKv.value, mptKv.value, paranoid) + leafChecker(fmt.Sprintf("%s key: %s", label, hex.EncodeToString(zkKv.key)), dbs, zkKv.value, mptKv.value, paranoid)🧰 Tools
🪛 GitHub Check: check
[failure] 119-119:
unnecessary conversion (unconvert)
246-251: Fix unnecessary type conversion in preimage key handling.The expression
[]byte(preimageKey)is unnecessary when iterating over bytes, aspreimageKeyis already a byte slice.- for _, b := range []byte(preimageKey)[20:] { + for _, b := range preimageKey[20:] {🧰 Tools
🪛 GitHub Check: check
[failure] 248-248:
unnecessary conversion (unconvert)
248-248: Fix another unnecessary type conversion in error message.The expression
[]byte(preimageKey)is unnecessary in the hex encoding call.- panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString([]byte(preimageKey)))) + panic(fmt.Sprintf("padded byte is not 0 (preimage %s)", hex.EncodeToString(preimageKey)))🧰 Tools
🪛 GitHub Check: check
[failure] 248-248:
unnecessary conversion (unconvert)
256-256: Fix unnecessary type conversion when copying preimage key.The expression
[]byte(preimageKey)is unnecessary aspreimageKeyis already a byte slice.- copy(zeroes, []byte(preimageKey)) + copy(zeroes, preimageKey)🧰 Tools
🪛 GitHub Check: check
[failure] 256-256:
unnecessary conversion (unconvert)
110-114: Consider a more graceful approach for handling leaf count mismatches.Instead of panicking when leaf counts don't match, consider logging the discrepancy and continuing with what you can compare, or providing more detailed information about the mismatched tries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/migration-checker/main.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: check
cmd/migration-checker/main.go
[failure] 248-248:
unnecessary conversion (unconvert)
[failure] 256-256:
unnecessary conversion (unconvert)
[failure] 119-119:
unnecessary conversion (unconvert)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (go)
- GitHub Check: test
🔇 Additional comments (2)
cmd/migration-checker/main.go (2)
73-77: Consider returning errors instead of callingpanicOnError.Using panic for error handling makes the code less robust and harder to test. For production tools or libraries, returning errors allows for more flexible handling by callers.
231-275: Improve error handling inloadZkTrie.Consider returning errors instead of panicking when preimages are not found. This allows for more flexible handling of incomplete or invalid tries.
🧰 Tools
🪛 GitHub Check: check
[failure] 248-248:
unnecessary conversion (unconvert)
[failure] 256-256:
unnecessary conversion (unconvert)
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