From 53b07eaba853103f9cde04f1fd15c5947f296756 Mon Sep 17 00:00:00 2001 From: cryptoAtwill <108330426+cryptoAtwill@users.noreply.github.com> Date: Wed, 20 Mar 2024 09:45:36 +0800 Subject: [PATCH] Tighten next bottom up checkpoint height (#824) --- contracts/src/errors/IPCErrors.sol | 1 + .../src/gateway/router/CheckpointingFacet.sol | 3 - .../subnet/SubnetActorCheckpointingFacet.sol | 60 +++--- .../test/integration/GatewayDiamond.t.sol | 18 -- .../test/integration/SubnetActorDiamond.t.sol | 176 +++++++++++++++++- contracts/test/unit/LibGateway.t.sol | 25 +++ 6 files changed, 233 insertions(+), 50 deletions(-) diff --git a/contracts/src/errors/IPCErrors.sol b/contracts/src/errors/IPCErrors.sol index e2150b332..ccb550514 100644 --- a/contracts/src/errors/IPCErrors.sol +++ b/contracts/src/errors/IPCErrors.sol @@ -22,6 +22,7 @@ error FailedRemoveIncompleteQuorum(); error GatewayCannotBeZero(); error InvalidActorAddress(); error InvalidCheckpointEpoch(); +error CannotSubmitFutureCheckpoint(); error InvalidBatchEpoch(); error InvalidCheckpointSource(); error InvalidBatchSource(); diff --git a/contracts/src/gateway/router/CheckpointingFacet.sol b/contracts/src/gateway/router/CheckpointingFacet.sol index d7fce3534..e9f6633e1 100644 --- a/contracts/src/gateway/router/CheckpointingFacet.sol +++ b/contracts/src/gateway/router/CheckpointingFacet.sol @@ -52,9 +52,6 @@ contract CheckpointingFacet is GatewayActorModifiers { bytes32 membershipRootHash, uint256 membershipWeight ) external systemActorOnly { - if (checkpoint.blockHeight % s.bottomUpCheckPeriod != 0) { - revert InvalidCheckpointEpoch(); - } if (LibGateway.bottomUpCheckpointExists(checkpoint.blockHeight)) { revert CheckpointAlreadyExists(); } diff --git a/contracts/src/subnet/SubnetActorCheckpointingFacet.sol b/contracts/src/subnet/SubnetActorCheckpointingFacet.sol index 10877cb15..d0a3c03bd 100644 --- a/contracts/src/subnet/SubnetActorCheckpointingFacet.sol +++ b/contracts/src/subnet/SubnetActorCheckpointingFacet.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 pragma solidity ^0.8.23; -import {InvalidBatchEpoch, MaxMsgsPerBatchExceeded, InvalidSignatureErr, BottomUpCheckpointAlreadySubmitted, InvalidCheckpointEpoch} from "../errors/IPCErrors.sol"; +import {InvalidBatchEpoch, MaxMsgsPerBatchExceeded, InvalidSignatureErr, BottomUpCheckpointAlreadySubmitted, CannotSubmitFutureCheckpoint, InvalidCheckpointEpoch} from "../errors/IPCErrors.sol"; import {IGateway} from "../interfaces/IGateway.sol"; import {BottomUpCheckpoint, BottomUpMsgBatch, BottomUpMsgBatchInfo} from "../structs/CrossNet.sol"; import {Validator, ValidatorSet} from "../structs/Subnet.sol"; @@ -12,6 +12,7 @@ import {LibValidatorSet, LibStaking} from "../lib/LibStaking.sol"; import {EnumerableSet} from "openzeppelin-contracts/utils/structs/EnumerableSet.sol"; import {LibSubnetActor} from "../lib/LibSubnetActor.sol"; import {Pausable} from "../lib/LibPausable.sol"; +import {LibGateway} from "../lib/LibGateway.sol"; contract SubnetActorCheckpointingFacet is SubnetActorModifiers, ReentrancyGuard, Pausable { using EnumerableSet for EnumerableSet.AddressSet; @@ -32,28 +33,20 @@ contract SubnetActorCheckpointingFacet is SubnetActorModifiers, ReentrancyGuard, bytes32 checkpointHash = keccak256(abi.encode(checkpoint)); - if (checkpoint.blockHeight <= s.lastBottomUpCheckpointHeight) { - revert BottomUpCheckpointAlreadySubmitted(); - } - - // When a bottom up msg batch becomes full, a bottom up checkpoint will be - // created before the next checkpoint period is reached. - if (checkpoint.blockHeight <= s.lastBottomUpCheckpointHeight + s.bottomUpCheckPeriod) { - // validate signatures and quorum threshold, revert if validation fails - validateActiveQuorumSignatures({signatories: signatories, hash: checkpointHash, signatures: signatures}); + // validate signatures and quorum threshold, revert if validation fails + validateActiveQuorumSignatures({signatories: signatories, hash: checkpointHash, signatures: signatures}); - // If the checkpoint height is the next expected height then this is a new checkpoint which must be executed - // in the Gateway Actor, the checkpoint and the relayer must be stored, last bottom-up checkpoint updated. - s.committedCheckpoints[checkpoint.blockHeight] = checkpoint; + // If the checkpoint height is the next expected height then this is a new checkpoint which must be executed + // in the Gateway Actor, the checkpoint and the relayer must be stored, last bottom-up checkpoint updated. + s.committedCheckpoints[checkpoint.blockHeight] = checkpoint; - s.lastBottomUpCheckpointHeight = checkpoint.blockHeight; + s.lastBottomUpCheckpointHeight = checkpoint.blockHeight; - // Commit in gateway to distribute rewards - IGateway(s.ipcGatewayAddr).commitCheckpoint(checkpoint); + // Commit in gateway to distribute rewards + IGateway(s.ipcGatewayAddr).commitCheckpoint(checkpoint); - // confirming the changes in membership in the child - LibStaking.confirmChange(checkpoint.nextConfigurationNumber); - } + // confirming the changes in membership in the child + LibStaking.confirmChange(checkpoint.nextConfigurationNumber); } /// @notice Checks whether the signatures are valid for the provided signatories and hash within the current validator set. @@ -96,17 +89,30 @@ contract SubnetActorCheckpointingFacet is SubnetActorModifiers, ReentrancyGuard, revert MaxMsgsPerBatchExceeded(); } - // if the bottom up messages' length is max, we consider that epoch valid - if (checkpoint.msgs.length == s.maxMsgsPerBottomUpBatch) { + uint256 lastBottomUpCheckpointHeight = s.lastBottomUpCheckpointHeight; + uint256 bottomUpCheckPeriod = s.bottomUpCheckPeriod; + + // cannot submit past bottom up checkpoint + if (checkpoint.blockHeight <= lastBottomUpCheckpointHeight) { + revert BottomUpCheckpointAlreadySubmitted(); + } + + uint256 nextCheckpointHeight = LibGateway.getNextEpoch(lastBottomUpCheckpointHeight, bottomUpCheckPeriod); + + if (checkpoint.blockHeight > nextCheckpointHeight) { + revert CannotSubmitFutureCheckpoint(); + } + + // the expected bottom up checkpoint height, valid height + if (checkpoint.blockHeight == nextCheckpointHeight) { return; } - // the max batch size not reached, we only support checkpoint period submission. - uint256 lastBottomUpCheckpointHeight = s.lastBottomUpCheckpointHeight; - if (checkpoint.blockHeight != lastBottomUpCheckpointHeight + s.bottomUpCheckPeriod) { - if (checkpoint.blockHeight != lastBottomUpCheckpointHeight) { - revert InvalidCheckpointEpoch(); - } + // if the bottom up messages' length is max, we consider that epoch valid, allow early submission + if (checkpoint.msgs.length == s.maxMsgsPerBottomUpBatch) { + return; } + + revert InvalidCheckpointEpoch(); } } diff --git a/contracts/test/integration/GatewayDiamond.t.sol b/contracts/test/integration/GatewayDiamond.t.sol index 2ecdd66cf..aa33a321f 100644 --- a/contracts/test/integration/GatewayDiamond.t.sol +++ b/contracts/test/integration/GatewayDiamond.t.sol @@ -1124,24 +1124,6 @@ contract GatewayActorDiamondTest is Test, IntegrationTestBase { ); vm.stopPrank(); - // failed to create a checkpoint with the height not multiple to checkpoint period - checkpoint = BottomUpCheckpoint({ - subnetID: gatewayDiamond.getter().getNetworkName(), - blockHeight: d + d / 2, - blockHash: keccak256("block2"), - nextConfigurationNumber: 2, - msgs: new IpcEnvelope[](0) - }); - - vm.startPrank(FilAddress.SYSTEM_ACTOR); - vm.expectRevert(InvalidCheckpointEpoch.selector); - gatewayDiamond.checkpointer().createBottomUpCheckpoint( - checkpoint, - membershipRoot, - weights[0] + weights[1] + weights[2] - ); - vm.stopPrank(); - (bool ok, uint256 e, ) = gatewayDiamond.getter().getCurrentBottomUpCheckpoint(); require(ok, "checkpoint not exist"); require(e == d, "out height incorrect"); diff --git a/contracts/test/integration/SubnetActorDiamond.t.sol b/contracts/test/integration/SubnetActorDiamond.t.sol index 7535697ef..5b64ee7c8 100644 --- a/contracts/test/integration/SubnetActorDiamond.t.sol +++ b/contracts/test/integration/SubnetActorDiamond.t.sol @@ -714,13 +714,13 @@ contract SubnetActorDiamondTest is Test, IntegrationTestBase { // skip the current checkpoint, should fail checkpointWithIncorrectHeight.blockHeight = saDiamond.getter().bottomUpCheckPeriod() + 1; - vm.expectRevert(InvalidCheckpointEpoch.selector); + vm.expectRevert(CannotSubmitFutureCheckpoint.selector); vm.prank(validators[0]); saDiamond.checkpointer().submitCheckpoint(checkpointWithIncorrectHeight, validators, signatures); // skip the curent checkpoint but submit at the next bottom up checkpoint, should fail checkpointWithIncorrectHeight.blockHeight = saDiamond.getter().bottomUpCheckPeriod() * 2; - vm.expectRevert(InvalidCheckpointEpoch.selector); + vm.expectRevert(CannotSubmitFutureCheckpoint.selector); vm.prank(validators[0]); saDiamond.checkpointer().submitCheckpoint(checkpointWithIncorrectHeight, validators, signatures); @@ -839,6 +839,163 @@ contract SubnetActorDiamondTest is Test, IntegrationTestBase { saDiamond.checkpointer().submitCheckpoint(checkpoint, validators, signatures); } + function testSubnetActorDiamond_submitCheckpoint_mixAndMatch() public { + (uint256[] memory keys, address[] memory validators, ) = TestUtils.getThreeValidators(vm); + bytes[] memory pubKeys = new bytes[](3); + bytes[] memory signatures = new bytes[](3); + + for (uint256 i = 0; i < 3; i++) { + vm.deal(validators[i], 10 gwei); + pubKeys[i] = TestUtils.deriveValidatorPubKeyBytes(keys[i]); + vm.prank(validators[i]); + saDiamond.manager().join{value: 10}(pubKeys[i]); + } + + vm.deal(address(saDiamond), 100 ether); + vm.prank(address(saDiamond)); + gatewayDiamond.manager().register{value: DEFAULT_MIN_VALIDATOR_STAKE + 3 * DEFAULT_CROSS_MSG_FEE}( + 3 * DEFAULT_CROSS_MSG_FEE + ); + + SubnetID memory localSubnetID = saDiamond.getter().getParent().createSubnetId(address(saDiamond)); + + IpcEnvelope[] memory msgs = new IpcEnvelope[](MAX_MSGS_PER_BATCH); + for (uint256 i = 0; i < MAX_MSGS_PER_BATCH; i++) { + IpcEnvelope memory crossMsg = TestUtils.newXnetCallMsg( + IPCAddress({subnetId: localSubnetID, rawAddress: FvmAddressHelper.from(address(saDiamond))}), + IPCAddress({ + subnetId: saDiamond.getter().getParent(), + rawAddress: FvmAddressHelper.from(address(saDiamond)) + }), + 1, + uint64(i) + ); + msgs[i] = crossMsg; + } + + vm.prank(validators[0]); + + // submit a full msg batch, even though next expected height is bottomUpCheckPeriod() + BottomUpCheckpoint memory checkpoint = BottomUpCheckpoint({ + subnetID: localSubnetID, + blockHeight: 1, + blockHash: keccak256("block1"), + nextConfigurationNumber: 0, + msgs: msgs + }); + submitCheckpointInternal(checkpoint, validators, signatures, keys); + require(saDiamond.getter().lastBottomUpCheckpointHeight() == 1, " checkpoint height incorrect"); + + // submit a full msg batch, allow early submission, + // even though next expected height is bottomUpCheckPeriod() + checkpoint = BottomUpCheckpoint({ + subnetID: localSubnetID, + blockHeight: 3, + blockHash: keccak256("block2"), + nextConfigurationNumber: 0, + msgs: msgs + }); + submitCheckpointInternal(checkpoint, validators, signatures, keys); + require(saDiamond.getter().lastBottomUpCheckpointHeight() == 3, " checkpoint height incorrect"); + + // should not allow submission of past checkpoints already confirmed, last bottom up checkpoint height is 3 + checkpoint = BottomUpCheckpoint({ + subnetID: localSubnetID, + blockHeight: 2, + blockHash: keccak256("block1"), + nextConfigurationNumber: 0, + msgs: msgs + }); + vm.expectRevert(BottomUpCheckpointAlreadySubmitted.selector); + submitCheckpointInternal(checkpoint, validators, signatures, keys); + + // submit future checkpoint, should reject + checkpoint = BottomUpCheckpoint({ + subnetID: localSubnetID, + blockHeight: saDiamond.getter().bottomUpCheckPeriod() + 1, + blockHash: keccak256("block2"), + nextConfigurationNumber: 0, + msgs: msgs + }); + vm.expectRevert(CannotSubmitFutureCheckpoint.selector); + submitCheckpointInternal(checkpoint, validators, signatures, keys); + + checkpoint = BottomUpCheckpoint({ + subnetID: localSubnetID, + blockHeight: saDiamond.getter().bottomUpCheckPeriod(), + blockHash: keccak256("block2"), + nextConfigurationNumber: 0, + msgs: new IpcEnvelope[](0) + }); + submitCheckpointInternal(checkpoint, validators, signatures, keys); + require( + saDiamond.getter().lastBottomUpCheckpointHeight() == saDiamond.getter().bottomUpCheckPeriod(), + " checkpoint height incorrect" + ); + + checkpoint = BottomUpCheckpoint({ + subnetID: localSubnetID, + blockHeight: saDiamond.getter().bottomUpCheckPeriod() + 1, + blockHash: keccak256("block2"), + nextConfigurationNumber: 0, + msgs: msgs + }); + submitCheckpointInternal(checkpoint, validators, signatures, keys); + require( + saDiamond.getter().lastBottomUpCheckpointHeight() == saDiamond.getter().bottomUpCheckPeriod() + 1, + " checkpoint height incorrect" + ); + + checkpoint = BottomUpCheckpoint({ + subnetID: localSubnetID, + blockHeight: saDiamond.getter().bottomUpCheckPeriod() + 2, + blockHash: keccak256("block2"), + nextConfigurationNumber: 0, + msgs: msgs + }); + submitCheckpointInternal(checkpoint, validators, signatures, keys); + require( + saDiamond.getter().lastBottomUpCheckpointHeight() == saDiamond.getter().bottomUpCheckPeriod() + 2, + " checkpoint height incorrect" + ); + + checkpoint = BottomUpCheckpoint({ + subnetID: localSubnetID, + blockHeight: saDiamond.getter().bottomUpCheckPeriod() + 3, + blockHash: keccak256("block2"), + nextConfigurationNumber: 0, + msgs: new IpcEnvelope[](0) + }); + vm.expectRevert(InvalidCheckpointEpoch.selector); + submitCheckpointInternal(checkpoint, validators, signatures, keys); + + checkpoint = BottomUpCheckpoint({ + subnetID: localSubnetID, + blockHeight: saDiamond.getter().bottomUpCheckPeriod() * 2, + blockHash: keccak256("block2"), + nextConfigurationNumber: 0, + msgs: new IpcEnvelope[](0) + }); + submitCheckpointInternal(checkpoint, validators, signatures, keys); + require( + saDiamond.getter().lastBottomUpCheckpointHeight() == saDiamond.getter().bottomUpCheckPeriod() * 2, + " checkpoint height incorrect" + ); + + checkpoint = BottomUpCheckpoint({ + subnetID: localSubnetID, + blockHeight: saDiamond.getter().bottomUpCheckPeriod() * 3, + blockHash: keccak256("block2"), + nextConfigurationNumber: 0, + msgs: new IpcEnvelope[](0) + }); + submitCheckpointInternal(checkpoint, validators, signatures, keys); + require( + saDiamond.getter().lastBottomUpCheckpointHeight() == saDiamond.getter().bottomUpCheckPeriod() * 3, + " checkpoint height incorrect" + ); + } + function testSubnetActorDiamond_submitCheckpointWithReward() public { (uint256[] memory keys, address[] memory validators, ) = TestUtils.getThreeValidators(vm); bytes[] memory pubKeys = new bytes[](3); @@ -1626,4 +1783,19 @@ contract SubnetActorDiamondTest is Test, IntegrationTestBase { saDiamond.pauser().unpause(); require(!saDiamond.pauser().paused(), "paused"); } + + function submitCheckpointInternal( + BottomUpCheckpoint memory checkpoint, + address[] memory validators, + bytes[] memory signatures, + uint256[] memory keys + ) internal { + bytes32 hash = keccak256(abi.encode(checkpoint)); + for (uint256 i = 0; i < 3; i++) { + (uint8 v, bytes32 r, bytes32 s) = vm.sign(keys[i], hash); + signatures[i] = abi.encodePacked(r, s, v); + } + + saDiamond.checkpointer().submitCheckpoint(checkpoint, validators, signatures); + } } diff --git a/contracts/test/unit/LibGateway.t.sol b/contracts/test/unit/LibGateway.t.sol index d04fc96fb..ad06ef546 100644 --- a/contracts/test/unit/LibGateway.t.sol +++ b/contracts/test/unit/LibGateway.t.sol @@ -426,4 +426,29 @@ contract LibGatewayTest is Test { t.applyMsg(childSubnet, crossMsg); } + + function test_nextCheckpointEpoch() public pure { + uint64 checkpointPeriod = 10; + + require(LibGateway.getNextEpoch(0, checkpointPeriod) == checkpointPeriod, "next epoch not correct"); + require(LibGateway.getNextEpoch(1, checkpointPeriod) == checkpointPeriod, "next epoch not correct"); + require(LibGateway.getNextEpoch(10, checkpointPeriod) == checkpointPeriod * 2, "next epoch not correct"); + require(LibGateway.getNextEpoch(15, checkpointPeriod) == checkpointPeriod * 2, "next epoch not correct"); + + checkpointPeriod = 17; + + require(LibGateway.getNextEpoch(0, checkpointPeriod) == checkpointPeriod, "next epoch not correct"); + require( + LibGateway.getNextEpoch(checkpointPeriod - 1, checkpointPeriod) == checkpointPeriod, + "next epoch not correct" + ); + require( + LibGateway.getNextEpoch(checkpointPeriod, checkpointPeriod) == checkpointPeriod * 2, + "next epoch not correct" + ); + require( + LibGateway.getNextEpoch(checkpointPeriod + 1, checkpointPeriod) == checkpointPeriod * 2, + "next epoch not correct" + ); + } }