-
Notifications
You must be signed in to change notification settings - Fork 2.6k
contracts: Add salt
argument to contract instantiation
#7482
Conversation
This is a good idea, and it will fit nicely into solidity where the salt can be specified on contract creation:
Currently Solang appends the salt to the input. However, I do have concerns about updating the host function |
I understand your concerns. However, is is too early to start with compatibility. I am doing all the breaking changes right now. We start with semver and compatiblity once pallet_contracts is officially released/deployed and we gain agency over the pallet contracts release cycle or version numbers. Technically, it is "released" but only because it is part of the mono repo. In practice it is still considered pre-release. |
5fbbc8a
to
962d97a
Compare
1809417
to
dc8b4ce
Compare
why not just like Ethereum, add We always thought "hash(deploying_address ++ code_hash ++ hash(input_bytes))" may be your particular design... So in our Patract RedSpot, we provide a tools to generate a accounts in repeated test to avoid this situation( what means we change deploying_address in every deploy)
And on the other hand, in our contract testnet Jupiter, we change the rule of generate an address like: we add a stroage to distinguish whether this deploy need to use We think the old design "hash(deploying_address ++ code_hash ++ hash(input_bytes))" may be useful in some situations. Add a All in all, I think the old design could be reserved in some situation, so that the contracts could be unique in chain. |
Explained in the opening post: The new design is strictly more powerful. You are free to supply the account nonce as
It was. And it is changing as explained in the opening post. For the why. I think you answered that yourself pretty well: You had to a a lot of gymnastics so that your tool works with the old semantic.
You can replicate the old behaviour yourself if you think that is useful to your use case. Just hash your input data and supply it as |
dc8b4ce
to
7f611f2
Compare
/bench runtime pallet pallet_contracts |
Finished benchmark for branch: at-instantiate-contract Benchmark: Benchmark Runtime Pallet cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs ResultsPallet: "pallet_contracts", Extrinsic: "update_schedule", Lowest values: [], Highest values: [], Steps: [50], Repeat: 20
|
Those errors where part of the decl_error for some time but where never actually returned. This allows proper debugging of failed restorations. Previously, any error did return the misleading `ContractTrapped`.
This allows us to make assumptions about the AccoutId that are necessary for testing and in order to benchmark the module properly. This also groups free standing functions into inherent functions in order to minimize the places where the new bounds need to be specified.
* Do not allow override by runtime author * Instantiate gained a new parameter "salt" This change is done now in expecation of the upcoming code rent which needs to change the instantiation dispatchable and host function anyways. The situation in where we have only something that is like CREATE2 makes it impossible for UIs to help the user to create an arbitrary amount of instantiations from the same code. With this change we have the same functionality as ethereum with a CREATE and CREATE2 instantation semantic.
The new trait bounds allows us to remove this workaround from the configuration trait.
It should be solely the responsiblity to determine proper values for these parameter. As a matter of fact most runtime weren't using these values anyways.
Because of the new bounds on the trait tests can't get away by using u64 as accound id. Replacing the 8 byte value by a 32 byte value creates out quite a bit of code churn.
The benchmarks need adaption to the new instantiate semantics. * Fix compile errors caused by adding new trait bounds * Fix compile errors caused by renaming storage and rent functions * Adapt host functions and dispatchables to the new salt * Add tests for instantiate host functions (was not possible before)
The new benchmarks add a new parameter for salt "s" to the instantiate weights that needs to be applied.
dd710ca
to
320bd3d
Compare
This test is adapted to use the new instantiate signature.
/// This is the address generation function used by contract instantation. Its result | ||
/// is only dependend on its inputs. It can therefore be used to reliably predict the | ||
/// address of a contract. This is akin to the formular of eth's CRATE2 opcode. There | ||
/// is no CREATE equivalent because CREATE2 is strictly more powerful. |
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.
Sure we want ethereum terminology (CREATE2) in docs?
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.
Did not think that intensely about that. What is your recommendation?
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.
LGTM
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.
thanks!
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.
LGTM
bot merge |
Trying merge. |
* make ClientConfig public (paritytech#7544) * sc-basic-authorship: remove useless dependencies (paritytech#7550) Signed-off-by: koushiro <koushiro.cqx@gmail.com> * Add slashing events to elections-phragmen. (paritytech#7543) * Add slashing events to elections-phragmen. * Fix build * Apply suggestions from code review * Update frame/elections-phragmen/src/lib.rs * Update frame/elections-phragmen/src/lib.rs Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com> Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com> * Remove necessity to pass ConsensusEngineId when registering notifications protocol (paritytech#7549) * Remove necessity to pass ConsensusEngineId when registering notifications protocol * Line width * Fix tests protocol name * Other renames * Doc update * Change issue in TODO * sc-cli: replace bip39 with tiny-bip39 (paritytech#7551) Signed-off-by: koushiro <koushiro.cqx@gmail.com> * Add extra docs to on_initialize (paritytech#7552) * Add some extra on_initialize docs. * Address review comments. * More Extensible Multiaddress Format (paritytech#7380) * More extensible multiaddress format * update name * Don't depend on indices to define multiaddress type * Use MultiAddress in Node Template too! * reduce traits, fix build * support multiple `StaticLookup` * bump tx version * feedback * Fix weight template to remove ugliness in rust doc (paritytech#7565) fixed weight template * Cargo.lock: Run cargo update (paritytech#7553) * Cargo.lock: Run cargo update * Cargo.lock: Downgrade cc to v1.0.62 * Cargo.lock: Revert wasm-* updates * .github: Add dependabot config and thus enable dependabot (paritytech#7509) * .github: Add dependabot config and thus enable dependabot * Update .github/dependabot.yml Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> * Thread-local parameter_types for testing. (paritytech#7542) * Thread-local parameter_types for testing. * Better docs. * Some minors * Merge'em * Update frame/support/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Align more to basti's trick * Update frame/support/src/lib.rs * Update frame/support/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de> * Bump wasm-bindgen-test from 0.3.12 to 0.3.17 (paritytech#7567) * Bump wasm-bindgen-test from 0.3.12 to 0.3.17 Bumps [wasm-bindgen-test](https://github.com/rustwasm/wasm-bindgen) from 0.3.12 to 0.3.17. - [Release notes](https://github.com/rustwasm/wasm-bindgen/releases) - [Changelog](https://github.com/rustwasm/wasm-bindgen/blob/master/CHANGELOG.md) - [Commits](https://github.com/rustwasm/wasm-bindgen/commits) Signed-off-by: dependabot[bot] <support@github.com> * Update wasm-bindgen pin to 0.2.68 Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> * pallet-evm: move to Frontier (Part IV) (paritytech#7573) * Bump secrecy from 0.6.0 to 0.7.0 (paritytech#7568) * Bump secrecy from 0.6.0 to 0.7.0 Bumps [secrecy](https://github.com/iqlusioninc/crates) from 0.6.0 to 0.7.0. - [Release notes](https://github.com/iqlusioninc/crates/releases) - [Commits](iqlusioninc/crates@secrecy/v0.6.0...secrecy/v0.7.0) Signed-off-by: dependabot[bot] <support@github.com> * Fix compilation errors Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> * Bump wasm-bindgen-futures from 0.4.17 to 0.4.18 (paritytech#7572) Bumps [wasm-bindgen-futures](https://github.com/rustwasm/wasm-bindgen) from 0.4.17 to 0.4.18. - [Release notes](https://github.com/rustwasm/wasm-bindgen/releases) - [Changelog](https://github.com/rustwasm/wasm-bindgen/blob/master/CHANGELOG.md) - [Commits](https://github.com/rustwasm/wasm-bindgen/commits) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump lru from 0.4.3 to 0.6.1 (paritytech#7577) Bumps [lru](https://github.com/jeromefroe/lru-rs) from 0.4.3 to 0.6.1. - [Release notes](https://github.com/jeromefroe/lru-rs/releases) - [Changelog](https://github.com/jeromefroe/lru-rs/blob/master/CHANGELOG.md) - [Commits](jeromefroe/lru-rs@0.4.3...0.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump wasm-bindgen-test from 0.3.17 to 0.3.18 (paritytech#7579) Bumps [wasm-bindgen-test](https://github.com/rustwasm/wasm-bindgen) from 0.3.17 to 0.3.18. - [Release notes](https://github.com/rustwasm/wasm-bindgen/releases) - [Changelog](https://github.com/rustwasm/wasm-bindgen/blob/master/CHANGELOG.md) - [Commits](https://github.com/rustwasm/wasm-bindgen/commits) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump wasm-timer from 0.2.4 to 0.2.5 (paritytech#7578) Bumps [wasm-timer](https://github.com/tomaka/wasm-timer) from 0.2.4 to 0.2.5. - [Release notes](https://github.com/tomaka/wasm-timer/releases) - [Commits](https://github.com/tomaka/wasm-timer/commits) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * grandpa: remove light-client specific block import pipeline (paritytech#7546) * grandpa: remove light-client specific block import * consensus, network: remove finality proofs * client/authority-discovery: Publish and query on exponential interval (paritytech#7545) * client/authority-discovery: Publish and query on exponential interval When a node starts up publishing and querying might fail due to various reasons, for example due to being not yet fully bootstrapped on the DHT. Thus one should retry rather sooner than later. On the other hand, a long running node is likely well connected and thus timely retries are not needed. For this reasoning use an exponentially increasing interval for `publish_interval`, `query_interval` and `priority_group_set_interval` instead of a constant interval. * client/authority-discovery/src/interval.rs: Add license header * .maintain/gitlab: Ensure adder collator tests are run on CI * sc-network: update some dependencies (paritytech#7582) Signed-off-by: koushiro <koushiro.cqx@gmail.com> * Bump linregress and do some other cleanups (paritytech#7580) * Wasm-builder 3.0 (paritytech#7532) * Build every wasm crate in its own project with wasm-builder Building all wasm crates in one workspace was a nice idea, however it just introduced problems: 1. We needed to prune old members, but this didn't worked for old git deps. 2. We locked the whole wasm workspace while building one crate. This could lead to infinitely locking the workspace on a crash. Now we just build every crate in its own project, this means we will build the dependencies multiple times. While building the dependencies multiple times, we still decrease the build time by around 30 seconds for Polkadot and Substrate because of the new parallelism ;) * Remove the requirement on wasm-builder-runner This removes the requirement on wasm-builder-runner by using the new `build_dep` feature of cargo. We use nightly anyway and that enables us to use this feature. This solves the problem of not mixing build/proc-macro deps with normal deps. By doing this we get rid off this complicated project structure and can depend directly on `wasm-builder`. This also removes all the code from wasm-builder-runner and mentions that it is deprecated. * Copy the `Cargo.lock` to the correct folder * Remove wasm-builder-runner * Update docs * Fix deterministic check Modified-by: Bastian Köcher <git@kchr.de> * Try to make the ui test happy * Switch to `SKIP_WASM_BUILD` * Rename `SKIP_WASM_BINARY` to the correct name... * Update utils/wasm-builder/src/builder.rs Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> * Update utils/wasm-builder/src/builder.rs Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> * Bump tracing from 0.1.21 to 0.1.22 (paritytech#7589) Bumps [tracing](https://github.com/tokio-rs/tracing) from 0.1.21 to 0.1.22. - [Release notes](https://github.com/tokio-rs/tracing/releases) - [Commits](tokio-rs/tracing@tracing-0.1.21...tracing-0.1.22) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * contracts: Add `salt` argument to contract instantiation (paritytech#7482) * pallet-contracts: Fix seal_restore_to to output proper module errors Those errors where part of the decl_error for some time but where never actually returned. This allows proper debugging of failed restorations. Previously, any error did return the misleading `ContractTrapped`. * Bind UncheckedFrom<T::Hash> + AsRef<[u8]> everywhere This allows us to make assumptions about the AccoutId that are necessary for testing and in order to benchmark the module properly. This also groups free standing functions into inherent functions in order to minimize the places where the new bounds need to be specified. * Rework contract address determination * Do not allow override by runtime author * Instantiate gained a new parameter "salt" This change is done now in expecation of the upcoming code rent which needs to change the instantiation dispatchable and host function anyways. The situation in where we have only something that is like CREATE2 makes it impossible for UIs to help the user to create an arbitrary amount of instantiations from the same code. With this change we have the same functionality as ethereum with a CREATE and CREATE2 instantation semantic. * Remove TrieIdGenerator The new trait bounds allows us to remove this workaround from the configuration trait. * Remove default parameters for config trait It should be solely the responsiblity to determine proper values for these parameter. As a matter of fact most runtime weren't using these values anyways. * Fix tests for new account id type Because of the new bounds on the trait tests can't get away by using u64 as accound id. Replacing the 8 byte value by a 32 byte value creates out quite a bit of code churn. * Fix benchmarks The benchmarks need adaption to the new instantiate semantics. * Fix compile errors caused by adding new trait bounds * Fix compile errors caused by renaming storage and rent functions * Adapt host functions and dispatchables to the new salt * Add tests for instantiate host functions (was not possible before) * Add benchmark results * Adapt to the new WeightInfo The new benchmarks add a new parameter for salt "s" to the instantiate weights that needs to be applied. * Fix deploying_wasm_contract_should_work integration test This test is adapted to use the new instantiate signature. * Break overlong line * Break more long lines Co-authored-by: Parity Benchmarking Bot <admin@parity.io> * */Cargo.toml: Remove unused dependencies (paritytech#7590) * */Cargo.toml: Remove unused dependencies Using cargo-udeps to detect unused dependencies. * client/network/Cargo: Revert dependency removal * Cargo.lock: Update Co-authored-by: Andrew Plaza <aplaza@liquidthink.net> Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com> Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> Co-authored-by: Alexander Theißen <alex.theissen@me.com> Co-authored-by: Max Inden <mail@max-inden.de> Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Wei Tang <wei@that.world> Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> Co-authored-by: Parity Benchmarking Bot <admin@parity.io>
…7482) * pallet-contracts: Fix seal_restore_to to output proper module errors Those errors where part of the decl_error for some time but where never actually returned. This allows proper debugging of failed restorations. Previously, any error did return the misleading `ContractTrapped`. * Bind UncheckedFrom<T::Hash> + AsRef<[u8]> everywhere This allows us to make assumptions about the AccoutId that are necessary for testing and in order to benchmark the module properly. This also groups free standing functions into inherent functions in order to minimize the places where the new bounds need to be specified. * Rework contract address determination * Do not allow override by runtime author * Instantiate gained a new parameter "salt" This change is done now in expecation of the upcoming code rent which needs to change the instantiation dispatchable and host function anyways. The situation in where we have only something that is like CREATE2 makes it impossible for UIs to help the user to create an arbitrary amount of instantiations from the same code. With this change we have the same functionality as ethereum with a CREATE and CREATE2 instantation semantic. * Remove TrieIdGenerator The new trait bounds allows us to remove this workaround from the configuration trait. * Remove default parameters for config trait It should be solely the responsiblity to determine proper values for these parameter. As a matter of fact most runtime weren't using these values anyways. * Fix tests for new account id type Because of the new bounds on the trait tests can't get away by using u64 as accound id. Replacing the 8 byte value by a 32 byte value creates out quite a bit of code churn. * Fix benchmarks The benchmarks need adaption to the new instantiate semantics. * Fix compile errors caused by adding new trait bounds * Fix compile errors caused by renaming storage and rent functions * Adapt host functions and dispatchables to the new salt * Add tests for instantiate host functions (was not possible before) * Add benchmark results * Adapt to the new WeightInfo The new benchmarks add a new parameter for salt "s" to the instantiate weights that needs to be applied. * Fix deploying_wasm_contract_should_work integration test This test is adapted to use the new instantiate signature. * Break overlong line * Break more long lines Co-authored-by: Parity Benchmarking Bot <admin@parity.io>
This PR adds a
salt: &[u8]
argument to theinstantiate
dispatchable and theseal_instantiate
host function. The new formula for contract address derivation is:This is akin to Ethereum's CREATE2 semantic.
This PR can be reviewed on a commit by commit basis. The description of each commit contains more information on what happened there.
Motivation
Currently, the formula to to derive a new contract address looks like this:
This formula has two problems: First, a generic UI cannot help the user to deploy the same contract multiple times. Even though the formula theoretically allows for deploying the same contract from the same sender multiple times this support needs to be baked into the contract code and the UI needs to be aware of that. Today, this is not the case. Second, hashing the contract input for every input is wasteful because it could be a a large data blob while something shorter would suffice as input for the address derivation.
The change in this PR solves these problems: The input data is no longer hashed and a generic UI could suggest the account nonce or some off-chain generated random number as default value for the salt. Since the
salt
is just raw bytes it could also be used to implement the old behaviour by supplying the hash of the input data assalt
.Please note that the decision against implementing CREATE semantic where the account nonce is used for the contract address is intentional: The new salt semantic is strictly more powerful and is needed anyways for counterfactual contract deployment.
Implementation Hurdles
Up until now the actual contract address derivation was performed by a runtime configuration supplied function. Theoretically, pallet_contracts wasn't able to make any assumptions about how the contract address was derived. In practice, assumptions must be made in order to write proper benchmarks which use the actual runtime configuration. Additionally, the whole construct is a workaround because
pallet_system::Trait::AccountId
is missing some trait bounds that allow properAccountId
derivation. This PR makes the pallet_contracts require those trait bounds and subsequently internalizes runtime passed derivation functions. The fallout from that is minimized by grouping many free standing functions to inherent functions to reduce the littering withwhere
clauses. See rfc-2089.Porting Guide
Client
@jacogr
Clients need to be aware of an additional argument to the
instantiate
dispatchable which has a new argumentsalt
:substrate/frame/contracts/src/lib.rs
Line 513 in dc8b4ce
Contracts
@Robbepop @seanyoung
Contracts need to be aware that the
seal_instantiate
host function has the same new argument:substrate/frame/contracts/src/wasm/runtime.rs
Lines 860 to 861 in 1809417
Runtime Integration
@ascjones
DetermineContractAddress
andTrieIdGenerator
are no longer part of thepallet_contracts::Trait
. They can just be removed from the configuration.Additionally those types are removed from the pallet public interface:
The pallet contracts cannot possible define sane defaults for rent parameters because that is an economic decision each runtime has to make. The rest of the defaults is removed for consistency and similar reason. Have a look at the substrate node for guidance.