Merge rust-bitcoin/rust-bitcoin#864: Cleanup block module

146d5e83d1 Improve docs for blockdata::block (Tobin Harding)
f03092c380 Fix erroneous function rustdoc (Tobin Harding)
5464848f45 Refactor check_witness_commitment (Tobin Harding)

Pull request description:

  Do some clean ups to the `blockdata::block` module.

  - Patch 1: Change predicate names (API breaking, could be seen as unnecessarily changing the API), can remove if NACK'd
  - Patch 2: Refactor to assist code clarity
  - Patch 3 and 4: are docs improvements, shouldn't be too controversial

ACKs for top commit:
  apoelstra:
    ACK 146d5e83d1
  dr-orlovsky:
    ACK 146d5e83d1

Tree-SHA512: 65cc414857c4569a389638b53eb99ed629bf67ae1d8ebdc9023e5974bb26902d4de41ec311bef3b5c895229d7d0df78d469a84c1e94fc0b7be7435338f0d510a

src/blockdata/block.rs

@@ -44,30 +44,30 @@ use VarInt;
 pub struct BlockHeader {
     /// The protocol version. Should always be 1.
     pub version: i32,
-    /// Reference to the previous block in the chain
+    /// Reference to the previous block in the chain.
     pub prev_blockhash: BlockHash,
-    /// The root hash of the merkle tree of transactions in the block
+    /// The root hash of the merkle tree of transactions in the block.
     pub merkle_root: TxMerkleNode,
-    /// The timestamp of the block, as claimed by the miner
+    /// The timestamp of the block, as claimed by the miner.
     pub time: u32,
     /// The target value below which the blockhash must lie, encoded as a
-    /// a float (with well-defined rounding, of course)
+    /// a float (with well-defined rounding, of course).
     pub bits: u32,
-    /// The nonce, selected to obtain a low enough blockhash
+    /// The nonce, selected to obtain a low enough blockhash.
     pub nonce: u32,
 }
 
 impl_consensus_encoding!(BlockHeader, version, prev_blockhash, merkle_root, time, bits, nonce);
 
 impl BlockHeader {
-    /// Return the block hash.
+    /// Returns the block hash.
     pub fn block_hash(&self) -> BlockHash {
         let mut engine = BlockHash::engine();
         self.consensus_encode(&mut engine).expect("engines don't error");
         BlockHash::from_engine(engine)
     }
 
-    /// Computes the target [0, T] that a blockhash must land in to be valid
+    /// Computes the target [0, T] that a blockhash must land in to be valid.
     pub fn target(&self) -> Uint256 {
         Self::u256_from_compact_target(self.bits)
     }
@@ -125,7 +125,7 @@ impl BlockHeader {
         compact | (size << 24) as u32
     }
 
-    /// Compute the popular "difficulty" measure for mining
+    /// Computes the popular "difficulty" measure for mining.
     pub fn difficulty(&self, network: Network) -> u64 {
         (max_target(network) / self.target()).low_u64()
     }
@@ -143,7 +143,7 @@ impl BlockHeader {
         if hash <= target { Ok(block_hash) } else { Err(BlockBadProofOfWork) }
     }
 
-    /// Returns the total work of the block
+    /// Returns the total work of the block.
     pub fn work(&self) -> Uint256 {
         // 2**256 / (target + 1) == ~target / (target+1) + 1    (eqn shamelessly stolen from bitcoind)
         let mut ret = !self.target();
@@ -169,7 +169,7 @@ pub struct Block {
 impl_consensus_encoding!(Block, header, txdata);
 
 impl Block {
-    /// Return the block hash.
+    /// Returns the block hash.
     pub fn block_hash(&self) -> BlockHash {
         self.header.block_hash()
     }
@@ -182,37 +182,41 @@ impl Block {
         }
     }
 
-    /// check if witness commitment in coinbase is matching the transaction list
+    /// Checks if witness commitment in coinbase matches the transaction list.
     pub fn check_witness_commitment(&self) -> bool {
-
+        const MAGIC: [u8; 6] = [0x6a, 0x24, 0xaa, 0x21, 0xa9, 0xed];
-        // witness commitment is optional if there are no transactions using SegWit in the block
+        // Witness commitment is optional if there are no transactions using SegWit in the block.
         if self.txdata.iter().all(|t| t.input.iter().all(|i| i.witness.is_empty())) {
             return true;
         }
-        if !self.txdata.is_empty() {
+
+        if self.txdata.is_empty() {
+            return false;
+        }
+
         let coinbase = &self.txdata[0];
-            if coinbase.is_coin_base() {
+        if !coinbase.is_coin_base() {
-                // commitment is in the last output that starts with below magic
+            return false;
+        }
+
+        // Commitment is in the last output that starts with magic bytes.
         if let Some(pos) = coinbase.output.iter()
-                    .rposition(|o| {
+            .rposition(|o| o.script_pubkey.len () >= 38 && o.script_pubkey[0..6] ==  MAGIC)
-                        o.script_pubkey.len () >= 38
+        {
-                            && o.script_pubkey[0..6] == [0x6a, 0x24, 0xaa, 0x21, 0xa9, 0xed] }) {
             let commitment = WitnessCommitment::from_slice(&coinbase.output[pos].script_pubkey.as_bytes()[6..38]).unwrap();
-                    // witness reserved value is in coinbase input witness
+            // Witness reserved value is in coinbase input witness.
             let witness_vec: Vec<_> = coinbase.input[0].witness.iter().collect();
             if witness_vec.len() == 1 && witness_vec[0].len() == 32 {
-                        match self.witness_root() {
+                if let Some(witness_root) = self.witness_root() {
-                            Some(witness_root) => return commitment == Self::compute_witness_commitment(&witness_root, witness_vec[0]),
+                    return commitment == Self::compute_witness_commitment(&witness_root, witness_vec[0]);
-                            None => return false,
-                        }
-                    }
                 }
             }
         }
+
         false
     }
 
-    /// Compute the transaction merkle root.
+    /// Computes the transaction merkle root.
     pub fn compute_merkle_root(&self) -> Option<TxMerkleNode> {
         let hashes = self.txdata.iter().map(|obj| obj.txid().as_hash());
         bitcoin_merkle_root(hashes).map(|h| h.into())
@@ -224,7 +228,7 @@ impl Block {
         self.compute_merkle_root()
     }
 
-    /// compute witness commitment for the transaction list
+    /// Computes the witness commitment for the block's transaction list.
     pub fn compute_witness_commitment (witness_root: &WitnessMerkleNode, witness_reserved_value: &[u8]) -> WitnessCommitment {
         let mut encoder = WitnessCommitment::engine();
         witness_root.consensus_encode(&mut encoder).expect("engines don't error");
@@ -232,7 +236,7 @@ impl Block {
         WitnessCommitment::from_engine(encoder)
     }
 
-    /// Merkle root of transactions hashed for witness
+    /// Computes the merkle root of transactions hashed for witness.
     pub fn witness_root(&self) -> Option<WitnessMerkleNode> {
         let hashes = self.txdata.iter().enumerate().map(|(i, t)|
             if i == 0 {
@@ -270,13 +274,12 @@ impl Block {
         base_weight + txs_weight
     }
 
-    /// Get the coinbase transaction, if one is present.
+    /// Returns the coinbase transaction, if one is present.
     pub fn coinbase(&self) -> Option<&Transaction> {
         self.txdata.first()
     }
 
-    /// Get the block height as encoded into the coinbase according to BIP34.
+    /// Returns the block height, as encoded in the coinbase transaction according to BIP34.
-    /// Returns [None] if not present.
     pub fn bip34_block_height(&self) -> Result<u64, Bip34Error> {
         // Citing the spec:
         // Add height as the first item in the coinbase transaction's scriptSig,