This repository was archived by the owner on Nov 11, 2022. It is now read-only.
Milestone 1 #144
Merged
Milestone 1 #144
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
tomusdrw
reviewed
Apr 9, 2021
tomusdrw
approved these changes
Apr 12, 2021
|
Resolves: #112 Re-syncing of a BEEFY node is done by calling validator_set() as part of finality notification processing. If a BEEFY node is out-of-sync, upon receiving a finality notification, it will either
|
tomusdrw
reviewed
Apr 13, 2021
beefy-gadget/src/worker.rs
Outdated
Comment on lines
332
to
335
| // NOTE: currently we act as if this block has been finalized by BEEFY as we perform | ||
| // the validator set changes instantly (insecure). Once proper validator set changes | ||
| // are implemented this should be removed | ||
| self.best_finalized_block = Some(*notification.header.number()); |
There was a problem hiding this comment.
I think this is a little bit confusing and I think we still miss a bit of logic. Let me elaborate.
We have two block variables that both affect should_vote_on:
best_finalized_blockbest_block_voted_on
I think we kept confusing the two and probably the implementation instructions were incorrect.
So the should_vote_on formula should be roughly something like that:
(best_observed_block - best_beefy_finalized_block).next_power_of_two()
The logic for current variables:
best_finalized_block=best_observed_block, but with extraNonevalue right after starting up (will be set toSomeon first notification).best_block_voted_on= in case the node is voting, this will get updated with last vote.
I think we need to change the logic (and names) of best_block_voted_on instead of best_finalized_block. Basically:
best_finalized_blockshould simply be updated every time we receive GRANDPA finality notification (IMHO we should even call itbest_grandpa_block). It can be boostraped withself.client.info().best_finalized.best_beefy_blockshould beNoneinitially and should only be updated in every case from this list:- We conclude a round (i.e there exists
SignedCommitment). - We transition to a new validator set.
- We conclude a round (i.e there exists
The second case is obviously a temporary measure for Milestone 1.
tomusdrw
approved these changes
Apr 13, 2021
tomusdrw
pushed a commit
that referenced
this pull request
Apr 23, 2021
* use best_finalized, prevent race * make best_finalized_block an Option, should_vote_on bails on None * Bump futures from 0.3.13 to 0.3.14 * Revert futures bump * Revert "Revert futures bump" This reverts commit a1b5e7e. * Revert "Bump futures from 0.3.13 to 0.3.14" This reverts commit a4e508b. * debug msg if the bail voting * validator_set() * local_id() * get rid of worker state * Apply review suggestions * fix should_vote_on()
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.
Changes we have identified for
Milestone 1:init_validator_set()should usebest_finalizedand notbest_hashclient.info()should be stored to prevent racesbest_finalized_blockshould be optionalshould_vote()should bail ifbest_finalized_blockisNoneThat should cover a non-optimized happy case.