Update genesis validator lifecycle #778

cryptoAtwill · 2024-03-08T11:42:37Z

Fixing genesis validators not updated when unstake and leave are called.

Currently in unstake, when a validator unstakes all the collateral previuosly staked, then the validator address will not be removed from genesisValidators, see line. The same happens to leave.

Note that this PR will fail the CI only when #784 is merged into this branch.

aakoshh · 2024-03-12T11:27:58Z

contracts/src/lib/LibSubnetActor.sol

-    event SubnetBootstrapped(Validator[]);
+    event SubnetBootstrapped(address[]);


Why does this have to change?

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

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

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?

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)

aakoshh · 2024-03-12T11:31:41Z

contracts/src/lib/LibSubnetActor.sol

        s.bootstrapped = true;
-        emit SubnetBootstrapped(s.genesisValidators);
+        s.genesis.bootstrap(s.validatorSet);


Should bootstrapped be moved into this new SubnetGenesis struct?

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.

contracts/src/lib/LibSubnetActor.sol

aakoshh · 2024-03-12T11:48:17Z

contracts/src/subnet/SubnetActorGetterFacet.sol

+                // for genesis validators, totalCollateral == confirmedCollateral
+                collateral: info.totalCollateral,
+                federatedPower: info.federatedPower,


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?

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

aakoshh · 2024-03-12T11:53:15Z

contracts/src/lib/LibStaking.sol

@@ -611,15 +586,15 @@ library LibValidatorTracking {
        ParentValidatorsTracker storage self,
        uint16 activeValidatorsLimit,
        GenesisValidator[] memory validators
-    ) internal returns (Validator[] memory membership) {


Where was membership assigned before? 🤔

contracts/src/lib/LibStaking.sol

cryptoAtwill · 2024-03-11T10:34:22Z

contracts/src/lib/LibStaking.sol

-                    ++i;
-                }
-            }
-            if (!alreadyValidator) {


The issue here is that if there are subsequent changes to the validator, like stake, the weight is not updated.

Good catch!
Also 🤯

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

contracts/src/lib/LibStaking.sol

aakoshh · 2024-03-12T12:50:52Z

contracts/test/integration/SubnetActorDiamond.t.sol

+        // unstake everything else
+        saDiamond.manager().unstake(validator1Stake / 2);
+        require(saDiamond.getter().genesisValidators().length == 0, "should not be validator");
+    }


Great! I couldn't write any of this, but it helps to understand the flow 🙏

aakoshh

This looks good to me, apart from my general comments about federated_power itself.

raulk · 2024-03-15T10:41:50Z

contracts/src/lib/LibGenesis.sol

+        address[] memory validators = self.validators.values();
+
+        for (uint256 i = 0; i < validators.length; ) {


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

raulk · 2024-03-15T10:43:47Z

contracts/src/lib/LibStaking.sol

-                    ++i;
-                }
-            }
-            if (!alreadyValidator) {


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

raulk · 2024-03-15T10:51:25Z

contracts/src/lib/LibSubnetActor.sol

-    event SubnetBootstrapped(Validator[]);
+    event SubnetBootstrapped(address[]);


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)

raulk · 2024-03-15T10:53:45Z

contracts/src/structs/Subnet.sol

+    /// TODO: migrating all the genesis related fields to this struct so that one can handle
+    ///       them all in a library.


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

raulk · 2024-03-15T10:54:01Z

contracts/src/structs/Subnet.sol

@@ -153,6 +154,17 @@ struct GenesisValidator {
    bytes metadata;
 }

+/// @notice Maintains the genesis information related to a particular subnet
+struct SubnetGenesis {


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

raulk · 2024-03-15T10:57:42Z

contracts/test/integration/SubnetActorDiamond.t.sol

@@ -90,6 +90,79 @@ contract SubnetActorDiamondTest is Test, IntegrationTestBase {
        );
    }

+    /// @notice Testing the genesis validator's collateral
+    function testSubnetActorDiamond_GenesisValidators() public {


Great work writing these tests!

raulk · 2024-03-15T20:39:09Z

Going to merge this because it's way easier to reason about the complete changeset all at once in the base branch.

cryptoAtwill added 2 commits March 8, 2024 14:41

update genesis validator lifecycle

d89c63e

lint

11eec56

cryptoAtwill requested review from raulk and aakoshh March 8, 2024 15:19

cryptoAtwill added 3 commits March 11, 2024 10:45

handle genesis in one place

97799c4

update membership

27db9bf

fix bug

a40e47b

cryptoAtwill mentioned this pull request Mar 11, 2024

[HOLD] Fix child subnet gateway lib staking not initialized #776

Closed

2 tasks

more tests

9923448

aakoshh reviewed Mar 12, 2024

View reviewed changes

contracts/src/lib/LibSubnetActor.sol Show resolved Hide resolved

aakoshh reviewed Mar 12, 2024

View reviewed changes

contracts/src/lib/LibStaking.sol Show resolved Hide resolved

cryptoAtwill commented Mar 12, 2024

View reviewed changes

aakoshh reviewed Mar 12, 2024

View reviewed changes

aakoshh approved these changes Mar 12, 2024

View reviewed changes

raulk approved these changes Mar 15, 2024

View reviewed changes

raulk merged commit d29b922 into init-staking Mar 15, 2024
14 of 22 checks passed

raulk deleted the init-staking-genesis branch March 15, 2024 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update genesis validator lifecycle #778

Update genesis validator lifecycle #778

cryptoAtwill commented Mar 8, 2024 •

edited

Loading

aakoshh Mar 12, 2024

aakoshh Mar 12, 2024

cryptoAtwill Mar 12, 2024

aakoshh Mar 12, 2024

raulk Mar 15, 2024

aakoshh Mar 12, 2024

cryptoAtwill Mar 12, 2024

aakoshh Mar 12, 2024

cryptoAtwill Mar 12, 2024

aakoshh Mar 12, 2024

cryptoAtwill Mar 11, 2024

aakoshh Mar 12, 2024

raulk Mar 15, 2024

aakoshh Mar 12, 2024

aakoshh left a comment

raulk Mar 15, 2024

raulk Mar 15, 2024

raulk Mar 15, 2024

raulk Mar 15, 2024

raulk Mar 15, 2024

raulk Mar 15, 2024

raulk commented Mar 15, 2024

		event SubnetBootstrapped(Validator[]);
		event SubnetBootstrapped(address[]);

		address[] memory validators = self.validators.values();

		for (uint256 i = 0; i < validators.length; ) {

		/// TODO: migrating all the genesis related fields to this struct so that one can handle
		/// them all in a library.

Update genesis validator lifecycle #778

Update genesis validator lifecycle #778

Conversation

cryptoAtwill commented Mar 8, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakoshh 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

raulk commented Mar 15, 2024

cryptoAtwill commented Mar 8, 2024 •

edited

Loading