Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch minSigners to f in RMNRemote & RMNHome #1495

Closed
wants to merge 7 commits into from
Closed

switch minSigners to f in RMNRemote & RMNHome #1495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a1929ea
switch minsigners to f in RMNRemote
RyanRHall Oct 8, 2024
9213c52
switch minsigners to f in RMNHome
RyanRHall Oct 9, 2024
1966ef9
update wrappers and snapshot
RyanRHall Oct 9, 2024
28d868e
update struck packing comments
RyanRHall Oct 9, 2024
9468868
change && to nested if
RyanRHall Oct 10, 2024
bdfa217
Merge 946886833d5827956aca83099537a1b4090c8c32 into 4377d9ab5cfd40622…
RyanRHall Oct 15, 2024
26b2eb2
Update gethwrappers
app-token-issuer-infra-releng[bot] Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 26 additions & 27 deletions contracts/gas-snapshots/ccip.gas-snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,49 +804,48 @@ PingPong_plumbing:test_Pausing_Success() (gas: 17810)
PingPong_startPingPong:test_StartPingPong_With_OOO_Success() (gas: 167051)
PingPong_startPingPong:test_StartPingPong_With_Sequenced_Ordered_Success() (gas: 186469)
RMNHome__validateStaticAndDynamicConfig:test_validateStaticAndDynamicConfig_DuplicateOffchainPublicKey_reverts() (gas: 18822)
RMNHome__validateStaticAndDynamicConfig:test_validateStaticAndDynamicConfig_DuplicatePeerId_reverts() (gas: 18682)
RMNHome__validateStaticAndDynamicConfig:test_validateStaticAndDynamicConfig_DuplicateSourceChain_reverts() (gas: 20371)
RMNHome__validateStaticAndDynamicConfig:test_validateStaticAndDynamicConfig_MinObserversTooHigh_reverts() (gas: 20810)
RMNHome__validateStaticAndDynamicConfig:test_validateStaticAndDynamicConfig_OutOfBoundsNodesLength_reverts() (gas: 137268)
RMNHome__validateStaticAndDynamicConfig:test_validateStaticAndDynamicConfig_OutOfBoundsObserverNodeIndex_reverts() (gas: 20472)
RMNHome_getConfigDigests:test_getConfigDigests_success() (gas: 1077745)
RMNHome__validateStaticAndDynamicConfig:test_validateStaticAndDynamicConfig_DuplicatePeerId_reverts() (gas: 18704)
RMNHome__validateStaticAndDynamicConfig:test_validateStaticAndDynamicConfig_DuplicateSourceChain_reverts() (gas: 20359)
RMNHome__validateStaticAndDynamicConfig:test_validateStaticAndDynamicConfig_NotEnoughObservers_reverts() (gas: 21025)
RMNHome__validateStaticAndDynamicConfig:test_validateStaticAndDynamicConfig_OutOfBoundsNodesLength_reverts() (gas: 137290)
RMNHome__validateStaticAndDynamicConfig:test_validateStaticAndDynamicConfig_OutOfBoundsObserverNodeIndex_reverts() (gas: 20494)
RMNHome_getConfigDigests:test_getConfigDigests_success() (gas: 1078931)
RMNHome_promoteCandidateAndRevokeActive:test_promoteCandidateAndRevokeActive_ConfigDigestMismatch_reverts() (gas: 23857)
RMNHome_promoteCandidateAndRevokeActive:test_promoteCandidateAndRevokeActive_NoOpStateTransitionNotAllowed_reverts() (gas: 10575)
RMNHome_promoteCandidateAndRevokeActive:test_promoteCandidateAndRevokeActive_OnlyOwner_reverts() (gas: 10936)
RMNHome_promoteCandidateAndRevokeActive:test_promoteCandidateAndRevokeActive_success() (gas: 1083071)
RMNHome_promoteCandidateAndRevokeActive:test_promoteCandidateAndRevokeActive_success() (gas: 1084257)
RMNHome_revokeCandidate:test_revokeCandidate_ConfigDigestMismatch_reverts() (gas: 19063)
RMNHome_revokeCandidate:test_revokeCandidate_OnlyOwner_reverts() (gas: 10963)
RMNHome_revokeCandidate:test_revokeCandidate_RevokingZeroDigestNotAllowed_reverts() (gas: 10606)
RMNHome_revokeCandidate:test_revokeCandidate_success() (gas: 28147)
RMNHome_setCandidate:test_setCandidate_ConfigDigestMismatch_reverts() (gas: 594679)
RMNHome_setCandidate:test_setCandidate_ConfigDigestMismatch_reverts() (gas: 596458)
RMNHome_setCandidate:test_setCandidate_OnlyOwner_reverts() (gas: 15177)
RMNHome_setCandidate:test_setCandidate_success() (gas: 588379)
RMNHome_setCandidate:test_setCandidate_success() (gas: 588972)
RMNHome_setDynamicConfig:test_setDynamicConfig_DigestNotFound_reverts() (gas: 30159)
RMNHome_setDynamicConfig:test_setDynamicConfig_MinObserversTooHigh_reverts() (gas: 18848)
RMNHome_setDynamicConfig:test_setDynamicConfig_OnlyOwner_reverts() (gas: 14115)
RMNHome_setDynamicConfig:test_setDynamicConfig_success() (gas: 103911)
RMNHome_setDynamicConfig:test_setDynamicConfig_success() (gas: 104512)
RMNRemote_constructor:test_constructor_success() (gas: 8334)
RMNRemote_constructor:test_constructor_zeroChainSelector_reverts() (gas: 59165)
RMNRemote_constructor:test_constructor_zeroChainSelector_reverts() (gas: 59248)
RMNRemote_curse:test_curse_AlreadyCursed_duplicateSubject_reverts() (gas: 154457)
RMNRemote_curse:test_curse_calledByNonOwner_reverts() (gas: 18780)
RMNRemote_curse:test_curse_success() (gas: 149365)
RMNRemote_global_and_legacy_curses:test_global_and_legacy_curses_success() (gas: 133464)
RMNRemote_setConfig:test_setConfig_addSigner_removeSigner_success() (gas: 976479)
RMNRemote_setConfig:test_setConfig_duplicateOnChainPublicKey_reverts() (gas: 323272)
RMNRemote_setConfig:test_setConfig_invalidSignerOrder_reverts() (gas: 80138)
RMNRemote_setConfig:test_setConfig_minSignersIs0_success() (gas: 700548)
RMNRemote_setConfig:test_setConfig_minSignersTooHigh_reverts() (gas: 54024)
RMNRemote_curse:test_curse_success() (gas: 149299)
RMNRemote_global_and_legacy_curses:test_global_and_legacy_curses_success() (gas: 133573)
RMNRemote_setConfig:test_setConfig_addSigner_removeSigner_success() (gas: 996367)
RMNRemote_setConfig:test_setConfig_duplicateOnChainPublicKey_reverts() (gas: 323727)
RMNRemote_setConfig:test_setConfig_invalidSignerOrder_reverts() (gas: 80215)
RMNRemote_setConfig:test_setConfig_notEnoughSigners_reverts() (gas: 54472)
RMNRemote_uncurse:test_uncurse_NotCursed_duplicatedUncurseSubject_reverts() (gas: 51912)
RMNRemote_uncurse:test_uncurse_calledByNonOwner_reverts() (gas: 18748)
RMNRemote_uncurse:test_uncurse_success() (gas: 40151)
RMNRemote_verify_withConfigNotSet:test_verify_reverts() (gas: 13650)
RMNRemote_verify_withConfigSet:test_verify_InvalidSignature_reverts() (gas: 78519)
RMNRemote_verify_withConfigSet:test_verify_OutOfOrderSignatures_duplicateSignature_reverts() (gas: 76336)
RMNRemote_verify_withConfigSet:test_verify_OutOfOrderSignatures_not_sorted_reverts() (gas: 83399)
RMNRemote_verify_withConfigSet:test_verify_ThresholdNotMet_reverts() (gas: 153002)
RMNRemote_verify_withConfigSet:test_verify_UnexpectedSigner_reverts() (gas: 387667)
RMNRemote_verify_withConfigSet:test_verify_minSignersIsZero_success() (gas: 184524)
RMNRemote_verify_withConfigSet:test_verify_success() (gas: 68207)
RMNRemote_uncurse:test_uncurse_success() (gas: 40116)
RMNRemote_verify_withConfigNotSet:test_verify_reverts() (gas: 15763)
RMNRemote_verify_withConfigSet:test_verify_InvalidSignature_reverts() (gas: 78749)
RMNRemote_verify_withConfigSet:test_verify_OutOfOrderSignatures_duplicateSignature_reverts() (gas: 76566)
RMNRemote_verify_withConfigSet:test_verify_OutOfOrderSignatures_not_sorted_reverts() (gas: 83629)
RMNRemote_verify_withConfigSet:test_verify_ThresholdNotMet_reverts() (gas: 323003)
RMNRemote_verify_withConfigSet:test_verify_UnexpectedSigner_reverts() (gas: 388140)
RMNRemote_verify_withConfigSet:test_verify_disabled_success() (gas: 185739)
RMNRemote_verify_withConfigSet:test_verify_success() (gas: 68442)
RMN_constructor:test_Constructor_Success() (gas: 48994)
RMN_getRecordedCurseRelatedOps:test_OpsPostDeployment() (gas: 19732)
RMN_lazyVoteToCurseUpdate_Benchmark:test_VoteToCurseLazilyRetain3VotersUponConfigChange_gas() (gas: 152296)
Expand Down
10 changes: 5 additions & 5 deletions contracts/src/v0.8/ccip/rmn/RMNHome.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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; 2f+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;
}
Expand Down Expand Up @@ -382,9 +382,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();
}
}
}
Expand Down
25 changes: 15 additions & 10 deletions contracts/src/v0.8/ccip/rmn/RMNRemote.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -45,11 +45,11 @@ 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
bytes32 rmnHomeContractConfigDigest; // Digest of the RMNHome contract config
Signer[] signers; // List of signers
bool enabled; // ──────────────────────╮ Whether the RMNRemote verification is enabled or not
uint64 f; // ──────────────────────────╯ Max number of faulty RMN nodes; 2f+1 signers are required
}

/// @dev part of the payload that RMN nodes sign: keccak256(abi.encode(RMN_V1_6_ANY2EVM_REPORT, report))
Expand All @@ -60,7 +60,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
Expand Down Expand Up @@ -92,10 +92,13 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNRemote {
Signature[] calldata signatures,
uint256 rawVs
) external view {
(bool enabled, uint64 f) = (s_config.enabled, s_config.f);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's pretty non-standard notation. Both are also used only once, does inlining te storage read really add a significant amount of gas?


if (s_configCount == 0) {
revert ConfigNotSet();
}
if (signatures.length < s_config.minSigners) revert ThresholdNotMet();
if (!enabled) return; // RMNRemote verification is disabled
if (signatures.length < (2 * f) + 1) revert ThresholdNotMet();

bytes32 digest = keccak256(
abi.encode(
Expand Down Expand Up @@ -138,9 +141,11 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNRemote {
}
}

// minSigners is tenable
if (!(newConfig.minSigners <= newConfig.signers.length)) {
revert MinSignersTooHigh();
// min signers requirement is tenable
if (newConfig.enabled) {
if (newConfig.signers.length < 2 * newConfig.f + 1) {
revert NotEnoughSigners();
}
}

// clear the old signers
Expand Down
25 changes: 11 additions & 14 deletions contracts/src/v0.8/ccip/test/rmn/RMNHome.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: 0, observerNodesBitmap: 1 << 0});
// Observers 0, 1 and 2 for source chain 9001
sourceChains[1] = RMNHome.SourceChain({chainSelector: 9001, f: 0, observerNodesBitmap: 1 << 0 | 1 << 1 | 1 << 2});

return Config({
staticConfig: RMNHome.StaticConfig({nodes: nodes, offchainConfig: abi.encode("static_config")}),
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}
}
Expand All @@ -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();

Expand All @@ -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();
Expand All @@ -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);
Expand Down
Loading
Loading