fix: make ForkIds unique (#6765)

* fix: make ForkIds unique

* fix return correct id

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<LocalForkId> {
+    fn create_fork(&mut self, create_fork: CreateFork) -> eyre::Result<LocalForkId> {
         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 <https://github.com/foundry-rs/foundry/issues/5935>
-        // 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<LocalForkId>,
@@ -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)

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<CreateFork>, CreateSender),
     /// Returns the Fork backend for the `ForkId` if it exists
     GetFork(ForkId, OneshotSender<Option<SharedBackend>>),
-    /// 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<ForkTask>,
 
-    /// 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<ForkId, CreatedFork>,
 
     /// 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((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) {
@@ -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,22 +419,30 @@ impl Future for MultiForkHandler {
 }
 
 /// Tracks the created Fork
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 struct CreatedFork {
     /// How the fork was initially created
     opts: CreateFork,
     /// Copy of the sender
     backend: SharedBackend,
     /// How many consumers there are, since a `SharedBacked` can be used by multiple
     /// consumers
-    num_senders: usize,
+    num_senders: Arc<AtomicUsize>,
 }
 
 // === 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)
     }
 }
 

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);

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);
+    }
+}