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

Remove blob signing #369

Merged
merged 15 commits into from
Nov 9, 2023
Merged

Conversation

realbigsean
Copy link
Contributor

Changes for ethereum/consensus-specs#3531

  • update the BlobSidecar struct in the GET blob_sidecars endpoint
  • remove SignedBlobSidecar type
  • use BlindedBeaconBlock structs rather than BlindedBlockContents structs
  • update BlockContents to wrap a BeaconBlock, an array of KZGCommitment's an array of KZGProof's, and an array of Blob's

My thinking here is we want to maintain a stateless flow (the unblinded flow) so we will have to pass along blobs to the VC. And in this case I think we might as well pass along proofs so they don't need to be recomputed. In the stateful (blinded) flow we only need to pass along the object we're signing, so I think it can be kept maximally simple. The downside is it's less intuitive which types should be expected when on an endpoint like /eth/v3/validator/blocks and what the types represent exactly.

The API changes here imply this block production flow:

Stateless

  • BN gets payload, commitments, proofs, blobs from the EL
  • BN assembles a block using the commitments and payload
  • VC receives the block, blobs, and proofs from the BN
  • VC Signs the block and returns everything
  • BN computes the commitment inclusion proof, assembles the sidecars, publishes

Stateful (blinded)

  • BN gets payload, commitments, proofs, blobs from the EL
  • BN assembles a block using the commitments and payload
  • BN caches payload, commitments, proofs, blobs
  • VC receives the blinded block
  • VC signs and returns the blinded block
  • BN unblinds the block, computes the commitment inclusion proof, assembles the sidecars using cached proofs and blobs, publishes

Copy link
Member

@ralexstokes ralexstokes 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 from a high-level review

@@ -6,83 +6,27 @@ Deneb:
minItems: 0
maxItems: 6

CommitmentInclusionProof:
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we use KZGCommitment elsewhere, should we refer to this as KZGCommitmentInclusionProof or is this considered more generic?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated field name to kzg_commitment_inclusion_proof ethereum/consensus-specs@4a609ce

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated both the struct name and field name here: e9d0538

@@ -76,14 +76,14 @@ get:
example: "12345"
data:
oneOf:
- $ref: '../../beacon-node-oapi.yaml#/components/schemas/BeaconBlock'
- $ref: "../../beacon-node-oapi.yaml#/components/schemas/BeaconBlock"
- $ref: "../../beacon-node-oapi.yaml#/components/schemas/Altair.BeaconBlock"
- $ref: "../../beacon-node-oapi.yaml#/components/schemas/Bellatrix.BeaconBlock"
- $ref: "../../beacon-node-oapi.yaml#/components/schemas/Bellatrix.BlindedBeaconBlock"
- $ref: "../../beacon-node-oapi.yaml#/components/schemas/Capella.BeaconBlock"
- $ref: "../../beacon-node-oapi.yaml#/components/schemas/Capella.BlindedBeaconBlock"
- $ref: "../../beacon-node-oapi.yaml#/components/schemas/Deneb.BlockContents"
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this blockContents too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where it's a little confusing and asymmetric. BlockContents is still used in the unblinded flow, because in order to keep it stateless we need to pass blobs along with the block back and forth. So this endpoint can return Deneb.BlockContents or Deneb.BlindedBeaconBlock

Copy link
Contributor

Choose a reason for hiding this comment

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

i would prefer a separate call by validator to pull blobs if it plans to reroute the publish since the data transfer is non trivial and will also resolve code spaghetti in the api, to address latency, validator call concurrently pull blobs while its signing the block

Copy link
Contributor

Choose a reason for hiding this comment

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

That would not meet the requirement for statelessness, as it would be possible for the beacon node to exit between returning the data in this call and a separate request being made for the blobs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i would prefer a separate call by validator to pull blobs if it plans to reroute the publish since the data transfer is non trivial and will also resolve code spaghetti in the api, to address latency, validator call concurrently pull blobs while its signing the block

I'm not sure this can be done without reintroducing the state requirement. Potentially we could set up an avenue for BNs to query ELs to check their local mempool for blobs matching a commitment, but I don't think this is really a great solution either. It might be better to mitigate the expense of transferring blob data by preferring SSZ transfer or preferring the blinded flow even when not using an external builder (this does put you at risk of missing a block proposal if you are using fallback between BNs however).

Copy link
Contributor

Choose a reason for hiding this comment

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

theoretically, the VC doesn't even need access to the blobs. it can just sign the block and let the blob be handled by the BN, as in, creating the BlobSidecar from SignedBeaconBlock + KzgProofs / Blobs

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it needs access, because the BN that they are sent to may not have the blobs anymore, if they were originally fetched from a different one.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, thats the idea of statelessness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v2 was only created for deneb... if its causing us to keep a container we don't need, thats sub-optimal...

right I think I missed your point. Agree we should deprecate V2

Copy link
Member

Choose a reason for hiding this comment

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

Just wanted to mention that keeping the blobs in the unblinded flow response makes it really easy to update our testing tools for the blob p2p scenarios in testnets.

$ref: "./block.yaml#/Deneb/BlindedBeaconBlock"
blinded_blob_sidecars:
$ref: "./blob_sidecar.yaml#/Deneb/BlindedBlobSidecars"
kzg_proofs:
Copy link
Contributor

@g11tech g11tech Nov 3, 2023

Choose a reason for hiding this comment

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

if we are assuming statelessness, are we requiring here for the beacon to build inclusion proofs for the blobs before transmitting them (since this is what validator is fetching)

Copy link
Contributor

Choose a reason for hiding this comment

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

The validator can compute them on its own, the inclusion proof can be computed solely from block.

Copy link
Contributor

Choose a reason for hiding this comment

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

correct however what i was clarifying that since validator publishes signed block contents, the intercepting beacon is would be building the proofs and publishing blocks (which is fine or may be good as well that while validator is signing the beacon can build the proofs and patch them in as validator starts publish flow although just a matter of 20-30ms as such)

(this comment is about SignedBlockContents)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking in implementation, a beacon node could start building the inclusion proofs and cache them when it builds a block. But should also have the ability to compute them on the fly (on a cache miss). So in the case where you publish through the BN building the block it'd be faster.

Copy link
Contributor

Choose a reason for hiding this comment

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

It takes milliseconds to compute those Merkle proofs, as they are solely rooted inside BeaconBlock (~2 MB structure, and no need to do expensive BeaconState reconstruction). no need to optimize for this, especially while there are still clients that use JSON instead of SSZ encoding to access these endpoints.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, lodestar will soon be transition to SSZ now lol

kzg_proofs:
Copy link
Contributor

Choose a reason for hiding this comment

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

similar comment as above

items:
$ref: '../primitive.yaml#/KZGProof'
minItems: 0
maxItems: 6
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use the theoretical max here, MAX_BLOB_COMMITMENTS_PER_BLOCK, for stable SSZ transport if the number changes in future

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for Blobs

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 considering ssz transport

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

Copy link
Contributor Author

@realbigsean realbigsean Nov 6, 2023

Choose a reason for hiding this comment

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

updated! 9a7c8b6

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
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
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.

i am onboard with the PR with a change that @etan-status proposed to keep the max length to MAX_BLOB_COMM... considering ssz transport encoding

@realbigsean
Copy link
Contributor Author

Ok I think I've addressed the feedback so far

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

@rolfyone
Copy link
Collaborator

rolfyone commented Nov 8, 2023

sorry for the less interesting bit, but changelog entry :)

Copy link
Contributor

@etan-status etan-status 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, my understanding is as follows:


Regular flow

  1. BN1 -- BeaconBlockContents --> VC (with blobs + kzg_proofs)
  2. VC -- SignedBeaconBlockContents --> BN2 --> libp2p (with blobs + kzg_proofs as is from VC to BN, just re-upload)

BN1 may be different from BN2


Blinded flow

  1. MEV relay -- BlindedBeaconBlock --> BN --> VC
  2. VC -- SignedBlindedBeaconBlock --> BN --> MEV relay
  3. MEV relay -- SignedBeaconBlockContents --> BN --> libp2p

@mcdee
Copy link
Contributor

mcdee commented Nov 8, 2023

@etan-status in your blinded flow part 1, the MEV relay currently provides an execution payload rather than an entire blinded beacon block.

With the advent of deneb this should change to the execution payload plus an array of blinded blob info. I think the blob info should be the blob roots plus the KZG commitments and KZG proofs, but would like to see if others on here concur.

@etan-status
Copy link
Contributor

etan-status commented Nov 8, 2023

ah, right. but from BN --> VC the executionpayload is then combined with the beacon block, so the the VC it looks the same, regardless.

Update:

  1. MEV relay -- blindedexecutionpayload --> BN -- BlindedBeaconBlock --> VC
  2. VC -- SignedBlindedBeaconBlock --> BN --> MEV relay
  3. MEV relay -- SignedBeaconBlockContents --> BN --> libp2p

@etan-status
Copy link
Contributor

No need for KZG proofs / blob roots. there is no more signature on the blobs.

@etan-status
Copy link
Contributor

And, in blinded flow, VC has to use the same MEV relay, so the same BN, to complete the flow. it is only in the unblinded flow where a different BN may be selected for publishing.

@mcdee
Copy link
Contributor

mcdee commented Nov 8, 2023

No need for KZG proofs / blob roots. there is no more signature on the blobs.

Okay, so just the KZG commitments to place inside the block body.

@etan-status
Copy link
Contributor

the hash_tree_root(kzg_commitments) would be sufficient. otherwise, there is not really blinding because the BN can identify what blobs were included by the relay, if they also observed the mempool.

@mcdee
Copy link
Contributor

mcdee commented Nov 8, 2023

And, in blinded flow, VC has to use the same MEV relay, so the same BN...

What's the restriction here? Is the BN now holding state that must be passed to the MEV relay, or is it just the same condition at current that the BN knows which relay to talk to in order to carry out the unblinding?

@etan-status
Copy link
Contributor

The latter. The BN knows the identity of the MEV relay who sent the bid, and also maintains the validator registration with that relay.

@mcdee
Copy link
Contributor

mcdee commented Nov 8, 2023

the hash_tree_root(kzg_commitments) would be sufficient. otherwise, there is not really blinding because the BN can identify what blobs were included by the relay, if they also observed the mempool.

But BlindedBeaconBlock has the full KZG commitments in its definition.

Also, blinding is really to avoid leaking bundles that could be picked apart, so not sure that knowing which blob transactions are in a block is an issue.

@etan-status
Copy link
Contributor

Sure, just saying, the protocol would work perfectly well even if BlindedBeaconBlock only contained hash_tree_root(kzg_commitments). If blinding doesn't try to hide the blob transactions, that's fine (but also kinda pointless) to include the full kzg_commitments in BlindedBeaconBlock.

Copy link
Collaborator

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM

@rolfyone rolfyone merged commit c097f1a into ethereum:master Nov 9, 2023
2 checks passed
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.

8 participants