feat: merge dev into main #123
Conversation
fix: NodeIterator return error from empty trie root
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Iterator
participant Trie
User->>Iterator: Initialize with empty trie
Iterator->>Trie: Check for nodes
Trie-->>Iterator: No nodes found
Iterator->>User: Return empty result
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=warning msg="[config_reader] The configuration option Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
trie/iterator_test.go (1)
664-685: Enhance test cases with more specific assertions and descriptive names.While the test cases correctly verify basic functionality, consider the following improvements:
- Use more descriptive test names that indicate the expected behavior
- Add positive assertions instead of relying on t.Fail()
- Add specific error type checking
Here's a suggested improvement:
t.Run("empty root", func(t *testing.T) { - t.Run("zk merkle tree", func(t *testing.T) { + t.Run("zk merkle tree iterator should not yield nodes for empty root", func(t *testing.T) { trie := NewEmptyMerkleTrie(NewZkDatabase(rawdb.NewMemoryDatabase())) it, _ := trie.NodeIterator(nil) + nodeCount := 0 for it.Next(true) { - t.Fail() + nodeCount++ } + if nodeCount != 0 { + t.Errorf("expected no nodes, but got %d nodes", nodeCount) + } if it.Error() != nil { t.Error(it.Error()) } }) - t.Run("zk trie", func(t *testing.T) { + t.Run("zk trie iterator should not yield nodes for empty root", func(t *testing.T) { trie, _ := NewZkTrie(common.Hash{}, NewZkDatabase(rawdb.NewDatabase(memorydb.New()))) it, _ := trie.NodeIterator(nil) + nodeCount := 0 for it.Next(true) { - t.Fail() + nodeCount++ } + if nodeCount != 0 { + t.Errorf("expected no nodes, but got %d nodes", nodeCount) + } if it.Error() != nil { t.Error(it.Error()) } }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
funding.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
trie/iterator.go(1 hunks)trie/iterator_test.go(1 hunks)
🔇 Additional comments (1)
trie/iterator.go (1)
859-859: LGTM! Consider verifying downstream impact.
The change to return zk.EmptyNodeValue.CanonicalValue() instead of nil for empty nodes improves consistency in node handling.
Let's verify if any code relies on nil returns for empty nodes:
✅ Verification successful
Let me gather more specific information about the usage context.
Let me search for more specific information about the EmptyNodeValue implementation.
Based on the gathered information, I can now provide the final response:
Change to return canonical empty node value is correct and safe
The modification to return zk.EmptyNodeValue.CanonicalValue() is a proper implementation as it:
- Returns a standardized byte representation
[]byte{byte(trie.NodeTypeEmpty)}for empty nodes - Aligns with the ZK Merkle tree's empty node handling throughout the codebase
- Is used consistently in other parts of the codebase for empty node representation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for code that might expect nil returns from zkMerkleTreeNodeBlobFunctions
# Look for direct function calls and error handling patterns
# Search for direct usage of the function
rg -A 5 "zkMerkleTreeNodeBlobFunctions"
# Search for nil checks after blob retrieval
rg -A 5 "if\s+blob\s*==\s*nil"
Length of output: 2016
Script:
#!/bin/bash
# Search for the usage of EmptyNodeValue and related code
rg -A 5 "EmptyNodeValue"
# Search for any nil checks specifically in the trie package
rg -A 5 "== nil" trie/
# Look for the implementation of CanonicalValue
ast-grep --pattern 'func ($x *$_) CanonicalValue() []byte'
Length of output: 41935
Script:
#!/bin/bash
# Look for EmptyNode implementation and its CanonicalValue method
rg -A 10 "type EmptyNode struct" trie/
# Search for any tests or code that verifies empty node behavior
rg -A 5 "TestEmpty.*Node" trie/
# Look for any code that handles empty node values
rg -A 5 "EmptyNode.*Value" trie/
Length of output: 4531
| it, _ := trie.NodeIterator(nil) | ||
| for it.Next(true) { |
There was a problem hiding this comment.
Check NodeIterator errors before proceeding with iteration.
The error returned from NodeIterator() should be checked before starting iteration.
Apply this pattern to both test cases:
- it, _ := trie.NodeIterator(nil)
+ it, err := trie.NodeIterator(nil)
+ if err != nil {
+ t.Fatalf("failed to create iterator: %v", err)
+ }
for it.Next(true) {Also applies to: 677-678
Description
This PR is for release
v0.5.4.