From fb708ca74b1665707350d8be4079f2d26e3086d1 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 31 Oct 2022 11:34:46 +1100 Subject: [PATCH 1/7] Add newline after brief description rustdocs should contain a newline to separate the brief description from the rest of long description. --- internals/src/hex/display.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/internals/src/hex/display.rs b/internals/src/hex/display.rs index 0077f78a..a2dbabf1 100644 --- a/internals/src/hex/display.rs +++ b/internals/src/hex/display.rs @@ -11,6 +11,7 @@ use super::Case; use crate::prelude::*; /// Extension trait for types that can be displayed as hex. +/// /// Types that have a single, obvious text representation being hex should **not** implement this /// trait and simply implement `Display` instead. /// From 31740710ee72d8499bbb6c0084bfd565096267eb Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 31 Oct 2022 12:23:10 +1100 Subject: [PATCH 2/7] blockdata: Improve rustdocs Do an audit of the `blockdata` module and clean up rustdocs. --- bitcoin/src/blockdata/block.rs | 6 ++--- bitcoin/src/blockdata/constants.rs | 20 +++++++-------- bitcoin/src/blockdata/script.rs | 38 ++++++++++++++++------------ bitcoin/src/blockdata/transaction.rs | 38 ++++++++++++++++------------ bitcoin/src/blockdata/witness.rs | 38 +++++++++++++++------------- 5 files changed, 77 insertions(+), 63 deletions(-) diff --git a/bitcoin/src/blockdata/block.rs b/bitcoin/src/blockdata/block.rs index 1fde7047..b56fbc64 100644 --- a/bitcoin/src/blockdata/block.rs +++ b/bitcoin/src/blockdata/block.rs @@ -96,7 +96,7 @@ impl Header { } } -/// Bitcoin block version number +/// Bitcoin block version number. /// /// Originally used as a protocol version, but repurposed for soft-fork signaling. /// @@ -146,7 +146,7 @@ impl Version { self.0 } - /// Check whether the version number is signalling a soft fork at the given bit. + /// Checks whether the version number is signalling a soft fork at the given bit. /// /// A block is signalling for a soft fork under BIP-9 if the first 3 bits are `001` and /// the version bit for the specific soft fork is toggled on. @@ -213,7 +213,7 @@ impl Block { self.header.block_hash() } - /// check if merkle root of header matches merkle root of the transaction list + /// Checks if merkle root of header matches merkle root of the transaction list. pub fn check_merkle_root(&self) -> bool { match self.compute_merkle_root() { Some(merkle_root) => self.header.merkle_root == merkle_root, diff --git a/bitcoin/src/blockdata/constants.rs b/bitcoin/src/blockdata/constants.rs index 8a9097dd..33ef6ad9 100644 --- a/bitcoin/src/blockdata/constants.rs +++ b/bitcoin/src/blockdata/constants.rs @@ -26,21 +26,21 @@ use crate::network::constants::Network; use crate::pow::CompactTarget; use crate::internal_macros::impl_bytes_newtype; -/// How many satoshis are in "one bitcoin" +/// How many satoshis are in "one bitcoin". pub const COIN_VALUE: u64 = 100_000_000; -/// How many seconds between blocks we expect on average +/// How many seconds between blocks we expect on average. pub const TARGET_BLOCK_SPACING: u32 = 600; -/// How many blocks between diffchanges +/// How many blocks between diffchanges. pub const DIFFCHANGE_INTERVAL: u32 = 2016; -/// How much time on average should occur between diffchanges +/// How much time on average should occur between diffchanges. pub const DIFFCHANGE_TIMESPAN: u32 = 14 * 24 * 3600; -/// The maximum allowed weight for a block, see BIP 141 (network rule) +/// The maximum allowed weight for a block, see BIP 141 (network rule). pub const MAX_BLOCK_WEIGHT: u32 = 4_000_000; -/// The minimum transaction weight for a valid serialized transaction +/// The minimum transaction weight for a valid serialized transaction. pub const MIN_TRANSACTION_WEIGHT: u32 = 4 * 60; -/// The factor that non-witness serialization data is multiplied by during weight calculation +/// The factor that non-witness serialization data is multiplied by during weight calculation. pub const WITNESS_SCALE_FACTOR: usize = 4; -/// The maximum allowed number of signature check operations in a block +/// The maximum allowed number of signature check operations in a block. pub const MAX_BLOCK_SIGOPS_COST: i64 = 80_000; /// Mainnet (bitcoin) pubkey address prefix. pub const PUBKEY_ADDRESS_PREFIX_MAIN: u8 = 0; // 0x00 @@ -62,7 +62,7 @@ pub const MAX_SCRIPTNUM_VALUE: u32 = 0x80000000; // 2^31 /// if you are doing anything remotely sane with monetary values). pub const MAX_MONEY: u64 = 21_000_000 * COIN_VALUE; -/// Constructs and returns the coinbase (and only) transaction of the Bitcoin genesis block +/// Constructs and returns the coinbase (and only) transaction of the Bitcoin genesis block. fn bitcoin_genesis_tx() -> Transaction { // Base let mut ret = Transaction { @@ -101,7 +101,7 @@ fn bitcoin_genesis_tx() -> Transaction { ret } -/// Constructs and returns the genesis block +/// Constructs and returns the genesis block. pub fn genesis_block(network: Network) -> Block { let txdata = vec![bitcoin_genesis_tx()]; let hash: sha256d::Hash = txdata[0].txid().into(); diff --git a/bitcoin/src/blockdata/script.rs b/bitcoin/src/blockdata/script.rs index 5f9b20cd..701de9e1 100644 --- a/bitcoin/src/blockdata/script.rs +++ b/bitcoin/src/blockdata/script.rs @@ -156,17 +156,17 @@ pub enum Error { /// Something did a non-minimal push; for more information see /// `https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki#Push_operators` NonMinimalPush, - /// Some opcode expected a parameter, but it was missing or truncated + /// Some opcode expected a parameter but it was missing or truncated. EarlyEndOfScript, - /// Tried to read an array off the stack as a number when it was more than 4 bytes + /// Tried to read an array off the stack as a number when it was more than 4 bytes. NumericOverflow, - /// Error validating the script with bitcoinconsensus library + /// Error validating the script with bitcoinconsensus library. #[cfg(feature = "bitcoinconsensus")] #[cfg_attr(docsrs, doc(cfg(feature = "bitcoinconsensus")))] BitcoinConsensus(bitcoinconsensus::Error), - /// Can not find the spent output + /// Can not find the spent output. UnknownSpentOutput(OutPoint), - /// Can not serialize the spending transaction + /// Can not serialize the spending transaction. Serialization } @@ -266,7 +266,8 @@ impl From for Error { } } -/// Helper to encode an integer in script(minimal CScriptNum) format. +/// Encodes an integer in script(minimal CScriptNum) format. +/// /// Writes bytes into the buffer and returns the number of bytes written. pub fn write_scriptint(out: &mut [u8; 8], n: i64) -> usize { let mut len = 0; @@ -297,7 +298,8 @@ pub fn write_scriptint(out: &mut [u8; 8], n: i64) -> usize { len } -/// Helper to decode an integer in script(minimal CScriptNum) format +/// Decodes an integer in script(minimal CScriptNum) format. +/// /// Notice that this fails on overflow: the result is the same as in /// bitcoind, that only 4-byte signed-magnitude values may be read as /// numbers. They can be added or subtracted (and a long time ago, @@ -343,6 +345,8 @@ pub fn read_scriptint(v: &[u8]) -> Result { Ok(ret) } +/// Decodes a boolean. +/// /// This is like "`read_scriptint` then map 0 to false and everything /// else as true", except that the overflow rules don't apply. #[inline] @@ -353,7 +357,7 @@ pub fn read_scriptbool(v: &[u8]) -> bool { } } -/// Read a script-encoded unsigned integer +/// Decodes a script-encoded unsigned integer. /// /// ## Errors /// @@ -395,7 +399,7 @@ impl Script { /// Creates a new empty script. pub fn new() -> Script { Script(vec![].into_boxed_slice()) } - /// Creates a new script builder + /// Creates a new script builder. pub fn builder() -> Builder { Builder::new() } @@ -863,15 +867,16 @@ pub struct Instructions<'a> { } impl<'a> Instructions<'a> { - /// Set the iterator to end so that it won't iterate any longer + /// Sets the iterator to end so that it won't iterate any longer. fn kill(&mut self) { let len = self.data.len(); self.data.nth(len.max(1) - 1); } - /// takes `len` bytes long slice from iterator and returns it advancing iterator - /// if the iterator is not long enough [`Error::EarlyEndOfScript`] is returned and the iterator is killed - /// to avoid returning an infinite stream of errors. + /// Takes a `len` bytes long slice from iterator and returns it, advancing the iterator. + /// + /// If the iterator is not long enough [`Error::EarlyEndOfScript`] is returned and the iterator + /// is killed to avoid returning an infinite stream of errors. fn take_slice_or_kill(&mut self, len: usize) -> Result<&'a [u8], Error> { if self.data.len() >= len { let slice = &self.data.as_slice()[..len]; @@ -968,9 +973,10 @@ impl Builder { /// Checks whether the script is the empty script. pub fn is_empty(&self) -> bool { self.0.is_empty() } - /// Adds instructions to push an integer onto the stack. Integers are - /// encoded as little-endian signed-magnitude numbers, but there are - /// dedicated opcodes to push some small integers. + /// Adds instructions to push an integer onto the stack. + /// + /// Integers are encoded as little-endian signed-magnitude numbers, but there are dedicated + /// opcodes to push some small integers. pub fn push_int(self, data: i64) -> Builder { // We can special-case -1, 1-16 if data == -1 || (1..=16).contains(&data) { diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index df3e2493..3814d0f0 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -146,6 +146,7 @@ impl std::error::Error for ParseOutPointError { } /// Parses a string-encoded transaction index (vout). +/// /// Does not permit leading zeroes or non-digit characters. fn parse_vout(s: &str) -> Result { if s.len() > 1 { @@ -319,13 +320,13 @@ impl Sequence { self.is_relative_lock_time() & (self.0 & Sequence::LOCK_TYPE_MASK > 0) } - /// Create a relative lock-time using block height. + /// Creates a relative lock-time using block height. #[inline] pub fn from_height(height: u16) -> Self { Sequence(u32::from(height)) } - /// Create a relative lock-time using time intervals where each interval is equivalent + /// Creates a relative lock-time using time intervals where each interval is equivalent /// to 512 seconds. /// /// Encoding finer granularity of time for relative lock-times is not supported in Bitcoin @@ -334,7 +335,7 @@ impl Sequence { Sequence(u32::from(intervals) | Sequence::LOCK_TYPE_MASK) } - /// Create a relative lock-time from seconds, converting the seconds into 512 second + /// Creates a relative lock-time from seconds, converting the seconds into 512 second /// interval with floor division. /// /// Will return an error if the input cannot be encoded in 16 bits. @@ -347,7 +348,7 @@ impl Sequence { } } - /// Create a relative lock-time from seconds, converting the seconds into 512 second + /// Creates a relative lock-time from seconds, converting the seconds into 512 second /// interval with ceiling division. /// /// Will return an error if the input cannot be encoded in 16 bits. @@ -366,7 +367,7 @@ impl Sequence { !self.is_final() } - /// Create a sequence from a u32 value. + /// Creates a sequence from a u32 value. #[inline] pub fn from_consensus(n: u32) -> Self { Sequence(n) @@ -590,6 +591,7 @@ pub struct Transaction { impl Transaction { /// Computes a "normalized TXID" which does not include any signatures. + /// /// This gives a way to identify a transaction that is "the same" as /// another in the sense of having same inputs and outputs. pub fn ntxid(&self) -> sha256d::Hash { @@ -602,9 +604,10 @@ impl Transaction { cloned_tx.txid().into() } - /// Computes the txid. For non-segwit transactions this will be identical - /// to the output of `wtxid()`, but for segwit transactions, - /// this will give the correct txid (not including witnesses) while `wtxid` + /// Computes the txid. + /// + /// For non-segwit transactions this will be identical to the output of `wtxid()`, but for + /// segwit transactions, this will give the correct txid (not including witnesses) while `wtxid` /// will also hash witnesses. pub fn txid(&self) -> Txid { let mut enc = Txid::engine(); @@ -615,9 +618,10 @@ impl Transaction { Txid::from_engine(enc) } - /// Computes SegWit-version of the transaction id (wtxid). For transaction with the witness - /// data this hash includes witness, for pre-witness transaction it is equal to the normal - /// value returned by txid() function. + /// Computes SegWit-version of the transaction id (wtxid). + /// + /// For transaction with the witness data this hash includes witness, for pre-witness + /// transaction it is equal to the normal value returned by txid() function. pub fn wtxid(&self) -> Wtxid { let mut enc = Wtxid::engine(); self.consensus_encode(&mut enc).expect("engines don't error"); @@ -642,7 +646,7 @@ impl Transaction { /// have the information to determine. /// - Does NOT handle the sighash single bug (see "Return type" section) /// - /// # Return type + /// # Returns /// /// This function can't handle the SIGHASH_SINGLE bug internally, so it returns [`EncodeSigningDataResult`] /// that must be handled by the caller (see [`EncodeSigningDataResult::is_sighash_single_bug`]). @@ -826,6 +830,7 @@ impl Transaction { } /// Verify that this transaction is able to spend its inputs. + /// /// The `spent` closure should not return the same [`TxOut`] twice! #[cfg(feature="bitcoinconsensus")] #[cfg_attr(docsrs, doc(cfg(feature = "bitcoinconsensus")))] @@ -846,14 +851,15 @@ impl Transaction { Ok(()) } - /// Is this a coin base transaction? + /// Checks if this is a coin base transaction. pub fn is_coin_base(&self) -> bool { self.input.len() == 1 && self.input[0].previous_output.is_null() } - /// Returns `true` if the transaction itself opted in to be BIP-125-replaceable (RBF). This - /// **does not** cover the case where a transaction becomes replaceable due to ancestors being - /// RBF. + /// Returns `true` if the transaction itself opted in to be BIP-125-replaceable (RBF). + /// + /// This **does not** cover the case where a transaction becomes replaceable due to ancestors + /// being RBF. pub fn is_explicitly_rbf(&self) -> bool { self.input.iter().any(|input| input.sequence.is_rbf()) } diff --git a/bitcoin/src/blockdata/witness.rs b/bitcoin/src/blockdata/witness.rs index 2715b8fe..63670260 100644 --- a/bitcoin/src/blockdata/witness.rs +++ b/bitcoin/src/blockdata/witness.rs @@ -18,7 +18,7 @@ use crate::prelude::*; use crate::VarInt; use crate::taproot::TAPROOT_ANNEX_PREFIX; -/// The Witness is the data used to unlock bitcoins since the [segwit upgrade](https://github.com/bitcoin/bips/blob/master/bip-0143.mediawiki) +/// The Witness is the data used to unlock bitcoin since the [segwit upgrade]. /// /// Can be logically seen as an array of bytestrings, i.e. `Vec>`, and it is serialized on the wire /// in that format. You can convert between this type and `Vec>` by using [`Witness::from_slice`] @@ -27,23 +27,25 @@ use crate::taproot::TAPROOT_ANNEX_PREFIX; /// For serialization and deserialization performance it is stored internally as a single `Vec`, /// saving some allocations. /// +/// [segwit upgrade]: #[derive(Clone, Default, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)] pub struct Witness { /// Contains the witness `Vec>` serialization without the initial varint indicating the - /// number of elements (which is stored in `witness_elements`) + /// number of elements (which is stored in `witness_elements`). content: Vec, - /// Number of elements in the witness. - /// It is stored separately (instead of as VarInt in the initial part of content) so that method - /// like [`Witness::push`] doesn't have case requiring to shift the entire array + /// The number of elements in the witness. + /// + /// Stored separately (instead of as a VarInt in the initial part of content) so that methods + /// like [`Witness::push`] don't have to shift the entire array. witness_elements: usize, - /// This is the valid index pointing to the beginning of the index area. This area is 4 * stack_size bytes - /// at the end of the content vector which stores the indices of each item. + /// This is the valid index pointing to the beginning of the index area. This area is 4 * + /// stack_size bytes at the end of the content vector which stores the indices of each item. indices_start: usize, } -/// Support structure to allow efficient and convenient iteration over the Witness elements +/// Support structure to allow efficient and convenient iteration over the Witness elements. pub struct Iter<'a> { inner: &'a [u8], indices_start: usize, @@ -162,7 +164,7 @@ impl Encodable for Witness { } impl Witness { - /// Create a new empty [`Witness`] + /// Creates a new empty [`Witness`]. pub fn new() -> Self { Witness::default() } @@ -202,17 +204,17 @@ impl Witness { } } - /// Convenience method to create an array of byte-arrays from this witness + /// Convenience method to create an array of byte-arrays from this witness. pub fn to_vec(&self) -> Vec> { self.iter().map(|s| s.to_vec()).collect() } - /// Returns `true` if the witness contains no element + /// Returns `true` if the witness contains no element. pub fn is_empty(&self) -> bool { self.witness_elements == 0 } - /// Returns a struct implementing [`Iterator`] + /// Returns a struct implementing [`Iterator`]. pub fn iter(&self) -> Iter { Iter { inner: self.content.as_slice(), @@ -221,12 +223,12 @@ impl Witness { } } - /// Returns the number of elements this witness holds + /// Returns the number of elements this witness holds. pub fn len(&self) -> usize { self.witness_elements } - /// Returns the bytes required when this Witness is consensus encoded + /// Returns the bytes required when this Witness is consensus encoded. pub fn serialized_len(&self) -> usize { self.iter() .map(|el| VarInt(el.len() as u64).len() + el.len()) @@ -234,14 +236,14 @@ impl Witness { + VarInt(self.witness_elements as u64).len() } - /// Clear the witness + /// Clear the witness. pub fn clear(&mut self) { self.content.clear(); self.witness_elements = 0; self.indices_start = 0; } - /// Push a new element on the witness, requires an allocation + /// Push a new element on the witness, requires an allocation. pub fn push>(&mut self, new_element: T) { self.push_slice(new_element.as_ref()); } @@ -284,7 +286,7 @@ impl Witness { Some(&self.content[start..start + varint.0 as usize]) } - /// Return the last element in the witness, if any + /// Returns the last element in the witness, if any. pub fn last(&self) -> Option<&[u8]> { if self.witness_elements == 0 { None @@ -293,7 +295,7 @@ impl Witness { } } - /// Return the second-to-last element in the witness, if any + /// Returns the second-to-last element in the witness, if any. pub fn second_to_last(&self) -> Option<&[u8]> { if self.witness_elements <= 1 { None From bbd39e5ecc5abbadb477a2e85e9c3fa2e6ab5fcf Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 31 Oct 2022 12:31:45 +1100 Subject: [PATCH 3/7] Use longer column width Reduce the number of lines of code by using a longer column width, 100 as is more-or-less standard in this repo. This patch only changes column width (line length), no other changes. --- bitcoin/src/consensus/encode.rs | 54 +++++++++++++++------------------ 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/bitcoin/src/consensus/encode.rs b/bitcoin/src/consensus/encode.rs index 4486e147..d9fbbf9e 100644 --- a/bitcoin/src/consensus/encode.rs +++ b/bitcoin/src/consensus/encode.rs @@ -297,43 +297,37 @@ pub trait Encodable { pub trait Decodable: Sized { /// Decode `Self` from a size-limited reader. /// - /// Like `consensus_decode` but relies on the reader being - /// limited in the amount of data it returns, e.g. by - /// being wrapped in [`std::io::Take`]. + /// Like `consensus_decode` but relies on the reader being limited in the amount of data it + /// returns, e.g. by being wrapped in [`std::io::Take`]. /// - /// Failling to obide to this requirement might lead to - /// memory exhaustion caused by malicious inputs. + /// Failling to obide to this requirement might lead to memory exhaustion caused by malicious + /// inputs. /// - /// Users should default to `consensus_decode`, but - /// when data to be decoded is already in a byte vector - /// of a limited size, calling this function directly - /// might be marginally faster (due to avoiding - /// extra checks). + /// Users should default to `consensus_decode`, but when data to be decoded is already in a byte + /// vector of a limited size, calling this function directly might be marginally faster (due to + /// avoiding extra checks). /// /// ### Rules for trait implementations /// - /// * Simple types that that have a fixed size (own and member fields), - /// don't have to overwrite this method, or be concern with it. - /// * Types that deserialize using externally provided length - /// should implement it: - /// * Make `consensus_decode` forward to `consensus_decode_bytes_from_finite_reader` - /// with the reader wrapped by `Take`. Failure to do so, without other - /// forms of memory exhaustion protection might lead to resource exhaustion - /// vulnerability. - /// * Put a max cap on things like `Vec::with_capacity` to avoid oversized - /// allocations, and rely on the reader running out of data, and collections - /// reallocating on a legitimately oversized input data, instead of trying - /// to enforce arbitrary length limits. - /// * Types that contain other types that implement custom `consensus_decode_from_finite_reader`, - /// should also implement it applying same rules, and in addition make sure to call - /// `consensus_decode_from_finite_reader` on all members, to avoid creating redundant - /// `Take` wrappers. Failure to do so might result only in a tiny performance hit. + /// * Simple types that that have a fixed size (own and member fields), don't have to overwrite + /// this method, or be concern with it. + /// * Types that deserialize using externally provided length should implement it: + /// * Make `consensus_decode` forward to `consensus_decode_bytes_from_finite_reader` with the + /// reader wrapped by `Take`. Failure to do so, without other forms of memory exhaustion + /// protection might lead to resource exhaustion vulnerability. + /// * Put a max cap on things like `Vec::with_capacity` to avoid oversized allocations, and + /// rely on the reader running out of data, and collections reallocating on a legitimately + /// oversized input data, instead of trying to enforce arbitrary length limits. + /// * Types that contain other types that implement custom + /// `consensus_decode_from_finite_reader`, should also implement it applying same rules, and + /// in addition make sure to call `consensus_decode_from_finite_reader` on all members, to + /// avoid creating redundant `Take` wrappers. Failure to do so might result only in a tiny + /// performance hit. #[inline] fn consensus_decode_from_finite_reader(reader: &mut R) -> Result { - // This method is always strictly less general than, `consensus_decode`, - // so it's safe and make sense to default to just calling it. - // This way most types, that don't care about protecting against - // resource exhaustion due to malicious input, can just ignore it. + // This method is always strictly less general than, `consensus_decode`, so it's safe and + // make sense to default to just calling it. This way most types, that don't care about + // protecting against resource exhaustion due to malicious input, can just ignore it. Self::consensus_decode(reader) } From c553299ace09816f5f80d85962109cd588078265 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 31 Oct 2022 12:34:50 +1100 Subject: [PATCH 4/7] Remove uninformative code comments The comments add no value to the code, remove them. --- bitcoin/src/consensus/encode.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/bitcoin/src/consensus/encode.rs b/bitcoin/src/consensus/encode.rs index d9fbbf9e..c3b6119b 100644 --- a/bitcoin/src/consensus/encode.rs +++ b/bitcoin/src/consensus/encode.rs @@ -458,7 +458,6 @@ impl Decodable for VarInt { } } -// Booleans impl Encodable for bool { #[inline] fn consensus_encode(&self, w: &mut W) -> Result { @@ -474,7 +473,6 @@ impl Decodable for bool { } } -// Strings impl Encodable for String { #[inline] fn consensus_encode(&self, w: &mut W) -> Result { @@ -493,7 +491,6 @@ impl Decodable for String { } } -// Cow<'static, str> impl Encodable for Cow<'static, str> { #[inline] fn consensus_encode(&self, w: &mut W) -> Result { @@ -513,8 +510,6 @@ impl Decodable for Cow<'static, str> { } } - -// Arrays macro_rules! impl_array { ( $size:literal ) => { impl Encodable for [u8; $size] { @@ -565,7 +560,6 @@ impl Encodable for [u16; 8] { } } -// Vectors macro_rules! impl_vec { ($type: ty) => { impl Encodable for Vec<$type> { @@ -689,7 +683,6 @@ fn sha2_checksum(data: &[u8]) -> [u8; 4] { [checksum[0], checksum[1], checksum[2], checksum[3]] } -// Checked data impl Encodable for CheckedData { #[inline] fn consensus_encode(&self, w: &mut W) -> Result { @@ -719,7 +712,6 @@ impl Decodable for CheckedData { } } -// References impl<'a, T: Encodable> Encodable for &'a T { fn consensus_encode(&self, w: &mut W) -> Result { (**self).consensus_encode(w) @@ -744,7 +736,6 @@ impl Encodable for sync::Arc { } } -// Tuples macro_rules! tuple_encode { ($($x:ident),*) => { impl <$($x: Encodable),*> Encodable for ($($x),*) { @@ -815,7 +806,6 @@ impl Decodable for TapLeafHash { } } -// Tests #[cfg(test)] mod tests { use super::*; From e1e59740657a1343d8d3e439754aea2dccd66b5c Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 31 Oct 2022 12:38:13 +1100 Subject: [PATCH 5/7] consensus: Improve rustdocs Do an audit of the `consensus` module and clean up rustdocs. --- bitcoin/src/consensus/encode.rs | 99 +++++++++++++++++---------------- 1 file changed, 51 insertions(+), 48 deletions(-) diff --git a/bitcoin/src/consensus/encode.rs b/bitcoin/src/consensus/encode.rs index c3b6119b..ef3d7344 100644 --- a/bitcoin/src/consensus/encode.rs +++ b/bitcoin/src/consensus/encode.rs @@ -35,33 +35,33 @@ use crate::blockdata::transaction::{TxOut, Transaction, TxIn}; #[cfg(feature = "std")] use crate::network::{message_blockdata::Inventory, address::{Address, AddrV2Message}}; -/// Encoding error +/// Encoding error. #[derive(Debug)] #[non_exhaustive] pub enum Error { - /// And I/O error + /// And I/O error. Io(io::Error), - /// PSBT-related error + /// PSBT-related error. Psbt(psbt::Error), - /// Tried to allocate an oversized vector + /// Tried to allocate an oversized vector. OversizedVectorAllocation { - /// The capacity requested + /// The capacity requested. requested: usize, - /// The maximum capacity + /// The maximum capacity. max: usize, }, - /// Checksum was invalid + /// Checksum was invalid. InvalidChecksum { - /// The expected checksum + /// The expected checksum. expected: [u8; 4], - /// The invalid checksum + /// The invalid checksum. actual: [u8; 4], }, - /// VarInt was encoded in a non-minimal way + /// VarInt was encoded in a non-minimal way. NonMinimalVarInt, - /// Parsing error + /// Parsing error. ParseFailed(&'static str), - /// Unsupported Segwit flag + /// Unsupported Segwit flag. UnsupportedSegwitFlag(u8), } @@ -114,7 +114,7 @@ impl From for Error { } } -/// Encode an object into a vector +/// Encodes an object into a vector. pub fn serialize(data: &T) -> Vec { let mut encoder = Vec::new(); let len = data.consensus_encode(&mut encoder).expect("in-memory writers don't error"); @@ -122,12 +122,12 @@ pub fn serialize(data: &T) -> Vec { encoder } -/// Encode an object into a hex-encoded string +/// Encodes an object into a hex-encoded string. pub fn serialize_hex(data: &T) -> String { serialize(data)[..].to_hex() } -/// Deserialize an object from a vector, will error if said deserialization +/// Deserializes an object from a vector, will error if said deserialization /// doesn't consume the entire vector. pub fn deserialize(data: &[u8]) -> Result { let (rv, consumed) = deserialize_partial(data)?; @@ -140,7 +140,7 @@ pub fn deserialize(data: &[u8]) -> Result { } } -/// Deserialize an object from a vector, but will not report an error if said deserialization +/// Deserializes an object from a vector, but will not report an error if said deserialization /// doesn't consume the entire vector. pub fn deserialize_partial(data: &[u8]) -> Result<(T, usize), Error> { let mut decoder = Cursor::new(data); @@ -151,57 +151,57 @@ pub fn deserialize_partial(data: &[u8]) -> Result<(T, usize), Erro } -/// Extensions of `Write` to encode data as per Bitcoin consensus +/// Extensions of `Write` to encode data as per Bitcoin consensus. pub trait WriteExt : io::Write { - /// Output a 64-bit uint + /// Outputs a 64-bit unsigned integer. fn emit_u64(&mut self, v: u64) -> Result<(), io::Error>; - /// Output a 32-bit uint + /// Outputs a 32-bit unsigned integer. fn emit_u32(&mut self, v: u32) -> Result<(), io::Error>; - /// Output a 16-bit uint + /// Outputs a 16-bit unsigned integer. fn emit_u16(&mut self, v: u16) -> Result<(), io::Error>; - /// Output a 8-bit uint + /// Outputs a 8-bit unsigned integer. fn emit_u8(&mut self, v: u8) -> Result<(), io::Error>; - /// Output a 64-bit int + /// Outputs a 64-bit signed integer. fn emit_i64(&mut self, v: i64) -> Result<(), io::Error>; - /// Output a 32-bit int + /// Outputs a 32-bit signed integer. fn emit_i32(&mut self, v: i32) -> Result<(), io::Error>; - /// Output a 16-bit int + /// Outputs a 16-bit signed integer. fn emit_i16(&mut self, v: i16) -> Result<(), io::Error>; - /// Output a 8-bit int + /// Outputs a 8-bit signed integer. fn emit_i8(&mut self, v: i8) -> Result<(), io::Error>; - /// Output a boolean + /// Outputs a boolean. fn emit_bool(&mut self, v: bool) -> Result<(), io::Error>; - /// Output a byte slice + /// Outputs a byte slice. fn emit_slice(&mut self, v: &[u8]) -> Result<(), io::Error>; } -/// Extensions of `Read` to decode data as per Bitcoin consensus +/// Extensions of `Read` to decode data as per Bitcoin consensus. pub trait ReadExt : io::Read { - /// Read a 64-bit uint + /// Reads a 64-bit unsigned integer. fn read_u64(&mut self) -> Result; - /// Read a 32-bit uint + /// Reads a 32-bit unsigned integer. fn read_u32(&mut self) -> Result; - /// Read a 16-bit uint + /// Reads a 16-bit unsigned integer. fn read_u16(&mut self) -> Result; - /// Read a 8-bit uint + /// Reads a 8-bit unsigned integer. fn read_u8(&mut self) -> Result; - /// Read a 64-bit int + /// Reads a 64-bit signed integer. fn read_i64(&mut self) -> Result; - /// Read a 32-bit int + /// Reads a 32-bit signed integer. fn read_i32(&mut self) -> Result; - /// Read a 16-bit int + /// Reads a 16-bit signed integer. fn read_i16(&mut self) -> Result; - /// Read a 8-bit int + /// Reads a 8-bit signed integer. fn read_i8(&mut self) -> Result; - /// Read a boolean + /// Reads a boolean. fn read_bool(&mut self) -> Result; - /// Read a byte slice + /// Reads a byte slice. fn read_slice(&mut self, slice: &mut [u8]) -> Result<(), Error>; } @@ -281,19 +281,21 @@ impl ReadExt for R { } } -/// Maximum size, in bytes, of a vector we are allowed to decode +/// Maximum size, in bytes, of a vector we are allowed to decode. pub const MAX_VEC_SIZE: usize = 4_000_000; -/// Data which can be encoded in a consensus-consistent way +/// Data which can be encoded in a consensus-consistent way. pub trait Encodable { - /// Encode an object with a well-defined format. - /// Returns the number of bytes written on success. + /// Encodes an object with a well-defined format. /// - /// The only errors returned are errors propagated from the writer. + /// # Returns + /// + /// The number of bytes written on success. The only errors returned are errors propagated from + /// the writer. fn consensus_encode(&self, writer: &mut W) -> Result; } -/// Data which can be encoded in a consensus-consistent way +/// Data which can be encoded in a consensus-consistent way. pub trait Decodable: Sized { /// Decode `Self` from a size-limited reader. /// @@ -345,11 +347,11 @@ pub trait Decodable: Sized { } } -/// A variable-length unsigned integer +/// A variable-length unsigned integer. #[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Debug)] pub struct VarInt(pub u64); -/// Data which must be preceded by a 4-byte checksum +/// Data which must be preceded by a 4-byte checksum. #[derive(PartialEq, Eq, Clone, Debug)] pub struct CheckedData(pub Vec); @@ -384,6 +386,7 @@ impl_int_encodable!(i64, read_i64, emit_i64); #[allow(clippy::len_without_is_empty)] // VarInt has on concept of 'is_empty'. impl VarInt { /// Gets the length of this VarInt when encoded. + /// /// Returns 1 for 0..=0xFC, 3 for 0xFD..=(2^16-1), 5 for 0x10000..=(2^32-1), /// and 9 otherwise. #[inline] @@ -624,7 +627,7 @@ struct ReadBytesFromFiniteReaderOpts { chunk_size: usize, } -/// Read `opts.len` bytes from reader, where `opts.len` could potentially be malicious +/// Read `opts.len` bytes from reader, where `opts.len` could potentially be malicious. /// /// This function relies on reader being bound in amount of data /// it returns for OOM protection. See [`Decodable::consensus_decode_from_finite_reader`]. @@ -677,7 +680,7 @@ impl Decodable for Box<[u8]> { } -/// Do a double-SHA256 on some data and return the first 4 bytes +/// Does a double-SHA256 on `data` and returns the first 4 bytes. fn sha2_checksum(data: &[u8]) -> [u8; 4] { let checksum = ::hash(data); [checksum[0], checksum[1], checksum[2], checksum[3]] From c9a49d5be7ddc2b16d322a21a83270768d75ac97 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Sun, 6 Nov 2022 06:45:00 +1100 Subject: [PATCH 6/7] blockdata: Improve content of rustdocs Recently we (tcharding) do some mechanical improvements to the rustdocs in the `blockdata` module without considering the content. On review a bunch of improvements were suggested. Improve the content of various rustdoc comments in the `blockdata` module. Suggested content came from reviewers, all mistakes are my own :) --- bitcoin/src/blockdata/transaction.rs | 27 +++++++++++++++++---------- bitcoin/src/blockdata/witness.rs | 2 +- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index 3814d0f0..e8fed9bd 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -604,11 +604,11 @@ impl Transaction { cloned_tx.txid().into() } - /// Computes the txid. + /// Computes the [`Txid`]. /// - /// For non-segwit transactions this will be identical to the output of `wtxid()`, but for - /// segwit transactions, this will give the correct txid (not including witnesses) while `wtxid` - /// will also hash witnesses. + /// Hashes the transaction **excluding** the segwit data (i.e. the marker, flag bytes, and the + /// witness fields themselves). For non-segwit transactions which do not have any segwit data, + /// this will be equal to [`Transaction::wtxid()`]. pub fn txid(&self) -> Txid { let mut enc = Txid::engine(); self.version.consensus_encode(&mut enc).expect("engines don't error"); @@ -618,10 +618,11 @@ impl Transaction { Txid::from_engine(enc) } - /// Computes SegWit-version of the transaction id (wtxid). + /// Computes the segwit version of the transaction id. /// - /// For transaction with the witness data this hash includes witness, for pre-witness - /// transaction it is equal to the normal value returned by txid() function. + /// Hashes the transaction **including** all segwit data (i.e. the marker, flag bytes, and the + /// witness fields themselves). For non-segwit transactions which do not have any segwit data, + /// this will be equal to [`Transaction::txid()`]. pub fn wtxid(&self) -> Wtxid { let mut enc = Wtxid::engine(); self.consensus_encode(&mut enc).expect("engines don't error"); @@ -644,7 +645,7 @@ impl Transaction { /// - Does NOT attempt to support OP_CODESEPARATOR. In general this would require evaluating /// `script_pubkey` to determine which separators get evaluated and which don't, which we don't /// have the information to determine. - /// - Does NOT handle the sighash single bug (see "Return type" section) + /// - Does NOT handle the sighash single bug (see "Returns" section) /// /// # Returns /// @@ -851,7 +852,12 @@ impl Transaction { Ok(()) } - /// Checks if this is a coin base transaction. + /// Checks if this is a coinbase transaction. + /// + /// The first transaction in the block distributes the mining reward and is called the coinbase + /// transaction. It is impossible to check if the transaction is first in the block, so this + /// function checks the structure of the transaction instead - the previous output must be + /// all-zeros (creates satoshis "out of thin air"). pub fn is_coin_base(&self) -> bool { self.input.len() == 1 && self.input[0].previous_output.is_null() } @@ -859,7 +865,8 @@ impl Transaction { /// Returns `true` if the transaction itself opted in to be BIP-125-replaceable (RBF). /// /// This **does not** cover the case where a transaction becomes replaceable due to ancestors - /// being RBF. + /// being RBF. Please note that transactions may be replaced even if they do not include the RBF + /// signal: . pub fn is_explicitly_rbf(&self) -> bool { self.input.iter().any(|input| input.sequence.is_rbf()) } diff --git a/bitcoin/src/blockdata/witness.rs b/bitcoin/src/blockdata/witness.rs index 63670260..3061961e 100644 --- a/bitcoin/src/blockdata/witness.rs +++ b/bitcoin/src/blockdata/witness.rs @@ -45,7 +45,7 @@ pub struct Witness { indices_start: usize, } -/// Support structure to allow efficient and convenient iteration over the Witness elements. +/// An iterator returning individual witness elements. pub struct Iter<'a> { inner: &'a [u8], indices_start: usize, From 26be9ddd27ad13828af35221c6f8c5a60f5e5823 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 8 Nov 2022 07:43:27 +1100 Subject: [PATCH 7/7] Make RBF rustdoc more scrary In order to really bring the security risks of RBF to peoples attention make the docs more scary. --- bitcoin/src/blockdata/transaction.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index e8fed9bd..6272ff53 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -864,9 +864,13 @@ impl Transaction { /// Returns `true` if the transaction itself opted in to be BIP-125-replaceable (RBF). /// + /// # Warning + /// + /// **Incorrectly relying on RBF may lead to monetary loss!** + /// /// This **does not** cover the case where a transaction becomes replaceable due to ancestors - /// being RBF. Please note that transactions may be replaced even if they do not include the RBF - /// signal: . + /// being RBF. Please note that transactions **may be replaced** even if they **do not** include + /// the RBF signal: . pub fn is_explicitly_rbf(&self) -> bool { self.input.iter().any(|input| input.sequence.is_rbf()) }