Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add Extra Check for Reformatted Root Node in GetNode #1007

Merged
merged 3 commits into from
Nov 26, 2024
Merged

fix: Add Extra Check for Reformatted Root Node in GetNode #1007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3881223
get the node reformatted by pruning
cool-develope Nov 20, 2024
9ea8b0b
Merge branch 'master' into pruning/reformatted_node_get
cool-develope Nov 20, 2024
da029aa
changelog
cool-develope Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Bug Fixes

- [#1007](https://github.com/cosmos/iavl/pull/1007) Add the extra check for the reformatted root node in `GetNode`

### Improvements

- [#952](https://github.com/cosmos/iavl/pull/952) Add `DeleteVersionsFrom(int64)` API.
Expand Down
24 changes: 24 additions & 0 deletions mutable_tree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1482,6 +1482,30 @@ func TestMutableTreeClose(t *testing.T) {
require.NoError(t, tree.Close())
}

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)
}
Comment on lines +1485 to +1507
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage and documentation for root node reformatting.

The test needs improvements in the following areas:

  1. 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

  2. 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.

Suggested change
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)
}
}


func TestMutableTree_InitialVersionZero(t *testing.T) {
db := dbm.NewMemDB()

Expand Down
14 changes: 14 additions & 0 deletions nodedb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,20 @@ func (ndb *nodeDB) GetNode(nk []byte) (*Node, error) {
if err != nil {
return nil, fmt.Errorf("can't get node %v: %v", nk, err)
}
if buf == nil && !isLegcyNode {
// if the node is reformatted by pruning, check against (version, 0)
nKey := GetNodeKey(nk)
if nKey.nonce == 1 {
nodeKey = ndb.nodeKey((&NodeKey{
version: nKey.version,
nonce: 0,
}).GetKey())
buf, err = ndb.db.Get(nodeKey)
if err != nil {
return nil, fmt.Errorf("can't get the reformatted node %v: %v", nk, err)
}
}
}
if buf == nil {
return nil, fmt.Errorf("Value missing for key %v corresponding to nodeKey %x", nk, nodeKey)
}
Expand Down
Loading