feat(evm-adapters): transport agnostic forking #1003

meetmangukiya · 2022-03-21T13:10:20Z

Motivation

Run tests on forked mainnet will probably gain some speed when using WebSocket / IPC which will prevent opening and closing connections for each RPC requests(account data, storage slots).

P.S.: Have not tested IPC

Solution

mattsse · 2022-03-21T13:19:44Z

thanks, good point!
since we're transitioning to revm I wonder whether it's even worth it to add this for sputnik or only add this to the revm branch

meetmangukiya · 2022-03-21T14:14:37Z

evm-adapters/src/sputnik/forked_backend/cache.rs

@@ -378,14 +379,16 @@ impl SharedBackend {
        cache: SharedCache<MemCache>,
        vicinity: MemoryVicinity,
        pin_block: Option<BlockId>,
+        runtime: Option<Runtime>,


Had to pass runtime along because WebSocket connection would be closed when the runtime that it was created on drops.

btw, i don't think RuntimeOrHandle was good idea here (i'm to blame) - it would be better to have clean async constructor with native spawn & wrap it in sync version.

meetmangukiya · 2022-03-21T14:26:00Z

since we're transitioning to revm I wonder whether it's even worth it to add this for sputnik or only add this to the revm branch

Not sure, do we plan to keep and support the sputnik evm alongside revm? I was thinking that's the plan since we have --evm-type flag

gakonst · 2022-03-21T15:05:10Z

Supportive of the PR in principle, we're getting rid of other evm types so could you give a stab maybe at integrating this against https://github.com/gakonst/foundry/tree/revm?

onbjerg · 2022-03-22T04:38:58Z

since we're transitioning to revm I wonder whether it's even worth it to add this for sputnik or only add this to the revm branch

Not sure, do we plan to keep and support the sputnik evm alongside revm? I was thinking that's the plan since we have --evm-type flag

No sir, we will not keep Sputnik. The multi-EVM thing was an early design thought but we're moving away from that. The relevant files in the revm branch is in evm/src/executor/fork and it should be pretty similar to the one in evm-adapters, so porting should not prove too difficult. Let me know if you have questions 😄

onbjerg

lgtm cc @mattsse to double check

mattsse

thanks a lot,
couple of nits

mattsse · 2022-03-29T23:17:09Z

evm/Cargo.toml

@@ -11,7 +11,7 @@ foundry-utils = { path = "./../utils" }
 serde_json = "1.0.67"
 serde = "1.0.130"
 hex = "0.4.3"
-ethers = { git = "https://github.com/gakonst/ethers-rs", default-features = false, features = ["solc-full"] }
+ethers = { git = "https://github.com/gakonst/ethers-rs", default-features = false, features = ["solc-full", "ws", "rustls", "ipc"] }


ipc is only available on unix.
~~and enabling this will definitely not compile on windows~~
~~so we need to feature gate this here as well~~
perhaps fine as ipc module should be ignored even if enabled on non linux
it should be ignored:

https://github.com/gakonst/ethers-rs/blob/367f3444ec627c6afa3bd9613644a6f903b3cb86/ethers-providers/src/transports/mod.rs#L22-L25

mattsse · 2022-03-29T23:20:33Z

evm/src/executor/builder.rs

+            let provider = Arc::new(
+                rt.block_on(Provider::connect_ipc(url)).expect("Failed to establish provider"),
+            );


needs a cfg for unix, otherwise should panic

mattsse · 2022-03-29T23:23:28Z

evm/src/executor/fork/backend.rs

+        provider: M,
+        db: BlockchainDb,
+        pin_block: Option<BlockId>,
+        runtime: Option<Runtime>,


hmm not a big fan of supplying the runtime here.
I'd consider creating a new runtime negligible additional effort

Yes, however, I am establishing the IPC and WebSocket connections. You cannot use those in different runtimes or after the runtime that it was created on ceases to exist.

imho this function should be async

mattsse · 2022-03-29T23:27:39Z

evm/src/executor/opts.rs

+                match rt {
+                    RuntimeOrHandle::Runtime(runtime) => runtime.block_on(fut),
+                    RuntimeOrHandle::Handle(handle) => handle.block_on(fut),
+                }
+                .expect("could not instantiate forked environment")


this getting a bit messy.
we have already a block_on in cli/utils I believe.

let's either replicate that for this crate or better make this function async so we only have to do it once for the whole evm_env function.

mattsse · 2022-03-29T23:27:52Z

evm/src/executor/opts.rs

+                let fut = async {
+                    let provider = Provider::connect_ipc(fork_url.as_str())
+                        .await
+                        .expect("could not instantiated provider");
+                    environment(&provider, self.env.chain_id, self.fork_block_number, self.sender)
+                        .await
+                };


also needs cfg

gakonst · 2022-03-29T23:32:28Z

Feels like ethers provider is getting to the point where we should do an enum over the transports and abstract it HTTP/WS/IPC away from the user..

I just tested this and weirdly enough when I run it with WSS twice caching works, but when I switch URL to be back to the https it reruns w/o caching. Didn't expect that, sounds like a bug?

Separately, here's a WS vs HTTPS comparison on performance on same Alchemy API key without cache:

WSS Time:

[PASS] testShutdownCost() (gas: 17127724)
Test result: ok. 1 passed; 0 failed; finished in 793.41s

HTTPS Time:

Running 1 test for src/Convex.t.sol:ConvexTest
[PASS] testShutdownCost() (gas: 17127724)
Test result: ok. 1 passed; 0 failed; finished in 348.97s

Seems like WS is slower?

mattsse · 2022-03-29T23:39:58Z

I just tested this and weirdly enough when I run it with WSS twice caching works, but when I switch URL to be back to the https it reruns w/o caching. Didn't expect that, sounds like a bug?

same host?
we're storing the host (Url::host) in the cache file and if they don't match we treat it as invalid and discard the cache.
perhaps we should to overthink that, and perhaps ignore that?

gakonst · 2022-03-29T23:41:35Z

Aha! It's https://eth-mainnet.alchemyapi.io/v2/<...> vs wss://eth-mainnet.ws.alchemyapi.io/v2/<..>. Maybe we ignore the full URL?

mattsse · 2022-03-29T23:50:00Z

yeah the host differs, there's a .ws. in the wss host.

let's ignore the endpoint in comparison but we can keep them as Vec<Url> instead?

onbjerg · 2022-03-30T13:42:20Z

Sorry to pile on here, but one last stretch goal (feel free to skip it!) would be to change the ETH_URL we use in our test workflow to use the websocket endpoint instead of HTTP 😄

meetmangukiya · 2022-03-31T03:11:10Z

I will try to look into why ws is so much slower. I suspect there might be something off in how we are handling the requests in SharedBackend maybe. Putting the PR on draft for now since I expect not to be able to look into this soon.

gakonst · 2022-04-18T16:41:11Z

@meetmangukiya just trying to keep things clean, any plans to resume this PR? else can I close as stale, and we re-visit later?

meetmangukiya · 2022-04-18T18:17:54Z

Can close for now. Might pick again when frustrated with lack of ws support. I actually couldn't think any rational reason why websocket is 2x slower. Thought might have to do something with the websocket server implementation in ethers or with the backend implementation in foundry. However, don't see anything obvious there that could lead to such unexpected results.

Are we open to merge ws and ipc provider support even though they may be slower or have no perfomance gains? If yes, i will address the review comments and can get this merged.

gakonst · 2022-04-18T18:23:17Z

I would say let's only merge if we figure out the performance overheads. It's possible it's in the ethers websocket impl, I have never tried benchmarking it.

meetmangukiya · 2022-04-18T18:33:33Z

I had tested ethersjs and ethers-rs websocket. There weren't significant differences in response times. But it was just simple looping of eth_gasPrice and logging response times so not the best way to benchmark.

sambacha · 2022-09-08T05:10:46Z

I had tested ethersjs and ethers-rs websocket. There weren't significant differences in response times. But it was just simple looping of eth_gasPrice and logging response times so not the best way to benchmark.

ethersjs just recently (as in after your comment) properly implemented WS FYI

meetmangukiya · 2022-09-08T05:43:21Z

@sambacha got a link to the PR?

sambacha · 2022-09-08T05:45:29Z

@sambacha got a link to the PR?

Here is the relevant issue which has a long thread about websockets and the various PRs ethers-io/ethers.js#1053

meetmangukiya marked this pull request as draft March 21, 2022 13:10

meetmangukiya force-pushed the transport-agnostic-forking branch from 5caa009 to c40bb87 Compare March 21, 2022 14:12

meetmangukiya commented Mar 21, 2022

View reviewed changes

meetmangukiya marked this pull request as ready for review March 21, 2022 14:26

onbjerg added the T-feature Type: feature label Mar 22, 2022

meetmangukiya marked this pull request as draft March 23, 2022 20:09

feat(evm-adapters): transport agnostic forking

cecd9dd

meetmangukiya force-pushed the transport-agnostic-forking branch from c40bb87 to cecd9dd Compare March 29, 2022 06:48

meetmangukiya marked this pull request as ready for review March 29, 2022 06:58

onbjerg approved these changes Mar 29, 2022

View reviewed changes

mattsse requested changes Mar 29, 2022

View reviewed changes

mattsse mentioned this pull request Mar 29, 2022

RPC storage cache should still be considered valid if the rpc endpoint changed #1129

Closed

onbjerg mentioned this pull request Mar 30, 2022

docs: specify that the RPC endpoint is HTTP/S mds1/convex-shutdown-simulation#17

Closed

meetmangukiya marked this pull request as draft March 31, 2022 03:11

This was referenced Apr 4, 2022

fix(evm): use forked sharedbackend correctly #1033

Merged

fix(evm): backend shutdown sequence #1212

Merged

gakonst closed this Apr 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(evm-adapters): transport agnostic forking #1003

feat(evm-adapters): transport agnostic forking #1003

meetmangukiya commented Mar 21, 2022 •

edited

Loading

mattsse commented Mar 21, 2022

meetmangukiya Mar 21, 2022

univerz Mar 21, 2022

meetmangukiya commented Mar 21, 2022

gakonst commented Mar 21, 2022

onbjerg commented Mar 22, 2022

onbjerg left a comment

mattsse left a comment

mattsse Mar 29, 2022

mattsse Mar 29, 2022

mattsse Mar 29, 2022

meetmangukiya Mar 30, 2022

univerz Mar 30, 2022

mattsse Mar 29, 2022

mattsse Mar 29, 2022

gakonst commented Mar 29, 2022 •

edited

Loading

mattsse commented Mar 29, 2022

gakonst commented Mar 29, 2022 •

edited

Loading

mattsse commented Mar 29, 2022

onbjerg commented Mar 30, 2022

meetmangukiya commented Mar 31, 2022

gakonst commented Apr 18, 2022

meetmangukiya commented Apr 18, 2022 •

edited

Loading

gakonst commented Apr 18, 2022

meetmangukiya commented Apr 18, 2022

sambacha commented Sep 8, 2022

meetmangukiya commented Sep 8, 2022

sambacha commented Sep 8, 2022

feat(evm-adapters): transport agnostic forking #1003

feat(evm-adapters): transport agnostic forking #1003

Conversation

meetmangukiya commented Mar 21, 2022 • edited Loading

Motivation

Solution

mattsse commented Mar 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meetmangukiya commented Mar 21, 2022

gakonst commented Mar 21, 2022

onbjerg commented Mar 22, 2022

onbjerg left a comment

Choose a reason for hiding this comment

mattsse 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

gakonst commented Mar 29, 2022 • edited Loading

mattsse commented Mar 29, 2022

gakonst commented Mar 29, 2022 • edited Loading

mattsse commented Mar 29, 2022

onbjerg commented Mar 30, 2022

meetmangukiya commented Mar 31, 2022

gakonst commented Apr 18, 2022

meetmangukiya commented Apr 18, 2022 • edited Loading

gakonst commented Apr 18, 2022

meetmangukiya commented Apr 18, 2022

sambacha commented Sep 8, 2022

meetmangukiya commented Sep 8, 2022

sambacha commented Sep 8, 2022

meetmangukiya commented Mar 21, 2022 •

edited

Loading

gakonst commented Mar 29, 2022 •

edited

Loading

gakonst commented Mar 29, 2022 •

edited

Loading

meetmangukiya commented Apr 18, 2022 •

edited

Loading