From 47cbb6f5134ce663753f89be5e6fd9892b0d366a Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Thu, 11 Jan 2024 19:03:56 +0100 Subject: [PATCH 1/2] fix: make ForkIds unique --- crates/evm/core/src/backend/mod.rs | 23 ++--------- crates/evm/core/src/fork/multi.rs | 61 +++++++++++++++++++++++------- crates/forge/tests/it/repros.rs | 3 ++ testdata/repros/Issue6759.t.sol | 19 ++++++++++ 4 files changed, 73 insertions(+), 33 deletions(-) create mode 100644 testdata/repros/Issue6759.t.sol diff --git a/crates/evm/core/src/backend/mod.rs b/crates/evm/core/src/backend/mod.rs index f2f1a355d6f5..771ba7b3d57c 100644 --- a/crates/evm/core/src/backend/mod.rs +++ b/crates/evm/core/src/backend/mod.rs @@ -993,20 +993,9 @@ impl DatabaseExt for Backend { self.inner.snapshots.clear() } - fn create_fork(&mut self, mut create_fork: CreateFork) -> eyre::Result { + fn create_fork(&mut self, create_fork: CreateFork) -> eyre::Result { trace!("create fork"); - let (fork_id, fork, _) = self.forks.create_fork(create_fork.clone())?; - - // Check for an edge case where the fork_id already exists, which would mess with the - // internal mappings. This can happen when two forks are created with the same - // endpoint and block number - // This is a hacky solution but a simple fix to ensure URLs are unique - if self.inner.contains_fork(&fork_id) { - // ensure URL is unique - create_fork.url.push('/'); - debug!(?fork_id, "fork id already exists. making unique"); - return self.create_fork(create_fork) - } + let (fork_id, fork, _) = self.forks.create_fork(create_fork)?; let fork_db = ForkDB::new(fork); let (id, _) = @@ -1134,7 +1123,8 @@ impl DatabaseExt for Backend { Ok(()) } - /// This is effectively the same as [`Self::create_select_fork()`] but updating an existing fork + /// This is effectively the same as [`Self::create_select_fork()`] but updating an existing + /// [ForkId] that is mapped to the [LocalForkId] fn roll_fork( &mut self, id: Option, @@ -1576,11 +1566,6 @@ pub struct BackendInner { // === impl BackendInner === impl BackendInner { - /// Returns `true` if the given [ForkId] already exists. - fn contains_fork(&self, id: &ForkId) -> bool { - self.created_forks.contains_key(id) - } - pub fn ensure_fork_id(&self, id: LocalForkId) -> eyre::Result<&ForkId> { self.issued_local_fork_ids .get(&id) diff --git a/crates/evm/core/src/fork/multi.rs b/crates/evm/core/src/fork/multi.rs index d09466871fdb..8e2c2e902381 100644 --- a/crates/evm/core/src/fork/multi.rs +++ b/crates/evm/core/src/fork/multi.rs @@ -22,17 +22,25 @@ use std::{ fmt, pin::Pin, sync::{ + atomic::AtomicUsize, mpsc::{channel as oneshot_channel, Sender as OneshotSender}, Arc, }, time::Duration, }; -/// The identifier for a specific fork, this could be the name of the network a custom descriptive -/// name. +/// The _unique_ identifier for a specific fork, this could be the name of the network a custom +/// descriptive name. #[derive(Clone, Debug, PartialEq, Eq, Hash)] pub struct ForkId(pub String); +impl ForkId { + /// Returns the identifier of the fork + pub fn as_str(&self) -> &str { + &self.0 + } +} + impl fmt::Display for ForkId { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { self.0.fmt(f) @@ -165,7 +173,7 @@ enum Request { CreateFork(Box, CreateSender), /// Returns the Fork backend for the `ForkId` if it exists GetFork(ForkId, OneshotSender>), - /// Adjusts the block that's being forked + /// Adjusts the block that's being forked, by creating a new fork at the new block RollFork(ForkId, u64, CreateSender), /// Returns the environment of the fork GetEnv(ForkId, GetEnvSender), @@ -194,7 +202,10 @@ pub struct MultiForkHandler { // tasks currently in progress pending_tasks: Vec, - /// All created Forks in order to reuse them + /// All _unique_ forkids mapped to their corresponding backend. + /// + /// Note: The backend can be shared by multiple ForkIds if the target the same provider and + /// block number. forks: HashMap, /// Optional periodic interval to flush rpc cache @@ -238,9 +249,16 @@ impl MultiForkHandler { let fork_id = create_fork_id(&fork.url, fork.evm_opts.fork_block_number); trace!(?fork_id, "created new forkId"); - if let Some(fork) = self.forks.get_mut(&fork_id) { - fork.num_senders += 1; - let _ = sender.send(Ok((fork_id, fork.backend.clone(), fork.opts.env.clone()))); + 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((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) { @@ -323,14 +341,21 @@ impl Future for MultiForkHandler { pin.handlers.push((id.clone(), handler)); let backend = fork.backend.clone(); let env = fork.opts.env.clone(); - pin.forks.insert(id.clone(), fork); + pin.forks.insert(id.clone(), fork.clone()); let _ = sender.send(Ok((id.clone(), backend.clone(), env.clone()))); - // also notify all additional senders + // also notify all additional senders and track unique forkIds for sender in additional_senders { - let _ = - sender.send(Ok((id.clone(), backend.clone(), env.clone()))); + 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(), + ))); } } Err(err) => { @@ -394,7 +419,7 @@ impl Future for MultiForkHandler { } /// Tracks the created Fork -#[derive(Debug)] +#[derive(Debug, Clone)] struct CreatedFork { /// How the fork was initially created opts: CreateFork, @@ -402,14 +427,22 @@ struct CreatedFork { backend: SharedBackend, /// How many consumers there are, since a `SharedBacked` can be used by multiple /// consumers - num_senders: usize, + num_senders: Arc, } // === impl CreatedFork === impl CreatedFork { pub fn new(opts: CreateFork, backend: SharedBackend) -> Self { - Self { opts, backend, num_senders: 1 } + 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) } } diff --git a/crates/forge/tests/it/repros.rs b/crates/forge/tests/it/repros.rs index 6845f51b9bd4..b53002ae2d17 100644 --- a/crates/forge/tests/it/repros.rs +++ b/crates/forge/tests/it/repros.rs @@ -287,3 +287,6 @@ test_repro!(6554; |config| { cheats_config.fs_permissions.add(PathPermission::read_write(path)); config.runner.cheats_config = std::sync::Arc::new(cheats_config); }); + +// https://github.com/foundry-rs/foundry/issues/6759 +test_repro!(6759); diff --git a/testdata/repros/Issue6759.t.sol b/testdata/repros/Issue6759.t.sol new file mode 100644 index 000000000000..45a2f42b0620 --- /dev/null +++ b/testdata/repros/Issue6759.t.sol @@ -0,0 +1,19 @@ +// 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/6759 +contract Issue6759Test is DSTest { + Vm constant vm = Vm(HEVM_ADDRESS); + + function testCreateMulti() public { + uint256 fork1 = vm.createFork("rpcAlias", 10); + uint256 fork2 = vm.createFork("rpcAlias", 10); + uint256 fork3 = vm.createFork("rpcAlias", 10); + assert(fork1 != fork2); + assert(fork1 != fork3); + assert(fork2 != fork3); + } +} From 59e98a8704af3ebd97103549f09faae7f42a12cf Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Thu, 11 Jan 2024 19:16:25 +0100 Subject: [PATCH 2/2] fix return correct id --- crates/evm/core/src/fork/multi.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/evm/core/src/fork/multi.rs b/crates/evm/core/src/fork/multi.rs index 8e2c2e902381..5aa7261bd440 100644 --- a/crates/evm/core/src/fork/multi.rs +++ b/crates/evm/core/src/fork/multi.rs @@ -258,7 +258,7 @@ impl MultiForkHandler { let backend = fork.backend.clone(); let env = fork.opts.env.clone(); self.forks.insert(unique_fork_id.clone(), fork); - let _ = sender.send(Ok((fork_id, backend, env))); + 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) {