Grandpa validator set handoff justification #1190
Conversation
this will allow the `BlockImport` to trigger an authority set change when importing a change block that provides a justification (when syncing)
this will be used by `BlockImport` to check whether the authority set for a given block is still live, if the authority set isn't live then importing a change block requires a justification.
andresilva
added
the
A3-in_progress
| @@ -1081,6 +1092,9 @@ impl<B, E, Block, RA> consensus::BlockImport<Block> for Client<B, E, Block, RA> | |||
| finalized, | |||
| auxiliary, | |||
| } = import_block; | |||
|
|
|||
| assert!(justification.is_some() && finalized || justification.is_none()); | |||
There was a problem hiding this comment.
Maybe we should create a type: enum Finality { Finalized, FinalizedAndJustified(justification), Unfinalized }.
core/finality-grandpa/src/lib.rs
Outdated
| struct GrandpaJustification<Block: BlockT> { | ||
| round: u64, | ||
| commit: Commit<Block>, | ||
| ancestry: Vec<Block::Header>, |
There was a problem hiding this comment.
maybe rename to vote_ancestries or something similar?
core/finality-grandpa/src/lib.rs
Outdated
| let mut ancestry_hashes = HashSet::new(); | ||
| let mut ancestry = Vec::new(); | ||
|
|
||
| let error = { |
There was a problem hiding this comment.
could we do this lazily with a closure?
| @@ -644,41 +853,129 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block> | |||
| { | |||
| use authorities::PendingChange; | |||
|
|
|||
| // we don't want to finalize on `inner.import_block` | |||
| let justification = block.justification.take(); | |||
There was a problem hiding this comment.
does this comment mean that import_block automatically finalizes when justification is Some?
The cloning and hashing within this import_block implementation could add up but perhaps this is premature optimization.
There was a problem hiding this comment.
Yes, that is the assumption. It only actually finalizes if finalized = true, but the contract of client.import_block has changed to: if a justification is provided, then the block must be finalized (this should maybe be statically typed instead of using different parameters for finalized and the justification).
| } | ||
|
|
||
| #[derive(Clone)] | ||
| struct SharedGrandpaOracle<Block: BlockT> { |
There was a problem hiding this comment.
a comment describing what this is for would be helpful.
as a heuristic to decide whether
- a block has been finalized and we should expect a justification, or
- a block hasn't been finalized and we should watch for a commit message
There was a problem hiding this comment.
Done, added the docs to GrandpaOracle instead.
| "missing justification for block that enacts authority set change".to_string() | ||
| ).into()); | ||
| }, | ||
| _ => {} |
There was a problem hiding this comment.
we may want something here for writing to the SharedAuthoritySet to note that we imported a block to the chain which should have enacted the authority set chain (this may well be a set of heads).
Actual code that periodically revisits this and re-requests justifications can be done in a follow-up PR.
There was a problem hiding this comment.
Fixed by making sure AuthoritySet only imports one pending change per fork.
| debug!(target: "afg", "Restoring old set after block import error: {:?}", e); | ||
| *authorities = old_set; | ||
| } | ||
| let enacts_change = self.authority_set.inner().read().enacts_change(number, |canon_number| { |
There was a problem hiding this comment.
Probably out of scope, but still: the change that is checked here (and applied later in finalize_block call) contains the same set of authorities as in the new_authorities set? Or those are/could be different sets?
There was a problem hiding this comment.
Also seems that if importing block with justification assumes finalization of the block (that's what @rphmeier asked above, iiuc), then imo we don't need new_authorities and authorities cache at all - we just need to refuse light-import of blocks which contain DigestItem::as_authorities_change and no justification. Could you, please, verify me here, @rphmeier ?
There was a problem hiding this comment.
@svyatonik yep, I believe that for GRANDPA we can indeed take that kind of approach. We may still want the authorities cache for other consensus algorithms.
There was a problem hiding this comment.
although this is a bit of future work, because it would require tracking the authorship validator changes in GRANDPA and saving justifications for those too upon finality.
this is a little bit harder than the GRANDPA set changes because we may have a commit for block N+K where a set change happened in block N; unlike GRANDPA-set changes where honest voters can never vote beyond N. so constructing the justification may become inefficient without a good way to prove ancestry far back in the chain.
This reverts commit 51eb2c5.
andresilva
force-pushed
the
andre/grandpa-handoff-justification
branch
from
ce1c504
to
e5c593c
Compare
andresilva
added
A0-please_review
andresilva
added
A0-please_review
|
I think this is good for review now, I'm still going to do some changes but I think those can be done in a follow up PR. |
rphmeier
added
A8-looksgood
and removed
A0-please_review
* core: make block justification optional * runtime: update wasm binaries * core: optionally pass justification on finalize_block * finality-grandpa: add channel to trigger authority set changes this will allow the `BlockImport` to trigger an authority set change when importing a change block that provides a justification (when syncing) * finality-grandpa: move finalize_block to free function * finality-grandpa: add GrandpaOracle for auth set liveness checking this will be used by `BlockImport` to check whether the authority set for a given block is still live, if the authority set isn't live then importing a change block requires a justification. * finality-grandpa: store justification on finalized transition blocks * finality-grandpa: check justification on authority set change blocks * finality-grandpa: poll grandpa liveness oracle every 10 seconds * finality-grandpa: spawn grandpa oracle in service setup * core: support multiple subscriptions per consensus gossip topic * finality-grandpa: create and verify justifications * finality-grandpa: update to local branch of grandpa * finality-grandpa: update to finality-grandpa v0.5.0 * finality-grandpa: move grandpa oracle code * finality-grandpa: fix canonality check * finality-grandpa: clean up error handling * finality-grandpa: fix canonical_at_height * finality-grandpa: fix tests * runtime: update wasm binaries * core: add tests for finalizing block with justification * finality-grandpa: improve validation of justifications * core: remove unused IncompleteJustification block import error * core: test multiple subscribers for same consensus gossip topic * Revert "finality-grandpa: improve validation of justifications" This reverts commit 51eb2c5. * finality-grandpa: fix commit validation * finality-grandpa: fix commit ancestry validation * finality-grandpa: use grandpa v0.5.1 * finality-grandpa: add docs * finality-grandpa: fix failing test * finality-grandpa: only allow a pending authority set change per fork * finality-grandpa: fix validator set transition test
* Introduce generalised proxies to polkadot * Introduce proxy to westend * Add proxy to Kusama. * Fix
Closes #1069.
finalize_blockis modified to take anOption<Justification>which is stored in the database. Grandpa will store justifications for authority set change blocks, these will be available through thesyncmodule later on and passed on to requesting nodes. Currently thesyncmodule always requests justifications but doesn't validate whether they're provided or not, theGrandpaBlockImportwill then check whether the block to be imported is a change block and whether a justification should have been provided (this is conditional on the current authority set not being live anymore).