Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

portal-bridge: Serve light client bootstrap data #860

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

ogenev
Copy link
Member

@ogenev ogenev commented Aug 24, 2023

What was wrong?

Closes #849

How was it fixed?

For every slot, we request the latest finalized block root, if a new finalized block root is available, request and serve new LightClientBootstrap data.

To-Do

  • Add entry to the release notes (may forgo for trivial changes)
  • Clean up commit history

@ogenev ogenev force-pushed the beacon-bridge-bootstrap branch 4 times, most recently from 03c8efb to 11de912 Compare August 25, 2023 11:16
@ogenev ogenev marked this pull request as ready for review August 25, 2023 11:22
Copy link
Collaborator

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! How hard would it be to add a peertest scenario for serve_light_client_bootstrap()? If it's not too complex to mock out the http req/res to the pandaops node, there shouldn't be too much more to add to the full data lifecycle here

Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet, getting closer!

Feel free to ping me if I wasn't clear on any of the comments.

Comment on lines 169 to 171
async fn serve_light_client_bootstrap(
api: ConsensusApi,
portal_clients: Vec<HttpClient>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like portal_clients is only ever used as a reference below, so we can skip the clone before passing it in by converting the argument into a references, right?

Suggested change
async fn serve_light_client_bootstrap(
api: ConsensusApi,
portal_clients: Vec<HttpClient>,
async fn serve_light_client_bootstrap(
api: ConsensusApi,
portal_clients: &Vec<HttpClient>,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to wrap this inside an Arc because we want to share it between threads.

@@ -79,14 +82,22 @@ impl BeaconBridge {
async fn launch_latest(&self) {
// Current sync committee period known by the bridge
let current_period = Arc::new(AtomicUsize::new(0));
let finalized_block_root = Arc::new(Mutex::new(String::new()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a hunch we can get rid of the mutex, but I'm not sure how to explain it. The easiest way I could think of was to show an example with the Atomic, which is the same principle I think. See #873

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some thoughts on the PR.

Comment on lines 195 to 199
let mut content_bytes = ForkName::Capella.as_fork_digest().to_vec();
content_bytes.append(&mut bootstrap.as_ssz_bytes());

let content_value = BeaconContentValue::decode(&content_bytes)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something feels backwards about building the serialized value here just to throw it away for the decoded version. It seems more direct to add a new() so we can do something like:

Suggested change
let mut content_bytes = ForkName::Capella.as_fork_digest().to_vec();
content_bytes.append(&mut bootstrap.as_ssz_bytes());
let content_value = BeaconContentValue::decode(&content_bytes)?;
let content_value = BeaconContentValue::LightClientBootstrap(ForkVersionedLightClientBootstrap::new(bootstrap));

Since the bootstrap is typed with the fork, it seems like we can match on it inside new to determine the fork.

As a side-benefit, this code block becomes agnostic to the ssz encoding, which seems like what we want.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding From<LightClientBootstrapCapella> for ForkVersionedLightClientBootstrap did the trick, we can then use .into() instead of new().

@ogenev ogenev force-pushed the beacon-bridge-bootstrap branch from 11de912 to 055b2d6 Compare September 4, 2023 09:56
@ogenev ogenev force-pushed the beacon-bridge-bootstrap branch from 055b2d6 to a9413c6 Compare September 4, 2023 11:16
@ogenev ogenev merged commit 7af5b97 into ethereum:master Sep 4, 2023
@ogenev ogenev deleted the beacon-bridge-bootstrap branch September 4, 2023 15:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

portal-bridge: Add LightClientBootstrap support for beacon bridge
3 participants