bug: invariant setup failure with compiler profiles enabled

[grandizzy]

Component

Forge

Have you ensured that all of these are up to date?

• Foundry
• Foundryup

What version of Foundry are you on?

No response

What version of Foundryup are you on?

No response

What command(s) is the bug in?

No response

Operating System

None

Describe the bug

Steps to reproduce:

• git clone https://github.com/ethereum-optimism/optimism.git
• git checkout opcm-up/upgrade-asr - branch with following compiler profiles enabled

additional_compiler_profiles = [
  { name = "dispute", optimizer_runs = 5000 },
]
compilation_restrictions = [
  { paths = "src/dispute/FaultDisputeGame.sol", optimizer_runs = 5000 },
  { paths = "src/dispute/PermissionedDisputeGame.sol", optimizer_runs = 5000 },
  { paths = "src/L1/OPContractsManager.sol", optimizer_runs = 5000 },
]

• cd optimism/packages/contracts-bedrock
• forge test --mt invariant_gasLimitBoundaries fails with

Ran 1 test for test/invariants/SystemConfig.t.sol:SystemConfig_GasLimitBoundaries_Invariant
[FAIL: failed to set up invariant testing environment: src/universal/Proxy.sol:Proxy does not have the selector 0xb40a817c] invariant_gasLimitBoundaries() (runs: 0, calls: 0, reverts: 0)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 9.84ms (4.89ms CPU time)

Ran 1 test suite in 49.66ms (9.84ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/invariants/SystemConfig.t.sol:SystemConfig_GasLimitBoundaries_Invariant
[FAIL: failed to set up invariant testing environment: src/universal/Proxy.sol:Proxy does not have the selector 0xb40a817c] invariant_gasLimitBoundaries() (runs: 0, calls: 0, reverts: 0)

• test pass if removing compiler profiles from foundry.toml and forge clean && forge build

[klkvr]

This is likely a side-effect of #9722. We are not able to load a SystemConfig interface here:

foundry/crates/evm/evm/src/executors/invariant/mod.rs

Lines 766 to 772 in f94ce46

	for IInvariantTest::FuzzInterface { addr, artifacts } in &interfaces {
	// Identifiers are specified as an array, so we loop through them.
	for identifier in artifacts {
	// Try to find the contract by name or identifier in the project's contracts.
	if let Some((_, contract)) =
	self.project_contracts.find_by_name_or_identifier(identifier)?
	{

because ArtifactId::name looks like SystemConfig.default which doesn't match this check:

foundry/crates/common/src/contracts.rs

Lines 252 to 253 in f94ce46

	.filter(|(artifact, _)| artifact.name == id || artifact.identifier() == id)
	.collect::<Vec<_>>();

It should get fixed with foundry-rs/compilers#241 but we could also make this check smarter by trying to compare with artifact.name.split(".").next().unwrap() which we already do in a bunch of places for the same reason, e.g

foundry/crates/script/src/build.rs

Lines 213 to 222 in f94ce46

	if let Some(target) = target_id {
	// We might have multiple artifacts for the same contract but with different
	// solc versions. Their names will have form of {name}.0.X.Y, so we are
	// stripping versions off before comparing them.
	let target_name = target.name.split('.').next().unwrap();
	let id_name = id.name.split('.').next().unwrap();
	if target_name != id_name {
	eyre::bail!("Multiple contracts in the target path. Please specify the contract name with `--tc ContractName`")
	}
	}

[grandizzy]

It should get fixed with foundry-rs/compilers#241 but we could also make this check smarter by trying to compare with artifact.name.split(".").next().unwrap() which we already do in a bunch of places for the same reason, e.g

Thanks, should work, though it then could find several matching contracts when compiling with multiple profiles, and bail. I guess find_by_name_or_identifier should be changed to avoid such, wdyt? (find_by_name_or_identifier is used in invariant and script only and I don't think there should be any side effect by removing such)

-    /// Finds a contract which has the same contract name or identifier as `id`. If more than one is
-    /// found, return error.
+    /// Finds a contract which has the same contract name or identifier as `id`.
     pub fn find_by_name_or_identifier(
         &self,
         id: &str,
     ) -> Result<Option<ArtifactWithContractRef<'_>>> {
-        let contracts = self
-            .iter()
-            .filter(|(artifact, _)| artifact.name == id || artifact.identifier() == id)
-            .collect::<Vec<_>>();
+        let contract = self.iter().find(|(artifact, _)| {
+            artifact.name.split(".").next().unwrap() == id || artifact.identifier() == id
+        });
 
-        if contracts.len() > 1 {
-            eyre::bail!("{id} has more than one implementation.");
-        }
-
-        Ok(contracts.first().cloned())
+        Ok(contract)
     }

[klkvr]

I don't think we should bail because this would mean that test will still fail if SystemConfig is compiled with multiple profiles/versions

However ambigiuty here is indeed bad for scripts because we're using this data to find/identify bytecodes

Given that for invariants we're only interested in ABIs ambiguity is fine (ABIs are same for any artifact), so we could introduce an additional helper find_abi_by_name_or_identifier which would be used there, and keep find_by_name_or_identifier the same just for scripts