Merge pull request #3 from datachainlab/misc

Fix error handlings and seals verification

Signed-off-by: Jun Kimura <[email protected]>

.github/workflows/test.yml

@@ -33,6 +33,8 @@ jobs:
       - uses: actions/setup-go@v5
         with:
           go-version: '1.21'
+          cache-dependency-path: |
+            e2e/relayer/go.sum
       - uses: dtolnay/rust-toolchain@nightly
       - uses: datachainlab/rust-cache@allow_registry_src_caching
         with:

README.md

@@ -1 +1,21 @@
-# besu-qbft-elc
+# besu-qbft-elc
+
+[![test](https://github.com/datachainlab/besu-qbft-elc/actions/workflows/test.yml/badge.svg?branch=main)](https://github.com/datachainlab/besu-qbft-elc/actions/workflows/test.yml)
+
+This repository provides a light client for the Hyperledger Besu's QBFT consensus algorithm. The light client is implemented as the Enclave Light Client (ELC) for [LCP](https://github.com/datachainlab/lcp).
+
+## Replated Repositories
+
+- [ibc-solidity](https://github.com/hyperledger-labs/yui-ibc-solidity): An IBC implementation in Solidity.
+- [yui-relayer](https://github.com/hyperledger-labs/yui-relayer): A relayer implementation for the IBC protocol, which also supports heterogeneous blockchains.
+- [besu-ibc-relay-prover](https://github.com/datachainlab/besu-ibc-relay-prover): The relayer's prover module for the Hyperledger Besu.
+
+## E2E Test
+
+**Prerequisites: Please check [the github actions workflow file](.github/workflows/test.yml) for the required dependencies.**
+
+You can run the e2e relay test by running the following commands:
+
+```bash
+make -C e2e e2e-test
+```

elc/src/client.rs

@@ -79,12 +79,12 @@ impl LightClient for BesuQBFTLightClient {
         let eth_header = EthHeader::parse(header.besu_header_rlp.as_slice())?;
         let commit_hash = eth_header.commit_hash()?;
 
-        self.verify_commit_seals_trusting(
+        Self::verify_commit_seals_trusting(
             &trusted_consensus_state.validators,
             &header.seals,
             commit_hash,
         )?;
-        self.verify_commit_seals_untrusting(
+        Self::verify_commit_seals_untrusting(
             &eth_header.extra.validators,
             &header.seals,
             commit_hash,
@@ -215,18 +215,13 @@ impl BesuQBFTLightClient {
     }
 
     fn verify_commit_seals_trusting(
-        &self,
         trusted_validators: &[Address],
         committed_seals: &[Vec<u8>],
         commit_hash: H256,
     ) -> Result<(), Error> {
         let mut marked = vec![false; trusted_validators.len()];
         let mut success = 0;
-        for seal in committed_seals {
-            if seal.is_empty() {
-                continue;
-            }
-
+        for seal in committed_seals.iter().filter(|seal| !seal.is_empty()) {
             let addr = verify_signature(commit_hash, seal)?;
             if let Some(pos) = trusted_validators.iter().position(|v| v == &addr) {
                 if !marked[pos] {
@@ -235,38 +230,46 @@ impl BesuQBFTLightClient {
                 }
             }
         }
-        if success * 3 <= trusted_validators.len() * 2 {
-            panic!("success * 3 <= trusted_validators.len() * 2");
+        if success * 3 <= trusted_validators.len() {
+            Err(Error::InsufficientTrustedValidatorsSeals {
+                actual: success * 3,
+                threshold: trusted_validators.len(),
+            })
+        } else {
+            Ok(())
         }
-        Ok(())
     }
 
     /// CONTRACT: the order of `committed_seals` must be corresponding to the order of `validators`
     fn verify_commit_seals_untrusting(
-        &self,
         untrusted_validators: &[Address],
         committed_seals: &[Vec<u8>],
         commit_hash: H256,
     ) -> Result<(), Error> {
         if untrusted_validators.len() != committed_seals.len() {
-            panic!("untrusted_validators.len() != committed_seals.len()");
+            return Err(Error::UntrustedValidatorsAndCommittedSealsLengthMismatch {
+                untrusted_validators_len: untrusted_validators.len(),
+                committed_seals_len: committed_seals.len(),
+            });
         }
-
         let mut success = 0;
-        for (validator, seal) in untrusted_validators.iter().zip(committed_seals.iter()) {
-            if seal.is_empty() {
-                continue;
-            }
-
+        for (validator, seal) in untrusted_validators
+            .iter()
+            .zip(committed_seals.iter())
+            .filter(|(_, seal)| !seal.is_empty())
+        {
             let addr = verify_signature(commit_hash, seal)?;
             if addr == *validator {
                 success += 1;
             }
         }
-
-        if success * 3 <= untrusted_validators.len() * 2 {
-            panic!("success * 3 <= untrusted_validators.len() * 2");
+        if success * 3 < untrusted_validators.len() * 2 {
+            Err(Error::InsuffientUntrustedValidatorsSeals {
+                actual: success * 3,
+                threshold: untrusted_validators.len() * 2,
+            })
+        } else {
+            Ok(())
         }
-        Ok(())
     }
 }

elc/src/client_state.rs

@@ -52,7 +52,11 @@ impl TryFrom<RawClientState> for ClientState {
     fn try_from(value: RawClientState) -> Result<Self, Self::Error> {
         Ok(ClientState {
             chain_id: U256::from_be_slice(&value.chain_id),
-            ibc_store_address: value.ibc_store_address.as_slice().try_into().unwrap(),
+            ibc_store_address: value
+                .ibc_store_address
+                .as_slice()
+                .try_into()
+                .map_err(Error::SliceToArrayConversionError)?,
             latest_height: value.latest_height.map_or(Height::zero(), |height| {
                 Height::new(height.revision_number, height.revision_height)
             }),

elc/src/commitment.rs

@@ -11,10 +11,10 @@ pub fn decode_eip1184_rlp_proof(proof: &[u8]) -> Result<Vec<Vec<u8>>, Error> {
     if r.is_list() {
         Ok(r.into_iter()
             .map(|r| {
-                let elems: Vec<Vec<u8>> = r.as_list().unwrap();
-                rlp::encode_list::<Vec<u8>, Vec<u8>>(&elems).into()
+                let elems: Vec<Vec<u8>> = r.as_list()?;
+                Ok(rlp::encode_list::<Vec<u8>, Vec<u8>>(&elems).into())
             })
-            .collect())
+            .collect::<Result<Vec<Vec<u8>>, Error>>()?)
     } else {
         Err(Error::InvalidRLPFormatNotList(proof.to_vec()))
     }
@@ -40,14 +40,15 @@ pub fn keccak256(bz: &[u8]) -> [u8; 32] {
 }
 
 pub fn verify_signature(sign_hash: H256, signature: &[u8]) -> Result<Address, Error> {
-    assert!(signature.len() == 65);
-
+    if signature.len() != 65 {
+        return Err(Error::InvalidSignatureLength(signature.len()));
+    }
     let mut s = Scalar::default();
     let _ = s.set_b32(&sign_hash.to_be_bytes());
 
-    let sig = Signature::parse_overflowing_slice(&signature[..64]).unwrap();
-    let rid = RecoveryId::parse(signature[64]).unwrap();
-    let signer: PublicKey = libsecp256k1::recover(&Message(s), &sig, &rid).unwrap();
+    let sig = Signature::parse_overflowing_slice(&signature[..64])?;
+    let rid = RecoveryId::parse(signature[64])?;
+    let signer: PublicKey = libsecp256k1::recover(&Message(s), &sig, &rid)?;
     Ok(address_from_pubkey(&signer))
 }
 

elc/src/consensus_state.rs

@@ -36,8 +36,12 @@ impl TryFrom<RawConsensusState> for ConsensusState {
             validators: value
                 .validators
                 .iter()
-                .map(|v| v.as_slice().try_into().unwrap())
-                .collect(),
+                .map(|v| {
+                    v.as_slice()
+                        .try_into()
+                        .map_err(Error::SliceToArrayConversionError)
+                })
+                .collect::<Result<_, _>>()?,
         })
     }
 }

elc/src/errors.rs

@@ -23,7 +23,6 @@ pub enum Error {
 
     /// invalid rlp format: not list: `{0:?}``
     InvalidRLPFormatNotList(Vec<u8>),
-
     /// invalid state root length: `{0}`
     InvalidStateRootLength(usize),
     /// invalid block number length: `{0}`
@@ -41,6 +40,19 @@ pub enum Error {
     /// unexpected client type: `{0}`
     UnexpectedClientType(String),
 
+    /// Invalid signature length: `{0}`
+    InvalidSignatureLength(usize),
+
+    /// untrusted validators and committed seals length mismatch: untrusted_validators_len={untrusted_validators_len} committed_seals_len={committed_seals_len}
+    UntrustedValidatorsAndCommittedSealsLengthMismatch {
+        untrusted_validators_len: usize,
+        committed_seals_len: usize,
+    },
+    /// insufficient trusted validators seals: actual={actual} threshold={threshold}
+    InsufficientTrustedValidatorsSeals { actual: usize, threshold: usize },
+    /// insufficient untrusted validators seals: actual={actual} threshold={threshold}
+    InsuffientUntrustedValidatorsSeals { actual: usize, threshold: usize },
+
     /// account not found: state_root={0:?} address={1:?}
     AccountNotFound(H256, Address),
     /// account storage root mismatch: expected={0:?} actual={1:?}
@@ -74,6 +86,10 @@ pub enum Error {
     Decode(prost::DecodeError),
     /// rlp decode error: `{0}`
     Rlp(rlp::DecoderError),
+    /// secp256k1 error: `{0}`
+    Secp256k1(libsecp256k1::Error),
+    /// conversion error from slice to array: `{0}`
+    SliceToArrayConversionError(core::array::TryFromSliceError),
 }
 
 impl LightClientSpecificError for Error {}
@@ -101,3 +117,9 @@ impl From<rlp::DecoderError> for Error {
         Self::Rlp(value)
     }
 }
+
+impl From<libsecp256k1::Error> for Error {
+    fn from(value: libsecp256k1::Error) -> Self {
+        Self::Secp256k1(value)
+    }
+}

elc/src/header.rs

@@ -39,6 +39,7 @@ impl QbftExtra {
         if it.len() != 5 {
             return Err(Error::InvalidHeaderExtraSize(it.len()));
         }
+        // unwrap is safe here because we have checked the length
         let vanity_data: Vec<u8> = it.next().unwrap().as_val()?;
         let validators: Vec<Vec<u8>> = it.next().unwrap().as_list()?;
         let raw_vote = it.next().unwrap().as_raw();