feat: Call revm directly without Foundry #13

DanielSchiavini · 2024-03-18T13:03:55Z

Get rid of foundry
Use revm directly, instead
Allow using the journal manually to load and commit

DaniPopes

Looks good, just questions

src/evm.rs

pyo3 will not accept lifetime/generic parameters so a lot of boilerplate code is needed.

src/types/execution_result.rs

charles-cooper · 2024-03-20T20:25:55Z

src/executor.rs

+    let exec = evm.handler.execution();
+    // call inner handling of call/create
+    let first_frame_or_result = match ctx.evm.env.tx.transact_to {
+        TransactTo::Call(_) => exec.call(


i think it will probably be cleaner do have two different functions call and create instead of using the tx.transact_to to parametrize behavior

but, maybe it's cleaner this way. i'm not sure 🤔

I'm assuming this is mostly copied from revm, is there a reason for this? Also cc @rakita

@charles-cooper
TxEnv is from revm I don't think we can fix this here?

@DaniPopes
I am not sure where my comments went but we need to document this. I basically removed the journal finalization to keep the transaction open

https://github.com/gakonst/pyrevm/blob/618ef67d6a2a4541288ea3ac91de17a19b040736/src/executor.rs#L41
https://github.com/gakonst/pyrevm/blob/618ef67d6a2a4541288ea3ac91de17a19b040736/src/executor.rs#L105

In Evm sense, this is how call/create are specified and are differentiated by that field, so it is best to stay like that. There is one TX type that can be CALL or CREATE.

src/database.rs

DaniPopes · 2024-03-21T00:33:56Z

src/executor.rs

+    let exec = evm.handler.execution();
+    // call inner handling of call/create
+    let first_frame_or_result = match ctx.evm.env.tx.transact_to {
+        TransactTo::Call(_) => exec.call(


I'm assuming this is mostly copied from revm, is there a reason for this? Also cc @rakita

src/types/execution_result.rs

rakita

Very creative usage! Left few comments

rakita · 2024-03-22T12:30:24Z

pyrevm.pyi


 class EVM:
    def __new__(
        cls: Type["EVM"],
        env: Optional[Env] = None,
        fork_url: Optional[str] = None,
-        fork_block_number: Optional[int] = None,
+        fork_block_number: Optional[str] = None,


Currious, why is this str?

it's either a hash or a number
https://github.com/gakonst/ethers-rs/blob/5394d899adca736a602e316e6f0c06fdb5aa64b9/ethers-core/src/types/block.rs#L579
I'll add more documentation

rakita · 2024-03-22T13:02:37Z

src/database.rs

+        let provider = Provider::<Http>::try_from(fork_url).map_err(pyerr)?;
+        let block = fork_block_number.map(|n| BlockId::from_str(n)).map_or(Ok(None), |v| v.map(Some)).map_err(pyerr)?;
+        let db = EthersDB::new(Arc::new(provider), block).unwrap();
+        // todo: do we need to get the blockEnv from the client?


Something like this is maybe needed for fetch block: https://github.com/bluealloy/revm/blob/main/crates/revm/src/db/ethersdb.rs#L23

block_number is private on ethersdb, I don't think we need to retrieve it here again

rakita · 2024-03-22T13:06:00Z

src/types/checkpoint.rs

+    pub journal_i: usize,
+}
+
+impl Hash for JournalCheckpoint {


Should add Hash to the JournalCheckpoint`

rakita · 2024-03-22T13:07:57Z

src/evm.rs

        };
+        self.checkpoints.insert(checkpoint, self.context.journaled_state.checkpoint());


Checkpoint increments depth, this should be decoupled in Revm. and move depth from JournaledState to EvmContext

That would be great, but that's a whole separate task

rakita · 2024-03-22T13:08:21Z

src/evm.rs

-                evm_opts,
-            })
+        if let Some(revm_checkpoint) = self.checkpoints.remove(&checkpoint) {
+            self.context.journaled_state.checkpoint_revert(revm_checkpoint);


Depth is a problem here

How come? The depth is increased when we create the checkpoint:
https://github.com/gakonst/pyrevm/blob/5bbc43d68046969fc598dced4d10256a4abd7d24/src/evm.rs#L80

rakita · 2024-03-22T13:12:07Z

src/evm.rs

+            account.clone()
+        };
+        self.context.journaled_state.state.insert(address_, account);
+        self.context.journaled_state.touch(&address_);


Set balance and touch are different operations. Balance change will not get reverted but it needs to be touched so change is noticed.

I'm not sure how to change the code based on your remark 🤔

rakita · 2024-03-22T13:12:50Z

src/evm.rs

-    fn get_balance(_self: PyRef<'_, Self>, address: &str) -> PyResult<U256> {
-        let balance = _self.0.get_balance(addr(address)?).map_err(pyerr)?;
+    fn get_balance(&mut self, address: &str) -> PyResult<U256> {
+        let (balance, _) = self.context.balance(addr(address)?).map_err(pyerr)?;


Are we okay to Error on account that are inside Evm?

Why is this going to error? The context.balance function seems to look first in the journaled state and then the db 🤔

rakita · 2024-03-22T13:17:02Z

src/executor.rs

+    let exec = evm.handler.execution();
+    // call inner handling of call/create
+    let first_frame_or_result = match ctx.evm.env.tx.transact_to {
+        TransactTo::Call(_) => exec.call(


In Evm sense, this is how call/create are specified and are differentiated by that field, so it is best to stay like that. There is one TX type that can be CALL or CREATE.

rakita · 2024-03-22T13:19:25Z

src/executor.rs

+}
+
+/// Calls the given evm. This is originally a copy of revm::Evm::transact, but it calls our own output function
+fn run_evm<EXT>(mut evm: Evm<'_, EXT, DB>, is_static: bool) -> PyResult<(ExecutionResult, EvmContext<DB>)> {


Didn't think that people were going to use handlers in this creative way! lol

lol, creative often means there's a simpler way to do it? It's my first rust PR so please bear with me 😆

rakita · 2024-03-22T13:23:45Z

src/executor.rs

+
+/// Returns the output of the transaction.
+/// This is mostly copied from revm::handler::mainnet::post_execution::output
+/// However, we removed the journal finalization to keep the transaction open.


This can be good feature set for Revm to have journaling open across transactions. Would need additional work inside JournalState to support it.

There are few unwraps() that could panic if revert is done on unknown State: https://github.com/bluealloy/revm/blob/main/crates/revm/src/journaled_state.rs

This solution seems to be working in our tests. Or are there other issues I'm overseeing?

DaniPopes

LGTM

I pushed a few changes to enable more checks in CI and fixing them

gakonst

Supportive of the idea - in general the Foundry forking backend is very powerful because it dedups requests when forking in parallel across many evm instances that reuse it.

gakonst · 2024-04-03T11:58:27Z

pytest/test.py

-        address, AccountInfo(code=load_contract_bin("full_math.bin"))
-    )
+def test_balances_fork():
+    evm = EVM(fork_url=fork_url, fork_block_number="0x3b01f793ed1923cd82df5fe345b3e12211aedd514c8546e69efd6386dc0c9a97")


nit rename param to fork_block since you're passing a hash

gakonst · 2024-04-03T13:02:59Z

src/database.rs

+type ForkDB = CacheDB<EthersDB<Provider<Http>>>;
+
+/// A wrapper around the `CacheDB` and `EthersDB` to provide a common interface
+/// without needing dynamic lifetime and generic parameters (unsupported in PyO3)


Ah didn't realize- makes sense.

gakonst · 2024-04-03T13:03:33Z

src/database.rs

+use std::sync::Arc;
+
+type MemDB = CacheDB<EmptyDBWrapper>;
+type ForkDB = CacheDB<EthersDB<Provider<Http>>>;


Follow up move to Alloy

what do you mean exactly?

gakonst · 2024-04-03T13:04:22Z

src/empty_db_wrapper.rs

+/// An empty database that always returns default values when queried.
+/// This will also _always_ return `Some(AccountInfo)`.
+///
+/// Copied from Foundry: <https://github.com/foundry-rs/foundry/blob/9e3ab9b3aff21c6e5ef/crates/evm/core/src/backend/in_memory_db.rs#L83-L92>
+#[derive(Clone, Debug, Default)]
+pub(crate) struct EmptyDBWrapper(EmptyDB);


Wonder if this is debt but yeah we can do on follow up

gakonst · 2024-04-03T13:04:59Z

src/evm.rs

+    }
+
+    fn snapshot(&mut self) -> PyResult<JournalCheckpoint> {
+        let checkpoint = JournalCheckpoint {


Defer to Dragan for using this part of the api

gakonst · 2024-04-03T13:05:32Z

src/executor.rs

+        let evm = Evm::builder()
+            .with_context_with_handler_cfg(ContextWithHandlerCfg {
+                cfg: handler_cfg,
+                context: Context {
+                    evm: evm_context,
+                    external: tracer,
+                },
+            })
+            .append_handler_register(inspector_handle_register)
+            .build();
+        run_evm(evm, is_static)


charles-cooper

hope we can get some more feedback from @rakita later but merging so we can cut a release

also ran `cargo update`

DanielSchiavini added 6 commits March 15, 2024 17:12

Try to call revm directly

1139793

Use load_account instead of basic

8877746

Allow retrieving a dict of account info

02e6f49

Copy EmptyDBWrapper from Foundry

33b94e5

Implement forking

28766d7

Cleanup

64e9459

DanielSchiavini marked this pull request as ready for review March 18, 2024 13:11

DaniPopes reviewed Mar 18, 2024

View reviewed changes

src/evm.rs Outdated Show resolved Hide resolved

DanielSchiavini marked this pull request as draft March 18, 2024 15:51

DanielSchiavini added 2 commits March 19, 2024 10:12

Extract database implementation into separate class

1a8d390

pyo3 will not accept lifetime/generic parameters so a lot of boilerplate code is needed.

Add tracing

7454bb0

DanielSchiavini force-pushed the revm branch from 8e4bd16 to 7454bb0 Compare March 19, 2024 13:42

DanielSchiavini added 8 commits March 19, 2024 17:58

Add comments

b05b48d

Save the full context and execution result

2085ed5

Clone the DB as checkpoint

ef712be

Inline transact method

29e0d3f

Inline output method

11ff79a

Omit commit after execution

890ef86

Refactor

50c4581

Rename and move

cfd0e46

charles-cooper reviewed Mar 20, 2024

View reviewed changes

src/types/execution_result.rs Outdated Show resolved Hide resolved

charles-cooper reviewed Mar 20, 2024

View reviewed changes

DaniPopes reviewed Mar 21, 2024

View reviewed changes

DanielSchiavini added 3 commits March 21, 2024 12:15

Expose actual PyBytes + check get_code return

84c1743

Fix tests

618ef67

Review comment

0dce9d8

DanielSchiavini marked this pull request as ready for review March 21, 2024 15:59

DanielSchiavini added 3 commits March 22, 2024 09:38

Documentation

f6aa771

Add create2, gas and is_static support

7ace79a

Fix stub

5bbc43d

rakita reviewed Mar 22, 2024

View reviewed changes

DanielSchiavini and others added 9 commits March 25, 2024 09:36

Expose logs

e77c776

Review comments

a93fa47

Duplicate implementation

28fe7df

Support gas price

68db546

Fix and set block_env

87cc451

chore: clippy

72b6130

chore: run more stuff in CI, format code

f6e2163

chore: less allocs

b379fea

chore: sort imports

a1e8052

DaniPopes approved these changes Mar 28, 2024

View reviewed changes

DanielSchiavini mentioned this pull request Apr 3, 2024

refactor[test]: bypass eth-tester and interface with evm backend directly vyperlang/vyper#3846

Merged

gakonst approved these changes Apr 3, 2024

View reviewed changes

DanielSchiavini added 2 commits April 4, 2024 11:30

Rename to fork_block

50b7fae

Add reset_transient_storage

525ec28

charles-cooper approved these changes Apr 14, 2024

View reviewed changes

Merge branch 'master' into revm

daee94f

also ran `cargo update`

charles-cooper merged commit 3ffe1f9 into paradigmxyz:master Apr 14, 2024
9 of 13 checks passed

DanielSchiavini deleted the revm branch April 15, 2024 07:31

		};
		self.checkpoints.insert(checkpoint, self.context.journaled_state.checkpoint());

feat: Call revm directly without Foundry #13

feat: Call revm directly without Foundry #13

Conversation

DanielSchiavini commented Mar 18, 2024 • edited Loading

DaniPopes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniPopes left a comment

Choose a reason for hiding this comment

gakonst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles-cooper left a comment

Choose a reason for hiding this comment

DanielSchiavini commented Mar 18, 2024 •

edited

Loading