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.

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

Wasm Serialization check seems to use hardcoded default instead of configurable value #1660

Closed
systemzax opened this issue Sep 20, 2023 · 3 comments · Fixed by #1770 or #1775
Closed
Assignees
Labels
👍 lgtm OCI Work exclusive to OCI team
Milestone

Comments

@systemzax
Copy link
Member

As I was attempting to deploy a (rather) large contract with several include headers statements, I received the "Too many function defs" error message.

Here is where this check is evaluated from what I can tell :

constexpr size_t max_size = eosio::chain::wasm_constraints::maximum_section_elements;

And this points to a constexpr set to 1024 :

constexpr unsigned maximum_section_elements = 1024; //elements

However, the wasm_parameters struct and the "cleos get consensus_parameters" indicate "max_section_elements": 8192.

I wonder if this is a leftover artefact due to an oversight, and should indeed be checking the wasm_parameters config instead of the hardcoded limit.

@spoonincode
Copy link
Member

spoonincode commented Sep 20, 2023

Did you activate CONFIGURABLE_WASM_LIMITS2? It appears cleos get consensus_parameters returns a wasm_config reflective of post-CONFIGURABLE_WASM_LIMITS2 activation, even when CONFIGURABLE_WASM_LIMITS2 hasn't been activated. (I'm not sure how much to consider that behavior a defect on first take.. the problem is post-CONFIGURABLE_WASM_LIMITS2 limits aren't 1:1 to those prior to to activation... it's clearly deceptive, but would a better alternative to be not to include wasm_config here prior to activation 🤔 )

@systemzax
Copy link
Member Author

Ah, that would explain it then. After activating the feature, I was able to deploy the contract, presumably because the max section elements is now effectively 8192 as reported via the get consensus_parameters API. Thanks!

@bhazzard
Copy link

bhazzard commented Sep 21, 2023

Since this protocol feature is activated on most chains, this one mostly affects folks spinning up test chains.

The most direct path is probably to simply not include wasm_config if the CONFIGURABLE_WASM_LIMITS2 protocol feature is not activated.

@bhazzard bhazzard added this to the Leap v4.0.5 milestone Sep 21, 2023
@bhazzard bhazzard added 👍 lgtm and removed triage labels Sep 21, 2023
@heifner heifner self-assigned this Oct 13, 2023
@heifner heifner added the OCI Work exclusive to OCI team label Oct 13, 2023
@heifner heifner moved this from Todo to In Progress in Team Backlog Oct 13, 2023
heifner added a commit that referenced this issue Oct 13, 2023
heifner added a commit that referenced this issue Oct 13, 2023
[4.0] Only return wasm config settings if configurable wasm limits enabled
heifner added a commit that referenced this issue Oct 13, 2023
heifner added a commit that referenced this issue Oct 13, 2023
[4.0 -> 5.0] Only return wasm config settings if configurable wasm limits enabled
heifner added a commit that referenced this issue Oct 13, 2023
heifner added a commit that referenced this issue Oct 16, 2023
[5.0 -> main] Only return wasm config settings if configurable wasm limits enabled
@github-project-automation github-project-automation bot moved this from In Progress to Done in Team Backlog Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment