Replies: 6 comments 28 replies
-
|
This is a great start! I'm leaving a few comments per section focusing on the high-level use-cases (and hopefully not bikeshedding on minor details): BlobsThe service scope makes a lot of sense and I think this generally should be the methods of the blob module/service: service BlobService {
rpc GetBlobByCommitment (BlobRequest) returns (BlobResponse) {}
rpc GetBlobs (BlobListRequest) returns (BlobListResponse) {}
rpc SubmitBlob (SubmitBlobRequest) returns (SubmitBlobResponse) {}
}That said, the
Proofs:
Data Availability ProofsHaving a proof API could be a good idea but the see the last point on Proofs above. It could be better to expose the proofs per service instead of a lower-level generalized Fraud Proofs
There is actually a concrete implementation, i.e. for bad encoding fraud proofs. Users will need two endpoints for those:
Extended HeadersI think this additionally needs a way to subscribe / listen to a header stream. SharesA few comments on the proposed API:
Other commentsThe two methods used by blobstream (but can potentially be useful for other rollups/data attestation bridges) that are not covered by the API nor my comments above, should also be part of the protobuf definition:
// GetDataCommitment collects the data roots over a provided ordered range of blocks,
// and then creates a new Merkle root of those data roots. The range is end exclusive.
GetDataCommitment(ctx context.Context, start, end uint64) (*ResultDataCommitment, error)
// GetDataRootInclusionProof creates an inclusion proof for the data root of block
// height `height` in the set of blocks defined by `start` and `end`. The range
// is end exclusive.
GetDataRootInclusionProof(
ctx context.Context,
height int64,
start, end uint64,
) (*ResultDataRootInclusionProof, error)It is also worth discussing if there should be 3 modes for each of the data types that can be requested:
I am not convinced 1. is ever needed tbh but left it here for completeness. Regarding key management and signing, and if the API should contain all grpc endpoints of the state machine as well, I'm also linking these two discussions as they are related:
Also, orthogonal to the above, there is no notion of error codes/error IDs in the above Response types. |
Beta Was this translation helpful? Give feedback.
-
|
Agreed concerning the Blobstream related API. We should have the following endpoints (where to put them is still being discussed):
The last 2 are only blobstream specific. But the first two can also be used in rollups fraud proofs. |
Beta Was this translation helpful? Give feedback.
-
|
Additional quick notes/questions from the WG call:
|
Beta Was this translation helpful? Give feedback.
-
|
Thanks @jbowen93. The API looks great. I'd like to add a few things to the Blob Module:
|
Beta Was this translation helpful? Give feedback.
-
|
In the case of sync submission, we can get both of these values. I think it's not necessary to keep both and return only
Yes, and we should provide an API that applies a |
Beta Was this translation helpful? Give feedback.
-
|
We can do it internally for message BlobRequest {
bytes commitment = 1;
bytes namespace = 2;
oneof identifier {
uint32 block_number = 3;
bytes tx_hash = 4;
}
} |
Beta Was this translation helpful? Give feedback.
-
|
Can node already request the blob by tx_hash? |
Beta Was this translation helpful? Give feedback.
-
|
No |
Beta Was this translation helpful? Give feedback.
-
One thing that I'm concerned about regarding this endpoint is that the user will not know that the returned height is correct and contains the transaction, until he requests the blob. I'm wondering shouldn't we return a PFB inclusion proof with the height? Or I'm missing something and it's not that critical? |
Beta Was this translation helpful? Give feedback.
-
|
Providing a list of things I find essential to be emphasized and some of them might be repeated. There are more little details that I need to look into as it is too early for them. The next step is incorporating all the feedback into the Draft for the next round of reviews. Avoid any dependence on
|
Beta Was this translation helpful? Give feedback.
-
Note that in proto3 all fields are "optional" by default. If we don't want to introduce one method for each mode, then I would suggest introducing an enum. Something along these lines: enum RetrievalMode {
PROOFS_AND_DATA = 0; // default
DATA_ONLY = 1;
PROOFS_ONLY = 2;
}
// example usage in blob request message:
message BlobRequest {
uint64 height = 1;
bytes namespace_id = 2;
bytes commitment = 3;
RetrievalMode mode = 4;
}
I would not be opposed to adding a separate method for each mode instead though. |
Beta Was this translation helpful? Give feedback.
-
|
Note that I meant "feature" instead of "future", corrected the OP. |
Beta Was this translation helpful? Give feedback.
-
Tx / blob submission optionsI would also suggest to add the following type below (maybe name it differently?). This will allow to set all necessary params for blob (or any tx) submission. Question is if we need both // TxConfig specifies additional options that will be applied to the Tx.
message TxConfig {
string signer_address = 1; // Specifies the address from the keystore that will sign transactions.
string key_name = 2; // Specifies the key from the keystore associated with an account that will be used to sign transactions.
float gas_price = 3; // Represents the amount to be paid per gas unit.
// bool is_gas_price_set = 4; // Indicates if the gas price has been explicitly set by the user.
uint64 gas = 5; // Specifies the gas amount.
string fee_granter_address = 6; // Specifies the account that will pay for the transaction.
}The blob service then would look sth like that: message SubmitBlobRequest {
Blob blob = 1;
TxConfig tx_config = 2;
}ref: #3349 Note that all fields in proto3 are optional by default but we could also more explicitly tag them with the |
Beta Was this translation helpful? Give feedback.
-
|
@vgonkivs or @celestiaorg/celestia-node what was the rationale for |
Beta Was this translation helpful? Give feedback.
-
|
Correct. Since the default gas price is -1, we need to understand - was 0 set explicitly by user or it wasn't set at all. |
Beta Was this translation helpful? Give feedback.
-
|
in v2 there will be a min-fee though: https://github.com/celestiaorg/celestia-app/blob/1bd0f133222f60c0f89779db1374c23a36eea213/x/minfee/README.md A gas_price of zero would not be allowed anyways 🤔 The default will be 0.000001 utia |
Beta Was this translation helpful? Give feedback.
-
|
Then, we can remove this field after v2 is merged 😊 |
Beta Was this translation helpful? Give feedback.
-
|
I think we can then simplify this here to: // TxConfig specifies additional options that will be applied to the Tx.
message TxConfig {
// Specifies the address or key id from the keystore that will sign transactions.
string signer = 1;
// Represents the amount to be paid per gas unit.
float gas_price = 2;
// Specifies the gas limit.
uint64 gas = 3;
// Specifies the fee granter account that will pay for the transaction.
string fee_granter_address = 4;
}The spec can then mention that |
Beta Was this translation helpful? Give feedback.
-
Error codesRegarding error handling, I would suggest a similar approach as @walldiss suggested here: #3529 (comment) Particularly, as we might want to expose this API via json over http. Suggested proto messsages: enum ErrorCode {
UNKNOWN_ERROR = 0; // TODO better start at a much higher error code range
INVALID_REQUEST = 1;
INVALID_PARAMS = 3;
INTERNAL_ERROR = 4;
// TODO Add more specific error codes here.
// related issues:
// https://github.com/celestiaorg/celestia-node/issues/3335#issuecomment-2079748320
// https://github.com/rollkit/go-da/issues/65
}
message RpcError {
ErrorCode code = 1;
string message = 2;
}Response messages should be changed to (for example): message BlobResponse {
oneof response {
Blob blob = 1;
RpcError error = 2;
}
} |
Beta Was this translation helpful? Give feedback.
-
|
Question: Should some errors be per service instead of in a central error package? |
Beta Was this translation helpful? Give feedback.
-
Fraud ServiceThe current fraud API is very go-idiomatic: it returns a channel but what is even worse, the user is completely left in the dark what type the returned Proof actually is: 
I would instead suggest the following (concrete) API and rename to befp for the module / service name: message BEFPRequest {
uint64 from_height = 1;
}
message BEFPsSubscribeRequest {}
message BadEncodingFraudProof {
// TODO add concrete fields here
}
service BadEncodingFraudProofService {
rpc GetAll(BEFPRequest) returns (BadEncodingFraudProof) {}
rpc SubscribeBEFPs(BEFPsSubscribeRequest) returns (stream BadEncodingFraudProof) {}
} |
Beta Was this translation helpful? Give feedback.
-
|
During the implementation we were assuming that there will be more than 1 concrete type of the Proof. Re validity proofs, I'm pretty sure, we can adjust code of the go-fraud(rename to go-proof) to handle both types of proofs if it's needed. |
Beta Was this translation helpful? Give feedback.
-
That is neat but that is even more different than a general/abstract "fraud" service. Once we cross that bridge and either ...
... and these would share the property that they should be semantically treated the same (e.g. all validity proofs would come through via a single "subscribe" method) then this would make sense. At this time, it simply does not make sense unless I am missing sth. Note that I am not arguing against abstracting this in the implementation/in go-fraud/go-proof (which also seems premature but OK) but rather for the developer facing API. |
Beta Was this translation helpful? Give feedback.
-
|
Or let me ask differently, how would you today describe to a Rust dev how to handle fraud proofs? Currently, one has to dig through multiple indirections of interfaces/repositories and try to match that with what the spec says about befps. I think having a concrete befp type returned that resembles what is in a spec can help to save a bunch of time how to handle that vs digging through node and go-fraud. |
Beta Was this translation helpful? Give feedback.
-
|
I agree that a concrete BEFP type should be described in proto so that X lang devs don't have to look through our code. Keeping this general interface is not a big deal as proto has |
Beta Was this translation helpful? Give feedback.
-
Because if they are attached, we don't need any generalization and can piggyback off the header, and if delayed, the p2p networking for proofs becomes necessary. |
Beta Was this translation helpful? Give feedback.
-
This discussion proposes an initial rough draft of a Protobuf API for the Celestia Node.
This API should be seen as an addition to the existing JSON RPC API, comments on the potential implications of this API replacing the existing API are premature.
This API doesn't provide an equivalent of the existing State API, for discussion of how the Node API should or shouldn't interact with the broader Celestia state machine see this post.
This API doesn't provide provide an equivalent of the existing Node or P2P APIs. These APIs should only be used by a node's admin and thus are out of scope.
This API is agnostic to any specific programming language's semantics.
This API attempts to follow a Resource-oriented design.
This API attempts to follow Protocol Buffers API Best Practices
Blobs
The
blobtype is defined in the celestia-node codebase here:types.Blobis a reference to"github.com/celestiaorg/celestia-app/x/blob/types"which defines its own blob proto here and references the go-square proto here.The only difference between these protos is that the latter adds a
bytes signer = 5;field, for simplicity we ignore that field.Data Availability Proofs
The
prooftype is defined in the celestia-node codebase here:nmt.Proofis a reference"github.com/celestiaorg/nmt"which defines aProofhere.A comment in the celestia-node codebase states
The referenced issue (#2303) is still open but is beyond the scope of this API.
Fraud Proofs
Fraud Proofs seem to be referenced in the celestia-node code here which references the go-fraud repo which a stub proto here.
There doesn't seem to be a concrete implementation of fraud proofs, so we leave this API as a TODO.
Extended Headers
The ExtendedHeader is already defined as a proto here
Shares
The celestia-node codebase defines a
NamespacedSharesResponsehere which references thesharemodule which defines aShareas[]byte.Extended Data Square
ExtendedDataSquareis defined ingithub.com/celestiaorg/rsmt2dhereTODO: Define the correct proto for an
ExtendedDataSquareBeta Was this translation helpful? Give feedback.
All reactions