Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

pallet-utility: Fix possible mismatch between native/wasm #10121

Merged
merged 2 commits into from
Oct 30, 2021

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Oct 29, 2021

The batched_calls_limit constant value includes the size_of of the runtime Call. As we compile
the runtime for native/wasm, we need to align the call size to ensure that it is the same on
wasm/native. This also solves the problem of different metadata outputs for the same runtime.

Progress on: paritytech/polkadot-sdk#934

CC @Slesarew

The `batched_calls_limit` constant value includes the `size_of` of the runtime `Call`. As we compile
the runtime for native/wasm, we need to align the call size to ensure that it is the same on
wasm/native. This also solves the problem of different metadata outputs for the same runtime.
@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Oct 29, 2021
@bkchr bkchr requested a review from thiolliere October 29, 2021 16:54
Copy link
Contributor

@thiolliere thiolliere left a comment

Choose a reason for hiding this comment

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

damn, I didn't foresee this.

in polkadot/kusama we have test that call is not too big and less than 230.

in case native and wasm size are not aligning like 1025 and 1023 we would still have the issue, but I guess 1024 is big enough indeed.

We could also add an integrity test to ensure call size is no more than 1024 as we run integrity test with native it should be fine.

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

Was there actually a difference in native vs wasm though?

Caused by what?

@shawntabrizi shawntabrizi added this to In progress in Runtime via automation Oct 30, 2021
@thiolliere
Copy link
Contributor

Was there actually a difference in native vs wasm though?

Caused by what?

the size of the call have some type which size are dependent on the platform, like a vec has a different size on 32bit or 64bit platform.

@bkchr
Copy link
Member Author

bkchr commented Oct 30, 2021

Was there actually a difference in native vs wasm though?

Caused by what?

mem::size_of. Depends also on how the compiler lays out the type and that depends on the architecture.

@bkchr bkchr merged commit e11a670 into master Oct 30, 2021
Runtime automation moved this from In progress to Done Oct 30, 2021
@bkchr bkchr deleted the bkchr-utility-mismatch branch October 30, 2021 13:09
@shawntabrizi shawntabrizi moved this from Done to Archive in Runtime Nov 11, 2021
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
…#10121)

* pallet-utility: Fix possible mismatch between native/wasm

The `batched_calls_limit` constant value includes the `size_of` of the runtime `Call`. As we compile
the runtime for native/wasm, we need to align the call size to ensure that it is the same on
wasm/native. This also solves the problem of different metadata outputs for the same runtime.

* Review feedback
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…#10121)

* pallet-utility: Fix possible mismatch between native/wasm

The `batched_calls_limit` constant value includes the `size_of` of the runtime `Call`. As we compile
the runtime for native/wasm, we need to align the call size to ensure that it is the same on
wasm/native. This also solves the problem of different metadata outputs for the same runtime.

* Review feedback
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants