Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

P2P clarifications when introducing engine_getBlobsV1 #3864

Open
wants to merge 21 commits into
base: dev
Choose a base branch
from

Conversation

tbenr
Copy link
Contributor

@tbenr tbenr commented Aug 2, 2024

This PR adds some clarifications in the P2P spec if engine_getBlobsV1 become a viable method to retrieve blobs.

Dictates that an honest validator, when successfully recover blobs for the current slot using local EL MUST:

  • republish the blob_sidecar over the p2p layer
  • make sure it updates all the data structures as if client had received the blob_sidecar over the gossip (ie update equivocation cache)

The basic idea is that, if engine_getBlobsV1 is used to recover missing blobs while following the head, it should behave like it has been received via gossip.
If it is used to retrieve recent past blobs the node could have missed, it should act as a byRoot RPC request (no republish)

@mkalinin
Copy link
Collaborator

mkalinin commented Aug 6, 2024

So the question is: how bad would be to mostly rely on EL mempool for data availability? what are the risks?

The risk is that nodes that aren’t connected to EL mempool or those that didn’t receive expected blobs on EL for any reason won’t be able to conclude DA. Probably, they will send ByRoot request and a remote peer will send those blobs retrieved via EL, but if neither remote peer has those blobs in the EL mempool then it will delay DA consideration. And we will likely see a split in attestations. I am not sure if any practical attack becomes possible because of that, but looks like heavily relying on EL mempool can increase attacking surface.

@jimmygchen
Copy link
Contributor

So the question is: how bad would be to mostly rely on EL mempool for data availability? what are the risks?

I think we can't just rely on the EL mempool for data availability. We would still require them to be broadcasted if the blob txs come from private mempools. Although viewing it as a complimentary thing is super useful:

  • Reduces block import latency: nodes can retrieve blobs from EL without waiting for them from gossip, hence making some blocks attestable earlier.
  • Improves network resiliency: being able to fetch blobs from EL reduces the likelihood of missed block due to delays in gossip propagation.
  • Allows further scaling without sacrificing decentralization: nodes with more resources could participate in sidecar computation and propagation, allowing nodes with limited bandwidth to continue to produce block post-PeerDAS.

We've implemented an experimental integration with this endpoint in Lighthouse, and it's proven to be quite reliable on testnets, and is probably the simplest and immediate solution to proposer bandiwdth problem after Pectra / PeerDAS.

@mkalinin
Copy link
Collaborator

mkalinin commented Oct 2, 2024

@tbenr what do you think about using “request from local execution layer client” in lieu of a specific Engine API method? The spec can mention a specific method as an example of a way of direct EL querying. In general, the spec is abstracted away from Engine API and its spec

@tbenr
Copy link
Contributor Author

tbenr commented Oct 2, 2024

you think about using “request from local execution layer client” in lieu

that makes sense!

specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Nice, looks great!

Co-authored-by: Mehdi AOUADI <mehdi.aouadi@gmail.com>
specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved
specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved
specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved
specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved
specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved
specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved
tbenr and others added 6 commits October 16, 2024 18:24
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for making those changes 🙏

@Scutua

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants