Update fendermint to match genesis changes in solidity #784

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

Update fendermint to align with the changes in solidity contracts. Some key changes are:

New type changes, i.e. adding GenesisValidator parameter that matches that in the contract.
The validator's power in fendermint is the sum of the validator's collateral + federated power from the parent. See LibStakingPower for references.
Update ipc get genesis from parent to use GenesisValidator.
Update genesis CLI commands to reflect changes on the contract.

This PR is compilable and integration tested, but it's pending updates in unit tests.

…yard/ipc into init-staking-fendermint

aakoshh · 2024-03-12T11:07:47Z

fendermint/vm/genesis/src/lib.rs

+    /// Convert from [LibStakingPower] to [Power] by specifying the number of significant
+    /// decimal places per FIL that grant 1 power.
+    ///
+    /// For example:
+    /// * with 3 decimal places, we get 1 power per milli FIL: 0.001 FIL => 1 power
+    /// * with 0 decimal places, we get 1 power per whole FIL: 1 FIL => 1 power
+    pub fn into_power(self: LibStakingPower, scale: PowerScale) -> Power {
+        let atto_per_power = Collateral::atto_per_power(scale);
+        let atto = self.federated_power.atto() + self.collateral.atto();
+        // Rounding away from zero, so with little collateral (e.g. in testing)
+        // we don't end up with everyone having 0 power and then being unable
+        // to produce a checkpoint because the threshold is 0.
+        let power = atto.div_ceil(&atto_per_power);
+        let power = power.min(BigInt::from(u64::MAX));
+        Power(power.try_into().expect("clipped to u64::MAX"))
+    }


There shouldn't be a need to copy-paste all this. The original is covered by tests, this isn't.

Since both federated_power and collateral are TokenAmount, you can just add the together, wrap the result in Collateral and call into_power on it: Collateral(self.federated_power + self.collateral).into_power(scale).

fendermint/vm/interpreter/src/fvm/state/ipc.rs

aakoshh · 2024-03-12T11:10:04Z

ipc/api/src/validator.rs

+//         .collect();
+//
+//     result
+// }


Why do we need this commented code?

will remove them

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

ipc/provider/src/manager/evm/manager.rs

@@ -14,7 +14,7 @@ use ipc_actors_abis::{
    subnet_actor_reward_facet,
 };
 use ipc_api::evm::{fil_to_eth_amount, payload_to_evm_address, subnet_id_to_evm_addresses};
-use ipc_api::validator::from_contract_validators;
+use ipc_api::validator::vec_try_from;


Since this is a public and completely generic function, just a convenience for some library builtins, it would be worth moving it into a utility module. It's strange to see it being imported from validator when it has nothing validator specific about it.

I don't even see it used anywhere else, so maybe just move it into this module as a private function.

aakoshh

I can see Maciej commenting here that it was never a requirement to support collateral and federation-assigned power at the same time. Your response, as I understand, is that this is implementation detail of LibStaking, ie. the lower level component does the book keeping this way, but this is not visible to the user (or the SubnetActor in your answer).

I don't know the details of LibStaking, why it needs to separate the two concepts. Is it because if you have collateral then if you leave you get something back? But isn't that decided by the parent? Isn't collateral just a label in the subnet? Couldn't the parent just look at the permission mode and decide whether to release tokens to the validator during unbonding or not?

In any case, in contrast to what I thought was an implementation detail, here the distinction proliferates through the API and the CLI, and now the user can assign both at the same time. I'm not sure if this is desirable. Maybe we just shouldn't call it collateral, but keep it as a single field?

My problem is that voting power can come from many places: it could be a consensus where PoW+PoS gives you power, or it could be coming from delegations. What we I think know is that for a subnet, it comes from the parent (and for rootnet, well, we don't do rootnets outside tests). I totally agree that collateral is misleading, but a proliferation of fields doesn't seem to be all that helpful to me.

On a different note, if we just wanted to make a distinction between tokens that can be returned to the users and other sources, we could perhaps call the fields funded and unfunded, with the latter being not backed by locked tokens.

Co-authored-by: Akosh Farkash <[email protected]>

raulk · 2024-03-15T20:03:51Z

I agree with @aakoshh. We do not need to track both collateral and weight.

But the problem is deeper: these should NOT be used in conjunction. There should just be weight, which is either set explicitly (federated) or as a projection of collateral (collateral), either or, but not the sum of both. This confusion came up in this thread and was corrected there, but the implementation hasn't been updated.

The "hybrid" model that uses both both collateral and federated is very unsound. There is no unit enforced for manually-assigned weights. So if the user assigns equal weights of 1, 1, 1, an attacker could come in and deposit 10 FIL and take over the network in a clean sweep.

raulk

Let's unblock by doing the following.

Parent subnet actor needs to track collateral and static power for every validator, before and after genesis.
Remove the GenesisValidator type and simply use ValidatorInfo type; unless I'm missing something, these types are redundant. @aakoshh also pointed it out in his review.
That should allow us to get rid of weirdness like this:
- the transformation in finalizeValidatorInfo.
- this conditional in genesisValidator().
The subnet actor translates the combination of validator collateral and static power into a weight, depending on the permissionMode.
- If collateral, use only collateral.
- If federated, use only the static power.
- If in the future we want to add a hybrid, we'll add the hybrid. But neither collateral nor federated should be hybrid.
The validator set we pull in genesis and we subsequently send as configuration changes MUST NOT include collateral nor static power, just the weight, i.e. it should be this type ideally, or something similar.

raulk · 2024-03-15T23:24:38Z

Actually, I just realised that the child gateway receives individual validator commands in the form of:

set_federated_power(new_power)
deposit(amount)
withdraw(amount)
...

It also stores full ValidatorInfo objects including the power, collateral, etc.

The child gateway then needs to reconcile all this info and recalculate the power locally based on the permissionMode. I think all of this is unnecessarily complex.

Ideally all this acccounting would be encapsulated in the source of truth: subnet actor at the parent. That actor would just emit new weights (computed based on its permissionMode) that propagate down to the child gateway to apply them in the subnet.

Unfortunately all of this is going to require a more profound discussion/architectural change to adjust. Ideally we would've had this discussion before implementing federated validation (at that time we just had collateral, which directly translated into weight), but that ship has sailed. At this stage, we might be better off just biting the bullet and refactoring later.

raulk · 2024-03-16T00:03:33Z

So I think we're just going to have the bullet and accept this change until we rearchitect to decouple the collateral and static power tracking at the parent, from their translation to a mere weight at the child.

That said, we should implement (2), (3) and (4) from above before we merge this PR.

Remove the GenesisValidator type and simply use ValidatorInfo type; unless I'm missing something, these types are redundant. @aakoshh also pointed it out in his review.

That should allow us to get rid of weirdness like this:

the transformation in finalizeValidatorInfo.

this conditional in genesisValidator().

The subnet actor translates the combination of validator collateral and static power into a weight, depending on the permissionMode.

If collateral, use only collateral.

If federated, use only the static power.

If in the future we want to add a hybrid, we'll add the hybrid. But neither collateral nor federated should be hybrid.

raulk · 2024-03-16T23:26:17Z

Merged in the interest of landing the fixes in the base branch onto main. Will discuss next steps with the team and @cryptoAtwill on Monday.

cryptoAtwill added 15 commits March 8, 2024 14:41

update genesis validator lifecycle

d89c63e

wip

66058b2

lint

11eec56

Merge branch 'init-staking-genesis' into init-staking-fendermint

ab5007b

align fendermint with contract

7e07404

lint

0e4f7ba

handle genesis in one place

97799c4

Merge branch 'init-staking-genesis' of protocol-github:consensus-ship…

fd67ab8

…yard/ipc into init-staking-fendermint

wip

3e9d26b

update membership

27db9bf

Merge branch 'init-staking-genesis' of protocol-github:consensus-ship…

83d8da6

…yard/ipc into init-staking-fendermint

fix typo

13bd4a4

more logging

c23d7ea

fix bug

a40e47b

Merge branch 'init-staking-genesis' of protocol-github:consensus-ship…

dd6f6aa

…yard/ipc into init-staking-fendermint

cryptoAtwill requested review from aakoshh and raulk March 11, 2024 10:30

cryptoAtwill changed the title ~~Init staking fendermint~~ Update fendermint to match genesis changes in solidity Mar 11, 2024

This was referenced Mar 11, 2024

Update genesis validator lifecycle #778

Merged

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

Closed

cryptoAtwill marked this pull request as draft March 11, 2024 10:37

aakoshh reviewed Mar 12, 2024

View reviewed changes

fendermint/vm/interpreter/src/fvm/state/ipc.rs Outdated Show resolved Hide resolved

aakoshh reviewed Mar 12, 2024

View reviewed changes

Update fendermint/vm/interpreter/src/fvm/state/ipc.rs

421aeea

Co-authored-by: Akosh Farkash <[email protected]>

Base automatically changed from init-staking-genesis to init-staking March 15, 2024 20:39

raulk force-pushed the init-staking-fendermint branch from c83762c to 421aeea Compare March 15, 2024 22:47

Merge branch 'init-staking' into init-staking-fendermint

0634811

raulk requested changes Mar 15, 2024

View reviewed changes

raulk assigned raulk and unassigned raulk Mar 15, 2024

raulk marked this pull request as ready for review March 16, 2024 15:25

raulk mentioned this pull request Mar 16, 2024

fix(federated validation): make subnet gateway use federated power to calculate power table #807

Merged

cleanup: rename type, implement Arbitrary, remove comments, refactor.

65ac374

raulk merged commit b88ce0c into init-staking Mar 16, 2024
12 of 14 checks passed

raulk deleted the init-staking-fendermint branch March 16, 2024 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update fendermint to match genesis changes in solidity #784

Update fendermint to match genesis changes in solidity #784

cryptoAtwill commented Mar 11, 2024 •

edited

Loading

aakoshh Mar 12, 2024

aakoshh Mar 12, 2024

cryptoAtwill Mar 13, 2024

aakoshh Mar 12, 2024

aakoshh Mar 12, 2024

aakoshh left a comment •

edited

Loading

raulk commented Mar 15, 2024 •

edited

Loading

raulk left a comment •

edited

Loading

raulk commented Mar 15, 2024

raulk commented Mar 16, 2024

raulk commented Mar 16, 2024

Update fendermint to match genesis changes in solidity #784

Update fendermint to match genesis changes in solidity #784

Conversation

cryptoAtwill commented Mar 11, 2024 • edited Loading

aakoshh Mar 12, 2024

Choose a reason for hiding this comment

aakoshh Mar 12, 2024

Choose a reason for hiding this comment

cryptoAtwill Mar 13, 2024

Choose a reason for hiding this comment

aakoshh Mar 12, 2024

Choose a reason for hiding this comment

aakoshh Mar 12, 2024

Choose a reason for hiding this comment

aakoshh left a comment • edited Loading

Choose a reason for hiding this comment

raulk commented Mar 15, 2024 • edited Loading

raulk left a comment • edited Loading

Choose a reason for hiding this comment

raulk commented Mar 15, 2024

raulk commented Mar 16, 2024

raulk commented Mar 16, 2024

cryptoAtwill commented Mar 11, 2024 •

edited

Loading

aakoshh left a comment •

edited

Loading

raulk commented Mar 15, 2024 •

edited

Loading

raulk left a comment •

edited

Loading