From a7f3458c273b7285e88a64990c5172f9761b20e8 Mon Sep 17 00:00:00 2001
From: Martin Habovstiak <martin.habovstiak@gmail.com>
Date: Thu, 22 Dec 2022 23:35:03 +0100
Subject: [PATCH 1/2] Fix bug in `ScriptBuf::extend` for short iterators

`ScriptBuf::extend` contained an optimization for short scripts that was
supposed to preallocate the buffer and then fill it. By mistake it
attempted to fill it from already-exhausted iterator instead of the
temporary array it drained the items into. This obviously produced
garbage (empty) values.

This was not caught by tests because the optimization is only active for
scripts with known maximum length and the test used `Instructions` which
doesn't know the maximum lenght.
---
 bitcoin/src/blockdata/script.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/bitcoin/src/blockdata/script.rs b/bitcoin/src/blockdata/script.rs
index dc3a0f91..68efcb18 100644
--- a/bitcoin/src/blockdata/script.rs
+++ b/bitcoin/src/blockdata/script.rs
@@ -1433,10 +1433,11 @@ impl<'a> core::iter::FromIterator<Instruction<'a>> for ScriptBuf {
 
 impl<'a> Extend<Instruction<'a>> for ScriptBuf {
     fn extend<T>(&mut self, iter: T) where T: IntoIterator<Item = Instruction<'a>> {
-        let mut iter = iter.into_iter();
+        let iter = iter.into_iter();
         // Most of Bitcoin scripts have only a few opcodes, so we can avoid reallocations in many
         // cases.
         if iter.size_hint().1.map(|max| max < 6).unwrap_or(false) {
+            let mut iter = iter.fuse();
             // `MaybeUninit` might be faster but we don't want to introduce more `unsafe` than
             // required.
             let mut head = [None; 5];
@@ -1445,8 +1446,10 @@ impl<'a> Extend<Instruction<'a>> for ScriptBuf {
                 total_size += instr.script_serialized_len();
                 *head = Some(instr);
             }
+            // Incorrect impl of `size_hint` breaks `Iterator` contract so we're free to panic.
+            assert!(iter.next().is_none(), "Buggy implementation of `Iterator` on {} returns invalid upper bound", core::any::type_name::<T::IntoIter>());
             self.reserve(total_size);
-            for instr in iter {
+            for instr in head.iter().cloned().flatten() {
                 self.push_instruction_no_opt(instr);
             }
         } else {

From 920599da943ba1f7e31bef851062252fe3859d8f Mon Sep 17 00:00:00 2001
From: Martin Habovstiak <martin.habovstiak@gmail.com>
Date: Thu, 22 Dec 2022 23:49:37 +0100
Subject: [PATCH 2/2] Add test for previous commit

If this test is added before the previous commit it will fail. It passes
now demonstrating the bug got fixed.
---
 bitcoin/src/blockdata/script.rs | 44 +++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/bitcoin/src/blockdata/script.rs b/bitcoin/src/blockdata/script.rs
index 68efcb18..b8dedb53 100644
--- a/bitcoin/src/blockdata/script.rs
+++ b/bitcoin/src/blockdata/script.rs
@@ -2594,6 +2594,50 @@ mod test {
         assert!(instructions.next().is_none());
     }
 
+    #[test]
+    fn script_extend() {
+        fn cmp_scripts(new_script: &Script, orig_script: &[Instruction<'_>]) {
+            let mut new_instr = new_script.instructions();
+            let mut orig_instr = orig_script.iter().cloned();
+            for (new, orig) in new_instr.by_ref().zip(orig_instr.by_ref()) {
+                assert_eq!(new.unwrap(), orig);
+            }
+            assert!(new_instr.next().is_none() && orig_instr.next().is_none())
+        }
+
+        let script_5_items = [
+            Instruction::Op(OP_DUP),
+            Instruction::Op(OP_HASH160),
+            Instruction::PushBytes(&[42; 20]),
+            Instruction::Op(OP_EQUALVERIFY),
+            Instruction::Op(OP_CHECKSIG),
+        ];
+        let new_script = script_5_items.iter().cloned().collect::<ScriptBuf>();
+        cmp_scripts(&new_script, &script_5_items);
+
+        let script_6_items = [
+            Instruction::Op(OP_DUP),
+            Instruction::Op(OP_HASH160),
+            Instruction::PushBytes(&[42; 20]),
+            Instruction::Op(OP_EQUALVERIFY),
+            Instruction::Op(OP_CHECKSIG),
+            Instruction::Op(OP_NOP),
+        ];
+        let new_script = script_6_items.iter().cloned().collect::<ScriptBuf>();
+        cmp_scripts(&new_script, &script_6_items);
+
+        let script_7_items = [
+            Instruction::Op(OP_DUP),
+            Instruction::Op(OP_HASH160),
+            Instruction::PushBytes(&[42; 20]),
+            Instruction::Op(OP_EQUALVERIFY),
+            Instruction::Op(OP_CHECKSIG),
+            Instruction::Op(OP_NOP),
+        ];
+        let new_script = script_7_items.iter().cloned().collect::<ScriptBuf>();
+        cmp_scripts(&new_script, &script_7_items);
+    }
+
     #[test]
     fn read_scriptbool_zero_is_false() {
         let v: Vec<u8> = vec![0x00, 0x00, 0x00, 0x00];