cosmos / iavl

Merkleized IAVL+ Tree implementation in Go
Apache License 2.0
420 stars 263 forks source link

fix: pass initial version at immutable tree creation #880

Closed beer-1 closed 7 months ago

beer-1 commented 7 months ago

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 InitialVersion should be used when you make working hash to newly added moules.

https://github.com/cosmos/iavl/blob/af7ae13f47a477f11c18a4856d4a0c4e4a344e45/node.go#L435

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] for packetforward). but it should be differ in current iavl because it is using version to make working hash. and also found it is using tree.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 different WorkingHash and then consensus broken.

coderabbitai[bot] commented 7 months ago

Walkthrough

The update primarily focuses on enhancing the ImmutableTree data structure within the mutable_tree.go file by incorporating an initialization step for the version field. This initialization sets the version to an int64 value, which is obtained from opts.InitialVersion. This modification aims to ensure that the ImmutableTree starts with a specific version, enhancing its version management capabilities from the moment of its creation.

Changes

File(s) Change Summary
mutable_tree.go Added initialization of version field in ImmutableTree to opts.InitialVersion.

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?

Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

Tips ### Chat There are 3 ways to chat with CodeRabbit: > Note: Auto-reply has been disabled for this repository by the repository owner. The CodeRabbit bot will not respond to your comments unless it is explicitly tagged. - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit tests for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository from git and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit tests.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` 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) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger a review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - The JSON schema for the configuration file is available [here](https://coderabbit.ai/integrations/coderabbit-overrides.v2.json). - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json` ### CodeRabbit Discord Community Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback.
cool-develope commented 7 months ago

Thanks for reporting this issue, it seems like the InitialVersion option is not used properly. Tree.version should be determined in LoadVersion so in this case, we should consider the InitialVersion while LoadVersion

beer-1 commented 7 months ago

Thanks for reporting this issue, it seems like the InitialVersion option is not used properly. Tree.version should be determined in LoadVersion so in this case, we should consider the InitialVersion while LoadVersion.

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

https://github.com/cosmos/iavl/blob/af7ae13f47a477f11c18a4856d4a0c4e4a344e45/mutable_tree.go#L461-L472