Merge rust-bitcoin#5103: Remove length prefix from the BytesEncoder

c9c41f7 Remove BytesEncoder::with_length_prefix (Tobin C. Harding)
ccd86cb Use an Encoder2 when encoding a script (Tobin C. Harding)

Pull request description:

  First remove usage of `BytesEncoder::with_length_prefix` when encoding a script. Then remove the constructor and the length prefix all together from the `BytesEncoder`.
  
  This is based on discussion in rust-bitcoin#5097, as suggested originally by jrakibi, and is a cleaner alternative to my first effort in rust-bitcoin#5098 (which tried to remove the _other_ constructor).
  
  The idea here is to have only one constructor per encoder so an encoders usage is unambiguous.


ACKs for top commit:
  jrakibi:
    ACK c9c41f7
  apoelstra:
    ACK c9c41f7; successfully ran local tests


Tree-SHA512: 3629b5359788fb0b8fa9d0608bad65d025005b09ea3b917014d97ad34889ae3f5549833177fd661d89ba6881ab6dceb8e96975484bd11ef60d77e1f2a8799854

Author: apoelstra

consensus_encoding/src/encode/encoders.rs

@@ -22,38 +22,21 @@ const SIZE: usize = compact_size::MAX_ENCODING_SIZE;
 /// An encoder for a single byte slice.
 pub struct BytesEncoder<'sl> {
     sl: Option<&'sl [u8]>,
-    compact_size: Option<ArrayVec<u8, SIZE>>,
 }
 
 impl<'sl> BytesEncoder<'sl> {
     /// Constructs a byte encoder which encodes the given byte slice, with no length prefix.
     pub fn without_length_prefix(sl: &'sl [u8]) -> Self {
-        Self { sl: Some(sl), compact_size: None }
-    }
-
-    /// Constructs a byte encoder which encodes the given byte slice, with the length prefix.
-    pub fn with_length_prefix(sl: &'sl [u8]) -> Self {
-        Self { sl: Some(sl), compact_size: Some(compact_size::encode(sl.len())) }
+        Self { sl: Some(sl) }
     }
 }
 
 impl Encoder for BytesEncoder<'_> {
-    fn current_chunk(&self) -> Option<&[u8]> {
-        if let Some(compact_size) = self.compact_size.as_ref() {
-            Some(compact_size)
-        } else {
-            self.sl
-        }
-    }
+    fn current_chunk(&self) -> Option<&[u8]> { self.sl }
 
     fn advance(&mut self) -> bool {
-        if self.compact_size.is_some() {
-            self.compact_size = None;
-            true
-        } else {
-            self.sl = None;
-            false
-        }
+        self.sl = None;
+        false
     }
 }
 
@@ -275,7 +258,7 @@ impl Encoder for CompactSizeEncoder {
 mod tests {
     use super::*;
 
-    struct TestBytes<'a>(&'a [u8], bool);
+    struct TestBytes<'a>(&'a [u8]);
 
     impl<'a> Encodable for TestBytes<'a> {
         type Encoder<'s>
@@ -284,11 +267,7 @@ mod tests {
             Self: 's;
 
         fn encoder(&self) -> Self::Encoder<'_> {
-            if self.1 {
-                BytesEncoder::with_length_prefix(self.0)
-            } else {
-                BytesEncoder::without_length_prefix(self.0)
-            }
+            BytesEncoder::without_length_prefix(self.0)
         }
     }
 
@@ -327,7 +306,7 @@ mod tests {
     fn encode_byte_slice_without_prefix() {
         // Should have one chunk with the byte data, then exhausted.
         let obj = [1u8, 2, 3];
-        let test_bytes = TestBytes(&obj, false);
+        let test_bytes = TestBytes(&obj);
         let mut encoder = test_bytes.encoder();
 
         assert_eq!(encoder.current_chunk(), Some(&[1u8, 2, 3][..]));
@@ -339,41 +318,14 @@ mod tests {
     fn encode_empty_byte_slice_without_prefix() {
         // Should have one empty chunk, then exhausted.
         let obj = [];
-        let test_bytes = TestBytes(&obj, false);
+        let test_bytes = TestBytes(&obj);
         let mut encoder = test_bytes.encoder();
 
         assert_eq!(encoder.current_chunk(), Some(&[][..]));
         assert!(!encoder.advance());
         assert_eq!(encoder.current_chunk(), None);
     }
 
-    #[test]
-    fn encode_byte_slice_with_prefix() {
-        // Should have length prefix chunk, then data chunk, then exhausted.
-        let obj = [1u8, 2, 3];
-        let test_bytes = TestBytes(&obj, true);
-        let mut encoder = test_bytes.encoder();
-
-        assert_eq!(encoder.current_chunk(), Some(&[3u8][..]));
-        assert!(encoder.advance());
-        assert_eq!(encoder.current_chunk(), Some(&[1u8, 2, 3][..]));
-        assert!(!encoder.advance());
-        assert_eq!(encoder.current_chunk(), None);
-    }
-
-    #[test]
-    fn encode_empty_byte_slice_with_prefix() {
-        // Should have length prefix chunk (0), then empty data chunk, then exhausted.
-        let obj = [];
-        let test_bytes = TestBytes(&obj, true);
-        let mut encoder = test_bytes.encoder();
-
-        assert_eq!(encoder.current_chunk(), Some(&[0u8][..]));
-        assert!(encoder.advance());
-        assert_eq!(encoder.current_chunk(), Some(&[][..]));
-        assert!(!encoder.advance());
-        assert_eq!(encoder.current_chunk(), None);
-    }
 
     #[test]
     fn encode_slice_with_elements() {
@@ -444,22 +396,6 @@ mod tests {
         assert_eq!(encoder.current_chunk(), None);
     }
 
-    #[test]
-    fn encode_two_byte_slices_mixed() {
-        // Should encode byte slice without prefix, then with prefix, then exhausted.
-        let enc1 = TestBytes(&[0xAA, 0xBB], false).encoder();
-        let enc2 = TestBytes(&[0xCC], true).encoder();
-        let mut encoder = Encoder2::new(enc1, enc2);
-
-        assert_eq!(encoder.current_chunk(), Some(&[0xAA, 0xBB][..]));
-        assert!(encoder.advance());
-        assert_eq!(encoder.current_chunk(), Some(&[1u8][..]));
-        assert!(encoder.advance());
-        assert_eq!(encoder.current_chunk(), Some(&[0xCC][..]));
-        assert!(!encoder.advance());
-        assert_eq!(encoder.current_chunk(), None);
-    }
-
     #[test]
     fn encode_three_arrays() {
         // Should encode three arrays in sequence, then exhausted.
@@ -525,13 +461,11 @@ mod tests {
 
     #[test]
     fn encode_mixed_composition_with_byte_slices() {
-        // Should encode byte slice with prefix, then array, then exhausted.
-        let enc1 = TestBytes(&[0xFF, 0xEE], true).encoder();
+        // Should encode byte slice, then array, then exhausted.
+        let enc1 = TestBytes(&[0xFF, 0xEE]).encoder();
         let enc2 = TestArray([0xDD, 0xCC]).encoder();
         let mut encoder = Encoder2::new(enc1, enc2);
 
-        assert_eq!(encoder.current_chunk(), Some(&[2u8][..]));
-        assert!(encoder.advance());
         assert_eq!(encoder.current_chunk(), Some(&[0xFF, 0xEE][..]));
         assert!(encoder.advance());
         assert_eq!(encoder.current_chunk(), Some(&[0xDD, 0xCC][..]));
@@ -627,35 +561,13 @@ mod tests {
         assert_eq!(encoder.current_chunk(), None);
     }
 
-    #[test]
-    fn encode_slice_with_mixed_byte_encoders() {
-        // Should encode slice of mixed byte encoders with different prefix settings, then exhausted.
-        let bytes1 = TestBytes(&[0x11, 0x12], false);
-        let bytes2 = TestBytes(&[0x21, 0x22, 0x23], true);
-        let bytes3 = TestBytes(&[], false);
-        let slice = &[bytes1, bytes2, bytes3];
-        let mut encoder = SliceEncoder::with_length_prefix(slice);
-
-        assert_eq!(encoder.current_chunk(), Some(&[3u8][..]));
-        assert!(encoder.advance());
-        assert_eq!(encoder.current_chunk(), Some(&[0x11, 0x12][..]));
-        assert!(encoder.advance());
-        assert_eq!(encoder.current_chunk(), Some(&[3u8][..]));
-        assert!(encoder.advance());
-        assert_eq!(encoder.current_chunk(), Some(&[0x21, 0x22, 0x23][..]));
-        assert!(encoder.advance());
-        assert_eq!(encoder.current_chunk(), Some(&[][..]));
-        assert!(!encoder.advance());
-        assert_eq!(encoder.current_chunk(), None);
-    }
-
     #[test]
     fn encode_complex_nested_structure() {
         // Should encode header, slice with elements, and footer with prefix, then exhausted.
-        let header = TestBytes(&[0xDE, 0xAD], false).encoder();
+        let header = TestBytes(&[0xDE, 0xAD]).encoder();
         let data_slice = &[TestArray([0x01, 0x02]), TestArray([0x03, 0x04])];
         let slice_enc = SliceEncoder::with_length_prefix(data_slice);
-        let footer = TestBytes(&[0xBE, 0xEF], true).encoder();
+        let footer = TestBytes(&[0xBE, 0xEF]).encoder();
         let mut encoder = Encoder3::new(header, slice_enc, footer);
 
         assert_eq!(encoder.current_chunk(), Some(&[0xDE, 0xAD][..]));
@@ -666,8 +578,6 @@ mod tests {
         assert!(encoder.advance());
         assert_eq!(encoder.current_chunk(), Some(&[0x03, 0x04][..]));
         assert!(encoder.advance());
-        assert_eq!(encoder.current_chunk(), Some(&[2u8][..]));
-        assert!(encoder.advance());
         assert_eq!(encoder.current_chunk(), Some(&[0xBE, 0xEF][..]));
         assert!(!encoder.advance());
         assert_eq!(encoder.current_chunk(), None);

consensus_encoding/tests/wrappers.rs

@@ -59,52 +59,28 @@ fn bytes_encoder_without_length_prefix() {
     assert_eq!(got, want);
 }
 
-#[test]
-fn bytes_encoder_with_length_prefix() {
-    #[derive(Debug, Default, Clone)]
-    pub struct Test(Vec<u8>);
-
-    impl Encodable for Test {
-        type Encoder<'e>
-            = TestBytesEncoder<'e>
-        where
-            Self: 'e;
-
-        fn encoder(&self) -> Self::Encoder<'_> {
-            TestBytesEncoder(BytesEncoder::with_length_prefix(self.0.as_ref()))
-        }
-    }
-
-    let t = Test(vec![0xca, 0xfe]);
-
-    let want = [0x02, 0xca, 0xfe];
-    let got = encoding::encode_to_vec(&t);
-
-    assert_eq!(got, want);
-}
-
 #[test]
 fn two_encoder() {
     #[derive(Debug, Default, Clone)]
     pub struct Test {
-        a: Vec<u8>, // Encode without prefix.
-        b: Vec<u8>, // Encode with prefix.
+        a: Vec<u8>,
+        b: Vec<u8>,
     }
 
     impl Encodable for Test {
         type Encoder<'e> = Encoder2<TestBytesEncoder<'e>, TestBytesEncoder<'e>>;
 
         fn encoder(&self) -> Self::Encoder<'_> {
             let a = TestBytesEncoder(BytesEncoder::without_length_prefix(self.a.as_ref()));
-            let b = TestBytesEncoder(BytesEncoder::with_length_prefix(self.b.as_ref()));
+            let b = TestBytesEncoder(BytesEncoder::without_length_prefix(self.b.as_ref()));
 
             Encoder2::new(a, b)
         }
     }
 
     let t = Test { a: vec![0xca, 0xfe], b: (vec![0xba, 0xbe]) };
 
-    let want = [0xca, 0xfe, 0x02, 0xba, 0xbe];
+    let want = [0xca, 0xfe, 0xba, 0xbe];
     let got = encoding::encode_to_vec(&t);
 
     assert_eq!(got, want);

primitives/src/script/borrowed.rs

@@ -7,7 +7,7 @@ use core::ops::{
 
 #[cfg(feature = "arbitrary")]
 use arbitrary::{Arbitrary, Unstructured};
-use encoding::{BytesEncoder, Encodable};
+use encoding::{BytesEncoder, CompactSizeEncoder, Encodable, Encoder2};
 
 use super::ScriptBuf;
 use crate::prelude::{Box, ToOwned, Vec};
@@ -155,7 +155,7 @@ impl<T> Script<T> {
 
 encoding::encoder_newtype! {
     /// The encoder for the [`Script<T>`] type.
-    pub struct ScriptEncoder<'e>(BytesEncoder<'e>);
+    pub struct ScriptEncoder<'e>(Encoder2<CompactSizeEncoder, BytesEncoder<'e>>);
 }
 
 impl<T> Encodable for Script<T> {
@@ -165,7 +165,12 @@ impl<T> Encodable for Script<T> {
         Self: 'a;
 
     fn encoder(&self) -> Self::Encoder<'_> {
-        ScriptEncoder(BytesEncoder::with_length_prefix(self.as_bytes()))
+        ScriptEncoder(
+            Encoder2::new(
+                CompactSizeEncoder::new(self.as_bytes().len()),
+                BytesEncoder::without_length_prefix(self.as_bytes())
+            )
+        )
     }
 }