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

refactor(store/v2)!: simplify storage #22683

Merged
merged 27 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
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
13 changes: 1 addition & 12 deletions crypto/keys/bls12_381/key_cgo.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@ package bls12_381

import (
"bytes"
"crypto/sha256"
"errors"
"fmt"

"github.com/cometbft/cometbft/crypto"
"github.com/cometbft/cometbft/crypto/tmhash"
"github.com/cometbft/cometbft/crypto/bls12381"
"github.com/cometbft/cometbft/crypto/tmhash"

"github.com/cosmos/cosmos-sdk/codec"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
Expand Down Expand Up @@ -84,11 +83,6 @@ func (privKey PrivKey) Sign(msg []byte) ([]byte, error) {
return nil, err
}

if len(msg) > bls12381.MaxMsgLen {
hash := sha256.Sum256(msg)
return secretKey.Sign(hash[:])
}

return secretKey.Sign(msg)
}

Expand Down Expand Up @@ -151,11 +145,6 @@ func (pubKey PubKey) VerifySignature(msg, sig []byte) bool {
return false
}

if len(msg) > bls12381.MaxMsgLen {
hash := sha256.Sum256(msg)
msg = hash[:]
}

tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
return pubK.VerifySignature(msg, sig)
}

Expand Down
1 change: 1 addition & 0 deletions runtime/v2/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ func (a *AppBuilder[T]) initGenesis(ctx context.Context, src io.Reader, txHandle
if err != nil {
return nil, fmt.Errorf("failed to read import state: %w", err)
}

var genesisJSON map[string]json.RawMessage
if err = json.Unmarshal(bz, &genesisJSON); err != nil {
return nil, err
Expand Down
6 changes: 3 additions & 3 deletions server/v2/cometbft/abci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ func TestConsensus_Query(t *testing.T) {
c := setUpConsensus(t, 100_000, cometmock.MockMempool[mock.Tx]{})

// Write data to state storage
err := c.store.GetStateStorage().ApplyChangeset(&store.Changeset{
err := c.store.GetStateCommitment().WriteChangeset(&store.Changeset{
Version: 1,
Changes: []store.StateChanges{
{
Expand Down Expand Up @@ -691,9 +691,8 @@ func setUpConsensus(t *testing.T, gasLimit uint64, mempool mempool.Mempool[mock.
)
require.NoError(t, err)

ss := cometmock.NewMockStorage(log.NewNopLogger(), t.TempDir())
sc := cometmock.NewMockCommiter(log.NewNopLogger(), string(actorName), "stf")
mockStore := cometmock.NewMockStore(ss, sc)
mockStore := cometmock.NewMockStore(sc)

am := appmanager.New(appmanager.Config{
ValidateTxGasLimit: gasLimit,
Expand Down Expand Up @@ -786,6 +785,7 @@ func TestOptimisticExecution(t *testing.T) {
Txs: ppReq.Txs,
}
fbResp, err := c.FinalizeBlock(context.Background(), fbReq)
require.Nil(t, fbResp)
require.Error(t, err)
require.ErrorContains(t, err, "test error") // from optimisticMockFunc
require.Equal(t, 1, calledTimes)
Expand Down
8 changes: 4 additions & 4 deletions server/v2/cometbft/internal/mock/mock_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func NewMockReader(v uint64, rs *MockStore, actor []byte) *MockReader {
}

func (roa *MockReader) Has(key []byte) (bool, error) {
val, err := roa.store.GetStateStorage().Has(roa.actor, roa.version, key)
val, err := roa.store.GetStateCommitment().Has(roa.actor, roa.version, key)
if err != nil {
return false, err
}
Expand All @@ -48,7 +48,7 @@ func (roa *MockReader) Has(key []byte) (bool, error) {
}

func (roa *MockReader) Get(key []byte) ([]byte, error) {
result, err := roa.store.GetStateStorage().Get(roa.actor, roa.version, key)
result, err := roa.store.GetStateCommitment().Get(roa.actor, roa.version, key)
if err != nil {
return nil, err
}
Expand All @@ -57,9 +57,9 @@ func (roa *MockReader) Get(key []byte) ([]byte, error) {
}

func (roa *MockReader) Iterator(start, end []byte) (corestore.Iterator, error) {
return roa.store.GetStateStorage().Iterator(roa.actor, roa.version, start, end)
return roa.store.GetStateCommitment().Iterator(roa.actor, roa.version, start, end)
}

func (roa *MockReader) ReverseIterator(start, end []byte) (corestore.Iterator, error) {
return roa.store.GetStateStorage().ReverseIterator(roa.actor, roa.version, start, end)
return roa.store.GetStateCommitment().ReverseIterator(roa.actor, roa.version, start, end)
}
24 changes: 3 additions & 21 deletions server/v2/cometbft/internal/mock/mock_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,12 @@ import (
"cosmossdk.io/store/v2/commitment/iavl"
dbm "cosmossdk.io/store/v2/db"
"cosmossdk.io/store/v2/proof"
"cosmossdk.io/store/v2/storage"
"cosmossdk.io/store/v2/storage/pebbledb"
)

type MockStore struct {
Storage storev2.VersionedWriter
Committer storev2.Committer
}

func NewMockStorage(logger log.Logger, dir string) storev2.VersionedWriter {
storageDB, _ := pebbledb.New(dir)
ss := storage.NewStorageStore(storageDB, logger)
return ss
}

func NewMockCommiter(logger log.Logger, actors ...string) storev2.Committer {
treeMap := make(map[string]commitment.Tree)
for _, actor := range actors {
Expand All @@ -36,8 +27,8 @@ func NewMockCommiter(logger log.Logger, actors ...string) storev2.Committer {
return sc
}

func NewMockStore(ss storev2.VersionedWriter, sc storev2.Committer) *MockStore {
return &MockStore{Storage: ss, Committer: sc}
func NewMockStore(sc storev2.Committer) *MockStore {
return &MockStore{Committer: sc}
}

func (s *MockStore) GetLatestVersion() (uint64, error) {
Expand All @@ -59,12 +50,7 @@ func (s *MockStore) StateLatest() (uint64, corestore.ReaderMap, error) {
}

func (s *MockStore) Commit(changeset *corestore.Changeset) (corestore.Hash, error) {
err := s.Storage.ApplyChangeset(changeset)
if err != nil {
return []byte{}, err
}

err = s.Committer.WriteChangeset(changeset)
err := s.Committer.WriteChangeset(changeset)
if err != nil {
return []byte{}, err
}
Expand All @@ -81,10 +67,6 @@ func (s *MockStore) StateAt(version uint64) (corestore.ReaderMap, error) {
return NewMockReaderMap(version, s), nil
}

func (s *MockStore) GetStateStorage() storev2.VersionedWriter {
return s.Storage
}

func (s *MockStore) GetStateCommitment() storev2.Committer {
return s.Committer
}
Expand Down
2 changes: 0 additions & 2 deletions server/v2/cometbft/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ func New[T transaction.Tx](
indexEvents[e] = struct{}{}
}

ss := store.GetStateStorage().(snapshots.StorageSnapshotter)
sc := store.GetStateCommitment().(snapshots.CommitSnapshotter)

snapshotStore, err := GetSnapshotStore(srv.config.ConfigTomlConfig.RootDir)
Expand Down Expand Up @@ -155,7 +154,6 @@ func New[T transaction.Tx](
snapshotStore,
srv.serverOptions.SnapshotOptions(cfg),
sc,
ss,
nil, // extensions snapshotter registered below
logger,
)
Expand Down
1 change: 1 addition & 0 deletions server/v2/stf/branch/bench_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ func Benchmark_Iterate(b *testing.B) {

// makeBranchStack creates a branch stack of the given size and initializes it with unique key-value pairs.
func makeBranchStack(b *testing.B, stackSize int) Store[store.KVStore] {
b.Helper()
parent := coretesting.NewMemKV()
branch := NewStore[store.KVStore](parent)
for i := 1; i < stackSize; i++ {
Expand Down
7 changes: 4 additions & 3 deletions server/v2/store/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,10 +375,11 @@ func createSnapshotsManager(
}

sm := snapshots.NewManager(
snapshotStore, snapshots.NewSnapshotOptions(interval, uint32(keepRecent)),
snapshotStore,
snapshots.NewSnapshotOptions(interval, uint32(keepRecent)),
store.GetStateCommitment().(snapshots.CommitSnapshotter),
store.GetStateStorage().(snapshots.StorageSnapshotter),
nil, logger)
nil,
logger)
return sm, nil
}

Expand Down
2 changes: 0 additions & 2 deletions store/iavl/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,6 @@ func TestLoadStore(t *testing.T) {
cIDHp := types.CommitID{Version: verHp, Hash: hash}
require.Nil(t, err)

// TODO: Prune this height

// Create current height Hc
updated, err = tree.Set([]byte("hello"), []byte("ciao"))
require.NoError(t, err)
Expand Down
30 changes: 30 additions & 0 deletions store/v2/commitment/iavl/tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,16 @@ func (t *IavlTree) Commit() ([]byte, uint64, error) {

// GetProof returns a proof for the given key and version.
func (t *IavlTree) GetProof(version uint64, key []byte) (*ics23.CommitmentProof, error) {
// the mutable tree is empty at genesis & when the key is removed, but the immutable tree is not
// by checking the latest version we can determine if we are in genesis or have a key that has been removed
lv, err := t.tree.GetLatestVersion()
if err != nil {
return nil, err
}
if lv == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

couldn't genesis be done at a height other than 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

when starting a node the version of the tree will always be 0, since its empty and if the root is nil the version is assumed to be 0, but when you write (apply change sets) it would be on the height/version you specify.

If you remove a store key the values are still there, unless pruned. In that case the mutable tree will nil because the version of the tree could be 30 but the key was removed at 20. Clients may still want to query version(s) <20. In this case the latest version of the tree will be 20 not 30.

return t.tree.GetProof(key)
}

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)
Expand All @@ -93,6 +103,16 @@ func (t *IavlTree) GetProof(version uint64, key []byte) (*ics23.CommitmentProof,

// Get implements the Reader interface.
func (t *IavlTree) Get(version uint64, key []byte) ([]byte, error) {
// the mutable tree is empty at genesis & when the key is removed, but the immutable tree is not
Copy link
Member

Choose a reason for hiding this comment

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

do you mean to say that t.tree.Get(key will return nil when key is absent? what's special about genesis?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry meant storekey, the above comment explains the flow

// by checking the latest version we can determine if we are in genesis or have a key that has been removed
lv, err := t.tree.GetLatestVersion()
if err != nil {
return nil, err
}
if lv == 0 {
return t.tree.Get(key)
}

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)
Expand All @@ -103,6 +123,16 @@ func (t *IavlTree) Get(version uint64, key []byte) ([]byte, error) {

// Iterator implements the Reader interface.
func (t *IavlTree) Iterator(version uint64, start, end []byte, ascending bool) (corestore.Iterator, error) {
// the mutable tree is empty at genesis & when the key is removed, but the immutable tree is not
// by checking the latest version we can determine if we are in genesis or have a key that has been removed
lv, err := t.tree.GetLatestVersion()
if err != nil {
return nil, err
}
if lv == 0 {
return t.tree.Iterator(start, end, ascending)
}

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)
Expand Down
32 changes: 15 additions & 17 deletions store/v2/commitment/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ func (c *CommitStore) SetInitialVersion(version uint64) error {
return nil
}

// GetProof returns a proof for the given key and version.
func (c *CommitStore) GetProof(storeKey []byte, version uint64, key []byte) ([]proof.CommitmentOp, error) {
rawStoreKey := conv.UnsafeBytesToStr(storeKey)
tree, ok := c.multiTrees[rawStoreKey]
Expand Down Expand Up @@ -268,8 +269,12 @@ func (c *CommitStore) GetProof(storeKey []byte, version uint64, key []byte) ([]p
// WARNING: This function is only used during the migration process. The SC layer
// generally does not provide a reader for the CommitStore.
func (c *CommitStore) getReader(storeKey string) (Reader, error) {
tree, ok := c.multiTrees[storeKey]
if !ok {
var tree Tree
if storeTree, ok := c.oldTrees[storeKey]; ok {
tree = storeTree
} else if storeTree, ok := c.multiTrees[storeKey]; ok {
tree = storeTree
} else {
return nil, fmt.Errorf("store %s not found", storeKey)
}

Expand All @@ -283,6 +288,14 @@ func (c *CommitStore) getReader(storeKey string) (Reader, error) {

// VersionExists implements store.VersionedReader.
func (c *CommitStore) VersionExists(version uint64) (bool, error) {
latestVersion, err := c.metadata.GetLatestVersion()
if err != nil {
return false, err
}
if latestVersion == 0 {
return true, nil
Copy link
Member

Choose a reason for hiding this comment

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

why does version 0 need special casing?

Copy link
Member Author

Choose a reason for hiding this comment

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

its meant to treat the flow as if there is no key set then its not found but we would still want to return true here

}

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

Potential Logic Error in VersionExists Method

In the VersionExists method, when latestVersion == 0, the method returns true regardless of the input version. This could incorrectly indicate that any version exists when, in fact, no versions may have been committed yet.

Consider adjusting the logic to accurately reflect the existence of the specified version:

	if latestVersion == 0 {
-		return true, nil
+		return version == 0, nil
	}

This change ensures that the method only returns true when checking for version 0 when no versions have been committed, aligning with expected behavior.

📝 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
latestVersion, err := c.metadata.GetLatestVersion()
if err != nil {
return false, err
}
if latestVersion == 0 {
return true, nil
}
latestVersion, err := c.metadata.GetLatestVersion()
if err != nil {
return false, err
}
if latestVersion == 0 {
return version == 0, nil
}

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

Potential incorrect behavior when latestVersion is zero in VersionExists method

When latestVersion is zero, the VersionExists method returns true for any version. This may not be accurate, as no versions exist yet.

Consider modifying the logic to return true only when version == 0:

func (c *CommitStore) VersionExists(version uint64) (bool, error) {
	latestVersion, err := c.metadata.GetLatestVersion()
	if err != nil {
		return false, err
	}
	if latestVersion == 0 {
-		return true, nil
+		return version == 0, nil
	}

	ci, err := c.metadata.GetCommitInfo(version)
	return ci != nil, err
}
📝 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
latestVersion, err := c.metadata.GetLatestVersion()
if err != nil {
return false, err
}
if latestVersion == 0 {
return true, nil
}
latestVersion, err := c.metadata.GetLatestVersion()
if err != nil {
return false, err
}
if latestVersion == 0 {
return version == 0, nil
}

ci, err := c.metadata.GetCommitInfo(version)
return ci != nil, err
}
Expand Down Expand Up @@ -435,12 +448,10 @@ func (c *CommitStore) Restore(
version uint64,
format uint32,
protoReader protoio.Reader,
chStorage chan<- *corestore.StateChanges,
) (snapshotstypes.SnapshotItem, error) {
var (
importer Importer
snapshotItem snapshotstypes.SnapshotItem
storeKey []byte
)

loop:
Expand All @@ -463,8 +474,6 @@ loop:
return snapshotstypes.SnapshotItem{}, fmt.Errorf("failed to close importer: %w", err)
}
}

storeKey = []byte(item.Store.Name)
tree := c.multiTrees[item.Store.Name]
if tree == nil {
return snapshotstypes.SnapshotItem{}, fmt.Errorf("store %s not found", item.Store.Name)
Expand Down Expand Up @@ -493,17 +502,6 @@ loop:
if node.Value == nil {
node.Value = []byte{}
}

// If the node is a leaf node, it will be written to the storage.
chStorage <- &corestore.StateChanges{
Actor: storeKey,
StateChanges: []corestore.KVPair{
{
Key: node.Key,
Value: node.Value,
},
},
}
}
err := importer.Add(node)
if err != nil {
Expand Down
Loading
Loading