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

feat(contracts): validator gating #1127

Merged
merged 16 commits into from
Sep 18, 2024
374 changes: 187 additions & 187 deletions contracts/.storage-layouts/GatewayActorModifiers.json

Large diffs are not rendered by default.

374 changes: 187 additions & 187 deletions contracts/.storage-layouts/GatewayDiamond.json

Large diffs are not rendered by default.

302 changes: 155 additions & 147 deletions contracts/.storage-layouts/SubnetActorDiamond.json

Large diffs are not rendered by default.

302 changes: 155 additions & 147 deletions contracts/.storage-layouts/SubnetActorModifiers.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions contracts/contracts/errors/IPCErrors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ error MethodNotAllowed(string reason);
error InvalidFederationPayload();
error DuplicatedGenesisValidator();
error NotEnoughGenesisValidators();
error PowerChangeRequestNotApproved();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
error PowerChangeRequestNotApproved();
error ValidatorPowerChangeDenied();


enum InvalidXnetMessageReason {
Sender,
Expand Down
14 changes: 14 additions & 0 deletions contracts/contracts/interfaces/IValidatorGater.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// SPDX-License-Identifier: MIT OR Apache-2.0
pragma solidity ^0.8.23;

import {SubnetID} from "../structs/Subnet.sol";

/// @title Validator Gater interface
/// This interface introduces the ability to intercept validator power updates before it's executed. Power updates could
/// come from staking, unstaking, and explicit validator membership adjustments (federated membership). With this interface,
/// it introduces an extra layer of checking to directly allow or deny the action, according to a user-defined policy.
interface IValidatorGater {
/// This intercepts the power update call.
/// @notice This method should revert if the power update is not allowed.
function interceptPowerDelta(SubnetID memory id, address validator, uint256 prevPower, uint256 newPower) external;
}
51 changes: 50 additions & 1 deletion contracts/contracts/lib/LibSubnetActor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,17 @@ 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 {IValidatorGater} from "../interfaces/IValidatorGater.sol";
import {Validator, ValidatorSet, PermissionMode, SubnetID} from "../structs/Subnet.sol";
import {SubnetActorModifiers} from "../lib/LibSubnetActorStorage.sol";
import {LibValidatorSet, LibStaking} from "../lib/LibStaking.sol";
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
import {LibSubnetActorStorage, SubnetActorStorage} from "./LibSubnetActorStorage.sol";
import {SubnetIDHelper} from "../lib/SubnetIDHelper.sol";

library LibSubnetActor {
using EnumerableSet for EnumerableSet.AddressSet;
using SubnetIDHelper for SubnetID;

event SubnetBootstrapped(Validator[]);

Expand All @@ -38,6 +41,39 @@ library LibSubnetActor {
return;
}

/// @notice Performs validator gating, i.e. checks if the validator power update is actually allowed.
function validatorGating(address validator, uint256 powerDelta, bool isIncrease) internal {
cryptoAtwill marked this conversation as resolved.
Show resolved Hide resolved
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 absolutely need these two variants?

Copy link
Contributor Author

@cryptoAtwill cryptoAtwill Aug 29, 2024

Choose a reason for hiding this comment

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

I have updated slightly to accommodate different usages.

SubnetActorStorage storage s = LibSubnetActorStorage.appStorage();

// zero address means no gating needed
if (s.validatorGater == address(0)) {
return;
}

uint256 oldPower = LibStaking.getPower(validator);
uint256 newPower = 0;
if (isIncrease) {
newPower = oldPower + powerDelta;
} else {
newPower = oldPower - powerDelta;
}

SubnetID memory id = s.parentId.createSubnetId(address(this));
IValidatorGater(s.validatorGater).interceptPowerDelta(id, validator, oldPower, newPower);
}

/// @notice Performs validator gating, i.e. checks if the validator power update is actually allowed.
function validatorGating(SubnetID memory id, address validator, uint256 prevPower, uint256 newPower) internal {
cryptoAtwill marked this conversation as resolved.
Show resolved Hide resolved
SubnetActorStorage storage s = LibSubnetActorStorage.appStorage();

// zero address means no gating needed
if (s.validatorGater == address(0)) {
return;
}

IValidatorGater(s.validatorGater).interceptPowerDelta(id, validator, prevPower, newPower);
}

/// @dev This function is used to bootstrap the subnet,
/// if its total collateral is greater than minimum activation collateral.
function bootstrapSubnetIfNeeded() internal {
Expand Down Expand Up @@ -76,6 +112,7 @@ library LibSubnetActor {
uint256[] calldata powers
) internal {
SubnetActorStorage storage s = LibSubnetActorStorage.appStorage();
SubnetID memory subnet = s.parentId.createSubnetId(address(this));

uint256 length = validators.length;

Expand All @@ -96,6 +133,7 @@ library LibSubnetActor {
revert DuplicatedGenesisValidator();
}

LibSubnetActor.validatorGating(subnet, validators[i], 0, powers[i]);
LibStaking.setMetadataWithConfirm(validators[i], publicKeys[i]);
LibStaking.setFederatedPowerWithConfirm(validators[i], powers[i]);

Expand Down Expand Up @@ -124,13 +162,24 @@ library LibSubnetActor {
uint256[] calldata powers
) internal {
uint256 length = validators.length;

SubnetActorStorage storage s = LibSubnetActorStorage.appStorage();
SubnetID memory subnet = s.parentId.createSubnetId(address(this));

for (uint256 i; i < length; ) {
// check addresses
address convertedAddress = publicKeyToAddress(publicKeys[i]);
if (convertedAddress != validators[i]) {
revert NotOwnerOfPublicKey();
}

LibSubnetActor.validatorGating(
subnet,
validators[i],
LibStaking.getPower(validators[i]),
powers[i]
);

// no need to do deduplication as set directly set the power, there wont be any addition of
// federated power.
LibStaking.setFederatedPower({validator: validators[i], metadata: publicKeys[i], amount: powers[i]});
Expand Down
2 changes: 2 additions & 0 deletions contracts/contracts/lib/LibSubnetActorStorage.sol
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet
mapping(address => uint256) genesisBalance;
/// @notice genesis balance addresses
address[] genesisBalanceKeys;
/// @notice The validator gater, if address(0), no validator gating is performed
address validatorGater;
}

library LibSubnetActorStorage {
Expand Down
11 changes: 10 additions & 1 deletion contracts/contracts/subnet/SubnetActorManagerFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ 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, SubnetID} from "../structs/Subnet.sol";
import {SubnetIDHelper} from "../lib/SubnetIDHelper.sol";
import {LibDiamond} from "../lib/LibDiamond.sol";
import {ReentrancyGuard} from "../lib/LibReentrancyGuard.sol";
import {SubnetActorModifiers} from "../lib/LibSubnetActorStorage.sol";
Expand All @@ -17,6 +18,7 @@ import {Pausable} from "../lib/LibPausable.sol";

contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausable {
using EnumerableSet for EnumerableSet.AddressSet;
using SubnetIDHelper for SubnetID;
using LibValidatorSet for ValidatorSet;
using Address for address payable;

Expand Down Expand Up @@ -134,6 +136,8 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
revert NotOwnerOfPublicKey();
}

LibSubnetActor.validatorGating(msg.sender, msg.value, true);

if (!s.bootstrapped) {
// if the subnet has not been bootstrapped, join directly
// without delays, and collect collateral to register
Expand Down Expand Up @@ -167,6 +171,8 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
revert MethodNotAllowed(ERR_VALIDATOR_NOT_JOINED);
}

LibSubnetActor.validatorGating(msg.sender, msg.value, true);

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

Expand Down Expand Up @@ -196,6 +202,9 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
if (collateral <= amount) {
revert NotEnoughCollateral();
}

LibSubnetActor.validatorGating(msg.sender, amount, false);

if (!s.bootstrapped) {
LibStaking.withdrawWithConfirm(msg.sender, amount);
return;
Expand Down
51 changes: 51 additions & 0 deletions contracts/contracts/subnet/SubnetValidatorGater.sol
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this contract should be in an examples directory.

Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// SPDX-License-Identifier: MIT OR Apache-2.0
pragma solidity ^0.8.23;

import {IValidatorGater} from "../interfaces/IValidatorGater.sol";
import {InvalidSubnet, NotAuthorized, PowerChangeRequestNotApproved} from "../errors/IPCErrors.sol";
import {SubnetID} from "../structs/Subnet.sol";
import {SubnetIDHelper} from "../lib/SubnetIDHelper.sol";
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";

/// This is a simple implementation of `IValidatorGater`. It makes sure the exact power change
/// request is approved. This is a very strict requirement.
contract SubnetValidatorGater is IValidatorGater, Ownable {
using SubnetIDHelper for SubnetID;

address public caller;
SubnetID public subnet;

mapping(bytes32 => bool) public allowed;

constructor(address _caller, SubnetID memory _subnet) Ownable(msg.sender) {
caller = _caller;
subnet = _subnet;
}

/// Only owner can approve the validator join request
function approve(address validator, uint256 prevPower, uint256 newPower) external onlyOwner {
allowed[genKey(validator, prevPower, newPower)] = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be generally more useful if in this standard implementation, you approve validators to hold a power between a min and a max, instead of explicitly approving every delta.

Note that in this implementation, the allowed map can only grow infinitely. With my proposal, it would be a static configuration. If you agree, make sure you handle deletions (e.g. when min and max is 0 for a validator).

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, I think the current one might be too strict.

}

function interceptPowerDelta(SubnetID memory id, address validator, uint256 prevPower, uint256 newPower) external override {
if (msg.sender != caller) {
revert NotAuthorized(msg.sender);
}
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 be having a helper method to verify that a given caller matches the subnet actor given a subnet ID.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would we need to store the caller if we had that method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, because the subnet actor is the only one that should be calling it.


if (id.equals(subnet)) {
revert InvalidSubnet();
}

bytes32 key = genKey(validator, prevPower, newPower);
if (!allowed[key]) {
revert PowerChangeRequestNotApproved();
}

// remove the approved request
delete allowed[key];
}

function genKey(address validator, uint256 prevPower, uint256 newPower) internal pure returns(bytes32) {
return keccak256(abi.encode(validator, prevPower, newPower));
}
}
Loading