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.

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 refnode at InitialVersion pointing at version 1 root #2

Merged
merged 17 commits into from
Aug 21, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
1 change: 1 addition & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ on:
push:
branches:
- master
- release/*
pull_request:

jobs:
Expand Down
38 changes: 37 additions & 1 deletion mutable_tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,9 @@ func (tree *MutableTree) Hash() []byte {

// WorkingHash returns the hash of the current working tree.
func (tree *MutableTree) WorkingHash() []byte {
return tree.root.hashWithCount(tree.WorkingVersion())
// Use tree.version instead of tree.WorkingVersion() because tree.version
// is always the latest version
return tree.root.hashWithCount(tree.version + 1)
Copy link
Member

Choose a reason for hiding this comment

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

just saying out loud here that this must've been included in the live data we have or we would have gotten app hash mismatches with mainnet?

Copy link
Member

Choose a reason for hiding this comment

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

Cosmos v0.47.x doesn't use the WorkingHash/WorkingVersion afaik, they are just defined on the iavl interface and unused by the rootmultistore

Copy link
Member Author

@drklee3 drklee3 Aug 20, 2024

Choose a reason for hiding this comment

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

right - this mostly updated to ensure the tree tests pass and keep the behavior consistent.

I thought it could be a potential issue with rollbacks via LoadVersionForOverwriting - two cases:

  1. Not possible via "regular" rollbacks since you can't load InitialVersion-1 to write InitialVersion again
  2. Creating a new tree with an existing node database, which is dependent on cosmos sdk behavior. If the node database still exists for this tree and we create a new tree at InitialVersion, this would hit the codepath in SaveVersion() that calls WorkingHash() that produces an incorrect mismatch

Added a test for the second case to make sure we aren't depending on cosmos handling it properly and WorkingHash() is correct in that case

Copy link
Member

Choose a reason for hiding this comment

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

Right, I forgot about the WorkingHash within those methods 🤦. I read through the LoadVersionForOverwriting test, nice comments there too -- some great thinking into how the iavl library is used 💯

}

func (tree *MutableTree) WorkingVersion() int64 {
Expand Down Expand Up @@ -746,6 +748,7 @@ func (tree *MutableTree) SaveVersion() ([]byte, int64, error) {
return nil, 0, err
}
} else {
// nodeKey already set, no changes to tree
if tree.root.nodeKey != nil {
// it means there are no updated nodes
if err := tree.ndb.SaveRoot(version, tree.root.nodeKey); err != nil {
Expand All @@ -761,10 +764,43 @@ func (tree *MutableTree) SaveVersion() ([]byte, int64, error) {
}
}
} else {
// No nodeKey, assign new nodeKeys and save nodes. This happens when
Copy link
Member

Choose a reason for hiding this comment

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

Well done explaining the logic branches here 👍

// any node in the tree has been updated as setting a key will
// reset all nodeKeys to nil
if err := tree.saveNewNodes(); err != nil {
return nil, 0, err
}
}

// Addition to node version patch for backwards compatibility:
// This is to resolve queries at InitialVersion AND at version 1.
// Only applies when InitialVersion > 1 and on the first save.
// The root node on a new tree is always at version 1 to maintain app
// hash backwards compatibility:
// InitialVersion != nodekey.Version
if tree.ndb.opts.InitialVersion > 1 && version == int64(tree.ndb.opts.InitialVersion) {
// SaveRoot is meant for saving a tree when there are no updates, which
// simply creates a reference node to the root node. We reuse this to
// create a reference node at InitialVersion with value of the root
// node at (1, 1).
if err := tree.ndb.SaveRoot(version, tree.root.nodeKey); err != nil {
return nil, 0, err
}

// Delete the root node at (1, 1) to be re-saved at (1, 0) below.
if err := tree.ndb.deleteFromPruning(tree.ndb.nodeKey(tree.root.nodeKey.GetKey())); err != nil {
return nil, 0, err
}

// Use a nonce of 0 to match pruning behavior that hides it from
// the version list.
// ndb.GetRoot will check for (version, 0) if (version, 1) does not
// exist.
tree.root.nodeKey.nonce = 0
if err := tree.ndb.SaveNode(tree.root); err != nil {
return nil, 0, err
}
}
}

if err := tree.ndb.Commit(); err != nil {
Expand Down
Loading
Loading