Consolidate MaxWasmSize constraints into a single var #809

bigs · 2022-04-19T21:40:30Z

The maxWasmCodeSize parameter is no longer used. You can set it by overriding x/wasm/types/MaxWasmSize at compile time (in app.go). The default is now set to 800kB but any chain can easily configure when doing a binary upgrade. This change eliminates a conflict wherein the parameter was only used in the WasmKeeper while a separate constant was used to validate MsgStoreCode messages.

ethanfrey · 2022-04-25T09:44:23Z

Good eye.

ValidateBasic does not have access to state, so we cannot check against params. If we remove this check here, I would like to ensure that it will actually be checked somewhere in CheckTx. Otherwise, you can get honest validators to fill up the chain with arbitrary large garbage data.

Removing it is not sufficient, we need an alternative. One idea is using a var rather than const and making this definable compile time (eg in app.go or using a go linker flag)

x/wasm/types/validation.go

alpe · 2022-04-25T09:55:19Z

Otherwise, you can get honest validators to fill up the chain with arbitrary large garbage data.

Good point!
A custom ante handler that reads the params would be an option, too.

bigs · 2022-04-25T15:17:22Z

The param is ultimately read in the uncompress function which is used in the Keeper's create and importCode functions. This (more configurable) redundancy was my motivation to remove it from the message itself. I noticed my transaction was failing client side and never even sending to a validator to be rejected.

EDIT: The param is maxWasmCodeSize.

ash-burnt · 2022-04-25T16:09:20Z

The weird thing here is that there is a hardcoded variable that exists, and also a param for the same value. The value is exposed to governance, but not actually honored by governance. It seems like this should be one or the other, not both.

ethanfrey · 2022-04-25T20:43:25Z

The param is ultimately read in the uncompress function which is used in the Keeper's create and importCode functions. This (more configurable) redundancy was my motivation to remove it from the message itself. I noticed my transaction was failing client side and never even sending to a validator to be rejected.

Yes, and eg. create is only called in DeliverTx, not CheckTx. We want a sanity check before including 5 MB invalid wasm tx in the blockchain in the first place (which ValidateBasic will do, but nothing else, as not x/wasm handler logic is called in CheckTx)

I agree we should only have one value. I would make it a compile-time variable and remove the param completely. (And document this along with migration for chains when they upgrade)

bigs · 2022-04-25T22:37:39Z

Ah, makes sense. I think I had my wires a bit crossed, there! Compile time seems reasonable as does a configuration object passed to NewAppModule, perhaps? Happy to take this on when we come to something, sensible.

bigs · 2022-04-26T16:05:20Z

@alpe @ethanfrey @ash-burnt Digging in a bit further, it seems like there are a few options:

Change the const to a var, eliminate the conflicting module param, and respect the var in ValidateBasic. It would probably also entail removing the check in the Keeper, which seems redundant, but I'm not entirely sure on that.
Implement an additional AnteDecorator in x/wasm/keeper that accepts the wasmd module's param Subspace as an argument and validates messages storing contracts in a transaction accordingly. We would then update app's NewAnteHandler to optionally accept a Subspace in HandlerOptions. When present, it can add our decorator to the stack. Similarly, my gut tells me we could then do away with the Keeper validation.

I'm relatively new to the ecosystem, so I won't claim to know which is best. Option 1 is certainly simplest and requires minimal modification to anything. Option 2 allows us to modulate this parameter via governance. For example, the wasm-opt optimized byte code for cw721-metadata-onchain comes out to 253KB which is already pushing half of the statically defined maximum. I could see a chain starting with a maximum around 500KB and then revising it upwards. Doing so via governance rather than cutting a completely new version of the application could save some headaches!

ethanfrey · 2022-04-26T18:46:17Z

I could see a chain starting with a maximum around 500KB and then revising it upwards.

I think we advise 600KB or 1200KB as default parameters (unaware the const was still there).

I would have chains just start with something like 800KB-1000KB to start, and then if this really is an issue and some contract creator convinces them this is important, they just include this in the next upgrade. A contract creator should realise this well before production (when building code, running on testnet, etc) and I don't see a quick turnaround as critical.

Most chains do upgrades every 1-3 months, so that seems reasonable.

TL;DR all for the simple solution - using var and updating default to 800 KB or so

Also removes redundant maxWasmSize param

bigs · 2022-04-26T20:02:20Z

@ethanfrey Pushed up a change that should address it the way we discussed (force-pushed for cleaner history, which auto-closed the branch.) Let me know what you think! Also, for posterity's sake here is a diff that implements the params & AnteHandler based approach.

EDIT: Updated PR description, as well.

ethanfrey

Good stuff. I agree with this.

Can you add a CHANGELOG entry? And update the description to add:

"Addresses #813"

and

"The WasmCodeSize parameter is no longer used. You can set it by overriding x/wasm/types/MaxWasmSize at compile time (in app.go). The default is now set to 800kB but any chain can easily configure when doing a binary upgrade."

You can expand the text, but that is the rough idea that should be there, and linked from the CHANGELOG. I will merge once those cleanup tasks are done. Thanks for the code contribution.

ethanfrey · 2022-04-27T11:24:17Z

x/wasm/types/validation.go

 	// MaxLabelSize is the longest label that can be used when Instantiating a contract
 	MaxLabelSize = 128
 )

+// MaxWasmSize is the largest a compiled contract code can be when storing code on chain
+var MaxWasmSize uint64 = 800 * 1024


This is good.
Can also be larger if you wish (param was upped to 1200 * 1024 earlier).

But I think 800 or 900 is a good default

But... why make uint64 and not int? Since we compare it to length which returns an int

I made this call because one of its uses requires a uint64 and with split use cases, I went unsigned as a length cannot be negative. I truly don't mind changing it, though!

bigs · 2022-04-27T14:59:36Z

@ethanfrey Should be sorted!

alpe

Nice work! Some tests are failing. Can you run make test, please?

Would be great if can add some doc to
https://github.com/CosmWasm/wasmd#runtime-flags
for this new flag

alpe · 2022-04-27T16:26:41Z

x/wasm/types/validation.go

 	// MaxLabelSize is the longest label that can be used when Instantiating a contract
 	MaxLabelSize = 128
 )

+// MaxWasmSize is the largest a compiled contract code can be when storing code on chain
+var MaxWasmSize int = 800 * 1024


Just curious why you increased the value to 800kb? Do you have a contract example with this size? We had some internal discussion for tgrade yesterday where I looked at some of our contracts and max was 555kb.

Suggested change

var MaxWasmSize int = 800 * 1024

var MaxWasmSize int = 777 * 1024

ah, Ethan suggested 800kb. We should have coordinated better. 🤷 It would help with tgrade if you can make it 777kb

777kb is an odd number.
But tgrade can easily override this default, so maybe best to use 800 and verify we can change it in the imported one

bigs · 2022-04-27T18:08:04Z

Ah wow -- sorry for moving too quickly. I need to update the protobuf definitions for the params. That broke all the tests!

ethanfrey · 2022-04-27T18:17:05Z

Hmmm... is it possible to enable CI runs on this? I didn't realise the tests hadn't all run until @alpe pointed it out.

ethanfrey · 2022-04-27T18:30:15Z

BTW, you can look at #822 for example how to update the README and CHANGELOG. (Just add one more line to the README section I created)

alpe · 2022-04-29T11:09:30Z

@bigs: we have some dependencies on this feature, so I jumped in this morning to rebased your work on top of current main in a new PR (#826) and fixed tests and protobuf. I hope you don't mind.

Can you take a look at the new PR, if this still makes sense for you? Thanks a lot for your work!

bigs · 2022-04-29T13:48:34Z

Totally! I was held up the last two days with other things (least of which is not the fact that my work laptop has an Arm processor which the docker images don’t work with.) I’ll review and we can get this out the door.

…

On Fri, Apr 29, 2022 at 07:09 Alexander Peters ***@***.***> wrote: @bigs <https://github.com/bigs>: we have some dependencies on this feature, so I jumped in this morning to rebased your work on top of current main in a new PR (#826 <#826>) and fixed tests and protobuf. I hope you don't mind. Can you take a look at the new PR, if this still makes sense for you? Thanks a lot for your work! — Reply to this email directly, view it on GitHub <#809 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABUCWRLGXHV7JSRS7VEZNLVHO7PLANCNFSM5T2EEE5Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

bigs · 2022-04-29T15:24:21Z

Closing in favor of #826

bigs requested a review from alpe as a code owner April 19, 2022 21:40

webmaster128 added this to the v0.27.0 milestone Apr 21, 2022

ethanfrey reviewed Apr 25, 2022

View reviewed changes

x/wasm/types/validation.go Show resolved Hide resolved

ethanfrey added the optional Not absolutely required for the milestone label Apr 25, 2022

This was referenced Apr 25, 2022

Move constants to params #202

Closed

Replace many const with var #813

Closed

bigs closed this Apr 26, 2022

Convert MaxWasmSize to var

eacdb33

Also removes redundant maxWasmSize param

bigs reopened this Apr 26, 2022

bigs changed the title ~~Remove MaxWasmSize restriction in ValidateBasic~~ Consolidate MaxWasmSize constraints into a single var Apr 26, 2022

ethanfrey removed the optional Not absolutely required for the milestone label Apr 27, 2022

ethanfrey reviewed Apr 27, 2022

View reviewed changes

ethanfrey mentioned this pull request Apr 27, 2022

Make MaxLabelSize a var not const #822

Merged

bigs added 2 commits April 27, 2022 10:51

Convert MaxWasmSize to int

b4d6d58

Update CHANGELOG

c6356a3

alpe suggested changes Apr 27, 2022

View reviewed changes

alpe mentioned this pull request Apr 29, 2022

Consolidate MaxWasmSize constraints into a single var #826

Merged

bigs closed this Apr 29, 2022

ValarDragon mentioned this pull request May 18, 2022

chore: upgrade wasmd to v0.27.0.rc3-osmo and ibc-go to v3 osmosis-labs/osmosis#1527

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate MaxWasmSize constraints into a single var #809

Consolidate MaxWasmSize constraints into a single var #809

bigs commented Apr 19, 2022 •

edited by alpe

Loading

ethanfrey commented Apr 25, 2022

alpe commented Apr 25, 2022

bigs commented Apr 25, 2022 •

edited

Loading

ash-burnt commented Apr 25, 2022

ethanfrey commented Apr 25, 2022

bigs commented Apr 25, 2022

bigs commented Apr 26, 2022

ethanfrey commented Apr 26, 2022

bigs commented Apr 26, 2022 •

edited

Loading

ethanfrey left a comment

ethanfrey Apr 27, 2022

ethanfrey Apr 27, 2022

bigs Apr 27, 2022

bigs commented Apr 27, 2022

alpe left a comment

alpe Apr 27, 2022

alpe Apr 27, 2022

ethanfrey Apr 27, 2022

bigs commented Apr 27, 2022

ethanfrey commented Apr 27, 2022

ethanfrey commented Apr 27, 2022

alpe commented Apr 29, 2022

bigs commented Apr 29, 2022 via email

bigs commented Apr 29, 2022

	var MaxWasmSize int = 800 * 1024
	var MaxWasmSize int = 777 * 1024

Consolidate MaxWasmSize constraints into a single var #809

Consolidate MaxWasmSize constraints into a single var #809

Conversation

bigs commented Apr 19, 2022 • edited by alpe Loading

ethanfrey commented Apr 25, 2022

alpe commented Apr 25, 2022

bigs commented Apr 25, 2022 • edited Loading

ash-burnt commented Apr 25, 2022

ethanfrey commented Apr 25, 2022

bigs commented Apr 25, 2022

bigs commented Apr 26, 2022

ethanfrey commented Apr 26, 2022

bigs commented Apr 26, 2022 • edited Loading

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey Apr 27, 2022

Choose a reason for hiding this comment

ethanfrey Apr 27, 2022

Choose a reason for hiding this comment

bigs Apr 27, 2022

Choose a reason for hiding this comment

bigs commented Apr 27, 2022

alpe left a comment

Choose a reason for hiding this comment

alpe Apr 27, 2022

Choose a reason for hiding this comment

alpe Apr 27, 2022

Choose a reason for hiding this comment

ethanfrey Apr 27, 2022

Choose a reason for hiding this comment

bigs commented Apr 27, 2022

ethanfrey commented Apr 27, 2022

ethanfrey commented Apr 27, 2022

alpe commented Apr 29, 2022

bigs commented Apr 29, 2022 via email

bigs commented Apr 29, 2022

bigs commented Apr 19, 2022 •

edited by alpe

Loading

bigs commented Apr 25, 2022 •

edited

Loading

bigs commented Apr 26, 2022 •

edited

Loading