celestiaorg / celestia-node

Celestia Data Availability Nodes
Apache License 2.0
935 stars 933 forks source link

[Feature Request]: make structs using app_version self contained #3977

Open zvolin opened 2 days ago

zvolin commented 2 days ago

Implementation ideas

Background

We are maintaining celestia ecosystem in Rust. We have crates like celestia-types which provides basic types and traits used in celestia, like eg. ExtendedDataSquare, and celestia-rpc which provides RPC client methods definitions for https://node-rpc-docs.celestia.org/. Those are meant to be a standalone building blocks for anything celestia related, whether it's just some application working with offline data or a fully fledged node.

app_version is used to put some constrains for types in celestia ecosystem. In example it puts a limit on square size or specifies parameters required for commitment generation. This effectively means that an EDS created in version 2 may not be valid in version 1. As app version is not stored within Blob and EDS this information needs to be routed somehow to perform a validation.

Problem

We're trying our best to make celestia ecosystem feel idiomatic in rust and one of the guidelines in Rust is to make invalid state inconstructible. This means that if we want to return ExtendedDataSquare to a user, we need to know the app_version to check if it's valid. Storing app_version inside Blob and EDS would make them self-contained instead, allowing for validation eg. during parsing them from json.

Example of issue

Due to the recent changes to celestia-node, the RPC api requires only height and not the whole ExtendedHeader in share.GetEDS. The most widely used json rpc crate in rust allows for expressing APIs using interfaces, which can then be called as a method on any client, to separately abstract out transport layer and the actual interface.

#[rpc]
trait Share {
    #[method(name = "share.GetEDS")]
    async fn share_get_eds(&self, height: u64) -> Result<ExtendedDataSquare, Error>;
}

let client = my_custom_http_client(); // or my_custom_ws_client() or my_custom_browser_fetch_client()
client.share_get_eds(1).await

But here we don't have enough information to construct the ExtendedDataSquare if we want to make sure the state is valid. There are some workarounds to that but none of them is great:

Solution

In my opinion the best solution is to have ExtendedHeader and Blob self contained. This could be done in celestia-app level (but is likely consensus breaking) or just introduce the app_version on celestia-node level (the blob is already modified wrt the struct present in app).

The example above is just a symptom that showed up in RPC, but the issue would be the same eg. when reading this data from filesystem, external services etc.

jcstein commented 1 day ago

cc: @celestiaorg/celestia-node 🙏 for visibility