Grandpa warp sync request-response protocol #7711
Conversation
or is there some signing of the result : that is the storage key, damn?
needed here. This assumes the hash from header is calculated.
github-actions
bot
added
the
A3-in_progress
… response protocol happens in the node code
|
Is this PR unfinished? |
|
This PR is basically the minimum amount of changes I needed to make to @cheme 's branch in order to get grandpa warp sync to work in substrate-lite. There are definately some things that need to be changed before it can be merged, getting the inspect subcommand working again is a big one. I'll change it to allow reviews anyway though. |
client/grandpa-warp-sync/src/lib.rs
Outdated
| pub fn new(protocol_id: ProtocolId, backend: Arc<TBackend>) -> (Self, ProtocolConfig) { | ||
| // Rate of arrival multiplied with the waiting time in the queue equals the queue length. | ||
| // | ||
| // An average Polkadot sentry node serves less than 5 requests per second. The 95th percentile |
There was a problem hiding this comment.
Well, that comment isn't true for warp syncing.
|
I did change a bit the iteration to not include change of authority set when another change is defined during the delay, should handle this case. I also added a simple cache (last two commit), it can be remove to another pr if needed. |
|
Could you please merge master so that the CI fixes itself |
…trate into ashley-grandpa-warp-sync
There was a problem hiding this comment.
After #7339 we should be able to rewrite the finality proof changes much more clearly. The main change being that we will have a mapping of set_id -> last_block_in_set, which means that we can just fetch justifications from disk based on that rather than iterating through all headers to find them. I have reviewed the changes in finality_proof lightly since based on this I expect this code to be rewritten soon.
I don't want to block this PR any further on it (I can make those changes after #7339 is merged). My only gripe right now is that the warp sync code should exist inside the grandpa crate.
client/grandpa-warp-sync/Cargo.toml
Outdated
| @@ -0,0 +1,28 @@ | |||
| [package] | |||
| description = "A request-response protocol for handling grandpa warp sync requests" | |||
| name = "sc-grandpa-warp-sync" | |||
There was a problem hiding this comment.
Why create a new crate for this? I think it should just go in the existing grandpa crate. This also avoids having to expose some grandpa internals.
There was a problem hiding this comment.
It's better to have it as a standalone crate IMO as it only can be used when BABE is also being used afaik.
There was a problem hiding this comment.
As it exists now I think it's independent from the block production protocol, it's creating finality proofs from genesis to some latest finalized block regardless of how that block itself is validated (although it's true that we also need to fetch BABE data elsewhere). I won't press on moving this to the grandpa crate but can we rename it to sc-finality-grandpa-warp-sync so the naming is closer to the other crate?
I had not reviewed the PR yet when I answered this question. For our purposes, as long as we find a justification then we can be sure it is the correct one. Otherwise this would mean that GRANDPA finalized contradicting things (i.e. different forks). The existing code lgtm. |
|
Merging master should hopefully now fix CI. |
|
If this PR satisfies everyone, I'd like to merge it today or tomorrow so that it is included in the upcoming Polkadot 0.7.28. |
|
Approving so we won't block this any further (as it blocks work on smoldot). But the |
|
(I DM'ed @cheme and no objection either) |
|
bot merge |
|
Trying merge. |
ghost
merged commit 
tomaka
added
B5-clientnoteworthy
and removed
B0-silent
See #1208. A messy first draft of a full-node request-response protocol for use with paritytech/smoldot#294. Essentially this takes @cheme's branch master...cheme:light_grandpa_warp and adds a request-response protocol on top.