grandpa: don't require justification of consensus changes on full node #3540
Conversation
| number, | ||
| ) | ||
| } | ||
| self.extra_justifications.try_finalize_root((hash, number), finalization_result, true); |
There was a problem hiding this comment.
This can happen if we process the notification for on_block_finalized before we actually process the import result. Either way it is fine because we also call try_finalize_root on on_block_finalized so any necessary bookkeeping is done correctly.
There was a problem hiding this comment.
@andresilva I thought this check prevents it from happening, no?
Demi-Marie
left a comment
Demi-Marie
left a comment
There was a problem hiding this comment.
Can you incude the PR comment in the source code? This will be very useful for future developers.
| number, | ||
| ) | ||
| } | ||
| self.extra_justifications.try_finalize_root((hash, number), finalization_result, true); |
|
|
||
| if best_finalized_number > self.best_seen_finalized_number { | ||
| match self.tree.finalize_with_ancestors( | ||
| match self.tree.finalize( |
There was a problem hiding this comment.
Could you, please, change it back to finalize_with_ancestors() for finality proofs? It isn't guaranteed that they're coming in-order. If that PR needs to be merged ASAP, just ping me - I'll create a follow-up PR.
There was a problem hiding this comment.
@andresilva @svyatonik i guess this is on-ice until this change is made?
There was a problem hiding this comment.
@svyatonik Doesn't it have the same guarantees as for justifications? The code that makes the requests is shared with justifications and it will request things in-order as they are "popped" from the tree. I understand that the finality proof for B might have a justification for block B+n, but for checking in the tree we will be using in B anyway right?
There was a problem hiding this comment.
@andresilva You're right, on_block_finalized is called for every block in finalization path => they'll be in-order anyway. But then probably we should reconsider this limitation - it could happen that we'll only receive notification for B+n, but no notification for B => we could end up with the same UnfinalizedAncestor error, right?
There was a problem hiding this comment.
Indeed, I did not remember that. Let's revert back to finalize_with_ancestors() then.
svyatonik
left a comment
svyatonik
left a comment
There was a problem hiding this comment.
The only concern I have, is that the number of full nodes that could serve justifications to light clients will decrease - the nodes that have imported consensus-changes-block without justification, will never receive it, afau. But that isn't the biggest problem right now.
#3540) * grandpa: don't request justification for consensus changes on full node * sync: finalize justification/finality proof requests in-order * sync: ignore result of try_finalize_root on justification import
paritytech#3540) * grandpa: don't request justification for consensus changes on full node * sync: finalize justification/finality proof requests in-order * sync: ignore result of try_finalize_root on justification import
Currently
GrandpaBlockImporttracks authority set changes inAuthoritySet, we require a justification for blocks that enact an authority set change. Additionally, the GRANDPA voter will restrict its votes to blocks that enact an authority set change. This guarantees that said blocks, if finalized, will always have a justification. Additionally,GrandpaBlockImportkeeps track of consensus changes, this is any kind of changes to the client cache (e.g. a BABE epoch change), and we also require justification for those blocks (added in #3301).The problem with the current logic is that the sync code will always make the justification requests in-order, therefore if we cannot guarantee that a justification exists for a given block, we might get stuck requesting something endlessly. This is the case with BABE epoch changes since the GRANDPA voter won't limit its votes to these blocks.
This PR changes
GrandpaBlockImportto only require a justification from sync for authority set changes (not consensus changes). I started by removing all of consensus changes fromGrandpaBlockImportbut this is still required so that we persist a justification for blocks that enact a consensus change (so that we can later on serve it to light clients).I also changed the logic for finalizing pending requests in sync back to what it was before #3301. Since we only request justifications that must be satisfiable then we can also make sure that we finalize the requests in-order (i.e.
finalize_with_ancestors -> finalize).