Fix generated address returned by Substrate RPC runtime call#8504
Fix generated address returned by Substrate RPC runtime call#8504
Conversation
|
/cmd prdoc --audience runtime_dev --bump patch |
…time_dev --bump patch'
pgherveou
left a comment
There was a problem hiding this comment.
skimmed trough it quickly, will take another look in a bit
|
/cmd fmt |
|
can you cherry pick this eb1fe2b I want to make sure that we are not breaking evm-test-suite here |
substrate/frame/revive/src/lib.rs
Outdated
| Self::convert_native_to_evm(value), | ||
| data, | ||
| storage_deposit_limit.is_unchecked(), | ||
| exec_context, |
There was a problem hiding this comment.
This is not good. Before the skip transfer was depending on whether we had a storage deposit limit. Now it is passed separately. We can have the situation where the limit is unchecked but the transfer is not skipped. I don't think we want that.
| gas_limit.unwrap_or(RuntimeBlockWeights::get().max_block), | ||
| pallet_revive::DepositLimit::Balance(storage_deposit_limit.unwrap_or(u128::MAX)), | ||
| input_data, | ||
| pallet_revive::ExecContext::Transaction, |
There was a problem hiding this comment.
This is dry_run. Same for the others.
| #[derive(Clone, Copy, Debug, PartialEq, Eq)] | ||
| pub enum ExecContext { | ||
| /// A normal transaction execution, where the nonce has been pre-incremented. | ||
| Transaction, | ||
|
|
||
| /// A dry run execution (through RPC), where the nonce has not been incremented. | ||
| DryRun { skip_transfer: bool }, | ||
| } |
There was a problem hiding this comment.
I don't think it makes sense to merge bump nonce logic with skip_transfer. Whether the transfer is skipped is already determined by the storage_deposit_limit argument. This also leads to the sitation that we need to add this arg to the call where it doesn't matter if we have to bump or not. We are also passing through this whole exec context even if we don't need the information later (as opposed to skip_transfer which we need for the whole stack duration).
I suggest just sticking to the original enum BumpNonce design. What do you think @pgherveou?
There was a problem hiding this comment.
Yes you are right my bad, you can just pass the BumpNonce as an extra argument to run so we only carry it on the Stack
https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/revive/src/exec.rs?plain=1#L1078-L1074
|
/cmd prdoc --audience runtime_dev --bump patch |
|
Command "prdoc --audience runtime_dev --bump patch" has failed ❌! See logs here |
cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs
Outdated
Show resolved
Hide resolved
|
I will have another look when the CI is green. The ones flagged as required. |
Co-authored-by: PG Herveou <pgherveou@gmail.com>
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
* master: (99 commits) Snowbridge: Remove asset location check for compatibility (#8473) add poke_deposit extrinsic to pallet-bounties (#8382) litep2p/peerset: Reject non-reserved peers in the reserved-only mode (#8650) Charge deposit based on key length (#8648) [pallet-revive] make subscription task panic on error (#8587) tx/metrics: Add metrics for the RPC v2 `transactionWatch_v1_submitAndWatch` (#8345) Bridges: Fix - Improve try-state for pallet-xcm-bridge-hub (#8615) Introduce CreateBare, deprecated CreateInherent (#7597) Use hashbrown hashmap/hashset in validation context (#8606) ci: rm gitlab config (#8622) 🔪 flaky and Zombienet tests (#8600) cumulus: adjust unincluded segment size metric buckets (#8617) Benchmark storage access on block validation (#8069) Revert 7934 es/remove tj changes (#8611) collator-protocol: add more collation observability (#8230) `fatxpool`: add fallback for ready at light (#8533) txpool: fix tx removal from unlocks set (#8500) XCMP weight metering: account for the MQ page position (#8344) fix epmb solution duplicate issue + add remote mining apparatus to epm (#8585) Fix generated address returned by Substrate RPC runtime call (#8504) ...
- Revert #8504 - Add a `prepare_dry_run` that run before bare_call / bare_instantiate --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
- Revert #8504 - Add a `prepare_dry_run` that run before bare_call / bare_instantiate --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
## Description
When dry-running a contract deployment through the runtime API, the
returned address does not match the actual address that will be used
when the transaction is submitted. This inconsistency occurs because the
address derivation logic doesn't properly account for the difference
between transaction execution and dry-run execution contexts.
The issue stems from the `create1` address derivation logic in
`exec.rs`:
```rust
address::create1(
&deployer,
// the Nonce from the origin has been incremented pre-dispatch, so we
// need to subtract 1 to get the nonce at the time of the call.
if origin_is_caller {
account_nonce.saturating_sub(1u32.into()).saturated_into()
} else {
account_nonce.saturated_into()
},
)
```
The code correctly subtracts 1 from the account nonce during a
transaction execution (because the nonce is incremented pre-dispatch),
but doesn't account for execution context - whether it's a real
transaction or a dry run through the RPC.
## Review Notes
This PR adds a new condition to check for the `IncrementOnce` when
calculating the nonce for address derivation:
```rust
address::create1(
&deployer,
// the Nonce from the origin has been incremented pre-dispatch, so we
// need to subtract 1 to get the nonce at the time of the call.
if origin_is_caller && matches!(exec_context, IncrementOnce::AlreadyIncremented) {
account_nonce.saturating_sub(1u32.into()).saturated_into()
} else {
account_nonce.saturated_into()
},
)
```
## Before Fix
- Dry-run contract deployment returns address derived with nonce N
- Actual transaction deployment creates contract at address derived with
nonce N-1
- Result: Inconsistent addresses between simulation and actual execution
## After Fix
- Dry-run and actual transaction deployments both create contracts at
the same address
- Result: Consistent contract addresses regardless of execution context
- Added test case to verify nonce handling in different execution
contexts
This fix ensures that users can rely on the address returned by a dry
run to match the actual address that will be used when the transaction
is submitted.
Fixes paritytech/contract-issues#37
# Checklist
* [x] My PR includes a detailed description as outlined in the
"Description" and its two subsections above.
* [x] My PR follows the [labeling requirements](
https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
) of this project (at minimum one label for `T` required)
* External contributors: ask maintainers to put the right label on your
PR.
* [x] I have made corresponding changes to the documentation (if
applicable)
* [x] I have added tests that prove my fix is effective or that my
feature works (if applicable)
---------
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: pgherveou <pgherveou@gmail.com>
- Revert #8504 - Add a `prepare_dry_run` that run before bare_call / bare_instantiate --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Description
When dry-running a contract deployment through the runtime API, the returned address does not match the actual address that will be used when the transaction is submitted. This inconsistency occurs because the address derivation logic doesn't properly account for the difference between transaction execution and dry-run execution contexts.
The issue stems from the
create1address derivation logic inexec.rs:The code correctly subtracts 1 from the account nonce during a transaction execution (because the nonce is incremented pre-dispatch), but doesn't account for execution context - whether it's a real transaction or a dry run through the RPC.
Review Notes
This PR adds a new condition to check for the
IncrementOncewhen calculating the nonce for address derivation:Before Fix
After Fix
This fix ensures that users can rely on the address returned by a dry run to match the actual address that will be used when the transaction is submitted.
Fixes paritytech/contract-issues#37
Checklist
Trequired)