feat: cometbft header verifier library#366
Conversation
Farhad-Shabani
force-pushed
the
farhad/impl-cometbft-verifier
branch
from
c411c77 to
1c51780
Compare
rnbguy
changed the title
| use protobuf::{base64, hex}; | ||
|
|
||
| #[test] | ||
| fn test_verify_update_header() { |
There was a problem hiding this comment.
Managed to get tendermint light client verified !
This test called the top verify_update_header 🙂
The header data is captured directly from hermes-sdk update client payload.
|
Remaining
|
| pub struct UntrustedBlockState { | ||
| pub signed_header: SignedHeader, | ||
| pub validators: ValidatorSet, | ||
| // pub next_validators: Option<ValidatorSet>, |
There was a problem hiding this comment.
this field is present in tendermint-rs but never used in IBC and always set to None in ibc-rs.
I removed this, as Option<T> implements Copy, not Clone. And ValidatoSet only impls Clone.
| pub proposer_priority: i64, | ||
| pub voting_power: u64, | ||
| pub proposer_priority: u64, | ||
| // pub name: Option<ByteArray>, // not present in protobuf |
There was a problem hiding this comment.
Are we keeping this on purpose?
There was a problem hiding this comment.
Farhad created the first version of these structs from tendermint-rs domain types -- which has this field. But in Cairo, I am using the structs for protobuf messages as well -- which doesn't have this field.
I guess, we can remove this.
| eq = false; | ||
| break; |
There was a problem hiding this comment.
We could do an early return and remove the need of eq variable
| eq = false; | |
| break; | |
| return false; |
There was a problem hiding this comment.
good catch! in old Cairo version, we couldn't return within a loop. It seems, it is allowed in the latest version.
| /// Type of vote (prevote or precommit) | ||
| pub vote_type: VoteType, | ||
| /// Block height | ||
| pub height: i64, |
There was a problem hiding this comment.
Didn't we change height to be u64?
| pub height: i64, | |
| pub height: u64, |
There was a problem hiding this comment.
I am not sure, why this was made to u64. i64 and u64 have different encoding in protobuf. e.g. encoding of 1u64 and 1i64 will be totally different.
maybe we can have protobuf types and then domain types (which may validate and maintain only u64) -- but for now, let's maintain a single version of the types.
| // validator_sets_match( | ||
| // untrusted.next_validators, untrusted.signed_header.header.next_validators_hash, | ||
| // ); |
There was a problem hiding this comment.
Why don't we check next validators?
There was a problem hiding this comment.
next validator field is removed.
check https://github.com/informalsystems/ibc-starknet/pull/366/files#r2022718374
| let mut context2 = Self::new(); | ||
| value.encode_raw(ref context2); | ||
| context2.buffer.len().encode_raw(ref self); | ||
| self.buffer.append(@context2.buffer); |
There was a problem hiding this comment.
Any reason for naming it context2?
| let mut context2 = Self::new(); | |
| value.encode_raw(ref context2); | |
| context2.buffer.len().encode_raw(ref self); | |
| self.buffer.append(@context2.buffer); | |
| let mut context = Self::new(); | |
| value.encode_raw(ref context); | |
| context.buffer.len().encode_raw(ref self); | |
| self.buffer.append(@context.buffer); |
There was a problem hiding this comment.
good catch ! I was copy-pasting 😄
There was a problem hiding this comment.
Ah. I just realized that I was using context2 to signify it is a second context or sub-context -- self being the first context or parent-context.
3 participants
Closes #367
Description
This PR includes my most recent local changes toward implementing CometBFT header verification. It provides the scaffolding types and functions needed for the verifier, though it's not yet complete. There are still some other underlying functions and types that need to be defined.
You can think of this as a continuation of PR #361, as it aims to integrate the signature verifier into the header verification process. The logic for these parts is included, but should be considered a first pass that definitely needs to be validated with unit tests.
Up to the team whether you want to continue implementation on top of this PR, break it down into smaller changes, or any other approach that better suits your progress.
cc @rnbguy @mpoke
Remark
I initially scaffolded everything in a new separate
cometbft_verifierlibrary, similar to how it exists as a standalone package undertendermint-rs. However, after consideration, I thought this might create unnecessary compilation overhead and complexity for our needs. For Starknet, I think the entire verifier implementation can live within thecometbftlibrary, which should meet all our requirements.