-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[Revive] Implement general gas tracking #10166
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
base: master
Are you sure you want to change the base?
Conversation
f2c48f5 to
cce6cc5
Compare
Fix maxPriorityFee RPC Change the EVM call opcodes to use proper gas for subcalls Update tests-evm.yml Update from github-actions[bot] running command 'prdoc --audience runtime_dev' fix fix [pallet-revive] fix subxt submit & add debug statments (#10016) - Fix subxt submit by default it's using `author_submitAndWatchExtrinsic` even though we just want to fire and forget - Add debug instructions to log the signer & nonce of new eth transactions when the node validate the transaction --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Version bumps and prdocs reordering from stable2509 (#9974) This PR backports regular version bumps and prdocs reordering from the stable2509 branch back to master --------- Co-authored-by: ParityReleases <[email protected]> Update .github/workflows/tests-evm.yml [Release|CI/CD] Fix polkadot prod docker image (#9975) This PR introduces a workaround to fix failing polkadot production image flow. The initial issue is that, for some reason, our key that used to sign the deb `InRelease` repo noted as expired on the first `apt update` run. But reimport of the same key fixes is it. Until the reason for this issue is fixed, this work around helps to keep the flow working Introduce `/cmd label` for labelling pull requests (#9915) This allows external contributors to set label for their pull request. Closes: #9873 Use parity-large-persistent-test for merge queue (#10025) Investigating issue with removing persistent runners from merge queue cc paritytech/devops#3875 pallet_revive: Lower the deposit costs for child trie items (#10027) Fixes #9246 --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> pallet_revive: Fix incorrect `block.gaslimit` (#10026) Fixes paritytech/contract-issues#112 --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> update tests-evm
cce6cc5 to
bd60888
Compare
bd60888 to
43e1bd1
Compare
|
@TorstenStueber Hello, thanks for the quick fix. I tested this by running the Uniswap v2 periphery test suite with the latest code from the For comparison, the same test suite has only 9 failing cases when run against the latest master branch. It's worth noting that although the error messages in Hardhat might differ between the failing tests, I've observed on the node that the root cause for all these failed transactions is an |
|
@sekisamu thanks for the message. Can you add instructions how to run the tests in that repository? E.g.,
|
|
@TorstenStueber Hello, I've updated the issue description and include the info you require.
And I've also tried again on the latest commit of |
|
@sekisamu it is fixed on the latest commit of this PR and both test suites (https://github.com/papermoonio/v2-periphery-polkadot/tree/revm and https://github.com/papermoonio/eth-transfer-test) are 100% successful. |
…--pallet pallet_revive'
|
Command "bench --runtime dev --pallet pallet_revive" has finished ✅ See logs here Subweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
…k into torsten/gas-fixes
pgherveou
left a comment
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.
The tests in substrate/frame/revive/src/metering/tests.rs are a bit hard to decipher
but otherwise looks good to me, approving now to unblock, will make a few more passes to make sure I am not missing anything.
| } | ||
|
|
||
| #[test] | ||
| fn substrate_metering_charges_works() { |
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.
these tests and values are a bit hard to decrypt
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.
Two things:
- The meaning of numbers in these tuples are unclear: It is just long tuples of numbers and I can add more structure to it but the test cases will then consume more space and the test file will become large.
- The numbers themselves are unclear: I can add another validation function that will make it more obvious how these numbers have been calculated.
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.
maybe this AI refactoring helps make things more readable / manageable by moving values to a JSON file like we do for precompile vector test
pg/gas-fix-comment...pg/gas-fix-comment-2
I am bit worried that all these tests seems to depend on exact gas value that might change with benchmark updates.
If there are no ways around that, let's use this approach, we can them find way to automate the test fixture updates
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.
Now worries. The tests are completely independent of concrete weights and benchmarks. I was aware that this would have been a nightmare to maintain.
The logic in the test are all directly fed values to the meters, no weight tokens that depend on weights.
I will have a look at the AI generated code.
539f6ef to
116ba4f
Compare
|
@TorstenStueber is this in a state where we could use it in anvil to run our integration tests to ensure compatibility? (after rebasing on top of latest master and fixing the conflicts) |
@alindima Yes, it is now! |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
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.
Really like the refactorings here, too. Couldn't find anything obviously wrong. Most of my comments are nits (we should really put a lot of error into explaining the fields of the ResourceMeter). Except for some questions around the gas stipend.
| max_total_gas: SignedGas<T>, | ||
| total_consumed_weight_before: Weight, | ||
| total_consumed_deposit_before: DepositOf<T>, | ||
|
|
||
| transaction_limits: TransactionLimits<T>, |
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.
Can you move the docs from above to the individual items as rust doc? Easier to understand what each thing is. Having good docs on data structurs is much more important than on functions. I can understand what it does if I understand the data structures.
For example, I don't understand what the difference is between max_total_gas and eth_gas_limit. I think having good docs for each field here would go a long way of explaining what is happening. i.e explaining why we need each field.
| math::substrate_execution::new_nested_meter(self, limit), | ||
| }?; | ||
|
|
||
| new_meter.adjust_effective_weight_limit()?; |
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.
Shouldn't this be called by the new_* functions?
| // if this is provided, we will additionally ensure that execution will not exhaust this | ||
| // weight limit | ||
| maybe_weight_limit: Option<Weight>, |
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.
When is is provided and when it isn't?
| pub mod gas; | ||
| pub mod math; | ||
| pub mod storage; | ||
| pub mod weight; |
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.
Are they pub on purpose? I would assume that most types in those modules are only used in this file. Maybe consider selectively pub use what you need?
| impl Sealed for super::Nested {} | ||
| } | ||
|
|
||
| #[cfg(test)] |
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.
Can you move tests to their own file (sub module) since you are touching it anyways?
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
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.
Can you move to its own file since you are touching this anyways?
| storage_bytes: u32, | ||
| pub storage_bytes: u32, | ||
| /// How many items of storage are accumulated in this contract's child trie. | ||
| storage_items: u32, | ||
| pub storage_items: u32, | ||
| /// This records to how much deposit the accumulated `storage_bytes` amount to. | ||
| pub storage_byte_deposit: BalanceOf<T>, | ||
| /// This records to how much deposit the accumulated `storage_items` amount to. | ||
| storage_item_deposit: BalanceOf<T>, | ||
| pub storage_item_deposit: BalanceOf<T>, | ||
| /// This records how much deposit is put down in order to pay for the contract itself. | ||
| /// | ||
| /// We need to store this information separately so it is not used when calculating any refunds | ||
| /// since the base deposit can only ever be refunded on contract termination. | ||
| storage_base_deposit: BalanceOf<T>, | ||
| pub storage_base_deposit: BalanceOf<T>, | ||
| /// The size of the immutable data of this contract. | ||
| immutable_data_len: u32, | ||
| pub immutable_data_len: u32, |
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 was hoping we could down on the public fields here and not increase them :D Where do we need to access them?
| // We use ALL_STIPEND to detect the typical gas limit solc defines as a call stipend | ||
| // This is just a heuristic | ||
| let add_stipend = | ||
| !value.is_zero() || gas_limit.try_into().is_ok_and(|limit: u64| limit == CALL_STIPEND); |
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 think those should be two conditionals:
- If
value !=0: Setgas_limitto at least the stipend (no heuristic just EVM semantics). Not all value transfers are plain transfers. Could call into more complicated logic. Don't think we can mess with disabling reentrancy here. - If
limit == CALL_STIPEND: Only this part is a heuristic. Transfer detected. Disable reentrancy on top of setting the limit to at least the stipend.
| if add_stipend { | ||
| ReentrancyProtection::AllowNext |
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 don't think we should disable reentrancy for ever CALL with value != 0. They are not necessarily plain transfers.
| let stipend = if *add_stipend { | ||
| let weight_stipend = determine_call_stipend::<T>(); | ||
| if weight_left.any_lt(weight_stipend) { | ||
| return Err(<Error<T>>::OutOfGas.into()) | ||
| } | ||
|
|
||
| weight_limit.saturating_accrue(weight_stipend); | ||
|
|
||
| Some(weight_stipend) |
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.
AFAIK the stipend is not added to the limit. I think its weight_limit = max(weight_limit, stipend).
fyi @mokita-j created a branch in foundry polkadot that compile against this branch |
xermicus
left a comment
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 looks very good. Thanks!
I ran the revive compiler integration test suite against this branch and saw no regressions introduced vs. master.
| clearStorageSlot(0); | ||
| clearStorageSlot(1); |
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.
| clearStorageSlot(0); | |
| clearStorageSlot(1); | |
| uint slot; | |
| assembly { | |
| slot := a.slot | |
| } | |
| clearStorageSlot(slot); | |
| assembly { | |
| slot := b.slot | |
| } | |
| clearStorageSlot(slot); |
Nit: This always gets the actual storage slot (makes it easier to refactor the test)
This PR implements the general gas tracking spec.
Follow-up PR to address gas scale: #10393
This PR ballooned into something much bigger than I expected. Many of the changes are due to the fact that all tests and a lot of the other logic has some touch points with the resource management logic. Most of the actual changes in logic are just in the folder
meteringof pallet-revive.The main changes are that
eth_transactextrinsics.Metering logic
Almost all changes in this PR are confined to the folder
meteringof pallet-revive. Before this PR there were two meters: a weight meter and a gas meter. They have now been combined into a main meter calledResourceMeter. Outside code only interacts with theResourceMeterand not individually with the gas or storage meter. The reason is that in Ethereum execution mode gas is a shared resource and interacting with one meter influences the limits of the other meter.Here are some finer points:
meteringfolderResourceMeter, most functions now don't use a separate gas meter and deposit meter anymore but just aResourceMeterRootandNested), there are two kind ofResourceMeter: the top-levelTransactionMeterused at the beginning of a transaction and aFrameMeterused once per frameTransactionMeterare specified through theTransactionLimitstype, which distinguishes between Substrate and Ethereum execution mode.FrameMeteris specified through the typeCallResources, which can either be a) no limits (e.g., in the case of contract creation), or b) a weight and deposit, or c) a gas limit.enforce_limithas been renamed tofinalizeas that describes the semantics bettertry_into_deposithas been renamed toexecute_postponed_depositsabsorb_weight_meter_only: when a frame reverts. In this case we ignore all storage deposits from the reverting frame. We still need to absorb the observed maximum deposit so that we determine the correct maximum deposit during dry running.absorb_all_meters: when a frame was successfuleffective_weight_limit, which needs to be recalculated whenever the deposit meter changes and is for optimization purposes.Option<...>. When it isNone, then this represents unlimited meters and this is only used for Ethereum style executions (the meters are not really unlimited, there will be a gas limit that effectively limits the resource usages of the weight and deposit meters).sync_to_executorandsync_from_executorare a bit simplified and there is no need forengine_fuel_leftanymore.Other Changes
gasfor weights has been consistently replaced byweighteth_callandeth_instantiate_with_codenow take aweight_limit(used to ensure that weight does not exceed the max extrinsic weight) and aneth_gas_limit(the new externally defined limit)compute_max_integer_quotientandcompute_max_integer_pair_quotient(defined insubstrate/frame/revive/src/evm/frees.rs) are meant to divide a number by the next fee multiplierGasMapperanymore as it will now be fed directly with the Ethereum gas values instead of weightsStrictprotection andAllowNextAllowNextallows to re-enter the same contract but only for the next frame. This is required to implement reentrancy protection for simple transfers with call stipendsStrictprotection we setallows_reentryof the caller tofalsebefore the creation of the new frame, forAllowNextwe to it after the creationu64::MAX(as discussed with @pgherveou)max_storage_depositin the deposit meter). For example, if a transaction encounters a storage deposit that is later refunded, then the total storage deposit is zero. However, the caller needs to provide enough resources so that temporarily the execution does not run out of gas and terminates the call prematurely.try_upload_codenow always takes a meter and records the storage deposit charge thereFixes
This fixes a couple of issues
OutOfGaserrors for some transactions contract-issues#208TODOs
with_ethereum_context)ExecConfigSignedGaseffective_gas_priceinstead of next fee multipliereffective_gas_price* used gasensure_not_overdrawnOther TODOs