Persist light client updates #5545
Conversation
| .ok_or(BeaconChainError::DBInconsistent(format!( | ||
| "Attested state not available {:?}", | ||
| attested_block.state_root() | ||
| )))?; |
There was a problem hiding this comment.
This not acceptable w.r.t performance. You should extend the use of cached_parts such that you never have to get states from the store in the happy path. The function to generate updates in LightClientUpdate::new should not be used.
See this document for an implementation suggestion https://hackmd.io/@dapplion/BkxPbc6eQT
…rsist-light-client-updates
…rsist-light-client-updates
There was a problem hiding this comment.
Overall looks good! Two points:
We should run the specs tests for the is_better_update function, see here https://github.com/ethereum/consensus-specs/blob/dev/tests/formats/light_client/update_ranking.md
The function under which updates are computed only gets triggered for recent blocks. See
lighthouse/beacon_node/beacon_chain/src/beacon_chain.rs
Lines 3944 to 3948 in ffe29c0
Now that the LC server computes recent updates AND full updates, the latter should include historical updates. Therefore we should be producing updates during forward range sync. Also for backfill sync, but that's a feature for another PR :)
| @@ -152,6 +197,84 @@ impl<T: BeaconChainTypes> LightClientServerCache<T> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| pub fn store_light_client_update( | |||
There was a problem hiding this comment.
You can inline this function above
let should_persist_light_client_update = if let Some(prev_light_client_update) = prev_light_client_update {
is_better_light_client_update(
&prev_light_client_update,
&new_light_client_update,
chain_spec,
)?
} else {
true
} | @@ -140,6 +151,40 @@ impl<T: BeaconChainTypes> LightClientServerCache<T> { | |||
| signature_slot, | |||
| chain_spec, | |||
| )?); | |||
|
|
|||
| let new_light_client_update = LightClientUpdate::new( | |||
There was a problem hiding this comment.
I think you should not condition creating new LightClientUpdate on having a non-zero finalized_block_root. If the chain does not finalize for a full sync committee period we still need to produce them. Your is_better_light_client_update implementation covers the case of finalized vs non-finalized updates and will prioritize the finalized ones.
LightClientUpdate should be produced anytime we may be able to produce a better update
| // Implements spec prioritization rules: | ||
| // Full nodes SHOULD provide the best derivable LightClientUpdate for each sync committee period | ||
| // ref: https://github.com/ethereum/consensus-specs/blob/113c58f9bf9c08867f6f5f633c4d98e0364d612a/specs/altair/light-client/full-node.md#create_light_client_update | ||
| fn is_better_light_client_update<E: EthSpec>( |
There was a problem hiding this comment.
Should we move this implementation to the LightClientUpdate type?
Issue Addressed
Partially #3651
Proposed Changes
This PR aims to persist light client updates. Once persisted, this will allow lighthouse nodes to serve them over RPC.