Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

Method to list incomplete checkpoints #207

Merged
merged 18 commits into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
394 changes: 220 additions & 174 deletions .storage-layouts/GatewayActorModifiers.json

Large diffs are not rendered by default.

392 changes: 219 additions & 173 deletions .storage-layouts/GatewayDiamond.json

Large diffs are not rendered by default.

186 changes: 93 additions & 93 deletions .storage-layouts/SubnetActorDiamond.json

Large diffs are not rendered by default.

188 changes: 94 additions & 94 deletions .storage-layouts/SubnetActorModifiers.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/GatewayDiamond.sol
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ contract GatewayDiamond {
s.topDownCheckPeriod = params.topDownCheckPeriod;
s.crossMsgFee = params.msgFee;
s.majorityPercentage = params.majorityPercentage;
s.bottomUpCheckpointRetentionIndex = 1;

// the root doesn't need to be explicitly initialized
if (s.networkName.isRoot()) {
Expand Down
5 changes: 5 additions & 0 deletions src/errors/IPCErrors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ error CallerHasNoStake();
error CannotReleaseZero();
error CannotSendCrossMsgToItself();
error CheckpointAlreadyExists();
error CheckpointAlreadyProcess();
error CheckpointInfoAlreadyExists();
error IncompleteCheckpointExists();
error FailedAddIncompleteCheckpoint();
error FailedRemoveIncompleteCheckpoint();
error CheckpointNotChained();
error CollateralIsZero();
error CollateralStillLockedInSubnet();
Expand Down Expand Up @@ -49,6 +53,7 @@ error NotSystemActor();
error NotRegisteredSubnet();
error NotValidator();
error PostboxNotExist();
error InvalidRetentionIndex();
error SignatureReplay();
error SubnetAlreadyKilled();
error SubnetNotActive();
Expand Down
29 changes: 28 additions & 1 deletion src/gateway/GatewayGetterFacet.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: MIT OR Apache-2.0
pragma solidity 0.8.19;

import {CrossMsg, BottomUpCheckpoint, StorableMsg, ParentFinality, CheckpointInfo} from "../structs/Checkpoint.sol";
import {CrossMsg, BottomUpCheckpoint, BottomUpCheckpointNew, StorableMsg, ParentFinality, CheckpointInfo} from "../structs/Checkpoint.sol";
import {EpochVoteTopDownSubmission} from "../structs/EpochVoteSubmission.sol";
import {SubnetID, Subnet} from "../structs/Subnet.sol";
import {Membership} from "../structs/Validator.sol";
Expand All @@ -10,13 +10,15 @@ import {LibGateway} from "../lib/LibGateway.sol";
import {GatewayActorStorage} from "../lib/LibGatewayActorStorage.sol";
import {LibVoting} from "../lib/LibVoting.sol";
import {SubnetIDHelper} from "../lib/SubnetIDHelper.sol";
import {EnumerableSet} from "openzeppelin-contracts/utils/structs/EnumerableSet.sol";

contract GatewayGetterFacet {
// slither-disable-next-line uninitialized-state
GatewayActorStorage internal s;

using SubnetIDHelper for SubnetID;
using CheckpointHelper for BottomUpCheckpoint;
using EnumerableSet for EnumerableSet.UintSet;

function crossMsgFee() external view returns (uint256) {
return s.crossMsgFee;
Expand Down Expand Up @@ -194,4 +196,29 @@ contract GatewayGetterFacet {
function getCheckpointCurrentWeight(uint64 h) public view returns (uint256) {
return s.bottomUpCheckpointInfo[h].currentWeight;
}

/// @notice get the incomplete checkpoint heights
function getIncompleteCheckpointHeights() public view returns (uint256[] memory) {
return s.incompleteCheckpoints.values();
}

/// @notice get the incomplete checkpoints
function getIncompleteCheckpoints() public view returns (BottomUpCheckpointNew[] memory) {
uint256[] memory heights = s.incompleteCheckpoints.values();
uint256 size = heights.length;

BottomUpCheckpointNew[] memory checkpoints = new BottomUpCheckpointNew[](size);
for (uint64 i = 0; i < size; ) {
checkpoints[i] = s.bottomUpCheckpoints[uint64(heights[i])];
unchecked {
++i;
}
}
return checkpoints;
}

/// @notice get the bottom-up checkpoint retention index
function getBottomUpRetentionIndex() public view returns (uint64) {
dnkolegov marked this conversation as resolved.
Show resolved Hide resolved
return s.bottomUpCheckpointRetentionIndex;
}
}
53 changes: 48 additions & 5 deletions src/gateway/GatewayRouterFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import {IPCMsgType} from "../enums/IPCMsgType.sol";
import {SubnetID, Subnet} from "../structs/Subnet.sol";
import {IPCMsgType} from "../enums/IPCMsgType.sol";
import {Membership} from "../structs/Validator.sol";
import {InconsistentPrevCheckpoint, NotEnoughSubnetCircSupply, InvalidCheckpointEpoch, InvalidSignature, NotAuthorized, SignatureReplay} from "../errors/IPCErrors.sol";
import {InvalidCheckpointSource, InvalidCrossMsgNonce, InvalidCrossMsgDstSubnet, CheckpointAlreadyExists, CheckpointInfoAlreadyExists} from "../errors/IPCErrors.sol";
import {InconsistentPrevCheckpoint, NotEnoughSubnetCircSupply, InvalidCheckpointEpoch, InvalidSignature, NotAuthorized, SignatureReplay, InvalidRetentionIndex, FailedRemoveIncompleteCheckpoint} from "../errors/IPCErrors.sol";
import {InvalidCheckpointSource, InvalidCrossMsgNonce, InvalidCrossMsgDstSubnet, CheckpointAlreadyExists, CheckpointInfoAlreadyExists, IncompleteCheckpointExists, CheckpointAlreadyProcess, FailedAddIncompleteCheckpoint} from "../errors/IPCErrors.sol";
import {MessagesNotSorted, NotInitialized, NotEnoughBalance, NotRegisteredSubnet} from "../errors/IPCErrors.sol";
import {NotValidator, SubnetNotActive, CheckpointNotCreated, CheckpointMembershipNotCreated, ZeroMembershipWeight} from "../errors/IPCErrors.sol";
import {SubnetIDHelper} from "../lib/SubnetIDHelper.sol";
Expand All @@ -25,6 +25,7 @@ import {FvmAddressHelper} from "../lib/FvmAddressHelper.sol";
import {FilAddress} from "fevmate/utils/FilAddress.sol";
import {ECDSA} from "openzeppelin-contracts/utils/cryptography/ECDSA.sol";
import {MerkleProof} from "openzeppelin-contracts/utils/cryptography/MerkleProof.sol";
import {EnumerableSet} from "openzeppelin-contracts/utils/structs/EnumerableSet.sol";

contract GatewayRouterFacet is GatewayActorModifiers {
using FilAddress for address;
Expand All @@ -34,6 +35,8 @@ contract GatewayRouterFacet is GatewayActorModifiers {
using CrossMsgHelper for CrossMsg;
using FvmAddressHelper for FvmAddress;
using StorableMsgHelper for StorableMsg;
using EnumerableSet for EnumerableSet.UintSet;
using EnumerableSet for EnumerableSet.AddressSet;

event QuorumReached(uint64 height, bytes32 checkpoint, uint256 quorumWeight);
event QuorumWeightUpdated(uint64 height, bytes32 checkpoint, uint256 newWeight);
Expand Down Expand Up @@ -199,6 +202,9 @@ contract GatewayRouterFacet is GatewayActorModifiers {
uint256 weight,
bytes memory signature
) external {
if (height < s.bottomUpCheckpointRetentionIndex) {
revert CheckpointAlreadyProcess();
dnkolegov marked this conversation as resolved.
Show resolved Hide resolved
}
BottomUpCheckpointNew memory checkpoint = s.bottomUpCheckpoints[height];
if (checkpoint.blockHeight == 0) {
revert CheckpointNotCreated();
Expand All @@ -218,7 +224,7 @@ contract GatewayRouterFacet is GatewayActorModifiers {
}

// Check whether the validator has already sent a valid signature
if (s.bottomUpCollectedSignatures[height][recoveredSignatory]) {
if (s.bottomUpCollectedSignatures[height].contains(recoveredSignatory)) {
revert SignatureReplay();
}

Expand All @@ -233,7 +239,10 @@ contract GatewayRouterFacet is GatewayActorModifiers {
// All checks passed.
// Adding signature and emitting events.

s.bottomUpCollectedSignatures[height][recoveredSignatory] = true;
bool ok = s.bottomUpCollectedSignatures[height].add(recoveredSignatory);
if (!ok) {
revert FailedAddIncompleteCheckpoint();
aakoshh marked this conversation as resolved.
Show resolved Hide resolved
}
checkpointInfo.currentWeight += weight;

if (checkpointInfo.currentWeight >= checkpointInfo.threshold) {
Expand Down Expand Up @@ -263,20 +272,28 @@ contract GatewayRouterFacet is GatewayActorModifiers {
bytes32 membershipRootHash,
uint256 membershipWeight
) external systemActorOnly {
if (checkpoint.blockHeight < s.bottomUpCheckpointRetentionIndex) {
revert CheckpointAlreadyProcess();
}
if (s.bottomUpCheckpoints[checkpoint.blockHeight].blockHeight > 0) {
revert CheckpointAlreadyExists();
}
if (s.bottomUpCheckpointInfo[checkpoint.blockHeight].threshold > 0) {
revert CheckpointInfoAlreadyExists();
}

if (membershipWeight == 0) {
revert ZeroMembershipWeight();
}

uint256 threshold = LibGateway.getThreshold(membershipWeight);

// store checkpoint
s.bottomUpCheckpoints[checkpoint.blockHeight] = checkpoint;

bool ok = s.incompleteCheckpoints.add(checkpoint.blockHeight);
if (!ok) {
revert FailedAddIncompleteCheckpoint();
}
aakoshh marked this conversation as resolved.
Show resolved Hide resolved
s.bottomUpCheckpointInfo[checkpoint.blockHeight] = CheckpointInfo({
hash: checkpoint.toHash(),
rootHash: membershipRootHash,
Expand All @@ -285,4 +302,30 @@ contract GatewayRouterFacet is GatewayActorModifiers {
reached: false
});
}

/// @notice garbage collect all checkpoints and related data for heights lower than `newIndex`.
/// @param newIndex - new retention index
function pruneBottomUpCheckpoints(uint64 newIndex) external systemActorOnly {
Copy link
Contributor

Choose a reason for hiding this comment

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

When is this function expected to be called externally? The idea is that this function should be called when there is no longer need to store the checkpoint signatures because it has already been submitted up, right?

If this is the case, would you mind adding this description in the comments so we are explcit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is that this function should be called when there is no longer need to store the checkpoint signatures because it has already been submitted up, right

correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated description

Copy link
Contributor

Choose a reason for hiding this comment

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

How do we expect to set the newIndex for a specific garbage collection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, the caller should know what checkpoints have been processed in the parent and can be removed in the child

uint64 oldRetentionIndex = s.bottomUpCheckpointRetentionIndex;

if (newIndex <= oldRetentionIndex) {
revert InvalidRetentionIndex();
}

for (uint64 h = oldRetentionIndex; h < newIndex; ) {
delete s.bottomUpCheckpoints[h];
delete s.bottomUpCheckpointInfo[h];
delete s.bottomUpCollectedSignatures[h];
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also clear the bottomUpCheckpointsLegacy while it exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those two bottom-up checkpoint types are probably misleading. But bottomUpCheckpointsLegacy is not used in implementing the new bottom-up checkpoint mechanism. It is in the code just to build it without removing all libraries, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I understand we want to get rid of it, just wanted to highlight that since I'm relying on it in consensus-shipyard/fendermint#286 due to the lack of any alternatives, technically it will have data in it as long as it exists and someone sends a bottom-up message this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be fixed in #214

bool ok = s.incompleteCheckpoints.remove(h);
if (!ok) {
revert FailedRemoveIncompleteCheckpoint();
}

unchecked {
++h;
}
}

s.bottomUpCheckpointRetentionIndex = newIndex;
}
}
10 changes: 9 additions & 1 deletion src/lib/LibGatewayActorStorage.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {SubnetID, Subnet} from "../structs/Subnet.sol";
import {Membership} from "../structs/Validator.sol";
import {AccountHelper} from "../lib/AccountHelper.sol";
import {FilAddress} from "fevmate/utils/FilAddress.sol";
import {EnumerableSet} from "openzeppelin-contracts/utils/structs/EnumerableSet.sol";

struct GatewayActorStorage {
/// @notice List of subnets
Expand Down Expand Up @@ -38,8 +39,15 @@ struct GatewayActorStorage {
/// @notice A mapping of block numbers to checkpoint data
// slither-disable-next-line uninitialized-state
mapping(uint64 => CheckpointInfo) bottomUpCheckpointInfo;
/// @notice The height of the last bottom-up checkpoint registered in the parent.
/// All checkpoint with the height less than or equal to that number can be garbage collected in the child subnet.
/// @dev Initial retention index is 1.
uint64 bottomUpCheckpointRetentionIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind explaining here that the retention is needed so the checkpoint and signatures are available so relayers can submit the chceckpoint? It wasn't clear to me the purpose of the RetentionIndex on a first read. Thanks!

Copy link
Contributor Author

@dnkolegov dnkolegov Sep 26, 2023

Choose a reason for hiding this comment

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

All checkpoints with a height less than retentionIndex may be removed from the history if we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant in the code, so it is clear :) But no need if you feel it may be redundant or clear once the end-to-end is implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code

/// @notice A list of incomplete checkpoints.
// slither-disable-next-line uninitialized-state
EnumerableSet.UintSet incompleteCheckpoints;
/// @notice The validators have already sent signatures at height `h`
mapping(uint64 => mapping(address => bool)) bottomUpCollectedSignatures;
mapping(uint64 => EnumerableSet.AddressSet) bottomUpCollectedSignatures;
/// @notice epoch => SubnetID => [childIndex, exists(0 - no, 1 - yes)]
mapping(uint64 => mapping(bytes32 => uint256[2])) children;
/// @notice epoch => SubnetID => check => exists
Expand Down
91 changes: 91 additions & 0 deletions test/GatewayDiamond.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -1457,6 +1457,55 @@ contract GatewayDiamondDeploymentTest is StdInvariant, Test {
vm.stopPrank();
}

function testGatewayDiamond_listIncompleteCheckpoints() public {
(, address[] memory addrs, uint256[] memory weights) = setupValidatorsForCheckpoint();

(bytes32 membershipRoot, ) = MerkleTreeHelper.createMerkleProofsForValidators(addrs, weights);

BottomUpCheckpointNew memory checkpoint1 = BottomUpCheckpointNew({
subnetID: gwGetter.getNetworkName(),
blockHeight: 1,
blockHash: keccak256("block1"),
nextConfigurationNumber: 1,
crossMessagesHash: keccak256("messages1")
});

BottomUpCheckpointNew memory checkpoint2 = BottomUpCheckpointNew({
subnetID: gwGetter.getNetworkName(),
blockHeight: 2,
blockHash: keccak256("block2"),
nextConfigurationNumber: 1,
crossMessagesHash: keccak256("messages2")
});

// create a checkpoint
vm.startPrank(FilAddress.SYSTEM_ACTOR);
gwRouter.createBottomUpCheckpoint(checkpoint1, membershipRoot, 10);
gwRouter.createBottomUpCheckpoint(checkpoint2, membershipRoot, 25);
vm.stopPrank();

uint256[] memory heights = gwGetter.getIncompleteCheckpointHeights();

require(heights.length == 2, "heights.length == 2");
require(heights[0] == 1, "heights[0] == 1");
require(heights[1] == 2, "heights[1] == 2");

CheckpointInfo memory info = gwGetter.getCheckpointInfo(1);
require(info.rootHash == membershipRoot, "info.rootHash == membershipRoot");
require(info.threshold == 7, "info.threshold == 10");

info = gwGetter.getCheckpointInfo(2);
require(info.rootHash == membershipRoot, "info.rootHash == membershipRoot");
require(info.threshold == 17, "info.threshold == 25");

BottomUpCheckpointNew[] memory incomplete = gwGetter.getIncompleteCheckpoints();
require(incomplete.length == 2, "incomplete.length == 2");
require(incomplete[0].blockHeight == 1, "incomplete[0].blockHeight");
require(incomplete[0].blockHash == keccak256("block1"), "incomplete[0].blockHash");
require(incomplete[1].blockHeight == 2, "incomplete[1].blockHeight");
require(incomplete[1].blockHash == keccak256("block2"), "incomplete[1].blockHash");
}

function testGatewayDiamond_addCheckpointSignature() public {
(uint256[] memory privKeys, address[] memory addrs, uint256[] memory weights) = setupValidatorsForCheckpoint();

Expand Down Expand Up @@ -1528,6 +1577,48 @@ contract GatewayDiamondDeploymentTest is StdInvariant, Test {
vm.stopPrank();
}

function testGatewayDiamond_garbage_collect_bottomUpCheckpoints() public {
(, address[] memory addrs, uint256[] memory weights) = setupValidatorsForCheckpoint();

(bytes32 membershipRoot, ) = MerkleTreeHelper.createMerkleProofsForValidators(addrs, weights);

uint64 index = gwGetter.getBottomUpRetentionIndex();
require(index == 1, "retention index is 1");

BottomUpCheckpointNew memory checkpoint;

// create a checkpoint
uint64 n = 10;
vm.startPrank(FilAddress.SYSTEM_ACTOR);
for (uint64 i = 1; i <= n; i++) {
checkpoint = BottomUpCheckpointNew({
subnetID: gwGetter.getNetworkName(),
blockHeight: i,
blockHash: keccak256("block"),
nextConfigurationNumber: 1,
crossMessagesHash: keccak256("messages")
});

gwRouter.createBottomUpCheckpoint(checkpoint, membershipRoot, 10);
}
vm.stopPrank();

index = gwGetter.getBottomUpRetentionIndex();
require(index == 1, "retention index is 1");

uint256[] memory heights = gwGetter.getIncompleteCheckpointHeights();
require(heights.length == n, "heights.len == n");

vm.startPrank(FilAddress.SYSTEM_ACTOR);
gwRouter.pruneBottomUpCheckpoints(4);
vm.stopPrank();

index = gwGetter.getBottomUpRetentionIndex();
require(index == 4, "index updated");
heights = gwGetter.getIncompleteCheckpointHeights();
require(heights.length == n - 3, "index decreased");
}

function totalWeight(uint256[] memory weights) internal pure returns (uint256 sum) {
for (uint64 i = 0; i < 3; i++) {
sum += weights[i];
Expand Down