Commit parent finality #193

cryptoAtwill · 2023-08-29T00:07:05Z

Initial attempt to store and retrieve IPC parent finality in gateway contract. The updates are as follows:

Move setMembership to GatewayLib and remove the corresponding external function call in GatewayManagerFacet.
Add commitParentFinality to RouterFacet.
Add queryParentFinality to GatewayGetterFacet.

In follow up PR will clean up the code by removing submit top down and bottom up checkpoint and all their internal functions.

test/GatewayDiamond.t.sol

src/gateway/GatewayRouterFacet.sol

src/gateway/GatewayGetterFacet.sol

adlrocha

LGTM. The reason I request the change is because to wrap it up we should remove

ipc-solidity-actors/src/gateway/GatewayManagerFacet.sol

Line 25 in cc109a5

function initGenesisEpoch(uint64 genesisEpoch) external systemActorOnly {

. I don't think is needed anymore. If we need the topDownGenesisEpoch for something, we can set it directly when the first proof of finality is committed, but it can be kind of a mess having to also call this initGenesisEpoch implicitly when we are actually doing it implicitly when committing finality.

We can discuss sync if it is not clear.

test/GatewayDiamond.t.sol

src/lib/LibGateway.sol

adlrocha · 2023-08-31T09:17:04Z

src/lib/LibGateway.sol

+                    revert ValidatorWeightIsZero();
+                }
+
+                s.validatorSet[s.validatorNonce][validatorAddress] = validatorWeight;


I don't think we need to address it immediately, but we will need to garbage collect the membership info instead of keeping all the history. We may be able to garbage collect outdated info.

I added another variable to track the block height of the last committed height, we can commit and delete at the same time. I can implement this logic in follow up PRs.

src/lib/LibGatewayActorStorage.sol

src/gateway/GatewayRouterFacet.sol

adlrocha · 2023-08-31T10:49:06Z

@cryptoAtwill, we should also remove all the code around TopDownVoting, _submitTopDownVote, etc. This is no longer necessary. I'll check the changes required to apply_messages and we can discuss if to apply them here or in some other PR.

cryptoAtwill · 2023-09-01T02:18:19Z

@adlrocha sounds good, I will do all those deletion related work in a PR on top of this so that the changes are not mixed in this PR.

cryptoAtwill · 2023-09-04T07:30:00Z

@adlrocha I will create a separate PR to delete other methods, such as initGenesis. Because keep on deleting them will fail the unit tests and the PR get bigger. Another PR might be neater.

src/gateway/GatewayMessengerFacet.sol

adlrocha

@cryptoAtwill, few more changes before we merge, I can address them myself if you want. Let me know.

src/gateway/GatewayMessengerFacet.sol

src/gateway/GatewayRouterFacet.sol

adlrocha · 2023-09-12T03:58:14Z

src/interfaces/ISubnetActor.sol

-    /// This functions verifies that the checkpoint is valid before
-    /// propagating it for commitment to the IPC gateway. It expects at least
-    /// votes from 2/3 of miners with collateral.
-    function submitCheckpoint(BottomUpCheckpoint calldata checkpoint) external;


I don't think we should remove submitCheckpoint. It will change a lot, but will still be necessary with the new design (can you revert this one)?

adlrocha · 2023-09-12T03:59:08Z

src/subnet/SubnetActorManagerFacet.sol

@@ -104,52 +104,6 @@ contract SubnetActorManagerFacet is ISubnetActor, SubnetActorModifiers, Reentran
        IGateway(s.ipcGatewayAddr).kill();
    }

-    /// @notice methods that allows a validator to submit a checkpoint (batch of messages) and vote for it with it's own voting power.
-    /// @param checkpoint - the batch messages data
-    function submitCheckpoint(BottomUpCheckpoint calldata checkpoint) external {


We shouldn't remove this one for the same of above, bottom up checkpoints will be re-designed (without votings) but we still need it.

adlrocha · 2023-09-12T04:00:01Z

src/gateway/GatewayRouterFacet.sol

    using StorableMsgHelper for StorableMsg;

-    /// @notice submit a checkpoint in the gateway. Called from a subnet once the checkpoint is voted for and reaches majority
-    function commitChildCheck(BottomUpCheckpoint calldata commit) external {


We should also keep this one in the gateway.

dnkolegov · 2023-09-12T20:09:15Z

src/gateway/GatewayRouterFacet.sol

-
-        subnet.prevCheckpoint = commit;
+    /// @notice commit the ipc parent finality into storage
+    function commitParentFinality(


Why is this method not in the GatewayActor interface?

I added this into the interface already

dnkolegov · 2023-09-12T20:23:06Z

src/gateway/GatewayRouterFacet.sol

-
-        subnet.prevCheckpoint = commit;
+    /// @notice commit the ipc parent finality into storage
+    function commitParentFinality(


Could you provide more information about that function? What is the relation between finality data and validators and why are these validators added to membership?

Right, I think this is the mechanism of the top down checkpointing in fendermint, not sure if it suitable to place the docs here.

But long story short, membership information is pulled from the parent ipc-agent or node in fendermint. Then fendermint call this function implicitly.

dnkolegov · 2023-09-12T20:31:44Z

src/lib/LibGatewayActorStorage.sol

@@ -15,6 +15,10 @@ struct GatewayActorStorage {
    /// @notice a mapping of block number to cross messages
    /// SubnetID => blockNumber => messages
    mapping(bytes32 => mapping(uint256 => CrossMsg[])) topDownMsgs;
+    /// @notice The parent finalities. Key is the block number, value is the finality struct.
+    mapping(uint256 => ParentFinality) finalitiesMap;


Is it that fine we do not have a relation between committed ParentFinality and validators that committed it? Do we need it?

Not really. We just need the block height to parent finality.

dnkolegov · 2023-09-12T20:35:14Z

src/gateway/GatewayRouterFacet.sol

-
-        subnet.prevCheckpoint = commit;
+    /// @notice commit the ipc parent finality into storage
+    function commitParentFinality(


Where is the proof (multisig) that this was committed by these validators?

For top down parent finality, we dont really need mutisig.

adlrocha

Thank you for the changes, 🚢

cryptoAtwill marked this pull request as draft August 29, 2023 00:07

adlrocha self-requested a review August 29, 2023 06:55

cryptoAtwill changed the title ~~initial commit~~ Commit parent finality Aug 30, 2023

cryptoAtwill requested a review from dnkolegov August 30, 2023 04:29

dnkolegov reviewed Aug 30, 2023

View reviewed changes

test/GatewayDiamond.t.sol Outdated Show resolved Hide resolved

test/GatewayDiamond.t.sol Outdated Show resolved Hide resolved

src/gateway/GatewayRouterFacet.sol Show resolved Hide resolved

src/gateway/GatewayGetterFacet.sol Show resolved Hide resolved

cryptoAtwill requested a review from dnkolegov August 31, 2023 04:50

cryptoAtwill marked this pull request as ready for review August 31, 2023 04:51

adlrocha suggested changes Aug 31, 2023

View reviewed changes

adlrocha reviewed Aug 31, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Show resolved Hide resolved

cryptoAtwill added 16 commits September 4, 2023 15:39

resolve checkpoint

b5bfa95

add commit finality

2f1cd42

update test case

24c3ffd

update tests

a8e147b

format code

b2310b1

add more tests

590475d

more tests

6c08048

add query check

bbd0761

big number of messages

d5e5348

format code

11fb2ce

split methods

96ad32f

resolve test

dc4ebca

fix error

6e6f210

fix error

bf3c01e

fix error

ba27b71

revert test

e4172aa

cryptoAtwill force-pushed the topdown-finality branch from cd0cc1a to e4172aa Compare September 4, 2023 07:53

format code

4d8c2b5

cryptoAtwill added 4 commits September 5, 2023 16:42

add latest parent finality viewer

b77da5c

fix tests

47d9a0a

fix bug

e44f787

update storage layout

093bf20

cryptoAtwill requested a review from adlrocha September 6, 2023 03:28

cryptoAtwill added 9 commits September 6, 2023 11:45

remove more code

2141c7a

update interface

4ae6b11

fix tests

d225510

update tests

0b5deb9

remove line

e18726e

remove unused validators

4c6d342

update error

d20c5e0

fix tests

366270d

fix tests

01b672e

dnkolegov reviewed Sep 6, 2023

View reviewed changes

src/gateway/GatewayMessengerFacet.sol Show resolved Hide resolved

cryptoAtwill added 2 commits September 6, 2023 20:39

update deployment

ba3090e

update deployment

1a57704

dnkolegov approved these changes Sep 11, 2023

View reviewed changes

adlrocha suggested changes Sep 12, 2023

View reviewed changes

dnkolegov reviewed Sep 12, 2023

View reviewed changes

cryptoAtwill added 5 commits September 13, 2023 10:37

add bottom up related methods back

638d118

remove unused code

d96c612

update gateway deployment

b03bd97

update registry template

b137e87

fix registry deploy template

c5b615e

adlrocha approved these changes Sep 13, 2023

View reviewed changes

cryptoAtwill merged commit 34f3bf6 into dev Sep 13, 2023
5 checks passed

cryptoAtwill deleted the topdown-finality branch September 13, 2023 06:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Commit parent finality #193

Commit parent finality #193

cryptoAtwill commented Aug 29, 2023 •

edited

Loading

adlrocha left a comment

adlrocha Aug 31, 2023

cryptoAtwill Sep 6, 2023

adlrocha commented Aug 31, 2023

cryptoAtwill commented Sep 1, 2023

cryptoAtwill commented Sep 4, 2023

adlrocha left a comment

adlrocha Sep 12, 2023

cryptoAtwill Sep 13, 2023

adlrocha Sep 12, 2023

cryptoAtwill Sep 13, 2023

adlrocha Sep 12, 2023

dnkolegov Sep 12, 2023

cryptoAtwill Sep 13, 2023

dnkolegov Sep 12, 2023

cryptoAtwill Sep 13, 2023

dnkolegov Sep 12, 2023

cryptoAtwill Sep 13, 2023

dnkolegov Sep 12, 2023

cryptoAtwill Sep 13, 2023

adlrocha left a comment

Commit parent finality #193

Commit parent finality #193

Conversation

cryptoAtwill commented Aug 29, 2023 • edited Loading

adlrocha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adlrocha commented Aug 31, 2023

cryptoAtwill commented Sep 1, 2023

cryptoAtwill commented Sep 4, 2023

adlrocha 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

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

adlrocha left a comment

Choose a reason for hiding this comment

cryptoAtwill commented Aug 29, 2023 •

edited

Loading