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

feat(store/v1): async pruning iavl #20321

Merged
merged 13 commits into from
Jun 18, 2024
Merged

Conversation

cool-develope
Copy link
Contributor

@cool-develope cool-develope commented May 8, 2024

Description

  • bump the iavl with async pruning feature

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Added methods to control pruning behavior during store commits, improving performance and flexibility.
  • Bug Fixes

    • Adjusted pruning logic to ensure proper version loading and commit behavior.
  • Chores

    • Updated module dependencies across several files, including upgrading github.com/cosmos/iavl to version v1.2.0.

@cool-develope cool-develope requested a review from a team as a code owner May 8, 2024 16:20
Copy link
Contributor

coderabbitai bot commented May 8, 2024

Walkthrough

Walkthrough

The updates primarily involve advancing dependencies, notably upgrading github.com/cosmos/iavl from v1.1.4 to v1.2.0. Additionally, new methods and interfaces for managing pruning during commit processes have been introduced. These adjustments aim to enhance commit and pruning functionalities across several components, impacting testing and dependency management in related modules.

Changes

Files Change Summary
simapp/gomod2nix.toml Updated module versions, specifically github.com/cosmos/iavl from "v1.1.4" to "v1.2.0", and hashes across dependencies.
store/rootmulti/store.go Added SetCommitting and PausePruning methods to manage commit and pruning processes.
store/types/store.go Updated Committer interface with SetCommitting method and added PausablePruner interface to manage pruning.
store/iavl/store.go Implemented PausablePruner interface, modified tree loading function, and added PausePruning method.
store/rootmulti/store_test.go Adjusted tests to incorporate new pruning logic and asynchronous checks.
client/v2/go.mod Updated github.com/cosmos/iavl from v1.1.4 to v1.2.0 as an indirect dependency.
go.mod Updated github.com/cosmos/iavl to version v1.2.0.
simapp/go.mod Upgraded github.com/cosmos/iavl dependency to v1.2.0.
store/go.mod Updated github.com/cosmos/iavl dependency to v1.2.0.
tests/go.mod Upgraded github.com/cosmos/iavl to v1.2.0.

Sequence Diagrams

Commit and Pruning Process

sequenceDiagram
    participant Client
    participant MultiStore
    participant IAVLStore

    Client->>MultiStore: SetCommitting(true)
    MultiStore->>IAVLStore: SetCommitting(true)
    Client-->>MultiStore: PausePruning(true)
    MultiStore-->>IAVLStore: PausePruning(true)

    Note over IAVLStore: Begin Commit
    
    IAVLStore-->>MultiStore: Notify Commit Start
    Client->>MultiStore: Commit
    MultiStore->>IAVLStore: Commit

    Note over IAVLStore: Perform Async Pruning

    MultiStore-->>Client: Commit Done
    MultiStore-->>IAVLStore: PausePruning(false)
    Client-->>MultiStore: PausePruning(false)
Loading

Existing vs New Flow (High-Level)

sequenceDiagram
    participant Client
    participant MultiStore
    participant IAVLStore

    Note over Client, MultiStore: Existing Flow
    Client->>MultiStore: Commit
    MultiStore->>IAVLStore: Commit

    Note over Client, MultiStore: New Flow
    Client->>MultiStore: SetCommitting(true)
    MultiStore->>IAVLStore: SetCommitting(true)
    Client-->>MultiStore: PausePruning(true)
    MultiStore-->>IAVLStore: PausePruning(true)

    Note over IAVLStore: Begin Commit
    
    IAVLStore-->>MultiStore: Notify Commit Start
    Client->>MultiStore: Commit
    MultiStore->>IAVLStore: Commit

    Note over IAVLStore: Perform Async Pruning

    MultiStore-->>Client: Commit Done
    MultiStore-->>IAVLStore: PausePruning(false)
    Client-->>MultiStore: PausePruning(false)
Loading

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
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • 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 testing code 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 and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @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.
  • Please see the configuration documentation for more information.
  • 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/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

github-actions bot commented May 8, 2024

@cool-develope your pull request is missing a changelog!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 83c4b9b and 0b3814f.
Files ignored due to path filters (2)
  • store/go.mod is excluded by !**/*.mod
  • store/go.sum is excluded by !**/*.sum
Files selected for processing (8)
  • store/iavl/store.go (2 hunks)
  • store/iavl/tree.go (2 hunks)
  • store/mem/store.go (1 hunks)
  • store/rootmulti/dbadapter.go (1 hunks)
  • store/rootmulti/store.go (2 hunks)
  • store/rootmulti/store_test.go (4 hunks)
  • store/transient/store.go (1 hunks)
  • store/types/store.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • store/rootmulti/store_test.go
Additional Context Used
Path-based Instructions (7)
store/transient/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

store/rootmulti/dbadapter.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

store/mem/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

store/iavl/tree.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

store/iavl/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

store/types/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

store/rootmulti/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (9)
store/transient/store.go (1)

34-34: The implementation of SetCommitting as a no-op is appropriate for a transient store, where data does not persist beyond a session.

store/rootmulti/dbadapter.go (1)

43-43: The implementation of SetCommitting as a no-op is appropriate for commitDBStoreAdapter, aligning with its characteristics of not computing any commit hash and not loading older state.

store/mem/store.go (1)

53-53: The implementation of SetCommitting as a no-op is appropriate for a memory store, where pruning operations are not required.

store/iavl/tree.go (2)

59-59: The implementation of SetCommitting to raise an exception on an immutable IAVL tree is appropriate, ensuring the tree's integrity by preventing state changes.


63-63: The implementation of UnsetCommitting to raise an exception on an immutable IAVL tree is appropriate, ensuring the tree's integrity by preventing state changes.

store/iavl/store.go (2)

55-55: The update to LoadStoreWithInitialVersion to include iavl.AsyncPruningOption(true) is correctly implemented, aligning with the PR's objectives to enhance pruning efficiency by enabling asynchronous operations.


153-153: The implementation of SetCommitting to delegate state changes based on the isCommit flag is appropriate, allowing the IAVL store to manage its committing status effectively.

store/types/store.go (1)

30-30: The addition of SetCommitting to the Committer interface is appropriate, enabling different store types to implement their own pruning logic during commit phases, aligning with the PR's objectives.

store/rootmulti/store.go (1)

461-466: Ensure that all stores implement SetCommitting method to avoid runtime panics.

Verification successful

The verification process has confirmed that all store types within the codebase implement the SetCommitting method. This includes the Store types in store/rootmulti, store/mem, store/iavl, and store/transient. Therefore, the concern about potential runtime panics due to missing implementations of this method is unfounded.

  • store/rootmulti/store.go: SetCommitting implemented
  • store/mem/store.go: SetCommitting implemented
  • store/iavl/store.go: SetCommitting implemented
  • store/transient/store.go: SetCommitting implemented
Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that all stores implement the `SetCommitting` method.

# Test: Search for stores that might not implement `SetCommitting`. Expect: No results.
rg --type go $'func \\\\(.*Store\\\\) SetCommitting\\\\('

Length of output: 173



Script:

#!/bin/bash
# Description: Verify that all stores implement the `SetCommitting` method.

# Test: Search for stores that might not implement `SetCommitting`. Expect: No results.
rg --type go 'func \(.*Store\) SetCommitting\('

Length of output: 319

store/rootmulti/store.go Fixed Show resolved Hide resolved
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

LGTM, just the question around breaking the interface, can we cast to iavl tree and avoid the breakage

Comment on lines +25 to 27
SetCommitting()
UnsetCommitting()
SaveVersion() ([]byte, int64, error)
Copy link
Member

Choose a reason for hiding this comment

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

do we need to break this interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is the interface of MutableTree, not allow to modify it

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 0b3814f and 977080c.
Files ignored due to path filters (6)
  • simapp/go.mod is excluded by !**/*.mod
  • simapp/go.sum is excluded by !**/*.sum
  • store/go.mod is excluded by !**/*.mod
  • store/go.sum is excluded by !**/*.sum
  • tests/go.mod is excluded by !**/*.mod
  • tests/go.sum is excluded by !**/*.sum
Files selected for processing (3)
  • simapp/gomod2nix.toml (7 hunks)
  • store/rootmulti/store.go (2 hunks)
  • store/types/store.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • store/rootmulti/store.go
  • store/types/store.go
Additional comments not posted (10)
simapp/gomod2nix.toml (10)

5-6: Ensure the updated version and hash for buf.build/gen/go/cometbft/cometbft/protocolbuffers/go are correct and consistent with the source.


8-9: Ensure the updated version and hash for buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go are correct and consistent with the source.


135-136: Ensure the updated version and hash for github.com/cosmos/iavl are correct and consistent with the source.


276-277: Ensure the updated version and hash for github.com/hashicorp/go-hclog are correct and consistent with the source.


390-391: Ensure the updated version and hash for github.com/pelletier/go-toml/v2 are correct and consistent with the source.


402-403: Ensure the updated version and hash for github.com/prometheus/client_golang are correct and consistent with the source.


514-515: Ensure the updated version and hash for golang.org/x/crypto are correct and consistent with the source.


517-518: Ensure the updated version and hash for golang.org/x/exp are correct and consistent with the source.


523-524: Ensure the updated version and hash for golang.org/x/net are correct and consistent with the source.


532-533: Ensure the updated version and hash for golang.org/x/sys are correct and consistent with the source.

@cool-develope
Copy link
Contributor Author

@tac0turtle , need new tag?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 977080c and 1c74b2e.
Files selected for processing (3)
  • store/iavl/store.go (2 hunks)
  • store/rootmulti/store.go (5 hunks)
  • store/types/store.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • store/rootmulti/store.go
Additional Context Used
Path-based Instructions (2)
store/iavl/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

store/types/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (3)
store/iavl/store.go (2)

55-55: Async pruning option added.


150-157: PausePruning method added.

store/types/store.go (1)

32-36: The new PausablePruner interface looks good and is well-documented.

@cool-develope
Copy link
Contributor Author

@kocubinski appreciate update 🙏

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 1c74b2e and 80ded44.
Files ignored due to path filters (6)
  • simapp/go.mod is excluded by !**/*.mod
  • simapp/go.sum is excluded by !**/*.sum
  • store/go.mod is excluded by !**/*.mod
  • store/go.sum is excluded by !**/*.sum
  • tests/go.mod is excluded by !**/*.mod
  • tests/go.sum is excluded by !**/*.sum
Files selected for processing (1)
  • simapp/gomod2nix.toml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • simapp/gomod2nix.toml

store/rootmulti/store.go Dismissed Show dismissed Hide dismissed
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Good start. The integration makes sense but I think IAVL needs some better protection to not block when UnsetCommitting is not used correct

store/rootmulti/store.go Outdated Show resolved Hide resolved
if pause {
st.tree.SetCommitting()
} else {
st.tree.UnsetCommitting()
Copy link
Contributor

Choose a reason for hiding this comment

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

I looked into the iavl impl and this method is not safe to be called (multiple times) without a SetCommitting before. This is risky for a public method as people may use it without deeper knowledge. PausePruning(false) does not look like a 💣
IMHO it makes sense to fix UnsetCommitting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was also thinking if we need to provide better locking so that no other process accidentally unlocks? Do you think such a scenario exists? In this case, control must be within the iavl and you can have a tree.DoWithCommitPaused{func(){...}) that is guarded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe those should be handled within iavl, cosmos/iavl#953
I added some comments for this PR.

// Ensure async pruning is done
time.Sleep(500 * time.Millisecond)
ms.Commit()

Copy link
Contributor

Choose a reason for hiding this comment

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

Tests are passing without PausePruning set. How can you ensure the expected behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PausePruning is handled within Commit, the extra commit is for batch flushing.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not straight forward but you can ensure the pauses calls with some decorator:

var _ types.PausablePruner = &pauseableCommitKVStoreStub{}

type pauseableCommitKVStoreStub struct {
	types.CommitKVStore
	pauseCalled []bool
}

func (p *pauseableCommitKVStoreStub) PausePruning(b bool) {
	p.pauseCalled = append(p.pauseCalled, b)
}

func TestPausePruningOnCommit(t *testing.T) {
	store := NewStore(dbm.NewMemDB(), log.NewNopLogger(), metrics.NewNoOpMetrics())
	store.SetPruning(pruningtypes.NewCustomPruningOptions(2, 11))
	store.MountStoreWithDB(testStoreKey1, types.StoreTypeIAVL, nil)
	require.NoError(t, store.LoadLatestVersion())

	myStub := &pauseableCommitKVStoreStub{CommitKVStore: store.stores[testStoreKey1]}
	store.stores[testStoreKey1] = myStub
	// when
	store.Commit()
	// then
	assert.Equal(t, []bool{true, false}, myStub.pauseCalled)
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 80ded44 and 21f0d35.
Files selected for processing (3)
  • store/rootmulti/store.go (5 hunks)
  • store/rootmulti/store_test.go (4 hunks)
  • store/types/store.go (1 hunks)
Additional Context Used
Path-based Instructions (3)
store/types/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

store/rootmulti/store_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

store/rootmulti/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (2)
store/types/store.go (1)

32-38: The addition of the PausablePruner interface is well-implemented and clearly documented.

store/rootmulti/store_test.go (1)

583-596: The test case for pruning the same heights twice is correctly implemented to ensure idempotency and stability of the pruning process.

store/rootmulti/store.go Show resolved Hide resolved
store/rootmulti/store.go Outdated Show resolved Hide resolved
store/rootmulti/store_test.go Show resolved Hide resolved
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Thanks for the updates and further infos! 🛥️
If you can add the type check to the iavl.Store, this would be great.

I have added some more test examples and nits. Feel free to adopt but no blockers

store/rootmulti/store.go Outdated Show resolved Hide resolved
store/rootmulti/store_test.go Outdated Show resolved Hide resolved
@@ -617,6 +624,10 @@ func TestMultiStore_PruningRestart(t *testing.T) {
actualHeightToPrune = ms.pruningManager.GetPruningHeight(ms.LatestVersion())
require.Equal(t, int64(8), actualHeightToPrune)

// Ensure async pruning is done
time.Sleep(500 * time.Millisecond)
ms.Commit()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do you need this extra commit? It is passing without

Copy link
Contributor Author

@cool-develope cool-develope May 29, 2024

Choose a reason for hiding this comment

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

yes, to flush the batch data which includes pruning info

store/rootmulti/store_test.go Outdated Show resolved Hide resolved
// Ensure async pruning is done
time.Sleep(500 * time.Millisecond)
ms.Commit()

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not straight forward but you can ensure the pauses calls with some decorator:

var _ types.PausablePruner = &pauseableCommitKVStoreStub{}

type pauseableCommitKVStoreStub struct {
	types.CommitKVStore
	pauseCalled []bool
}

func (p *pauseableCommitKVStoreStub) PausePruning(b bool) {
	p.pauseCalled = append(p.pauseCalled, b)
}

func TestPausePruningOnCommit(t *testing.T) {
	store := NewStore(dbm.NewMemDB(), log.NewNopLogger(), metrics.NewNoOpMetrics())
	store.SetPruning(pruningtypes.NewCustomPruningOptions(2, 11))
	store.MountStoreWithDB(testStoreKey1, types.StoreTypeIAVL, nil)
	require.NoError(t, store.LoadLatestVersion())

	myStub := &pauseableCommitKVStoreStub{CommitKVStore: store.stores[testStoreKey1]}
	store.stores[testStoreKey1] = myStub
	// when
	store.Commit()
	// then
	assert.Equal(t, []bool{true, false}, myStub.pauseCalled)
}

store/iavl/store.go Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 21f0d35 and cd502be.
Files selected for processing (3)
  • store/iavl/store.go (3 hunks)
  • store/rootmulti/store.go (5 hunks)
  • store/rootmulti/store_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
  • store/rootmulti/store.go
  • store/rootmulti/store_test.go
Additional Context Used
Path-based Instructions (1)
store/iavl/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (2)
store/iavl/store.go (2)

34-34: Interface implementation confirmed.

The Store struct is correctly asserted to implement the types.PausablePruner interface, ensuring type safety.


151-158: Review the implementation of PausePruning.

This method toggles the committing state of the tree, which is crucial for the new asynchronous pruning feature. Ensure that the locking mechanism is robust enough to handle concurrent accesses as suggested in previous comments.

@@ -52,7 +53,7 @@ func LoadStore(db dbm.DB, logger log.Logger, key types.StoreKey, id types.Commit
// provided DB. An error is returned if the version fails to load, or if called with a positive
// version on an empty tree.
func LoadStoreWithInitialVersion(db dbm.DB, logger log.Logger, key types.StoreKey, id types.CommitID, initialVersion uint64, cacheSize int, disableFastNode bool, metrics metrics.StoreMetrics) (types.CommitKVStore, error) {
tree := iavl.NewMutableTree(wrapper.NewDBWrapper(db), cacheSize, disableFastNode, logger, iavl.InitialVersionOption(initialVersion))
tree := iavl.NewMutableTree(wrapper.NewDBWrapper(db), cacheSize, disableFastNode, logger, iavl.InitialVersionOption(initialVersion), iavl.AsyncPruningOption(true))
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Ensure consistent handling of the new pruning options.

The new pruning option iavl.AsyncPruningOption is only applied in one instance of iavl.NewMutableTree in store/iavl/store.go. Other instances of iavl.NewMutableTree do not include this option, which may lead to inconsistent behavior.

  • store/v2/commitment/iavl/tree.go
  • store/prefix/store_test.go
  • store/iavl/store_test.go
  • store/cache/cache_test.go

Please review these instances to ensure the new pruning option is correctly and consistently applied.

Analysis chain

Ensure proper handling of new pruning options.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that the new pruning options are correctly handled throughout the system.

# Test: Search for usage of `iavl.AsyncPruningOption` to ensure it's correctly implemented.
rg --type go $'iavl.AsyncPruningOption'

Length of output: 223



Script:

#!/bin/bash
# Description: Verify that the new pruning options are correctly handled throughout the system.

# Test: Search for all instances of `iavl.NewMutableTree` to ensure the new pruning option is consistently applied.
rg --type go 'iavl.NewMutableTree'

Length of output: 2000

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 63d0dec and b435387.

Files ignored due to path filters (5)
  • client/v2/go.sum is excluded by !**/*.sum
  • go.sum is excluded by !**/*.sum
  • simapp/go.sum is excluded by !**/*.sum
  • store/go.sum is excluded by !**/*.sum
  • tests/go.sum is excluded by !**/*.sum
Files selected for processing (6)
  • client/v2/go.mod (1 hunks)
  • go.mod (1 hunks)
  • simapp/go.mod (1 hunks)
  • simapp/gomod2nix.toml (11 hunks)
  • store/go.mod (1 hunks)
  • tests/go.mod (1 hunks)
Files skipped from review due to trivial changes (4)
  • client/v2/go.mod
  • go.mod
  • store/go.mod
  • tests/go.mod
Additional comments not posted (4)
simapp/go.mod (1)

90-90: Update to github.com/cosmos/iavl v1.2.0 aligns with the PR's goal to enhance the IAVL pruning logic.

This update is crucial for the new asynchronous pruning features introduced in the store module, ensuring compatibility with the latest IAVL methods.

simapp/gomod2nix.toml (3)

291-292: Updated github.com/hashicorp/golang-lru/v2 to v2.0.7. Ensure this update does not introduce any breaking changes or performance regressions.

Verification successful

The verification scripts executed did not find any mentions of breaking changes in the release notes for any of the recent versions of github.com/hashicorp/golang-lru/v2. Additionally, no issues or discussions mentioning breaking changes were found in the repository. The usage of github.com/hashicorp/golang-lru/v2 in the codebase is indirect, as indicated by the go.mod and go.sum files.

Given this information, it appears that the update to v2.0.7 of github.com/hashicorp/golang-lru/v2 does not introduce any breaking changes or performance regressions.

  • No breaking changes were found in the release notes for versions v2.0.0 to v2.0.6.
  • No issues or discussions related to breaking changes were found in the repository.
  • The dependency is used indirectly in the codebase.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify no breaking changes in hashicorp golang-lru v2.0.7.

# Test: Search for breaking changes mentioned in the release notes.
curl -s https://github.com/hashicorp/golang-lru/releases/tag/v2.0.7 | grep "breaking changes"

Length of output: 95



Script:

#!/bin/bash
# Description: Verify no breaking changes in recent versions of hashicorp golang-lru.

# Define the versions to check.
versions=("v2.0.6" "v2.0.5" "v2.0.4" "v2.0.3" "v2.0.2" "v2.0.1" "v2.0.0")

# Loop through each version and check for breaking changes in the release notes.
for version in "${versions[@]}"; do
  echo "Checking release notes for version $version..."
  curl -s "https://github.com/hashicorp/golang-lru/releases/tag/$version" | grep -i "breaking changes"
done

Length of output: 1699


Script:

#!/bin/bash
# Description: Search for issues or discussions mentioning breaking changes in the hashicorp golang-lru repository.

# Search for issues mentioning breaking changes.
curl -s "https://api.github.com/repos/hashicorp/golang-lru/issues?state=all" | jq '.[] | select(.title | test("breaking changes"; "i")) | {title, html_url}'

# Search for discussions mentioning breaking changes.
curl -s "https://api.github.com/repos/hashicorp/golang-lru/discussions?state=all" | jq '.[] | select(.title | test("breaking changes"; "i")) | {title, html_url}'

Length of output: 388


Script:

#!/bin/bash
# Description: Ensure compatibility with the rest of the codebase by checking for direct usage of hashicorp golang-lru.

# Search for import statements of hashicorp golang-lru in the codebase.
rg 'github.com/hashicorp/golang-lru/v2'

Length of output: 7348


553-554: Updated google.golang.org/protobuf to v1.34.2. Verify compatibility with other protobuf-related dependencies and ensure there are no deprecation issues.

Verification successful

Let's gather the versions of the protobuf-related dependencies and check for any compatibility issues or deprecations.


To ensure compatibility and check for any deprecations or breaking changes, I need to verify the release notes and breaking changes for the protobuf-related dependencies. This will confirm if the updated version of google.golang.org/protobuf to v1.34.2 is compatible with the other protobuf dependencies.


The release notes for the protobuf-related dependencies indicate that there are no breaking changes or significant compatibility issues with the updated version of google.golang.org/protobuf to v1.34.2. The updates include minor features and bug fixes, which are unlikely to affect the existing functionalities.

  • google.golang.org/protobuf v1.34.2: Minor feature and bug fixes.
  • github.com/golang/protobuf v1.5.4: Updated descriptor.proto to the latest version.
  • github.com/gogo/protobuf v1.3.2: Bug fixes and tested versions.

Based on this information, the update appears to be safe and compatible with the other protobuf-related dependencies.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Check compatibility of updated protobuf version with other protobuf-related dependencies.

# Test: List all protobuf-related dependencies and their versions.
grep "protobuf" simapp/gomod2nix.toml

Length of output: 146



Script:

#!/bin/bash
# Description: List versions of all protobuf-related dependencies.

# Test: Get version information for protobuf-related dependencies.
grep -A 2 "github.com/gogo/protobuf" simapp/gomod2nix.toml
grep -A 2 "github.com/golang/protobuf" simapp/gomod2nix.toml
grep -A 2 "google.golang.org/protobuf" simapp/gomod2nix.toml

Length of output: 554



Script:

#!/bin/bash
# Description: Check release notes for breaking changes and compatibility issues for protobuf-related dependencies.

# Fetch release notes for google.golang.org/protobuf v1.34.2
curl -s https://api.github.com/repos/protocolbuffers/protobuf-go/releases/tags/v1.34.2 | jq '.body'

# Fetch release notes for github.com/golang/protobuf v1.5.4
curl -s https://api.github.com/repos/golang/protobuf/releases/tags/v1.5.4 | jq '.body'

# Fetch release notes for github.com/gogo/protobuf v1.3.2
curl -s https://api.github.com/repos/gogo/protobuf/releases/tags/v1.3.2 | jq '.body'

Length of output: 1091


129-130: Updated github.com/cosmos/iavl to v1.2.0. Confirm this version supports the new asynchronous pruning features as described in the PR.

Verification successful

The updated version v1.2.0 of github.com/cosmos/iavl includes the methods PausePruning and SetCommitting, which support asynchronous pruning features.

  • store/iavl/tree.go
  • store/iavl/store.go
  • store/v2/commitment/iavl/tree.go
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Check for asynchronous pruning support in IAVL v1.2.0.

# Test: Search for relevant methods in the IAVL repository.
rg --type go "PausePruning|SetCommitting" -- /path/to/local/iavl/repo

Length of output: 136



Script:

#!/bin/bash
# Locate the `github.com/cosmos/iavl` repository directory within the project.
fd -t d 'iavl'

Length of output: 52



Script:

#!/bin/bash
# Check for asynchronous pruning support in the identified directories.

# Search `store/iavl/` for relevant methods.
rg --type go "PausePruning|SetCommitting" store/iavl/

# Search `store/v2/commitment/iavl/` for relevant methods.
rg --type go "PausePruning|SetCommitting" store/v2/commitment/iavl/

Length of output: 703

@cool-develope cool-develope added this pull request to the merge queue Jun 18, 2024
Merged via the queue into main with commit 7edd868 Jun 18, 2024
90 checks passed
@cool-develope cool-develope deleted the store/async_pruning_iavl branch June 18, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants