Allow for "Nick's Method" Style Transactions & Contract Deployments in Revive#10476
Allow for "Nick's Method" Style Transactions & Contract Deployments in Revive#10476
Conversation
|
/cmd prdoc --audience runtime_dev --bump patch |
…time_dev --bump patch'
Differential Tests Results (PolkaVM)Specified Tests
Counts
FailuresThe test specifiers seen in this section have the format 'path::case_idx::compilation_mode' and they're compatible with the revive differential tests framework and can be specified to it directly in the same way that they're provided through the The failures are provided in an expandable section to ensure that the PR does not get polluted with information. Please click on the section below for more information Detailed Differential Tests Failure Information
|
Differential Tests Results (REVM)Specified Tests
Counts
FailuresThe test specifiers seen in this section have the format 'path::case_idx::compilation_mode' and they're compatible with the revive differential tests framework and can be specified to it directly in the same way that they're provided through the The failures are provided in an expandable section to ensure that the PR does not get polluted with information. Please click on the section below for more information Detailed Differential Tests Failure Information
|
pgherveou
left a comment
There was a problem hiding this comment.
I am not sure we should allow that though I think the competitor disallow it now
can you double check it?
for example this is what I found in Geth
https://github.com/ethereum/go-ethereum/blob/6f2cbb7a27ba7e62b0bdb2090755ef0d271714be/internal/ethapi/api.go?plain=1#L1557-L1560
removing chain-id protection (for legacy tx) seems like a step backward 🤔
as you mentioned seems to only be disabled at the rpc layer quickly checked other implementation, these changes seems to match what other implementation do |
|
@pgherveou pre EIP155 transactions are still supported AFAIK -> https://github.com/papermoonio/pre-eip155-hardhat (Moonbase and Sepolia). Typically, applications are recommended to switch to EIP155 transaction signing, including the actual chain ID. One option could be to include a client flag to force EIP155 check, etc. |
xermicus
left a comment
There was a problem hiding this comment.
I'm not convinced that this is a good idea and we need it. The referenced EIP is marked as stagnant (so, not live) anyways.
| // We would like to allow for transactions without a chain id to be executed through pallet | ||
| // revive. These are called unprotected transactions and they are transactions that predate | ||
| // EIP-155 which do not include a Chain ID. These transactions are still useful today in certain | ||
| // patterns in Ethereum such as "Nick's Method" for contract deployment which allows a contract | ||
| // to be deployed on all chains with the same address. This is only allowed for legacy | ||
| // transactions and isn't allowed for any other transaction type. | ||
| // * Here's a relevant EIP: https://eips.ethereum.org/EIPS/eip-2470 | ||
| // * Here's Nick's article: https://weka.medium.com/how-to-send-ether-to-11-440-people-187e332566b7 |
There was a problem hiding this comment.
Where would you recommend the doc comment to go? AFAIK there is no place to add a doc comment other than functions, types, etc, so there's no place that would accept a doc comment here
| // We would like to allow for transactions without a chain id to be executed through pallet | ||
| // revive. These are called unprotected transactions and they are transactions that predate | ||
| // EIP-155 which do not include a Chain ID. These transactions are still useful today in certain | ||
| // patterns in Ethereum such as "Nick's Method" for contract deployment which allows a contract | ||
| // to be deployed on all chains with the same address. This is only allowed for legacy | ||
| // transactions and isn't allowed for any other transaction type. | ||
| // * Here's a relevant EIP: https://eips.ethereum.org/EIPS/eip-2470 | ||
| // * Here's Nick's article: https://weka.medium.com/how-to-send-ether-to-11-440-people-187e332566b7 |
There was a problem hiding this comment.
The linked article doesn't talk about chain IDs at all. What am I missing?
There was a problem hiding this comment.
No, Nick's method is not really talking about chain IDs at all, but for the contracts mentioned in the issue they are not using EIP2718 transaction envelope or EIP155, meaning that the contracts don't expect the chain-id as part of the RLP encode being signed.
This means that if we don't support pre-eip155 transactions (of legacy type) then we won't be able to deploy some of the contracts that use Nick's method to ensure the same address across different chains.
With type 2 transactions, the chain ID is included in the RLP encode but the v value is no longer relevant.
There was a problem hiding this comment.
This means that if we don't support pre-eip155 transactions (of legacy type) then we won't be able to deploy some of the contracts that use Nick's method to ensure the same address across different chains.
FWIW we don't have to do this hack since we have governance on Polkadot.
I added a section in the PR doc that shows the various competitors and proof that this is allowed. Additionally, this adheres to EIP-155.
This is the RPC check I mentioned in the PR doc, you can see that it can be skipped if
It's not removed, it's only removed for legacy transactions that do not include it. |
Yup, this is a good idea and it matches what other chains do too where there is an additional check in the RPC layer that can be disabled through a flag. I added that to this PR. |
|
Can you add the protection to the RPC in this PR? If we allow it in the runtime we also need to add the same protection Geth has at the same time. |
# Description This is a PR that updates the commit hash of the revive-differential-tests framework and the compilation caches to a version that includes fixes to certain tests that used hard-coded gas values. The compilation caches required an update since this was a change to the contract's code. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Torsten Stüber <[email protected]>
|
@athei Yup, I just added it. I believe that with this we match Geth's behavior at the protocol level and the RPC level. |
There was a problem hiding this comment.
There should only be one prdoc file per PR.
|
@pgherveou @0xOmarA, any updates on when this will be merged and available on Passet Hub? We need it to deploy hyperbridge contracts. |
|
This is almost done. But author is in holidays already. I'll take this over and backport to the release branch. |
Thank you |
…n Revive (#10476) # Description This pull request allows for “[Nick’s Method](https://weka.medium.com/how-to-send-ether-to-11-440-people-187e332566b7)” style of deploying contracts which was requested in paritytech/contract-issues#225 and was attempted to be solved in paritytech/contract-issues#99. The “Nick Method” style of contract deployment is a very useful concept to use when we wish to deploy a contract on multiple chains **with the same address**. It allows us to do that by constructing a deployment transaction that can be executed on any chain, thus allowing us to get the same address for the contract’s deployment on any chain. Additionally, this method allows for the contracts to be deployed trustlessly, meaning that if any actor (regardless of whether they’re honest or not) follow the same method for deploying the contract then they’d get the same address across all chains. This allows anyone in the Polkadot ecosystem to take existing contracts that use this method of deployment (e.g., `Multicall3` and the ERC-1820 registry) and deploy them on Polkadot and there’s already trust that the code is the same. In order to be able to use the same transaction across multiple chains transaction authors need to: - Not include a chain id. - Include a high gas limit so that the transaction works on (almost) all chains. As mentioned in paritytech/contract-issues#225, this pattern is already being used in a number of contracts. Most notably, `Multicall3` and the ERC-1820 Registry. Before this PR this method didn’t work in revive since the chain id was an absolute must and any transaction that didn’t include a chain id would fail with an `Invalid Transaction` error as seen below: https://github.com/paritytech/polkadot-sdk/blob/c688963f51c55b3c2a16a00a33c4a086792a1544/substrate/frame/revive/src/evm/call.rs#L71-L76 The above implementation misses an important detail: legacy transactions are permitted to not have the chain id set while all other non-legacy transactions do not permit that. The models we had for legacy transactions respected that, but we still did the chain id check for all transaction types, which is incorrect. Therefore, this part of the code was changed to the following: ```rust match (tx.chain_id, tx.r#type.as_ref()) { (None, Some(super::Byte(TYPE_LEGACY))) => {}, (Some(chain_id), ..) => if chain_id != <T as Config>::ChainId::get().into() { log::debug!(target: LOG_TARGET, "Invalid chain_id {chain_id:?}"); return Err(InvalidTransaction::Call); }, (None, ..) => { log::debug!(target: LOG_TARGET, "Invalid chain_id None"); return Err(InvalidTransaction::Call); }, } ``` The above code skips the chain id check if the transaction is of the legacy type. Otherwise, the chain id is checked. If no chain id is provided and the transaction is not of the legacy type then we error out. ## Integration Be aware that we now allow for legacy transactions to not have the chain ID set in order to allow for “[Nick’s Method](https://weka.medium.com/how-to-send-ether-to-11-440-people-187e332566b7)” style of contract deployment. Non-legacy transaction continue to require the chain id to be provided. ## Review Notes - The main change that this PR makes can be found in the `substrate/frame/revive/src/evm/call.rs` file allowing for legacy contracts to not have the chain id set. - Two new tests were added with this PR: - `dry_run_contract_deployment_with_nick_method_works` - `contract_deployment_with_nick_method_works` - Both of the above tests test that “[Nick’s Method](https://weka.medium.com/how-to-send-ether-to-11-440-people-187e332566b7)” can be used to deploy the singleton factory contract provided in [ERC-2470](https://eips.ethereum.org/EIPS/eip-2470) in a dry run and in an `eth_transact` call. - Note that the above tests needed to modify the transaction provided in the ERC to update its gas limit (since the provided gas limit was too low for our platform), **which breaks the whole idea of Nick’s Method where the same transaction can be submitted to the same chain**. I suspect that #10393 should fix this issue with the new gas scaling. ## Behavior On Other Chains Since some of the comments on this PR talked about whether this is a good idea, I wanted to add this section as justification for adding this and also to review what other chains do with regards to unprotected transactions. I think that the security of this feature can be justified by: * The fact that this feature is allowed on most other EVM chains, including Ethereum mainnet itself. * That metamask will fill in the chain-id automatically for users (as seen [here](https://docs.metamask.io/wallet/how-to/send-transactions?utm_source=chatgpt.com#chain-id)), thus preventing users from broadcasting a re-playable transaction. * That the default behavior of Alloy and other developer tools is to fill in the chain ID when the developer is constructing transactions. * That people who want to use this feature need to start their own RPC with the `--allow-unprotected-txs` flag set The behavior of Geth when it comes to unprotected transactions is: * They're allowed at the protocol level for legacy transactions. * They're disallowed at the protocol level for other transaction types. * They're not allowed to be submitted through the Geth RPC unless the `--rpc.allow_unprotected_txs` flag is set when Geth is started. The above behavior matches what EIP-155 set in place for legacy transactions. The following is an overview of the various chains and their stance on unprotected transactions: * Ethereum/Geth: Allowed at the protocol level, the node must be started with a special flag to permit it to be submitted through the RPC ([source](https://geth.ethereum.org/docs/fundamentals/command-line-options)) * Avalanche: Allowed at the protocol level, the node must be started with a special flag to permit it to be submitted through the RPC ([source](https://build.avax.network/docs/nodes/chain-configs/subnet-evm#transaction-processing)) * Polygon: Allowed at the protocol level, the node must be started with a special flag to permit it to be submitted through the RPC ([source](https://forum.polygon.technology/t/bor-v0-4-0-mainnet-release-indore-fork/12280)) * Arbitrum: Allowed at the protocol level, the node must be started with a special flag to permit it to be submitted through the RPC ([source](https://pkg.go.dev/github.com/nim4/go-arbitrum/cmd/utils#:~:text=AllowUnprotectedTxs%20%3D%20%26cli.BoolFlag%7B%0A%09%09Name%3A%20%20%20%20%20%22rpc.allow%2Dunprotected%2Dtxs%22%2C%0A%09%09Usage%3A%20%20%20%20%22Allow%20for%20unprotected%20(non%20EIP155%20signed)%20transactions%20to%20be%20submitted%20via%20RPC%22%2C%0A%09%09Category%3A%20flags.APICategory%2C%0A%09%7D)) --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Torsten Stüber <[email protected]> Co-authored-by: Alexander Theißen <[email protected]> (cherry picked from commit 0241d9b)
|
Successfully created backport PR for |
…n Revive (#10476) # Description This pull request allows for “[Nick’s Method](https://weka.medium.com/how-to-send-ether-to-11-440-people-187e332566b7)” style of deploying contracts which was requested in paritytech/contract-issues#225 and was attempted to be solved in paritytech/contract-issues#99. The “Nick Method” style of contract deployment is a very useful concept to use when we wish to deploy a contract on multiple chains **with the same address**. It allows us to do that by constructing a deployment transaction that can be executed on any chain, thus allowing us to get the same address for the contract’s deployment on any chain. Additionally, this method allows for the contracts to be deployed trustlessly, meaning that if any actor (regardless of whether they’re honest or not) follow the same method for deploying the contract then they’d get the same address across all chains. This allows anyone in the Polkadot ecosystem to take existing contracts that use this method of deployment (e.g., `Multicall3` and the ERC-1820 registry) and deploy them on Polkadot and there’s already trust that the code is the same. In order to be able to use the same transaction across multiple chains transaction authors need to: - Not include a chain id. - Include a high gas limit so that the transaction works on (almost) all chains. As mentioned in paritytech/contract-issues#225, this pattern is already being used in a number of contracts. Most notably, `Multicall3` and the ERC-1820 Registry. Before this PR this method didn’t work in revive since the chain id was an absolute must and any transaction that didn’t include a chain id would fail with an `Invalid Transaction` error as seen below: https://github.com/paritytech/polkadot-sdk/blob/c688963f51c55b3c2a16a00a33c4a086792a1544/substrate/frame/revive/src/evm/call.rs#L71-L76 The above implementation misses an important detail: legacy transactions are permitted to not have the chain id set while all other non-legacy transactions do not permit that. The models we had for legacy transactions respected that, but we still did the chain id check for all transaction types, which is incorrect. Therefore, this part of the code was changed to the following: ```rust match (tx.chain_id, tx.r#type.as_ref()) { (None, Some(super::Byte(TYPE_LEGACY))) => {}, (Some(chain_id), ..) => if chain_id != <T as Config>::ChainId::get().into() { log::debug!(target: LOG_TARGET, "Invalid chain_id {chain_id:?}"); return Err(InvalidTransaction::Call); }, (None, ..) => { log::debug!(target: LOG_TARGET, "Invalid chain_id None"); return Err(InvalidTransaction::Call); }, } ``` The above code skips the chain id check if the transaction is of the legacy type. Otherwise, the chain id is checked. If no chain id is provided and the transaction is not of the legacy type then we error out. ## Integration Be aware that we now allow for legacy transactions to not have the chain ID set in order to allow for “[Nick’s Method](https://weka.medium.com/how-to-send-ether-to-11-440-people-187e332566b7)” style of contract deployment. Non-legacy transaction continue to require the chain id to be provided. ## Review Notes - The main change that this PR makes can be found in the `substrate/frame/revive/src/evm/call.rs` file allowing for legacy contracts to not have the chain id set. - Two new tests were added with this PR: - `dry_run_contract_deployment_with_nick_method_works` - `contract_deployment_with_nick_method_works` - Both of the above tests test that “[Nick’s Method](https://weka.medium.com/how-to-send-ether-to-11-440-people-187e332566b7)” can be used to deploy the singleton factory contract provided in [ERC-2470](https://eips.ethereum.org/EIPS/eip-2470) in a dry run and in an `eth_transact` call. - Note that the above tests needed to modify the transaction provided in the ERC to update its gas limit (since the provided gas limit was too low for our platform), **which breaks the whole idea of Nick’s Method where the same transaction can be submitted to the same chain**. I suspect that #10393 should fix this issue with the new gas scaling. ## Behavior On Other Chains Since some of the comments on this PR talked about whether this is a good idea, I wanted to add this section as justification for adding this and also to review what other chains do with regards to unprotected transactions. I think that the security of this feature can be justified by: * The fact that this feature is allowed on most other EVM chains, including Ethereum mainnet itself. * That metamask will fill in the chain-id automatically for users (as seen [here](https://docs.metamask.io/wallet/how-to/send-transactions?utm_source=chatgpt.com#chain-id)), thus preventing users from broadcasting a re-playable transaction. * That the default behavior of Alloy and other developer tools is to fill in the chain ID when the developer is constructing transactions. * That people who want to use this feature need to start their own RPC with the `--allow-unprotected-txs` flag set The behavior of Geth when it comes to unprotected transactions is: * They're allowed at the protocol level for legacy transactions. * They're disallowed at the protocol level for other transaction types. * They're not allowed to be submitted through the Geth RPC unless the `--rpc.allow_unprotected_txs` flag is set when Geth is started. The above behavior matches what EIP-155 set in place for legacy transactions. The following is an overview of the various chains and their stance on unprotected transactions: * Ethereum/Geth: Allowed at the protocol level, the node must be started with a special flag to permit it to be submitted through the RPC ([source](https://geth.ethereum.org/docs/fundamentals/command-line-options)) * Avalanche: Allowed at the protocol level, the node must be started with a special flag to permit it to be submitted through the RPC ([source](https://build.avax.network/docs/nodes/chain-configs/subnet-evm#transaction-processing)) * Polygon: Allowed at the protocol level, the node must be started with a special flag to permit it to be submitted through the RPC ([source](https://forum.polygon.technology/t/bor-v0-4-0-mainnet-release-indore-fork/12280)) * Arbitrum: Allowed at the protocol level, the node must be started with a special flag to permit it to be submitted through the RPC ([source](https://pkg.go.dev/github.com/nim4/go-arbitrum/cmd/utils#:~:text=AllowUnprotectedTxs%20%3D%20%26cli.BoolFlag%7B%0A%09%09Name%3A%20%20%20%20%20%22rpc.allow%2Dunprotected%2Dtxs%22%2C%0A%09%09Usage%3A%20%20%20%20%22Allow%20for%20unprotected%20(non%20EIP155%20signed)%20transactions%20to%20be%20submitted%20via%20RPC%22%2C%0A%09%09Category%3A%20flags.APICategory%2C%0A%09%7D)) --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Torsten Stüber <[email protected]> Co-authored-by: Alexander Theißen <[email protected]> (cherry picked from commit 0241d9b)
|
Successfully created backport PR for |
Backport #10476 into `unstable2507` from 0xOmarA. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Omar <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Torsten Stüber <[email protected]> Co-authored-by: Alexander Theißen <[email protected]>
Backport #10476 into `stable2512` from 0xOmarA. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Omar <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Torsten Stüber <[email protected]> Co-authored-by: Alexander Theißen <[email protected]>
|
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/release-of-smart-contracts-on-polkadot/16848/3 |
Description
This pull request allows for “Nick’s Method” style of deploying contracts which was requested in paritytech/contract-issues#225 and was attempted to be solved in paritytech/contract-issues#99.
The “Nick Method” style of contract deployment is a very useful concept to use when we wish to deploy a contract on multiple chains with the same address. It allows us to do that by constructing a deployment transaction that can be executed on any chain, thus allowing us to get the same address for the contract’s deployment on any chain.
Additionally, this method allows for the contracts to be deployed trustlessly, meaning that if any actor (regardless of whether they’re honest or not) follow the same method for deploying the contract then they’d get the same address across all chains. This allows anyone in the Polkadot ecosystem to take existing contracts that use this method of deployment (e.g.,
Multicall3and the ERC-1820 registry) and deploy them on Polkadot and there’s already trust that the code is the same.In order to be able to use the same transaction across multiple chains transaction authors need to:
As mentioned in paritytech/contract-issues#225, this pattern is already being used in a number of contracts. Most notably,
Multicall3and the ERC-1820 Registry.Before this PR this method didn’t work in revive since the chain id was an absolute must and any transaction that didn’t include a chain id would fail with an
Invalid Transactionerror as seen below:polkadot-sdk/substrate/frame/revive/src/evm/call.rs
Lines 71 to 76 in c688963
The above implementation misses an important detail: legacy transactions are permitted to not have the chain id set while all other non-legacy transactions do not permit that. The models we had for legacy transactions respected that, but we still did the chain id check for all transaction types, which is incorrect. Therefore, this part of the code was changed to the following:
The above code skips the chain id check if the transaction is of the legacy type. Otherwise, the chain id is checked. If no chain id is provided and the transaction is not of the legacy type then we error out.
Integration
Be aware that we now allow for legacy transactions to not have the chain ID set in order to allow for “Nick’s Method” style of contract deployment. Non-legacy transaction continue to require the chain id to be provided.
Review Notes
substrate/frame/revive/src/evm/call.rsfile allowing for legacy contracts to not have the chain id set.dry_run_contract_deployment_with_nick_method_workscontract_deployment_with_nick_method_workseth_transactcall.Behavior On Other Chains
Since some of the comments on this PR talked about whether this is a good idea, I wanted to add this section as justification for adding this and also to review what other chains do with regards to unprotected transactions.
I think that the security of this feature can be justified by:
--allow-unprotected-txsflag setThe behavior of Geth when it comes to unprotected transactions is:
--rpc.allow_unprotected_txsflag is set when Geth is started.The above behavior matches what EIP-155 set in place for legacy transactions.
The following is an overview of the various chains and their stance on unprotected transactions: