Tighten next bottom up checkpoint height (#824)

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();

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();
         }

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();
     }
 }

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");

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);
+    }
 }

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"
+        );
+    }
 }