Repository: rust-bitcoin/rust-bitcoin (language: rust)

Issue:
primitives: Remove `TxOut::NULL`
Issue currently just tracks this task.

TODO: Write this issue properly.

Discussed here: https://github.com/rust-bitcoin/rust-bitcoin/pull/3794#discussion_r1931169559


Discussion / hints:
To add a bit more color: this pseudo-object is serialized into a signature hash when creating a legacy sighash with the SIGHASH_SINGLE mode.

It is also used as a component of BIP322 transactions, which are (by construction) invalid but still supposed to parse and behave like other transactions. (BTW, this may be a reason that we can't restrict this range, or a reason that we may want to special case `0xffff...ffff` as a "poison value" in `Amount`, or something?)

When our `Amount` type had the range of `u64` it was OK but if we restrict it to `MAX_MONEY` then it will become invalid and we can't expose it as a constant anymore.
> this pseudo-object is serialized into a signature hash when creating a legacy sighash with the SIGHASH_SINGLE mode.

Right, we can simply feed hasher a constant byte slice. It'll save some perf in non-optimized builds too.

> BIP322

I don't see it there, are you confusing it with prevout index or a different BIP?

And honestly, even if it was, do we really want to allow obscure values in `Transaction` just because some BIP that has nothing to do with consensus decided to produce invalid transaction? If we really have to reuse deserialization code:

```rust
pub struct Transaction<A = Amount> {
    pub outputs: Vec<TxOut<A>,
   // ...
}

pub struct TxOut<A = Amount> {
    pub value: A,
    pub script_pubkey: ScriptBuf,
}

impl<A: ConsensusDecode> ConsensusDecode for TxOut<A> { /* ... */ }

// Has the same size as Amount if we mark invalid range.
pub enum ObscureAmount {
    Valid(Amount),
    Poison,
}

impl ConsensusDecode for ObscureAmount { /* ... */ }
```
Ah, yes, I am confusing this with the null prevout, which is fine. Ok, crisis averted.

>And honestly, even if it was, do we really want to allow obscure values in Transaction just because some BIP that has nothing to do with consensus decided to produce invalid transaction?

But yes -- this is a critical BIP and the fact that it's been rewritten so many times over the years instead of somebody just running with it has caused a ton of damage in the ecosystem, including exchanges requiring valid "test transactions" or requiring users to run their coins through legacy outputs so they can use signmessage.

Fix patch:
diff --git a/api/primitives/all-features.txt b/api/primitives/all-features.txt
index b2cfbe1e0..9564cd7ce 100644
--- a/api/primitives/all-features.txt
+++ b/api/primitives/all-features.txt
@@ -46,7 +46,6 @@ impl bitcoin_primitives::taproot::TapTweakHash
 impl bitcoin_primitives::transaction::OutPoint
 impl bitcoin_primitives::transaction::Transaction
 impl bitcoin_primitives::transaction::TxIn
-impl bitcoin_primitives::transaction::TxOut
 impl bitcoin_primitives::transaction::Txid
 impl bitcoin_primitives::transaction::Version
 impl bitcoin_primitives::transaction::Wtxid
@@ -1258,7 +1257,6 @@ pub const bitcoin_primitives::transaction::OutPoint::COINBASE_PREVOUT: Self
 pub const bitcoin_primitives::transaction::OutPoint::SIZE: usize
 pub const bitcoin_primitives::transaction::Transaction::MAX_STANDARD_WEIGHT: bitcoin_units::weight::Weight
 pub const bitcoin_primitives::transaction::TxIn::EMPTY_COINBASE: bitcoin_primitives::transaction::TxIn
-pub const bitcoin_primitives::transaction::TxOut::NULL: Self
 pub const bitcoin_primitives::transaction::Txid::COINBASE_PREVOUT: Self
 pub const bitcoin_primitives::transaction::Txid::DISPLAY_BACKWARD: bool
 pub const bitcoin_primitives::transaction::Version::ONE: Self
diff --git a/api/primitives/alloc-only.txt b/api/primitives/alloc-only.txt
index 1ccd74b29..6154e801c 100644
--- a/api/primitives/alloc-only.txt
+++ b/api/primitives/alloc-only.txt
@@ -46,7 +46,6 @@ impl bitcoin_primitives::taproot::TapTweakHash
 impl bitcoin_primitives::transaction::OutPoint
 impl bitcoin_primitives::transaction::Transaction
 impl bitcoin_primitives::transaction::TxIn
-impl bitcoin_primitives::transaction::TxOut
 impl bitcoin_primitives::transaction::Txid
 impl bitcoin_primitives::transaction::Version
 impl bitcoin_primitives::transaction::Wtxid
@@ -1184,7 +1183,6 @@ pub const bitcoin_primitives::transaction::OutPoint::COINBASE_PREVOUT: Self
 pub const bitcoin_primitives::transaction::OutPoint::SIZE: usize
 pub const bitcoin_primitives::transaction::Transaction::MAX_STANDARD_WEIGHT: bitcoin_units::weight::Weight
 pub const bitcoin_primitives::transaction::TxIn::EMPTY_COINBASE: bitcoin_primitives::transaction::TxIn
-pub const bitcoin_primitives::transaction::TxOut::NULL: Self
 pub const bitcoin_primitives::transaction::Txid::COINBASE_PREVOUT: Self
 pub const bitcoin_primitives::transaction::Txid::DISPLAY_BACKWARD: bool
 pub const bitcoin_primitives::transaction::Version::ONE: Self
diff --git a/bitcoin/src/crypto/sighash.rs b/bitcoin/src/crypto/sighash.rs
index 83a82433b..0671e3654 100644
--- a/bitcoin/src/crypto/sighash.rs
+++ b/bitcoin/src/crypto/sighash.rs
@@ -22,11 +22,11 @@ use io::Write;
 
 use crate::address::script_pubkey::ScriptExt as _;
 use crate::consensus::{encode, Encodable};
-use crate::prelude::{Borrow, BorrowMut, String, ToOwned, Vec};
+use crate::prelude::{Borrow, BorrowMut, String, ToOwned};
 use crate::taproot::{LeafVersion, TapLeafHash, TapLeafTag, TAPROOT_ANNEX_PREFIX};
 use crate::transaction::TransactionExt as _;
 use crate::witness::Witness;
-use crate::{transaction, Amount, Script, ScriptBuf, Sequence, Transaction, TxIn, TxOut};
+use crate::{transaction, Amount, Script, Sequence, Transaction, TxOut};
 
 /// Used for signature hash for invalid use of SIGHASH_SINGLE.
 #[rustfmt::skip]
@@ -959,63 +959,59 @@ impl<R: Borrow<Transaction>> SighashCache<R> {
             script_pubkey: &Script,
             sighash_type: u32,
         ) -> Result<(), io::Error> {
+            use crate::consensus::encode::WriteExt;
+
             let (sighash, anyone_can_pay) =
                 EcdsaSighashType::from_consensus(sighash_type).split_anyonecanpay_flag();
 
-            // Build tx to sign
-            let mut tx = Transaction {
-                version: self_.version,
-                lock_time: self_.lock_time,
-                input: vec![],
-                output: vec![],
-            };
+            self_.version.consensus_encode(writer)?;
             // Add all inputs necessary..
             if anyone_can_pay {
-                tx.input = vec![TxIn {
-                    previous_output: self_.input[input_index].previous_output,
-                    script_sig: script_pubkey.to_owned(),
-                    sequence: self_.input[input_index].sequence,
-                    witness: Witness::default(),
-                }];
+                writer.emit_compact_size(1u8)?;
+                self_.input[input_index].previous_output.consensus_encode(writer)?;
+                script_pubkey.consensus_encode(writer)?;
+                self_.input[input_index].sequence.consensus_encode(writer)?;
             } else {
-                tx.input = Vec::with_capacity(self_.input.len());
+                writer.emit_compact_size(self_.input.len())?;
                 for (n, input) in self_.input.iter().enumerate() {
-                    tx.input.push(TxIn {
-                        previous_output: input.previous_output,
-                        script_sig: if n == input_index {
-                            script_pubkey.to_owned()
-                        } else {
-                            ScriptBuf::new()
-                        },
-                        sequence: if n != input_index
-                            && (sighash == EcdsaSighashType::Single
-                                || sighash == EcdsaSighashType::None)
-                        {
-                            Sequence::ZERO
-                        } else {
-                            input.sequence
-                        },
-                        witness: Witness::default(),
-                    });
+                    input.previous_output.consensus_encode(writer)?;
+                    if n == input_index {
+                        script_pubkey.consensus_encode(writer)?;
+                    } else {
+                        Script::new().consensus_encode(writer)?;
+                    }
+                    if n != input_index
+                        && (sighash == EcdsaSighashType::Single
+                            || sighash == EcdsaSighashType::None)
+                    {
+                        Sequence::ZERO.consensus_encode(writer)?;
+                    } else {
+                        input.sequence.consensus_encode(writer)?;
+                    }
                 }
             }
             // ..then all outputs
-            tx.output = match sighash {
-                EcdsaSighashType::All => self_.output.clone(),
+            match sighash {
+                EcdsaSighashType::All => {
+                    self_.output.consensus_encode(writer)?;
+                },
                 EcdsaSighashType::Single => {
-                    let output_iter = self_
-                        .output
-                        .iter()
-                        .take(input_index + 1) // sign all outputs up to and including this one, but erase
-                        .enumerate() // all of them except for this one
-                        .map(|(n, out)| if n == input_index { out.clone() } else { TxOut::NULL });
-                    output_iter.collect()
-                }
-                EcdsaSighashType::None => vec![],
+                    // sign all outputs up to and including this one, but erase
+                    // all of them except for this one
+                    let count = input_index.min(self_.output.len() - 1);
+                    writer.emit_compact_size(count + 1)?;
+                    for _ in 0..count {
+                        // consensus encoding of the "NULL txout" - max amount, empty script_pubkey
+                        writer.write_all(&[0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x00])?;
+                    }
+                    self_.output[count].consensus_encode(writer)?;
+                },
+                EcdsaSighashType::None => {
+                    writer.emit_compact_size(0u8)?;
+                },
                 _ => unreachable!(),
             };
-            // hash the result
-            tx.consensus_encode(writer)?;
+            self_.lock_time.consensus_encode(writer)?;
             sighash_type.to_le_bytes().consensus_encode(writer)?;
             Ok(())
         }
@@ -1529,10 +1525,13 @@ mod tests {
     use super::*;
     use crate::consensus::deserialize;
     use crate::locktime::absolute;
-    use crate::script::ScriptBufExt as _;
+    use crate::script::{ScriptBuf, ScriptBufExt as _};
+    use crate::TxIn;
 
     extern crate serde_json;
 
+    const DUMMY_TXOUT: TxOut = TxOut { value: Amount::MIN, script_pubkey: ScriptBuf::new() };
+
     #[test]
     fn sighash_single_bug() {
         const SIGHASH_SINGLE: u32 = 3;
@@ -1542,7 +1541,7 @@ mod tests {
             version: transaction::Version::ONE,
             lock_time: absolute::LockTime::ZERO,
             input: vec![TxIn::EMPTY_COINBASE, TxIn::EMPTY_COINBASE],
-            output: vec![TxOut::NULL],
+            output: vec![DUMMY_TXOUT],
         };
         let script = ScriptBuf::new();
         let cache = SighashCache::new(&tx);
@@ -1743,13 +1742,13 @@ mod tests {
             c.taproot_signature_hash(0, &empty_prevouts, None, None, TapSighashType::All),
             Err(TaprootError::PrevoutsSize(PrevoutsSizeError))
         );
-        let two = [TxOut::NULL, TxOut::NULL];
+        let two = [DUMMY_TXOUT, DUMMY_TXOUT];
         let too_many_prevouts = Prevouts::All(&two);
         assert_eq!(
             c.taproot_signature_hash(0, &too_many_prevouts, None, None, TapSighashType::All),
             Err(TaprootError::PrevoutsSize(PrevoutsSizeError))
         );
-        let tx_out = TxOut::NULL;
+        let tx_out = DUMMY_TXOUT;
         let prevout = Prevouts::One(1, &tx_out);
         assert_eq!(
             c.taproot_signature_hash(0, &prevout, None, None, TapSighashType::All),
diff --git a/bitcoin/src/psbt/mod.rs b/bitcoin/src/psbt/mod.rs
index 184e785ae..96e36786a 100644
--- a/bitcoin/src/psbt/mod.rs
+++ b/bitcoin/src/psbt/mod.rs
@@ -2279,7 +2279,7 @@ mod tests {
             version: transaction::Version::TWO,
             lock_time: absolute::LockTime::ZERO,
             input: vec![TxIn::EMPTY_COINBASE, TxIn::EMPTY_COINBASE],
-            output: vec![TxOut::NULL],
+            output: vec![TxOut { value: Amount::from_sat(0), script_pubkey: ScriptBuf::new() }],
         };
         let mut psbt = Psbt::from_unsigned_tx(unsigned_tx).unwrap();
 
diff --git a/primitives/src/transaction.rs b/primitives/src/transaction.rs
index bb4005798..e91540e55 100644
--- a/primitives/src/transaction.rs
+++ b/primitives/src/transaction.rs
@@ -350,13 +350,6 @@ pub struct TxOut {
     pub script_pubkey: ScriptBuf,
 }
 
-#[cfg(feature = "alloc")]
-impl TxOut {
-    /// This is used as a "null txout" in consensus signing code.
-    pub const NULL: Self =
-        TxOut { value: Amount::from_sat(0xffffffffffffffff), script_pubkey: ScriptBuf::new() };
-}
-
 /// A reference to a transaction output.
 ///
 /// ### Bitcoin Core References


Test patch: