fix: Add refnode at InitialVersion pointing at version 1 root #2

drklee3 · 2024-08-12T21:59:23Z

Background

Nodes are saved with version 1 instead of InitialVersion. This was part of the patch in order to preserve node hashes and thus apphash.
Loading version at InitialVersion is not found when it should, there is no separate rootnode lookup based on the different InitialVersion instead of root node version

Changes

Add test to reproduce issue to load versions due to our patch
Add initial patch to add a new refnode at InitialVersion that points at version 1 root node
Set the refnode at (InitialVersion, 0)
- nonce of 0 similar to pruning such that it is only used by reference nodes and not shown as a valid version.

pirtleshell · 2024-08-14T21:24:00Z

mutable_tree.go

-	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)


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?

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

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:

Not possible via "regular" rollbacks since you can't load InitialVersion-1 to write InitialVersion again

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

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 💯

pirtleshell

excellent test coverage and we love a fix that doesn't require syncing from the upgrades again (or injecting more complex data into the tree than a reference node pointer to an existing data node)

seems good to merge as long as test for pruning are on our radar and either included here or in a different pr.

mutable_tree.go

This reverts commit 6365e4e. Adding this just so we have reference of the other option

nddeluca

Approved! Great testing -- I'm confident from reading the tests that we've not only fixed the iavl hash compatibility with new modified trees on upgrades, but fetching versions and pruning also will work as expected.

The only thing I saw is there is one test that is still skipped -- I'm not sure if that's meant to be a reference or we missed updating it.

nddeluca · 2024-08-20T23:18:10Z

mutable_tree.go

@@ -761,10 +764,43 @@ func (tree *MutableTree) SaveVersion() ([]byte, int64, error) {
 				}
 			}
 		} else {
+			// No nodeKey, assign new nodeKeys and save nodes. This happens when


Well done explaining the logic branches here 👍

mutable_tree_test.go

nddeluca · 2024-08-20T23:26:14Z

mutable_tree_test.go

+	require.NoError(t, err)
+
+	// ------------------------------
+	// Verify


I like the comment and the test organization 🏎️

nddeluca · 2024-08-20T23:27:29Z

mutable_tree_test.go

+	require.False(t, hasVersion1, "version 1 should not be found")
+
+	// Internal ndb methods for good measure
+	firstVersion, err := tree.ndb.getFirstVersion()


Defensive testing is great 🛡️

nddeluca · 2024-08-20T23:28:41Z

tree_test.go

@@ -1952,5 +1955,5 @@ func TestWorkingHashWithInitialVersion(t *testing.T) {

 	commitHash1, _, err := tree.SaveVersion()
 	require.NoError(t, err)
-	require.Equal(t, commitHash1, commitHash)
+	assert.Equal(t, hex.EncodeToString(commitHash1), hex.EncodeToString(commitHash))


I prefer assert & comparing hex strings as well 👍

nddeluca · 2024-08-20T23:31:51Z

mutable_tree_test.go

+	})
+
+	t.Run("3. AvailableVersions()", func(t *testing.T) {
+		versions := tree.AvailableVersions()


The attention to detail on exercising all affected API calls is excellent

drklee3 added 2 commits August 12, 2024 14:57

test: Update test for loading patched initial version

2d4c81a

fix: Add refnode to initial version to point at version 1

628affc

drklee3 changed the title ~~test: Update test for loading patched initial version~~ fix: Add refnode at InitialVersion pointing at version 1 root Aug 12, 2024

drklee3 added 3 commits August 12, 2024 15:33

docs: Update comments on patch

5cf60dd

fix: Update WorkingHash() to also use tree.version + 1

33f1680

ci: Run test workflow on release branches

882046b

pirtleshell reviewed Aug 14, 2024

View reviewed changes

pirtleshell approved these changes Aug 14, 2024

View reviewed changes

drklee3 added 6 commits August 15, 2024 11:51

fix: Prevent nil panic on reading tree.root.nodeKey on empty tree

2d28d39

test: Add test for empty tree, pruning

f73700b

test: Temp skip ndb.hasVersion(1)

6c6dbcf

test: Add cases for node db versions

178156c

fix: Hide root node at version 1

2644f53

chore: remove todo comment

01ca777

drklee3 marked this pull request as ready for review August 16, 2024 18:57

drklee3 requested review from pirtleshell and nddeluca August 16, 2024 19:10

pirtleshell reviewed Aug 16, 2024

View reviewed changes

mutable_tree.go Outdated Show resolved Hide resolved

drklee3 force-pushed the dl-v1.2.0-initial-node-ver branch from 6365e4e to 01ca777 Compare August 16, 2024 19:58

drklee3 added 4 commits August 16, 2024 13:03

refactor: Alternative way to set the initialversion root node

a6d56f9

Revert "refactor: Alternative way to set the initialversion root node"

0e0bd3e

This reverts commit 6365e4e. Adding this just so we have reference of the other option

doc: Clarify patch comments

5237676

test: Ensure apphash stays the same for different InitialVersions

c1d6db0

nddeluca approved these changes Aug 20, 2024

View reviewed changes

nddeluca reviewed Aug 20, 2024

View reviewed changes

drklee3 added 2 commits August 20, 2024 16:40

test: Update skipped (1, 0) root node test

cdfc587

test: Add test for rollbacks on new tree and existing ndb

d5920a1

drklee3 merged commit a6787a0 into release/v1.2.0-kava-v0.26-x Aug 21, 2024
4 checks passed

drklee3 deleted the dl-v1.2.0-initial-node-ver branch August 21, 2024 16:22

drklee3 mentioned this pull request Aug 21, 2024

fix: Retract v1.2.0-kava.1 #3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add refnode at InitialVersion pointing at version 1 root #2

fix: Add refnode at InitialVersion pointing at version 1 root #2

drklee3 commented Aug 12, 2024 •

edited

Loading

pirtleshell Aug 14, 2024

nddeluca Aug 20, 2024

drklee3 Aug 20, 2024 •

edited

Loading

nddeluca Aug 21, 2024

pirtleshell left a comment

nddeluca left a comment

nddeluca Aug 20, 2024

nddeluca Aug 20, 2024

nddeluca Aug 20, 2024

nddeluca Aug 20, 2024

nddeluca Aug 20, 2024

fix: Add refnode at InitialVersion pointing at version 1 root #2

fix: Add refnode at InitialVersion pointing at version 1 root #2

Conversation

drklee3 commented Aug 12, 2024 • edited Loading

Background

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drklee3 Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirtleshell left a comment

Choose a reason for hiding this comment

nddeluca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drklee3 commented Aug 12, 2024 •

edited

Loading

drklee3 Aug 20, 2024 •

edited

Loading