Skip to content
Merged
Show file tree
Hide file tree
Changes from 14 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
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.

5 changes: 5 additions & 0 deletions contracts/contracts/SubnetActorDiamond.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ contract SubnetActorDiamond {
PermissionMode permissionMode;
SupplySource supplySource;
SubnetID parentId;
address validatorGater;
}

constructor(IDiamond.FacetCut[] memory _diamondCut, ConstructorParams memory params, address owner) {
Expand Down Expand Up @@ -95,6 +96,10 @@ contract SubnetActorDiamond {
s.changeSet.startConfigurationNumber = LibStaking.INITIAL_CONFIGURATION_NUMBER;
// Set the supply strategy.
s.supplySource = params.supplySource;

if (params.validatorGater != address(0)) {
s.validatorGater = params.validatorGater;
}
}

function _fallback() internal {
Expand Down
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 ValidatorPowerChangeDenied();

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

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

/// The power range that an approved validator can have.
struct PowerRange {
uint256 min;
uint256 max;
}

/// This is a simple implementation of `IValidatorGater`. It makes sure the exact power change
/// request is approved. This is a very strict requirement.
/// See sample cli usage in tasks/validator-gater.ts
contract SubnetValidatorGater is IValidatorGater, Ownable {
using SubnetIDHelper for SubnetID;

SubnetID public subnet;
mapping(address => PowerRange) public allowed;

constructor() Ownable(msg.sender) {}

function setSubnet(SubnetID calldata id) external onlyOwner {
subnet = id;
}

function isAllow(address validator, uint256 power) public view returns (bool) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit

Suggested change
function isAllow(address validator, uint256 power) public view returns (bool) {
function isAllowed(address validator, uint256 power) public view returns (bool) {

PowerRange memory range = allowed[validator];
return range.min <= power && power <= range.max;
}

/// Only owner can approve the validator join request
function approve(address validator, uint256 minPower, uint256 maxPower) external onlyOwner {
allowed[validator] = PowerRange({min: minPower, max: maxPower});
}

/// Revoke approved power range
function revoke(address validator) external onlyOwner {
delete allowed[validator];
}

function interceptPowerDelta(
SubnetID memory id,
address validator,
uint256 /*prevPower*/,
uint256 newPower
) external view override {
SubnetID memory targetSubnet = subnet;

if (!id.equals(targetSubnet)) {
revert InvalidSubnet();
}

if (msg.sender != targetSubnet.getAddress()) {
revert NotAuthorized(msg.sender);
}

if (!isAllow(validator, newPower)) {
revert ValidatorPowerChangeDenied();
}
}
}
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;
}
46 changes: 45 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,42 @@ library LibSubnetActor {
return;
}

/// @notice Performs validator gating, i.e. checks if the validator power update is actually allowed.
function gateValidatorPowerDelta(address validator, uint256 oldPower, uint256 newPower) internal {
SubnetActorStorage storage s = LibSubnetActorStorage.appStorage();

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

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 gateValidatorNewPowers(address[] calldata validators, uint256[] calldata newPowers) internal {
SubnetActorStorage storage s = LibSubnetActorStorage.appStorage();

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

SubnetID memory subnet = s.parentId.createSubnetId(address(this));
IValidatorGater gater = IValidatorGater(s.validatorGater);
uint256 length = validators.length;

for (uint256 i; i < length; ) {
uint256 oldPower = LibStaking.getPower(validators[i]);
gater.interceptPowerDelta(subnet, validators[i], oldPower, newPowers[i]);

unchecked {
i++;
}
}
}

/// @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 @@ -83,6 +122,8 @@ library LibSubnetActor {
revert NotEnoughGenesisValidators();
}

LibSubnetActor.gateValidatorNewPowers(validators, powers);

for (uint256 i; i < length; ) {
// check addresses
address convertedAddress = publicKeyToAddress(publicKeys[i]);
Expand Down Expand Up @@ -124,6 +165,9 @@ library LibSubnetActor {
uint256[] calldata powers
) internal {
uint256 length = validators.length;

LibSubnetActor.gateValidatorNewPowers(validators, powers);

for (uint256 i; i < length; ) {
// check addresses
address convertedAddress = publicKeyToAddress(publicKeys[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
19 changes: 18 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 @@ -67,6 +69,11 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
payable(msg.sender).sendValue(amount);
}

function setValidatorGater(address gater) external notKilled {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The gater needs to be updated this way as compared to in a constructor because the gater needs the subnet address, but the subnet is not created at the constructor. Alternatively, one can also create the gater first then set the subnet address in gater. But I figured this is less intrusive to downstream ipc rust code and in the redesign, we can update accordingly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the second approach you proposed leads to better security and encapsulation.

  1. This approach leaves open an opportunity window between subnet construction and setValidatorGater where any validator could join, as the subnet is effectively unguarded.
  2. Not all gater implementations will require whitelisting subnets, e.g. you could have a general purpose / utility gater that verifies if a validator is a Filecoin SP, an NFT holder, or something else. And any subnet who wants to reuse that logic could choose to rely on it.

So let's set the validator gater in the creation parameters, and let the gater manage the subnet whitelisting if it needs to.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@cryptoAtwill Do we still needs this? Since the IPC CLI only supports the constructor right? Also point no. 1 from Raul is very legit. I think we should just have users to have setSubnet method on their own gater implementations if they need to.

LibDiamond.enforceIsContractOwner();
s.validatorGater = gater;
}

/// @notice Sets the federated power of validators.
/// @dev method that allows the contract owner to set the validators' federated power.
/// @param validators The addresses of validators.
Expand Down Expand Up @@ -134,6 +141,8 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
revert NotOwnerOfPublicKey();
}

LibSubnetActor.gateValidatorPowerDelta(msg.sender, 0, msg.value);

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 +176,9 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
revert MethodNotAllowed(ERR_VALIDATOR_NOT_JOINED);
}

uint256 collateral = LibStaking.totalValidatorCollateral(msg.sender);
LibSubnetActor.gateValidatorPowerDelta(msg.sender, collateral, collateral + msg.value);
Comment on lines +179 to +180
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The problem that makes this impractical to use is that weights are relative. So I'm thinking it might be useful to pass the projected sum of total weights to the gater. That way the gater could reject takeover attempts (by having the old power, new power, and total network power). Granted, the gater could call back the subnet actor to get the current membership table and calculate the total, I think so? What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the idea works, but just from gas concern + responsitbility, I'm not sure if SubnetActor should enforce this.

I can achieve the same check with my own Gater implementation because I know the old and new value. In my custom gater implementation, I can hardcode a total power limit. Then with each validator update, I can calculate the new total power with oldTotalPower = oldTotalPower + validatorNewPower - validatorOldPower. Then I can do the comparison with the hardcoded limit.

I feel (I could be wrong) the current interface is general enough for first iteration, adding total network power might incur a gas cost to users who dont need it. And I can achieve that with my custom implementation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah here I think I stand with Will. Since the users can have any arbitrary logic in the gater, I would leave it to them. They can quite easily store the current total powers if they want to.


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

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

LibSubnetActor.gateValidatorPowerDelta(msg.sender, collateral, collateral - amount);

if (!s.bootstrapped) {
LibStaking.withdrawWithConfirm(msg.sender, amount);
return;
Expand All @@ -221,6 +236,8 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
revert NotValidator(msg.sender);
}

LibSubnetActor.gateValidatorPowerDelta(msg.sender, amount, 0);

// slither-disable-next-line unused-return
s.bootstrapOwners.remove(msg.sender);
delete s.bootstrapNodes[msg.sender];
Expand Down
38 changes: 19 additions & 19 deletions contracts/scripts/python/build_selector_library.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,25 +67,25 @@ def get_selectors(contract):
def main():
contract_selectors = {}
filepaths_to_target = [
'src/GatewayDiamond.sol',
'src/SubnetActorDiamond.sol',
'src/SubnetRegistryDiamond.sol',
'src/OwnershipFacet.sol',
'src/diamond/DiamondCutFacet.sol',
'src/diamond/DiamondLoupeFacet.sol',
'src/gateway/GatewayGetterFacet.sol',
'src/gateway/GatewayManagerFacet.sol',
'src/gateway/GatewayMessengerFacet.sol',
'src/gateway/router/CheckpointingFacet.sol',
'src/gateway/router/TopDownFinalityFacet.sol',
'src/gateway/router/XnetMessagingFacet.sol',
'src/subnet/SubnetActorGetterFacet.sol',
'src/subnet/SubnetActorManagerFacet.sol',
'src/subnet/SubnetActorPauseFacet.sol',
'src/subnet/SubnetActorRewardFacet.sol',
'src/subnet/SubnetActorCheckpointingFacet.sol',
'src/subnetregistry/RegisterSubnetFacet.sol',
'src/subnetregistry/SubnetGetterFacet.sol',
Comment on lines -70 to -88
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

'contracts/GatewayDiamond.sol',
'contracts/SubnetActorDiamond.sol',
'contracts/SubnetRegistryDiamond.sol',
'contracts/OwnershipFacet.sol',
'contracts/diamond/DiamondCutFacet.sol',
'contracts/diamond/DiamondLoupeFacet.sol',
'contracts/gateway/GatewayGetterFacet.sol',
'contracts/gateway/GatewayManagerFacet.sol',
'contracts/gateway/GatewayMessengerFacet.sol',
'contracts/gateway/router/CheckpointingFacet.sol',
'contracts/gateway/router/TopDownFinalityFacet.sol',
'contracts/gateway/router/XnetMessagingFacet.sol',
'contracts/subnet/SubnetActorGetterFacet.sol',
'contracts/subnet/SubnetActorManagerFacet.sol',
'contracts/subnet/SubnetActorPauseFacet.sol',
'contracts/subnet/SubnetActorRewardFacet.sol',
'contracts/subnet/SubnetActorCheckpointingFacet.sol',
'contracts/subnetregistry/RegisterSubnetFacet.sol',
'contracts/subnetregistry/SubnetGetterFacet.sol',
Comment on lines +70 to +88
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for catching this!

'test/helpers/ERC20PresetFixedSupply.sol',
'test/helpers/NumberContractFacetEight.sol',
'test/helpers/NumberContractFacetSeven.sol',
Expand Down
5 changes: 4 additions & 1 deletion contracts/tasks/deploy-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ task('deploy-registry')
name: 'SubnetActorGetterFacet',
libraries: ['SubnetIDHelper'],
},
{ name: 'SubnetActorManagerFacet' },
{
name: 'SubnetActorManagerFacet',
libraries: ['SubnetIDHelper'],
},
{ name: 'SubnetActorPauseFacet' },
{ name: 'SubnetActorRewardFacet' },
{ name: 'SubnetActorCheckpointingFacet' },
Expand Down
1 change: 1 addition & 0 deletions contracts/tasks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ import './deploy-gateway'
import './deploy-registry'
import './deploy'
import './upgrade'
import './validator-gater'
Loading