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

Drop "blinded blob sidecar" concept after using signed header to authenticate blobs #90

Merged
merged 5 commits into from
Nov 8, 2023

Conversation

ralexstokes
Copy link
Member

Following ethereum/consensus-specs#3531, we can make some strong simplifications to the builder-specs which this PR implements.

Changes to getHeader:
The relay must still provide KZGCommitments in a signed bid but they no longer need to pass along proofs or blob roots.

Changes to getPayload:
The relay simply needs the SignedBlindedBeaconBlock (as per the status quo) and can still assemble the unblinded beacon block. The relay will still reveal the execution payloads alongside the full "blob bundle".

Open questions:
The simplification from the linked consensus-specs PR requires blob gossip to include a short Merkle proof from the signed header's body root to each KZG commitment it includes. This PR assumes the relay will do this work independently although we could demand the proposer sends these along as well with the SignedBlindedBeaconBlock.

Pros: less computation for relays (and less code to debug/maintain)
Cons: demands consensus clients have done this work ahead of time (they may wait until after selecting the remote payload as the blob sidecar gossip can come after)

@ralexstokes ralexstokes force-pushed the deneb-sidecar-updates branch 2 times, most recently from 9863ff4 to f92a222 Compare October 30, 2023 21:22
@avalonche
Copy link
Contributor

As the relay will need to verify proposer inputs anyways it seems like relays will need to do the same amount of compute with more work on the consensus clients.

@ralexstokes
Copy link
Member Author

As the relay will need to verify proposer inputs anyways it seems like relays will need to do the same amount of compute with more work on the consensus clients.

there is a small optimization here where the beacon node (who can optimistically compute the inclusion proofs) can send them to the relay, so that the relay only has to do verification (and not compute and verify themselves)

the question comes down to: are relays ok to maintain this code to compute the proofs? or should they rely on beacon nodes to compute and send along with the signed header?

@ralexstokes ralexstokes force-pushed the deneb-sidecar-updates branch 2 times, most recently from 4af7e69 to cd58f35 Compare November 2, 2023 15:20
@ralexstokes
Copy link
Member Author

linter is failing as it depends on ethereum/beacon-APIs#369, will update once that PR is merged

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm

@potuz
Copy link

potuz commented Nov 2, 2023

I'd like to get a little more discussion about this design. I can see clear tradeoffs between the proposer doing it or the relay doing it. On the one hand this reduces code complexity on the relay and adds it to the proposer that presumably is running a more vetted software. On the other hand it adds latency to block production as now the proposer needs to send back the proofs. The relay still needs to assemble the sidecar with the actual blob contents. If we were to have the relay assembling the sidecars beforehand, it can start building most of the proof (out of the 17 entries in the proof, only the last four require entries from the Beacon Block Body). So most of the work can be done before the blinded beacon block comes back.

Even if we were to force the proposer to send the proofs, an intermediate step that would alleviate the extra data on the wire is to send just the last four roots in the proof and let the relay compute the remaining 13.

Given the above it seems to me that it's more beneficial for the network as a whole to have the relay building the proofs and not the proposers.

@realbigsean
Copy link

Can the proposer start computing the proofs and cache them while the validator is signing the block?

@potuz
Copy link

potuz commented Nov 2, 2023

The proposer can, but the problem is sending them over the wire. These are 3264 bytes for 6 blobs

@ralexstokes
Copy link
Member Author

ralexstokes commented Nov 2, 2023

@potuz this is a valid solution but it does complicate things even more than "proposer sends full proofs" or "relay and proposer both compute them independently"

im not convinced the additional latency is going to matter that much as proof creation should be quite cheap and like @realbigsean is saying either or both parties can pipeline proof creation as an optimization

I would need to see a prototype demonstrating the latency gain is enough to warrant the more complex (especially conceptually) solution you are proposing. The latest change here has the proposer compute the proofs in fact to make things as simple as possible for the relay.

Also worth noting that while block/blob gossip is latency-sensitive, the relay (if they are really that worried) can implement these optimizations and in either case they will get a head start on gossip relative to the proposer so there isn't any real latency gain from the network-global perspective

@potuz
Copy link

potuz commented Nov 2, 2023

What I'm proposing is that the relay does the full proof so that the proposer doesn't incurr in any extra network latency. The argument about the majority of the proof being computable by the relay before getting any signature from the proposer is just to exemplify this. If instead of counting the 17 roots you count only those 13, the proposer will be sending 2496 extra bytes that could have been completely computed by the relay before even sending the bid

@realbigsean
Copy link

26,112 bits / 10Mbps = 2.6ms . That on top of the optimization @potuz is suggesting seems significant. It also does make the API a bit simpler. As far as relay complexity, all clients will have code for the proof computation implemented so the methods should be pretty widely available. It gets a bit complex if they actually implements potuz optimization. but overall it does seem worth doing. Are we able to get feedback from people actually running relays?

@g11tech
Copy link
Contributor

g11tech commented Nov 3, 2023

not in favor of partial optimizations to save a few ms over http, so latency isn't a factor here imo and we should either let all proofs come from beacon or all proofs generated by the relay

i prefer beacon but again not so hard bend on this. if proof generation by relay has the benefit that it can transmit block as soon as it sees it but again with the cpus we have today, i don't think its gonna make any diff

@avalonche
Copy link
Contributor

agree that the optimisations of a few ms doesn't seem significant as there are better optimisations available (like this issue with 200-300ms improvements) and proofs should either all be computed on the relay or from the beacon node.
Are there examples / libraries on how to implement the optimisation @potuz mentioned? From the perspective of relay code complexity it shouldn't be too much of an issue to include proof creation but not the more complex optimisation proposed.

specs/deneb/builder.md Outdated Show resolved Hide resolved
specs/deneb/builder.md Outdated Show resolved Hide resolved
items:
$ref: '../../beacon-apis/types/deneb/blob_sidecar.yaml#/KZGCommitmentInclusionProof'
minItems: 0
maxItems: 6

Choose a reason for hiding this comment

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

the maxItems should be the theoretical max: MAX_BLOB_COMMITMENTS_PER_BLOCK, for stable SSZ transfer

Copy link
Member Author

Choose a reason for hiding this comment

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

going to have relay compute independently, so not a concern anymore

etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Nov 4, 2023
`BlobSidecar` is no longer signed, instead use Merkle proof to link
blobs with block.

- ethereum/consensus-specs#3531

Associated beacon-API / builder-specs still TBD; minimal changes done
to compile in similar style to previous spec, but not standardized yet.

- ethereum/beacon-APIs#369
- ethereum/builder-specs#90
@potuz
Copy link

potuz commented Nov 4, 2023

agree that the optimisations of a few ms doesn't seem significant as there are better optimisations available

It doesn't make much sense for the proposer to get the kzg commitments, then compute the proofs and then send them back when the relay has everything it needs to start computing this before it even sends the list to the proposer. The design is simply much cleaner if the relay is in charge of this.

It is also hard for the proposer to parallelize this since at the same time it will be computing proofs for its local block (together with all the other tasks when proposing). And as a plus is better forward looking for ePBS when the kzg commitments won't even be in the block and the builder will have to include these proofs in the payload.

@ralexstokes
Copy link
Member Author

yeah i think this is reasonable, i'll roll back to the original state with independent computation later today

and we will be sure to call out the relevant code for the relays and make sure they can pass test vectors

@ralexstokes
Copy link
Member Author

@avalonche @g11tech @potuz @realbigsean @etan-status

I've made the relevant updates and think this is ready to merge in the next day or two

I'll follow back up with some pointers on how relays should compute and verify KZG commitment inclusion proofs in another PR

@g11tech
Copy link
Contributor

g11tech commented Nov 5, 2023

Considering ePBS then it makes sense for relay to do the compute 💪

@potuz
Copy link

potuz commented Nov 6, 2023

Can't approve it cause I'm out of my depth with regards to APIs, but this looks good to me!

etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Nov 6, 2023
`BlobSidecar` is no longer signed, instead use Merkle proof to link
blobs with block.

- ethereum/consensus-specs#3531

Associated beacon-API / builder-specs still TBD; minimal changes done
to compile in similar style to previous spec, but not standardized yet.

- ethereum/beacon-APIs#369
- ethereum/builder-specs#90
minItems: 0
maxItems: 6


BlobsBundle:
allOf:
- $ref: '#/Deneb/BlobsBundleCommon'
Copy link
Contributor

Choose a reason for hiding this comment

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

small cleanup: i think BlobsBundleCommon would not be required, just BlobsBundle

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I agree, I think this was added in an earlier rev where the factoring made more sense

I can drop it

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm

types/deneb/bid.yaml Outdated Show resolved Hide resolved
@ralexstokes ralexstokes merged commit b7dd53f into ethereum:main Nov 8, 2023
3 checks passed
@ralexstokes ralexstokes deleted the deneb-sidecar-updates branch November 8, 2023 14:08
etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Nov 13, 2023
The `BlobSidecar` construction has been moved to the relay and is no
longer done by the BN / VC in blinded flow. Builder bid contents have
been shrinked from full `BlindedBlobBundle` to `blob_kzg_commitments`.

- ethereum/builder-specs#90
- ethereum/beacon-APIs#369
etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Nov 16, 2023
The `BlobSidecar` construction has been moved to the relay and is no
longer done by the BN / VC in blinded flow. Builder bid contents have
been shrinked from full `BlindedBlobBundle` to `blob_kzg_commitments`.

- ethereum/builder-specs#90
- ethereum/beacon-APIs#369
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants