Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

Top Down Checkpoint - Parent View #205

Closed
wants to merge 41 commits into from

Conversation

cryptoAtwill
Copy link
Contributor

Adding parent view into top down checkpoint, so that the child can constantly sync with the parent to get the latest state of the parent. At the same time, it also make finality proposal, checks finality proposals.

@cryptoAtwill cryptoAtwill mentioned this pull request Aug 21, 2023
@cryptoAtwill cryptoAtwill changed the title Parent view Top Down Checkpoint - Parent view Aug 21, 2023
@cryptoAtwill cryptoAtwill changed the title Top Down Checkpoint - Parent view Top Down Checkpoint - Parent View Aug 21, 2023
Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff! I added comments about where I'd consider using STM differently. I'm confused about the checking of top-down proposals when it comes to cross messages, it looks like either me or the code are missing something there.

fendermint/vm/topdown/src/error.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/error.rs Show resolved Hide resolved
fendermint/vm/topdown/src/finality.rs Outdated Show resolved Hide resolved
Ok(cache.get_value(height).map(|i| i.0.clone()))
}

fn validator_set(&self, height: BlockHeight) -> StmResult<Option<ValidatorSet>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the ValidatorSet contains every validator's key and address. How does it get included at every height? Could it be shared between them when nothing changes? Or be just the configuration ID, to be retrieved from yet another sequential cache? Although the increments are not necessarily uniform there AFAIK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can query the ValidatorSet at every block height in ipc agent. The thing I'm more concerned about is the size of the validators. If there are significant number of validators in the subnet, then every proposal would contain the full list of validators which the validators should not change that frequent. So we are wasting space/bandwidth for sth that does not change much. I have a feeling, which could be wrong, but a cid of the last committed validator set + validator changes in the proposal would be more efficient.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I totally agree, at least in your cache you could include the validator set changes in a monotonically increasing list of configuration IDs. You could treat them similarly to cross messages. Staying with our example of checkpointing every 10th block, if the last checkpoint was at height 50, and now we are 60, then the agent could add the new finality view by including:

  • The hash of block 60
  • The messages between height 51 and 60
  • The validator set changes that happened between height 51 and 60, sorted by configuration ID

The child validators are not obliged to adopt all validator sets, e.g. if between 51 and 60 we went from configuration 5 to configuration 7, then they can send the next checkpoint signed by validator set 5, stating that they will next adopt validator set 6, or 7, it's up to them. As long as they agree on what these numbers mean, they can use them to identify to the parent who to expect the next signatures from.

Copy link
Contributor Author

@cryptoAtwill cryptoAtwill Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, only block hash and height is in the finality.

fendermint/vm/topdown/src/finality.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/lib.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/lib.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/lib.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/lib.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/finality.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@adlrocha adlrocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice progress! My main concern is with how top-down messages are proposed. I am either misunderstanding something or I think we are doing something wrong. Have a look at my comments and let me know if they make sense.

Additionally, it would be really helpful if you could add more extensive comments on the implementation of the DefaultFinalityProvider so it is clear the end-to-end of the parent view (the flow of information, how are variables updated and when, and is each function expected to be used).

fendermint/vm/topdown/src/error.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/finality.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/finality.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/finality.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/finality.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/lib.rs Show resolved Hide resolved
fendermint/vm/topdown/src/finality.rs Show resolved Hide resolved
fendermint/vm/topdown/src/lib.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/finality.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/finality.rs Outdated Show resolved Hide resolved
cryptoAtwill and others added 8 commits August 24, 2023 11:28
@cryptoAtwill
Copy link
Contributor Author

cryptoAtwill commented Aug 24, 2023

@adlrocha @aakoshh The querying of top down messages is changed to indexing by height.

As discussed in slack, we dont include top down messages and validator set in the finality, we just propose on the block hash and height. Once it’s finalized, in deliver_tx, we detect it’s TopDownMessage, then query the top down messages for that height from cache and validate the block hash then apply the top down message. Same for validator set.

Please help take a look again.

Copy link
Contributor

@adlrocha adlrocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just missing some clarification about the garbage collection strategy (see comments), and I am ready to approve

fendermint/vm/topdown/src/finality.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/finality.rs Outdated Show resolved Hide resolved
fendermint/vm/topdown/src/lib.rs Show resolved Hide resolved
fendermint/vm/topdown/src/finality.rs Show resolved Hide resolved
cryptoAtwill and others added 3 commits August 25, 2023 15:30
Co-authored-by: adlrocha <6717133+adlrocha@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants