Decouples light-sync state from chain spec #9491
Conversation
This decouples the light-sync state from chain spec. First, the light-sync state currently only works with BABE+Grandpa, so not all *Substrate* based chains can use this feature. The next problem was also that this pulled the `sc-consensus-babe` and `sc-finality-grandpa` crate into `sc-chain-spec`. If a chain now wants to support the light-sync state, it needs to add the `LightSyncStateExtension` to the chain spec as an extension. This is documented in the crate level docs of `sc-sync-state-rpc`. If this extension is not available, `SyncStateRpc` fails at initialization.
bkchr
added
A0-please_review
client/consensus/babe/src/lib.rs
Outdated
| @@ -129,7 +129,7 @@ pub use sp_consensus_babe::{ | |||
| }, | |||
| AuthorityId, AuthorityPair, AuthoritySignature, BabeApi, BabeAuthorityWeight, | |||
| BabeEpochConfiguration, BabeGenesisConfiguration, ConsensusLog, BABE_ENGINE_ID, | |||
| VRF_OUTPUT_LENGTH, | |||
| VRF_OUTPUT_LENGTH, BabeBlockWeight, | |||
There was a problem hiding this comment.
You need to configure your editor to cargo fmt now 😏.
|
What is the "light sync state" used for? Looks like the code that was supposed to use it was never written? Why not just remove it? Also, the light client does not really need to have epochs and authorities in the chain spec. All that's needed is a trusted finalized block hash. The rest should be recoverable from the state. |
It used by the light clients as a starting state. @tomaka should be better at explaining for what this is actually required. |
|
It's used by smoldot to start syncing from this checkpoint, on substrate only the code to export the sync state exists. This was written before warp sync was implemented on smoldot so maybe it's not super relevant anymore.
I think when this was written there was no easy way to get this data from the runtime, I agree that now if we still want to have a checkpoint for light clients to start syncing we only need to provide finalized block hash, we can even start warp syncing from such a block. Since this is used by smoldot currently I'll defer to @tomaka on what to do right now. |
|
The light sync state was "designed" and added before Grandpa warp sync existed. At the time, we were also trying as much as possible to avoid having to do runtime calls in the light client (as this makes the light client binary size much larger than otherwise, and requires downloading the runtime code from the network). For this reason, retrieving the Babe epoch and finalized authorities from the state wasn't really a good solution. Nowadays, yes we probably just need a finalized block hash. However, knowing the list of Grandpa authorities of this finalized block is necessary in order to verify the warp syncing. This list could also be retrieved from the network, but this adds some latency and most importantly we run the risk that all the nodes of the network have already pruned the block and are incapable of giving us that list. |
|
So, you would like to keep it @tomaka? |
|
Right now, this light sync state contains what a light client needs in order to start syncing from a certain block. We could remove the Babe-related stuff from the light sync state, but keep the Grandpa-related stuff, in which case the light sync state would contain what a light client needs in order to warp sync from a certain block. If we remove also the Grandpa-related stuff and keep only a finalized block header or hash, then we risk that everyone on the network has pruned the block, and/or that we make lots of useless network requests trying to find a node that hasn't. This is too much removal IMO. Smoldot can accommodate any of the three, I don't care, but I'd be leaning towards removing the Babe stuff. But keep in mind that having everything needed in order to immediately start syncing (without doing any warp sync) might also be desirable in the absolute. |
|
Okay, then I'm going with keeping it as is for now. |
|
bot merge |
|
Trying merge. |
ghost
merged commit * Substrate companion #9491 paritytech/substrate#9491 * update Substrate * Fix build Co-authored-by: parity-processbot <>
|
Could it be that a Cumulus companion is missing for this? |
necessary because of paritytech/substrate#9491
necessary because of paritytech/substrate#9491
necessary because of paritytech/substrate#9491
necessary because of paritytech/substrate#9491
necessary because of paritytech/substrate#9491
necessary because of paritytech/substrate#9491
* Substrate companion #9491 paritytech/substrate#9491 * update Substrate * Fix build Co-authored-by: parity-processbot <>
This decouples the light-sync state from chain spec. First, the
light-sync state currently only works with BABE+Grandpa, so not
all Substrate based chains can use this feature. The next problem was
also that this pulled the
sc-consensus-babeandsc-finality-grandpacrate into
sc-chain-spec.If a chain now wants to support the light-sync state, it needs to add
the
LightSyncStateExtensionto the chain spec as an extension. This isdocumented in the crate level docs of
sc-sync-state-rpc. If thisextension is not available,
SyncStateRpcfails at initialization.polkadot companion: paritytech/polkadot#3568
Fixes: #8886