This repository has been archived by the owner on Jan 11, 2024. It is now read-only.
Look ahead limit use finalized height #433
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adlrocha
approved these changes
Nov 15, 2023
fendermint/vm/topdown/src/sync.rs
Outdated
| @@ -223,16 +223,17 @@ where | |||
| return Ok(()); | |||
| } | |||
|
|
|||
| let ending_height = parent_chain_head_height - config.chain_head_delay; | |||
| // we consider the chain head finalized only after the `chain_head_delay` | |||
| let finalized_chain_head = parent_chain_head_height - config.chain_head_delay; | |||
There was a problem hiding this comment.
Suggested change
| let finalized_chain_head = parent_chain_head_height - config.chain_head_delay; | |
| let proposal_chain_head = parent_chain_head_height - config.chain_head_delay; |
| @@ -389,12 +394,13 @@ async fn parent_views_in_block_range( | |||
| mut previous_hash: BlockHash, | |||
| start_height: BlockHeight, | |||
| end_height: BlockHeight, | |||
| look_ahead_limit: BlockHeight, | |||
There was a problem hiding this comment.
Can you add a comment of how look_ahead_limit modifies the behavior of this function?
added 3 commits
November 15, 2023 19:41
aakoshh
reviewed
Nov 15, 2023
| /// Sadly, the height h + 1 could be null block, we need to continuously look ahead until we found | ||
| /// a height that is not null. But we cannot go all the way to the block head as it's not considered | ||
| /// final yet. So we need to use a `look_ahead_limit` that restricts how far as head we should go. | ||
| /// If we still cannot find a height that is non-null, maybe we should try later |
There was a problem hiding this comment.
To confirm: if we go ahead to the limit and still can't find a non-null block, are we ever going to get out of it by trying later?
cryptoAtwill
added a commit
that referenced
this pull request
Nov 15, 2023
* skip null round retry * rearrange logging * specify import * better null error handling * format code * add tracing logs * expand tilda * prettier logging * prettier logs * update logging * prettier log * use ipc user instead of root * specify log path * remove unused logs * Update fendermint/vm/topdown/src/finality/null.rs Co-authored-by: Akosh Farkash <[email protected]> * Update fendermint/vm/interpreter/src/chain.rs Co-authored-by: Akosh Farkash <[email protected]> * Update fendermint/vm/topdown/src/sync.rs Co-authored-by: Akosh Farkash <[email protected]> * structured logging and log dir optional * add debug logger * add debug logs * log fendermint only * Look ahead limit use finalized height (#433) * look ahead limit use finalized height * rename variable * more comments * more logs * comments * Update fendermint/vm/topdown/src/sync.rs Co-authored-by: Akosh Farkash <[email protected]> * rename makes everything clearer * Update fendermint/app/src/main.rs Co-authored-by: Akosh Farkash <[email protected]> * clean logs * log to console * update docker commands --------- Co-authored-by: Akosh Farkash <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix look ahead null block error.
This is a tricky edge case, say the current height is 100 and chain head is 1000 and our block fetching limit is 100. So we will be fetching from height 100 to 200 in this batch. Now, when in the loop we queried till block 199 and we need to fetch the top down messages. Because we need the block hash at block 200, so we query block 200 first to get the block hash. But sadly block 200 is a null block which throws an error. We should fetch block 201. As a safety precaution, if 201 is a null block ,we need to fetch 202 and so on. Theoretically, we can go all the way to block head. But block head is not considered finalised, so we put a
look_ahead_limit.According to the old logic, the
look_ahead_limitis just the end block height of the current batch, which is 200 in the above example. So if block 200 is a null block, an error will be thrown as we reached look ahead limit. That's why the fetching parent is stuck. We change thelook_ahead_limittofinalised_height, i.e. the height x blocks away from the chain head that we considered final. If 200 is the finalised height, it will throw an error, but as chain head progresses, thefinalised_heightwill move beyond 200 and we should not get stuck.