fix: pass initial version at immutable tree creation #880
Conversation
WalkthroughThe update primarily focuses on enhancing the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
beer-1
changed the title
|
Thanks for reporting this issue, it seems like the |
Not sure this is correct way to fix it. Maybe result should be same. If you like this more, I can fix the PR . if firstVersion == 0 {
if targetVersion <= 0 {
+ tree.version = int64(tree.ndb.opts.InitialVersion)
if !tree.skipFastStorageUpgrade {
tree.mtx.Lock()
defer tree.mtx.Unlock()
_, err := tree.enableFastStorageAndCommitIfNotEnabled()
return 0, err
}
return 0, nil
}
return 0, fmt.Errorf("no versions found while trying to load %v", targetVersion)
}Lines 461 to 472 in af7ae13 |
mutable_tree.go
Outdated
| @@ -56,7 +56,7 @@ func NewMutableTree(db dbm.DB, cacheSize int, skipFastStorageUpgrade bool, lg lo | |||
| } | |||
|
|
|||
| ndb := newNodeDB(db, cacheSize, opts, lg) | |||
| head := &ImmutableTree{ndb: ndb, skipFastStorageUpgrade: skipFastStorageUpgrade} | |||
| head := &ImmutableTree{ndb: ndb, skipFastStorageUpgrade: skipFastStorageUpgrade, version: int64(opts.InitialVersion)} | |||
There was a problem hiding this comment.
I know the result is same.
The happy case is to load the current version while constructing the tree in LoadVersion, so we don't need to set the InitialVersion option. The InitialVersion is just optional considering the above case like new module.
But if we force set the version here, it can be misleading the meaning of InitialVersion.
There was a problem hiding this comment.
I don't have strong opinion on this, so happy to follow your way.
Is the above solution (in comment) is good? or should we make other changes on LoadVersion function?
I have tested, and the above solution is also working.
There was a problem hiding this comment.
no, we should move this part to the LoadVersion, your commit will be not working when set the initial version by SetInitialVersion without opts.
There was a problem hiding this comment.
okay, I moved the fix to LoadVersion and tested it. It's also working well :)
@cool-develope
In previous iavl, the version is not affecting to working hash. but in current iavl, the version is a part of working hash, so the
InitialVersionshould be used when you make working hash to newly added moules.iavl/node.go
Line 435 in af7ae13
The problem is happening when you adding an new module
packetforward, you can easily reproduce the consensus breaking with software upgrade proposal. You should run more than two nodes and after upgrade restart validator node and see other nodes.We deeply debugged it and found the working hash for the stores, which are added via upgrade, are same whatever the height you did upgrade (
[164 128 39 101 194 146 82 214 50 145 131 89 83 171 212 207 142 176 239 6 10 8 254 1 95 237 121 132 93 24 213 34]forpacketforward). but it should be differ in current iavl because it is using version to make working hash. and also found it is usingtree.version, which is 0, when a new module firstly added by upgrade module. This wrong store hash is kept only in runtime. When you restart the node, it makes differentWorkingHashand then consensus broken.