(Discussion): canonical api draft #4041

walldiss · 2025-01-13T12:55:31Z

New gRPC API Design Document and Proto Definitions

This PR introduces a comprehensive design document (readme.md) and proto definitions for the new gRPC-based API for Celestia Node. The design focuses on improving security, simplicity, and developer experience while enabling smooth migration from the current JSON-RPC implementation.

Key Features:

Unified status codes and error handling across all services
Enhanced data availability verification
Support for both row root (V0) and data root (V1) verification approaches
Simplified blob submission and management
Clear separation between different service domains (blobs, shares, proofs)

Looking for feedback on:

Overall API structure and design decisions
Security considerations
Migration approach (V0/V1)
Error handling strategy
Any missing functionality

TODO:
[ ] Header service
[ ] http annotation for http gateway support

Note: The proto definitions and design doc should be reviewed together as they complement each other.

codecov-commenter · 2025-01-13T13:00:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.46%. Comparing base (2469e7a) to head (aa44ce7).
Report is 414 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4041      +/-   ##
==========================================
+ Coverage   44.83%   45.46%   +0.62%     
==========================================
  Files         265      309      +44     
  Lines       14620    22173    +7553     
==========================================
+ Hits         6555    10080    +3525     
- Misses       7313    11016    +3703     
- Partials      752     1077     +325

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

liamsi · 2025-01-14T15:01:00Z

api/V1/readme.md

+Standardized Status Codes
+```protobuf
+// Common status codes used across all services
+enum StatusCode {


I do think we should have the possibility for a dev to know if an error stems from node or e.g. somewhere from app or the cosmos sdk. Maybe that is already possible with the list somehow (e.g. not by the code but via the error string or sth?).

liamsi · 2025-01-14T18:02:55Z

api/V1/readme.md

+  - The API will verify availability for both the requested block and all preceding blocks within the sampling window
+  - Data will only be returned if the entire chain segment is verified as available
+  - Failed availability checks will return a specific `data_not_available` error code
+


~~Could this slow anything down from an app perspective?~~

ah, nvm. It's optional for cases where this could matter.

liamsi · 2025-01-14T18:05:09Z

api/V1/common.proto

+  // Error codes (100+)
+  // ERROR_DATA_NOT_AVAILABLE is returned if data availability sampling gurantees was required for request
+  // and failed. Keep in mind that data availability sampling will be ensured for all blocks in
+  // sampling window till the requested height
+  ERROR_DATA_NOT_AVAILABLE = 100;


Will the caller be able to differentiate between e.g. sampling timing out, and other cases?

liamsi · 2025-01-14T18:12:52Z

api/V1/readme.md

+  rpc GetShare(GetShareRequest) returns (GetShareResponse);
+  rpc GetEDS(GetEDSRequest) returns (GetEDSResponse);
+
+  // Range operations - support both V0 (row roots) and V1 (data root) responses


Both responses below return V0 though?

Ah, in the protos I see there is also GetRangeResponseV1 etc. Maybe mention that we only list v0 here to keep the readme concise.

vgonkivs · 2025-01-17T09:05:10Z

api/V1/blob_service.proto

+  double gas_price = 1;    // Amount to be paid per gas unit
+  uint64 gas = 2;         // Gas amount
+  string key_name = 3;    // Keystore key for signing


I'd make them optional too.

Aren't all options optional?

vgonkivs · 2025-01-17T09:14:57Z

api/V1/blob_service.proto

+  uint64 height = 1;
+  bytes namespace = 2;
+  bytes commitment = 3;
+  optional bool provide_commitment_proof = 4;


I don't think we need this field. The initial idea was to divide an inclusion proof and the commitment proof(blob proof). The difference between them the blob proof additionally contains subtree roots to build the commitment(link. After discussions with @walldiss we agreed that it makes sense to always return the full(blob) proof.

vgonkivs · 2025-01-17T09:59:19Z

api/V1/blob_service.proto

+
+message SubscribeAllRequest {
+  bytes namespaces = 1;
+  uint64 from_height = 2;


Let's clarify this: from_height should not be used to get the historical data(we have get methods for these purposes). Subscriptions should be used for the recent data only.

vgonkivs · 2025-01-17T10:01:32Z

api/V1/blob_service.proto

+message SubmitDataResponse {
+  celestia.node.v1.common.Status status = 1;
+  uint64 height = 2;
+  repeated bytes commitments = 3;
+}


Let's include the tx hash here. iirc, some teams are using SubmitPFB in the state module only bc it additionally provides a tx hash.

vgonkivs · 2025-01-17T10:05:31Z

api/V1/blob_service.proto

+  repeated celestia.node.v1.types.BlobWithProofV1 blobs = 2;
+}
+
+message SubmitDataRequest {


What about submitting data under multiple namespaces?

I'd suggest to add one more method:

message SubmitBlobsRequest { repeated Blobs blobs = 1; // blobs from different namespaces can be set SubmitOptions options = 2; }

vgonkivs · 2025-01-17T10:09:20Z

api/V1/blob_service.proto

+message BlobsSubscribeRequest {
+  bytes namespaces = 1;
+  uint64 from_height = 2;
+  repeated bytes commitment = 3;
+  optional celestia.node.v1.common.Options options = 4;
+}


Such a method could be useful in case we support async blob submission. but iirc, it's not feasible rn as error handling in this case might be tricky.

vgonkivs · 2025-01-17T10:12:03Z

api/V1/types.proto

+  optional bytes signer = 2;
+  bytes commitment = 3;
+}
+


Let's define BlobProof(reference here)

vgonkivs · 2025-01-17T10:16:31Z

api/V1/blob_service.proto

+// BlobService - handles blob operations
+service BlobService {
+  // Core operations
+  rpc GetBlob(GetBlobRequest) returns (GetBlobResponseV0);


what if the submitted blob is v1,v2...? Maybe it's better to have a generic BlobResponse and extend the blob's message with additional optional fields?

vgonkivs · 2025-01-17T10:19:11Z

api/V1/types.proto

+message Blob {
+  bytes data = 1;
+  optional bytes signer = 2;
+  bytes commitment = 3;
+}


I've got your point the Blob is part of the namespace, and by submitting/getting the blob, the user already has it but I'd anyway make it part of the message for consistency.

vgonkivs · 2025-01-17T10:23:40Z

api/V1/proofs.proto

+  Proof proof = 2;
+}
+
+message RowRootProof {


As we discussed with @walldiss, he does not want to import data structures from different repos. So, let's copy past it from the tendermint

ramin

i hope my comments are welcome. I've added some inline thoughts both from perspective as a keen observer and API user, high level it feels like this first pass is a literal "proto-fication" of the existing APIs vs a clean re-design considering dev ergonomics.

As a user:

i'd appreciate some aggressive design iterations on naming, there's lots of name stuttering inside files, services, messages
i think we could consider modularizing more, adding more files (ie: blob/v1 etc) so you could compose / import / use these protos as a developer building against node piecemeal, will then make gradual expansion easier
some of the proto's defined as *_service.proto should (i think) be just <name>.proto, they can then for example, define a Service, thus reading cleaner, like shares.Service, vs share_service.ShareService. Then, various types could be included there, and even nested in sub folder protos
i think another sweep or two of usability considerations should be made, that support end user/developer usage (ie: namespace being a messagehelpers etc) being supported in the proto messages to be more complex types, we could also consider protoc-gen-validate to define length and validation of these types etc in the canonical API, vs "needing to know" to submit a good request

that's all for now, thank you 😸

ramin · 2025-01-17T10:40:06Z

api/V1/blob_service.proto

+
+  // Subscription operations
+  rpc SubscribeBlobs(BlobsSubscribeRequest) returns (stream BlobsSubscribeResponse);
+  rpc SubscribeAll(SubscribeAllRequest) returns (stream SubscribeAllResponse);


i think it'd be really clean to have a single rpc in the service here

blobs.Subscribe(SubscriptionRequest)

That that would have make any config that differentiates all from a particular namespace or commitment like a filter, ie in plain english "i am subscribing to everything UNLESS i specify some options/filters in my subscription request configuration"

ramin · 2025-01-17T10:45:00Z

api/V1/fraud_service.proto

+  rpc GetAll(GetAllBEFPRequest) returns (GetAllBEFPResponse);
+
+  // Subscribe to receive fraud proofs as they are discovered
+  rpc SubscribeBEFPs(SubscribeBEFPRequest) returns (stream SubscribeBEFPResponse);


again, cleaner naming here would be to NOT repeat BEFP everywhere, its already in the name of the service,

eg:

BadEncodingFraudProofService.Subscribe() doesn't need the stutter of BEFP again if the service's concern is befp already

ramin · 2025-01-17T10:45:34Z

api/V1/proofs.proto

+// Proof system supports two verification approaches:
+// 1. Row roots (V0) - Uses row roots from Data Availability Header (DAH) for verification
+// 2. Data root (V1) - Uses data root proofs directly, more efficient and doesn't require DAH
+enum ProofType {


proofs.Type imo

ramin · 2025-01-17T10:45:50Z

api/V1/proofs.proto

+enum ProofType {
+  PROOF_TYPE_UNSPECIFIED = 0;
+  // Data proofs
+  PROOF_TYPE_DATA_INCLUSION = 1;


DATA_INCLUSION

ramin · 2025-01-17T10:48:54Z

api/V1/types.proto

+  // Shares should be from the same row
+  repeated celestia.node.v1.types.Share shares = 1;
+  celestia.node.v1.proof.RowRootProof row_proof = 2;      // NMT proof to the row root
+}


stylistic: I think i disagree with a types.proto, the types should maybe be defined where the reset of their usage is (mostly)

I think it makes sense to move definitions to the places where they are used.

Agree with this (and this is how I see it done in other repos)

ramin · 2025-01-17T10:50:45Z

api/V1/types.proto

+  uint64 index = 3;  // index of the first share of the blob in the EDS
+}
+
+message BlobWithProofV1 {


i don't love the developer ergonomics V0 and V1 at the end of the messages, could we consider modularizing more,

api/v1/blobs/v0/proofs.proto
api/v1/blobs/v1/proofs.proto

Then be deliberate about which gets imported and referenced wherever? ie: I think we could them simplify the Request to take one of either version and validate and respond properly in the service, would make the interface nice and clean

ramin · 2025-01-17T10:52:00Z

api/V1/blob_service.proto

+
+message GetBlobRequest {
+  uint64 height = 1;
+  bytes namespace = 2;


i've been thinking, it might be nice to make namespace a distinct type, where the API could then support some nicer features, like submitting in raw text, decode/encode, etc etc. At present, it feels very literal and as designed by someone who knows the internals, vs a developer using the API

cmwaters

Great work. Just tried to do a first pass of everything.

A more general topic regarding the proof API. What do you think about prioritising the data root based proving system first before releasing this API?

cmwaters · 2025-01-22T04:17:09Z

api/V1/blob_service.proto

+  double gas_price = 1;    // Amount to be paid per gas unit
+  uint64 gas = 2;         // Gas amount
+  string key_name = 3;    // Keystore key for signing


Aren't all options optional?

cmwaters · 2025-01-22T04:30:32Z

api/V1/types.proto

+  // Shares should be from the same row
+  repeated celestia.node.v1.types.Share shares = 1;
+  celestia.node.v1.proof.RowRootProof row_proof = 2;      // NMT proof to the row root
+}


Agree with this (and this is how I see it done in other repos)

cmwaters · 2025-01-22T04:32:04Z

api/V1/proofs.proto

+  // TBD: structure for commitment non-inclusion
+  PROOF_TYPE_COMMITMENT_NON_INCLUSION = 4;


Is this used anywhere?

cmwaters · 2025-01-22T04:33:28Z

api/V1/proof_service.proto

+// ProofsService - verification service that supports both V0 and V1 approaches
+service ProofsService {
+  // V0 verification of inclusion of shares. Requires row root to perform verification.
+  // Does not verifies namespace rules of provided shares and should be used only
+  // to verify inclusion of data spread over multiple namespaces
+  rpc VerifySharesV0(VerifySharesRequestV0) returns (VerifyProofResponse);
+  // Performs what VerifySharesV0 does with additional namespace rules verification on top.
+  rpc VerifyNamespaceV0(VerifyNamespaceRequestV0) returns (VerifyProofResponse);
+
+  // V1 verification
+  // VerifyDataProof verifies raw blob data using data root proof
+  rpc VerifyDataProof(VerifyDataRequest) returns (VerifyProofResponse);
+  // VerifyCommitmentProof verifies inclusion of blob, by using commitment and commitment data root proof.
+  // Provided proof type should be PROOF_TYPE_COMMITMENT
+  rpc VerifyCommitmentProof(VerifyCommitmentRequest) returns (VerifyProofResponse);
+}


I would prefer separate v0 and v1 proof services (especially if we plan on deprecating v0 in the future)

cmwaters · 2025-01-22T04:36:20Z

api/V1/proof_service.proto

+  rpc VerifyDataProof(VerifyDataRequest) returns (VerifyProofResponse);
+  // VerifyCommitmentProof verifies inclusion of blob, by using commitment and commitment data root proof.
+  // Provided proof type should be PROOF_TYPE_COMMITMENT
+  rpc VerifyCommitmentProof(VerifyCommitmentRequest) returns (VerifyProofResponse);


Semantic nit: I know the usage of Commitment is widespread already but I find it both vague and misleading (to commit over data is to sign over it). To make it more understandable to the users I would separate them into the two data structs they are proving inclusion over:

BlobInclusionProofs

ShareInclusionProofs (I think the way you have this, this can also be a range)

cmwaters · 2025-01-22T05:37:38Z

api/V1/common.proto

+  // ERROR_DATA_NOT_AVAILABLE is returned if data availability sampling gurantees was required for request
+  // and failed. Keep in mind that data availability sampling will be ensured for all blocks in
+  // sampling window till the requested height
+  ERROR_DATA_NOT_AVAILABLE = 100;


Is this only returned by the SharesAvailable method?

cmwaters · 2025-01-22T05:38:07Z

api/V1/common.proto

+  // sampling window till the requested height
+  ERROR_DATA_NOT_AVAILABLE = 100;
+  // Fraud was detected in the chain before requested block
+  ERROR_FRAUD_DETECTED = 101;


What would cause this error? (i.e. more specifically)

cmwaters · 2025-01-22T05:39:24Z

api/V1/common.proto

+  ERROR_DATA_NOT_AVAILABLE = 100;
+  // Fraud was detected in the chain before requested block
+  ERROR_FRAUD_DETECTED = 101;
+  ERROR_TIMEOUT = 102;


Are all timeouts handed by gRPC itself i.e. contexts in the case of go. Do they need to be included here?

cmwaters · 2025-01-22T05:42:12Z

api/V1/share_service.proto

+}
+
+// ShareService - handles data share operations
+service ShareService {


We have subscription options for both blob and fraud servies, why not share services?

I can imagine most base rollups (or rollup full nodes in general) will subscribe to all data in one or more namespaces.

cmwaters · 2025-01-22T05:43:13Z

api/V1/share_service.proto

+message GetShareRequest {
+  uint64 height = 1;
+  uint32 row = 2;
+  uint32 col = 3;
+  optional celestia.node.v1.common.Options options = 4;
+}


Will you also be able to reference a share based on an index (rather than rol and col). Some of the responses return the starting index of a blob.

canonical api draft

aa44ce7

liamsi reviewed Jan 14, 2025

View reviewed changes

vgonkivs reviewed Jan 17, 2025

View reviewed changes

ramin reviewed Jan 17, 2025

View reviewed changes

cmwaters reviewed Jan 22, 2025

View reviewed changes

walldiss self-assigned this Feb 7, 2025

		// TBD: structure for commitment non-inclusion
		PROOF_TYPE_COMMITMENT_NON_INCLUSION = 4;

(Discussion): canonical api draft #4041

Are you sure you want to change the base?

(Discussion): canonical api draft #4041

Conversation

walldiss commented Jan 13, 2025

codecov-commenter commented Jan 13, 2025 • edited Loading

Codecov Report

liamsi Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

liamsi Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgonkivs Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

ramin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmwaters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 13, 2025 •

edited

Loading

liamsi Jan 14, 2025 •

edited

Loading

liamsi Jan 14, 2025 •

edited

Loading

liamsi Jan 14, 2025 •

edited

Loading

vgonkivs Jan 17, 2025 •

edited

Loading