feat: update chain_head#1836
Conversation
KolbyML
left a comment
KolbyML
left a comment
There was a problem hiding this comment.
Why are the process_*_update() functions triggered by both the database updates from the LightClient and the BeaconNetworkStorage I understand why historical_summaries is only done in BeaconNetworkStorage, I am confused what the reason for having process_optimistic_update() and process_finality_update() triggered by both is
Other then that the PR looks pretty good, I am just curious on a few details
| portalnet_config.clone(), | ||
| storage_config_factory.create(&Subnetwork::Beacon, Distance::MAX)?, | ||
| header_oracle.clone(), | ||
| chain_head.clone(), |
There was a problem hiding this comment.
| chain_head.clone(), | |
| chain_head, |
do we need clone here?
| let mut watch_receivers = client.get_light_client_watch_receivers().await; | ||
| let mut beacon_client = beacon_client_clone.lock().await; | ||
| *beacon_client = Some(client); |
There was a problem hiding this comment.
| let mut watch_receivers = client.get_light_client_watch_receivers().await; | |
| let mut beacon_client = beacon_client_clone.lock().await; | |
| *beacon_client = Some(client); | |
| let mut watch_receivers = client.get_light_client_watch_receivers().await; | |
| *beacon_client_clone.lock().await = Some(client); |
shouldn't this lock be scoped so it is dropped? currently this mutex would never drop due to the loop {} below. where as the lock used to instantly be unlocked, due to the object being dropped, which is no longer the case
We could use {} to scope it, or manually drop the object, but any of these 3 solutions should work
There was a problem hiding this comment.
You are right, I missed this. Thanks
| if self.store.optimistic_header.slot == update.attested_header.beacon.slot | ||
| || self.store.finalized_header.slot == update.finalized_header.beacon.slot | ||
| { | ||
| self.watch_senders | ||
| .update | ||
| .send_replace(Some(LightClientUpdate::Electra(update.clone()))); | ||
| } |
There was a problem hiding this comment.
This feels weird, I get why we have to do this because apply_generic_update doesn't get the actual update, it feels non-intuitive at first glance though
There was a problem hiding this comment.
I modified the condition, and hopefully made it cleaner and more correct. WDYT?
| let generic_update = GenericUpdate::from(update); | ||
| self.apply_generic_update(&generic_update); | ||
| if self.store.optimistic_header.slot == update.attested_header.beacon.slot | ||
| || self.store.finalized_header.slot == update.finalized_header.beacon.slot | ||
| { | ||
| self.watch_senders | ||
| .finality_update | ||
| .send_replace(Some(LightClientFinalityUpdate::Electra(update.clone()))); | ||
| } |
There was a problem hiding this comment.
| let generic_update = GenericUpdate::from(update); | |
| self.apply_generic_update(&generic_update); | |
| if self.store.optimistic_header.slot == update.attested_header.beacon.slot | |
| || self.store.finalized_header.slot == update.finalized_header.beacon.slot | |
| { | |
| self.watch_senders | |
| .finality_update | |
| .send_replace(Some(LightClientFinalityUpdate::Electra(update.clone()))); | |
| } | |
| let generic_update = GenericUpdate::from(update); | |
| self.apply_generic_update(&generic_update); | |
| if self.store.optimistic_header.slot == update.attested_header.beacon.slot | |
| || self.store.finalized_header.slot == update.finalized_header.beacon.slot | |
| { | |
| self.watch_senders | |
| .finality_update | |
| .send_replace(Some(LightClientFinalityUpdate::Electra(update.clone()))); | |
| } |
what is the reasoning in if self.store.optimistic_header.slot == update.attested_header.beacon.slot here for the LightClientFinalityUpdate one?
There was a problem hiding this comment.
I'm not sure I understand the question. The LightClientFinalityUpdate can update the optimistic header as well.
Ideally, it shouldn't be needed. But because of the several reasons (see #1815), the light-client isn't aware of every update that is gossiped to us (hence, called from I do plan to clean this up as process of working on #1815, but that feels like slightly lower priority at the moment. |
KolbyML
left a comment
KolbyML
left a comment
There was a problem hiding this comment.
PR looks fine, wouldn't our code work without the lightclient changes though. Ideally the LightClient shouldn't be making FindContent requests on the network, and should only be listening to P2P recieved on beacon. If Beacon is always getting and storing the latest Finalized and Optimistic update, I don't get the point in listening from the light client end, but I don't think it is worth blocking this PR over that.
Yes, I agree. But currently that's not how it works. As I said, working on #1815 will improve this.
At the moment, it's not. We need to improve several thing on the beacon side for this to be reliable (part of the #1815 ) |
2 participants
What was wrong?
The ChainHead wasn't being updated
How was it fixed?
Now we update the ChainHead whenever:
Future work:
To-Do