Merge #2456 #2469 #2478

2456: docs: document errors r=doitian,yangby-cryptape a=keroro520 2469: docs: add missing documents for test-chain-utils r=doitian,yangby-cryptape,keroro520 a=chuijiaolianying 2478: test: fix test "test_prepare_uncles"; it could failed with a very small chance r=zhangsoledad,keroro520 a=yangby-cryptape #### Issue - `ChainController::process_block(..)` is synchronized, it will wait for the response. https://github.com/nervosnetwork/ckb/blob/6e34d89a44aaeba3ffbfbf62c173e6a0a6547273/chain/src/chain.rs#L200-L208 - But when `ChainService::process_block(..)`, it will send `NewUncle(..)` to `TxPoolController` without waiting for the response. https://github.com/nervosnetwork/ckb/blob/6e34d89a44aaeba3ffbfbf62c173e6a0a6547273/chain/src/chain.rs#L491-L497 - So, with a very small probability, the uncle block may not have been processed by `TxPoolService`. So the follow line will throw the error: `panicked at 'index out of bounds: the len is 0 but the index is 0`. https://github.com/nervosnetwork/ckb/blob/6e34d89a44aaeba3ffbfbf62c173e6a0a6547273/chain/src/tests/block_assembler.rs#L210 #### Reproduce Append `::std::thread::sleep(::std::time::Duration::from_millis(500));` to line 629 in the follow code section to delay `TxPool` update `NewUncle` can reproduce this issue. https://github.com/nervosnetwork/ckb/blob/6e34d89a44aaeba3ffbfbf62c173e6a0a6547273/tx-pool/src/service.rs#L629-L637 Co-authored-by: keroro <[email protected]> Co-authored-by: chuijiaolianying <[email protected]> Co-authored-by: Boyu Yang <[email protected]>
nervosnetwork · Dec 30, 2020 · 4a5e8a2 · 4a5e8a2
4 parents f0999f0 + 1a850b4 + 642fb9c + 2e972bb
commit 4a5e8a2
Show file tree

Hide file tree

Showing 18 changed files with 447 additions and 255 deletions.
diff --git a/chain/src/tests/block_assembler.rs b/chain/src/tests/block_assembler.rs
@@ -201,7 +201,7 @@ fn test_prepare_uncles() {
         .unwrap()
         .unwrap();
     // block number 3, epoch 0
-    while (Into::<u64>::into(block_template.number)) != 3 {
+    while (Into::<u64>::into(block_template.number)) != 3 || block_template.uncles.is_empty() {
         block_template = tx_pool
             .get_block_template(None, None, None)
             .unwrap()

diff --git a/error/Cargo.toml b/error/Cargo.toml
@@ -4,7 +4,7 @@ version = "0.39.0-pre"
 license = "MIT"
 authors = ["Nervos Core Dev <[email protected]>"]
 edition = "2018"
-description = "TODO(doc): @keroro520 crate description"
+description = "Underlying error types used over ckb crates"
 homepage = "https://github.com/nervosnetwork/ckb"
 repository = "https://github.com/nervosnetwork/ckb"
 

diff --git a/error/src/internal.rs b/error/src/internal.rs
@@ -15,20 +15,27 @@ pub struct SilentError;
 #[error("{0}")]
 pub struct OtherError(String);
 
-/// TODO(doc): @keroro520
+/// A list specifying categories of ckb internal error.
+///
+/// This list is intended to grow over time and it is not recommended to exhaustively match against it.
+///
+/// It is used with the [`InternalError`].
+///
+/// [`InternalError`]: ../ckb_error/struct.InternalError.html
 #[derive(Debug, PartialEq, Eq, Clone, Copy, Display)]
 pub enum InternalErrorKind {
-    /// An arithmetic overflow occurs during capacity calculation,
-    /// e.g. `Capacity::safe_add`
+    /// An arithmetic overflow occurs during capacity calculation, e.g. `Capacity::safe_add`
     CapacityOverflow,
 
     /// Persistent data had corrupted
     DataCorrupted,
 
-    /// Database exception
+    /// Error occurs during database operations
     Database,
 
-    /// Block Assembler error
+    /// It indicates that the underlying error is [`BlockAssemblerError`]
+    ///
+    /// [`BlockAssemblerError`]: ../ckb_tx_pool/error/enum.BlockAssemblerError.html
     BlockAssembler,
 
     /// VM internal error
@@ -44,7 +51,7 @@ pub enum InternalErrorKind {
     Other,
 }
 
-def_error_base_on_kind!(InternalError, InternalErrorKind);
+def_error_base_on_kind!(InternalError, InternalErrorKind, "Internal error.");
 
 impl_error_conversion_with_kind!(InternalError, crate::ErrorKind::Internal, crate::Error);
 

diff --git a/error/src/lib.rs b/error/src/lib.rs
@@ -1,4 +1,4 @@
-//! TODO(doc): @keroro520
+//! Underlying error types used over ckb crates.
 
 use std::{error::Error as StdError, fmt, ops::Deref, sync::Arc};
 
@@ -15,30 +15,54 @@ use prelude::*;
 #[derive(Debug, Clone)]
 pub struct AnyError(Arc<anyhow::Error>);
 
-/// TODO(doc): @keroro520
+/// A list specifying categories of ckb error.
+///
+/// This list is intended to grow over time and it is not recommended to exhaustively match against it.
+///
+/// It is used with [`Error`].
+///
+/// [`Error`]: ./struct.Error.html
 #[derive(Debug, Clone, Copy, Eq, PartialEq, Display)]
 pub enum ErrorKind {
-    /// TODO(doc): @keroro520
+    /// It indicates that the underlying error is [`OutPointError`].
+    ///
+    /// [`OutPointError`]: ../ckb_types/core/error/enum.OutPointError.html
     OutPoint,
-    /// TODO(doc): @keroro520
+    /// It indicates that the underlying error is [`TransactionError`].
+    ///
+    /// [`TransactionError`]: ../ckb_verification/enum.TransactionError.html
     Transaction,
-    /// TODO(doc): @keroro520
+    /// It indicates that the underlying error is [`Reject`].
+    ///
+    /// [`Reject`]: ../ckb_tx_pool/error/enum.Reject.html
     SubmitTransaction,
-    /// TODO(doc): @keroro520
+    /// It indicates that the underlying error is [`TransactionScriptError`].
+    ///
+    /// [`TransactionScriptError`]: ../ckb_script/struct.TransactionScriptError.html
     Script,
-    /// TODO(doc): @keroro520
+    /// It indicates that the underlying error is [`HeaderError`].
+    ///
+    /// [`HeaderError`]: ../ckb_verification/struct.HeaderError.html
     Header,
-    /// TODO(doc): @keroro520
+    /// It indicates that the underlying error is [`BlockError`]
+    ///
+    /// [`BlockError`]: ../ckb_verification/struct.BlockError.html
     Block,
-    /// TODO(doc): @keroro520
+    /// It indicates that the underlying error is [`InternalError`]
+    ///
+    /// [`InternalError`]: ./struct.InternalError.html
     Internal,
-    /// TODO(doc): @keroro520
+    /// It indicates that the underlying error is [`DaoError`]
+    ///
+    /// [`DaoError`]: ../ckb_types/core/error/enum.OutPointError.html
     Dao,
-    /// TODO(doc): @keroro520
+    /// It indicates that the underlying error is [`SpecError`]
+    ///
+    /// [`SpecError`]: ../ckb_chain_spec/enum.SpecError.html
     Spec,
 }
 
-def_error_base_on_kind!(Error, ErrorKind);
+def_error_base_on_kind!(Error, ErrorKind, "Top-level ckb error type.");
 
 impl<E> From<E> for AnyError
 where

diff --git a/error/src/util.rs b/error/src/util.rs
@@ -1,8 +1,9 @@
-//! TODO(doc): @keroro520
+//! Error-related macros
 
-/// Compare two errors
+/// Compare two errors.
 ///
-/// Used for testing only
+/// NOTE: Used for testing only!
+#[doc(hidden)]
 #[macro_export]
 macro_rules! assert_error_eq {
     ($left:expr, $right:expr) => {
@@ -23,7 +24,27 @@ macro_rules! assert_error_eq {
     }
 }
 
-/// TODO(doc): @keroro520
+/// A macro to implement conversion from source type to target type with an implicit error kind.
+///
+/// ## Examples
+///
+/// ```ignore
+/// impl_error_conversion_with_kind!(SourceType, error_kind, TargetType)
+/// ```
+///
+/// the expanded code:
+///
+/// ```ignore
+/// impl From<SourceType> for TargetType {
+///     fn from(source: SourceType) -> TargetType {
+///         TargetType {
+///             kind: error_kind,
+///             inner: source.into(),
+///         }
+///     }
+/// }
+/// ```
+#[doc(hidden)]
 #[macro_export]
 macro_rules! impl_error_conversion_with_kind {
     ($source:ty, $kind:expr, $target:ty) => {
@@ -35,7 +56,26 @@ macro_rules! impl_error_conversion_with_kind {
     };
 }
 
-/// TODO(doc): @keroro520
+/// A macro to implement conversion from source type to target type based on an implicit middle adaptor.
+///
+/// ## Examples
+///
+/// ```ignore
+/// impl_error_conversion_with_adaptor!(SourceType, AdaptorType, TargetType)
+/// ```
+///
+/// the expanded code:
+///
+/// ```ignore
+/// impl From<SourceType> for TargetType {
+///     fn from(source: SourceType) -> TargetType {
+///         let adaptor: AdaptorType = source.into();
+///         let target: TargetType = adaptor.into();
+///         target
+///     }
+/// }
+/// ```
+#[doc(hidden)]
 #[macro_export]
 macro_rules! impl_error_conversion_with_adaptor {
     ($source:ty, $adaptor:ty, $target:ty) => {

diff --git a/util/dao/Cargo.toml b/util/dao/Cargo.toml
@@ -4,7 +4,7 @@ version = "0.39.0-pre"
 license = "MIT"
 authors = ["Nervos Core Dev <[email protected]>"]
 edition = "2018"
-description = "TODO(doc): @keroro520 crate description"
+description = "This crate provides implementation to calculate dao field"
 homepage = "https://github.com/nervosnetwork/ckb"
 repository = "https://github.com/nervosnetwork/ckb"
 

diff --git a/util/dao/src/lib.rs b/util/dao/src/lib.rs
@@ -1,4 +1,5 @@
-//! TODO(doc): @keroro520
+//! This crate provides implementation to calculate dao field.
+
 use byteorder::{ByteOrder, LittleEndian};
 use ckb_chain_spec::consensus::Consensus;
 use ckb_dao_utils::{extract_dao_data, pack_dao_data, DaoError};
@@ -17,18 +18,16 @@ use ckb_types::{
 use std::collections::HashSet;
 use std::convert::TryFrom;
 
-/// TODO(doc): @keroro520
+/// Dao field calculator
+/// `DaoCalculator` is a facade to calculate the dao field.
 pub struct DaoCalculator<'a, CS, DL> {
-    /// TODO(doc): @keroro520
-    pub consensus: &'a Consensus,
-    /// TODO(doc): @keroro520
-    pub store: &'a CS,
-    /// TODO(doc): @keroro520
-    pub data_loader: DL,
+    consensus: &'a Consensus,
+    store: &'a CS,
+    data_loader: DL,
 }
 
 impl<'a, CS: ChainStore<'a>> DaoCalculator<'a, CS, DataLoaderWrapper<'a, CS>> {
-    /// TODO(doc): @keroro520
+    /// Creates a new `DaoCalculator`.
     pub fn new(consensus: &'a Consensus, store: &'a CS) -> Self {
         let data_loader = DataLoaderWrapper::new(store);
         DaoCalculator {
@@ -38,7 +37,7 @@ impl<'a, CS: ChainStore<'a>> DaoCalculator<'a, CS, DataLoaderWrapper<'a, CS>> {
         }
     }
 
-    /// TODO(doc): @keroro520
+    /// Returns the primary block reward for `target` block.
     pub fn primary_block_reward(&self, target: &HeaderView) -> Result<Capacity, Error> {
         let target_epoch = self
             .store
@@ -49,7 +48,7 @@ impl<'a, CS: ChainStore<'a>> DaoCalculator<'a, CS, DataLoaderWrapper<'a, CS>> {
         target_epoch.block_reward(target.number())
     }
 
-    /// TODO(doc): @keroro520
+    /// Returns the secondary block reward for `target` block.
     pub fn secondary_block_reward(&self, target: &HeaderView) -> Result<Capacity, Error> {
         if target.number() == 0 {
             return Ok(Capacity::zero());
@@ -75,11 +74,13 @@ impl<'a, CS: ChainStore<'a>> DaoCalculator<'a, CS, DataLoaderWrapper<'a, CS>> {
         Ok(Capacity::shannons(reward))
     }
 
-    /// TODO(doc): @keroro520
-    // Used for testing only.
-    //
-    // Notice unlike primary_block_reward and secondary_epoch_reward above,
-    // this starts calculating from parent, not target header.
+    /// Returns the sum of primary block reward and secondary block reward for `target` block.
+    ///
+    /// Notice unlike primary_block_reward and secondary_epoch_reward above,
+    /// this starts calculating from parent, not target header.
+    ///
+    /// NOTE: Used for testing only!
+    #[doc(hidden)]
     pub fn base_block_reward(&self, parent: &HeaderView) -> Result<Capacity, Error> {
         let target_number = self
             .consensus
@@ -97,7 +98,10 @@ impl<'a, CS: ChainStore<'a>> DaoCalculator<'a, CS, DataLoaderWrapper<'a, CS>> {
         Ok(primary_block_reward.safe_add(secondary_block_reward)?)
     }
 
-    /// TODO(doc): @keroro520
+    /// Calculates the new dao field after packaging these transactions. It returns the dao field in [`Byte32`] format. Please see [`extract_dao_data`] if you intend to see the detailed content.
+    ///
+    /// [`Byte32`]: ../ckb_types/packed/struct.Byte32.html
+    /// [`extract_dao_data`]: ../ckb_dao_utils/fn.extract_dao_data.html
     pub fn dao_field(
         &self,
         rtxs: &[ResolvedTransaction],
@@ -163,7 +167,9 @@ impl<'a, CS: ChainStore<'a>> DaoCalculator<'a, CS, DataLoaderWrapper<'a, CS>> {
         Ok(pack_dao_data(current_ar, current_c, current_s, current_u))
     }
 
-    /// TODO(doc): @keroro520
+    /// Returns the maximum capacity that the deposited `out_point` acts [withdrawing phase 1] at `withdrawing_header_hash.`
+    ///
+    /// [withdrawing phase 1]: https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0023-dao-deposit-withdraw/0023-dao-deposit-withdraw.md#withdraw-phase-1
     pub fn maximum_withdraw(
         &self,
         out_point: &OutPoint,
@@ -189,7 +195,7 @@ impl<'a, CS: ChainStore<'a>> DaoCalculator<'a, CS, DataLoaderWrapper<'a, CS>> {
         )
     }
 
-    /// TODO(doc): @keroro520
+    /// Returns the total transactions fee of `rtx`.
     pub fn transaction_fee(&self, rtx: &ResolvedTransaction) -> Result<Capacity, Error> {
         let maximum_withdraw = self.transaction_maximum_withdraw(rtx)?;
         rtx.transaction

diff --git a/util/dao/utils/Cargo.toml b/util/dao/utils/Cargo.toml
@@ -4,7 +4,7 @@ version = "0.39.0-pre"
 license = "MIT"
 authors = ["Nervos Core Dev <[email protected]>"]
 edition = "2018"
-description = "TODO(doc): @keroro520 crate description"
+description = "This crate provides several util functions to operate the dao field and NervosDAO related errors"
 homepage = "https://github.com/nervosnetwork/ckb"
 repository = "https://github.com/nervosnetwork/ckb"
 

diff --git a/util/dao/utils/src/error.rs b/util/dao/utils/src/error.rs
@@ -1,21 +1,41 @@
 use ckb_error::{prelude::*, Error, ErrorKind};
 
-/// TODO(doc): @keroro520
+/// Errors due to the fact that the NervosDAO rules are not respected.
+///
+/// [NervosDAO]: https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0023-dao-deposit-withdraw/0023-dao-deposit-withdraw.md
 #[derive(Error, Debug, PartialEq, Clone, Eq)]
 pub enum DaoError {
-    /// TODO(doc): @keroro520
+    /// This error occurs during calculating the dao field for a block, which broadly indicates that it cannot find a required block.
     #[error("InvalidHeader")]
     InvalidHeader,
-    /// TODO(doc): @keroro520
+
+    /// When withdraws from NervosDAO, it requires the deposited header and withdrawing header to help calculating interest.
+    /// This error occurs at [withdrawing phase 2] for the below cases:
+    ///   - The [`HeaderDeps`] does not include the withdrawing block hash. The withdrawing block hash
+    ///     indicates the block which packages the target transaction at [withdrawing phase 1].
+    ///   - The [`HeaderDeps`] does not include the deposited block hash. The deposited block hash
+    ///     indicates the block which packages the target transaction at [deposit phase]. Please see
+    ///     [withdrawing phase 2] for more details.
+    ///
+    /// [deposit phase]: https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0023-dao-deposit-withdraw/0023-dao-deposit-withdraw.md#deposit
+    /// [withdrawing phase 1]: https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0023-dao-deposit-withdraw/0023-dao-deposit-withdraw.md#withdraw-phase-1
+    /// [withdrawing phase 2]: https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0023-dao-deposit-withdraw/0023-dao-deposit-withdraw.md#withdraw-phase-2
     #[error("InvalidOutPoint")]
     InvalidOutPoint,
-    /// TODO(doc): @keroro520
+
+    /// When withdraws from NervosDAO, the deposited header should be specified via witness. This error
+    /// indicates the corresponding witness is unexpected. See
+    /// [the code](https://github.com/nervosnetwork/ckb/blob/69ff8311cdb312a0ef45d524060719eea5e90e9e/util/dao/src/lib.rs#L280-L301)
+    /// for more detail.
+    ///
+    /// See also:
+    /// - [0023-dao-deposit-withdraw](https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0023-dao-deposit-withdraw/0023-dao-deposit-withdraw.md)
     #[error("InvalidDaoFormat")]
     InvalidDaoFormat,
-    /// TODO(doc): @keroro520
+    /// Calculation overflow
     #[error("Overflow")]
     Overflow,
-    /// TODO(doc): @keroro520
+    /// ZeroC
     #[error("ZeroC")]
     ZeroC,
 }