-
Notifications
You must be signed in to change notification settings - Fork 189
Add data column sidecars debug endpoint #537
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
Add data column sidecars debug endpoint #537
Conversation
Introduces support for data column sidecars as part of EIP-7594 (PeerDAS): - Add /eth/v1/debug/beacon/data_column_sidecars/{block_id} endpoint - Define DataColumnSidecar and related types for Fulu fork - Add Cell primitive type for 2048-byte data columns
types/primitive.yaml
Outdated
@@ -142,3 +142,8 @@ KZGProof: | |||
pattern: "^0x[a-fA-F0-9]{96}$" | |||
description: "A G1 curve point. Used for verifying that the `KZGCommitment` for a given `Blob` is correct." | |||
|
|||
Cell: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cell is a FullDAS concept, this should be a DataColumn or Column, for correct typing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this is the name used in the CL spec
types/primitive.yaml
Outdated
type: string | ||
format: hex | ||
pattern: "^0x[a-fA-F0-9]{4096}$" | ||
description: "A 2048-byte hexadecimal string, prefixed with `0x`" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description
should describe semantics, not the format (see Blob
for how it should be done).
The format is already given by the pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline this, don't have explicit Cell
primitive yet because it's not in PeerDAS as such
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved inline
@@ -118,6 +118,8 @@ paths: | |||
$ref: "./apis/beacon/blocks/attestations.v2.yaml" | |||
/eth/v1/beacon/blob_sidecars/{block_id}: | |||
$ref: "./apis/beacon/blob_sidecars/blob_sidecars.yaml" | |||
/eth/v1/debug/beacon/data_column_sidecars/{block_id}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this fits in the debug
namespace, should it be beacon?
/eth/v1/beacon/data_column_sidecars/{block_id}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it useful for general consumption @dapplion ?
apis/debug/data_column_sidecars.yaml
Outdated
$ref: '../../beacon-node-oapi.yaml#/components/parameters/BlockId' | ||
- name: indices | ||
in: query | ||
description: Array of indices for data column sidecars to request for in the specified block. Returns all data column sidecars in the block if not specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description should clarify that it will return only the columns that this node is custodying.
Question, if this node has in custody indices [0,1] and the query parameter indices is [1,2]:
- Should the node error, or
- Should the node return [1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the node return [1]
probably this, might be worth to do a follow-up PR that clarifies this
types/fulu/data_column_sidecar.yaml
Outdated
|
||
DataColumnSidecar: | ||
type: object | ||
description: "A data column sidecar as defined in the Fulu consensus spec." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be good to add a reference to CL spec type here
similar to
beacon-APIs/types/electra/attestation.yaml
Lines 36 to 38 in 8717bf6
Attestation: | |
type: object | |
description: "The [`Attestation`](https://github.com/ethereum/consensus-specs/blob/v1.5.0/specs/electra/beacon-chain.md#attestation) object from the CL spec." |
types/primitive.yaml
Outdated
@@ -142,3 +142,8 @@ KZGProof: | |||
pattern: "^0x[a-fA-F0-9]{96}$" | |||
description: "A G1 curve point. Used for verifying that the `KZGCommitment` for a given `Blob` is correct." | |||
|
|||
Cell: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this is the name used in the CL spec
**Motivation** - ethereum/beacon-APIs#537 **Description** Add `GET /eth/v1/debug/beacon/data_column_sidecars/{block_id}` endpoint to retrieve data column sidecars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from discussion sounds like no blockers so merging (irl. discussions)
Beacon API spec PR: ethereum/beacon-APIs#537
Implement new endpoint: - ethereum/beacon-APIs#537
* Add /eth/v1/debug/beacon/data_column_sidecars/{block_id} Implement new endpoint: - ethereum/beacon-APIs#537 * Fix compilation * Fix test endpoint * Handle data column request while Fulu is not configured
Introduces support for data column sidecars as part of EIP-7594 (PeerDAS):