PeerDAS - cell proof computation changes #516
Conversation
types/fulu/block_contents.yaml
Outdated
| properties: | ||
| block: | ||
| $ref: "./block.yaml#/Fulu/BeaconBlock" | ||
| cell_proofs: |
There was a problem hiding this comment.
How do people feel about keeping the kzg_proofs field name? Not clear to me how much we gain from this rename, and given the beacon APIs are fork aware, this change doesn't feel necessary.
0x00101010
requested review from
dapplion,
jimmygchen,
jtraglia,
mcdee and
nflaig
types/fulu/execution_payload.yaml
Outdated
| $ref: "../bellatrix/transactions.yaml#/Bellatrix/Transactions" | ||
| withdrawals: | ||
| $ref: "../capella/withdrawals.yaml#/Capella/Withdrawals" | ||
| proof_version: |
There was a problem hiding this comment.
is this the only field that actually changes compared to Electra which requires to update BeaconBlockBody? I can't find that in the CL spec
There was a problem hiding this comment.
Yes, this is the only field change, also cc @jtraglia to see if we want to add that to CL spec
There was a problem hiding this comment.
also cc @jtraglia to see if we want to add that to CL spec
that would be good otherwise I am not sure how to merge this as beacon-api generally follows the consensus spec, but we can merge this soon, just wanna do another minor release for Electra before that in a few days
There was a problem hiding this comment.
Ah sorry just now seeing this. I will make a PR for this.
There was a problem hiding this comment.
@0x00101010 why do we have the proof version here?
the proof version byte is just to disambiguate the EL wrapper, i don't see where it leaks into the CL at all
There was a problem hiding this comment.
I've removed Fulu BeaconBlock / SignedBeaconBlock / ExecutionPayload, since it does not change, we can reuse the Electra definition
|
Do we need |
No we don't need it, I just wanna focus the discussion for now on PeerDAS required changes, so I tried to keep my review minimal for now, can simplify this before merging |
There was a problem hiding this comment.
Also need fulu here:
beacon-APIs/beacon-node-oapi.yaml
Line 353 in 1eda435
There are a few endpoints that lists the consensus versions, perhaps we could update them to use this enum too, to avoid having to update it everywhere?
beacon-APIs/apis/beacon/blocks/attestations.v2.yaml
Lines 26 to 29 in 1eda435
beacon-APIs/apis/beacon/blocks/attestations.v2.yaml
Lines 26 to 29 in 1eda435
beacon-APIs/apis/debug/state.v2.yaml
Line 29 in 1eda435
beacon-APIs/apis/validator/block.v3.yaml
Line 92 in 1eda435
| $ref: './types/electra/consolidation.yaml#/Electra/PendingConsolidation' | ||
| Electra.PendingPartialWithdrawal: | ||
| $ref: './types/electra/withdrawal.yaml#/Electra/PendingPartialWithdrawal' | ||
| Fulu.BlockContents: |
There was a problem hiding this comment.
I think we also need a Fulu.BlobSidecars for the getBlobSidecars endpoint? Could be in a separate PR though.
There was a problem hiding this comment.
Yeah, since max blob number is not determined, prefer to do it later
There was a problem hiding this comment.
Blob count is not the only thing that changes - each blob sidecar has a KzgProof field that should be changed to plurals with cell proofs. I reckon we could set a high but realistic blob count for the current PeerDAS, to avoid having to update this endpoint on every BPO hard fork?
There was a problem hiding this comment.
cool, will update it tomorrow with other changes
There was a problem hiding this comment.
I think we also need a Fulu.BlobSidecars for the getBlobSidecars endpoint? Could be in a separate PR though.
or we could deprecate the current one and at a new getDataColumnSidecars? I feel like this is something consumers should give feedback on before doing something, afaik, L2 clients use those apis
There was a problem hiding this comment.
I think we'd probably want getBlobsSidecarV2 for blob consumers (L2s). DataColumnSidecars is unlikely useful to end users - it's a CL-specific structure used for sample distribution and requires extra effort to convert into blobs, which L2 users are unlikely interested in. There could be use cases I'm missing though.
There was a problem hiding this comment.
I think we'd probably want getBlobsSidecarV2 for blob consumers (L2s)
how would a CL be able to reconstruct the blob if it's not a supernode? and is there any reason why we need a v2 api, the current api is fork-aware so can change the type if required
There was a problem hiding this comment.
Let's kick off the discussion on getBlobsSidecar/V2 api, let's do it in a separate PR to speed up merging this one
definitely worth simplifying this but refactoring seems out of scope of this PR, we can clean up afterwards |
| for the returned blob sidecars in terms of ordering. | ||
| After Fulu, clients may reconstruct the blob sidecars from data columns and return corresponding blobs to users. It is users' responsibility | ||
| to ensure that they're connected to a super node (custodies > 64 columns) to guarantee the availability of the blobs. |
There was a problem hiding this comment.
>=is more correct here, the node can construct with 50% of all columns.NUMBER_OF_COLUMNSis a config value, so a percentage may be better here.
| to ensure that they're connected to a super node (custodies > 64 columns) to guarantee the availability of the blobs. | |
| to ensure that they're connected to a supernode (or a node that custodies >= 50% of all columns) to guarantee the availability of the blobs. |
There was a problem hiding this comment.
Changes have been moved out of this PR to allow further discussion separately and to move ahead merging this
nflaig
left a comment
nflaig
left a comment
There was a problem hiding this comment.
LGTM - thanks @0x00101010 for taking care of this
0x00101010
changed the title
7 participants
No description provided.