From a0eef1f661be46a3ec3927ae2e0f4624d5b036f5 Mon Sep 17 00:00:00 2001 From: grandizzy Date: Mon, 12 Feb 2024 16:32:19 +0200 Subject: [PATCH 1/5] - if no block provided then fork is created using latest block for fork id (url@latest_block). if block provided then fork id is url@provided_block (this means there'll no longer be a url@latest fork id but always url@block) - after creation of fork the id is checked if in inserted forks. If it doesn't exist then it's inserted, otherwise the existing backend is reused and a new fork url@latest_block-1 recorded CreatedFork::inc_senders increments number of senders and returns the new unique fork id. CreatedFork::num_senders was removed MultiForkHandler::insert_new_fork added to handle fork insertion / send on channels --- crates/evm/core/src/fork/multi.rs | 100 ++++++++++++++++-------------- 1 file changed, 53 insertions(+), 47 deletions(-) diff --git a/crates/evm/core/src/fork/multi.rs b/crates/evm/core/src/fork/multi.rs index 483686980833..b86b78fa14ec 100644 --- a/crates/evm/core/src/fork/multi.rs +++ b/crates/evm/core/src/fork/multi.rs @@ -171,7 +171,8 @@ impl MultiFork { type Handler = BackendHandler>>; -type CreateFuture = Pin> + Send>>; +type CreateFuture = + Pin> + Send>>; type CreateSender = OneshotSender>; type GetEnvSender = OneshotSender>; @@ -258,26 +259,35 @@ impl MultiForkHandler { let fork_id = ForkId::new(&fork.url, fork.evm_opts.fork_block_number); trace!(?fork_id, "created new forkId"); - if let Some(fork) = self.forks.get(&fork_id).cloned() { - // assign a new unique fork id but reuse the existing backend - let unique_fork_id: ForkId = - format!("{}-{}", fork_id.as_str(), fork.num_senders()).into(); - trace!(?fork_id, ?unique_fork_id, "created new unique forkId"); - fork.inc_senders(); - let backend = fork.backend.clone(); - let env = fork.opts.env.clone(); - self.forks.insert(unique_fork_id.clone(), fork); - let _ = sender.send(Ok((unique_fork_id, backend, env))); - } else { - // there could already be a task for the requested fork in progress - if let Some(in_progress) = self.find_in_progress_task(&fork_id) { - in_progress.push(sender); - return; - } + if let Some(in_progress) = self.find_in_progress_task(&fork_id) { + in_progress.push(sender); + return; + } - // need to create a new fork - let task = Box::pin(create_fork(fork)); - self.pending_tasks.push(ForkTask::Create(task, fork_id, sender, Vec::new())); + // need to create a new fork + let task = Box::pin(create_fork(fork)); + self.pending_tasks.push(ForkTask::Create(task, fork_id, sender, Vec::new())); + } + + fn insert_new_fork( + &mut self, + fork_id: ForkId, + fork: CreatedFork, + sender: CreateSender, + additional_senders: Vec, + ) { + self.forks.insert(fork_id.clone(), fork.clone()); + let _ = sender.send(Ok((fork_id.clone(), fork.backend.clone(), fork.opts.env.clone()))); + + // notify all additional senders and track unique forkIds + for sender in additional_senders { + let next_fork_id = fork.inc_senders(fork_id.clone()); + self.forks.insert(next_fork_id.clone(), fork.clone()); + let _ = sender.send(Ok(( + next_fork_id.clone(), + fork.backend.clone(), + fork.opts.env.clone(), + ))); } } @@ -346,25 +356,17 @@ impl Future for MultiForkHandler { ForkTask::Create(mut fut, id, sender, additional_senders) => { if let Poll::Ready(resp) = fut.poll_unpin(cx) { match resp { - Ok((fork, handler)) => { - pin.handlers.push((id.clone(), handler)); - let backend = fork.backend.clone(); - let env = fork.opts.env.clone(); - pin.forks.insert(id.clone(), fork.clone()); - - let _ = sender.send(Ok((id.clone(), backend.clone(), env.clone()))); - - // also notify all additional senders and track unique forkIds - for sender in additional_senders { - fork.inc_senders(); - let unique_fork_id: ForkId = - format!("{}-{}", id.as_str(), fork.num_senders()).into(); - pin.forks.insert(unique_fork_id.clone(), fork.clone()); - let _ = sender.send(Ok(( - unique_fork_id, - backend.clone(), - env.clone(), - ))); + Ok((fork_id, fork, handler)) => { + if let Some(fork) = pin.forks.get(&fork_id).cloned() { + pin.insert_new_fork( + fork.inc_senders(fork_id.clone()), + fork, + sender, + additional_senders, + ); + } else { + pin.handlers.push((fork_id.clone(), handler)); + pin.insert_new_fork(fork_id, fork, sender, additional_senders); } } Err(err) => { @@ -446,12 +448,14 @@ impl CreatedFork { Self { opts, backend, num_senders: Arc::new(AtomicUsize::new(1)) } } - fn inc_senders(&self) { - self.num_senders.fetch_add(1, std::sync::atomic::Ordering::Relaxed); - } - - fn num_senders(&self) -> usize { - self.num_senders.load(std::sync::atomic::Ordering::Relaxed) + /// Increment senders and return unique identifier of the fork + fn inc_senders(&self, fork_id: ForkId) -> ForkId { + format!( + "{}-{}", + fork_id.as_str(), + self.num_senders.fetch_add(1, std::sync::atomic::Ordering::Relaxed) + ) + .into() } } @@ -483,7 +487,7 @@ impl Drop for ShutDownMultiFork { /// Creates a new fork /// /// This will establish a new `Provider` to the endpoint and return the Fork Backend -async fn create_fork(mut fork: CreateFork) -> eyre::Result<(CreatedFork, Handler)> { +async fn create_fork(mut fork: CreateFork) -> eyre::Result<(ForkId, CreatedFork, Handler)> { let provider = Arc::new( ProviderBuilder::new(fork.url.as_str()) .maybe_max_retry(fork.evm_opts.fork_retries) @@ -511,5 +515,7 @@ async fn create_fork(mut fork: CreateFork) -> eyre::Result<(CreatedFork, Handler let db = BlockchainDb::new(meta, cache_path); let (backend, handler) = SharedBackend::new(provider, db, Some(number.to::().into())); let fork = CreatedFork::new(fork, backend); - Ok((fork, handler)) + let fork_id = ForkId::new(&fork.opts.url, number.to::().into()); + + Ok((fork_id, fork, handler)) } From 1429a6ef022daeef65963d9c6925b96b01323b1b Mon Sep 17 00:00:00 2001 From: grandizzy Date: Mon, 12 Feb 2024 18:44:59 +0200 Subject: [PATCH 2/5] Dummy test --- crates/forge/tests/it/repros.rs | 3 +++ testdata/repros/Issue6616.t.sol | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 testdata/repros/Issue6616.t.sol diff --git a/crates/forge/tests/it/repros.rs b/crates/forge/tests/it/repros.rs index 9c743a12c325..0838ffed019f 100644 --- a/crates/forge/tests/it/repros.rs +++ b/crates/forge/tests/it/repros.rs @@ -300,3 +300,6 @@ test_repro!(6759); // https://github.com/foundry-rs/foundry/issues/6966 test_repro!(6966); + +// https://github.com/foundry-rs/foundry/issues/6616 +test_repro!(6616); diff --git a/testdata/repros/Issue6616.t.sol b/testdata/repros/Issue6616.t.sol new file mode 100644 index 000000000000..2986257feef4 --- /dev/null +++ b/testdata/repros/Issue6616.t.sol @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +pragma solidity 0.8.18; + +import "ds-test/test.sol"; +import "../cheats/Vm.sol"; + +// https://github.com/foundry-rs/foundry/issues/6616 +contract Issue6616Test is DSTest { + Vm constant vm = Vm(HEVM_ADDRESS); + + function testCreateForkRollLatestBlock() public { + vm.createSelectFork("rpcAlias"); + uint256 startBlock = block.number; + for (uint256 i; i < 10; i++) { + vm.sleep(5000); + vm.createSelectFork("rpcAlias"); + if (block.number > startBlock) break; + } + assertGt(block.number, startBlock); + } +} From eafa9e5a61650e3b0dc8896f6305262049ce0472 Mon Sep 17 00:00:00 2001 From: grandizzy Date: Tue, 13 Feb 2024 08:30:38 +0200 Subject: [PATCH 3/5] Add back comment, minor code style --- crates/evm/core/src/fork/multi.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/evm/core/src/fork/multi.rs b/crates/evm/core/src/fork/multi.rs index b86b78fa14ec..3e709d7457a0 100644 --- a/crates/evm/core/src/fork/multi.rs +++ b/crates/evm/core/src/fork/multi.rs @@ -259,6 +259,7 @@ impl MultiForkHandler { let fork_id = ForkId::new(&fork.url, fork.evm_opts.fork_block_number); trace!(?fork_id, "created new forkId"); + // there could already be a task for the requested fork in progress if let Some(in_progress) = self.find_in_progress_task(&fork_id) { in_progress.push(sender); return; @@ -503,19 +504,19 @@ async fn create_fork(mut fork: CreateFork) -> eyre::Result<(ForkId, CreatedFork, // we need to use the block number from the block because the env's number can be different on // some L2s (e.g. Arbitrum). - let number = block.header.number.unwrap_or(meta.block_env.number); + let number = block.header.number.unwrap_or(meta.block_env.number).to::(); // determine the cache path if caching is enabled let cache_path = if fork.enable_caching { - Config::foundry_block_cache_dir(meta.cfg_env.chain_id, number.to::()) + Config::foundry_block_cache_dir(meta.cfg_env.chain_id, number) } else { None }; let db = BlockchainDb::new(meta, cache_path); - let (backend, handler) = SharedBackend::new(provider, db, Some(number.to::().into())); + let (backend, handler) = SharedBackend::new(provider, db, Some(number.into())); let fork = CreatedFork::new(fork, backend); - let fork_id = ForkId::new(&fork.opts.url, number.to::().into()); + let fork_id = ForkId::new(&fork.opts.url, number.into()); Ok((fork_id, fork, handler)) } From a41554560ea94d71336b509ea53ac68e110f1431 Mon Sep 17 00:00:00 2001 From: grandizzy Date: Tue, 13 Feb 2024 09:48:06 +0200 Subject: [PATCH 4/5] Consume fork ids in insert_new_fork --- crates/evm/core/src/fork/multi.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/crates/evm/core/src/fork/multi.rs b/crates/evm/core/src/fork/multi.rs index 3e709d7457a0..deb0e9f55161 100644 --- a/crates/evm/core/src/fork/multi.rs +++ b/crates/evm/core/src/fork/multi.rs @@ -284,11 +284,7 @@ impl MultiForkHandler { for sender in additional_senders { let next_fork_id = fork.inc_senders(fork_id.clone()); self.forks.insert(next_fork_id.clone(), fork.clone()); - let _ = sender.send(Ok(( - next_fork_id.clone(), - fork.backend.clone(), - fork.opts.env.clone(), - ))); + let _ = sender.send(Ok((next_fork_id, fork.backend.clone(), fork.opts.env.clone()))); } } @@ -360,7 +356,7 @@ impl Future for MultiForkHandler { Ok((fork_id, fork, handler)) => { if let Some(fork) = pin.forks.get(&fork_id).cloned() { pin.insert_new_fork( - fork.inc_senders(fork_id.clone()), + fork.inc_senders(fork_id), fork, sender, additional_senders, From 28e28228013e69ac49ecb8a61336f80ecf79c1cf Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Mon, 19 Feb 2024 13:17:37 +0100 Subject: [PATCH 5/5] add comment --- testdata/repros/Issue6616.t.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/testdata/repros/Issue6616.t.sol b/testdata/repros/Issue6616.t.sol index 2986257feef4..6c9991f0e79b 100644 --- a/testdata/repros/Issue6616.t.sol +++ b/testdata/repros/Issue6616.t.sol @@ -11,6 +11,7 @@ contract Issue6616Test is DSTest { function testCreateForkRollLatestBlock() public { vm.createSelectFork("rpcAlias"); uint256 startBlock = block.number; + // this will create new forks and exit once a new latest block is found for (uint256 i; i < 10; i++) { vm.sleep(5000); vm.createSelectFork("rpcAlias");