feat(contracts): validator gating #1127

cryptoAtwill · 2024-08-29T09:04:03Z

This is a simple implementation of validator gating as outlined in #1121.

The changes are:

Added an interface to the ValidatorGater interface.
Initial a simple implementation of validator gating contract that only allows exact power change.
Update federated and collateral based power update with gating
Unit tests + integration test cases
Fixed a minor selector update error

raulk

This looks directionally good to me! TODO:

Solidity integration tests.
IPC subnet creation / genesis tooling update.
Docs.

raulk · 2024-08-29T10:54:56Z

contracts/contracts/subnet/SubnetValidatorGater.sol

+        if (msg.sender != caller) {
+            revert NotAuthorized(msg.sender);
+        }


We should be having a helper method to verify that a given caller matches the subnet actor given a subnet ID.

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

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

raulk · 2024-08-29T10:56:31Z

contracts/contracts/subnet/SubnetValidatorGater.sol

I think this contract should be in an examples directory.

raulk · 2024-08-29T10:58:30Z

contracts/contracts/subnet/SubnetValidatorGater.sol

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


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).

Yeah, I think the current one might be too strict.

contracts/contracts/lib/LibSubnetActor.sol

raulk · 2024-08-29T10:59:32Z

contracts/contracts/lib/LibSubnetActor.sol

@@ -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 {


Do we absolutely need these two variants?

I have updated slightly to accommodate different usages.

Co-authored-by: raulk <raul@protocol.ai>

…lidator-gating

cryptoAtwill · 2024-08-30T09:12:48Z

contracts/contracts/subnet/SubnetActorManagerFacet.sol

@@ -67,6 +69,11 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
        payable(msg.sender).sendValue(amount);
    }

+    function setValidatorGater(address gater) external notKilled {


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.

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

This approach leaves open an opportunity window between subnet construction and setValidatorGater where any validator could join, as the subnet is effectively unguarded.

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.

@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.

raulk

Just a few comments. Other than that, it looks fine! We're missing the ipc-cli subnet create update, right?

raulk · 2024-08-30T12:06:27Z

contracts/contracts/errors/IPCErrors.sol

@@ -79,6 +79,7 @@ error MethodNotAllowed(string reason);
 error InvalidFederationPayload();
 error DuplicatedGenesisValidator();
 error NotEnoughGenesisValidators();
+error PowerChangeRequestNotApproved();


Suggested change

error PowerChangeRequestNotApproved();

error ValidatorPowerChangeDenied();

raulk · 2024-08-30T12:12:35Z

contracts/contracts/subnet/SubnetActorManagerFacet.sol

@@ -67,6 +69,11 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
        payable(msg.sender).sendValue(amount);
    }

+    function setValidatorGater(address gater) external notKilled {


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

This approach leaves open an opportunity window between subnet construction and setValidatorGater where any validator could join, as the subnet is effectively unguarded.

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.

raulk · 2024-08-30T12:16:27Z

contracts/contracts/subnet/SubnetActorManagerFacet.sol

+        uint256 collateral = LibStaking.totalValidatorCollateral(msg.sender);
+        LibSubnetActor.gateValidatorPowerDelta(msg.sender, collateral, collateral + msg.value);


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?

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.

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.

raulk · 2024-08-30T12:16:56Z

contracts/scripts/python/build_selector_library.py

-         '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',


raulk · 2024-08-30T12:17:09Z

contracts/scripts/python/build_selector_library.py

+         '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',


Thanks for catching this!

raulk

Just a few comments. Other than that, it looks fine! We're missing the ipc-cli subnet create update, right?

…/ipc into validator-gating

karlem · 2024-09-18T14:31:55Z

contracts/contracts/subnet/SubnetActorManagerFacet.sol

@@ -67,6 +69,11 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
        payable(msg.sender).sendValue(amount);
    }

+    function setValidatorGater(address gater) external notKilled {


@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.

karlem · 2024-09-18T15:12:35Z

contracts/contracts/examples/SubnetValidatorGater.sol

+        subnet = id;
+    }
+
+    function isAllow(address validator, uint256 power) public view returns (bool) {


Nit

Suggested change

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

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

karlem · 2024-09-18T16:01:58Z

contracts/contracts/subnet/SubnetActorManagerFacet.sol

+        uint256 collateral = LibStaking.totalValidatorCollateral(msg.sender);
+        LibSubnetActor.gateValidatorPowerDelta(msg.sender, collateral, collateral + msg.value);


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.

initial gating

49555f7

cryptoAtwill requested a review from a team as a code owner August 29, 2024 09:04

cryptoAtwill marked this pull request as draft August 29, 2024 09:04

cryptoAtwill changed the title ~~feat(contract): validator gating~~ feat(contracts): validator gating Aug 29, 2024

collateral validator gating

5697ee4

raulk reviewed Aug 29, 2024

View reviewed changes

cryptoAtwill and others added 5 commits August 29, 2024 20:05

Update contracts/contracts/lib/LibSubnetActor.sol

2e7761c

Co-authored-by: raulk <raul@protocol.ai>

Update contracts/contracts/lib/LibSubnetActor.sol

43ee469

Co-authored-by: raulk <raul@protocol.ai>

review feedbacks

d961819

Merge branch 'main' of protocol-github:consensus-shipyard/ipc into va…

05d1095

…lidator-gating

adding tests

9f00973

cryptoAtwill marked this pull request as ready for review August 30, 2024 09:02

cryptoAtwill requested a review from raulk August 30, 2024 09:02

cryptoAtwill commented Aug 30, 2024

View reviewed changes

raulk reviewed Aug 30, 2024

View reviewed changes

cryptoAtwill added 2 commits September 2, 2024 16:08

update gater interface

7102a78

update ipc

92a1dc4

raulk linked an issue Sep 2, 2024 that may be closed by this pull request

[Staking] Validator gating #1121

Closed

cryptoAtwill and others added 5 commits September 2, 2024 19:14

Merge branch 'main' into validator-gating

2cec1a7

update test

374a7df

Merge branch 'validator-gating' of protocol-github:consensus-shipyard…

6057dd8

…/ipc into validator-gating

adding deployments and sample cli

e89c069

lint

4c6000c

karlem approved these changes Sep 18, 2024

View reviewed changes

karlem added 2 commits September 18, 2024 19:15

feat: add simple docs

3dcacd0

fix: typo

9388b24

karlem merged commit 8403e47 into main Sep 18, 2024
14 checks passed

karlem deleted the validator-gating branch September 18, 2024 20:33

raulk restored the validator-gating branch September 19, 2024 15:36

raulk deleted the validator-gating branch September 19, 2024 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(contracts): validator gating #1127

feat(contracts): validator gating #1127

cryptoAtwill commented Aug 29, 2024 •

edited

Loading

raulk left a comment •

edited by cryptoAtwill

Loading

raulk Aug 29, 2024

raulk Aug 29, 2024

cryptoAtwill Aug 29, 2024

raulk Aug 29, 2024

raulk Aug 29, 2024

cryptoAtwill Aug 29, 2024

raulk Aug 29, 2024

cryptoAtwill Aug 29, 2024 •

edited

Loading

cryptoAtwill Aug 30, 2024

raulk Aug 30, 2024

karlem Sep 18, 2024

raulk left a comment

raulk Aug 30, 2024

raulk Aug 30, 2024

raulk Aug 30, 2024

cryptoAtwill Sep 2, 2024

karlem Sep 18, 2024

raulk Aug 30, 2024

raulk Aug 30, 2024

raulk left a comment

karlem Sep 18, 2024

karlem Sep 18, 2024

karlem Sep 18, 2024

	error PowerChangeRequestNotApproved();
	error ValidatorPowerChangeDenied();

		uint256 collateral = LibStaking.totalValidatorCollateral(msg.sender);
		LibSubnetActor.gateValidatorPowerDelta(msg.sender, collateral, collateral + msg.value);

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

feat(contracts): validator gating #1127

feat(contracts): validator gating #1127

Conversation

cryptoAtwill commented Aug 29, 2024 • edited Loading

raulk left a comment • edited by cryptoAtwill Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptoAtwill Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptoAtwill commented Aug 29, 2024 •

edited

Loading

raulk left a comment •

edited by cryptoAtwill

Loading

cryptoAtwill Aug 29, 2024 •

edited

Loading