pallet_revive: Replace adhoc pre-compiles with pre-compile framework#8262
pallet_revive: Replace adhoc pre-compiles with pre-compile framework#8262
Conversation
|
/cmd fmt |
|
/cmd bench --runtime dev --pallet pallet_revive |
|
Command "bench --runtime dev --pallet pallet_revive" has started 🚀 See logs here |
…--pallet pallet_revive'
|
Command "bench --runtime dev --pallet pallet_revive" has finished ✅ See logs here DetailsSubweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
|
/cmd bench --runtime dev --pallet pallet_revive |
|
Command "bench --runtime dev --pallet pallet_revive" has started 🚀 See logs here |
…--pallet pallet_revive'
|
Command "bench --runtime dev --pallet pallet_revive" has finished ✅ See logs here DetailsSubweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
Co-authored-by: PG Herveou <pgherveou@gmail.com>
Co-authored-by: PG Herveou <pgherveou@gmail.com>
xermicus
left a comment
There was a problem hiding this comment.
Looks really good!
Just one thing: I tried the revive integration tests against this branch and there is a failing case passing against master: Delegate calls to unknown (non-existing) contract address revert with an out of gas error. I could not pin down the root cause quickly but to me this looks like a small regression?
Co-authored-by: xermicus <cyrill@parity.io>
Can you send me a link to the test code in question? |
|
I see the out of gas revert not only for the zero address but any arbitrary address I set there. |
|
I made a change that the gas for loading the code from storage is charged from the nested meter and not the parent meter. But in your test all remaining gas is passed. So it it should not affect this.
Weird. As if delegate calls don't work at all. However, we have tests for delegate calls which continued to work unchanged. I have to investigate further. I will try to run this test against my dev node and add some logs. |
|
The arguments stay the same so I assume there must be something changed in the pallet. Trace log from the PR branch: On The weight in that log corresponds to the sandbox runners default limit.. Or is the runners config wrong? To test against this branch I just change the |
Agreed. It is a regression. Didn't had time to track it down, yet. |
|
So I think I found the bug. When we error out early when creating a new frame we never refund the nested gas into the parent meter. The bug was introduced earlier but the changes here added one more case where this can happen: This is because I moved the check if the delegated to contract exists after the spawning of the nested meter. I did a bit of research: What we should do is that everything except the execution costs should be charged from the parent meter as this is what EVM does. I will add a regression test and fix this. |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
Awesome thanks for digging this up! |
…8262) This PRs adds the ability to define custom pre-compiles from outside the pallet. Before, all pre-compiles were hard coded as part of the pallet. 1. Adding a pre-compile is as easy as implementing the new `Precompile` trait on any type. It can be added to the pallet by passing it into `Config::Precompiles`. This config know excepts a tuple of multiple types that each implement `Precompile`. 2. Each pre-compile has to implement Solidity ABI. Meaning its inputs and outputs are encoded according to Eth ABI. This makes writing a pre-compile much simpler since it doesn't have to implement its own decoding logic. More importantly: It makes it trivial to consume the API from a Solidity contract. 3. We constrain the address space of pre-compiles to a safe range so that they cannot accidentally match a wide range creating a collision with real contracts. 4. We check that pre-compile address ranges do not overlap at compile time. 5. Pre-compiles behave exactly as a normal contract. They exist as frames on the call stack and the environment they observe is their own (not the one of the calling contract). They can also be delegate called which changes the semantics in the same way as for normal contracts: They observe the environment of the calling contract. 6. They can also be called by the origin without any other contract in-between. Check the rustdocs of the `precompile` module on how to write a pre-compile. 1. A new module `precompiles` is added that contains the framework to write pre-compiles. It also contains the sub module `builtin` that contains hard coded pre-compiles which exist Ethereum. 2. The `pure_precompiles` module was deleted since all its pre-compiles were ported over to the new framework and are now housed in `builtin`. 4. The `CallSetup` type is moved outside of the `benchmarking` module because it is also needed for testing code now. It is intended to be used for implementors outside of the crate to test the pre-compiles (in addition to benchmarking them). - #8363 - #8364 - #8362 Fixes [#6716](#6716) --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: PG Herveou <pgherveou@gmail.com> Co-authored-by: xermicus <cyrill@parity.io>
This brings in `stable2506` Polkadot SDK, and integrates many new features. Integrated breaking changes to be verified by the original authors: - [x] ~paritytech/polkadot-sdk#8127 @kianenigma @Ank4n~ This will come in with AHM, and not before. - [x] paritytech/polkadot-sdk#7597 @gui1117 - [x] paritytech/polkadot-sdk#8254 @bkchr - [x] paritytech/polkadot-sdk#7592 @bkontur - [x] paritytech/polkadot-sdk#8382 @UtkarshBhardwaj007 - [x] paritytech/polkadot-sdk#8021 @serban300 - [x] paritytech/polkadot-sdk#8344 @serban300 - [x] paritytech/polkadot-sdk#8262 @athei - [x] paritytech/polkadot-sdk#8584 @athei - [x] paritytech/polkadot-sdk#8299 @skunert - [x] paritytech/polkadot-sdk#8652 @pgherveou - [x] paritytech/polkadot-sdk#8554 @pgherveou - [x] paritytech/polkadot-sdk#8281 @mrshiposha - [x] paritytech/polkadot-sdk#7730 @franciscoaguirre - [x] paritytech/polkadot-sdk#8599 @yrong @claravanstaden - [x] paritytech/polkadot-sdk#8531 @bkontur - [x] paritytech/polkadot-sdk#8409 @kianenigma - [x] paritytech/polkadot-sdk#9137 @franciscoaguirre - [x] paritytech/polkadot-sdk#7944 @bkontur - [x] paritytech/polkadot-sdk#8179 @bkontur - [x] paritytech/polkadot-sdk#8037 @yrong --------- Co-authored-by: GitHub Action <action@github.com> Co-authored-by: claravanstaden <claravanstaden64@gmail.com> Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: Alain Brenzikofer <alain@integritee.network> Co-authored-by: kianenigma <kian@parity.io> Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com> Co-authored-by: ron <yrong1997@gmail.com> Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Co-authored-by: Overkillus <maciej.zyszkiewicz@parity.io>
This PRs adds the ability to define custom pre-compiles from outside the pallet. Before, all pre-compiles were hard coded as part of the pallet.
Design
Precompiletrait on any type. It can be added to the pallet by passing it intoConfig::Precompiles. This config know excepts a tuple of multiple types that each implementPrecompile.Check the rustdocs of the
precompilemodule on how to write a pre-compile.Changes
precompilesis added that contains the framework to write pre-compiles. It also contains the sub modulebuiltinthat contains hard coded pre-compiles which exist Ethereum.pure_precompilesmodule was deleted since all its pre-compiles were ported over to the new framework and are now housed inbuiltin.CallSetuptype is moved outside of thebenchmarkingmodule because it is also needed for testing code now. It is intended to be used for implementors outside of the crate to test the pre-compiles (in addition to benchmarking them).Follow Ups
CallSetupAPI #8363try_buildtests for pre-compile framework #8364Fixes #6716