From 7ea7a3b95ba683f79299bf0502b321c7803c7cc3 Mon Sep 17 00:00:00 2001 From: baku-ccron Date: Thu, 27 Aug 2026 18:06:46 +0000 Subject: [PATCH] Create every fork before selecting any Foundry captures the pre-fork EVM's account set when the FIRST fork is selected, and seeds every fork created after that capture with it, so `createSelectFork` inside a network loop put networks 2..n on the wrong side of the capture. A `dep.code.length` read in a deploy script, added for logging, was enough to make every network after the first revert `MissingDependency` against a dependency that has code there. Closes #157 Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 9 +- src/abstract/RainDeployVerifyChain.sol | 6 +- src/lib/LibRainDeploy.sol | 48 ++++-- test/src/abstract/RainDeployVerifyChain.t.sol | 33 ++++ test/src/lib/LibRainDeploy.t.sol | 145 ++++++++++++++++++ 5 files changed, 224 insertions(+), 17 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 09d36c5..475af76 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -33,6 +33,9 @@ relocated. - **`slither.config.json` filters those abstracts by exact filename, never by the `src/abstract/` prefix.** Keep it that way when adding an abstract: a prefix filter would silently exempt a deployable file added there later. -- **A `vm.createSelectFork` failure is not a missing deployment.** It is an - unreachable or rate-limited endpoint. Only `NotDeployedOnNetwork`, from a - network that forked, says anything about the deployment. +- **A `vm.createFork` or `vm.createSelectFork` failure is not a missing + deployment.** It is an unreachable or rate-limited endpoint. Only + `NotDeployedOnNetwork`, from a network that forked, says anything about the + deployment. The deploy and the chain matrix create every fork before selecting + any, so such a failure takes the whole run before any network is checked + rather than stopping partway down the list. diff --git a/src/abstract/RainDeployVerifyChain.sol b/src/abstract/RainDeployVerifyChain.sol index 1338aef..b50ab26 100644 --- a/src/abstract/RainDeployVerifyChain.sol +++ b/src/abstract/RainDeployVerifyChain.sol @@ -120,11 +120,9 @@ abstract contract RainDeployVerifyChain is RainDeployVerifyBase { } string[] memory networks = LibRainDeploy.supportedNetworks(); + uint256[] memory forkIds = LibRainDeploy.createForks(vm, networks); for (uint256 i = 0; i < networks.length; i++) { - // createSelectFork returns a fork id that is not needed here; bind - // and reference it so the unused-return lint stays satisfied. - uint256 forkId = vm.createSelectFork(networks[i]); - (forkId); + vm.selectFork(forkIds[i]); for (uint256 j = 0; j < derived.length; j++) { checkDeployedOnNetwork(networks[i], derived[j]); } diff --git a/src/lib/LibRainDeploy.sol b/src/lib/LibRainDeploy.sol index 2c6dda5..1f0c3e3 100644 --- a/src/lib/LibRainDeploy.sol +++ b/src/lib/LibRainDeploy.sol @@ -254,6 +254,33 @@ library LibRainDeploy { return deployedAddress; } + /// Creates a fork for every network, returning their ids in list order. + /// Nothing is selected here: the caller selects each in turn. + /// + /// Creating them all BEFORE the first one is selected is the whole point. + /// Foundry captures the account set of the pre-fork EVM when the first fork + /// is selected, and seeds every fork CREATED after that capture with it, so + /// any address the calling script touched beforehand is carried onto the + /// second and later networks as the empty account the default 31337 EVM has + /// for it. A `dep.code.length` read added to a deploy script for logging is + /// enough. Forks created before the capture read their chain, which is why + /// the first network was always right and every one after it was wrong. + /// + /// Closed here rather than by a rule about what a deploy script may read, + /// because the trap is invisible from where a consumer sits: the read is + /// ordinary, the failure names a real address on a network that really has + /// it, and nothing connects the two. + /// @param vm The Vm instance to fork with. + /// @param networks The network names to fork, as `[rpc_endpoints]` aliases. + /// @return The fork id of each network, positionally paired. + function createForks(Vm vm, string[] memory networks) internal returns (uint256[] memory) { + uint256[] memory forkIds = new uint256[](networks.length); + for (uint256 i = 0; i < networks.length; i++) { + forkIds[i] = vm.createFork(networks[i]); + } + return forkIds; + } + /// Returns the list of networks currently supported by Rain deployments. /// @return The list of supported network names. function supportedNetworks() internal pure returns (string[] memory) { @@ -390,11 +417,9 @@ library LibRainDeploy { if (readCalls.length == 0) { revert NoResolvedAddressReads(target); } + uint256[] memory forkIds = createForks(vm, networks); for (uint256 i = 0; i < networks.length; i++) { - // createSelectFork returns a fork id that is not needed here; bind - // and reference it so the unused-return lint stays satisfied. - uint256 forkId = vm.createSelectFork(networks[i]); - (forkId); + vm.selectFork(forkIds[i]); console2.log("Checking resolved addresses on network:", networks[i]); checkResolvedAddresses(networks[i], target, readCalls, expectedAddresses); } @@ -415,8 +440,13 @@ library LibRainDeploy { /// already-deployed dependency as missing and abort an otherwise-valid /// deploy. Each network is handled independently: the Zoltu deploy is /// idempotent (an existing contract is skipped), so a failure on one network - /// leaves the others intact and the script can simply be re-run, which is why - /// no separate all-network pre-flight is needed. + /// leaves the others intact and the script can simply be re-run. + /// + /// The forks themselves are all created up front, before any is selected — + /// `createForks` says why it has to be that way round. Endpoint reachability + /// is therefore the one thing that IS all-network: an alias that cannot be + /// forked stops the run before anything is broadcast, rather than partway + /// through it. /// @param vm The Vm instance to use for forking and broadcasting. /// @param networks The list of network names to deploy to. /// @param deployer The deployer address. @@ -450,11 +480,9 @@ library LibRainDeploy { if (derivedAddress != expectedAddress) { revert UnexpectedDeployedAddress(expectedAddress, derivedAddress); } + uint256[] memory forkIds = createForks(vm, networks); for (uint256 i = 0; i < networks.length; i++) { - // createSelectFork returns a fork id that is not needed here; bind - // and reference it so the unused-return lint stays satisfied. - uint256 forkId = vm.createSelectFork(networks[i]); - (forkId); + vm.selectFork(forkIds[i]); console2.log("Deploying to network:", networks[i]); console2.log("Block number:", block.number); diff --git a/test/src/abstract/RainDeployVerifyChain.t.sol b/test/src/abstract/RainDeployVerifyChain.t.sol index d11b4b5..267deb6 100644 --- a/test/src/abstract/RainDeployVerifyChain.t.sol +++ b/test/src/abstract/RainDeployVerifyChain.t.sol @@ -112,6 +112,39 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain this.testSuitesLiveOnEverySupportedNetwork(); } + /// The matrix MUST create every fork BEFORE it checks anything on any of + /// them, for the reason `LibRainDeploy.createForks` gives: a fork created + /// after the first select is seeded with whatever was read before it, so a + /// deployed address the caller touched first would read as absent on every + /// network but the one the matrix reaches first. + /// + /// A run that fails on that FIRST network is what makes the order visible. + /// The matrix never reached the last supported network, so a fork of it + /// exists only if it was created up front. + function testChainMatrixCreatesEveryForkFirst() external { + vm.etch(ADDRESS_REGISTRY_DEPLOYED_ADDRESS, hex""); + + vm.expectRevert( + abi.encodeWithSelector( + NotDeployedOnNetwork.selector, + LibRainDeploy.ARBITRUM_ONE, + "address-registry-0-0-1", + ADDRESS_REGISTRY_DEPLOYED_ADDRESS + ) + ); + this.testSuitesLiveOnEverySupportedNetwork(); + + // This test forks nothing of its own, so the matrix's forks are ids 0 + // upwards in `supportedNetworks()` order. Selecting the last of them at + // all is the assertion; the chain ids say it is a different network + // from the one the failure named, rather than another fork of it. + uint256 lastNetwork = LibRainDeploy.supportedNetworks().length - 1; + vm.selectFork(0); + uint256 firstChainId = block.chainid; + vm.selectFork(lastNetwork); + assertNotEq(block.chainid, firstChainId); + } + /// EVERY suite MUST be checked, not just the first one the matrix /// reaches. The version missing here is the LAST one, so a matrix that /// stopped after the first version would pass. diff --git a/test/src/lib/LibRainDeploy.t.sol b/test/src/lib/LibRainDeploy.t.sol index adbaf70..9c5be2c 100644 --- a/test/src/lib/LibRainDeploy.t.sol +++ b/test/src/lib/LibRainDeploy.t.sol @@ -235,6 +235,67 @@ contract LibRainDeployTest is Test { assertEq(LibRainDeploy.ZOLTU_FACTORY.codehash, LibRainDeploy.ZOLTU_FACTORY_CODEHASH); } + /// `createForks` MUST create every fork and select NONE of them, because + /// creating them all before the first select is the whole of what it is for. + /// + /// Foundry captures the account set of the pre-fork EVM when the first fork + /// is SELECTED, and seeds every fork created after that capture with it. A + /// `createForks` that selected as it went would put every fork but the first + /// on the wrong side of that capture, which is the defect it exists to + /// remove. + function testCreateForksSelectsNothing() external { + string[] memory networks = new string[](2); + networks[0] = LibRainDeploy.BASE; + networks[1] = LibRainDeploy.ARBITRUM_ONE; + + uint256[] memory forkIds = LibRainDeploy.createForks(vm, networks); + assertEq(forkIds.length, 2); + + // `activeFork()` reverts when nothing is selected, so the call failing + // is the assertion. Made low level for that reason: a plain `vm` call + // would take the test down with it. + (bool active,) = address(vm).call(abi.encodeWithSignature("activeFork()")); + assertFalse(active, "createForks selected a fork"); + + // Every id it handed back is a real fork of the network in that + // position, so nothing was skipped or forked twice. + vm.selectFork(forkIds[0]); + assertEq(block.chainid, BASE_CHAIN_ID); + vm.selectFork(forkIds[1]); + assertEq(block.chainid, ARBITRUM_ONE_CHAIN_ID); + } + + /// A read the caller made BEFORE any fork existed MUST NOT decide what a + /// `createForks` fork holds, on any network. + /// + /// The read here is the one a deploy script makes for logging, and on the + /// default 31337 EVM it answers zero for an address that is on every chain + /// this repo deploys to. Forks created after the first select are seeded + /// with that answer, so the whole difference between a network that reads + /// its chain and one that reports the Zoltu factory missing is the order + /// the forks were created in. + function testCreateForksIgnoresAPreForkRead() external { + // The poisoning read, before anything is forked. + assertEq(LibRainDeploy.ZOLTU_FACTORY.code.length, 0); + + string[] memory networks = new string[](2); + networks[0] = LibRainDeploy.BASE; + networks[1] = LibRainDeploy.ARBITRUM_ONE; + + uint256[] memory forkIds = LibRainDeploy.createForks(vm, networks); + + // The SECOND network is the one that reads the pre-fork answer when the + // forks are created as the loop reaches them. Both are asserted so a + // first network that broke would be seen as well. + vm.selectFork(forkIds[0]); + assertEq(block.chainid, BASE_CHAIN_ID); + assertEq(LibRainDeploy.ZOLTU_FACTORY.codehash, LibRainDeploy.ZOLTU_FACTORY_CODEHASH); + + vm.selectFork(forkIds[1]); + assertEq(block.chainid, ARBITRUM_ONE_CHAIN_ID); + assertEq(LibRainDeploy.ZOLTU_FACTORY.codehash, LibRainDeploy.ZOLTU_FACTORY_CODEHASH); + } + /// External wrapper for `deployAndBroadcast` so that /// `vm.expectRevert` works at the correct call depth. /// @param networks The list of network names to deploy to. @@ -323,6 +384,50 @@ contract LibRainDeployTest is Test { assertEq(result.codehash, mockDeployableCodeHash()); } + /// A read of a declared dependency BEFORE any fork exists MUST NOT change + /// what `deployToNetworks` sees on any network. + /// + /// This is rainlanguage/rain.deploy#157 end to end. A deploy script that + /// logs `dependencies[i].code.length` executes that read on the default + /// 31337 EVM, where the account is empty, and every fork created after the + /// first select is seeded with it — so the first network read its chain and + /// every one after it reverted `MissingDependency` against a dependency + /// that demonstrably has code there. It cost several failed production + /// deploy runs to diagnose, because the check itself is sound and the + /// address it names is real. + /// + /// The dependency is the Zoltu factory because it is the one address this + /// repo knows is live on every supported network, so the second network + /// disagreeing with the first can only be the defect and never the chain. + function testDeployToNetworksIgnoresAPreForkDependencyRead() external { + // The read a deploy script makes for logging, before anything forks. + assertEq(LibRainDeploy.ZOLTU_FACTORY.code.length, 0); + + string[] memory networks = new string[](2); + networks[0] = LibRainDeploy.BASE; + networks[1] = LibRainDeploy.ARBITRUM_ONE; + + address[] memory dependencies = new address[](1); + dependencies[0] = LibRainDeploy.ZOLTU_FACTORY; + + address result = this.externalDeployToNetworks( + networks, + address(this), + type(MockDeployable).creationCode, + "test/concrete/MockDeployable.sol:MockDeployable", + mockDeployableAddress(), + mockDeployableCodeHash(), + dependencies + ); + assertEq(result, mockDeployableAddress()); + + // The second network is the one the pre-fork read used to take out, and + // a deploy landed on it. + vm.selectFork(1); + assertEq(block.chainid, ARBITRUM_ONE_CHAIN_ID); + assertEq(result.codehash, mockDeployableCodeHash()); + } + /// External wrapper for `deployToNetworks` so that `vm.expectRevert` /// works at the correct call depth. /// @param networks The list of network names to deploy to. @@ -1251,6 +1356,46 @@ contract LibRainDeployTest is Test { this.externalCheckResolvedAddressesOnNetworks(networks, address(target), ownerReadCalls(), expected(account)); } + /// `checkResolvedAddressesOnNetworks` MUST create every fork BEFORE it + /// checks anything on any of them, for the reason `createForks` gives: a + /// fork created after the first select is seeded with whatever the caller + /// read before the call, so a target it logged would read as having no code + /// on every network but the first. + /// + /// A run that fails on the FIRST network is what makes the order visible + /// from outside. The loop never reached the second network, so a fork of it + /// exists only if it was created up front. + function testCheckResolvedAddressesOnNetworksCreatesEveryForkFirst() external { + bytes32 name = keccak256("testCheckResolvedAddressesOnNetworksCreatesEveryForkFirst"); + address account = address(0xf00); + address wrong = address(0xba4); + (, MockResolvedOwner consumer) = deployRegistryAndConsumer(name, account); + vm.makePersistent(address(consumer)); + + string[] memory networks = new string[](2); + networks[0] = LibRainDeploy.ARBITRUM_ONE; + networks[1] = LibRainDeploy.BASE; + + vm.expectRevert( + abi.encodeWithSelector( + LibRainDeploy.UnexpectedResolvedAddress.selector, + LibRainDeploy.ARBITRUM_ONE, + address(consumer), + uint256(0), + wrong, + account + ) + ); + this.externalCheckResolvedAddressesOnNetworks(networks, address(consumer), ownerReadCalls(), expected(wrong)); + + // This test forks nothing of its own, so ids 0 and 1 are the call's, in + // the order it was given. Selecting id 1 at all is the assertion; the + // chain id says it is the second network rather than another fork of + // the first. + vm.selectFork(1); + assertEq(block.chainid, BASE_CHAIN_ID); + } + /// `deployToNetworks` MUST deploy when every dependency has code on the /// network, i.e. a present dependency is not treated as missing. function testDeployToNetworksPresentDependencyDeploys() external {