Change minSigners to f in RMNRemote/RMNHome (#14817)

* switch minsigners to f in RMNRemote

* switch minsigners to f in RMNHome

* update struck packing comments

* change && to nested if

* reduce signer requirement from 2f+1 to f+1

* remove enabled flag from RMN

* move rmn activation flag to offramp

* add changeset

* reduce optimizer runs for offramp

* [Bot] Update changeset file with jira issues

* swap minSigners config for f in offchain code

* fix integration test

* don't use f=0 in RMNHome tests

* fix TestRMNHomeReader_GetRMNNodesInfo

* update TestInitialDeploy with timeout and RMN disabled

* change commitment timeout to 3 minutes

* add error handling to wg.wait()

* [Bot] Update changeset file with jira issues

* update rmn_test.go

* update snapshot & wrappers

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>

Author: RyanRHall

contracts/.changeset/swift-pumpkins-impress.md

@@ -0,0 +1,10 @@
+---
+'@chainlink/contracts': minor
+---
+
+change minSigners to f in RMNRemote/RMNHome
+
+
+PR issue: CCIP-3614
+
+Solidity Review issue: CCIP-3966

contracts/foundry.toml

@@ -26,7 +26,7 @@ single_line_statement_blocks = "preserve"
 solc_version = '0.8.24'
 src = 'src/v0.8/ccip'
 test = 'src/v0.8/ccip/test'
-optimizer_runs = 800 
+optimizer_runs = 500
 evm_version = 'paris'
 
 [profile.functions]

contracts/gas-snapshots/ccip.gas-snapshot

@@ -8,7 +8,7 @@ echo " └───────────────────────
 
 SOLC_VERSION="0.8.24"
 OPTIMIZE_RUNS=26000
-OPTIMIZE_RUNS_OFFRAMP=800
+OPTIMIZE_RUNS_OFFRAMP=500
 
 
 SCRIPTPATH="$( cd "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"

contracts/scripts/native_solc_compile_all_ccip

@@ -123,7 +123,8 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
   /// @dev Since DynamicConfig is part of DynamicConfigSet event, if changing it, we should update the ABI on Atlas
   struct DynamicConfig {
     address feeQuoter; // ──────────────────────────────╮ FeeQuoter address on the local chain
-    uint32 permissionLessExecutionThresholdSeconds; // ─╯ Waiting time before manual execution is enabled
+    uint32 permissionLessExecutionThresholdSeconds; //  | Waiting time before manual execution is enabled
+    bool isRMNVerificationDisabled; // ─────────────────╯ Flag whether the RMN verification is disabled or not
     address messageInterceptor; // Optional message interceptor to validate incoming messages (zero address = no interceptor)
   }
 
@@ -787,10 +788,13 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
     bytes32 rawVs
   ) external {
     CommitReport memory commitReport = abi.decode(report, (CommitReport));
+    DynamicConfig storage dynamicConfig = s_dynamicConfig;
 
     // Verify RMN signatures
-    if (commitReport.merkleRoots.length > 0) {
-      i_rmnRemote.verify(address(this), commitReport.merkleRoots, commitReport.rmnSignatures, commitReport.rmnRawVs);
+    if (!dynamicConfig.isRMNVerificationDisabled) {
+      if (commitReport.merkleRoots.length > 0) {
+        i_rmnRemote.verify(address(this), commitReport.merkleRoots, commitReport.rmnSignatures, commitReport.rmnRawVs);
+      }
     }
 
     // Check if the report contains price updates
@@ -803,7 +807,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
         // If prices are not stale, update the latest epoch and round
         s_latestPriceSequenceNumber = ocrSequenceNumber;
         // And update the prices in the fee quoter
-        IFeeQuoter(s_dynamicConfig.feeQuoter).updatePrices(commitReport.priceUpdates);
+        IFeeQuoter(dynamicConfig.feeQuoter).updatePrices(commitReport.priceUpdates);
       } else {
         // If prices are stale and the report doesn't contain a root, this report
         // does not have any valid information and we revert.

contracts/src/v0.8/ccip/offRamp/OffRamp.sol

@@ -68,7 +68,7 @@ contract RMNHome is OwnerIsCreator, ITypeAndVersion {
   error DuplicateOffchainPublicKey();
   error DuplicateSourceChain();
   error OutOfBoundsObserverNodeIndex();
-  error MinObserversTooHigh();
+  error NotEnoughObservers();
   error ConfigDigestMismatch(bytes32 expectedConfigDigest, bytes32 gotConfigDigest);
   error DigestNotFound(bytes32 configDigest);
   error RevokingZeroDigestNotAllowed();
@@ -81,7 +81,7 @@ contract RMNHome is OwnerIsCreator, ITypeAndVersion {
 
   struct SourceChain {
     uint64 chainSelector; // ─────╮ The Source chain selector.
-    uint64 minObservers; // ──────╯ Required number of observers to agree on an observation for this source chain.
+    uint64 f; // ─────────────────╯ Maximum number of faulty observers; f+1 observers required to agree on an observation for this source chain.
     // ObserverNodesBitmap & (1<<i) == (1<<i) iff StaticConfig.nodes[i] is an observer for this source chain.
     uint256 observerNodesBitmap;
   }
@@ -386,9 +386,9 @@ contract RMNHome is OwnerIsCreator, ITypeAndVersion {
         bitmap &= bitmap - 1;
       }
 
-      // minObservers are tenable
-      if (currentSourceChain.minObservers > observersCount) {
-        revert MinObserversTooHigh();
+      // min observers are tenable
+      if (observersCount < 2 * currentSourceChain.f + 1) {
+        revert NotEnoughObservers();
       }
     }
   }

contracts/src/v0.8/ccip/rmn/RMNHome.sol

@@ -27,7 +27,7 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNRemote {
   error DuplicateOnchainPublicKey();
   error InvalidSignature();
   error InvalidSignerOrder();
-  error MinSignersTooHigh();
+  error NotEnoughSigners();
   error NotCursed(bytes16 subject);
   error OutOfOrderSignatures();
   error ThresholdNotMet();
@@ -45,11 +45,10 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNRemote {
   }
 
   /// @dev the contract config
-  /// @dev note: minSigners can be set to 0 to disable verification for chains without RMN support
   struct Config {
     bytes32 rmnHomeContractConfigDigest; // Digest of the RMNHome contract config
-    Signer[] signers; // List of signers
-    uint64 minSigners; // Threshold for the number of signers required to verify a report
+    Signer[] signers; //                    List of signers
+    uint64 f; //                            Max number of faulty RMN nodes; f+1 signers are required to verify a report, must configure 2f+1 signers in total
   }
 
   /// @dev part of the payload that RMN nodes sign: keccak256(abi.encode(RMN_V1_6_ANY2EVM_REPORT, report))
@@ -60,7 +59,7 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNRemote {
     address rmnRemoteContractAddress; // ─────╯ The address of this contract
     address offrampAddress; //                  The address of the offramp on the same chain as this contract
     bytes32 rmnHomeContractConfigDigest; //     The digest of the RMNHome contract config
-    Internal.MerkleRoot[] merkleRoots; //   The dest lane updates
+    Internal.MerkleRoot[] merkleRoots; //       The dest lane updates
   }
 
   /// @dev this is included in the preimage of the digest that RMN nodes sign
@@ -97,7 +96,7 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNRemote {
     if (s_configCount == 0) {
       revert ConfigNotSet();
     }
-    if (signatures.length < s_config.minSigners) revert ThresholdNotMet();
+    if (signatures.length < s_config.f + 1) revert ThresholdNotMet();
 
     bytes32 digest = keccak256(
       abi.encode(
@@ -142,9 +141,9 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNRemote {
       }
     }
 
-    // minSigners is tenable
-    if (!(newConfig.minSigners <= newConfig.signers.length)) {
-      revert MinSignersTooHigh();
+    // min signers requirement is tenable
+    if (newConfig.signers.length < 2 * newConfig.f + 1) {
+      revert NotEnoughSigners();
     }
 
     // clear the old signers

contracts/src/v0.8/ccip/rmn/RMNRemote.sol

@@ -3336,6 +3336,47 @@ contract OffRamp_commit is OffRampSetup {
     assertEq(block.timestamp, s_offRamp.getMerkleRoot(SOURCE_CHAIN_SELECTOR_1, root));
   }
 
+  function test_RootWithRMNDisabled_success() public {
+    // force RMN verification to fail
+    vm.mockCallRevert(address(s_mockRMNRemote), abi.encodeWithSelector(IRMNRemote.verify.selector), bytes(""));
+
+    // but ☝️ doesn't matter because RMN verification is disabled
+    OffRamp.DynamicConfig memory dynamicConfig = _generateDynamicOffRampConfig(address(s_feeQuoter));
+    dynamicConfig.isRMNVerificationDisabled = true;
+    s_offRamp.setDynamicConfig(dynamicConfig);
+
+    uint64 max1 = 931;
+    bytes32 root = "Only a single root";
+
+    Internal.MerkleRoot[] memory roots = new Internal.MerkleRoot[](1);
+    roots[0] = Internal.MerkleRoot({
+      sourceChainSelector: SOURCE_CHAIN_SELECTOR_1,
+      onRampAddress: ON_RAMP_ADDRESS_1,
+      minSeqNr: 1,
+      maxSeqNr: max1,
+      merkleRoot: root
+    });
+
+    OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({
+      priceUpdates: _getEmptyPriceUpdates(),
+      merkleRoots: roots,
+      rmnSignatures: s_rmnSignatures,
+      rmnRawVs: 0
+    });
+
+    vm.expectEmit();
+    emit OffRamp.CommitReportAccepted(commitReport.merkleRoots, commitReport.priceUpdates);
+
+    vm.expectEmit();
+    emit MultiOCR3Base.Transmitted(uint8(Internal.OCRPluginType.Commit), s_configDigestCommit, s_latestSequenceNumber);
+
+    _commit(commitReport, s_latestSequenceNumber);
+
+    assertEq(max1 + 1, s_offRamp.getSourceChainConfig(SOURCE_CHAIN_SELECTOR).minSeqNr);
+    assertEq(0, s_offRamp.getLatestPriceSequenceNumber());
+    assertEq(block.timestamp, s_offRamp.getMerkleRoot(SOURCE_CHAIN_SELECTOR_1, root));
+  }
+
   function test_StaleReportWithRoot_Success() public {
     uint64 maxSeq = 12;
     uint224 tokenStartPrice = IFeeQuoter(s_offRamp.getDynamicConfig().feeQuoter).getTokenPrice(s_sourceFeeToken).value;

contracts/src/v0.8/ccip/test/offRamp/OffRamp.t.sol

@@ -173,8 +173,9 @@ contract OffRampSetup is FeeQuoterSetup, MultiOCR3BaseSetup {
     address feeQuoter
   ) internal pure returns (OffRamp.DynamicConfig memory) {
     return OffRamp.DynamicConfig({
-      permissionLessExecutionThresholdSeconds: PERMISSION_LESS_EXECUTION_THRESHOLD_SECONDS,
       feeQuoter: feeQuoter,
+      permissionLessExecutionThresholdSeconds: PERMISSION_LESS_EXECUTION_THRESHOLD_SECONDS,
+      isRMNVerificationDisabled: false,
       messageInterceptor: address(0)
     });
   }

contracts/src/v0.8/ccip/test/offRamp/OffRampSetup.t.sol

@@ -23,9 +23,9 @@ contract RMNHomeTest is Test {
 
     RMNHome.SourceChain[] memory sourceChains = new RMNHome.SourceChain[](2);
     // Observer 0 for source chain 9000
-    sourceChains[0] = RMNHome.SourceChain({chainSelector: 9000, minObservers: 1, observerNodesBitmap: 1 << 0});
-    // Observers 1 and 2 for source chain 9001
-    sourceChains[1] = RMNHome.SourceChain({chainSelector: 9001, minObservers: 2, observerNodesBitmap: 1 << 1 | 1 << 2});
+    sourceChains[0] = RMNHome.SourceChain({chainSelector: 9000, f: 1, observerNodesBitmap: 1 << 0 | 1 << 1 | 1 << 2});
+    // Observers 0, 1 and 2 for source chain 9001
+    sourceChains[1] = RMNHome.SourceChain({chainSelector: 9001, f: 1, observerNodesBitmap: 1 << 0 | 1 << 1 | 1 << 2});
 
     return Config({
       staticConfig: RMNHome.StaticConfig({nodes: nodes, offchainConfig: abi.encode("static_config")}),
@@ -114,7 +114,7 @@ contract RMNHome_setCandidate is RMNHomeTest {
     for (uint256 i = 0; i < storedDynamicConfig.sourceChains.length; i++) {
       RMNHome.SourceChain memory storedSourceChain = storedDynamicConfig.sourceChains[i];
       assertEq(storedSourceChain.chainSelector, versionedConfig.dynamicConfig.sourceChains[i].chainSelector);
-      assertEq(storedSourceChain.minObservers, versionedConfig.dynamicConfig.sourceChains[i].minObservers);
+      assertEq(storedSourceChain.f, versionedConfig.dynamicConfig.sourceChains[i].f);
       assertEq(storedSourceChain.observerNodesBitmap, versionedConfig.dynamicConfig.sourceChains[i].observerNodesBitmap);
     }
     assertEq(storedDynamicConfig.offchainConfig, versionedConfig.dynamicConfig.offchainConfig);
@@ -152,7 +152,7 @@ contract RMNHome_revokeCandidate is RMNHomeTest {
     bytes32 digest = s_rmnHome.setCandidate(config.staticConfig, config.dynamicConfig, ZERO_DIGEST);
     s_rmnHome.promoteCandidateAndRevokeActive(digest, ZERO_DIGEST);
 
-    config.dynamicConfig.sourceChains[0].minObservers--;
+    config.dynamicConfig.sourceChains[1].f--;
     s_rmnHome.setCandidate(config.staticConfig, config.dynamicConfig, ZERO_DIGEST);
   }
 
@@ -305,11 +305,11 @@ contract RMNHome__validateStaticAndDynamicConfig is RMNHomeTest {
     s_rmnHome.setCandidate(config.staticConfig, config.dynamicConfig, ZERO_DIGEST);
   }
 
-  function test_validateStaticAndDynamicConfig_MinObserversTooHigh_reverts() public {
+  function test_validateStaticAndDynamicConfig_NotEnoughObservers_reverts() public {
     Config memory config = _getBaseConfig();
-    config.dynamicConfig.sourceChains[0].minObservers++;
+    config.dynamicConfig.sourceChains[0].f++;
 
-    vm.expectRevert(RMNHome.MinObserversTooHigh.selector);
+    vm.expectRevert(RMNHome.NotEnoughObservers.selector);
     s_rmnHome.setCandidate(config.staticConfig, config.dynamicConfig, ZERO_DIGEST);
   }
 }
@@ -324,7 +324,7 @@ contract RMNHome_setDynamicConfig is RMNHomeTest {
     (bytes32 priorActiveDigest,) = s_rmnHome.getConfigDigests();
 
     Config memory config = _getBaseConfig();
-    config.dynamicConfig.sourceChains[0].minObservers--;
+    config.dynamicConfig.sourceChains[1].f--;
 
     (, bytes32 candidateConfigDigest) = s_rmnHome.getConfigDigests();
 
@@ -335,10 +335,7 @@ contract RMNHome_setDynamicConfig is RMNHomeTest {
 
     (RMNHome.VersionedConfig memory storedVersionedConfig, bool ok) = s_rmnHome.getConfig(candidateConfigDigest);
     assertTrue(ok);
-    assertEq(
-      storedVersionedConfig.dynamicConfig.sourceChains[0].minObservers,
-      config.dynamicConfig.sourceChains[0].minObservers
-    );
+    assertEq(storedVersionedConfig.dynamicConfig.sourceChains[0].f, config.dynamicConfig.sourceChains[0].f);
 
     // Asser the digests don't change when updating the dynamic config
     (bytes32 activeDigest, bytes32 candidateDigest) = s_rmnHome.getConfigDigests();
@@ -349,7 +346,7 @@ contract RMNHome_setDynamicConfig is RMNHomeTest {
   // Asserts the validation function is being called
   function test_setDynamicConfig_MinObserversTooHigh_reverts() public {
     Config memory config = _getBaseConfig();
-    config.dynamicConfig.sourceChains[0].minObservers++;
+    config.dynamicConfig.sourceChains[0].f++;
 
     vm.expectRevert(abi.encodeWithSelector(RMNHome.DigestNotFound.selector, ZERO_DIGEST));
     s_rmnHome.setDynamicConfig(config.dynamicConfig, ZERO_DIGEST);