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/v2): route to the commitment during migration #22290

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
12 changes: 12 additions & 0 deletions store/v2/commitment/iavl/tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

var (
_ commitment.Tree = (*IavlTree)(nil)
_ commitment.Reader = (*IavlTree)(nil)
_ store.PausablePruner = (*IavlTree)(nil)
)

Expand Down Expand Up @@ -76,6 +77,7 @@ func (t *IavlTree) GetProof(version uint64, key []byte) (*ics23.CommitmentProof,
return immutableTree.GetProof(key)
}

// Get implements the Reader interface.
func (t *IavlTree) Get(version uint64, key []byte) ([]byte, error) {
immutableTree, err := t.tree.GetImmutable(int64(version))
if err != nil {
Expand All @@ -85,6 +87,16 @@ func (t *IavlTree) Get(version uint64, key []byte) ([]byte, error) {
return immutableTree.Get(key)
}

// Iterator implements the Reader interface.
func (t *IavlTree) Iterator(version uint64, start, end []byte, ascending bool) (corestore.Iterator, error) {
immutableTree, err := t.tree.GetImmutable(int64(version))
if err != nil {
return nil, fmt.Errorf("failed to get immutable tree at version %d: %w", version, err)
}

return immutableTree.Iterator(start, end, ascending)
}

// GetLatestVersion returns the latest version of the tree.
func (t *IavlTree) GetLatestVersion() (uint64, error) {
v, err := t.tree.GetLatestVersion()
Expand Down
58 changes: 54 additions & 4 deletions store/v2/commitment/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ var (
_ store.UpgradeableStore = (*CommitStore)(nil)
_ snapshots.CommitSnapshotter = (*CommitStore)(nil)
_ store.PausablePruner = (*CommitStore)(nil)

// NOTE: CommitStore implements store.VersionedReader, but it is not used in the
// store v2. It is only used during the migration process.
_ store.VersionedReader = (*CommitStore)(nil)
)

// MountTreeFn is a function that mounts a tree given a store key.
Expand Down Expand Up @@ -275,21 +279,67 @@ func (c *CommitStore) GetProof(storeKey []byte, version uint64, key []byte) ([]p
return []proof.CommitmentOp{commitOp, *storeCommitmentOp}, nil
}

// Get implements store.VersionedReader.
func (c *CommitStore) Get(storeKey []byte, version uint64, key []byte) ([]byte, error) {
tree, ok := c.multiTrees[conv.UnsafeBytesToStr(storeKey)]
func (c *CommitStore) getReader(storeKey string) (Reader, error) {
tree, ok := c.multiTrees[storeKey]
if !ok {
return nil, fmt.Errorf("store %s not found", storeKey)
}

bz, err := tree.Get(version, key)
reader, ok := tree.(Reader)
if !ok {
return nil, errors.New("tree does not implement Reader")
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error message for better context

Including the storeKey in the error message when the tree does not implement Reader can provide more context for debugging.

Suggested change:

- return nil, errors.New("tree does not implement Reader")
+ return nil, fmt.Errorf("tree for store %s does not implement Reader", storeKey)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return nil, errors.New("tree does not implement Reader")
return nil, fmt.Errorf("tree for store %s does not implement Reader", storeKey)

}

return reader, nil
}
Comment on lines +282 to +294
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error message for better context

The getReader method is well-implemented and improves code reusability. However, the error message when the tree doesn't implement Reader could be more informative.

Consider including the storeKey in the error message:

- return nil, errors.New("tree does not implement Reader")
+ return nil, fmt.Errorf("tree for store %s does not implement Reader", storeKey)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (c *CommitStore) getReader(storeKey string) (Reader, error) {
tree, ok := c.multiTrees[storeKey]
if !ok {
return nil, fmt.Errorf("store %s not found", storeKey)
}
bz, err := tree.Get(version, key)
reader, ok := tree.(Reader)
if !ok {
return nil, errors.New("tree does not implement Reader")
}
return reader, nil
}
func (c *CommitStore) getReader(storeKey string) (Reader, error) {
tree, ok := c.multiTrees[storeKey]
if !ok {
return nil, fmt.Errorf("store %s not found", storeKey)
}
reader, ok := tree.(Reader)
if !ok {
return nil, fmt.Errorf("tree for store %s does not implement Reader", storeKey)
}
return reader, nil
}


// VersionExists implements store.VersionedReader.
func (c *CommitStore) VersionExists(version uint64) (bool, error) {
ci, err := c.metadata.GetCommitInfo(version)
return ci != nil, err
}

// Get implements store.VersionedReader.
func (c *CommitStore) Get(storeKey []byte, version uint64, key []byte) ([]byte, error) {
reader, err := c.getReader(conv.UnsafeBytesToStr(storeKey))
if err != nil {
return nil, err
}

bz, err := reader.Get(version, key)
if err != nil {
return nil, fmt.Errorf("failed to get key %s from store %s: %w", key, storeKey, err)
}

return bz, nil
}

// Has implements store.VersionedReader.
func (c *CommitStore) Has(storeKey []byte, version uint64, key []byte) (bool, error) {
val, err := c.Get(storeKey, version, key)
return len(val) > 0, err
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct. Empty slice is an supported value but not nil

Suggested change
return len(val) > 0, err
return val != nil, err

You can use this test to verify:

func (s *CommitStoreTestSuite) TestStore_Has() {
	storeKeys := []string{storeKey1}
	myKey := []byte("myKey")
	const initialVersion = 1

	specs := map[string]struct {
		src          *corestore.Changeset
		queryVersion uint64
		expExists    bool
		expErr       bool
	}{
		"known key with some value": {
			src: corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{
				storeKey1: {{Key: myKey, Value: []byte("my-value")}},
			}),
			queryVersion: initialVersion,
			expExists:    true,
		},
		"known key with empty value": {
			src: corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{
				storeKey1: {{Key: myKey, Value: []byte("")}},
			}),
			queryVersion: initialVersion,
			expExists:    true,
		},
		"unknown key": {
			src:          corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{}),
			queryVersion: initialVersion,
			expExists:    false,
		},
		"unknown version": {
			src: corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{
				storeKey1: {{Key: myKey, Value: []byte("")}},
			}),
			queryVersion: initialVersion + 1,
			expErr:       true,
		},
	}
	for name, spec := range specs {
		s.T().Run(name, func(t *testing.T) {
			commitStore, err := s.NewStore(dbm.NewMemDB(), storeKeys, nil, coretesting.NewNopLogger())
			require.NoError(t, err)
			require.NoError(t, commitStore.WriteChangeset(spec.src))
			_, err = commitStore.Commit(initialVersion)
			require.NoError(t, err)

			// when
			gotResult, gotErr := commitStore.Has([]byte(storeKey1), spec.queryVersion, myKey)

			// then
			if spec.expErr {
				require.Error(t, gotErr)
				return
			}
			require.NoError(t, gotErr)
			require.Equal(t, spec.expExists, gotResult)
		})
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, let me try to add this test case!

Copy link
Contributor

Choose a reason for hiding this comment

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

}

// Iterator implements store.VersionedReader.
func (c *CommitStore) Iterator(storeKey []byte, version uint64, start, end []byte) (corestore.Iterator, error) {
reader, err := c.getReader(conv.UnsafeBytesToStr(storeKey))
if err != nil {
return nil, err
}

return reader.Iterator(version, start, end, true)
}

// ReverseIterator implements store.VersionedReader.
func (c *CommitStore) ReverseIterator(storeKey []byte, version uint64, start, end []byte) (corestore.Iterator, error) {
reader, err := c.getReader(conv.UnsafeBytesToStr(storeKey))
if err != nil {
return nil, err
}

return reader.Iterator(version, start, end, false)
}

// Prune implements store.Pruner.
func (c *CommitStore) Prune(version uint64) error {
// prune the metadata
Expand Down
14 changes: 8 additions & 6 deletions store/v2/commitment/tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

ics23 "github.com/cosmos/ics23/go"

corestore "cosmossdk.io/core/store"
snapshotstypes "cosmossdk.io/store/v2/snapshots/types"
)

Expand All @@ -29,19 +30,20 @@ type Tree interface {
SetInitialVersion(version uint64) error
GetProof(version uint64, key []byte) (*ics23.CommitmentProof, error)

// Get attempts to retrieve a value from the tree for a given version.
//
// NOTE: This method only exists to support migration from IAVL v0/v1 to v2.
// Once migration is complete, this method should be removed and/or not used.
Get(version uint64, key []byte) ([]byte, error)

Prune(version uint64) error
Export(version uint64) (Exporter, error)
Import(version uint64) (Importer, error)

io.Closer
}

// Reader is the optional interface that is only used to read data from the tree
// during the migration process.
type Reader interface {
Get(version uint64, key []byte) ([]byte, error)
Iterator(version uint64, start, end []byte, ascending bool) (corestore.Iterator, error)
}

Comment on lines +40 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider the impact of removing Get from Tree interface on existing implementations.

By removing the Get method from the Tree interface and introducing the Reader interface, existing code that relies on Tree.Get may break. Ensure that all implementations and usages of Tree are updated to reflect this change, and consider providing guidance or deprecation notices to assist with the migration.

// Exporter is the interface that wraps the basic Export methods.
type Exporter interface {
Next() (*snapshotstypes.SnapshotIAVLItem, error)
Expand Down
4 changes: 0 additions & 4 deletions store/v2/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@ type VersionedReader interface {

Iterator(storeKey []byte, version uint64, start, end []byte) (corestore.Iterator, error)
ReverseIterator(storeKey []byte, version uint64, start, end []byte) (corestore.Iterator, error)

// Close releases associated resources. It should NOT be idempotent. It must
// only be called once and any call after may panic.
io.Closer
}

// UpgradableDatabase defines an API for a versioned database that allows pruning
Expand Down
34 changes: 17 additions & 17 deletions store/v2/root/reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,38 +14,38 @@ var (
// operations. This is useful for exposing a read-only view of the RootStore at
// a specific version in history, which could also be the latest state.
type ReaderMap struct {
rootStore store.RootStore
version uint64
vReader store.VersionedReader
version uint64
}

func NewReaderMap(v uint64, rs store.RootStore) *ReaderMap {
func NewReaderMap(v uint64, vr store.VersionedReader) *ReaderMap {
return &ReaderMap{
rootStore: rs,
version: v,
vReader: vr,
version: v,
Comment on lines +21 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use descriptive parameter names for clarity.

In the constructors NewReaderMap and NewReader, the parameters v and vr are abbreviated and may reduce readability. According to the Uber Go Style Guide, parameter names should be self-explanatory. Consider renaming v to version and vr to versionedReader for better clarity.

Apply this diff to update the parameter names:

-func NewReaderMap(v uint64, vr store.VersionedReader) *ReaderMap {
+func NewReaderMap(version uint64, versionedReader store.VersionedReader) *ReaderMap {
	return &ReaderMap{
-		vReader: vr,
-		version: v,
+		vReader: versionedReader,
+		version: version,
	}
}

-func NewReader(v uint64, vr store.VersionedReader, actor []byte) *Reader {
+func NewReader(version uint64, versionedReader store.VersionedReader, actor []byte) *Reader {
	return &Reader{
-		version: v,
-		vReader: vr,
+		version: version,
+		vReader: versionedReader,
		actor:   actor,
	}
}

Also applies to: 39-43

}
}

func (roa *ReaderMap) GetReader(actor []byte) (corestore.Reader, error) {
return NewReader(roa.version, roa.rootStore, actor), nil
return NewReader(roa.version, roa.vReader, actor), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use conventional receiver names for consistency.

The receiver name roa is used for both ReaderMap and Reader methods, which can be confusing. As per Go conventions and the Uber Go Style Guide, receiver names should be a concise abbreviation of the type name. Consider using rm for ReaderMap and r for Reader to enhance code readability.

Apply this diff to update the receiver names:

-func (roa *ReaderMap) GetReader(actor []byte) (corestore.Reader, error) {
+func (rm *ReaderMap) GetReader(actor []byte) (corestore.Reader, error) {
-	return NewReader(roa.version, roa.vReader, actor), nil
+	return NewReader(rm.version, rm.vReader, actor), nil
}

-func (roa *Reader) Has(key []byte) (bool, error) {
+func (r *Reader) Has(key []byte) (bool, error) {
-	val, err := roa.vReader.Has(roa.actor, roa.version, key)
+	val, err := r.vReader.Has(r.actor, r.version, key)
	if err != nil {
		return false, err
	}
	return val, nil
}

-func (roa *Reader) Get(key []byte) ([]byte, error) {
+func (r *Reader) Get(key []byte) ([]byte, error) {
-	return roa.vReader.Get(roa.actor, roa.version, key)
+	return r.vReader.Get(r.actor, r.version, key)
}

-func (roa *Reader) Iterator(start, end []byte) (corestore.Iterator, error) {
+func (r *Reader) Iterator(start, end []byte) (corestore.Iterator, error) {
-	return roa.vReader.Iterator(roa.actor, roa.version, start, end)
+	return r.vReader.Iterator(r.actor, r.version, start, end)
}

-func (roa *Reader) ReverseIterator(start, end []byte) (corestore.Iterator, error) {
+func (r *Reader) ReverseIterator(start, end []byte) (corestore.Iterator, error) {
-	return roa.vReader.ReverseIterator(roa.actor, roa.version, start, end)
+	return r.vReader.ReverseIterator(r.actor, r.version, start, end)
}

Also applies to: 48-48, 57-57, 61-61, 65-65

}

// Reader represents a read-only adapter for accessing data from the root store.
type Reader struct {
version uint64 // The version of the data.
rootStore store.RootStore // The root store to read data from.
actor []byte // The actor associated with the data.
version uint64 // The version of the data.
vReader store.VersionedReader // The root store to read data from.
actor []byte // The actor associated with the data.
}

func NewReader(v uint64, rs store.RootStore, actor []byte) *Reader {
func NewReader(v uint64, vr store.VersionedReader, actor []byte) *Reader {
return &Reader{
version: v,
rootStore: rs,
actor: actor,
version: v,
vReader: vr,
actor: actor,
}
}

func (roa *Reader) Has(key []byte) (bool, error) {
val, err := roa.rootStore.GetStateStorage().Has(roa.actor, roa.version, key)
val, err := roa.vReader.Has(roa.actor, roa.version, key)
if err != nil {
return false, err
}
Expand All @@ -54,13 +54,13 @@ func (roa *Reader) Has(key []byte) (bool, error) {
}

func (roa *Reader) Get(key []byte) ([]byte, error) {
return roa.rootStore.GetStateStorage().Get(roa.actor, roa.version, key)
return roa.vReader.Get(roa.actor, roa.version, key)
}

func (roa *Reader) Iterator(start, end []byte) (corestore.Iterator, error) {
return roa.rootStore.GetStateStorage().Iterator(roa.actor, roa.version, start, end)
return roa.vReader.Iterator(roa.actor, roa.version, start, end)
}

func (roa *Reader) ReverseIterator(start, end []byte) (corestore.Iterator, error) {
return roa.rootStore.GetStateStorage().ReverseIterator(roa.actor, roa.version, start, end)
return roa.vReader.ReverseIterator(roa.actor, roa.version, start, end)
}
43 changes: 31 additions & 12 deletions store/v2/root/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,27 +118,46 @@ func (s *Store) SetInitialVersion(v uint64) error {
return s.stateCommitment.SetInitialVersion(v)
}

func (s *Store) StateLatest() (uint64, corestore.ReaderMap, error) {
v, err := s.GetLatestVersion()
func (s *Store) getVersionedReader(version uint64) (store.VersionedReader, error) {
Copy link
Member

Choose a reason for hiding this comment

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

go doc pls

isExist, err := s.stateStorage.VersionExists(version)
if err != nil {
return 0, nil, err
return nil, err
}
if isExist {
return s.stateStorage, nil
}

return v, NewReaderMap(v, s), nil
if vReader, ok := s.stateCommitment.(store.VersionedReader); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding the interface to the Committer interface so that you can get rid of the type cast?

type Committer interface {
	VersionedReader
...

Copy link
Member

Choose a reason for hiding this comment

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

we dont want to force commitment structures to implement iteration. This is here because of iavl mainly. If iavl didnt have iteration then this wouldnt be needed because iteration wouldnt be used in the state machine like today

isExist, err := vReader.VersionExists(version)
if err != nil {
return nil, err
}
if isExist {
return vReader, nil
}
}

return nil, fmt.Errorf("version %d does not exist", version)
Copy link
Contributor

Choose a reason for hiding this comment

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

note: this error is also returned when the type cast failed.

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, it is expected

}

// StateAt checks if the requested version is present in ss.
func (s *Store) StateAt(v uint64) (corestore.ReaderMap, error) {
// check if version is present in state storage
isExist, err := s.stateStorage.VersionExists(v)
func (s *Store) StateLatest() (uint64, corestore.ReaderMap, error) {
v, err := s.GetLatestVersion()
if err != nil {
return nil, err
return 0, nil, err
}
if !isExist {
return nil, fmt.Errorf("version %d does not exist", v)

vReader, err := s.getVersionedReader(v)
if err != nil {
return 0, nil, err
}

return NewReaderMap(v, s), nil
return v, NewReaderMap(v, vReader), nil
}

// StateAt returns a read-only view of the state at a given version.
func (s *Store) StateAt(v uint64) (corestore.ReaderMap, error) {
vReader, err := s.getVersionedReader(v)
return NewReaderMap(v, vReader), err
}

func (s *Store) GetStateStorage() store.VersionedWriter {
Expand Down
Loading