Add blob signing endpoints #302
Conversation
Co-authored-by: 0xGabi <[email protected]> Co-authored-by: Paul Harris <[email protected]>
Consensus-spec references may need to be fixed once deneb is renamed in that project. related to ethereum/consensus-specs#3207
…this back to bellatrix transactions like the other similar references.
|
Probably need to add the new endpoint to the top level |
|
The top level |
beacon-node-oapi.yaml
Outdated
| @@ -89,6 +89,8 @@ paths: | |||
| $ref: "./apis/beacon/blocks/blinded_blocks.yaml" | |||
| /eth/v1/beacon/blocks: | |||
| $ref: "./apis/beacon/blocks/blocks.yaml" | |||
| /eth/v1/beacon/blob_sidecar: | |||
There was a problem hiding this comment.
do you think we should make this /eth/v1/beacon/blinded_blob_sidecar?
|
ethereum/consensus-specs#3244 is now ready for review - it would be nice to leave this PR open for a bit until that has been merged and we've had time to evaluate the impact of blinded-only blobs. |
apis/beacon/blobs/post_blobs.yaml
Outdated
| Instructs the beacon node to broadcast a newly signed blob to the beacon network, | ||
| to be included in the beacon chain. A success response (20x) indicates that the blob | ||
| passed gossip validation and was successfully broadcast onto the network. | ||
| The beacon node is also expected to integrate the blob into state once it receives the |
There was a problem hiding this comment.
The description here needs to be changed. Blobs are not expected to be integrated into state.
apis/beacon/blobs/post_blobs.yaml
Outdated
| "202": | ||
| description: "The blob could not be integrated into the beacon node's database as it failed validation, but was successfully broadcast." | ||
| "400": | ||
| description: "The `SignedBlobSidecar` object is invalid and could not be broadcast" |
There was a problem hiding this comment.
Shouldn't be SignedBlindedBlobSidecar instead of SignedBlobSidecar?
There was a problem hiding this comment.
i guess description is talking about constructed object
|
A curiosity. What's the rationale behind having separate I can image a more streamlined flow in VC which is (remote)sign and publish for each Sidecar. But I can still see benefits of having aggregated methods for remote-signing and publishing: it better scale with increasing number of blobs. |
I'm guessing it's for the same reason as separating blobs from the block request - so that it can be sent to the BN and lets it starts publishing to gossip more quickly. Although I wonder if it still provide the same benefit if we're sending blinded blobs between VC and BN. Does it still make sense to even separate block and blobs if we go with blinded blobs? |
On the proposer side, blobs are available immediately, all together. When VC post them all (blindly signed) to BN, bn can gossip them all together in parallel. The only problem i see is that the return code for the publish will (maybe) take in consideration partial failures. So we exchange a single call with more complex error handling\retry from the VC side. Same consideration applies for remote signing (web3signer) all blinded blobs with one call. |
Yeah, I honestly don't have a sense for whether a batch or individual requests would be faster and at what number of blobs. It made intuitive sense to me to view this endpoint as a "gossip publisher" in which case having it mirror the gossip topic's contents made sense. Open to changing this. As far as how blinding impacts this, we can blind and cache in the beacon node per-blob, so I'm not sure I see how that impacts whether to publish them separately. Would keeping them together in blinding be a reduction in complexity or faster? |
I agree the error handling gets more complex, the 200 vs 202 response codes end up having different meanings here |
|
Another consideration is the "stickiness" required by blinded flows. I mean, when a VC can use multiple BNs, the publish calls must end on the same BN that handled the block/blobs production. Different BNs wont be able to unblind them. (I don't know if any clients has implemented a shared cache between different BN "clusters", teku didn't). So having multiple calls required to complete the entire flow become more exposed to eventual failover in the middle of the flow. Maybe @mcdee might have opinions on these topics. |
I think that's definitely a very strong point against separating the calls if a shared cache is required between BNs. Although I'm not fully grasping why different BNs can't unblind them? |
|
Where are we at with this? Can we fix conflicts and think about merging? |
… add-blob-signing-endpoints
|
Fixed the conflicts @rolfyone I'm still interested it people have thoughts on this: #302 (comment) But that's it from my POV |
|
add an entry in the changes for one :) |
This reverts commit f0ffbb8.
Resolves #300
Notes
KZGProofandBlobto primitivesGETrequest for block and blobs and a separatePOSTrequest for each signed blobQuestions
POSTrequests. I'm not sure how we should handle this. We could keep the existing meaning for each, it will just mean most requests return a 202 and only the final block/blob message for a slot leads to import and returns a 200.BlockContentsstruct for SSZ serialization. This struct is solely for communication here and not used in consensus elsewhere. Should the SSZ definition be included here? Should we add some similar type's definition to the consensus spec? Can we describe this SSZ type in terms of spec objects? (e.g.,SignedBeaconBlockwithBlobSidecarsappended?)/validator/blockendpoints to return a new type only for deneb (BlockContents) ?