From 313406d6abd9bb3b66cd7defd4bfece8ffd7c6f0 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Wed, 29 Jan 2025 22:24:51 +0100 Subject: [PATCH 1/4] Optimize `encode_signing_data_to_inner` The `encode_signing_data_to_inner` function previously constructed a transaction internally, requiring a bunch of allocations, which it'd then consensus-serialize into a writer (hasher). It also used a dummy `TxOut::NULL` value which we want to get rid of. To get rid of both allocations and the NULL value we serialize the transaction on-the-fly. Because the encoding doesn't involve witnesses it's not too complicated and the consensus encoding will never change so there are no desync bugs possible. We may later change this to an abstract transaction though. --- bitcoin/src/crypto/sighash.rs | 91 +++++++++++++++++------------------ 1 file changed, 44 insertions(+), 47 deletions(-) diff --git a/bitcoin/src/crypto/sighash.rs b/bitcoin/src/crypto/sighash.rs index 83a82433b..a98415604 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> SighashCache { 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,7 +1525,8 @@ 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; From a9ffb1571c157fc1f91d549b65a75a98103a5538 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Wed, 29 Jan 2025 22:30:13 +0100 Subject: [PATCH 2/4] Stop using `TxOut::NULL` in tests We want to get rid of this constant, so we replace it in tests with 0 amount, empty script. Notably, the tests were already using it as a dummy value - the exact amount was irrelevant, so this change doesn't break anything. --- bitcoin/src/crypto/sighash.rs | 8 +++++--- bitcoin/src/psbt/mod.rs | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/bitcoin/src/crypto/sighash.rs b/bitcoin/src/crypto/sighash.rs index a98415604..0671e3654 100644 --- a/bitcoin/src/crypto/sighash.rs +++ b/bitcoin/src/crypto/sighash.rs @@ -1530,6 +1530,8 @@ mod tests { 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; @@ -1539,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); @@ -1740,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(); From dd2df2bf101f4857a4aa563cd4fc05ccc1f52388 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Wed, 29 Jan 2025 22:33:47 +0100 Subject: [PATCH 3/4] Delete `TxOut::NULL` We've stopped using `TxOut::NULL` in the code and we want to restrict `Amount` to 21M BTC, so we are now deleting this constant without deprecation. Deprecation can be backported if needed. --- primitives/src/transaction.rs | 7 ------- 1 file changed, 7 deletions(-) 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 From 53bcdefba5f60878cc7049d6e157e21e985bb72c Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 30 Jan 2025 09:06:36 +1100 Subject: [PATCH 4/4] api: Run just check-api --- api/primitives/all-features.txt | 2 -- api/primitives/alloc-only.txt | 2 -- 2 files changed, 4 deletions(-) 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