Repository: chainx-org/ChainX (language: rust)

Issue:
Improve bitcoin
Signed-off-by: koushiro <koushiro.cqx@gmail.com>


Discussion / hints:

Fix patch:
diff --git a/xpallets/gateway/bitcoin/src/header/header_proof.rs b/xpallets/gateway/bitcoin/src/header/header_proof.rs
index c443d8315..45610ffb3 100644
--- a/xpallets/gateway/bitcoin/src/header/header_proof.rs
+++ b/xpallets/gateway/bitcoin/src/header/header_proof.rs
@@ -1,20 +1,16 @@
 // Copyright 2019-2020 ChainX Project Authors. Licensed under GPL-3.0.
 
-// Substrate
 use frame_support::{dispatch::DispatchResult, traits::UnixTime};
 use sp_std::{cmp, convert::TryFrom};
 
-// ChainX
-use xp_logging::{debug, error, info, warn};
-
-// light-bitcoin
 use light_bitcoin::{
     chain::BlockHeader as BtcHeader,
     keys::Network,
-    primitives::{Compact, H256, U256},
+    primitives::{hash_rev, Compact, H256, U256},
 };
 
-use super::ChainErr;
+use xp_logging::{debug, error, info, warn};
+
 use crate::types::{BtcHeaderInfo, BtcParams};
 use crate::{Error, Module, Trait};
 
@@ -25,17 +21,17 @@ pub struct HeaderVerifier<'a> {
 }
 
 impl<'a> HeaderVerifier<'a> {
-    pub fn new<T: Trait>(header_info: &'a BtcHeaderInfo) -> Result<Self, ChainErr> {
+    pub fn new<T: Trait>(header_info: &'a BtcHeaderInfo) -> Self {
         let current = T::UnixTime::now();
         // if convert from u64 to u32 failed, ignore timestamp check
         // timestamp check are not important
         let current_time = u32::try_from(current.as_secs()).ok();
 
-        Ok(Self {
+        Self {
             work: HeaderWork::new(header_info),
             proof_of_work: HeaderProofOfWork::new(&header_info.header),
             timestamp: HeaderTimestamp::new(&header_info.header, current_time),
-        })
+        }
     }
 
     pub fn check<T: Trait>(&self) -> DispatchResult {
@@ -47,9 +43,7 @@ impl<'a> HeaderVerifier<'a> {
         self.proof_of_work.check::<T>(&params)?;
         // ignore this in benchmarks
         #[cfg(not(feature = "runtime-benchmarks"))]
-        {
-            self.timestamp.check::<T>(&params)?;
-        }
+        self.timestamp.check::<T>(&params)?;
 
         Ok(())
     }
@@ -103,12 +97,12 @@ pub fn work_required<T: Trait>(parent_hash: H256, height: u32, params: &BtcParam
     parent_header.bits
 }
 
-pub fn is_retarget_height(height: u32, p: &BtcParams) -> bool {
+fn is_retarget_height(height: u32, p: &BtcParams) -> bool {
     height % p.retargeting_interval() == 0
 }
 
 /// Algorithm used for retargeting work every 2 weeks
-pub fn work_required_retarget<T: Trait>(
+fn work_required_retarget<T: Trait>(
     parent_header: BtcHeader,
     height: u32,
     params: &BtcParams,
@@ -162,7 +156,7 @@ pub fn work_required_retarget<T: Trait>(
 }
 
 /// Returns constrained number of seconds since last retarget
-pub fn retarget_timespan(retarget_timestamp: u32, last_timestamp: u32, p: &BtcParams) -> u32 {
+fn retarget_timespan(retarget_timestamp: u32, last_timestamp: u32, p: &BtcParams) -> u32 {
     // TODO i64??
     // subtract unsigned 32 bit numbers in signed 64 bit space in
     // order to prevent underflow before applying the range constraint.
@@ -188,7 +182,7 @@ impl<'a> HeaderProofOfWork<'a> {
     }
 
     fn check<T: Trait>(&self, p: &BtcParams) -> DispatchResult {
-        if is_valid_proof_of_work(p.max_bits(), self.header.bits, &self.header.hash()) {
+        if is_valid_proof_of_work(p.max_bits(), self.header.bits, self.header.hash()) {
             Ok(())
         } else {
             Err(Error::<T>::InvalidPoW.into())
@@ -196,26 +190,14 @@ impl<'a> HeaderProofOfWork<'a> {
     }
 }
 
-fn reverse_hash256(hash: &H256) -> H256 {
-    let mut res: H256 = H256::from_slice(hash.as_bytes());
-    let bytes = res.as_bytes_mut();
-    bytes.reverse();
-    res
-}
-
-pub fn is_valid_proof_of_work(max_work_bits: Compact, bits: Compact, hash: &H256) -> bool {
-    let maximum = match max_work_bits.to_u256() {
-        Ok(max) => max,
-        _err => return false,
-    };
-
-    let target = match bits.to_u256() {
-        Ok(target) => target,
-        _err => return false,
-    };
-
-    let value = U256::from(reverse_hash256(hash).as_bytes());
-    target <= maximum && value <= target
+fn is_valid_proof_of_work(max_work_bits: Compact, bits: Compact, hash: H256) -> bool {
+    match (max_work_bits.to_u256(), bits.to_u256()) {
+        (Ok(maximum), Ok(target)) => {
+            let value = U256::from(hash_rev(hash).as_bytes());
+            target <= maximum && value <= target
+        }
+        _ => false,
+    }
 }
 
 pub struct HeaderTimestamp<'a> {
@@ -249,7 +231,7 @@ impl<'a> HeaderTimestamp<'a> {
             // if get chain timestamp error, just ignore blockhead time check
             warn!(
                 "[check_header_timestamp] Header:{:?}, get unix timestamp error, ignore it",
-                self.header.hash()
+                hash_rev(self.header.hash())
             );
             Ok(())
         }
diff --git a/xpallets/gateway/bitcoin/src/header/mod.rs b/xpallets/gateway/bitcoin/src/header/mod.rs
index 607281a4c..2c79462ce 100644
--- a/xpallets/gateway/bitcoin/src/header/mod.rs
+++ b/xpallets/gateway/bitcoin/src/header/mod.rs
@@ -2,54 +2,29 @@
 
 mod header_proof;
 
-// Substrate
 use frame_support::{StorageMap, StorageValue};
 use sp_runtime::DispatchResult;
 use sp_std::{cmp::Ordering, prelude::*};
 
-// ChainX
-use xp_logging::{error, info};
+use light_bitcoin::primitives::{hash_rev, H256};
 
-// light-bitcoin
-use light_bitcoin::primitives::H256;
+use xp_logging::{error, info};
 
 use crate::types::{BtcHeaderIndex, BtcHeaderInfo};
 use crate::{ConfirmedIndex, Error, MainChain, Module, Trait};
 
 pub use self::header_proof::HeaderVerifier;
 
-#[derive(PartialEq, Eq, Copy, Clone)]
-pub enum ChainErr {
-    /// Not Found
-    NotFound,
-    /// Ancient fork
-    AncientFork,
-}
-
-impl ChainErr {
-    pub fn to_err<T: Trait>(self) -> Error<T> {
-        self.into()
-    }
-}
-
-impl<T: Trait> From<ChainErr> for Error<T> {
-    fn from(err: ChainErr) -> Self {
-        match err {
-            ChainErr::NotFound => Error::<T>::HeaderNotFound,
-            ChainErr::AncientFork => Error::<T>::HeaderAncientFork,
-        }
-    }
-}
-
-/// todo move this issue to ChainX-org/ChainX repo
-/// #issue 501 https://github.com/chainpool/ChainX/issues/501 would explain how to define
-/// confirmation count
-///      confirmed_height = now_height - (confirmations - 1)
+/// Look back the headers to pick the confirmed index,
+/// return the header indexes on the look back path.
+///
+/// The definition of block confirmation count:
+/// confirmed_height = now_height - (confirmations - 1)
 ///           |--- confirmations = 4 ---|
 /// b(prev) - b(confirm)  -  b  -  b  - b
-///           4              3     2    1 (confirmations)
-///           97             98    99   100(height)
-/// this function would pick the confirmed Index, and return the Index on the look back path
+///           4              3     2    1       (confirmations)
+///           97             98    99   100     (height)
+///
 fn look_back_confirmed_header<T: Trait>(
     header_info: &BtcHeaderInfo,
 ) -> (Option<BtcHeaderIndex>, Vec<BtcHeaderIndex>) {
@@ -63,7 +38,7 @@ fn look_back_confirmed_header<T: Trait>(
         height: header_info.height,
     });
     // e.g. when confirmations is 4, loop 3 times max
-    for i in 1..confirmations {
+    for cnt in 1..confirmations {
         if let Some(current_info) = Module::<T>::headers(&prev_hash) {
             chain.push(BtcHeaderIndex {
                 hash: prev_hash,
@@ -71,10 +46,13 @@ fn look_back_confirmed_header<T: Trait>(
             });
             prev_hash = current_info.header.previous_header_hash;
         } else {
-            // if not find current header info, should be exceed genesis height, jump out of loop
+            // if cannot find current header info, should be exceed genesis height, jump out of loop
+            // e.g. want to get the previous header of #98, but genesis height is 98,
+            // obviously, we cannot find the header of #97.
             info!(
                 "[update_confirmed_header] Can not find header ({:?}), current reverse count:{}",
-                prev_hash, i
+                hash_rev(prev_hash),
+                cnt
             );
             break;
         }
@@ -82,7 +60,7 @@ fn look_back_confirmed_header<T: Trait>(
     let len = chain.len();
     if len == confirmations as usize {
         // confirmations must more than 0, thus, chain.last() must be some
-        (chain.last().map(Clone::clone), chain)
+        (chain.last().cloned(), chain)
     } else {
         (None, chain)
     }
@@ -97,21 +75,6 @@ pub fn update_confirmed_header<T: Trait>(header_info: &BtcHeaderInfo) -> Option<
         ConfirmedIndex::put(index);
         index
     })
-
-    // if let Some(index) = confirmed {
-    //     // update confirmed index
-
-    // } else {
-    //     info!(
-    //         "[update_confirmed_header]|not find prev header, use genesis instead|prev:{:?}",
-    //         should_confirmed_hash
-    //     );
-    //     let info = Module::<T>::genesis_info();
-    //     BtcHeaderIndex {
-    //         hash: info.0.hash(),
-    //         height: info.1,
-    //     }
-    // }
 }
 
 pub fn check_confirmed_header<T: Trait>(header_info: &BtcHeaderInfo) -> DispatchResult {
@@ -125,7 +88,7 @@ pub fn check_confirmed_header<T: Trait>(header_info: &BtcHeaderInfo) -> Dispatch
                     // b  ---------------- b  ------ b --- b --- b(best)
                     // |(now_confirmed)--- b  ------ b --- b(now)
                     // 99              100       101  102    103
-                    // current_confirmed > now_ocnfirmed
+                    // current_confirmed > now_confirmed
                     Ok(())
                 }
                 Ordering::Equal => {
diff --git a/xpallets/gateway/bitcoin/src/lib.rs b/xpallets/gateway/bitcoin/src/lib.rs
index f1c351d5b..dd7b5182c 100644
--- a/xpallets/gateway/bitcoin/src/lib.rs
+++ b/xpallets/gateway/bitcoin/src/lib.rs
@@ -504,10 +504,9 @@ impl<T: Trait> Module<T> {
             header,
             height: prev_info.height + 1,
         };
-        // check
-        let c =
-            header::HeaderVerifier::new::<T>(&header_info).map_err::<Error<T>, _>(Into::into)?;
-        c.check::<T>()?;
+        // verify header
+        let header_verifier = header::HeaderVerifier::new::<T>(&header_info);
+        header_verifier.check::<T>()?;
 
         with_transaction_result(|| {
             // insert into storage


Test patch: