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

Adapt pallet-contracts to WeightV2 #12421

Merged
merged 7 commits into from
Oct 5, 2022
Merged

Conversation

athei
Copy link
Member

@athei athei commented Oct 4, 2022

A few changes were necessary to make pallet-contracts compatible with the new weight.

Remove ContractAccessWeight

This was a workaround used to emulate the huge PoV impact generated by loading a contract. This is now replaced by setting the proof_size to the size of the loaded contract. This is an underestimation but the whole system is just in place to prevent the worst offenders. Charging for anything else is missing anyways and will come later.

Return the whole Weight struct from dry runs

Dry runs now include the proof_size so that clients can use this to learn about how much proof_size a transaction will consume.

Fix inter contract calls

seal_call and seal_instantiate allow only setting 1D weight. Eventually we need to add new versions of those. But the old versions need fixing, too. Current behavior is to set proof_size to 0 which is not a good default. It will prevent the callee to do anything useful. This PR sets the proof_size to "inherit" which allows it to use all of the remaining proof size from the overarching limit.

Fix compatibility extrinsics

When adding the 2D weight we also added *_old_weight compat extrinsics to allow in-storage extrinsics to be still decoded. However, they also set proof_size to 0 which makes them useless. This PR sets some sensible default so they continue working. We also removed duplication in docs and code from them.

cumulus companion: paritytech/cumulus#1726
Closes #12410

@athei athei added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Oct 4, 2022
@ascjones
Copy link
Contributor

ascjones commented Oct 5, 2022

Closes #12410

Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM, though I would really like to see some tests for the proof_size accounting

@@ -824,6 +823,8 @@ fn deploy_and_call_other_contract() {
// Drop previous events
initialize_block(2);

println!("--------------");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: is this intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. Some leftover. Thanks.

/// Used by backwards compatible extrinsics. We cannot just set the proof to zero
/// or an old `Call` will just fail.
fn compat_weight(gas_limit: OldWeight) -> Weight {
Weight::from(gas_limit).set_proof_size(u64::from(T::MaxCodeLen::get()) * 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question: Is the MacCodeLen * 2 always a sufficient approximation of gas?

Copy link
Member Author

@athei athei Oct 5, 2022

Choose a reason for hiding this comment

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

Depends on how many contracts you call and how big they are in a single transaction. This is just a ad-hoc solution to support in-storage Calls that might exist in a Scheduler of some runtime. This is very niche. You should use the dry-run to come up with a proper value and don't rely on this.

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing this!

@athei
Copy link
Member Author

athei commented Oct 5, 2022

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/cumulus#1726

@athei
Copy link
Member Author

athei commented Oct 5, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 7a8de49 into master Oct 5, 2022
@paritytech-processbot paritytech-processbot bot deleted the at/contracts-pov branch October 5, 2022 11:10
ordian added a commit that referenced this pull request Oct 5, 2022
* master: (42 commits)
  Adapt `pallet-contracts` to WeightV2 (#12421)
  Improved election pallet testing (#12327)
  Bump prost to 0.11+ (#12419)
  Use saturating add for alliance::disband witness data (#12418)
  [Fix] Rename VoterBagsList -> VoterList to match pdot (#12416)
  client/beefy: small code improvements (#12414)
  BEEFY: Simplify hashing for pallet-beefy-mmr (#12393)
  Add @koute to `docs/CODEOWNERS` and update stale paths (#12408)
  docs/CODEOWNERS: add @acatangiu as MMR owner (#12406)
  Remove unnecessary Clone trait bounds on CountedStorageMap (#12402)
  Fix `Weight::is_zero` (#12396)
  Beefy on-demand justifications as a custom RequestResponse protocol (#12124)
  Remove contracts RPCs (#12358)
  pallet-mmr: generate historical proofs (#12324)
  unsafe_pruning flag removed (#12385)
  Carry over where clauses defined in Config to Call and Hook (#12388)
  Properly set the max proof size weight on defaults and tests (#12383)
  BEEFY: impl TypeInfo for SignedCommitment (#12382)
  bounding staking: `BoundedElectionProvider` trait (#12362)
  New Pallet: Root offences (#11943)
  ...
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Replace contract access weight by proper PoV component

* Return the whole weight struct from dry-runs

* Fixup `seal_call` and `seal_instantiate`

* Fix duplicate extrinsics

* Remove ContractAccessWeight from runtime

* Fix doc link

* Remove leftover debugging output
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. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Contracts: return proof_size with ContractResult from dry-runs
4 participants