Merge rust-bitcoin/rust-bitcoin#1035: Take Writer/Reader by &mut in consensus en/decoding

1fea098 Support unsized `R` and `W` in consensus encode/decode (Dawid Ciężarkiewicz)
a24a3b0 Forward `consensus_decode` to `consensus_decode_from_finite_reader` (Dawid Ciężarkiewicz)
9c754ca Take Writer/Reader by `&mut` in consensus en/decoding (Dawid Ciężarkiewicz)

Pull request description:

  Fix #1020 (see more relevant discussion there)

  This definitely makes the amount of generics compiler
  has to generate by avoding generating the same functions
  for `R`, `&mut R`, `&mut &mut R` and so on.

  old:

  ```
  > ls -al target/release/deps/bitcoin-07a9dabf1f3e0266
  -rwxrwxr-x 1 dpc dpc 9947832 Jun  2 22:42 target/release/deps/bitcoin-07a9dabf1f3e0266
  > strip target/release/deps/bitcoin-07a9dabf1f3e0266
  > ls -al target/release/deps/bitcoin-07a9dabf1f3e0266
  -rwxrwxr-x 1 dpc dpc 4463024 Jun  2 22:46 target/release/deps/bitcoin-07a9dabf1f3e0266
  ```

  new:

  ```

  > ls -al target/release/deps/bitcoin-07a9dabf1f3e0266
  -rwxrwxr-x 1 dpc dpc 9866800 Jun  2 22:44 target/release/deps/bitcoin-07a9dabf1f3e0266
  > strip target/release/deps/bitcoin-07a9dabf1f3e0266
  > ls -al target/release/deps/bitcoin-07a9dabf1f3e0266
  -rwxrwxr-x 1 dpc dpc 4393392 Jun  2 22:45 target/release/deps/bitcoin-07a9dabf1f3e0266
  ```

  In the unit-test binary itself, it saves ~100KB of data.

  I did not expect much performance gains, but turn out I was wrong(*):

  old:

  ```
  test blockdata::block::benches::bench_block_deserialize                 ... bench:   1,072,710 ns/iter (+/- 21,871)
  test blockdata::block::benches::bench_block_serialize                   ... bench:     191,223 ns/iter (+/- 5,833)
  test blockdata::block::benches::bench_block_serialize_logic             ... bench:      37,543 ns/iter (+/- 732)
  test blockdata::block::benches::bench_stream_reader                     ... bench:   1,872,455 ns/iter (+/- 149,519)
  test blockdata::transaction::benches::bench_transaction_deserialize     ... bench:         136 ns/iter (+/- 3)
  test blockdata::transaction::benches::bench_transaction_serialize       ... bench:          51 ns/iter (+/- 8)
  test blockdata::transaction::benches::bench_transaction_serialize_logic ... bench:           5 ns/iter (+/- 0)
  test blockdata::transaction::benches::bench_transaction_size            ... bench:           3 ns/iter (+/- 0)
  ```

  new:

  ```
  test blockdata::block::benches::bench_block_deserialize                 ... bench:   1,028,574 ns/iter (+/- 10,910)
  test blockdata::block::benches::bench_block_serialize                   ... bench:     162,143 ns/iter (+/- 3,363)
  test blockdata::block::benches::bench_block_serialize_logic             ... bench:      30,725 ns/iter (+/- 695)
  test blockdata::block::benches::bench_stream_reader                     ... bench:   1,437,071 ns/iter (+/- 53,694)
  test blockdata::transaction::benches::bench_transaction_deserialize     ... bench:          92 ns/iter (+/- 2)
  test blockdata::transaction::benches::bench_transaction_serialize       ... bench:          17 ns/iter (+/- 0)
  test blockdata::transaction::benches::bench_transaction_serialize_logic ... bench:           5 ns/iter (+/- 0)
  test blockdata::transaction::benches::bench_transaction_size            ... bench:           4 ns/iter (+/- 0)
  ```

  (*) - I'm benchmarking on a noisy laptop. Take this with a grain of salt. But I think
  at least it doesn't make anything slower.

  While doing all this manual labor that will probably generate conflicts,
  I took a liberty of changing generic type names and variable names to
  `r` and `R` (reader) and `w` and `W` for writer.

ACKs for top commit:
  RCasatta:
    ACK 1fea098 tested in downstream lib, space saving in compiled code confirmed
  apoelstra:
    ACK 1fea098

Tree-SHA512: bc11994791dc97cc468dc9d411b9abf52ad475f23adf5c43d563f323bae0da180c8f57f2f17c1bb7b9bdcf523584b0943763742b81362880206779872ad7489f

Author: apoelstra

src/blockdata/script.rs

@@ -23,7 +23,6 @@
 //! This module provides the structures and functions needed to support scripts.
 //!
 
-use crate::consensus::encode::MAX_VEC_SIZE;
 use crate::prelude::*;
 
 use crate::io;
@@ -1093,20 +1092,15 @@ impl serde::Serialize for Script {
 
 impl Encodable for Script {
     #[inline]
-    fn consensus_encode<S: io::Write>(&self, s: S) -> Result<usize, io::Error> {
-        self.0.consensus_encode(s)
+    fn consensus_encode<W: io::Write + ?Sized>(&self, w: &mut W) -> Result<usize, io::Error> {
+        self.0.consensus_encode(w)
     }
 }
 
 impl Decodable for Script {
     #[inline]
-    fn consensus_decode_from_finite_reader<D: io::Read>(d: D) -> Result<Self, encode::Error> {
-        Ok(Script(Decodable::consensus_decode_from_finite_reader(d)?))
-    }
-
-    #[inline]
-    fn consensus_decode<D: io::Read>(d: D) -> Result<Self, encode::Error> {
-        Self::consensus_decode_from_finite_reader(d.take(MAX_VEC_SIZE as u64))
+    fn consensus_decode_from_finite_reader<R: io::Read + ?Sized>(r: &mut R) -> Result<Self, encode::Error> {
+        Ok(Script(Decodable::consensus_decode_from_finite_reader(r)?))
     }
 }
 

src/blockdata/transaction.rs

@@ -37,7 +37,6 @@ use crate::blockdata::constants::WITNESS_SCALE_FACTOR;
 use crate::blockdata::script::Script;
 use crate::blockdata::witness::Witness;
 use crate::consensus::{encode, Decodable, Encodable};
-use crate::consensus::encode::MAX_VEC_SIZE;
 use crate::hash_types::{Sighash, Txid, Wtxid};
 use crate::VarInt;
 use crate::util::sighash::UINT256_ONE;
@@ -731,50 +730,45 @@ impl Transaction {
 impl_consensus_encoding!(TxOut, value, script_pubkey);
 
 impl Encodable for OutPoint {
-    fn consensus_encode<S: io::Write>(&self, mut s: S) -> Result<usize, io::Error> {
-        let len = self.txid.consensus_encode(&mut s)?;
-        Ok(len + self.vout.consensus_encode(s)?)
+    fn consensus_encode<W: io::Write + ?Sized>(&self, w: &mut W) -> Result<usize, io::Error> {
+        let len = self.txid.consensus_encode(w)?;
+        Ok(len + self.vout.consensus_encode(w)?)
     }
 }
 impl Decodable for OutPoint {
-    fn consensus_decode<D: io::Read>(mut d: D) -> Result<Self, encode::Error> {
+    fn consensus_decode<R: io::Read + ?Sized>(r: &mut R) -> Result<Self, encode::Error> {
         Ok(OutPoint {
-            txid: Decodable::consensus_decode(&mut d)?,
-            vout: Decodable::consensus_decode(d)?,
+            txid: Decodable::consensus_decode(r)?,
+            vout: Decodable::consensus_decode(r)?,
         })
     }
 }
 
 impl Encodable for TxIn {
-    fn consensus_encode<S: io::Write>(&self, mut s: S) -> Result<usize, io::Error> {
+    fn consensus_encode<W: io::Write + ?Sized>(&self, w: &mut W) -> Result<usize, io::Error> {
         let mut len = 0;
-        len += self.previous_output.consensus_encode(&mut s)?;
-        len += self.script_sig.consensus_encode(&mut s)?;
-        len += self.sequence.consensus_encode(s)?;
+        len += self.previous_output.consensus_encode(w)?;
+        len += self.script_sig.consensus_encode(w)?;
+        len += self.sequence.consensus_encode(w)?;
         Ok(len)
     }
 }
 impl Decodable for TxIn {
     #[inline]
-    fn consensus_decode_from_finite_reader<D: io::Read>(mut d: D) -> Result<Self, encode::Error> {
+    fn consensus_decode_from_finite_reader<R: io::Read + ?Sized>(r: &mut R) -> Result<Self, encode::Error> {
         Ok(TxIn {
-            previous_output: Decodable::consensus_decode_from_finite_reader(&mut d)?,
-            script_sig: Decodable::consensus_decode_from_finite_reader(&mut d)?,
-            sequence: Decodable::consensus_decode_from_finite_reader(d)?,
+            previous_output: Decodable::consensus_decode_from_finite_reader(r)?,
+            script_sig: Decodable::consensus_decode_from_finite_reader(r)?,
+            sequence: Decodable::consensus_decode_from_finite_reader(r)?,
             witness: Witness::default(),
         })
     }
-
-    #[inline]
-    fn consensus_decode<D: io::Read>(d: D) -> Result<Self, encode::Error> {
-        Self::consensus_decode_from_finite_reader(d.take(MAX_VEC_SIZE as u64))
-    }
 }
 
 impl Encodable for Transaction {
-    fn consensus_encode<S: io::Write>(&self, mut s: S) -> Result<usize, io::Error> {
+    fn consensus_encode<W: io::Write + ?Sized>(&self, w: &mut W) -> Result<usize, io::Error> {
         let mut len = 0;
-        len += self.version.consensus_encode(&mut s)?;
+        len += self.version.consensus_encode(w)?;
         // To avoid serialization ambiguity, no inputs means we use BIP141 serialization (see
         // `Transaction` docs for full explanation).
         let mut have_witness = self.input.is_empty();
@@ -785,36 +779,36 @@ impl Encodable for Transaction {
             }
         }
         if !have_witness {
-            len += self.input.consensus_encode(&mut s)?;
-            len += self.output.consensus_encode(&mut s)?;
+            len += self.input.consensus_encode(w)?;
+            len += self.output.consensus_encode(w)?;
         } else {
-            len += 0u8.consensus_encode(&mut s)?;
-            len += 1u8.consensus_encode(&mut s)?;
-            len += self.input.consensus_encode(&mut s)?;
-            len += self.output.consensus_encode(&mut s)?;
+            len += 0u8.consensus_encode(w)?;
+            len += 1u8.consensus_encode(w)?;
+            len += self.input.consensus_encode(w)?;
+            len += self.output.consensus_encode(w)?;
             for input in &self.input {
-                len += input.witness.consensus_encode(&mut s)?;
+                len += input.witness.consensus_encode(w)?;
             }
         }
-        len += self.lock_time.consensus_encode(s)?;
+        len += self.lock_time.consensus_encode(w)?;
         Ok(len)
     }
 }
 
 impl Decodable for Transaction {
-    fn consensus_decode_from_finite_reader<D: io::Read>(mut d: D) -> Result<Self, encode::Error> {
-        let version = i32::consensus_decode_from_finite_reader(&mut d)?;
-        let input = Vec::<TxIn>::consensus_decode_from_finite_reader(&mut d)?;
+    fn consensus_decode_from_finite_reader<R: io::Read + ?Sized>(r: &mut R) -> Result<Self, encode::Error> {
+        let version = i32::consensus_decode_from_finite_reader(r)?;
+        let input = Vec::<TxIn>::consensus_decode_from_finite_reader(r)?;
         // segwit
         if input.is_empty() {
-            let segwit_flag = u8::consensus_decode_from_finite_reader(&mut d)?;
+            let segwit_flag = u8::consensus_decode_from_finite_reader(r)?;
             match segwit_flag {
                 // BIP144 input witnesses
                 1 => {
-                    let mut input = Vec::<TxIn>::consensus_decode_from_finite_reader(&mut d)?;
-                    let output = Vec::<TxOut>::consensus_decode_from_finite_reader(&mut d)?;
+                    let mut input = Vec::<TxIn>::consensus_decode_from_finite_reader(r)?;
+                    let output = Vec::<TxOut>::consensus_decode_from_finite_reader(r)?;
                     for txin in input.iter_mut() {
-                        txin.witness = Decodable::consensus_decode_from_finite_reader(&mut d)?;
+                        txin.witness = Decodable::consensus_decode_from_finite_reader(r)?;
                     }
                     if !input.is_empty() && input.iter().all(|input| input.witness.is_empty()) {
                         Err(encode::Error::ParseFailed("witness flag set but no witnesses present"))
@@ -823,7 +817,7 @@ impl Decodable for Transaction {
                             version,
                             input,
                             output,
-                            lock_time: Decodable::consensus_decode_from_finite_reader(d)?,
+                            lock_time: Decodable::consensus_decode_from_finite_reader(r)?,
                         })
                     }
                 }
@@ -835,15 +829,11 @@ impl Decodable for Transaction {
             Ok(Transaction {
                 version,
                 input,
-                output: Decodable::consensus_decode_from_finite_reader(&mut d)?,
-                lock_time: Decodable::consensus_decode_from_finite_reader(d)?,
+                output: Decodable::consensus_decode_from_finite_reader(r)?,
+                lock_time: Decodable::consensus_decode_from_finite_reader(r)?,
             })
         }
     }
-
-    fn consensus_decode<D: io::Read>(d: D) -> Result<Self, encode::Error> {
-        Self::consensus_decode_from_finite_reader(d.take(MAX_VEC_SIZE as u64))
-    }
 }
 
 /// This type is consensus valid but an input including it would prevent the transaction from
@@ -1041,6 +1031,19 @@ mod tests {
     use super::EcdsaSighashType;
     use crate::util::sighash::SighashCache;
 
+    const SOME_TX: &str = "0100000001a15d57094aa7a21a28cb20b59aab8fc7d1149a3bdbcddba9c622e4f5f6a99ece010000006c493046022100f93bb0e7d8db7bd46e40132d1f8242026e045f03a0efe71bbb8e3f475e970d790221009337cd7f1f929f00cc6ff01f03729b069a7c21b59b1736ddfee5db5946c5da8c0121033b9b137ee87d5a812d6f506efdd37f0affa7ffc310711c06c7f3e097c9447c52ffffffff0100e1f505000000001976a9140389035a9225b3839e2bbf32d826a1e222031fd888ac00000000";
+
+    #[test]
+    fn encode_to_unsized_writer() {
+        let mut buf = [0u8; 1024];
+        let raw_tx = Vec::from_hex(SOME_TX).unwrap();
+        let tx: Transaction = Decodable::consensus_decode(&mut raw_tx.as_slice()).unwrap();
+
+        let size = tx.consensus_encode(&mut &mut buf[..]).unwrap();
+        assert_eq!(size, SOME_TX.len() / 2);
+        assert_eq!(raw_tx, &buf[..size]);
+    }
+
     #[test]
     fn test_outpoint() {
         assert_eq!(OutPoint::from_str("i don't care"),

src/blockdata/witness.rs

@@ -46,8 +46,8 @@ pub struct Iter<'a> {
 }
 
 impl Decodable for Witness {
-    fn consensus_decode<D: Read>(mut d: D) -> Result<Self, Error> {
-        let witness_elements = VarInt::consensus_decode(&mut d)?.0 as usize;
+    fn consensus_decode<R: Read + ?Sized>(r: &mut R) -> Result<Self, Error> {
+        let witness_elements = VarInt::consensus_decode(r)?.0 as usize;
         if witness_elements == 0 {
             Ok(Witness::default())
         } else {
@@ -62,7 +62,7 @@ impl Decodable for Witness {
             for _ in 0..witness_elements {
                 second_to_last = last;
                 last = cursor;
-                let element_size_varint = VarInt::consensus_decode(&mut d)?;
+                let element_size_varint = VarInt::consensus_decode(r)?;
                 let element_size_varint_len = element_size_varint.len();
                 let element_size = element_size_varint.0 as usize;
                 let required_len = cursor
@@ -86,9 +86,9 @@ impl Decodable for Witness {
 
                 resize_if_needed(&mut content, required_len);
                 element_size_varint
-                    .consensus_encode(&mut content[cursor..cursor + element_size_varint_len])?;
+                    .consensus_encode(&mut &mut content[cursor..cursor + element_size_varint_len])?;
                 cursor += element_size_varint_len;
-                d.read_exact(&mut content[cursor..cursor + element_size])?;
+                r.read_exact(&mut content[cursor..cursor + element_size])?;
                 cursor += element_size;
             }
             content.truncate(cursor);
@@ -113,10 +113,10 @@ fn resize_if_needed(vec: &mut Vec<u8>, required_len: usize) {
 }
 
 impl Encodable for Witness {
-    fn consensus_encode<W: Write>(&self, mut writer: W) -> Result<usize, io::Error> {
+    fn consensus_encode<W: Write + ?Sized>(&self, w: &mut W) -> Result<usize, io::Error> {
         let len = VarInt(self.witness_elements as u64);
-        len.consensus_encode(&mut writer)?;
-        writer.emit_slice(&self.content[..])?;
+        len.consensus_encode(w)?;
+        w.emit_slice(&self.content[..])?;
         Ok(self.content.len() + len.len())
     }
 }
@@ -145,7 +145,7 @@ impl Witness {
             last = cursor;
             let el_len_varint = VarInt(el.len() as u64);
             el_len_varint
-                .consensus_encode(&mut content[cursor..cursor + el_len_varint.len()])
+                .consensus_encode(&mut &mut content[cursor..cursor + el_len_varint.len()])
                 .expect("writers on vec don't errors, space granted by content_size");
             cursor += el_len_varint.len();
             content[cursor..cursor + el.len()].copy_from_slice(&el);
@@ -208,7 +208,7 @@ impl Witness {
             .resize(current_content_len + element_len_varint.len() + new_element.len(), 0);
         let end_varint = current_content_len + element_len_varint.len();
         element_len_varint
-            .consensus_encode(&mut self.content[current_content_len..end_varint])
+            .consensus_encode(&mut &mut self.content[current_content_len..end_varint])
             .expect("writers on vec don't error, space granted through previous resize");
         self.content[end_varint..].copy_from_slice(new_element);
     }
@@ -225,7 +225,7 @@ impl Witness {
 
 
     fn element_at(&self, index: usize) -> Option<&[u8]> {
-        let varint = VarInt::consensus_decode(&self.content[index..]).ok()?;
+        let varint = VarInt::consensus_decode(&mut &self.content[index..]).ok()?;
         let start = index + varint.len();
         Some(&self.content[start..start + varint.0 as usize])
     }
@@ -253,7 +253,7 @@ impl<'a> Iterator for Iter<'a> {
     type Item = &'a [u8];
 
     fn next(&mut self) -> Option<Self::Item> {
-        let varint = VarInt::consensus_decode(self.inner.as_slice()).ok()?;
+        let varint = VarInt::consensus_decode(&mut self.inner.as_slice()).ok()?;
         self.inner.nth(varint.len() - 1)?; // VarInt::len returns at least 1
         let len = varint.0 as usize;
         let slice = &self.inner.as_slice()[..len];