-
Notifications
You must be signed in to change notification settings - Fork 256
Add benchmark weights for permissioned EVM extension #3554
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
Conversation
97fd4bc to
da4803a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now ready for another review.
c8701d8 to
2ef4eba
Compare
2ef4eba to
7ea98c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see only benmarks for signed calls. what about unsigned calls ?
7ea98c2 to
f949403
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some ways to simplify the code a bit, and implemented bare self-contained call extension weights.
I see only benmarks for signed calls. what about unsigned calls ?
The signed and unsigned codepaths are almost identical, so there's no point in having a separate benchmark. The only difference is a HashSet lookup on the signed path, which makes it a tiny amount more expensive:
subspace/domains/pallets/evm-tracker/src/create_contract.rs
Lines 28 to 69 in 7ea98c2
| pub fn is_create_contract_allowed<Runtime>( | |
| call: &RuntimeCallFor<Runtime>, | |
| signer: &EthereumAccountId, | |
| ) -> (bool, u32) | |
| where | |
| Runtime: frame_system::Config<AccountId = EthereumAccountId> | |
| + pallet_ethereum::Config | |
| + pallet_evm::Config | |
| + crate::Config, | |
| RuntimeCallFor<Runtime>: | |
| MaybeIntoEthCall<Runtime> + MaybeIntoEvmCall<Runtime> + MaybeNestedCall<Runtime>, | |
| Result<pallet_ethereum::RawOrigin, OriginFor<Runtime>>: From<OriginFor<Runtime>>, | |
| { | |
| // If the account is allowed to create contracts, or it's not a contract call, return true. | |
| // Only enters allocating code if this account can't create contracts. | |
| if crate::Pallet::<Runtime>::is_allowed_to_create_contracts(signer) { | |
| return (true, 0); | |
| } | |
| let (is_create, call_count) = is_create_contract::<Runtime>(call); | |
| (!is_create, call_count) | |
| } | |
| /// If anyone is allowed to create contracts, allows contracts. Otherwise, rejects contracts. | |
| /// Returns false if the call is a contract call, and there is a specific (possibly empty) allow | |
| /// list. Otherwise, returns true. | |
| pub fn is_create_unsigned_contract_allowed<Runtime>(call: &RuntimeCallFor<Runtime>) -> (bool, u32) | |
| where | |
| Runtime: frame_system::Config + pallet_ethereum::Config + pallet_evm::Config + crate::Config, | |
| RuntimeCallFor<Runtime>: | |
| MaybeIntoEthCall<Runtime> + MaybeIntoEvmCall<Runtime> + MaybeNestedCall<Runtime>, | |
| Result<pallet_ethereum::RawOrigin, OriginFor<Runtime>>: From<OriginFor<Runtime>>, | |
| { | |
| // If any account is allowed to create contracts, or it's not a contract call, return true. | |
| // Only enters allocating code if there is a contract creation filter. | |
| if crate::Pallet::<Runtime>::is_allowed_to_create_unsigned_contracts() { | |
| return (true, 0); | |
| } | |
| let (is_create, call_count) = is_create_contract::<Runtime>(call); | |
| (!is_create, call_count) | |
| } |
subspace/crates/sp-domains/src/lib.rs
Lines 885 to 894 in 7ea98c2
| pub fn is_allowed(&self, who: &AccountId) -> bool { | |
| match self { | |
| PermissionedActionAllowedBy::Accounts(accounts) => accounts.contains(who), | |
| PermissionedActionAllowedBy::Anyone => true, | |
| } | |
| } | |
| pub fn is_anyone_allowed(&self) -> bool { | |
| matches!(self, PermissionedActionAllowedBy::Anyone) | |
| } |
This is explained in the benchmark comments here:
https://github.com/autonomys/subspace/pull/3554/files#diff-ea89c9f2e5298171277613b2228895b8521f1d3bd265b436caa2d04269c43f1eR78
A similar logic applies to self-contained calls, which make exactly the same method calls as signed calls, and do similar checks, but inside the fp_self_contained::SelfContainedCall implementation.
f949403 to
9f5c715
Compare
|
This is ready for another review. |
|
@NingLin-P the weights in this PR are only used in one runtime, can you confirm that it is ok to do it this way, or would you like them moved into the runtime itself? |
Have they been generated from the reference machine ? |
This PR has already been mostly reviewed, so I'd like to avoid blocking merging this PR on other related tasks. We have multiple weights that need to be re-generated on the reference machine and moved to the runtimes:
It seems easier to do them both at the same time in a separate PR. As part of that PR I'd also like to update the benchmark script so it automatically picks up new weights (ticket #3582). We're also missing the documentation for the reference machine benchmarking process/hardware (#3587), which makes it harder to do benchmarking on the reference machine. @NingLin-P did you document the reference benchmarking process somewhere? |
I think we can keep the weight in Line 41 in 945f113
So next time when we update the weight with the reference machine, this extension weight will also be updated automatically. |
This PR adds substrate weights for the permissioned EVM extension that checks if contract creation is allowed.
We decided to use the default benchmark file template in PR #3129, but that template assumes the file is outside the crate. We might want to add a template which replaces the crate name with literally
crate. (But is otherwise the same as the default.)Close #3530.
Code contributor checklist: