From 271b45299f38fda5285c33ed101f91b367869abf Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 15 Jan 2024 09:47:35 +1100 Subject: [PATCH] Improve Signature field names Applies to both `ecdsa::Signature` and `taproot::Signature`. Re-name the `Signature` fields with more descriptive names. The names used were decided upon in the issue discussion. Impove rustdocs while we are at it. Note, the change to `sign-tx-segwit-v0` is refactor only, the diff does not show it but we have a local variable already called `sighash_type` that is equal to `EcdsaSighashType::All`. Includes a function argument rename as well, just to be uniform. Fix: #2139 --- bitcoin/examples/sighash.rs | 15 +++++++---- bitcoin/examples/sign-tx-segwit-v0.rs | 4 +-- bitcoin/examples/sign-tx-taproot.rs | 4 +-- bitcoin/examples/taproot-psbt.rs | 24 ++++++++--------- bitcoin/src/blockdata/witness.rs | 4 +-- bitcoin/src/crypto/ecdsa.rs | 39 +++++++++++++++------------ bitcoin/src/crypto/key.rs | 4 +-- bitcoin/src/crypto/sighash.rs | 4 +-- bitcoin/src/crypto/taproot.rs | 32 +++++++++++----------- bitcoin/src/psbt/mod.rs | 6 +++-- bitcoin/tests/serde.rs | 8 +++--- 11 files changed, 78 insertions(+), 66 deletions(-) diff --git a/bitcoin/examples/sighash.rs b/bitcoin/examples/sighash.rs index bdddaa7e..04b0fb3f 100644 --- a/bitcoin/examples/sighash.rs +++ b/bitcoin/examples/sighash.rs @@ -44,7 +44,7 @@ fn compute_sighash_p2wpkh(raw_tx: &[u8], inp_idx: usize, value: u64) { let mut cache = sighash::SighashCache::new(&tx); let sighash = cache - .p2wpkh_signature_hash(inp_idx, &spk, Amount::from_sat(value), sig.hash_ty) + .p2wpkh_signature_hash(inp_idx, &spk, Amount::from_sat(value), sig.sighash_type) .expect("failed to compute sighash"); println!("Segwit p2wpkh sighash: {:x}", sighash); let msg = secp256k1::Message::from_digest(sighash.to_byte_array()); @@ -93,9 +93,9 @@ fn compute_sighash_legacy(raw_tx: &[u8], inp_idx: usize, script_pubkey_bytes_opt let sig = ecdsa::Signature::from_slice(instr.unwrap().push_bytes().unwrap().as_bytes()) .expect("failed to parse sig"); let sighash = cache - .legacy_signature_hash(inp_idx, script_code, sig.hash_ty.to_u32()) + .legacy_signature_hash(inp_idx, script_code, sig.sighash_type.to_u32()) .expect("failed to compute sighash"); - println!("Legacy sighash: {:x} (sighash flag {})", sighash, sig.hash_ty); + println!("Legacy sighash: {:x} (sighash flag {})", sighash, sig.sighash_type); } } @@ -126,9 +126,14 @@ fn compute_sighash_p2wsh(raw_tx: &[u8], inp_idx: usize, value: u64) { assert!((70..=72).contains(&sig_len), "signature length {} out of bounds", sig_len); //here we assume that all sighash_flags are the same. Can they be different? let sighash = cache - .p2wsh_signature_hash(inp_idx, witness_script, Amount::from_sat(value), sig.hash_ty) + .p2wsh_signature_hash( + inp_idx, + witness_script, + Amount::from_sat(value), + sig.sighash_type, + ) .expect("failed to compute sighash"); - println!("Segwit p2wsh sighash: {:x} ({})", sighash, sig.hash_ty); + println!("Segwit p2wsh sighash: {:x} ({})", sighash, sig.sighash_type); } } diff --git a/bitcoin/examples/sign-tx-segwit-v0.rs b/bitcoin/examples/sign-tx-segwit-v0.rs index ef7f969c..6a8879d3 100644 --- a/bitcoin/examples/sign-tx-segwit-v0.rs +++ b/bitcoin/examples/sign-tx-segwit-v0.rs @@ -66,10 +66,10 @@ fn main() { // Sign the sighash using the secp256k1 library (exported by rust-bitcoin). let msg = Message::from(sighash); - let sig = secp.sign_ecdsa(&msg, &sk); + let signature = secp.sign_ecdsa(&msg, &sk); // Update the witness stack. - let signature = bitcoin::ecdsa::Signature { sig, hash_ty: EcdsaSighashType::All }; + let signature = bitcoin::ecdsa::Signature { signature, sighash_type }; let pk = sk.public_key(&secp); *sighasher.witness_mut(input_index).unwrap() = Witness::p2wpkh(&signature, &pk); diff --git a/bitcoin/examples/sign-tx-taproot.rs b/bitcoin/examples/sign-tx-taproot.rs index c9025df2..f8fe3f02 100644 --- a/bitcoin/examples/sign-tx-taproot.rs +++ b/bitcoin/examples/sign-tx-taproot.rs @@ -72,10 +72,10 @@ fn main() { // Sign the sighash using the secp256k1 library (exported by rust-bitcoin). let tweaked: TweakedKeypair = keypair.tap_tweak(&secp, None); let msg = Message::from_digest(sighash.to_byte_array()); - let sig = secp.sign_schnorr(&msg, &tweaked.to_inner()); + let signature = secp.sign_schnorr(&msg, &tweaked.to_inner()); // Update the witness stack. - let signature = bitcoin::taproot::Signature { sig, hash_ty: sighash_type }; + let signature = bitcoin::taproot::Signature { signature, sighash_type }; sighasher.witness_mut(input_index).unwrap().push(&signature.to_vec()); // Get the signed transaction. diff --git a/bitcoin/examples/taproot-psbt.rs b/bitcoin/examples/taproot-psbt.rs index 1b4d4a81..66f6b796 100644 --- a/bitcoin/examples/taproot-psbt.rs +++ b/bitcoin/examples/taproot-psbt.rs @@ -282,14 +282,14 @@ fn generate_bip86_key_spend_tx( let unsigned_tx = psbt.unsigned_tx.clone(); psbt.inputs.iter_mut().enumerate().try_for_each::<_, Result<(), Box>>( |(vout, input)| { - let hash_ty = input + let sighash_type = input .sighash_type .and_then(|psbt_sighash_type| psbt_sighash_type.taproot_hash_ty().ok()) .unwrap_or(TapSighashType::All); let hash = SighashCache::new(&unsigned_tx).taproot_key_spend_signature_hash( vout, &sighash::Prevouts::All(input_txouts.as_slice()), - hash_ty, + sighash_type, )?; let (_, (_, derivation_path)) = input @@ -304,7 +304,7 @@ fn generate_bip86_key_spend_tx( None, input, hash, - hash_ty, + sighash_type, secp, ); @@ -512,7 +512,7 @@ impl BenefactorWallet { psbt.outputs = vec![Output::default()]; psbt.unsigned_tx.lock_time = absolute::LockTime::ZERO; - let hash_ty = input + let sighash_type = input .sighash_type .and_then(|psbt_sighash_type| psbt_sighash_type.taproot_hash_ty().ok()) .unwrap_or(TapSighashType::All); @@ -522,7 +522,7 @@ impl BenefactorWallet { value: input_value, script_pubkey: prevout_script_pubkey, }]), - hash_ty, + sighash_type, )?; { @@ -538,7 +538,7 @@ impl BenefactorWallet { None, input, hash, - hash_ty, + sighash_type, &self.secp, ); } @@ -654,7 +654,7 @@ impl BeneficiaryWallet { let secret_key = self.master_xpriv.derive_priv(&self.secp, &derivation_path)?.to_priv().inner; for lh in leaf_hashes { - let hash_ty = TapSighashType::All; + let sighash_type = TapSighashType::All; let hash = SighashCache::new(&unsigned_tx).taproot_script_spend_signature_hash( 0, &sighash::Prevouts::All(&[TxOut { @@ -662,7 +662,7 @@ impl BeneficiaryWallet { script_pubkey: input_script_pubkey.clone(), }]), *lh, - hash_ty, + sighash_type, )?; sign_psbt_taproot( &secret_key, @@ -670,7 +670,7 @@ impl BeneficiaryWallet { Some(*lh), &mut psbt.inputs[0], hash, - hash_ty, + sighash_type, &self.secp, ); } @@ -730,7 +730,7 @@ fn sign_psbt_taproot( leaf_hash: Option, psbt_input: &mut psbt::Input, hash: TapSighash, - hash_ty: TapSighashType, + sighash_type: TapSighashType, secp: &Secp256k1, ) { let keypair = secp256k1::Keypair::from_seckey_slice(secp, secret_key.as_ref()).unwrap(); @@ -740,9 +740,9 @@ fn sign_psbt_taproot( }; let msg = secp256k1::Message::from_digest(hash.to_byte_array()); - let sig = secp.sign_schnorr(&msg, &keypair); + let signature = secp.sign_schnorr(&msg, &keypair); - let final_signature = taproot::Signature { sig, hash_ty }; + let final_signature = taproot::Signature { signature, sighash_type }; if let Some(lh) = leaf_hash { psbt_input.tap_script_sigs.insert((pubkey, lh), final_signature); diff --git a/bitcoin/src/blockdata/witness.rs b/bitcoin/src/blockdata/witness.rs index b7011638..daf744a7 100644 --- a/bitcoin/src/blockdata/witness.rs +++ b/bitcoin/src/blockdata/witness.rs @@ -637,9 +637,9 @@ mod test { // The very first signature in block 734,958 let sig_bytes = hex!("304402207c800d698f4b0298c5aac830b822f011bb02df41eb114ade9a6702f364d5e39c0220366900d2a60cab903e77ef7dd415d46509b1f78ac78906e3296f495aa1b1b541"); - let sig = secp256k1::ecdsa::Signature::from_der(&sig_bytes).unwrap(); + let signature = secp256k1::ecdsa::Signature::from_der(&sig_bytes).unwrap(); let mut witness = Witness::default(); - let signature = crate::ecdsa::Signature { sig, hash_ty: EcdsaSighashType::All }; + let signature = crate::ecdsa::Signature { signature, sighash_type: EcdsaSighashType::All }; witness.push_ecdsa_signature(&signature); let expected_witness = vec![hex!( "304402207c800d698f4b0298c5aac830b822f011bb02df41eb114ade9a6702f364d5e39c0220366900d2a60cab903e77ef7dd415d46509b1f78ac78906e3296f495aa1b1b54101") diff --git a/bitcoin/src/crypto/ecdsa.rs b/bitcoin/src/crypto/ecdsa.rs index 14251fac..2ce06002 100644 --- a/bitcoin/src/crypto/ecdsa.rs +++ b/bitcoin/src/crypto/ecdsa.rs @@ -22,24 +22,24 @@ const MAX_SIG_LEN: usize = 73; #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))] pub struct Signature { - /// The underlying ECDSA Signature - pub sig: secp256k1::ecdsa::Signature, - /// The corresponding hash type - pub hash_ty: EcdsaSighashType, + /// The underlying ECDSA Signature. + pub signature: secp256k1::ecdsa::Signature, + /// The corresponding hash type. + pub sighash_type: EcdsaSighashType, } impl Signature { /// Constructs an ECDSA Bitcoin signature for [`EcdsaSighashType::All`]. - pub fn sighash_all(sig: secp256k1::ecdsa::Signature) -> Signature { - Signature { sig, hash_ty: EcdsaSighashType::All } + pub fn sighash_all(signature: secp256k1::ecdsa::Signature) -> Signature { + Signature { signature, sighash_type: EcdsaSighashType::All } } /// Deserializes from slice following the standardness rules for [`EcdsaSighashType`]. pub fn from_slice(sl: &[u8]) -> Result { - let (hash_ty, sig) = sl.split_last().ok_or(Error::EmptySignature)?; - let hash_ty = EcdsaSighashType::from_standard(*hash_ty as u32)?; - let sig = secp256k1::ecdsa::Signature::from_der(sig).map_err(Error::Secp256k1)?; - Ok(Signature { sig, hash_ty }) + let (sighash_type, sig) = sl.split_last().ok_or(Error::EmptySignature)?; + let sighash_type = EcdsaSighashType::from_standard(*sighash_type as u32)?; + let signature = secp256k1::ecdsa::Signature::from_der(sig).map_err(Error::Secp256k1)?; + Ok(Signature { signature, sighash_type }) } /// Serializes an ECDSA signature (inner secp256k1 signature in DER format). @@ -47,9 +47,9 @@ impl Signature { /// This does **not** perform extra heap allocation. pub fn serialize(&self) -> SerializedSignature { let mut buf = [0u8; MAX_SIG_LEN]; - let signature = self.sig.serialize_der(); + let signature = self.signature.serialize_der(); buf[..signature.len()].copy_from_slice(&signature); - buf[signature.len()] = self.hash_ty as u8; + buf[signature.len()] = self.sighash_type as u8; SerializedSignature { data: buf, len: signature.len() + 1 } } @@ -59,14 +59,19 @@ impl Signature { /// [`serialize`](Self::serialize) method instead. pub fn to_vec(self) -> Vec { // TODO: add support to serialize to a writer to SerializedSig - self.sig.serialize_der().iter().copied().chain(iter::once(self.hash_ty as u8)).collect() + self.signature + .serialize_der() + .iter() + .copied() + .chain(iter::once(self.sighash_type as u8)) + .collect() } } impl fmt::Display for Signature { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::LowerHex::fmt(&self.sig.serialize_der().as_hex(), f)?; - fmt::LowerHex::fmt(&[self.hash_ty as u8].as_hex(), f) + fmt::LowerHex::fmt(&self.signature.serialize_der().as_hex(), f)?; + fmt::LowerHex::fmt(&[self.sighash_type as u8].as_hex(), f) } } @@ -77,8 +82,8 @@ impl FromStr for Signature { let bytes = Vec::from_hex(s)?; let (sighash_byte, signature) = bytes.split_last().ok_or(Error::EmptySignature)?; Ok(Signature { - sig: secp256k1::ecdsa::Signature::from_der(signature)?, - hash_ty: EcdsaSighashType::from_standard(*sighash_byte as u32)?, + signature: secp256k1::ecdsa::Signature::from_der(signature)?, + sighash_type: EcdsaSighashType::from_standard(*sighash_byte as u32)?, }) } } diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index 9ee810cc..f5ac68d7 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -202,7 +202,7 @@ impl PublicKey { msg: &secp256k1::Message, sig: &ecdsa::Signature, ) -> Result<(), Error> { - Ok(secp.verify_ecdsa(msg, &sig.sig, &self.inner)?) + Ok(secp.verify_ecdsa(msg, &sig.signature, &self.inner)?) } } @@ -321,7 +321,7 @@ impl CompressedPublicKey { msg: &secp256k1::Message, sig: &ecdsa::Signature, ) -> Result<(), Error> { - Ok(secp.verify_ecdsa(msg, &sig.sig, &self.0)?) + Ok(secp.verify_ecdsa(msg, &sig.signature, &self.0)?) } } diff --git a/bitcoin/src/crypto/sighash.rs b/bitcoin/src/crypto/sighash.rs index 4b805281..4ed1cf53 100644 --- a/bitcoin/src/crypto/sighash.rs +++ b/bitcoin/src/crypto/sighash.rs @@ -586,10 +586,10 @@ impl TapSighashType { } /// Constructs a [`TapSighashType`] from a raw `u8`. - pub fn from_consensus_u8(hash_ty: u8) -> Result { + pub fn from_consensus_u8(sighash_type: u8) -> Result { use TapSighashType::*; - Ok(match hash_ty { + Ok(match sighash_type { 0x00 => Default, 0x01 => All, 0x02 => None, diff --git a/bitcoin/src/crypto/taproot.rs b/bitcoin/src/crypto/taproot.rs index ca771c8c..c0a1a2d7 100644 --- a/bitcoin/src/crypto/taproot.rs +++ b/bitcoin/src/crypto/taproot.rs @@ -18,10 +18,10 @@ use crate::taproot::serialized_signature::{self, SerializedSignature}; #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))] pub struct Signature { - /// The underlying schnorr signature - pub sig: secp256k1::schnorr::Signature, - /// The corresponding hash type - pub hash_ty: TapSighashType, + /// The underlying schnorr signature. + pub signature: secp256k1::schnorr::Signature, + /// The corresponding hash type. + pub sighash_type: TapSighashType, } impl Signature { @@ -30,14 +30,14 @@ impl Signature { match sl.len() { 64 => { // default type - let sig = secp256k1::schnorr::Signature::from_slice(sl)?; - Ok(Signature { sig, hash_ty: TapSighashType::Default }) + let signature = secp256k1::schnorr::Signature::from_slice(sl)?; + Ok(Signature { signature, sighash_type: TapSighashType::Default }) } 65 => { - let (hash_ty, sig) = sl.split_last().expect("Slice len checked == 65"); - let hash_ty = TapSighashType::from_consensus_u8(*hash_ty)?; - let sig = secp256k1::schnorr::Signature::from_slice(sig)?; - Ok(Signature { sig, hash_ty }) + let (sighash_type, signature) = sl.split_last().expect("Slice len checked == 65"); + let sighash_type = TapSighashType::from_consensus_u8(*sighash_type)?; + let signature = secp256k1::schnorr::Signature::from_slice(signature)?; + Ok(Signature { signature, sighash_type }) } len => Err(SigFromSliceError::InvalidSignatureSize(len)), } @@ -48,11 +48,11 @@ impl Signature { /// Note: this allocates on the heap, prefer [`serialize`](Self::serialize) if vec is not needed. pub fn to_vec(self) -> Vec { // TODO: add support to serialize to a writer to SerializedSig - let mut ser_sig = self.sig.as_ref().to_vec(); - if self.hash_ty == TapSighashType::Default { + let mut ser_sig = self.signature.as_ref().to_vec(); + if self.sighash_type == TapSighashType::Default { // default sighash type, don't add extra sighash byte } else { - ser_sig.push(self.hash_ty as u8); + ser_sig.push(self.sighash_type as u8); } ser_sig } @@ -63,13 +63,13 @@ impl Signature { /// You can get a slice from it using deref coercions or turn it into an iterator. pub fn serialize(self) -> SerializedSignature { let mut buf = [0; serialized_signature::MAX_LEN]; - let ser_sig = self.sig.serialize(); + let ser_sig = self.signature.serialize(); buf[..64].copy_from_slice(&ser_sig); - let len = if self.hash_ty == TapSighashType::Default { + let len = if self.sighash_type == TapSighashType::Default { // default sighash type, don't add extra sighash byte 64 } else { - buf[64] = self.hash_ty as u8; + buf[64] = self.sighash_type as u8; 65 }; SerializedSignature::from_raw_parts(buf, len) diff --git a/bitcoin/src/psbt/mod.rs b/bitcoin/src/psbt/mod.rs index b59d55f6..13943728 100644 --- a/bitcoin/src/psbt/mod.rs +++ b/bitcoin/src/psbt/mod.rs @@ -371,8 +371,10 @@ impl Psbt { Ok((msg, sighash_ty)) => (msg, sighash_ty), }; - let sig = - ecdsa::Signature { sig: secp.sign_ecdsa(&msg, &sk.inner), hash_ty: sighash_ty }; + let sig = ecdsa::Signature { + signature: secp.sign_ecdsa(&msg, &sk.inner), + sighash_type: sighash_ty, + }; let pk = sk.public_key(secp); diff --git a/bitcoin/tests/serde.rs b/bitcoin/tests/serde.rs index 04686a7c..6112ce6d 100644 --- a/bitcoin/tests/serde.rs +++ b/bitcoin/tests/serde.rs @@ -174,8 +174,8 @@ fn serde_regression_extended_pub_key() { fn serde_regression_ecdsa_sig() { let s = include_str!("data/serde/ecdsa_sig_hex"); let sig = ecdsa::Signature { - sig: secp256k1::ecdsa::Signature::from_str(s.trim()).unwrap(), - hash_ty: EcdsaSighashType::All, + signature: secp256k1::ecdsa::Signature::from_str(s.trim()).unwrap(), + sighash_type: EcdsaSighashType::All, }; let got = serialize(&sig).unwrap(); @@ -348,8 +348,8 @@ fn serde_regression_proprietary_key() { fn serde_regression_taproot_sig() { let s = include_str!("data/serde/taproot_sig_hex"); let sig = taproot::Signature { - sig: secp256k1::schnorr::Signature::from_str(s.trim()).unwrap(), - hash_ty: TapSighashType::All, + signature: secp256k1::schnorr::Signature::from_str(s.trim()).unwrap(), + sighash_type: TapSighashType::All, }; let got = serialize(&sig).unwrap();