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

gsk967 · 2021-12-16T14:12:27Z

Description

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

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)

…ync_v2_store

roysc

LGTM, except the whitespace changes. (Note - reviewed internally at vulcanize#18)

docs/core/proto-docs.md

store/v2/multi/snapshot.go

i-norden

A few small changes requested

store/v2/multi/snapshot.go

store/v2/multi/snapshot_test.go

…cosmos-sdk into sai/10194_state_sync_v2_store

gsk967 · 2022-01-12T14:22:44Z

@i-norden Can you please review again, I addressed the pr comments

i-norden

LGTM Sai!

robert-zaremba

let's find a way to reduce copy-paste

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

store/v2/multi/snapshot.go

…elines

roysc · 2022-02-25T13:46:47Z

store/v2/multi/snapshot.go

+		return sdkerrors.Wrapf(err, "error while getting the snapshot versions at height %v", height)
+	}
+	if !versions.Exists(height) {
+		return sdkerrors.Wrapf(sdkerrors.ErrNotFound, "cannot find snapshot at height %v", height)


this will be covered by an error from getView, we actually don't need to access statedb.Versions() at all in these tests

roysc · 2022-02-25T13:47:31Z

store/v2/multi/snapshot.go

+	// get the saved snapshot at height
+	vs, err := rs.getView(int64(height))
+	if err != nil {
+		return sdkerrors.Wrap(err, fmt.Sprintf("error while get the version at height %d", height))


let's add a case to cover this error

roysc · 2022-02-25T13:50:22Z

store/v2/multi/snapshot.go

+		return strings.Compare(sKeys[i], sKeys[j]) == -1
+	})
+
+	var storeByteKeys [][]byte


could just use this and remove skeys

roysc · 2022-02-25T13:58:33Z

store/v2/multi/snapshot.go

+	if err != nil {
+		return snapshottypes.SnapshotItem{}, sdkerrors.Wrapf(err, "error while getting the snapshot versions at height %v", height)
+	}
+	if versions.Count() != 0 {


Instead, better to check LastCommitID().Version == 0, then we can remove versions.

And lets add a testcase for this

roysc · 2022-02-25T14:21:46Z

store/v2/multi/snapshot.go

+
+		switch item := snapshotItem.Item.(type) {
+		case *snapshottypes.SnapshotItem_Schema:
+			if len(rs.schema) != 0 {


We will actually need to change this behavior. StoreKeys are registered by the app at init, and exist as unique objects referenced only from modules that have access to them. (I recently corrected the v2 store to follow this pattern.)

So, the store needs to have a schema containing all the stores received by the snapshot before the snapshot is received - this is the current behavior for the current MultiStore.

So here, instead of overwriting the store's schema, we'll just check that it matches the one received.

roysc

Looks good. It would be good to add some more failure test cases, like out-of-order messages. But no need to block on that.

roysc · 2022-02-25T16:46:49Z

store/v2/multi/snapshot.go

-					return snapshottypes.SnapshotItem{}, sdkerrors.Wrap(err, "error at set the store schema key values")
-				}
+			if !rs.schema.equal(receivedStoreSchema) {
+				return snapshottypes.SnapshotItem{}, sdkerrors.Wrap(sdkerrors.ErrLogic, "received store schema is not matched with app schema empty")


"received schema does not match app schema"

yihuang · 2022-02-26T03:41:38Z

snapshots/chunk.go

+
+// ValidRestoreHeight will check height is valid for snapshot restore or not
+func ValidRestoreHeight(format uint32, height uint64) error {
+	if format != snapshottypes.CurrentFormat {


I think we should increase the format number?

Right, I forgot to mention this. I wonder if we should also change it for the snapshot extensions support?

but CurrentFormat is static , may be we need to change the CurrentFormat is configurable

snapshot extensions feature is backward compatible if the app doesn't actually register any extensions, so I think it's good there.

but CurrentFormat is static , may be we need to change the CurrentFormat is configurable

Just bump the value of the constant? it's for the receiver to fail fast when receiving an unsupported format.

@gsk967 , could you go thorough the comments and resolve them?

done

yihuang · 2022-02-26T03:46:16Z

store/v2/multi/snapshot.go

+			subStore.Set(item.KV.Key, item.KV.Value)
+
+		default:
+			return snapshottypes.SnapshotItem{}, sdkerrors.Wrapf(sdkerrors.ErrLogic, "unknown snapshot item %T", item)


When see an unknown item, we should treat it as a success, and return that unknown item to the snapshot manager, because that could be the beginning of the next extension snapshotter.

I fixed a similar issue in rootmulti store's Restore implementation: #11286

Thanks i fixed that one now , can review again changes

yihuang

When restoration sees an unknown item, it should treat it as the ending of the current snapshot, and return that item to the snapshot manager, because it could be the beginning of the next extension snapshot.

gsk967 · 2022-02-28T09:28:12Z

@yihuang @roysc Guys can you look into changes , i addressed the pr review comments

yihuang

LGTM

robert-zaremba · 2022-02-28T13:54:55Z

@gsk967 , could you go thorough the comments and resolve them?

gsk967 · 2022-02-28T14:16:40Z

@robert-zaremba done

Ian/10194 state sync v2 store

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

feat: add state sync for v2 store

cb239cf

gsk967 requested review from aaronc and alexanderbez as code owners December 16, 2021 14:12

update the changelog

7d0c571

github-actions bot added the C:Store label Dec 16, 2021

gsk967 mentioned this pull request Dec 16, 2021

feat: add the snapshot sync for v2 store vulcanize/cosmos-sdk#18

Closed

19 tasks

Sai Kumar added 3 commits December 16, 2021 21:14

update the changelog

b662da1

revert the changelog

d7aa01d

revert the changelog

8370ef2

i-norden self-requested a review December 16, 2021 16:10

i-norden changed the title ~~feat: add state sync for v2 store~~ feat: ADR-040: add state sync for v2 store Dec 16, 2021

i-norden mentioned this pull request Dec 17, 2021

ADR-040 Implementation: Support state sync #10194

Closed

5 tasks

Sai Kumar and others added 2 commits December 20, 2021 15:37

Merge remote-tracking branch 'upstream/master' into sai/10194_state_s…

2f65306

…ync_v2_store

Merge branch 'master' into sai/10194_state_sync_v2_store

91a433b

i-norden mentioned this pull request Dec 21, 2021

ADR-040 Implementation #9816

Closed

38 tasks

i-norden requested a review from robert-zaremba January 5, 2022 14:36

roysc approved these changes Jan 12, 2022

View reviewed changes

docs/core/proto-docs.md Outdated Show resolved Hide resolved

store/v2/multi/snapshot.go Outdated Show resolved Hide resolved

i-norden reviewed Jan 12, 2022

View reviewed changes

store/v2/multi/snapshot.go Outdated Show resolved Hide resolved

store/v2/multi/snapshot.go Outdated Show resolved Hide resolved

i-norden reviewed Jan 12, 2022

View reviewed changes

store/v2/multi/snapshot_test.go Outdated Show resolved Hide resolved

Sai Kumar added 3 commits January 12, 2022 19:43

chore: address the pr comments

172b628

Merge branch 'sai/10194_state_sync_v2_store' of github.com:vulcanize/…

689b695

…cosmos-sdk into sai/10194_state_sync_v2_store

Merge upstream/master

57ad95b

gsk967 requested a review from i-norden January 12, 2022 14:22

roysc mentioned this pull request Jan 20, 2022

feat!: ADR-040: Refactor App to use v2.MultiStore #10174

Closed

19 tasks

i-norden approved these changes Jan 25, 2022

View reviewed changes

Merge the remote upstream/master

54253f3

robert-zaremba reviewed Jan 27, 2022

View reviewed changes

proto/cosmos/base/store/v1beta1/snapshot.proto Outdated Show resolved Hide resolved

proto/cosmos/base/store/v1beta1/snapshot.proto Outdated Show resolved Hide resolved

store/v2/multi/snapshot.go Outdated Show resolved Hide resolved

store/v2/multi/snapshot.go Outdated Show resolved Hide resolved

gsk967 and others added 2 commits January 28, 2022 12:46

Merge branch 'master' into sai/10194_state_sync_v2_store

2bbde5a

refactor: add common functions for snapshot restore,export stream pip…

00681c1

…elines

roysc suggested changes Feb 25, 2022

View reviewed changes

chore: address the review comments

b6cde03

roysc approved these changes Feb 25, 2022

View reviewed changes

yihuang reviewed Feb 26, 2022

View reviewed changes

yihuang requested changes Feb 26, 2022

View reviewed changes

gsk967 and others added 3 commits February 28, 2022 12:03

Merge branch 'master' into sai/10194_state_sync_v2_store

a34cfb8

chore: address the review comments

3d09bbd

chore: address the review comments

808fc1e

gsk967 requested review from yihuang and roysc February 28, 2022 09:28

yihuang approved these changes Feb 28, 2022

View reviewed changes

gsk967 and others added 2 commits February 28, 2022 19:40

Merge branch 'master' into sai/10194_state_sync_v2_store

f422e7c

chore: bump the snapshot format

beb757c

i-norden added 2 commits February 28, 2022 13:21

use new format version in unit tests

ab3ffde

imports ordering

f47ace3

robert-zaremba approved these changes Mar 2, 2022

View reviewed changes

robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 2, 2022

gsk967 added 2 commits March 3, 2022 09:41

Merge pull request #19 from vulcanize/ian/10194_state_sync_v2_store

f5478fd

Ian/10194 state sync v2 store

Merge branch 'master' into sai/10194_state_sync_v2_store

43b113f

github-actions bot removed C:x/authz C:Simulations labels Mar 3, 2022

merge the upstream master

1dbf325

github-actions bot added the C:x/gov label Mar 4, 2022

mergify bot merged commit 489b399 into cosmos:master Mar 4, 2022

roysc deleted the sai/10194_state_sync_v2_store branch July 3, 2022 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gsk967 commented Dec 16, 2021

roysc left a comment

i-norden left a comment

gsk967 commented Jan 12, 2022

i-norden left a comment

robert-zaremba left a comment

roysc Feb 25, 2022

roysc Feb 25, 2022

roysc Feb 25, 2022

roysc Feb 25, 2022

roysc Feb 25, 2022

gsk967 Feb 25, 2022

roysc left a comment

roysc Feb 25, 2022

yihuang Feb 26, 2022

roysc Feb 26, 2022

gsk967 Feb 28, 2022

yihuang Feb 28, 2022 •

edited

Loading

gsk967 Feb 28, 2022

yihuang Feb 26, 2022 •

edited

Loading

yihuang Feb 28, 2022

gsk967 Feb 28, 2022

yihuang left a comment

gsk967 commented Feb 28, 2022 •

edited

Loading

yihuang left a comment

robert-zaremba commented Feb 28, 2022

gsk967 commented Feb 28, 2022

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

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

Conversation

gsk967 commented Dec 16, 2021

Description

State Sync for V2 Store (ADR-40)

Author Checklist

Reviewers Checklist

roysc left a comment

Choose a reason for hiding this comment

i-norden left a comment

Choose a reason for hiding this comment

gsk967 commented Jan 12, 2022

i-norden left a comment

Choose a reason for hiding this comment

robert-zaremba 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

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

yihuang Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Feb 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang left a comment

Choose a reason for hiding this comment

gsk967 commented Feb 28, 2022 • edited Loading

yihuang left a comment

Choose a reason for hiding this comment

robert-zaremba commented Feb 28, 2022

gsk967 commented Feb 28, 2022

yihuang Feb 28, 2022 •

edited

Loading

yihuang Feb 26, 2022 •

edited

Loading

gsk967 commented Feb 28, 2022 •

edited

Loading