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.

Sign up for GitHub

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

Jump to bottom

Remove check for wasm limit size in state sync #1471

Merged
merged 3 commits into from
Jul 5, 2023
Merged

Remove check for wasm limit size in state sync #1471

merged 3 commits into from
Jul 5, 2023

Conversation

pinosu
Copy link
Contributor

@pinosu pinosu commented Jun 29, 2023

Resolves #1467

@pinosu pinosu requested a review from alpe as a code owner June 29, 2023 11:12
@pinosu pinosu force-pushed the 1467-accept_statesync_wasm_data branch from 24eb158 to fab8f0e Compare June 29, 2023 11:17
alpe
alpe reviewed Jun 29, 2023
View reviewed changes
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Good start but let's do less ;-)
An open question for me is the genesis import. Do we want to ensure the MaxWasmSize or accept what comes in as in the state sync example? It feels that the cases are very similar. WDYT?

x/wasm/ioutils/ioutil.go Outdated

// UncompressWithLimit fails if wasm size exceeds the limit.
// It expects a valid gzip source to unpack or fails. See IsGzip
func UncompressWithLimit(gzipSrc []byte, limit uint64) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break api compatibility easily. Let's keep the old name just in case. The param is self explaining

x/wasm/ioutils/ioutil.go Outdated
@@ -11,7 +11,21 @@ import (
)

// Uncompress expects a valid gzip source to unpack or fails. See IsGzip
func Uncompress(gzipSrc []byte, limit uint64) ([]byte, error) {
func Uncompress(gzipSrc []byte) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to copy the code. Instead of adding a new method, you can simply pass math.MaxUint as limit. This makes it also more readable on the caller, IMHO.

x/wasm/ioutils/ioutil_test.go Outdated
assert.Equal(t, spec.expResult, r)
})
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

good test. But we would need one on the snapshotter to ensure a file > MaxWasmSize is accepted.

@pinosu pinosu force-pushed the 1467-accept_statesync_wasm_data branch from 207d5ef to 4091868 Compare July 4, 2023 14:54
@pinosu pinosu force-pushed the 1467-accept_statesync_wasm_data branch from 4091868 to 4a6962e Compare July 4, 2023 16:05
alpe
alpe approved these changes Jul 5, 2023
View reviewed changes
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Great work! Please see my comments on restoring the original size limit. Otherwise 💯 🏄

x/wasm/keeper/genesis_test.go Outdated
@@ -133,6 +135,12 @@ func TestGenesisExportImport(t *testing.T) {
srcIT := srcCtx.KVStore(wasmKeeper.storeKey).Iterator(nil, nil)
dstIT := dstCtx.KVStore(dstKeeper.storeKey).Iterator(nil, nil)

t.Cleanup(func() {
types.MaxWasmSize = 800 * 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

Great to do proper housekeeping here! I am a bit afraid that we will easily miss this line whenever the default would be touched in the future. Can you use a temp variable for the original value instead and then use it here?
like:

origMaxWasmSize := types.MaxWasmSize
types.MaxWasmSize = 1
...
types.MaxWasmSize = origMaxWasmSize

x/wasm/keeper/snapshotter_integration_test.go Outdated
@@ -67,6 +67,11 @@ func TestSnapshotter(t *testing.T) {
require.NoError(t, err)
assert.NotNil(t, snapshot)

types.MaxWasmSize = 1
t.Cleanup(func() {
types.MaxWasmSize = 800 * 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

As commented before, please use a temp variable to not hard code the value here

@pinosu pinosu merged commit 1763477 into main Jul 5, 2023
1 check passed
@pinosu pinosu deleted the 1467-accept_statesync_wasm_data branch July 5, 2023 09:33
@faddat faddat mentioned this pull request Jul 16, 2023
Closed
@pinosu pinosu added the backport/v0.3x Backport patches to sdk45 release branch label Jul 21, 2023
mergify bot pushed a commit that referenced this pull request Jul 21, 2023
@pinosu @mergify
Remove check for wasm limit size in state sync (#1471)
c176994 
* Remove check for wasm limit size in state sync

* Fix comments

* Store original value in variable

(cherry picked from commit 1763477)

# Conflicts:
#	x/wasm/keeper/genesis_test.go
@mergify mergify bot mentioned this pull request Jul 21, 2023
Merged
pinosu added a commit that referenced this pull request Jul 21, 2023
@mergify @pinosu
Remove check for wasm limit size in state sync (backport #1471) (#1529)
c529e97 
* Remove check for wasm limit size in state sync (#1471)

* Remove check for wasm limit size in state sync

* Fix comments

* Store original value in variable

(cherry picked from commit 1763477)

# Conflicts:
#	x/wasm/keeper/genesis_test.go

* Fix conflicts

---------

Co-authored-by: pinosu <[email protected]>
Co-authored-by: Pino' Surace <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alpe alpe alpe approved these changes

Assignees
No one assigned
Labels
backport/v0.3x Backport patches to sdk45 release branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accept wasm data from state-sync as valid
2 participants
@pinosu @alpe