From 587a66da4760424ef20676c46251488cd84d350e Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Sun, 23 Mar 2025 18:44:28 +0100 Subject: [PATCH 1/4] Add a bunch of missing conversions for `Witness` `Witness` was missing conversions from arrays (and variations) which was annoying when creating known-sized witnesses. These come up when spending statically-known inputs and in tests. --- primitives/src/witness.rs | 40 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/primitives/src/witness.rs b/primitives/src/witness.rs index 101c37caa..b2242932a 100644 --- a/primitives/src/witness.rs +++ b/primitives/src/witness.rs @@ -410,6 +410,46 @@ impl From> for Witness { fn from(vec: Vec<&[u8]>) -> Self { Witness::from_slice(&vec) } } +impl From<[&[u8]; N]> for Witness { + #[inline] + fn from(arr: [&[u8]; N]) -> Self { Witness::from_slice(&arr) } +} + +impl From<&[&[u8]; N]> for Witness { + #[inline] + fn from(arr: &[&[u8]; N]) -> Self { Witness::from_slice(arr) } +} + +impl From<&[[u8; N]]> for Witness { + #[inline] + fn from(slice: &[[u8; N]]) -> Self { Witness::from_slice(slice) } +} + +impl From<&[&[u8; N]]> for Witness { + #[inline] + fn from(slice: &[&[u8; N]]) -> Self { Witness::from_slice(slice) } +} + +impl From<[[u8; M]; N]> for Witness { + #[inline] + fn from(slice: [[u8; M]; N]) -> Self { Witness::from_slice(&slice) } +} + +impl From<&[[u8; M]; N]> for Witness { + #[inline] + fn from(slice: &[[u8; M]; N]) -> Self { Witness::from_slice(slice) } +} + +impl From<[&[u8; M]; N]> for Witness { + #[inline] + fn from(slice: [&[u8; M]; N]) -> Self { Witness::from_slice(&slice) } +} + +impl From<&[&[u8; M]; N]> for Witness { + #[inline] + fn from(slice: &[&[u8; M]; N]) -> Self { Witness::from_slice(slice) } +} + impl Default for Witness { #[inline] fn default() -> Self { Self::new() } From c8078360d24d2e1de85a31a2e97a7a4fe96c7912 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Sun, 23 Mar 2025 18:46:35 +0100 Subject: [PATCH 2/4] Impl `PartialEq` between `Witness` and containers Since `Witness` is semantically equivalent to `&[&[u8]]` they should also be comparable. However we only had the impl to compare `Witness` with itself. Being able to compare `Witness` with other containers is particularly needed in tests. --- primitives/src/witness.rs | 126 +++++++++++++++++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-) diff --git a/primitives/src/witness.rs b/primitives/src/witness.rs index b2242932a..f05456262 100644 --- a/primitives/src/witness.rs +++ b/primitives/src/witness.rs @@ -14,7 +14,7 @@ use internals::compact_size; use internals::wrap_debug::WrapDebug; use internals::slice::SliceExt; -use crate::prelude::Vec; +use crate::prelude::{Box, Vec}; /// The Witness is the data used to unlock bitcoin since the [SegWit upgrade]. /// @@ -238,6 +238,109 @@ fn decode_cursor(bytes: &[u8], start_of_indices: usize, index: usize) -> Option< bytes.get_array::<4>(start).map(|index_bytes| u32::from_ne_bytes(*index_bytes) as usize) } +// Note: we use `Borrow` in the following `PartialEq` impls specifically because of its additional +// constraints on equality semantics. +impl> PartialEq<[T]> for Witness { + fn eq(&self, rhs: &[T]) -> bool { + if self.len() != rhs.len() { + return false; + } + self.iter().zip(rhs).all(|(left, right)| left == right.borrow()) + } +} + +impl> PartialEq<&[T]> for Witness { + fn eq(&self, rhs: &&[T]) -> bool { + *self == **rhs + } +} + +impl> PartialEq for [T] { + fn eq(&self, rhs: &Witness) -> bool { + *rhs == *self + } +} + +impl> PartialEq for &[T] { + fn eq(&self, rhs: &Witness) -> bool { + *rhs == **self + } +} + +impl> PartialEq<[T; N]> for Witness { + fn eq(&self, rhs: &[T; N]) -> bool { + *self == *rhs.as_slice() + } +} + +impl> PartialEq<&[T; N]> for Witness { + fn eq(&self, rhs: &&[T; N]) -> bool { + *self == *rhs.as_slice() + } +} + +impl> PartialEq for [T; N] { + fn eq(&self, rhs: &Witness) -> bool { + *rhs == *self + } +} + +impl> PartialEq for &[T; N] { + fn eq(&self, rhs: &Witness) -> bool { + *rhs == **self + } +} + +impl> PartialEq> for Witness { + fn eq(&self, rhs: &Vec) -> bool { + *self == **rhs + } +} + +impl> PartialEq for Vec { + fn eq(&self, rhs: &Witness) -> bool { + *rhs == *self + } +} + +impl> PartialEq> for Witness { + fn eq(&self, rhs: &Box<[T]>) -> bool { + *self == **rhs + } +} + +impl> PartialEq for Box<[T]> { + fn eq(&self, rhs: &Witness) -> bool { + *rhs == *self + } +} + +impl> PartialEq> for Witness { + fn eq(&self, rhs: &alloc::rc::Rc<[T]>) -> bool { + *self == **rhs + } +} + +impl> PartialEq for alloc::rc::Rc<[T]> { + fn eq(&self, rhs: &Witness) -> bool { + *rhs == *self + } +} + +#[cfg(target_has_atomic = "ptr")] +impl> PartialEq> for Witness { + fn eq(&self, rhs: &alloc::sync::Arc<[T]>) -> bool { + *self == **rhs + } +} + +#[cfg(target_has_atomic = "ptr")] +impl> PartialEq for alloc::sync::Arc<[T]> { + fn eq(&self, rhs: &Witness) -> bool { + *rhs == *self + } +} + /// Debug implementation that displays the witness as a structured output containing: /// - Number of witness elements /// - Total bytes across all elements @@ -643,6 +746,27 @@ mod test { assert!(expected.is_empty()); } + #[test] + fn partial_eq() { + const EMPTY_BYTES: &[u8] = &[]; + assert_eq!(Vec::<&[u8]>::new(), Witness::new()); + macro_rules! ck { + ($container:expr) => { + { + let container = $container; + let witness = Witness::from(Clone::clone(&container)); + assert_eq!(witness, container, stringify!($container)); + } + } + } + ck!([EMPTY_BYTES]); + ck!([EMPTY_BYTES, EMPTY_BYTES]); + ck!([[42]]); + ck!([[42, 21]]); + ck!([&[42], EMPTY_BYTES]); + ck!([[42u8], [21]]); + } + #[test] #[cfg(feature = "serde")] fn serde_bincode_backward_compatibility() { From 3551ec2c698fe682da972e727da63cf1d574818f Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Sun, 23 Mar 2025 19:26:39 +0100 Subject: [PATCH 3/4] Don't access internalls of `Witness` in tests Accessing the internals of tested object is problematic because it makes changes to layout harder, it makes tests harder to read and it checks implementation details rather than semantics of the API (behvaior). We had such tests in `primitives::witness` so this changes them to use the newly added APIs instead. However, it still leaves `from_parts__unstable` which needs to be dealt with separately. --- primitives/src/witness.rs | 30 ++++-------------------------- 1 file changed, 4 insertions(+), 26 deletions(-) diff --git a/primitives/src/witness.rs b/primitives/src/witness.rs index f05456262..880781e9c 100644 --- a/primitives/src/witness.rs +++ b/primitives/src/witness.rs @@ -581,11 +581,7 @@ mod test { // A witness with a single element that is empty (zero length). fn single_empty_element() -> Witness { - // The first is 0 serialized as a compact size integer. - // The last four bytes represent start at index 0. - let content = [0_u8; 5]; - - Witness { witness_elements: 1, content: content.to_vec(), indices_start: 1 } + Witness::from([[0u8; 0]]) } #[test] @@ -620,13 +616,7 @@ mod test { witness.push(push); assert!(!witness.is_empty()); - let elements = [1u8, 11]; - let expected = Witness { - witness_elements: 1, - content: append_u32_vec(&elements, &[0]), // Start at index 0. - indices_start: elements.len(), - }; - assert_eq!(witness, expected); + assert_eq!(witness, [[11_u8]]); let element_0 = push.as_slice(); assert_eq!(element_0, &witness[0]); @@ -643,13 +633,7 @@ mod test { let push = [21u8, 22u8]; witness.push(push); - let elements = [1u8, 11, 2, 21, 22]; - let expected = Witness { - witness_elements: 2, - content: append_u32_vec(&elements, &[0, 2]), - indices_start: elements.len(), - }; - assert_eq!(witness, expected); + assert_eq!(witness, [&[11_u8] as &[_], &[21, 22]]); let element_1 = push.as_slice(); assert_eq!(element_1, &witness[1]); @@ -666,13 +650,7 @@ mod test { let push = [31u8, 32u8]; witness.push(push); - let elements = [1u8, 11, 2, 21, 22, 2, 31, 32]; - let expected = Witness { - witness_elements: 3, - content: append_u32_vec(&elements, &[0, 2, 5]), - indices_start: elements.len(), - }; - assert_eq!(witness, expected); + assert_eq!(witness, [&[11_u8] as &[_], &[21, 22], &[31, 32]]); let element_2 = push.as_slice(); assert_eq!(element_2, &witness[2]); From 84bee2f7b06a7bd1f435aaad18fa76a15188326e Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Sun, 23 Mar 2025 21:00:47 +0100 Subject: [PATCH 4/4] Simplify `Witness` construction in tests The `Witness`-related tests were constructing `Witness` in over-complicated way by serializing `Vec>` and then deserializing `Witness` even though they were not supposed to test serialization but Taproot accessor methods. This was difficult to understand and maintain. This change simplifies them to just construct the `Witness` from array of `Vec`s using the recently-added constructors. Note that we already have serialization tests written separately so we're not losing meaningful coverage here. --- bitcoin/src/blockdata/witness.rs | 65 +++++++------------------------- 1 file changed, 13 insertions(+), 52 deletions(-) diff --git a/bitcoin/src/blockdata/witness.rs b/bitcoin/src/blockdata/witness.rs index fd57da387..f8e8abe5b 100644 --- a/bitcoin/src/blockdata/witness.rs +++ b/bitcoin/src/blockdata/witness.rs @@ -387,14 +387,8 @@ mod test { // annex starting with 0x50 causes the branching logic. let annex = hex!("50"); - let witness_vec = vec![tapscript.clone(), control_block.clone()]; - let witness_vec_annex = vec![tapscript.clone(), control_block, annex]; - - let witness_serialized: Vec = serialize(&witness_vec); - let witness_serialized_annex: Vec = serialize(&witness_vec_annex); - - let witness = deserialize::(&witness_serialized[..]).unwrap(); - let witness_annex = deserialize::(&witness_serialized_annex[..]).unwrap(); + let witness = Witness::from([&*tapscript, &control_block]); + let witness_annex = Witness::from([&*tapscript, &control_block, &annex]); // With or without annex, the tapscript should be returned. assert_eq!(witness.tapscript(), Some(Script::from_bytes(&tapscript[..]))); @@ -409,14 +403,8 @@ mod test { // annex starting with 0x50 causes the branching logic. let annex = hex!("50"); - let witness_vec = vec![tapscript.clone(), control_block.clone()]; - let witness_vec_annex = vec![tapscript.clone(), control_block, annex]; - - let witness_serialized: Vec = serialize(&witness_vec); - let witness_serialized_annex: Vec = serialize(&witness_vec_annex); - - let witness = deserialize::(&witness_serialized[..]).unwrap(); - let witness_annex = deserialize::(&witness_serialized_annex[..]).unwrap(); + let witness = Witness::from([&*tapscript, &control_block]); + let witness_annex = Witness::from([&*tapscript, &control_block, &annex]); let expected_leaf_script = LeafScript { version: LeafVersion::TapScript, script: Script::from_bytes(&tapscript) }; @@ -432,14 +420,8 @@ mod test { // annex starting with 0x50 causes the branching logic. let annex = hex!("50"); - let witness_vec = vec![signature.clone()]; - let witness_vec_annex = vec![signature.clone(), annex]; - - let witness_serialized: Vec = serialize(&witness_vec); - let witness_serialized_annex: Vec = serialize(&witness_vec_annex); - - let witness = deserialize::(&witness_serialized[..]).unwrap(); - let witness_annex = deserialize::(&witness_serialized_annex[..]).unwrap(); + let witness = Witness::from([&*signature]); + let witness_annex = Witness::from([&*signature, &annex]); // With or without annex, no tapscript should be returned. assert_eq!(witness.tapscript(), None); @@ -454,18 +436,9 @@ mod test { let annex = hex!("50"); let signature = vec![0xff; 64]; - let witness_vec = vec![tapscript.clone(), control_block.clone()]; - let witness_vec_annex = vec![tapscript.clone(), control_block.clone(), annex.clone()]; - let witness_vec_key_spend_annex = vec![signature, annex]; - - let witness_serialized: Vec = serialize(&witness_vec); - let witness_serialized_annex: Vec = serialize(&witness_vec_annex); - let witness_serialized_key_spend_annex: Vec = serialize(&witness_vec_key_spend_annex); - - let witness = deserialize::(&witness_serialized[..]).unwrap(); - let witness_annex = deserialize::(&witness_serialized_annex[..]).unwrap(); - let witness_key_spend_annex = - deserialize::(&witness_serialized_key_spend_annex[..]).unwrap(); + let witness = Witness::from([&*tapscript, &control_block]); + let witness_annex = Witness::from([&*tapscript, &control_block, &annex]); + let witness_key_spend_annex = Witness::from([&*signature, &annex]); // With or without annex, the tapscript should be returned. assert_eq!(witness.taproot_control_block(), Some(&control_block[..])); @@ -480,14 +453,8 @@ mod test { // annex starting with 0x50 causes the branching logic. let annex = hex!("50"); - let witness_vec = vec![tapscript.clone(), control_block.clone()]; - let witness_vec_annex = vec![tapscript.clone(), control_block.clone(), annex.clone()]; - - let witness_serialized: Vec = serialize(&witness_vec); - let witness_serialized_annex: Vec = serialize(&witness_vec_annex); - - let witness = deserialize::(&witness_serialized[..]).unwrap(); - let witness_annex = deserialize::(&witness_serialized_annex[..]).unwrap(); + let witness = Witness::from([&*tapscript, &control_block]); + let witness_annex = Witness::from([&*tapscript, &control_block, &annex]); // With or without annex, the tapscript should be returned. assert_eq!(witness.taproot_annex(), None); @@ -498,14 +465,8 @@ mod test { // annex starting with 0x50 causes the branching logic. let annex = hex!("50"); - let witness_vec = vec![signature.clone()]; - let witness_vec_annex = vec![signature.clone(), annex.clone()]; - - let witness_serialized: Vec = serialize(&witness_vec); - let witness_serialized_annex: Vec = serialize(&witness_vec_annex); - - let witness = deserialize::(&witness_serialized[..]).unwrap(); - let witness_annex = deserialize::(&witness_serialized_annex[..]).unwrap(); + let witness = Witness::from([&*signature]); + let witness_annex = Witness::from([&*signature, &annex]); // With or without annex, the tapscript should be returned. assert_eq!(witness.taproot_annex(), None);