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.

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

Update genesis validator lifecycle #778

Merged
merged 6 commits into from
Mar 15, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
8 changes: 7 additions & 1 deletion contracts/src/GatewayDiamond.sol
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,13 @@ contract GatewayDiamond {
// through the gateway constructor in the future.
s.maxMsgsPerBottomUpBatch = MAX_MSGS_PER_BATCH;

LibValidatorTracking.init(s.validatorsTracker, params.activeValidatorsLimit, params.genesisValidators);
Validator[] memory validators = LibValidatorTracking.init(
s.validatorsTracker,
params.activeValidatorsLimit,
params.genesisValidators
);
Membership memory initial = Membership({configurationNumber: 0, validators: validators});
LibGateway.updateMembership(initial);
}

function _fallback() internal {
Expand Down
57 changes: 57 additions & 0 deletions contracts/src/lib/LibGenesis.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// SPDX-License-Identifier: MIT OR Apache-2.0
pragma solidity ^0.8.23;

import {GenesisValidator, SubnetGenesis, ValidatorInfo, ValidatorSet} from "../structs/Subnet.sol";
import {EnumerableSet} from "openzeppelin-contracts/utils/structs/EnumerableSet.sol";

/// @notice Provides convinient util methods to handle genesis states of the subnet
library LibGenesis {
using EnumerableSet for EnumerableSet.AddressSet;

/// @notice Dumps the validator's genesis information
function getValidatorInfo(SubnetGenesis storage self, address validator) internal view returns(GenesisValidator memory info){
info = self.validatorInfo[validator];
}

function addValidator(SubnetGenesis storage self, address validator) internal {
self.validators.add(validator);
}

function removeValidator(SubnetGenesis storage self, address validator) internal {
self.validators.remove(validator);
}

/// @notice Handles the genesis state when the subnet is bootstrapped. From this point onwards,
/// no genesis state of the subnet can be changed.
/// @param validatorInfo The validator staking information from LibStaking
function bootstrap(SubnetGenesis storage self, ValidatorSet storage validatorInfo) internal {
finalizeValidatorInfo(self, validatorInfo);
}

// ============ Interal functions ==============

/// @notice Finalizes the genesis validator information as the subnet is bootstrapped. After
/// this point, the genesis validator info can no longer be changed.
/// @param validatorInfo The validator staking information from LibStaking
function finalizeValidatorInfo(SubnetGenesis storage self, ValidatorSet storage validatorInfo) internal {
address[] memory validators = self.validators.values();

for (uint256 i = 0; i < validators.length; ) {
Comment on lines +37 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we harden this by asserting that validatorInfo contains exactly the validators we expect (and hold in storage)?

address addr = validators[i];

ValidatorInfo memory info = validatorInfo.validators[addr];
GenesisValidator memory genesis = GenesisValidator({
collateral: info.totalCollateral,
federatedPower: info.federatedPower,
addr: addr,
metadata: info.metadata
});

self.validatorInfo[addr] = genesis;

unchecked {
i++;
}
}
}
}
33 changes: 4 additions & 29 deletions contracts/src/lib/LibStaking.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {LibStakingChangeLog} from "./LibStakingChangeLog.sol";
import {PermissionMode, StakingReleaseQueue, StakingChangeLog, StakingChange, StakingChangeRequest, StakingOperation, StakingRelease, ValidatorSet, AddressStakingReleases, ParentValidatorsTracker, GenesisValidator, Validator} from "../structs/Subnet.sol";
import {WithdrawExceedingCollateral, NotValidator, CannotConfirmFutureChanges, NoCollateralToWithdraw, AddressShouldBeValidator, InvalidConfigurationNumber} from "../errors/IPCErrors.sol";
import {Address} from "openzeppelin-contracts/utils/Address.sol";
import {EnumerableSet} from "openzeppelin-contracts/utils/structs/EnumerableSet.sol";

library LibAddressStakingReleases {
/// @notice Add new release to the storage. Caller makes sure the release.releasedAt is ordered
Expand Down Expand Up @@ -173,7 +174,6 @@ library LibValidatorSet {
collateral += getTotalConfirmedCollateral(validators);
}


/// @notice Get the total power of the validators.
/// The function reverts if at least one validator is not in the active validator set.
function getTotalPowerOfValidators(
Expand Down Expand Up @@ -377,6 +377,7 @@ library LibStaking {
using LibMaxPQ for MaxPQ;
using LibMinPQ for MinPQ;
using Address for address payable;
using EnumerableSet for EnumerableSet.AddressSet;

uint64 internal constant INITIAL_CONFIGURATION_NUMBER = 1;

Expand Down Expand Up @@ -469,31 +470,6 @@ library LibStaking {
s.validatorSet.recordDeposit(validator, amount);
// confirm deposit that updates the confirmed collateral
s.validatorSet.confirmDeposit(validator, amount);

if (!s.bootstrapped) {
// add to initial validators avoiding duplicates if it
// is a genesis validator.
bool alreadyValidator;
uint256 length = s.genesisValidators.length;
for (uint256 i; i < length; ) {
if (s.genesisValidators[i].addr == validator) {
alreadyValidator = true;
break;
}
unchecked {
++i;
}
}
if (!alreadyValidator) {
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 issue here is that if there are subsequent changes to the validator, like stake, the weight is not updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!
Also 🤯

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a authoritative state diagram showing the lifecycle stages of a subnet, all transition edges, and all possible actions within each stage?

uint256 collateral = s.validatorSet.validators[validator].confirmedCollateral;
Validator memory val = Validator({
addr: validator,
weight: collateral,
metadata: s.validatorSet.validators[validator].metadata
});
s.genesisValidators.push(val);
}
}
}

/// @notice Confirm the withdraw directly without going through the confirmation process
Expand Down Expand Up @@ -541,7 +517,6 @@ library LibStaking {
}

// =============== Other functions ================

/// @notice Claim the released collateral
function claimCollateral(address validator) internal {
SubnetActorStorage storage s = LibSubnetActorStorage.appStorage();
Expand Down Expand Up @@ -611,15 +586,15 @@ library LibValidatorTracking {
ParentValidatorsTracker storage self,
uint16 activeValidatorsLimit,
GenesisValidator[] memory validators
) internal returns (Validator[] memory membership) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where was membership assigned before? 🤔

) internal returns (Validator[] memory) {
self.validators.activeLimit = activeValidatorsLimit;
// Start the next configuration number from 1, 0 is reserved for no change and the genesis membership
self.changes.nextConfigurationNumber = LibStaking.INITIAL_CONFIGURATION_NUMBER;
// The startConfiguration number is also 1 to match with nextConfigurationNumber, indicating we have
// empty validator change logs
self.changes.startConfigurationNumber = LibStaking.INITIAL_CONFIGURATION_NUMBER;

initValidators(self, validators);
return initValidators(self, validators);
}

function initValidators(
Expand Down
15 changes: 9 additions & 6 deletions contracts/src/lib/LibSubnetActor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,18 @@ import {VALIDATOR_SECP256K1_PUBLIC_KEY_LENGTH} from "../constants/Constants.sol"
import {ERR_PERMISSIONED_AND_BOOTSTRAPPED} from "../errors/IPCErrors.sol";
import {NotEnoughGenesisValidators, DuplicatedGenesisValidator, NotOwnerOfPublicKey, MethodNotAllowed} from "../errors/IPCErrors.sol";
import {IGateway} from "../interfaces/IGateway.sol";
import {Validator, ValidatorSet, PermissionMode} from "../structs/Subnet.sol";
import {Validator, ValidatorSet, PermissionMode, SubnetGenesis} from "../structs/Subnet.sol";
import {SubnetActorModifiers} from "../lib/LibSubnetActorStorage.sol";
import {LibValidatorSet, LibStaking} from "../lib/LibStaking.sol";
import {LibGenesis} from "../lib/LibGenesis.sol";
import {EnumerableSet} from "openzeppelin-contracts/utils/structs/EnumerableSet.sol";
import {LibSubnetActorStorage, SubnetActorStorage} from "./LibSubnetActorStorage.sol";

library LibSubnetActor {
using EnumerableSet for EnumerableSet.AddressSet;
using LibGenesis for SubnetGenesis;

event SubnetBootstrapped(Validator[]);
event SubnetBootstrapped(address[]);
Comment on lines -17 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this have to change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the subscriber would be interested in the actual powers, wouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can call genesisValidators to query their information. Getter is free and omitting the addresses should be cheaper.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a contract designer, so I am possibly completely off the mark on this: I thought events should ideally be self-contained so you learn what you have to without the need for further queries, and if you do need a query, then I'm not sure why it should return even the list of addresses, why not just a signal to flag up the block where the query can be made?

Copy link
Contributor

Choose a reason for hiding this comment

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

This event payload seems somewhat arbitrary. I wouldn't dwell too much on it right now, we'll need to change it going forward anyway to serve practical observability purposes. There are many ways to skin a cat, but subjective best practices:

  • Keep events as small as they can be.
  • Indexed fields: only publish identification data that subscribers will be interested to filter by through equality match (as that's the only predicate supported in Eth JSON-RPCs), e.g. publish addresses, IDs, kinds, categories, classes, etc. but do not publish balances, amounts, etc. as indexed fields.
  • In the payload, only publish data that refers to the transaction itself, which would otherwise NOT be available in state (e.g. deltas, actioning address, etc.) Public state data can be fetched from the contract directly, so it's superfluous to publish here. (Draw parallels with event streaming principles)


/// @notice Ensures that the subnet is operating under Collateral-based permission mode.
/// @dev Reverts if the subnet is not in Collateral mode.
Expand Down Expand Up @@ -48,7 +50,8 @@ library LibSubnetActor {
if (totalCollateral >= s.minActivationCollateral) {
if (LibStaking.totalActiveValidators() >= s.minValidators) {
s.bootstrapped = true;
emit SubnetBootstrapped(s.genesisValidators);
s.genesis.bootstrap(s.validatorSet);
emit SubnetBootstrapped(s.genesis.validators.values());

// register adding the genesis circulating supply (if it exists)
IGateway(s.ipcGatewayAddr).register{value: totalCollateral + s.genesisCircSupply}(s.genesisCircSupply);
Expand Down Expand Up @@ -98,16 +101,16 @@ library LibSubnetActor {

LibStaking.setMetadataWithConfirm(validators[i], publicKeys[i]);
LibStaking.setFederatedPowerWithConfirm(validators[i], powers[i]);

s.genesisValidators.push(Validator({addr: validators[i], weight: powers[i], metadata: publicKeys[i]}));
s.genesis.addValidator(validators[i]);

unchecked {
++i;
}
}

s.bootstrapped = true;
emit SubnetBootstrapped(s.genesisValidators);
s.genesis.bootstrap(s.validatorSet);
Comment on lines 111 to +112
Copy link
Contributor

Choose a reason for hiding this comment

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

Should bootstrapped be moved into this new SubnetGenesis struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, actually I think a lot of the parameters can be moved to SubnetGenesis struct so that we have better encapsulation, now it's scattered around. If this struct is ok, then I will create more PRs to migrate the fields one by one.

emit SubnetBootstrapped(s.genesis.validators.values());

// register adding the genesis circulating supply (if it exists)
IGateway(s.ipcGatewayAddr).register{value: s.genesisCircSupply}(s.genesisCircSupply);
Expand Down
6 changes: 3 additions & 3 deletions contracts/src/lib/LibSubnetActorStorage.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pragma solidity ^0.8.23;
import {ConsensusType} from "../enums/ConsensusType.sol";
import {NotGateway, SubnetAlreadyKilled} from "../errors/IPCErrors.sol";
import {BottomUpCheckpoint, BottomUpMsgBatchInfo} from "../structs/CrossNet.sol";
import {SubnetID, ValidatorSet, StakingChangeLog, StakingReleaseQueue, SupplySource, Validator, PermissionMode} from "../structs/Subnet.sol";
import {SubnetID, ValidatorSet, StakingChangeLog, StakingReleaseQueue, SupplySource, Validator, SubnetGenesis, PermissionMode} from "../structs/Subnet.sol";
import {EnumerableSet} from "openzeppelin-contracts/utils/structs/EnumerableSet.sol";

struct SubnetActorStorage {
Expand Down Expand Up @@ -55,8 +55,8 @@ import {EnumerableSet} from "openzeppelin-contracts/utils/structs/EnumerableSet.
EnumerableSet.AddressSet bootstrapOwners;
/// @notice contains all committed bottom-up checkpoint at specific epoch
mapping(uint256 => BottomUpCheckpoint) committedCheckpoints;
/// @notice initial set of validators joining in genesis
Validator[] genesisValidators;
/// @notice Tracks the subnet genesis state
SubnetGenesis genesis;
/// @notice genesis balance to be allocated to the subnet in genesis.
mapping(address => uint256) genesisBalance;
/// @notice genesis balance addresses
Expand Down
12 changes: 12 additions & 0 deletions contracts/src/structs/Subnet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pragma solidity ^0.8.23;
import {FvmAddress} from "./FvmAddress.sol";
import {MaxPQ} from "../lib/priority/LibMaxPQ.sol";
import {MinPQ} from "../lib/priority/LibMinPQ.sol";
import {EnumerableSet} from "openzeppelin-contracts/utils/structs/EnumerableSet.sol";

/// @notice A subnet identity type.
struct SubnetID {
Expand Down Expand Up @@ -153,6 +154,17 @@ struct GenesisValidator {
bytes metadata;
}

/// @notice Maintains the genesis information related to a particular subnet
struct SubnetGenesis {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work -- this type makes a ton of sense.

/// The genesis validators
EnumerableSet.AddressSet validators;
/// Tracks the validator info. This is only populated when the subnet is bootstrapped
mapping(address => GenesisValidator) validatorInfo;

/// TODO: migrating all the genesis related fields to this struct so that one can handle
/// them all in a library.
Comment on lines +164 to +165
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to do this now, to minimise the migration logic we'll need to write to upgrade early adopters?

}

/// @notice Validator struct stored in the gateway.
struct Validator {
uint256 weight;
Expand Down
40 changes: 36 additions & 4 deletions contracts/src/subnet/SubnetActorGetterFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pragma solidity ^0.8.23;
import {ConsensusType} from "../enums/ConsensusType.sol";
import {BottomUpCheckpoint, IpcEnvelope} from "../structs/CrossNet.sol";
import {SubnetID, SupplySource} from "../structs/Subnet.sol";
import {SubnetID, ValidatorInfo, Validator, PermissionMode} from "../structs/Subnet.sol";
import {SubnetID, ValidatorInfo, Validator, GenesisValidator, PermissionMode} from "../structs/Subnet.sol";
import {SubnetActorStorage} from "../lib/LibSubnetActorStorage.sol";
import {SubnetIDHelper} from "../lib/SubnetIDHelper.sol";
import {Address} from "openzeppelin-contracts/utils/Address.sol";
Expand Down Expand Up @@ -55,8 +55,40 @@ contract SubnetActorGetterFacet {
}

/// @notice Returns the initial set of validators of the genesis block.
function genesisValidators() external view returns (Validator[] memory) {
return s.genesisValidators;
function genesisValidators() external view returns (GenesisValidator[] memory validators) {
uint256 total = s.genesis.validators.length();

validators = new GenesisValidator[](total);

if (s.bootstrapped) {
// subnet boostrapped, that means validator information at genesis should be locked
// and cannot be changed anymore, fetch from s.genesis

for (uint256 i = 0; i < total; ) {
address addr = s.genesis.validators.at(i);
validators[i] = s.genesis.validatorInfo[addr];

unchecked {
i++;
}
}
return validators;
}

for (uint256 i = 0; i < total; ) {
address addr = s.genesis.validators.at(i);
ValidatorInfo memory info = getValidator(addr);
validators[i] = GenesisValidator({
// for genesis validators, totalCollateral == confirmedCollateral
collateral: info.totalCollateral,
federatedPower: info.federatedPower,
Comment on lines +82 to +84
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be 0? I can't see where a validator is removed from SubnetGenesis, but I assume that they can leave before genesis and remove their collateral?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, when they unstake or leave with zero collateral, they are removed from genesis validator.

addr: addr,
metadata: info.metadata
});
unchecked {
i++;
}
}
}

// @notice Provides the circulating supply of the genesis block.
Expand Down Expand Up @@ -114,7 +146,7 @@ contract SubnetActorGetterFacet {

/// @notice Returns detailed information about a specific validator.
/// @param validatorAddress The address of the validator to query information for.
function getValidator(address validatorAddress) external view returns (ValidatorInfo memory validator) {
function getValidator(address validatorAddress) public view returns (ValidatorInfo memory validator) {
validator = s.validatorSet.validators[validatorAddress];
}

Expand Down
13 changes: 11 additions & 2 deletions contracts/src/subnet/SubnetActorManagerFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ import {VALIDATOR_SECP256K1_PUBLIC_KEY_LENGTH} from "../constants/Constants.sol"
import {ERR_VALIDATOR_JOINED, ERR_VALIDATOR_NOT_JOINED} from "../errors/IPCErrors.sol";
import {InvalidFederationPayload, SubnetAlreadyBootstrapped, NotEnoughFunds, CollateralIsZero, CannotReleaseZero, NotOwnerOfPublicKey, EmptyAddress, NotEnoughBalance, NotEnoughCollateral, NotValidator, NotAllValidatorsHaveLeft, InvalidPublicKeyLength, MethodNotAllowed, SubnetNotBootstrapped} from "../errors/IPCErrors.sol";
import {IGateway} from "../interfaces/IGateway.sol";
import {Validator, ValidatorSet} from "../structs/Subnet.sol";
import {Validator, ValidatorSet, SubnetGenesis} from "../structs/Subnet.sol";
import {LibDiamond} from "../lib/LibDiamond.sol";
import {ReentrancyGuard} from "../lib/LibReentrancyGuard.sol";
import {SubnetActorModifiers} from "../lib/LibSubnetActorStorage.sol";
import {LibValidatorSet, LibStaking} from "../lib/LibStaking.sol";
import {LibGenesis} from "../lib/LibGenesis.sol";
import {EnumerableSet} from "openzeppelin-contracts/utils/structs/EnumerableSet.sol";
import {Address} from "openzeppelin-contracts/utils/Address.sol";
import {LibSubnetActor} from "../lib/LibSubnetActor.sol";
Expand All @@ -18,6 +19,7 @@ import {Pausable} from "../lib/LibPausable.sol";
contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausable {
using EnumerableSet for EnumerableSet.AddressSet;
using LibValidatorSet for ValidatorSet;
using LibGenesis for SubnetGenesis;
using Address for address payable;

/// @notice method to add some initial balance into a subnet that hasn't yet bootstrapped.
Expand Down Expand Up @@ -142,6 +144,7 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
// confirm validators deposit immediately
LibStaking.setMetadataWithConfirm(msg.sender, publicKey);
LibStaking.depositWithConfirm(msg.sender, msg.value);
s.genesis.addValidator(msg.sender);

LibSubnetActor.bootstrapSubnetIfNeeded();
} else {
Expand Down Expand Up @@ -169,7 +172,6 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa

if (!s.bootstrapped) {
LibStaking.depositWithConfirm(msg.sender, msg.value);

LibSubnetActor.bootstrapSubnetIfNeeded();
} else {
LibStaking.deposit(msg.sender, msg.value);
Expand Down Expand Up @@ -198,6 +200,12 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
}
if (!s.bootstrapped) {
LibStaking.withdrawWithConfirm(msg.sender, amount);

uint256 total = LibStaking.totalValidatorCollateral(msg.sender);
if (total == 0) {
s.genesis.removeValidator(msg.sender);
}

return;
}

Expand Down Expand Up @@ -237,6 +245,7 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa

// interaction must be performed after checks and changes
LibStaking.withdrawWithConfirm(msg.sender, amount);
s.genesis.removeValidator(msg.sender);
return;
}
LibStaking.withdraw(msg.sender, amount);
Expand Down
Loading