ibc
ibc copied to clipboard
cosmos
ICS2: Clarification on client verification functions
The general declaration of a client verification function is (did some reordering):
type verifyObjectState = (
clientState: ClientState, // used to retrieve CommitmentRoot
height: Height, // used to retrieve CommitmentRoot
proof: CommitmentProof,
prefix: CommitmentPrefix,
... <args> // to construct the CommitmentPath
expected_object: <Object> // to derive the value for verification
)
The current spec implies that the CommitmentRoot at height can be retrieved from the ClientState and same is shown in the ICS example.
I noticed that the signatures in latest SDK implementation add store and cdc parameters and they are used by the concrete client implementation (tendermint) to retrieve from the store the ConsensusState from which the CommitmentRoot is obtained.
Should these APIs be updated with the store parameter? Not sure about the marshaler (cdc).
Maybe the caller could serialize (i.e. expected_object: []bytes). But then along the same lines the CommitmentRoot could be passed directly to these verify functions ... and in general all the data required so the client verification doesn't need to read the host store, unmarshal, etc .
We started to work on the IBC modules in Rust and trying as much as possible to conform to the spec :)
(Note: In SDK func (cs ClientState) VerifyClientConsensusState(.. also adds a provingRoot parameter, not showing in the ICS)
The store and cdc are Cosmos SDK internal state-accessor / state-decoder interfaces, I don't think they should be included in the IBC specifications. The IBC specification generally uses get, set, delete as defined in ICS 24 for store access; if it would be clearer, ICS 2 can be revised to only use these instead of object accessor methods. Would that help, do you think?
The store and cdc are Cosmos SDK internal state-accessor / state-decoder interfaces, I don't think they should be included in the IBC specifications.
Ideally a client implementation following the APIs should be easily integrated, without changes, with different IBC implementations.
The IBC specification generally uses get, set, delete as defined in ICS 24 for store access;
Understood and yes, it would help. For the client verification functions there is a get for consensus state that needs to be called against a store that is owned by the host and not in the parameters.
On a different note... I might be missing something but it looks like the client verification functions can be generic. They just seem to retrieve the host data needed for the final call to ICS 23's verifyMembership = (root: CommitmentRoot, proof: CommitmentProof, path: CommitmentPath, value: Value) . If true, these implementations should be part of the ICS 2 and not client specific ones (ala ICS 7).
Right, I'll clarify those bits.
On a different note... I might be missing something but it looks like the client verification functions can be generic. They just seem to retrieve the host data needed for the final call to ICS 23's verifyMembership = (root: CommitmentRoot, proof: CommitmentProof, path: CommitmentPath, value: Value) . If true, these implementations should be part of the ICS 2 and not client specific ones (ala ICS 7).
I'm not quite sure I understand; these are intentionally part of the ICS 2 interface so clients can define them differently (perhaps not using ICS 23, for example, as the solo machine client does). Parts could be abstracted out, certainly, if that's what you mean.
Ah, good point on the solo machine client, thanks! I will look into it more closely.
I was browsing a bit the SDK client code and it is hard to see what is really client specific. For example the verify functions take a prefix and an ID, used to construct the commitment path. But this should be done in the same way by all clients. There is also a lot of common verification code (e.g. checking if client is frozen, checking non-null for proof, prefix, etc). I don't want to start a debate on whether parameter checks should be part of the API contract, or verification should be done in the implementation, or both. Just pointing that trying to isolate what precisely is specific to clients in the verify functions is a bit challenging.
I also noticed that in the solo case, all verify functions change the client state. Will try to understand why, and in particular why in the verification steps. But this has some implications on the model we had in mind for the Rust handler implementation. e.g. for the connection msg handler, the verification part operates on a state in a read-only mode and, if successful, would update the state (connection store and maybe add a connection to the client). Was not expecting the client state to change and definitely not in the verify functions.
Sorry for going back and forth between ICS and implementation details, just trying to understand in detail the ICS-02 verify functions, API signatures, expectations.
I was browsing a bit the SDK client code and it is hard to see what is really client specific. For example the verify functions take a prefix and an ID, used to construct the commitment path. But this should be done in the same way by all clients. There is also a lot of common verification code (e.g. checking if client is frozen, checking non-null for proof, prefix, etc). I don't want to start a debate on whether parameter checks should be part of the API contract, or verification should be done in the implementation, or both. Just pointing that trying to isolate what precisely is specific to clients in the verify functions is a bit challenging.
Yes, this is true, and likely in the SDK these common elements have not yet been optimally abstracted out.
I also noticed that in the solo case, all verify functions change the client state. Will try to understand why, and in particular why in the verification steps. But this has some implications on the model we had in mind for the Rust handler implementation. e.g. for the connection msg handler, the verification part operates on a state in a read-only mode and, if successful, would update the state (connection store and maybe add a connection to the client). Was not expecting the client state to change and definitely not in the verify functions.
Hmm, yeah, it is a bit different but it is very convenient for the solo machine client. Open to suggestions though, do you think it could be written in a different way? We want to avoid requiring a "header update" for every signature, though.
Sorry for going back and forth between ICS and implementation details, just trying to understand in detail the ICS-02 verify functions, API signatures, expectations.
Not at all! These are all excellent questions.
@AdityaSripal This issue seems related to https://github.com/cosmos/ibc/issues/684.
I might be missing something but it looks like the client verification functions can be generic. They just seem to retrieve the host data needed for the final call to ICS 23's verifyMembership = (root: CommitmentRoot, proof: CommitmentProof, path: CommitmentPath, value: Value) . If true, these implementations should be part of the ICS 2 and not client specific ones (ala ICS 7).
The generic verification methods were added in ICS 02 in PR #687. Besides that, I think there's no other action item left from this issue, so I will close it for now.
