Add state sync support #823

ethanfrey · 2022-04-27T17:51:30Z

This replaces #816 incorporating much of the feedback.

Main changes:

Iterates over codes in the iavl tree to get proper list
compresses wasm before sending over snapshot item
skips duplicates (multiple codes with same byte code)
uses wasmvm.Create() on restore, which will precompile the code
pins all proper codes in the cache, just like on normal startup
Play better with other extensions - done in 7a29351

TODO:

unit tests

As a side, I pull the compress logic (in cli/utils) and the uncompress logic (from keeper) into a common package. That could be it's own PR, if someone really cares, I guess I could cherry pick 71ff81e out

ethanfrey · 2022-04-27T18:14:07Z

x/wasm/keeper/snapshotter.go

+		// we should return it an let the manager handle this one
+		payload := item.GetExtensionPayload()
+		if payload == nil {
+			return item, nil


@yihuang If I understand #816 (comment) correctly, then this is the correct behavior. Can you confirm or correct me?

if not a payload, it is because another module begins.

Got you, I need to run finalise here as well.

assafmo · 2022-04-27T18:32:16Z

x/wasm/keeper/snapshotter.go

+
+func (ws *WasmSnapshotter) Snapshot(height uint64, protoWriter protoio.Writer) error {
+	var rerr error
+	ctx := sdk.NewContext(ws.cms, tmproto.Header{}, false, log.NewNopLogger())


giansalex · 2022-04-27T18:36:07Z

x/wasm/keeper/snapshotter.go

+		item := snapshot.SnapshotItem{}
+		err := protoReader.ReadMsg(&item)
+		if err == io.EOF {
+			return snapshot.SnapshotItem{}, finalize(ctx, ws.wasm)


there may be other modules after wasm snapshotter, and this condition will not occur.

So then how would you exit the loop?

when SnaphostItem is not a payload

Oh, okay, so this condition is still needed and it was just a general comment.

yes, is necessary, but finalize(ctx, ws.wasm) will not run in some scenarios

I think I addressed this for the other case we end without error.
Please confirm

@ethanfrey yes, ok

Nit: The multitstore snapshotter handle the finalization at the end of loop, and use break here and when payload == nil.

x/wasm/keeper/snapshotter.go

This helps state sync to track the WASM files too, as they're not in IAVL

ethanfrey · 2022-04-27T19:46:06Z

Thanks for the reviews, I made some updated

codecov · 2022-04-27T19:56:50Z

Codecov Report

Merging #823 (1253c8e) into main (23c75b1) will increase coverage by 0.34%.
The diff coverage is 55.91%.

@@            Coverage Diff             @@
##             main     #823      +/-   ##
==========================================
+ Coverage   59.25%   59.60%   +0.34%     
==========================================
  Files          51       52       +1     
  Lines        5898     5944      +46     
==========================================
+ Hits         3495     3543      +48     
+ Misses       2150     2139      -11     
- Partials      253      262       +9

Impacted Files	Coverage Δ
app/test_helpers.go	`0.84% <0.00%> (+0.08%)`	⬆️
x/wasm/ioutils/utils.go	`71.42% <ø> (ø)`
app/app.go	`87.65% <33.33%> (-1.30%)`	⬇️
x/wasm/client/cli/tx.go	`25.00% <66.66%> (ø)`
x/wasm/keeper/snapshotter.go	`66.66% <66.66%> (ø)`
x/wasm/ioutils/ioutil.go	`100.00% <100.00%> (ø)`
x/wasm/keeper/keeper.go	`88.09% <100.00%> (+0.70%)`	⬆️

ethanfrey · 2022-04-28T13:40:40Z

I used this line from @giansalex to properly load from a historical height: https://github.com/disperze/wasmd/blob/state-sync-sdk-45.2/x/wasm/keeper/snapshot.go#L49

However, all the tests panic when I try to commit the state before snapshotting (so I have a height to read from). It keeps saying the version was already committed. I spent 30 minutes digging in the sdk, but remain lost. Maybe someone has insights.

See 7663d6f and error messages

ethanfrey · 2022-04-28T14:49:16Z

I think this is about as good as I can do.

Happy for reviews. And if anyone wants to make a PR on top to address the historical height issue (or add app level tests), I would be happy to merge it in. Otherwise, I would revisit this next week Tues/Wed and try to get this in main.

Also, very happy for any manual testing on this. I definitely need that before tagging the release, but would merge before that if possible.

webmaster128

nice

x/wasm/ioutils/utils.go

x/wasm/keeper/snapshotter.go

webmaster128 · 2022-04-28T15:24:50Z

x/wasm/keeper/snapshotter.go

+		return sdkerrors.Wrap(types.ErrCreateFailed, err.Error())
+	}
+
+	// FIXME: check which codeIDs the checksum matches??


Which code ID is not important. But ensuring the checksum exists in the codes list at all makes sense. Otherwise a malicious node could send all kind of unrelated Wasm that gets stored and compiled, right?

I mean, they can send us anything.
And at the point we have the checksum to compare it has been stored and compiled.

I guess this check would only allow one useless wasm.

I am more interested in asserting that all needed wasm has been synced. The biggest problem is a sync where some wasm file is missing and the node crashes later when that contract is called.

I guess this check would only allow one useless wasm.

What prevents the other node from sending us thousands of Wasm blobs that we decompress, store and compile but that are not part of the blockchain at all?

I am more interested in asserting that all needed wasm has been synced. The biggest problem is a sync where some wasm file is missing and the node crashes later when that contract is called.

Right, the codes missing issue is mentioned in the finalize method.

I would leave this for a follow up PR, nice to have check. I would love to get a full-node integration test on current state

yihuang · 2022-04-29T00:10:54Z

x/wasm/keeper/snapshotter.go

+	// Since codeIDs and wasm are immutible, it is never wrong to return new wasm data than the
+	// user requests
+	// ------
+	// cacheMS, err := ws.cms.CacheMultiStoreWithVersion(int64(height))


If we don't fix the storage at the height, would different nodes generate different snapshots, because their current block height maybe different?

Yes that might make result in 2 nodes creating a different snapshot for the same height, which will make a client reject the snapshot.

yihuang · 2022-04-29T01:47:30Z

FYI, I just proposed an API change to cosmos-sdk to make the snapshotter api safer to use: cosmos/cosmos-sdk#11825

alpe

I need to make myself more familiar with the internals of the snapshot package. Beside some minor comments, your code looks very good.

alpe · 2022-04-28T15:55:48Z

x/wasm/keeper/snapshotter.go

+)
+
+/*
+API to implement:


This is a lot of in line doc. Personally, I don't think it adds more value than a code reference, like this:

var ( _ snapshot.Snapshotter = &WasmSnapshotter{} _ snapshot.ExtensionSnapshotter = &WasmSnapshotter{} )

Doc needs to be maintained, while the code reference gives you the latest with 1 click.

ExtensionSnapshotter inherits from Snapshotter so that would be enough as a ref

alpe · 2022-04-28T15:57:31Z

x/wasm/keeper/snapshotter.go

+*/
+
+// Format 1 is just gzipped wasm byte code for each item payload. No protobuf envelope, no metadata.
+const SnapshotFormat = 1


👍 good description. To render nice Godoc, it should start with the element name, In this case SnapshotFormat

alpe · 2022-05-02T15:33:23Z

x/wasm/keeper/snapshotter.go

+func (ws *WasmSnapshotter) Restore(
+	height uint64, format uint32, protoReader protoio.Reader,
+) (snapshot.SnapshotItem, error) {
+	if format == 1 {


Suggested change

if format == 1 {

if format == SnapshotFormat {

alpe · 2022-05-02T15:56:22Z

x/wasm/keeper/snapshotter_test.go

+	}
+	initMsgBz, err := json.Marshal(initMsg)
+	require.NoError(t, err)
+	contractAddr, _, err := keepers.ContractKeeper.Instantiate(ctx, codeID, creator, nil, initMsgBz, "demo contract 1", deposit)


no need to instantiate or query here

alpe · 2022-05-02T16:00:45Z

x/wasm/keeper/snapshotter_test.go

+
+	recovery := NewWasmSnapshotter(newKeepers.MultiStore, newKeepers.WasmKeeper)
+	_, err = recovery.Restore(100, 1, protoio.NewFullReader(&buf, buf.Len()))
+	require.NoError(t, err)


the restored wasm file should to be confirmed. Can you call k.wasmVM.GetCode(hash) with the original hash?

alpe · 2022-05-03T10:55:44Z

x/wasm/keeper/snapshotter.go

+			return true
+		}
+
+		compressedWasm, err := ioutils.GzipIt(wasmBytes)


Just found:

The current version 1 snapshot format is a zlib-compressed, length-prefixed Protobuf stream of cosmos.base.store.v1beta1.SnapshotItem messages, split into chunks at exact 10 MB byte boundaries.

https://github.com/cosmos/cosmos-sdk/tree/main/snapshots#snapshot-format

Add snapshotter integration tests and minor cleanup

assafmo · 2022-05-04T17:38:35Z

Glad to see this merged! Did you find what caused ws.cms.CacheMultiStoreWithVersion(int64(height)) not to work at first?

ethanfrey · 2022-05-04T19:43:35Z

It worked. Just not with our test code.

Alex used a different test setup (the full app setup) and it works now

ethanfrey commented Apr 27, 2022

View reviewed changes

assafmo reviewed Apr 27, 2022

View reviewed changes

giansalex reviewed Apr 27, 2022

View reviewed changes

x/wasm/keeper/snapshotter.go Outdated Show resolved Hide resolved

assafmo and others added 10 commits April 27, 2022 21:01

Add WasmSnapshotter

3080845

This helps state sync to track the WASM files too, as they're not in IAVL

Make snapshot from keeper

870bba8

Rough draft of the restore method using multistore

abfadce

properly register the snapshot manager

f789aed

Move snapshotter to keeper and implement

9952475

Add compression to snapshot wasm bytecode

1b3c133

Put GzipIt and Uncompress logic in a common folder

4cccddb

Fix linting errors

f68ef90

Avoid sending duplicate wasm codes over the wire

abad37d

Properly handle when a message not for this extension arrives

d2b0fb5

ethanfrey force-pushed the add-state-sync branch from 7a29351 to d2b0fb5 Compare April 27, 2022 19:01

ethanfrey added 3 commits April 27, 2022 21:30

Start testing snapshots

a5dd494

Basic tend to end test for no errors

fea9d39

Address @giansalex PR comments

9705a8d

assafmo mentioned this pull request Apr 28, 2022

Add WASM files to state sync #816

Closed

ethanfrey added 2 commits April 28, 2022 15:26

Updates inspired by @giansalex branch

d8426fb

Cannot commit in my test cases to load from proper height

7663d6f

SRemove loading history until I can fix tests here

a154a9a

ethanfrey marked this pull request as ready for review April 28, 2022 14:45

ethanfrey requested a review from alpe as a code owner April 28, 2022 14:45

webmaster128 reviewed Apr 28, 2022

View reviewed changes

Cleanup from Simon's comments

cdcb18d

ethanfrey force-pushed the add-state-sync branch from 3e512fd to cdcb18d Compare April 28, 2022 18:55

yihuang reviewed Apr 29, 2022

View reviewed changes

alpe reviewed May 2, 2022

View reviewed changes

alpe reviewed May 3, 2022

View reviewed changes

alpe and others added 5 commits May 3, 2022 13:48

Add snapshotter integration tests and minor cleanup

6b4accb

Minor test update

297a646

Fix linter warnings

a5695ee

Merge pull request #834 from CosmWasm/add-state-sync_integration

cb2e8f2

Add snapshotter integration tests and minor cleanup

Merge branch 'main' into add-state-sync

1253c8e

ethanfrey merged commit f2c2a58 into main May 4, 2022

ethanfrey deleted the add-state-sync branch May 4, 2022 08:30

ethanfrey mentioned this pull request May 4, 2022

Support state sync #478

Closed

Cashmaney added a commit to scrtlabs/SecretNetwork that referenced this pull request May 8, 2022

Fix snapshot creation according to CosmWasm/wasmd#823 (comment)

89a4668

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add state sync support #823

Add state sync support #823

ethanfrey commented Apr 27, 2022 •

edited

Loading

ethanfrey Apr 27, 2022

giansalex Apr 27, 2022 •

edited

Loading

ethanfrey Apr 27, 2022

assafmo Apr 27, 2022

giansalex Apr 27, 2022 •

edited

Loading

assafmo Apr 27, 2022 •

edited

Loading

giansalex Apr 27, 2022

assafmo Apr 27, 2022

giansalex Apr 27, 2022 •

edited

Loading

ethanfrey Apr 27, 2022

giansalex Apr 27, 2022

yihuang Apr 28, 2022 •

edited

Loading

ethanfrey commented Apr 27, 2022

codecov bot commented Apr 27, 2022 •

edited

Loading

ethanfrey commented Apr 28, 2022

ethanfrey commented Apr 28, 2022

webmaster128 left a comment

webmaster128 Apr 28, 2022

ethanfrey Apr 28, 2022

webmaster128 Apr 28, 2022

ethanfrey Apr 28, 2022

yihuang Apr 29, 2022 •

edited

Loading

assafmo May 1, 2022

yihuang commented Apr 29, 2022 •

edited

Loading

alpe left a comment

alpe Apr 28, 2022

alpe May 3, 2022

alpe Apr 28, 2022

alpe May 2, 2022

alpe May 2, 2022

alpe May 2, 2022

alpe May 3, 2022

assafmo commented May 4, 2022

ethanfrey commented May 4, 2022

Add state sync support #823

Add state sync support #823

Conversation

ethanfrey commented Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

giansalex Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giansalex Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

assafmo Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giansalex Apr 27, 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 Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

ethanfrey commented Apr 27, 2022

codecov bot commented Apr 27, 2022 • edited Loading

Codecov Report

ethanfrey commented Apr 28, 2022

ethanfrey commented Apr 28, 2022

webmaster128 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 Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang commented Apr 29, 2022 • edited Loading

alpe 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

assafmo commented May 4, 2022

ethanfrey commented May 4, 2022

ethanfrey commented Apr 27, 2022 •

edited

Loading

giansalex Apr 27, 2022 •

edited

Loading

giansalex Apr 27, 2022 •

edited

Loading

assafmo Apr 27, 2022 •

edited

Loading

giansalex Apr 27, 2022 •

edited

Loading

yihuang Apr 28, 2022 •

edited

Loading

codecov bot commented Apr 27, 2022 •

edited

Loading

yihuang Apr 29, 2022 •

edited

Loading

yihuang commented Apr 29, 2022 •

edited

Loading