Add WASM files to state sync #816

assafmo · 2022-04-25T19:19:37Z

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

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

assafmo · 2022-04-25T19:22:32Z

app/app.go

@@ -682,6 +682,11 @@ func NewWasmApp(
 	app.scopedIBCKeeper = scopedIBCKeeper
 	app.scopedTransferKeeper = scopedTransferKeeper
 	app.scopedWasmKeeper = scopedWasmKeeper
+
+	app.SnapshotManager().RegisterExtensions(
+		wasm.NewWasmSnapshotter(filepath.Join(wasmDir, "wasm", "state", "wasm")),


There's probably a nicer way of doing this, but honestly this directory structure is buried so deep inside https://github.com/CosmWasm/cosmwasm/blob/be4d94a8d73ceab359226b8ea0a5b4be032a07e6/packages/vm/src/cache.rs#L105 so maybe it's better to leave it that way for now. Just a thought.

Yeah, that top level wasm comes from wasmd from long ago.
I have thought of changing it, but it would be a very painful breaking change to all running nodes.

Actually, notice wasmDir := filepath.Join(homePath, "wasm") on line 448 or so. That already contains the first wasm.

It should be filepath.Join(wasmDir, "state", "wasm")

The file structure is private and does not provide any stability guarantees. There is not even a guarantee Wasm is stored as file on disk in the next version of cosmwasm-vm. The file system should not be accessed from wasmd. Instead there is GetCode and Create in wasmvm to load and store a Wasm blob.

There's another "wasm" along the way

wasmd/x/wasm/keeper/keeper.go

Line 99 in 3fb7371

wasmer, err := wasmvm.NewVM(filepath.Join(homeDir, "wasm"), supportedFeatures, contractMemoryLimit, wasmConfig.ContractDebugMode, wasmConfig.MemoryCacheSize)

The file system should not be accessed from wasmd. Instead there is GetCode and Create in wasmvm to load and store a Wasm blob.

This is a good point, we can iterate over the values in the iavl tree rather than filenames. That is cleaner. And it will also "force" the node restoring from snapshotting to do the precompiling of the wasm files upon syncing, not later, providing more stable execution times later.

sounds legit!

ethanfrey

Thank you for your contribution.

It looks solid in general. Added a few comments, also an idea how to make a simple unit test just copying arbitrary files based on names between two dirs. That would be a nice sanity check before manual testing.

What do you think of a custom SnapshotItem payload format (not just appending bytes)?

ethanfrey · 2022-04-25T20:28:12Z

app/app.go

@@ -682,6 +682,11 @@ func NewWasmApp(
 	app.scopedIBCKeeper = scopedIBCKeeper
 	app.scopedTransferKeeper = scopedTransferKeeper
 	app.scopedWasmKeeper = scopedWasmKeeper
+
+	app.SnapshotManager().RegisterExtensions(
+		wasm.NewWasmSnapshotter(filepath.Join(wasmDir, "wasm", "state", "wasm")),


Actually, notice wasmDir := filepath.Join(homePath, "wasm") on line 448 or so. That already contains the first wasm.

It should be filepath.Join(wasmDir, "state", "wasm")

ethanfrey · 2022-04-25T20:29:56Z

x/wasm/wasm_snapshotter.go

+		return err
+	}
+
+	// In case snapshotting needs to be deterministic


Yeah although I'm still not sure whether that's needed.

ethanfrey · 2022-04-25T20:32:12Z

x/wasm/wasm_snapshotter.go

+			return err
+		}
+
+		// snapshotItem is 64 bytes of the file name, then the actual WASM bytes


This works. But maybe we want a real (eg. more future-proof) format.

Eg. like a Protobuf object with two fields and a type. Especially if we ever want to sync anything else

ethanfrey · 2022-04-25T20:33:34Z

x/wasm/wasm_snapshotter.go

+	return nil
+}
+
+func (ws *WasmSnapshotter) Restore(


Is it possible to do a unit test with some simple data, by feeding the output of snapshot into restore (or another instance with different directory) and ensuring the proper files are copied verbatim (and misnamed files ignored)?

ethanfrey · 2022-04-25T20:35:54Z

x/wasm/wasm_snapshotter.go

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


This seems to read the entire data and return nothing ever. Why does it return (snapshot.SnapshotItem, error) rather than error? (eg. Is there something I/we are missing?)

I know this is not your design, but wonder why it is there

Yeah, I took this from the sdk tests. Not sure if the return value goes anywhere here, but I can confirm that this works without any issue (tested manually on Secret). Also might be irrelevant for this case as we're dealing with files and not IAVL.

All this logic is in a loop where it seems like its the extension responsibility to handle the next SnapshotItem to the next one, which doesn't make sense because a plugin can break another one without knowing.

See https://github.com/cosmos/cosmos-sdk/blob/6f070623741fe0d6851d79ada41e6e2b1c67e236/snapshots/manager.go#L333

My guess is that is possible that in the protobuf stream there might be more items and if the format is not the one that wasmd can understand then it should return it.

My guess is that is possible that in the protobuf stream there might be more items and if the format is not the one that wasmd can understand then it should return it.

Yes, very good idea. It seems like we should not error on some unknown extension, but just return it, as there may in the future be multiple extensions than need to cooperate somehow. Not sure how this all works together.

Maybe @yihuang could clarify how we should handle this properly

Yes, very good idea. It seems like we should not error on some unknown extension, but just return it, as there may in the future be multiple extensions than need to cooperate somehow. Not sure how this all works together.

exactly, we should return the item if it's unknown to us.

which doesn't make sense because a plugin can break another one without knowing.

yeah, that's possible unfortunately, maybe we can create some helpers at module side to retain the last item automatically, without changing the api in cosmos-sdk.

ah, I got it... we either end on EOF, or when we hit something where GetExtensionPayload() returns nil (another type we don't expect)

which doesn't make sense because a plugin can break another one without knowing.

yeah, that's possible unfortunately, maybe we can create some helpers at module side to retain the last item automatically, without changing the api in cosmos-sdk.

// ExtensionPayloadReader only reads the payloads of current extension. type ExtensionPayloadReader struct { protoReader protoio.Reader LastItem types.SnapshotItem } func NewExtensionPayloadReader(protoReader protoio.Reader) *ExtensionPayloadReader { return &ExtensionPayloadReader{ protoReader: protoReader, LastItem: types.SnapshotItem{}, } } // ReadPayload reads a payload item from the stream. func (epr *ExtensionPayloadReader) ReadPayload() ([]byte, error) { epr.LastItem = types.SnapshotItem{} if err := epr.protoReader.ReadMsg(&epr.LastItem); err != nil { return nil, err } payload := epr.LastItem.GetExtensionPayload() if payload == nil { // end of payload stream of current extension return nil, io.EOF } return payload.Payload, nil }

I think of sth like this could help, the manager pass the ExtensionPayloadReader extension's Restore method, but it's api breaking, so maybe change this in next version.

webmaster128

Thank you Assaf

webmaster128 · 2022-04-25T20:37:44Z

app/app.go

@@ -682,6 +682,11 @@ func NewWasmApp(
 	app.scopedIBCKeeper = scopedIBCKeeper
 	app.scopedTransferKeeper = scopedTransferKeeper
 	app.scopedWasmKeeper = scopedWasmKeeper
+
+	app.SnapshotManager().RegisterExtensions(
+		wasm.NewWasmSnapshotter(filepath.Join(wasmDir, "wasm", "state", "wasm")),


The file structure is private and does not provide any stability guarantees. There is not even a guarantee Wasm is stored as file on disk in the next version of cosmwasm-vm. The file system should not be accessed from wasmd. Instead there is GetCode and Create in wasmvm to load and store a Wasm blob.

webmaster128 · 2022-04-25T20:39:11Z

x/wasm/wasm_snapshotter.go

+var wasmFileNameRegex = regexp.MustCompile(`^[a-f0-9]{64}$`)
+
+func (ws *WasmSnapshotter) Snapshot(height uint64, protoWriter protoio.Writer) error {
+	wasmFiles, err := ioutil.ReadDir(ws.wasmDirectory)


wasmFiles, i.e. the list of checksums needs to come from the main Tendermint database. Otherwise a node can inject all kind of data here bypassing consensus.

webmaster128 · 2022-04-25T20:40:12Z

x/wasm/wasm_snapshotter.go

+	return []uint32{1}
+}
+
+var wasmFileNameRegex = regexp.MustCompile(`^[a-f0-9]{64}$`)


Those file names can change any time. I'm actually thinking about adding a .wasm file extension for node operator friendlyness.

This is a good point, but for a given wasmd version, there will be only one storage format (and realistically, changing this will be a major state-breaking change needing an internal wasmvm migration, so unlikely to happen, or at least very infrequently).

If it does change, we make a new wasmd (eg. 2.0) with snapshot format 2. Upon receiving that the data is parsed in a different way (eg the new internal format). The snapshotter itself (they one reading), will just need to implement the new format when we break this internal storage format.

That said, I do agree with using the GetCode and Create calls

I agree that it's better to do it through the proper API, and let future migrations less to worry about WRT snapshotting.

This also "solves" the wasm/wasm/state/wasm issue here.

assafmo · 2022-04-25T20:49:46Z

@webmaster128 is right, it's better to go through the existing API instead of accessing the filesystem directly.

webmaster128 · 2022-04-25T20:55:38Z

@webmaster128 is right, it's better to go through the existing API instead of accessing the filesystem directly.

Thank you. Let me know if you need additional APIs or more advances features than the two mentioned above. But those are the two basic operations.

webmaster128 · 2022-04-25T20:58:45Z

x/wasm/wasm_snapshotter.go

+		snapshotItem := append([]byte(wasmFile.Name()), wasmBytes...)
+
+		snapshot.WriteExtensionItem(protoWriter, snapshotItem)


Have you considered compressing the Wasm before transfer? Or is this taken care of at a different level of the state sync stack?

I didn't consider it, not sure if it's a problem. The wasm files are usually far smaller than the rest of the blockchain's state.

It can be a nice addition though.

Also not sure if it's taken care of by the sdk.

Okay, no priority or blocker for sure. I was just curious since Wasm blobs can be compressed nicely. But we can always make that an optional extension later.

Interesting observation nonetheless. 💪

wasmd already gzips wasm blobs when you submit a tx or through governance.

wasmd/x/wasm/client/cli/tx.go

Line 88 in d5ef3ba

if wasmUtils.IsWasm(wasm) {

wasmd/x/wasm/client/utils/utils.go

Line 24 in d5ef3ba

func GzipIt(input []byte) ([]byte, error) {

and then uncompressed when the keeper.Create is called

wasmd/x/wasm/keeper/ioutil.go

Line 18 in d5ef3ba

func uncompress(src []byte, limit uint64) ([]byte, error) {

maybe some of this can be reused it really reduces the size but it will depend on each chain on how many contracts they have. Although as previously mentioned smaller than the actual chain data.

giansalex · 2022-04-26T04:09:52Z

I was working on this feature, but tests are pending... I have updated with master branch, maybe it can be used as a reference.

Snapshot:

Iterate codes
Remove duplicate hash
Save CodeHash, WasmBytes using SnapshotWasmItem

Restore:

Get codehash, wasmbytes from SnapshotPayload
Create wasm using vm
Verify hash

https://github.com/disperze/wasmd/blob/state-sync-sdk-45.2/x/wasm/keeper/snapshot.go
[ full diff ]

webmaster128 · 2022-04-26T07:10:41Z

@giansalex Thank you! Could you add your work as a second PR to provide better visibility and being able to comment inline?

alpe

Thanks for the PR and all the good comments here. Nice collaboration!
I think @assafmo solution was a very pragmatic approach and automates what people already do with sharing zipped wasm folders.
I completely agree with @webmaster128 suggestions for decoupling and a more resilient solution that uses existing apis. Upstream implementations may change and can break this easily otherwise.
Proper tests can be built with our mock

@giansalex Please do a draft PR, so that we can comment already. 🙏
I peeked at the code and it seems to address all the raised points. Good work

ethanfrey · 2022-04-26T08:24:31Z

I got a anon review via DM:

When restoring all the wasm files, we must ensure to tell the wasmvm to pin the proper codes.

If we use the Create method, this will just be one more check on CodeInfo and possibly another call to wasmvm

alpe · 2022-04-26T09:40:01Z

InitializePinnedCodes can be used to pin all codes again.

assafmo · 2022-04-26T11:03:29Z

I got a anon review via DM:

When restoring all the wasm files, we must ensure to tell the wasmvm to pin the proper codes.

If we use the Create method, this will just be one more check on CodeInfo and possibly another call to wasmvm

I'm not sure it that's necessary. Codes info are already stored in the IAVL.

webmaster128 · 2022-04-26T11:28:04Z

I'm not sure it that's necessary. Codes info are already stored in the IAVL.

InitializePinnedCodes is basically the answer to that DM request. It's really about lifting contracts into cache after a node restart or state sync.

tomtau · 2022-04-27T01:04:46Z

copy-pasting from Discord:

fyi we have this simple test on CI for state sync: https://github.com/crypto-org-chain/chain-main/blob/master/integration_tests/test_basic.py#L96 via this tool: https://github.com/crypto-com/pystarport ... so I imagine a test on CI for wasmd may be an extension of it: create a network, deploy a contract there, create a new node with state sync to join the network, submit a tx to invoke the contract-related functionality, assert that query responses from the state sync'ed and the original nodes are the same

ethanfrey · 2022-04-27T11:38:41Z

@assafmo Very helpful PR showing how to implement such an extension.

There have been a number of wonderful comments explaining how to improve the process, and add more features (pinning, compression, etc).

I would like to get a version of this in incorporating all the input. Do you want to update this PR with all feedback? Or shall I write one? I'm happy either day. I would try to write it or review it tomorrow (Thursday) to get this in soon, and let people test manually on some tag.

assafmo · 2022-04-27T11:52:44Z

I'm not sure if I can get to it that soon, so go for it, no ego here. ❤️
BTW I'm not sure that pinning is necessary here.

ethanfrey

Building on this, I have made a new PR that attempts to incorporate all the discussion.

Please check out #823

It should be complete now, "just" missing tests, but good for a first review.

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

x/wasm/wasm_snapshotter.go

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


ah, I got it... we either end on EOF, or when we hit something where GetExtensionPayload() returns nil (another type we don't expect)

ethanfrey · 2022-04-28T09:22:02Z

I was working on this feature, but tests are pending... I have updated with master branch, maybe it can be used as a reference.

@giansalex sorry I didn't see your code earlier. I was looking at open PRs... even draft.

I will look now, definitely would have saved me time yesterday and some things I can learn from it still

assafmo · 2022-04-28T10:27:55Z

Closing in favor of #823.

Add WasmSnapshotter

8486bdf

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

assafmo requested a review from alpe as a code owner April 25, 2022 19:19

assafmo commented Apr 25, 2022

View reviewed changes

ethanfrey reviewed Apr 25, 2022

View reviewed changes

webmaster128 reviewed Apr 25, 2022

View reviewed changes

alpe reviewed Apr 26, 2022

View reviewed changes

ethanfrey mentioned this pull request Apr 27, 2022

Add state sync support #823

Merged

ethanfrey reviewed Apr 27, 2022

View reviewed changes

assafmo closed this Apr 28, 2022

yihuang mentioned this pull request Apr 29, 2022

Make extension snapshotter interface safer to use cosmos/cosmos-sdk#11824

Closed

4 tasks

		snapshotItem := append([]byte(wasmFile.Name()), wasmBytes...)

		snapshot.WriteExtensionItem(protoWriter, snapshotItem)

Add WASM files to state sync #816

Add WASM files to state sync #816

Conversation

assafmo commented Apr 25, 2022

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

ethanfrey 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

Choose a reason for hiding this comment

jhernandezb Apr 25, 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 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

assafmo Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

assafmo commented Apr 25, 2022

webmaster128 commented Apr 25, 2022

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

jhernandezb Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

giansalex commented Apr 26, 2022 • edited Loading

webmaster128 commented Apr 26, 2022

alpe left a comment • edited Loading

Choose a reason for hiding this comment

ethanfrey commented Apr 26, 2022

alpe commented Apr 26, 2022

assafmo commented Apr 26, 2022

webmaster128 commented Apr 26, 2022 • edited Loading

tomtau commented Apr 27, 2022

ethanfrey commented Apr 27, 2022

assafmo commented Apr 27, 2022

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Apr 28, 2022

assafmo commented Apr 28, 2022

jhernandezb Apr 25, 2022 •

edited

Loading

yihuang Apr 27, 2022 •

edited

Loading

assafmo Apr 25, 2022 •

edited

Loading

jhernandezb Apr 25, 2022 •

edited

Loading

giansalex commented Apr 26, 2022 •

edited

Loading

alpe left a comment •

edited

Loading

webmaster128 commented Apr 26, 2022 •

edited

Loading