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

Improve call, and usage in pallet utility #9418

Merged
12 commits merged into from
Aug 7, 2021
Merged

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Jul 22, 2021

this decrease the length of call to less than 200 (184 exactly) bytes.
and improve the usage in utility.

companion: paritytech/polkadot#3522

@gui1117 gui1117 requested a review from andresilva as a code owner July 22, 2021 14:43
@gui1117 gui1117 added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited B0-silent Changes should not be mentioned in any release notes B3-apinoteworthy and removed B0-silent Changes should not be mentioned in any release notes labels Jul 22, 2021
@gui1117 gui1117 changed the title Improve call Improve call, and usage in pallet utility Jul 22, 2021
@gui1117
Copy link
Contributor Author

gui1117 commented Jul 22, 2021

@jacogr I have 1 question and 1 note:

  • Some calls now have a Box<...> for their type instead of just the type directly, the encoding is same as the inner type. Does this needs consideration from UI ?
    (EDIT: in the metadata Box<Foo> will appear instead of Foo)
  • batch and batch_all now have a strong limit on the number of batched calls, the limit can be found on metadata, this can also needs consideration from UI.

any remark is welcomed

@jacogr
Copy link
Contributor

jacogr commented Jul 22, 2021

Box wrappings are ignored and stripped on the type parsing. (It has no meaning outside the Rust code)

If there is a batch limit on constants, can be read.

Comment on lines 1630 to 1638
#[test]
fn call_size() {
assert!(
core::mem::size_of::<Call>() <= 200,
"size of Call is more than 200: some calls have too big arguments, use Box to reduce the
size of Call.
If the limit is too strong, maybe consider increase the limit to 300.",
);
}
Copy link
Member

@ordian ordian Jul 22, 2021

Choose a reason for hiding this comment

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

have you considered making a static_assertion instead to catch this at compile time instead?
https://docs.rs/static_assertions/1.1.0/static_assertions/macro.const_assert.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave a try but I think the error message is better with a regular assert.
But all in all this assert is not really important there is no important issue with using a big call enum

error[E0080]: evaluation of constant value failed
    --> bin/node/runtime/src/lib.rs:1632:3
     |
1632 | /         static_assertions::const_assert!(
1633 | |             core::mem::size_of::<Call>() <= 20,
1634 | |         );
     | |__________^ attempt to compute `0_usize - 1_usize`, which would overflow
     |
     = note: this error originates in the macro `static_assertions::const_assert` (in Nightly builds, run with -Z macro-backtrace for more info)

Copy link
Member

Choose a reason for hiding this comment

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

Until const panics are a thing, static assertions are rather useless.

#[test]
fn call_size() {
assert!(
core::mem::size_of::<Call>() <= 200,
Copy link
Member

Choose a reason for hiding this comment

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

200 is an arbitrary number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes IIRC it is the same number as clippy lint

Copy link
Contributor

Choose a reason for hiding this comment

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

what clippy lint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

looks okay to me, probably need more input from others

bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 1630 to 1638
#[test]
fn call_size() {
assert!(
core::mem::size_of::<Call>() <= 200,
"size of Call is more than 200: some calls have too big arguments, use Box to reduce the
size of Call.
If the limit is too strong, maybe consider increase the limit to 300.",
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Until const panics are a thing, static assertions are rather useless.

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

@shawntabrizi
Copy link
Member

shawntabrizi commented Aug 4, 2021

if we want to prevent a large memory allocation before decoding, this pr is not good enough. this would only prevent it after decode

the proposed solution from sr labs is to instead make the signature of batch take vec<box<call>> where now we know each box<call> will be 4 bytes, and then the vec should have no problems w/ size before decode.

However, this is potentially a huge performance decrease, especially as the vec size increases

TODO: Follow up

@gui1117
Copy link
Contributor Author

gui1117 commented Aug 4, 2021

the proposed solution from sr labs is to instead make the signature of batch take vec<box<call>> where now we know each box<call> will be 4 bytes, and then the vec should have no problems w/ size before decode.

it is implemented here #9339

@bkchr
Copy link
Member

bkchr commented Aug 4, 2021

if we want to prevent a large memory allocation before decoding, this pr is not good enough. this would only prevent it after decode

If we can not decode a call, it is invalid anyway. I don't see the reason why we need to work here on better solutions.

@gui1117
Copy link
Contributor Author

gui1117 commented Aug 6, 2021

I think this PR improve call by reducing its size, and makes utility batch function limit more explicit for users.

So we can merge it. Otherwise we can close it

@bkchr
Copy link
Member

bkchr commented Aug 6, 2021

I'm for merging this.

@shawntabrizi
Copy link
Member

okay, then lets go ahead and do that :)

I just wanted to post what was said in my last meeting with the auditors.

@gui1117 gui1117 added D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Aug 7, 2021
@gui1117
Copy link
Contributor Author

gui1117 commented Aug 7, 2021

bot merge

@ghost
Copy link

ghost commented Aug 7, 2021

Waiting for commit status.

@ghost ghost merged commit 670ca51 into master Aug 7, 2021
@ghost ghost deleted the gui-finally-improve-call branch August 7, 2021 09:34
ghost pushed a commit to paritytech/polkadot that referenced this pull request Aug 7, 2021
* add test for call size

* fix box arg

* fix xcm variant length + increase limit a bit

* fix para sudo wrapper call length

* reorganize

* fmt

* fix tests

* update Substrate

Co-authored-by: parity-processbot <>
@stze stze added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Aug 11, 2021
ggwpez pushed a commit to ggwpez/runtimes that referenced this pull request Mar 10, 2023
* add test for call size

* fix box arg

* fix xcm variant length + increase limit a bit

* fix para sudo wrapper call length

* reorganize

* fmt

* fix tests

* update Substrate

Co-authored-by: parity-processbot <>
ggwpez pushed a commit to ggwpez/runtimes that referenced this pull request Jul 13, 2023
* add test for call size

* fix box arg

* fix xcm variant length + increase limit a bit

* fix para sudo wrapper call length

* reorganize

* fmt

* fix tests

* update Substrate

Co-authored-by: parity-processbot <>
This pull request was closed.
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. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants