State Sync for V2 Store (ADR 40) #10705

gsk967 · 2021-12-08T13:51:34Z

Description

Snapshot Sync For New Node

Closes: #XXXX

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...

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 ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

GetVersion rename to databucket, indexbucket

- GetVersion - migration (StoreUpgrades) - flat.Store query uses getversion

erikdies

First pass notes, still needs a qualified technical review.

erikdies · 2021-12-08T13:58:11Z

CHANGELOG.md

@@ -54,6 +54,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [\#10379](https://github.com/cosmos/cosmos-sdk/pull/10379) Add validation to `x/upgrade` CLI `software-upgrade` command `--plan-info` value.
 * [\#10561](https://github.com/cosmos/cosmos-sdk/pull/10561) Add configurable IAVL cache size to app.toml
 * [\10507](https://github.com/cosmos/cosmos-sdk/pull/10507) Add middleware for tx priority.
+* [\#10430](https://github.com/cosmos/cosmos-sdk/pull/10430) ADR-040: Add store/v2 `MultiStore` implementation


Add a hash to L56 to make the markdown consistent.
* [\#10507](https://github.com/cosmos/cosmos-sdk/pull/10507) Add middleware for tx priority.

erikdies · 2021-12-08T14:14:49Z

internal/db/iterator_adapter.go

+// (store/types).Iterator interface.
+func DBToStoreIterator(source dbm.Iterator) *dbAsStoreIter {
+	ret := &dbAsStoreIter{Iterator: source}
+	ret.Next()


Why iterate here?

On the DB iterator Next and Valid were combined into Next() bool, so you have to call Next once to know if the iterator is valid

Got it. I don't know if this is us, but not a fan of that API; I would assume that we were throwing away the first value.

I agree, not my idea. The motivation was to reduce the size of the Iterator interface I think. I should have pushed against this harder, all it's done is create useless boilerplate code

erikdies · 2021-12-08T14:15:34Z

proto/cosmos/base/store/v1beta1/snapshot.proto

+// SnapshotSchema is an exported schema of store
+message SnapshotSchema{
+  bytes key = 1;
+  bytes type = 2;
 }


erikdies · 2021-12-08T14:19:45Z

store/v2/dbadapter/store_test.go

+	require.Panics(t, func() { store.Set([]byte(""), []byte("value")) }, "setting an empty key should panic")
+
+	require.Equal(t, types.StoreTypeDB, store.GetStoreType())
+	store.GetStoreType()


What is this doing?

nothing apparently, i'll remove

erikdies · 2021-12-08T14:20:09Z

store/v2/dbadapter/store_test.go

+
+var errFoo = errors.New("dummy")
+
+func TestAccessors(t *testing.T) {


This test is long, can it be broken into more understandable parts?

it's mainly long because of the mocking code. Other than that it's just simple CRUD function tests

erikdies · 2021-12-08T15:15:47Z

store/v2/root/test_util.go

+	onKey []byte
+}
+
+func (dbVersionsFails) Versions() (dbm.VersionSet, error) { return nil, errors.New("dbVersionsFails") }


Why no receiver? (Just a question - I've never seen a go method without a receiver)

This is an interface wrapper which just exists to trigger a failure, so it doesn't actually need to use the receiver object

erikdies · 2021-12-08T15:20:12Z

store/v2/transient/store.go

+	return
+}
+
+func (ts *Store) SetPruning(types.PruningOptions)  {}


Is it worth logging something here, letting the caller know they have invoked a noop?

Probably not a big deal - this is copied from the old version and shouldn't actually be called by anything. it's an extraneous part of the Committer store interface.

However, now that I think of it, we could probably remove the v2/mem and v2/transient stores altogether. They are only used as part of the root store now and just wrap the dbadapter.Store

erikdies · 2021-12-08T15:30:13Z

store/v2/root/cache_store.go

+func (cs *cacheStore) Write() {
+	for skey, sub := range cs.substores {
+		sub.Write()
+		delete(cs.substores, skey)


Why are we deleting the key here?

I was thinking that cache stores are invalidated after writing, so the substores can be cleared. But I actually misunderstood this, it seems they can be reused. So we shouldn't delete here, I'll fix

erikdies · 2021-12-08T15:38:14Z

store/v2/root/store_test.go

+		string([]byte{0x01}):       "1",
+	})
+
+	var testCase = func(t *testing.T, iter types.Iterator, expected []string) {


Can this take a description?

I could add it, but I'm not sure what a useful description would be here

erikdies · 2021-12-08T15:39:34Z

store/v2/root/store_test.go

+	opts.Pruning = types.PruneNothing
+
+	// Ensure Store's commit is rolled back in each failure case...
+	t.Run("recover after failed Commit", func(t *testing.T) {


I would like more test breakdowns like this. These r.Runs are readable and descriptive.

Ok I can look at adding more

roysc

Pretty good, mostly just need some fixes on the schema handling

roysc · 2021-12-08T16:01:08Z

proto/cosmos/base/store/v1beta1/snapshot.proto

+
+// SnapshotSchema is an exported schema of store
+message SnapshotSchema{
+  bytes key = 1;


It's probably better if we put the whole schema in one message. Also, we don't need to include the type - the only store type that forms part of the committed state is StoreTypePersistent (so it's the one only accessible for past heights). So we can assume all stores are this type when sending this and just send an array of bytes keys.

ok i will change the SnapshotSchema to send all the keys as an array of bytes keys.

roysc · 2021-12-08T16:07:35Z

store/v2/root/store.go

+			sKey := item.Schema.GetKey()
+			sValue := item.Schema.GetType()
+			if len(sValue) != 1 || !validSubStoreType(types.StoreType(sValue[0])) {
+				return sdkerrors.Wrap(err, fmt.Sprintf("invalid mapping for store key: %v => %v", item.Schema.Key, item.Schema.Type))


nit - can use sKey instead of item.Schema.Key

roysc · 2021-12-08T16:46:17Z

store/v2/root/store.go

+
+		case *storetypes.SnapshotItem_Store:
+			// set the new snapshot store schema to root-store
+			rs.schema = ret.StoreSchema


We also need to write the new schema to the DB as here - this can be done after the sync is finished.

Also, I think there should be a sanity check to make sure the schema message is received before any Store or KV messages.

@roysc we need to delete the old schema and write the new schema after the sync ? or just the write the new schema to prefixdb

We should check at the beginning that the existing schema is empty, then just write the new one - meant to link to this line

roysc · 2021-12-08T16:54:10Z

store/v2/root/store.go

 func (rs *Store) Restore(height uint64, format uint32, chunks <-chan io.ReadCloser, ready chan<- struct{}) error {
+	if height == 0 {


We should add a check this is being run on an empty store, ie. there are no saved versions and the schema is empty.

What about if we restore one version snapshot then we will receive new version snapshot right ?
so we need check only that version exists or not right ?
@roysc

snapshot should only be done for a new instance of an application to catch up the state - see here. So we don't need to worry about multiple versions being restored for now. Just need to check that the db's versions.Count() == 0 so we know it's a fresh store instance

roysc · 2021-12-08T17:03:31Z

store/v2/root/store.go

+		}
+
+		for key := range vs.schema {
+			getSubstore, err := vs.getSubstore(key)


nit, don't name the var as a verb - sstore or just sub is better

roysc · 2021-12-08T17:08:24Z

One more nit actually - can we move these functions to a new file, like root/snapshot.go?

roysc · 2021-12-08T17:13:19Z

Oh I see @erikdies reviewed a lot of stuff from #10430. On second thought @gsk967 I should have asked you to open this targeting my branch for that PR, so this only contains the diff between them. We can still change it, and change back master once mine is merged, so this is easier to read. Sorry about that.

roysc · 2021-12-14T08:48:38Z

store/v2/root/snapshot_test.go

@@ -190,6 +190,12 @@ func TestMultistoreSnapshotRestore(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, sourceSubStore, targetSubStore)
 	}
+
+	// restore checking for non-empty store
+	target2 := newMultiStoreWithBasicData(t, memdb.NewDB(), 0)


~~This does the same as above. We need to check that the restore fails if there is an existing data or schema in the target.~~

~~It would also be good to check that it fails if there's an existing saved version, even if it contains no data~~

My mistake, this checks the existing-version case, just need one for the existing-schema case

## Description Closes: #10705 State Sync for V2 Store (ADR-40) --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

## Description Closes: cosmos#10705 State Sync for V2 Store (ADR-40) --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

roysc added 30 commits December 2, 2021 14:13

add store/v2 types, RootStore

0b732a4

store/v2 dbadapter

2ffea14

store/v2 mem & transient stores

66aec81

db iterator adapter

4d1735d

flat store additions

5f8d494

GetVersion rename to databucket, indexbucket

add RootStore implementation

4afd095

- GetVersion - migration (StoreUpgrades) - flat.Store query uses getversion

rename Merkle* to StateCommitment*

3b3f63e

move store/v2/flat.* => store/v2/root.*

792e93f

update docs

46bf6a3

rm StoreTypeDecoupled

65105e7

smt.Store - raise smt.InvalidKeyError

77effa4

Combine root/store & root/root_store

2ada818

RootStore uses per-substore SMTs

fa85a72

docs + changelog

d889e07

Revert changes to prefix store

b2007bf

try to fix rocksdb test case for CI

f1caf0f

add checks to testcase

d2f0ada

nit

2ae149f

rootstore cleanup

62f1fc7

cleanup, comments

2677b2a

PR revisions - godoc, cleanup

0d5c769

tests + mocks for v2/dbadapter

35843aa

PR revisions

5c9eb81

godoc, comments

21e3745

test commit - failed revert recovery

5f6b355

update docs

856b5fa

cleanup

d28890b

godoc & code comments

f568704

PR revisions

5a53647

fix v2 mem & tran stores

1432859

roysc and others added 2 commits December 6, 2021 15:51

Merge branch 'master' into roysc/adr-040-create-rootstore

15b9b5c

feat: add the snapshot sync for v2 store

c4d6e42

github-actions bot added C:Store Type: Build labels Dec 8, 2021

erikdies reviewed Dec 8, 2021

View reviewed changes

roysc suggested changes Dec 8, 2021

View reviewed changes

Sai Kumar added 6 commits December 9, 2021 16:10

address the pr comments

9eda661

address the pr comments

551dd3b

removed prefix registry from restore

8f68ced

addressed the pr comments

b024b20

test: add test cases for snapshot for v2 store

4284df5

test: add test for checking restore for non-empty store

db80903

roysc suggested changes Dec 14, 2021

View reviewed changes

Sai Kumar added 4 commits December 14, 2021 16:25

test: add test for checking restore for store with existed schema

58e4795

address the pr comments

21715e9

address the pr comments

3431130

Merge the master

59a7cbb

gsk967 closed this Dec 16, 2021

github-actions bot added C:CLI C:x/authz C:x/gov C:x/nft T: ADR An issue or PR relating to an architectural decision record T: CI and removed Type: Build labels Dec 16, 2021

gsk967 mentioned this pull request Dec 16, 2021

feat: ADR-040: add state sync for v2 store #10794

Merged

19 tasks


		var errFoo = errors.New("dummy")

		func TestAccessors(t *testing.T) {

		func (rs *Store) Restore(height uint64, format uint32, chunks <-chan io.ReadCloser, ready chan<- struct{}) error {
		if height == 0 {

State Sync for V2 Store (ADR 40) #10705

State Sync for V2 Store (ADR 40) #10705

Conversation

gsk967 commented Dec 8, 2021

Description

Author Checklist

Reviewers Checklist

erikdies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roysc Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roysc Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roysc Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roysc commented Dec 8, 2021

roysc commented Dec 8, 2021 • edited Loading

roysc Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

roysc Dec 9, 2021 •

edited

Loading

roysc Dec 9, 2021 •

edited

Loading

roysc Dec 9, 2021 •

edited

Loading

roysc commented Dec 8, 2021 •

edited

Loading

roysc Dec 14, 2021 •

edited

Loading