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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0b3814f
bump iavl
cool-develope May 8, 2024
612c14d
Merge branch 'main' into store/async_pruning_iavl
cool-develope May 14, 2024
977080c
go mod tidy
cool-develope May 14, 2024
1c74b2e
draft approach
kocubinski May 16, 2024
80ded44
Merge branch 'main' into store/async_pruning_iavl
cool-develope May 17, 2024
21f0d35
comments
cool-develope May 25, 2024
cd502be
comments
cool-develope May 29, 2024
63d0dec
Merge branch 'main' into store/async_pruning_iavl
cool-develope May 29, 2024
960b89b
Merge branch 'main' into store/async_pruning_iavl
cool-develope May 31, 2024
c285a8d
Merge branch 'main' into store/async_pruning_iavl
cool-develope Jun 3, 2024
ca0e980
go mod tidy
cool-develope Jun 3, 2024
7c89f7c
Merge branch 'main' into store/async_pruning_iavl
cool-develope Jun 18, 2024
b435387
go mod tidy
cool-develope Jun 18, 2024
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
2 changes: 1 addition & 1 deletion store/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ require (
github.com/cosmos/cosmos-db v1.0.2
github.com/cosmos/cosmos-proto v1.0.0-beta.5
github.com/cosmos/gogoproto v1.4.12
github.com/cosmos/iavl v1.1.2
github.com/cosmos/iavl v1.1.3-0.20240508141442-4ad064ece06e
github.com/cosmos/ics23/go v0.10.0
github.com/golang/mock v1.6.0
github.com/golang/protobuf v1.5.4 // indirect
Expand Down
2 changes: 2 additions & 0 deletions store/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ github.com/cosmos/gogoproto v1.4.12 h1:vB6Lbe/rtnYGjQuFxkPiPYiCybqFT8QvLipDZP8Jp
github.com/cosmos/gogoproto v1.4.12/go.mod h1:LnZob1bXRdUoqMMtwYlcR3wjiElmlC+FkjaZRv1/eLY=
github.com/cosmos/iavl v1.1.2 h1:zL9FK7C4L/P4IF1Dm5fIwz0WXCnn7Bp1M2FxH0ayM7Y=
github.com/cosmos/iavl v1.1.2/go.mod h1:jLeUvm6bGT1YutCaL2fIar/8vGUE8cPZvh/gXEWDaDM=
github.com/cosmos/iavl v1.1.3-0.20240508141442-4ad064ece06e h1:XsqaeADjXcUTxNVIH2lUCukHnBwRLNE0INyNDtwDCQw=
github.com/cosmos/iavl v1.1.3-0.20240508141442-4ad064ece06e/go.mod h1:vCYmRQUJU1wwj0oRD3wMEtOM9sJNDP+GFMaXmIxZ/rU=
github.com/cosmos/ics23/go v0.10.0 h1:iXqLLgp2Lp+EdpIuwXTYIQU+AiHj9mOC2X9ab++bZDM=
github.com/cosmos/ics23/go v0.10.0/go.mod h1:ZfJSmng/TBNTBkFemHHHj5YY7VAU/MBU980F4VU1NG0=
github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
Expand Down
11 changes: 10 additions & 1 deletion store/iavl/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,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


isUpgradeable, err := tree.IsUpgradeable()
if err != nil {
Expand Down Expand Up @@ -147,6 +147,15 @@ func (st *Store) LastCommitID() types.CommitID {
}
}

// SetCommitting implements CommitKVStore interface.
func (st *Store) SetCommitting(isCommit bool) {
if isCommit {
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.

}
}

// SetPruning panics as pruning options should be provided at initialization
// since IAVl accepts pruning options directly.
func (st *Store) SetPruning(_ pruningtypes.PruningOptions) {
Expand Down
10 changes: 10 additions & 0 deletions store/iavl/tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ type (
Get(key []byte) ([]byte, error)
Set(key, value []byte) (bool, error)
Remove(key []byte) ([]byte, bool, error)
SetCommitting()
UnsetCommitting()
SaveVersion() ([]byte, int64, error)
Comment on lines +25 to 27
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

Version() int64
Hash() []byte
Expand Down Expand Up @@ -53,6 +55,14 @@ func (it *immutableTree) Remove(_ []byte) ([]byte, bool, error) {
panic("cannot call 'Remove' on an immutable IAVL tree")
}

func (it *immutableTree) SetCommitting() {
panic("cannot call 'SetCommitting' on an immutable IAVL tree")
}

func (it *immutableTree) UnsetCommitting() {
panic("cannot call 'UnsetCommitting' on an immutable IAVL tree")
}

func (it *immutableTree) SaveVersion() ([]byte, int64, error) {
panic("cannot call 'SaveVersion' on an immutable IAVL tree")
}
Expand Down
3 changes: 3 additions & 0 deletions store/mem/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ func (s Store) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.Cach
// Commit performs a no-op as entries are persistent between commitments.
func (s *Store) Commit() (id types.CommitID) { return }

// SetCommitting is a no-op as entries.
func (s *Store) SetCommitting(_ bool) {}

func (s *Store) SetPruning(pruning pruningtypes.PruningOptions) {}

// GetPruning is a no-op as pruning options cannot be directly set on this store.
Expand Down
2 changes: 2 additions & 0 deletions store/rootmulti/dbadapter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ func (cdsa commitDBStoreAdapter) WorkingHash() []byte {
return commithash
}

func (cdsa commitDBStoreAdapter) SetCommitting(_ bool) {}

func (cdsa commitDBStoreAdapter) SetPruning(_ pruningtypes.PruningOptions) {}

// GetPruning is a no-op as pruning options cannot be directly set on this store.
Expand Down
11 changes: 11 additions & 0 deletions store/rootmulti/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,13 @@
return rs.lastCommitInfo.CommitID()
}

// SetCommitting implements Committer/CommitStore.
cool-develope marked this conversation as resolved.
Show resolved Hide resolved
func (rs *Store) SetCommitting(committing bool) {
for _, store := range rs.stores {
store.SetCommitting(committing)
}
github-advanced-security[bot] marked this conversation as resolved.
Fixed Show resolved Hide resolved
}

// Commit implements Committer/CommitStore.
func (rs *Store) Commit() types.CommitID {
var previousHeight, version int64
Expand All @@ -479,7 +486,11 @@
rs.logger.Debug("commit header and version mismatch", "header_height", rs.commitHeader.Height, "version", version)
}

// set the committing flag on all stores to block the pruning
rs.SetCommitting(true)
rs.lastCommitInfo = commitStores(version, rs.stores, rs.removalMap)
// unset the committing flag on all stores to continue the pruning
rs.SetCommitting(false)
rs.lastCommitInfo.Timestamp = rs.commitHeader.Time
defer rs.flushMetadata(rs.db, version, rs.lastCommitInfo)

Expand Down
30 changes: 20 additions & 10 deletions store/rootmulti/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,10 @@ func TestMultiStore_Pruning(t *testing.T) {
ms.Commit()
}

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

for _, v := range tc.saved {
_, err := ms.CacheMultiStoreWithVersion(v)
require.NoError(t, err, "expected no error when loading height: %d", v)
Expand Down Expand Up @@ -563,16 +567,6 @@ func TestMultiStore_Pruning_SameHeightsTwice(t *testing.T) {

require.Equal(t, numVersions, lastCommitInfo.Version)

for v := int64(1); v < numVersions-int64(keepRecent); v++ {
err := ms.LoadVersion(v)
require.Error(t, err, "expected error when loading pruned height: %d", v)
}

for v := (numVersions - int64(keepRecent)); v < numVersions; v++ {
err := ms.LoadVersion(v)
require.NoError(t, err, "expected no error when loading height: %d", v)
}

// Get latest
err := ms.LoadVersion(numVersions - 1)
require.NoError(t, err)
Expand All @@ -585,8 +579,20 @@ func TestMultiStore_Pruning_SameHeightsTwice(t *testing.T) {
require.Equal(t, numVersions, lastCommitInfo.Version)

// Ensure that can commit one more height with no panic
// and the async pruning is done
time.Sleep(500 * time.Millisecond)
cool-develope marked this conversation as resolved.
Show resolved Hide resolved
lastCommitInfo = ms.Commit()
require.Equal(t, numVersions+1, lastCommitInfo.Version)

for v := int64(1); v < numVersions-int64(keepRecent); v++ {
err := ms.LoadVersion(v)
require.Error(t, err, "expected error when loading pruned height: %d", v)
}

for v := (numVersions - int64(keepRecent)); v < numVersions; v++ {
err := ms.LoadVersion(v)
require.NoError(t, err, "expected no error when loading height: %d", v)
}
}

func TestMultiStore_PruningRestart(t *testing.T) {
Expand Down Expand Up @@ -617,6 +623,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)
cool-develope marked this conversation as resolved.
Show resolved Hide resolved
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 •
edited
Loading

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


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

for v := int64(1); v <= actualHeightToPrune; v++ {
_, err := ms.CacheMultiStoreWithVersion(v)
require.Error(t, err, "expected error when loading height: %d", v)
Expand Down
3 changes: 3 additions & 0 deletions store/transient/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ func (ts *Store) Commit() (id types.CommitID) {
return
}

// Implements CommitStoreWithPruning, no-op.
func (ts *Store) SetCommitting(_ bool) {}

func (ts *Store) SetPruning(_ pruningtypes.PruningOptions) {}

// GetPruning is a no-op as pruning options cannot be directly set on this store.
Expand Down
4 changes: 4 additions & 0 deletions store/types/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ type Committer interface {
// WorkingHash returns the hash of the KVStore's state before commit.
WorkingHash() []byte

// SetCommitting let the pruning handler know that the store is being committed
// or not, so the handler can decide to prune or not the store.
SetCommitting(isCommit bool)

SetPruning(pruningtypes.PruningOptions)
GetPruning() pruningtypes.PruningOptions
}
Expand Down
Loading