[Fix] Initialize the BFT DAG with the correct certificates at bootup #3162
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
howardwu
reviewed
Mar 9, 2024
howardwu
approved these changes
Mar 9, 2024
Co-authored-by: Howard Wu <9260812+howardwu@users.noreply.github.com> Signed-off-by: Raymond Chu <14917648+raychu86@users.noreply.github.com>
This was referenced Mar 23, 2024
howardwu
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
This PR initializes the BFT
DAGwith the correct committed certificate state by updatingsync_bft_dag_at_bootupto simply commit all thecertificatesthat were fetched from ledger. Previously, we were callingDAG::insertandBFT::commit_leader_certificateto simulate the commit process, but because we can guarantee these certificates have already been committed (exist in the ledger), this logic was incorrect.In addition, we've updated
order_dag_with_dfsto skip certificates that already exist in the ledger for the same reason. There is no need to reorder certificates that have been ordered before in previous subdags.With the removal of the extraneous logic, we could also simplify the
sync_bft_dag_at_bootupchannel and remove theIS_SYNCINGconst generic.Test Plan
Unit tests have been added to check that the DAG state of the BFT after bootup has the correct commit state.