grandpa-rpc: allow proving finality of blocks from latest authority set #8585
Conversation
octol
added
A3-in_progress
github-actions
bot
added
A0-please_review
| #[derive(Debug, Encode, Decode, Clone, PartialEq)] | ||
| pub struct AuthoritySetChanges<N>(Vec<(u64, N)>); | ||
|
|
||
| /// The response when queuering for a the set id for a specific block. Either we get a set id |
There was a problem hiding this comment.
| /// The response when queuering for a the set id for a specific block. Either we get a set id | |
| /// The response when querying for a the set id for a specific block. Either we get a set id |
|
I'm not quite sure what this changes with respect to grandpa warp sync. Does it make anything faster for smoldot? |
andresilva
left a comment
andresilva
left a comment
There was a problem hiding this comment.
If you don't want to deal with removing ProvableJustification in this PR that's OK, but please don't change the signature of best_justification and update_best_justification.
| ) -> R | ||
| where | ||
| Header: HeaderT, | ||
| J: ProvableJustification<Header>, |
There was a problem hiding this comment.
I wanted to remove ProvableJustification as it is a bit useless right now. Tests can just create a proper GrandpaJustification.
| #[derive(Debug, PartialEq)] | ||
| pub enum AuthoritySetChangeId<N> { | ||
| /// The requested block is in the latest set. | ||
| Latest, | ||
| /// Tuple containing the set id and the last block number of that set. | ||
| Set(SetId, N), | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't really like this type, seems overly complicated for what we're doing here. Perhaps we should just drop the assumption that some data might not be available.
| } | ||
| } | ||
|
|
||
| pub(crate) fn block_is_current_set(&self, block_number: N) -> Option<bool> { |
There was a problem hiding this comment.
Can just be moved above as it's not used elsewhere.
It should make no difference for grandpa warp sync, this only affects the |
|
Thanks for the feedback :) |
|
Pushed a new set of commits that primarily removes |
andresilva
changed the title
andresilva
left a comment
andresilva
left a comment
There was a problem hiding this comment.
I pushed some changes myself. Tests still need some cleaning up.
| }; | ||
| let proof = check_finality_proof::<_, TestJustification>( | ||
| 1, | ||
| let proof = check_finality_proof::<Block>( |
There was a problem hiding this comment.
With the latest changes I think we're not actually testing anything here.
There was a problem hiding this comment.
We're mostly just testing GrandpaJustification::verify, but that's mostly what check_finality_proof does anyway I think?
|
|
||
| for block in to_finalize { | ||
| let just = block.encode(); | ||
| client.finalize_block(BlockId::Number(*block), Some((ID, just))).unwrap(); |
There was a problem hiding this comment.
The justification we're creating here is actually just a block number. This probably made sense before the latest changes when the test code was generic on the justification type.
There was a problem hiding this comment.
It's just some Vec<u8>, but wanted something unique per block in case the tests checked for those justifications. I'll change this to something simpler
| precommits, | ||
| }; | ||
|
|
||
| let best_grandpa_just = GrandpaJustification::from_commit(&client, 8, commit).unwrap(); |
There was a problem hiding this comment.
Maybe adding some comments around the setup in this test will make it easier to understand.
| }; | ||
|
|
||
| let best_grandpa_just = GrandpaJustification::from_commit(&client, 8, commit).unwrap(); | ||
| store_best_justification(&client, &best_grandpa_just); |
There was a problem hiding this comment.
If this test is supposed to be using the justification from the block then why store the best justification? That's what we test in the test below.
There was a problem hiding this comment.
Storing the best justification too, just to check that it is not the one that gets used
| let block8 = &blocks[7]; | ||
|
|
||
| let grandpa_just8 = vec![42]; | ||
| client.finalize_block(BlockId::Number(8), Some((ID, grandpa_just8.clone()))).unwrap(); |
There was a problem hiding this comment.
Shouldn't we store a valid justification here?
|
bot merge |
|
Trying merge. |
|
Bot will approve on the behalf of @andresilva, since they are a team lead, in an attempt to reach the minimum approval count |
ghost
merged commit 3 participants
In #8392 we started storing the latest justification for every finalized block. With this we can now respond with this latest justification in
prove_finalitywhen asked for a block in the latest authority set.This also removes
ProvableJustificationAlso fix a bug where the
FinalityProof::unknown_headersdid not contain the last header in the sequence, that is, the header corresponding to the justification also included.