fix: Add Extra Check for Reformatted Root Node in GetNode #1007
Conversation
WalkthroughThe pull request introduces a new test function, Changes
Possibly related PRs
Suggested reviewers
Poem
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 (2)
nodedb.go (2)
162-175: Performance impact is minimal and acceptable.The additional database lookup for reformatted nodes is justified:
- It only occurs in specific edge cases
- The nodeCache will help mitigate the performance impact
- The correctness guarantee outweighs the minimal performance cost
Consider monitoring the cache hit rates for these reformatted nodes to ensure optimal performance.
172-172: Consider enhancing the error message for better debugging.The error message could be more specific about the reformatting attempt.
- return nil, fmt.Errorf("can't get the reformatted node %v: %v", nk, err) + return nil, fmt.Errorf("can't get the reformatted node (version=%d, nonce=0) from original (version=%d, nonce=1): %v", nKey.version, nKey.version, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
mutable_tree_test.go(1 hunks)nodedb.go(1 hunks)
🔇 Additional comments (2)
nodedb.go (2)
162-175: LGTM! The implementation correctly handles reformatted root nodes.
The added code successfully addresses the issue with reformatted root nodes by:
- Checking for nil buffer and non-legacy nodes
- Attempting to retrieve nodes with nonce 0 when nonce 1 isn't found
- Providing clear error messages for debugging
This change aligns well with the PR objectives of fixing access to reformatted root nodes.
162-175: Verify edge cases in the implementation.
Let's verify the following aspects:
- Legacy node handling remains unaffected
- Error handling for reformatted nodes
- Version consistency across node references
✅ Verification successful
Let me gather more information to verify the implementation thoroughly.
Based on the gathered information, I can now provide the final response:
Implementation handles edge cases appropriately
The verification confirms:
- Legacy nodes are handled separately through the
isLegcyNodeflag, with no interference from the reformatting logic - Error handling is consistent across node retrievals, with specific error messages for reformatted nodes
- NodeKey structure properly maintains version and nonce fields, ensuring version consistency
The implementation correctly handles the edge case of reformatted nodes (nonce=1) by falling back to the original version (nonce=0) while maintaining proper error handling and version consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation handles edge cases correctly
# Check if there are any legacy nodes that might be affected
echo "Checking for legacy nodes..."
ast-grep --pattern 'isLegacy == true && nonce == 1'
# Check error handling patterns
echo "Checking error handling consistency..."
rg "can't get (the reformatted )?node" -A 2
# Check version handling in node references
echo "Checking version handling..."
ast-grep --pattern 'nodeKey.version = $version'
Length of output: 617
Script:
#!/bin/bash
# Let's check the complete context around node handling and versioning
# Check how NodeKey is used in the codebase
echo "Checking NodeKey usage patterns..."
ast-grep --pattern 'type NodeKey struct {
$$$
}'
# Look for nonce handling patterns
echo "Checking nonce handling..."
ast-grep --pattern 'nonce: $expr'
# Check for version handling in GetNodeKey
echo "Checking GetNodeKey implementation..."
ast-grep --pattern 'func GetNodeKey($$$) {
$$$
}'
# Look for other error cases in node retrieval
echo "Checking node retrieval patterns..."
rg "ndb\.db\.Get\(.*\)" -A 2
Length of output: 1725
| func TestReferenceRootPruning(t *testing.T) { | ||
| memDB := dbm.NewMemDB() | ||
| tree := NewMutableTree(memDB, 0, true, NewNopLogger()) | ||
|
|
||
| _, err := tree.Set([]byte("foo"), []byte("bar")) | ||
| require.NoError(t, err) | ||
| _, _, err = tree.SaveVersion() | ||
| require.NoError(t, err) | ||
|
|
||
| _, _, err = tree.SaveVersion() | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = tree.Set([]byte("foo1"), []byte("bar")) | ||
| require.NoError(t, err) | ||
| _, _, err = tree.SaveVersion() | ||
| require.NoError(t, err) | ||
|
|
||
| err = tree.DeleteVersionsTo(1) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = tree.Set([]byte("foo"), []byte("bar*")) | ||
| require.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and documentation for root node reformatting.
The test needs improvements in the following areas:
-
Add assertions to verify:
- The final state after setting the new value
- The root node reformatting (nodeKey change from (version, 1) to (version, 0))
- References from subsequent versions to the reformatted node
-
Add documentation to explain the test's purpose and steps
Here's the suggested implementation:
func TestReferenceRootPruning(t *testing.T) {
+ // This test verifies that when a root node is reformatted during pruning
+ // (changing its nodeKey from (version, 1) to (version, 0)):
+ // 1. The reformatted node remains accessible
+ // 2. References from subsequent versions are correctly resolved
+
memDB := dbm.NewMemDB()
tree := NewMutableTree(memDB, 0, true, NewNopLogger())
// Set initial key-value pair
_, err := tree.Set([]byte("foo"), []byte("bar"))
require.NoError(t, err)
_, ver1, err := tree.SaveVersion()
require.NoError(t, err)
// Save empty version to create a reference to the root
_, ver2, err := tree.SaveVersion()
require.NoError(t, err)
// Add another key-value pair
_, err = tree.Set([]byte("foo1"), []byte("bar"))
require.NoError(t, err)
_, ver3, err := tree.SaveVersion()
require.NoError(t, err)
// Delete versions up to ver1, triggering root node reformatting
err = tree.DeleteVersionsTo(ver1)
require.NoError(t, err)
// Verify the root node was reformatted
+ rootKey := GetRootKey(ver1)
+ node, err := tree.ndb.GetNode(rootKey)
+ require.NoError(t, err)
+ require.Equal(t, int64(0), node.nodeKey.index, "root node should be reformatted with index 0")
// Set new value and verify it's accessible
_, err = tree.Set([]byte("foo"), []byte("bar*"))
require.NoError(t, err)
+ _, ver4, err := tree.SaveVersion()
+ require.NoError(t, err)
+
+ // Verify the value is correctly set
+ val, err := tree.Get([]byte("foo"))
+ require.NoError(t, err)
+ require.Equal(t, []byte("bar*"), val, "new value should be accessible after root reformatting")
+
+ // Verify references from subsequent versions
+ for _, version := range []int64{ver2, ver3, ver4} {
+ _, err := tree.GetVersioned([]byte("foo"), version)
+ require.NoError(t, err, "value should be accessible in version %d", version)
+ }
}📝 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 TestReferenceRootPruning(t *testing.T) { | |
| memDB := dbm.NewMemDB() | |
| tree := NewMutableTree(memDB, 0, true, NewNopLogger()) | |
| _, err := tree.Set([]byte("foo"), []byte("bar")) | |
| require.NoError(t, err) | |
| _, _, err = tree.SaveVersion() | |
| require.NoError(t, err) | |
| _, _, err = tree.SaveVersion() | |
| require.NoError(t, err) | |
| _, err = tree.Set([]byte("foo1"), []byte("bar")) | |
| require.NoError(t, err) | |
| _, _, err = tree.SaveVersion() | |
| require.NoError(t, err) | |
| err = tree.DeleteVersionsTo(1) | |
| require.NoError(t, err) | |
| _, err = tree.Set([]byte("foo"), []byte("bar*")) | |
| require.NoError(t, err) | |
| } | |
| func TestReferenceRootPruning(t *testing.T) { | |
| // This test verifies that when a root node is reformatted during pruning | |
| // (changing its nodeKey from (version, 1) to (version, 0)): | |
| // 1. The reformatted node remains accessible | |
| // 2. References from subsequent versions are correctly resolved | |
| memDB := dbm.NewMemDB() | |
| tree := NewMutableTree(memDB, 0, true, NewNopLogger()) | |
| // Set initial key-value pair | |
| _, err := tree.Set([]byte("foo"), []byte("bar")) | |
| require.NoError(t, err) | |
| _, ver1, err := tree.SaveVersion() | |
| require.NoError(t, err) | |
| // Save empty version to create a reference to the root | |
| _, ver2, err := tree.SaveVersion() | |
| require.NoError(t, err) | |
| // Add another key-value pair | |
| _, err = tree.Set([]byte("foo1"), []byte("bar")) | |
| require.NoError(t, err) | |
| _, ver3, err := tree.SaveVersion() | |
| require.NoError(t, err) | |
| // Delete versions up to ver1, triggering root node reformatting | |
| err = tree.DeleteVersionsTo(ver1) | |
| require.NoError(t, err) | |
| // Verify the root node was reformatted | |
| rootKey := GetRootKey(ver1) | |
| node, err := tree.ndb.GetNode(rootKey) | |
| require.NoError(t, err) | |
| require.Equal(t, int64(0), node.nodeKey.index, "root node should be reformatted with index 0") | |
| // Set new value and verify it's accessible | |
| _, err = tree.Set([]byte("foo"), []byte("bar*")) | |
| require.NoError(t, err) | |
| _, ver4, err := tree.SaveVersion() | |
| require.NoError(t, err) | |
| // Verify the value is correctly set | |
| val, err := tree.Get([]byte("foo")) | |
| require.NoError(t, err) | |
| require.Equal(t, []byte("bar*"), val, "new value should be accessible after root reformatting") | |
| // Verify references from subsequent versions | |
| for _, version := range []int64{ver2, ver3, ver4} { | |
| _, err := tree.GetVersioned([]byte("foo"), version) | |
| require.NoError(t, err, "value should be accessible in version %d", version) | |
| } | |
| } |
|
do you need to add a changelog? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
7-7: Enhance the changelog entry with more details about the bug and its impact.While the entry follows the correct format, it could be more descriptive about the actual problem being fixed. Consider expanding it to:
- - [#1007](https://github.com/cosmos/iavl/pull/1007) Add the extra check for the reformatted root node in `GetNode` + - [#1007](https://github.com/cosmos/iavl/pull/1007) Fix root node accessibility after reformatting by adding an extra check in `GetNode`. Previously, reformatted root nodes with changed nodeKey from (version, 1) to (version, 0) became inaccessible, affecting both the reformatted version and subsequent versions referencing it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🔇 Additional comments (1)
CHANGELOG.md (1)
Line range hint 1-8: LGTM! The changelog structure is consistent.
The entry is correctly placed in the "Bug Fixes" section under "Unreleased", following the established changelog structure.
|
shall we merge? |
|
@Mergifyio backport release/v1.2.x |
|
@Mergifyio backport release/v1.3.x |
✅ Backports have been created
|
✅ Backports have been created
|
(cherry picked from commit cf74234) # Conflicts: # mutable_tree_test.go
(cherry picked from commit cf74234)
Problem Description
When the root node is reformatted due to pruning, its nodeKey changes from (version, 1) to (version, 0). As a result:
Proposed Solution
Add an extra check in the GetNode function to handle reformatted root nodes. This ensures that:
Summary by CodeRabbit
New Features
MutableTreeafter version deletions.DeleteVersionsFrom(int64)andGetLatestVersion.Bug Fixes
GetNodemethod to improve retrieval logic for nodes, ensuring better handling of cases withnilbuffers and legacy nodes.Documentation