Skip to content
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

Suggested Celestia Client Architecture #5318

Open
AdityaSripal opened this issue Dec 5, 2023 · 6 comments
Open

Suggested Celestia Client Architecture #5318

AdityaSripal opened this issue Dec 5, 2023 · 6 comments
Assignees

Comments

@AdityaSripal
Copy link
Member

This issue is a writeup of how to build a Celestia client that will act as the underlying DA client for rollups clients to verify. Once consensus is reached, this can be formalized with a document on the specs

One thing to note is that Celestia is an SDK chain running CometBFT consensus. Thus we can reuse the tendermint client for most of the client logic.

However Celestia has some important key modifications it needs to make.

The first is that rollup block data (header + txs) are committed to in the DataHash of the CometBFT header. In the 07-tendermint client, we discard this information when compressing the header down to a consensus state. However, it is necessary that we add the DataHash to the consensus state for DA verification.

*Note theoretically, one could add this as metadata in the client store, thus outside the consensus state itself. But I find that to be a bit hacky and not in keeping with the client and proof semantics.

The second thing we will need to do is to allow different types of proofs to be handled by the Celestia light client VerifyMembership and VerifyNonMembership methods.

Of course we will want to continue supporting standard proofs against the app hash, but in order for the celestia light client to serve as DA verification for rollup clients, it will also need to support inclusion proofs against the data hash stored in the consensus state.

The proposal here would be to unmarshal the proof []byte sent to VerifyMembership into a specific type:

type CelestiaProof struct {
    ProofType enum
    Proof []byte
}

The ProofType enum will select one of a specific set of supported proof types (e.g. AppHashInclusionProof, DataHashInclusionProof, DataHashBlockDataProof)

Based on the prooftype specified, the VerifyMembership unmarshals the underlying proof accordingly and executes proof-specific verification logic.

*Note there are multiple types of proofs that can be verified against the data hash. So we should support those at separate enums.

IMPORTANT: The caller that wishes to do the verification should be the one to set the ProofType enum and wrap provided proof in a CelestiaProof. Do not allow relayers or untrusted users to set the enum themselves as this could be a potential attack vector

Discussion with @nashqueue needed to understand which proof types will need to be supported.

TLDR;

Changes to Tendermint Client needed:

  1. Add data hash to ConsensusState on UpdateClient
  2. Support multiple proof types and verification logic in VerifyMembership endpoint
@AdityaSripal AdityaSripal added the needs discussion Issues that need discussion before they can be worked on label Dec 5, 2023
@nashqueue
Copy link

Importing this implementation should capture all cases https://github.com/celestiaorg/nmt/blob/master/proof.go

@damiannolan
Copy link
Member

Importing this implementation should capture all cases https://github.com/celestiaorg/nmt/blob/master/proof.go

I'm curious if we should be rather using ShareProof or if the raw nmt proof is the desired option?

@nashqueue
Copy link

Yes that would make sense. The proof verification would live in the nmt.

@crodriguezvega crodriguezvega added this to the Rollkit integration milestone Jan 3, 2024
@damiannolan
Copy link
Member

Hey @nashqueue, I've looked into duplicating the 07-tendermint and modifying some parts to create a celestia (da client) which rollkit clients written as wasm contracts will call back to.

In order to get the code together, we'll likely need to iron some dependency issues which I'll mention below.

Firstly, I'll give an outline below of the changes that we discussed would be required and a rough idea of what I had in mind.

  1. Extend the ConsensusState to include the DataHash, i.e. the data availability root
message ConsensusState {
  option (gogoproto.goproto_getters) = false;

  // timestamp that corresponds to the block height in which the ConsensusState
  // was stored.
  google.protobuf.Timestamp timestamp = 1 [(gogoproto.nullable) = false, (gogoproto.stdtime) = true];
  // commitment root (i.e app hash)
  ibc.core.commitment.v1.MerkleRoot root = 2 [(gogoproto.nullable) = false];
  // hash of next validator set
  bytes next_validators_hash = 3 [(gogoproto.casttype) = "github.com/cometbft/cometbft/libs/bytes.HexBytes"];
+ // commitment data availability root (i.e data hash)
+ ibc.core.commitment.v1.MerkleRoot data_availability_root = 4 [(gogoproto.nullable) = false];
}

Adding this is pretty straight forward and on client updates we can easily add the DataHash here (ref):

consensusState := &ConsensusState{
	Timestamp:            header.GetTime(),
	Root:                 commitmenttypes.NewMerkleRoot(header.Header.GetAppHash()),
	NextValidatorsHash:   header.Header.NextValidatorsHash,
+       DataAvailabilityRoot: header.Header.GetDataHash(),
}
  1. Add a new Proof type for celestia clients which can support one of multiple proof types. This allows the celestia client to be used synonymously to how the 07-tendermint client already works today, but additionally would support the ShareProof type against the DataAvailabilityRoot, for rollkit clients to prove inclusion of blob data in the square.

We can use a protobuf oneof type to support both the ShareProof and MerkleProof (23-commitment). Regular merkle proofs will prove application state against the AppHash, whereas the share proofs will prove against the DataHash.

// Proof defines a message type which encapsulates a proof operation.
message Proof {
    oneof op {
        // ics23 based merkle proof used for celestia application state  
        ibc.core.commitment.v1.MerkleProof app_proof = 1 [(gogoproto.nullable) = false];
        // celestia share proof used for celestia data availability layer
        celestia.core.v1.proof.ShareProof data_availability_proof = 2 [(gogoproto.nullable) = false];
    }
}

We would then modify the VerifyMembership methods of the lightclient to support unmarshalling proof []byte to this type. I imagine it would look something like this, type switching on the proof:

var proof types.Proof
if err := cdc.Unmarshal(proofBz, proof); err != nil {
   return errorsmod.Wrap(commitmenttypes.ErrInvalidProof, "failed to unmarshal proof into celestia proof type")
}

switch p := proof.Op.(type) {
case *celestia.ShareProof:
    shareProof := p.DataAvailabilityProof    
    return shareProof.Validate(consensusState.GetDataAvailabilityRoot())
case *commitmenttypes.MerkleProof:
    merkleProof := p.AppProof
    return merkleProof.VerifyMembership(cs.ProofSpecs, consensusState.GetRoot(), merklePath, value)
}

Regarding the dependency issues I mentioned at the start of the comment. Essentially, at the moment it's hard to plug celestia.core.v1.proof.ShareProof directly in here. There seems to exist two different ShareProof types which I've found.
One maintained in celestia-core and one maintained in celestia-app. Note, the typeURL I used previously is that of the one maintained in celestia-app.

I think it may be problematic to consume protos from celestia-core due to the cometbft fork. It seems the raw Go type ShareProof is created by converting the protobuf type and this is what we need to call Validate(dataHash) on.

If we are going to import any of these protobuf messages we would need them to be published to BSR (buf schema registry) which atm, I don't think they are, but its fairly trivial to set it up. Would be happy to create an issue if needs be or help out.

If we will use the ShareProof maintained in celestia-app, then we may need to potentially move some code around to be able to convert and call the appropriate Validate() method. Happy to discuss this stuff synchronously tomorrow also!

@AdityaSripal
Copy link
Member Author

If we use the same client for app hash proofs and data hash proofs, then we will need to also have the custom self-client validation in. Because we will also need to validate self consensus state with the added datahash.

Arguably, might make sense to have a standard TM client for connection Celestia app to other chains. But have the custom Celestia DA client as a dependency for rollup clients

@damiannolan
Copy link
Member

Agree! Simplifying it to only use as a DA client for rollup clients is probably the way to go. We can just remove the protobuf oneof type and unmarshal only ShareProofs in VerifyMembership to use with the DataHash. Connections between celestia-app and other chains can continue to use regular 07-tendermint clients.

@crodriguezvega crodriguezvega moved this to Todo 🏃 in ibc-go Feb 1, 2024
@damiannolan damiannolan self-assigned this Feb 5, 2024
@damiannolan damiannolan moved this from Todo 🏃 to In progress 👷 in ibc-go Feb 5, 2024
@damiannolan damiannolan moved this from In progress 👷 to In review 👀 in ibc-go Feb 21, 2024
@crodriguezvega crodriguezvega added 07-celestia and removed needs discussion Issues that need discussion before they can be worked on labels Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In review 👀
Development

No branches or pull requests

4 participants