Top Down Checkpoint - Parent Sync #206

cryptoAtwill · 2023-08-21T03:04:11Z

Constantly syncs with the parent using a polling mechanism. The ipc-agent pending integration.

…ermint into parent-sync

aakoshh · 2023-09-06T09:31:47Z

fendermint/vm/topdown/examples/run_polling.rs

+
+#[derive(Parser, Debug)]
+pub struct Options {
+    /// The URL of the ipc agent's RPC endpoint.


Ah, okay, I figured later this would be the agent.

You know what would be super useful is to add a README.md to the topdown crate with a section about how to run this example, which should include instructions for starting both Lotus and the agent, or at least links to where this is explained, because I don't think readers will know, which limits the usefulness of this example.

Have you thought this?

aakoshh · 2023-09-06T09:34:14Z

fendermint/vm/topdown/examples/run_polling.rs

+    #[arg(long, short)]
+    pub subnet_id: String,
+
+    /// The subnet id expressed a string


Wrong docs. If this is between mainnet and testnet, maybe it could say which value corresponds to what.

aakoshh · 2023-09-06T09:41:21Z

fendermint/vm/topdown/src/sync.rs

+        }
+
+        let new_parent_views =
+            get_new_parent_views(agent_proxy, starting_height, ending_height).await?;


Need to be somewhat careful here that if a node hasn't seen anything previously then it will I think start from the Genesis of Lotus and load everything into memory.

The IPC genesis of Fendermint should have a non-zero starting height (the height at which this subnet has been created), and as we discussed earlier this routine must not be started while the node is syncing with others, so new joiners don't fetch the entire history of the subnet.

There is an issue that tracks node is syncing with others.
Regarding the genesis, I think adding into the config should do it.

aakoshh · 2023-09-06T09:45:15Z

fendermint/vm/topdown/src/sync.rs

+        let validator_set = agent_proxy
+            .get_validator_set(h)
+            .await
+            .context("cannot fetch validator set")?;


It would be worth creating an issue to optimise this so that if there's no change then it only records the configuration ID. Like a method that says get_validator_set_if_changed(h, current_id) and it returns None if it's the same as the current_id, so we don't store a copy of the entire power table at every height in memory if nothing happened. cc @adlrocha

I think for all configuration id related issues, we should handle them altogether

fendermint/vm/topdown/src/sync.rs

fendermint/vm/topdown/src/finality.rs

aakoshh · 2023-09-06T12:27:15Z

fendermint/vm/topdown/src/finality.rs

+            return abort(Error::HeightThresholdNotReached);
+        }
+
+        let height = latest_height - self.config.chain_head_delay;


This seems to be not needed any more, because the syncer only adds data that is final (or should only do so anyway, like I said you should add docstrings to communicate this).

Really nice catch. I'm doing the integration testing, and with all the updates, I think the initial logic might have to update a little.

Hm, I thought this would change.

aakoshh · 2023-09-06T12:40:46Z

fendermint/vm/topdown/src/finality.rs

+        let finality = self
+            .last_committed_finality
+            .read_clone()?
+            .ok_or(StmError::Abort(Error::CommittedFinalityNotReady))?;


By the way I'm surprised that this is possible. There must always be something that we can consider final on the parent: genesis; either that of the entire parent chain, or where the subnet was conceived.

This is what we discussed turning into a panic in the new phased initialisation:

Internally the last_committed_finality is a TVar<Option<...>> and starts empty because we don't want to peak into the genesis file, and we might be restoring from a snapshot anyway that CometBFT needs to fetch for us.

Before the genesis is applied or the snapshot is loaded, CometBFT will not give us blocks to process, so the App should not call this method.

If it did, that would be a programming error, and we can just panic! if the last_committed_finality is None.

During genesis or loading a snapshot, the interpreter should call set_committed_finality to cause this to go become Some.

After that it's safe to query and return Stm<IPCParentFinality>

On subsequent starts we can query the ledger to know where to resume from.

Notably we discussed that we won't query in a loop because then there is a race condition between filling the cache and querying it.

fendermint/vm/topdown/src/finality.rs

* prepare and process * prepare and process * lint and format * format code * use dev branch * Update fendermint/app/config/default.toml Co-authored-by: Akosh Farkash <aakoshh@gmail.com> * Update fendermint/vm/interpreter/src/chain.rs Co-authored-by: Akosh Farkash <aakoshh@gmail.com> * review feedback * lint * fmt code * fix test * format code * update default subnet id * merge with parent branch * format code --------- Co-authored-by: Akosh Farkash <aakoshh@gmail.com>

aakoshh · 2023-09-11T11:53:49Z

fendermint/app/config/default.toml

+
+[ipc.config]
+# The number of blocks behind chain head to propose parent finality.
+chain_head_delay = 10


Suggested change

chain_head_delay = 10

chain_head_delay = 900

It's Filecoin, and this is the safe default.

aakoshh · 2023-09-11T11:55:49Z

fendermint/app/src/cmd/run.rs

@@ -21,6 +23,11 @@ cmd! {
  }
 }

+async fn create_parent_finality(settings: &Settings) -> anyhow::Result<InMemoryFinalityProvider> {


Just curious why this has to be async.
Or indeed why it has to return a Result.

aakoshh · 2023-09-11T12:04:46Z

fendermint/vm/interpreter/src/chain.rs

+        match atomically_or_err::<_, fendermint_vm_topdown::Error, _>(|| {
+            finality_provider.next_proposal()
+        })
+        .await?


Based on this code it still doesn't look like we're on the same page.

What I have been trying to say in my PR comments, Slack, and Zoom is that

next_proposal should not return an Error, it should be fn next_proposal(&self) -> Stm<Option<Proposal>>.

it should be called with atomically, not atomically_or_err

if you use ? in an interpreter and it fails it will crash Fendermint, so only ever use it for fatal errors

none of the error conditions I remember are fatal, some were temporary, some config based

Sorry if I seem stuck on this issue, it's not because I am trying to be super pedantic, but because we have to be so careful about what we treat as an error. In other cases it's easy to say just return anyhow::Result, who cares, ? will make it disappear, but it's precisely that pattern that gets us into trouble here. @adlrocha asked the same thing first, if it's okay to return an error. It's okay if there is nothing else you could return. Like you can't reach the database, so what will you do? We must make sure that if we return errors its' because a) we want the call site to handle them or b) we want the world to stop basically.

aakoshh · 2023-09-11T12:05:56Z

fendermint/vm/interpreter/src/chain.rs

+                    block_hash: proposal.block_hash,
+                })))
+            }
+        }


Funny how you wedged this in between the creation and the use of ckpts.

aakoshh · 2023-09-11T12:07:59Z

fendermint/vm/interpreter/src/chain.rs

        // Append at the end - if we run out of block space, these are going to be reproposed in the next block.
        msgs.extend(ckpts);
        Ok(msgs)
    }

    /// Perform finality checks on top-down transactions and availability checks on bottom-up transactions.
-    async fn process(&self, pool: Self::State, msgs: Vec<Self::Message>) -> anyhow::Result<bool> {
+    async fn process(&self, state: Self::State, msgs: Vec<Self::Message>) -> anyhow::Result<bool> {


Suggested change

async fn process(&self, state: Self::State, msgs: Vec<Self::Message>) -> anyhow::Result<bool> {

async fn process(&self, (pool, provider): Self::State, msgs: Vec<Self::Message>) -> anyhow::Result<bool> {

I think this works and would make the rest of the code more readable. state.0.get_status is a head scracher.

aakoshh · 2023-09-11T12:08:46Z

fendermint/vm/interpreter/src/chain.rs

+                    if atomically_or_err(|| state.1.check_proposal(&prop))
+                        .await
+                        .is_err()


check_proposal should return Stm<bool> 🙏

aakoshh · 2023-09-11T12:12:54Z

fendermint/vm/topdown/src/lib.rs

+pub trait ParentFinalityProvider: ParentViewProvider {
+    /// Obtains the last committed finality
+    fn last_committed_finality(&self) -> StmResult<IPCParentFinality, Error>;
+    /// Latest proposal for parent finality
+    fn next_proposal(&self) -> StmResult<Option<IPCParentFinality>, Error>;
+    /// Check if the target proposal is valid
+    fn check_proposal(&self, proposal: &IPCParentFinality) -> StmResult<(), Error>;
+    /// Called when finality is committed
+    fn on_finality_committed(&self, finality: &IPCParentFinality) -> StmResult<(), Error>;
+}


Suggested change

pub trait ParentFinalityProvider: ParentViewProvider {

/// Obtains the last committed finality

fn last_committed_finality(&self) -> StmResult<IPCParentFinality, Error>;

/// Latest proposal for parent finality

fn next_proposal(&self) -> StmResult<Option<IPCParentFinality>, Error>;

/// Check if the target proposal is valid

fn check_proposal(&self, proposal: &IPCParentFinality) -> StmResult<(), Error>;

/// Called when finality is committed

fn on_finality_committed(&self, finality: &IPCParentFinality) -> StmResult<(), Error>;

}

pub trait ParentFinalityProvider: ParentViewProvider {

/// Obtains the last committed finality

fn last_committed_finality(&self) -> Stm<IPCParentFinality>;

/// Latest proposal for parent finality

fn next_proposal(&self) -> Stm<Option<IPCParentFinality>>;

/// Check if the target proposal is valid

fn check_proposal(&self, proposal: &IPCParentFinality) -> Stm<bool>;

/// Called when finality is committed

fn on_finality_committed(&self, finality: &IPCParentFinality) -> Stm<()>;

}

aakoshh · 2023-09-11T12:14:12Z

fendermint/vm/topdown/src/lib.rs

+    fn latest_height(&self) -> StmResult<Option<BlockHeight>, Error>;
+    /// Get the block hash at height
+    fn block_hash(&self, height: BlockHeight) -> StmResult<Option<BlockHash>, Error>;


Suggested change

fn latest_height(&self) -> StmResult<Option<BlockHeight>, Error>;

/// Get the block hash at height

fn block_hash(&self, height: BlockHeight) -> StmResult<Option<BlockHash>, Error>;

fn latest_height(&self) -> Stm<Option<BlockHeight>>;

/// Get the block hash at height

fn block_hash(&self, height: BlockHeight) -> Stm<Option<BlockHash>>;

aakoshh · 2023-09-11T12:14:57Z

fendermint/vm/topdown/src/lib.rs

+    /// Get the validator set at height
+    fn validator_set(&self, height: BlockHeight) -> StmResult<Option<ValidatorSet>, Error>;


Suggested change

/// Get the validator set at height

fn validator_set(&self, height: BlockHeight) -> StmResult<Option<ValidatorSet>, Error>;

aakoshh · 2023-09-11T12:15:26Z

fendermint/vm/topdown/src/lib.rs

+    /// Get the validator set at height
+    fn validator_set(&self, height: BlockHeight) -> StmResult<Option<ValidatorSet>, Error>;
+    /// Get the top down messages at height
+    fn top_down_msgs(&self, height: BlockHeight) -> StmResult<Vec<CrossMsg>, Error>;


Suggested change

fn top_down_msgs(&self, height: BlockHeight) -> StmResult<Vec<CrossMsg>, Error>;

fn top_down_msgs(&self, height: BlockHeight) -> Stm<Option<Vec<CrossMsg>>>;

aakoshh · 2023-09-12T09:21:57Z

So the fixes to the comments in this PR are supposed to be in #236

cryptoAtwill added 30 commits August 19, 2023 20:15

adding cache

fa4db93

initial

72943a3

add finality provider

5540fd7

add values method

ead95fe

Merge branch 'top-down-checkpoint' into parent-view

ce09ca9

impl parent view provider

24f012a

add headers

9445706

add update

082b603

add remove key above

07590f0

Merge branch 'top-down-checkpoint' into parent-view

c6592fb

add new parent view

5977596

initial

f7e32d1

Merge branch 'parent-view' into parent-sync

f643ce0

wip

91a9856

remove arc

ebe9da7

Merge branch 'parent-view' into parent-sync

d700879

wip

43233b5

update interface

2137051

Merge branch 'parent-view' into parent-sync

2119523

add update parent view

ca590c9

add nonce

79e8db5

Merge branch 'parent-view' into parent-sync

9d296e0

format code

0ecb577

add unit tests

84fb03b

update unit test

561d0a8

add more checks

f713120

Merge branch 'top-down-checkpoint' into parent-view

54c4c0d

testing wip

6d6358c

add cache increment

7b1e7b7

Merge branch 'top-down-checkpoint' into parent-view

126954e

cryptoAtwill added 2 commits September 5, 2023 12:04

Merge branch 'parent-sync' of protocol-github:consensus-shipyard/fend…

3753c23

…ermint into parent-sync

update finality

220c81e

cryptoAtwill requested review from aakoshh and adlrocha September 6, 2023 02:21

aakoshh reviewed Sep 6, 2023

View reviewed changes

fendermint/vm/topdown/src/sync.rs Outdated Show resolved Hide resolved

aakoshh reviewed Sep 6, 2023

View reviewed changes

fendermint/vm/topdown/src/finality.rs Show resolved Hide resolved

aakoshh reviewed Sep 6, 2023

View reviewed changes

fendermint/vm/topdown/src/finality.rs Outdated Show resolved Hide resolved

aakoshh reviewed Sep 6, 2023

View reviewed changes

fendermint/vm/topdown/src/finality.rs Show resolved Hide resolved

cryptoAtwill and others added 3 commits September 7, 2023 13:33

refine sync logic

f8b47bc

update next proposal error checking

3dc084e

aakoshh reviewed Sep 11, 2023

View reviewed changes

cryptoAtwill closed this Sep 27, 2023

cryptoAtwill deleted the parent-sync branch September 27, 2023 01:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Top Down Checkpoint - Parent Sync #206

Top Down Checkpoint - Parent Sync #206

cryptoAtwill commented Aug 21, 2023

aakoshh Sep 6, 2023

aakoshh Sep 11, 2023

aakoshh Sep 6, 2023

aakoshh Sep 11, 2023

aakoshh Sep 6, 2023

cryptoAtwill Sep 7, 2023 •

edited

Loading

aakoshh Sep 6, 2023

cryptoAtwill Sep 7, 2023

aakoshh Sep 6, 2023

cryptoAtwill Sep 7, 2023

aakoshh Sep 11, 2023

aakoshh Sep 6, 2023

aakoshh Sep 11, 2023 •

edited

Loading

aakoshh Sep 11, 2023

aakoshh Sep 11, 2023 •

edited

Loading

aakoshh Sep 11, 2023

aakoshh Sep 11, 2023

aakoshh Sep 11, 2023

aakoshh Sep 11, 2023

aakoshh Sep 11, 2023

aakoshh Sep 11, 2023

aakoshh Sep 11, 2023

aakoshh Sep 11, 2023

aakoshh commented Sep 12, 2023

	async fn process(&self, state: Self::State, msgs: Vec<Self::Message>) -> anyhow::Result<bool> {
	async fn process(&self, (pool, provider): Self::State, msgs: Vec<Self::Message>) -> anyhow::Result<bool> {

		/// Get the validator set at height
		fn validator_set(&self, height: BlockHeight) -> StmResult<Option<ValidatorSet>, Error>;

	fn top_down_msgs(&self, height: BlockHeight) -> StmResult<Vec<CrossMsg>, Error>;
	fn top_down_msgs(&self, height: BlockHeight) -> Stm<Option<Vec<CrossMsg>>>;

Top Down Checkpoint - Parent Sync #206

Top Down Checkpoint - Parent Sync #206

Conversation

cryptoAtwill commented Aug 21, 2023

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 Sep 7, 2023 • 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

aakoshh Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakoshh Sep 11, 2023 • 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

aakoshh commented Sep 12, 2023

cryptoAtwill Sep 7, 2023 •

edited

Loading

aakoshh Sep 11, 2023 •

edited

Loading

aakoshh Sep 11, 2023 •

edited

Loading