From 024e619519d05e28053ec80bd2d9ac2d360c8b13 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 09:03:16 +1100 Subject: [PATCH 01/15] Fix manifest description Its v2 not v1, woops. While we are at it capitalise Signed. --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 417be6a..fd8dd4b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,7 +4,7 @@ version = "0.1.0" authors = ["Tobin C. Harding "] license = "CC0-1.0" repository = "https://github.com/tcharding/rust-psbt/" -description = "Partially signed Bitcoin Transaction, v0 and v1" +description = "Partially Signed Bitcoin Transaction, v0 and v2" categories = ["cryptography::cryptocurrencies"] keywords = [ "crypto", "bitcoin" ] readme = "../README.md" From b7f0b86bee0e890c4a8689b9189a667e94845613 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 09:13:07 +1100 Subject: [PATCH 02/15] Fix no-std build, use prelude --- src/v0/miniscript/error.rs | 1 + src/v2/miniscript/finalize.rs | 1 + src/v2/miniscript/mod.rs | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/v0/miniscript/error.rs b/src/v0/miniscript/error.rs index 4ddfaab..a2b3a88 100644 --- a/src/v0/miniscript/error.rs +++ b/src/v0/miniscript/error.rs @@ -4,6 +4,7 @@ use core::fmt; use crate::bitcoin::{sighash, ScriptBuf}; use crate::miniscript::{self, descriptor, interpreter}; +use crate::prelude::*; #[cfg(doc)] use crate::v0::Psbt; diff --git a/src/v2/miniscript/finalize.rs b/src/v2/miniscript/finalize.rs index a21024d..6432f47 100644 --- a/src/v2/miniscript/finalize.rs +++ b/src/v2/miniscript/finalize.rs @@ -17,6 +17,7 @@ use miniscript::{ }; use crate::error::{write_err, FundingUtxoError}; +use crate::prelude::*; use crate::v2::map::input::{self, Input}; use crate::v2::miniscript::satisfy::InputSatisfier; use crate::v2::miniscript::InterpreterCheckError; diff --git a/src/v2/miniscript/mod.rs b/src/v2/miniscript/mod.rs index 46c497d..ebc5f6d 100644 --- a/src/v2/miniscript/mod.rs +++ b/src/v2/miniscript/mod.rs @@ -31,7 +31,7 @@ use miniscript::miniscript::satisfy::Placeholder; use miniscript::{interpreter, Interpreter, MiniscriptKey}; use crate::error::write_err; -use crate::prelude::Borrow; +use crate::prelude::*; use crate::v2::map::Input; use crate::v2::{DetermineLockTimeError, Psbt}; From 3a61b125be9b2cb0379e80e10ce2a4be544a8884 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 09:13:48 +1100 Subject: [PATCH 03/15] Fix readme path --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index fd8dd4b..d6ef1cd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,7 +7,7 @@ repository = "https://github.com/tcharding/rust-psbt/" description = "Partially Signed Bitcoin Transaction, v0 and v2" categories = ["cryptography::cryptocurrencies"] keywords = [ "crypto", "bitcoin" ] -readme = "../README.md" +readme = "README.md" edition = "2021" rust-version = "1.56.1" exclude = ["tests", "contrib"] From f10679ea5ec5f659cc16a46dc115c66d1a5e9dd2 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 09:15:40 +1100 Subject: [PATCH 04/15] Clean up manifest dependencies --- Cargo.toml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d6ef1cd..1b7e772 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,11 +27,10 @@ miniscript-std = ["std", "miniscript/std"] miniscript-no-std = ["no-std", "miniscript/no-std"] [dependencies] -bitcoin = { version = "0.31.0", default-features = false, features = [] } +bitcoin = { version = "0.31.0", default-features = false } -# Currenty miniscript only works in with "std" enabled. +# Do not use this feature, use "miniscript-std" or "miniscript-no-std" instead. miniscript = { version = "11.0.0", default-features = false, optional = true } - # Do NOT use this as a feature! Use the `serde` feature instead. actual-serde = { package = "serde", version = "1.0.103", default-features = false, features = [ "derive", "alloc" ], optional = true } # There is no reason to use this dependency directly, it is activated by the "no-std" feature. From d3eb400c7d98d3a1fcf7f7c4f29976a562341029 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 09:27:29 +1100 Subject: [PATCH 05/15] Publicly re-export Version --- src/lib.rs | 4 ++-- src/version.rs | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 4ce2b76..1dc7b0d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -54,11 +54,11 @@ use std::io; #[cfg(not(feature = "std"))] use core2::io; -use crate::version::Version; - #[rustfmt::skip] // Keep pubic re-exports separate +#[doc(inline)] pub use crate::{ sighash_type::PsbtSighashType, + version::Version, }; /// PSBT version 0 - the original PSBT version. diff --git a/src/version.rs b/src/version.rs index c869d36..7bb4dc9 100644 --- a/src/version.rs +++ b/src/version.rs @@ -16,9 +16,13 @@ pub struct Version(u32); impl Version { /// The original PSBT format [BIP-174]. + /// + /// [BIP-174]: pub const ZERO: Self = Self(0); /// The second PSBT version [BIP-370]. + /// + /// [BIP-370]: pub const TWO: Self = Self(2); } From 93ee2a21b27dce0dd9b84080284a4b76da4827f9 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 09:34:17 +1100 Subject: [PATCH 06/15] Improve Constructor link --- src/v2/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/v2/mod.rs b/src/v2/mod.rs index 5a30574..aed9335 100644 --- a/src/v2/mod.rs +++ b/src/v2/mod.rs @@ -73,7 +73,7 @@ pub fn combine(this: Psbt, that: Psbt) -> Result { this.comb /// - You need to set the fallback lock time. /// - You need to set the sighash single flag. /// -/// If not use [`Constructor::default()`] to carry out both roles. +/// If not use the [`Constructor`] to carry out both roles e.g., `Constructor::default()`. /// /// See `examples/v2-separate-creator-constructor.rs`. #[derive(Debug, Clone, PartialEq, Eq, Hash)] From 88002e542d16b7d966be21a3cdf44ba04ca5d57f Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 09:41:34 +1100 Subject: [PATCH 07/15] Put id in the same place as for Signer Make the docs easier to read, put the `id` function in the same place as it is in `Signer`. --- src/v2/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/v2/mod.rs b/src/v2/mod.rs index aed9335..04caa32 100644 --- a/src/v2/mod.rs +++ b/src/v2/mod.rs @@ -378,6 +378,11 @@ impl Updater { Ok(Self(psbt)) } + /// Returns this PSBT's unique identification. + pub fn id(&self) -> Txid { + self.0.id().expect("Updater guarantees lock time can be determined") + } + /// Updater role, update the sequence number for input at `index`. pub fn set_sequence( mut self, @@ -389,11 +394,6 @@ impl Updater { Ok(self) } - /// Returns this PSBT's unique identification. - pub fn id(&self) -> Txid { - self.0.id().expect("Updater guarantees lock time can be determined") - } - /// Converts the inner PSBT v2 to a PSBT v0. pub fn into_psbt_v0(self) -> v0::Psbt { let unsigned_tx = From be9a5625cce9d1fe9cb0b58bc8e767fbfa65fbc9 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 09:54:39 +1100 Subject: [PATCH 08/15] Add id to Extractor and Finalizer Both these roles should be able to get the PSBT's id. --- src/v2/extract.rs | 49 ++++++++++++++++++++++++++++------- src/v2/miniscript/finalize.rs | 20 ++++++++++++-- 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/src/v2/extract.rs b/src/v2/extract.rs index 8cffc7a..8ae6086 100644 --- a/src/v2/extract.rs +++ b/src/v2/extract.rs @@ -15,7 +15,7 @@ use core::fmt; -use bitcoin::{FeeRate, Transaction}; +use bitcoin::{FeeRate, Transaction, Txid}; use crate::error::{write_err, FeeError}; use crate::v2::{DetermineLockTimeError, Psbt}; @@ -30,13 +30,19 @@ impl Extractor { /// Creates an `Extractor`. /// /// An extractor can only accept a PSBT that has been finalized. - pub fn new(psbt: Psbt) -> Result { + pub fn new(psbt: Psbt) -> Result { if psbt.inputs.iter().any(|input| !input.is_finalized()) { - return Err(PsbtNotFinalizedError); + return Err(Error::PsbtNotFinalized); } + let _ = psbt.determine_lock_time()?; Ok(Self(psbt)) } + + /// Returns this PSBT's unique identification. + pub fn id(&self) -> Txid { + self.0.id().expect("Extractor guarantees lock time can be determined") + } } impl Extractor { @@ -118,19 +124,42 @@ impl Extractor { } } -/// Attempted to extract tx from an unfinalized PSBT. -#[derive(Debug, Clone, PartialEq, Eq)] -#[non_exhaustive] -pub struct PsbtNotFinalizedError; +/// Error constructing a [`Finalizer`]. +#[derive(Debug)] +pub enum Error { + /// Attempted to extract tx from an unfinalized PSBT. + PsbtNotFinalized, + /// Finalizer must be able to determine the lock time. + DetermineLockTime(DetermineLockTimeError), +} -impl fmt::Display for PsbtNotFinalizedError { +impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "attempted to extract tx from an unfinalized PSBT") + use Error::*; + + match *self { + PsbtNotFinalized => write!(f, "attempted to extract tx from an unfinalized PSBT"), + DetermineLockTime(ref e) => + write_err!(f, "extractor must be able to determine the lock time"; e), + } } } #[cfg(feature = "std")] -impl std::error::Error for PsbtNotFinalizedError {} +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use Error::*; + + match *self { + DetermineLockTime(ref e) => Some(e), + PsbtNotFinalized => None, + } + } +} + +impl From for Error { + fn from(e: DetermineLockTimeError) -> Self { Self::DetermineLockTime(e) } +} /// Error caused by fee calculation when extracting a [`Transaction`] from a PSBT. #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/src/v2/miniscript/finalize.rs b/src/v2/miniscript/finalize.rs index 6432f47..7550aeb 100644 --- a/src/v2/miniscript/finalize.rs +++ b/src/v2/miniscript/finalize.rs @@ -10,7 +10,7 @@ use core::fmt; use bitcoin::hashes::hash160; use bitcoin::secp256k1::{Secp256k1, Verification}; use bitcoin::taproot::LeafVersion; -use bitcoin::{sighash, Address, Network, Script, ScriptBuf, Witness, XOnlyPublicKey}; +use bitcoin::{sighash, Address, Network, Script, ScriptBuf, Txid, Witness, XOnlyPublicKey}; use miniscript::{ interpreter, BareCtx, Descriptor, ExtParams, Legacy, Miniscript, Satisfier, Segwitv0, SigType, Tap, ToPublicKey, @@ -21,7 +21,7 @@ use crate::prelude::*; use crate::v2::map::input::{self, Input}; use crate::v2::miniscript::satisfy::InputSatisfier; use crate::v2::miniscript::InterpreterCheckError; -use crate::v2::{PartialSigsSighashTypeError, Psbt}; +use crate::v2::{DetermineLockTimeError, PartialSigsSighashTypeError, Psbt}; /// Implements the BIP-370 Finalized role. #[derive(Debug, Clone, PartialEq, Eq, Hash)] @@ -38,11 +38,17 @@ impl Finalizer { for input in psbt.inputs.iter() { let _ = input.funding_utxo()?; } + let _ = psbt.determine_lock_time()?; psbt.check_partial_sigs_sighash_type()?; Ok(Self(psbt)) } + /// Returns this PSBT's unique identification. + pub fn id(&self) -> Txid { + self.0.id().expect("Finalizer guarantees lock time can be determined") + } + /// Finalize the PSBT. /// /// # Returns @@ -361,6 +367,8 @@ fn construct_tap_witness( pub enum Error { /// An input is missing its funding UTXO. FundingUtxo(FundingUtxoError), + /// Finalizer must be able to determine the lock time. + DetermineLockTime(DetermineLockTimeError), /// An input has incorrect sighash type for its partial sigs (ECDSA). PartialSigsSighashType(PartialSigsSighashTypeError), } @@ -370,7 +378,10 @@ impl fmt::Display for Error { use Error::*; match *self { + // TODO: Loads of error messages are capitalized, they should not be. FundingUtxo(ref e) => write_err!(f, "Finalizer missing funding UTXO"; e), + DetermineLockTime(ref e) => + write_err!(f, "finalizer must be able to determine the lock time"; e), PartialSigsSighashType(ref e) => write_err!(f, "Finalizer sighash type error"; e), } } @@ -383,6 +394,7 @@ impl std::error::Error for Error { match *self { FundingUtxo(ref e) => Some(e), + DetermineLockTime(ref e) => Some(e), PartialSigsSighashType(ref e) => Some(e), } } @@ -392,6 +404,10 @@ impl From for Error { fn from(e: FundingUtxoError) -> Self { Self::FundingUtxo(e) } } +impl From for Error { + fn from(e: DetermineLockTimeError) -> Self { Self::DetermineLockTime(e) } +} + impl From for Error { fn from(e: PartialSigsSighashTypeError) -> Self { Self::PartialSigsSighashType(e) } } From 53eca5a953618eeb10e34f6dad60e30ef093ea16 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 09:57:47 +1100 Subject: [PATCH 09/15] Improve Extractor rustdocs --- src/v2/extract.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/v2/extract.rs b/src/v2/extract.rs index 8ae6086..36f0ab4 100644 --- a/src/v2/extract.rs +++ b/src/v2/extract.rs @@ -9,7 +9,7 @@ //! //! It is only possible to extract a transaction from a PSBT _after_ it has been finalized. However //! the Extractor role may be fulfilled by a separate entity to the Finalizer hence this is a -//! separate module and does not require `rust-miniscript`. +//! separate module and does not require the "miniscript" feature be enabled. //! //! [BIP-174]: From 7db0291b1dfe7bf0ed28c9bfbfabb2fce6621da4 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 10:03:02 +1100 Subject: [PATCH 10/15] Rename DeserializePsbtError to DeserializeError --- src/v0/error.rs | 15 +++++++-------- src/v0/mod.rs | 15 +++++++-------- src/v2/error.rs | 13 ++++++------- src/v2/mod.rs | 8 ++++---- tests/util.rs | 4 ++-- 5 files changed, 26 insertions(+), 29 deletions(-) diff --git a/src/v0/error.rs b/src/v0/error.rs index 14b0d80..0235f8c 100644 --- a/src/v0/error.rs +++ b/src/v0/error.rs @@ -14,10 +14,9 @@ use crate::v0::Psbt; /// /// This error is returned when deserializing a complete PSBT, not for deserializing parts /// of it or individual data types. -// TODO: This can change to `serialize::Error` if we rename `serialize::Error` to `serialize::Error`. #[derive(Debug)] #[non_exhaustive] -pub enum DeserializePsbtError { +pub enum DeserializeError { /// Invalid magic bytes, expected the ASCII for "psbt" serialized in most significant byte order. // TODO: Consider adding the invalid bytes. InvalidMagic, @@ -36,28 +35,28 @@ pub enum DeserializePsbtError { UnsignedTxChecks(UnsignedTxChecksError), } -impl fmt::Display for DeserializePsbtError { +impl fmt::Display for DeserializeError { fn fmt(&self, _f: &mut fmt::Formatter<'_>) -> fmt::Result { todo!() } } #[cfg(feature = "std")] -impl std::error::Error for DeserializePsbtError { +impl std::error::Error for DeserializeError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { todo!() } } -impl From for DeserializePsbtError { +impl From for DeserializeError { fn from(e: global::DecodeError) -> Self { Self::DecodeGlobal(e) } } -impl From for DeserializePsbtError { +impl From for DeserializeError { fn from(e: input::DecodeError) -> Self { Self::DecodeInput(e) } } -impl From for DeserializePsbtError { +impl From for DeserializeError { fn from(e: output::DecodeError) -> Self { Self::DecodeOutput(e) } } -impl From for DeserializePsbtError { +impl From for DeserializeError { fn from(e: UnsignedTxChecksError) -> Self { Self::UnsignedTxChecks(e) } } diff --git a/src/v0/mod.rs b/src/v0/mod.rs index 4710de0..9ab2a11 100644 --- a/src/v0/mod.rs +++ b/src/v0/mod.rs @@ -30,7 +30,7 @@ use crate::v0::map::{global, Map}; #[rustfmt::skip] // Keep pubic re-exports separate pub use self::{ - error::{IndexOutOfBoundsError, SignerChecksError, SignError, UnsignedTxChecksError, DeserializePsbtError}, + error::{IndexOutOfBoundsError, SignerChecksError, SignError, UnsignedTxChecksError, DeserializeError}, map::{Input, Output, Global}, }; @@ -84,20 +84,19 @@ impl Psbt { buf } - // TODO: Change this to use DeserializePsbtError (although that name is shit) same as v2. /// Deserialize a value from raw binary data. - pub fn deserialize(bytes: &[u8]) -> Result { + pub fn deserialize(bytes: &[u8]) -> Result { const MAGIC_BYTES: &[u8] = b"psbt"; if bytes.get(0..MAGIC_BYTES.len()) != Some(MAGIC_BYTES) { - return Err(DeserializePsbtError::InvalidMagic); + return Err(DeserializeError::InvalidMagic); } const PSBT_SERPARATOR: u8 = 0xff_u8; if bytes.get(MAGIC_BYTES.len()) != Some(&PSBT_SERPARATOR) { - return Err(DeserializePsbtError::InvalidSeparator); + return Err(DeserializeError::InvalidSeparator); } - let mut d = bytes.get(5..).ok_or(DeserializePsbtError::NoMorePairs)?; + let mut d = bytes.get(5..).ok_or(DeserializeError::NoMorePairs)?; let global = Global::decode(&mut d)?; global.unsigned_tx_checks()?; @@ -565,7 +564,7 @@ mod display_from_str { #[non_exhaustive] pub enum PsbtParseError { /// Error in internal PSBT data structure. - PsbtEncoding(DeserializePsbtError), + PsbtEncoding(DeserializeError), /// Error in PSBT Base64 encoding. Base64Encoding(bitcoin::base64::DecodeError), } @@ -808,7 +807,7 @@ mod tests { use crate::{io, raw, V0}; #[track_caller] - pub fn hex_psbt(s: &str) -> Result { + pub fn hex_psbt(s: &str) -> Result { let r: Result, bitcoin::hex::HexToBytesError> = Vec::from_hex(s); match r { Err(_e) => panic!("unable to parse hex string {}", s), diff --git a/src/v2/error.rs b/src/v2/error.rs index 62f8aa5..93a43e3 100644 --- a/src/v2/error.rs +++ b/src/v2/error.rs @@ -14,10 +14,9 @@ use crate::v2::map::{global, input, output}; /// /// This error is returned when deserializing a complete PSBT, not for deserializing parts /// of it or individual data types. -// TODO: This can change to `serialize::Error` if we rename `serialize::Error` to `serialize::Error`. #[derive(Debug)] #[non_exhaustive] -pub enum DeserializePsbtError { +pub enum DeserializeError { /// Invalid magic bytes, expected the ASCII for "psbt" serialized in most significant byte order. // TODO: Consider adding the invalid bytes. InvalidMagic, @@ -34,24 +33,24 @@ pub enum DeserializePsbtError { DecodeOutput(output::DecodeError), } -impl fmt::Display for DeserializePsbtError { +impl fmt::Display for DeserializeError { fn fmt(&self, _f: &mut fmt::Formatter<'_>) -> fmt::Result { todo!() } } #[cfg(feature = "std")] -impl std::error::Error for DeserializePsbtError { +impl std::error::Error for DeserializeError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { todo!() } } -impl From for DeserializePsbtError { +impl From for DeserializeError { fn from(e: global::DecodeError) -> Self { Self::DecodeGlobal(e) } } -impl From for DeserializePsbtError { +impl From for DeserializeError { fn from(e: input::DecodeError) -> Self { Self::DecodeInput(e) } } -impl From for DeserializePsbtError { +impl From for DeserializeError { fn from(e: output::DecodeError) -> Self { Self::DecodeOutput(e) } } diff --git a/src/v2/mod.rs b/src/v2/mod.rs index 04caa32..8e3182e 100644 --- a/src/v2/mod.rs +++ b/src/v2/mod.rs @@ -50,7 +50,7 @@ use crate::v2::map::{global, input, output, Map}; #[rustfmt::skip] // Keep public exports separate. #[doc(inline)] pub use self::{ - error::{IndexOutOfBoundsError, SignError, PsbtNotModifiableError, NotUnsignedError, OutputsNotModifiableError, InputsNotModifiableError, DetermineLockTimeError, DeserializePsbtError, PartialSigsSighashTypeError}, + error::{IndexOutOfBoundsError, SignError, PsbtNotModifiableError, NotUnsignedError, OutputsNotModifiableError, InputsNotModifiableError, DetermineLockTimeError, DeserializeError, PartialSigsSighashTypeError}, map::{Input, InputBuilder, Output, OutputBuilder, Global}, extract::{ExtractTxError, ExtractTxFeeRateError, Extractor} }; @@ -603,8 +603,8 @@ impl Psbt { } /// Deserialize a value from raw binary data. - pub fn deserialize(bytes: &[u8]) -> Result { - use DeserializePsbtError::*; + pub fn deserialize(bytes: &[u8]) -> Result { + use DeserializeError::*; const MAGIC_BYTES: &[u8] = b"psbt"; if bytes.get(0..MAGIC_BYTES.len()) != Some(MAGIC_BYTES) { @@ -1269,7 +1269,7 @@ mod display_from_str { #[non_exhaustive] pub enum PsbtParseError { /// Error in internal PSBT data structure. - PsbtEncoding(DeserializePsbtError), + PsbtEncoding(DeserializeError), /// Error in PSBT Base64 encoding. Base64Encoding(bitcoin::base64::DecodeError), } diff --git a/tests/util.rs b/tests/util.rs index b9da648..496d620 100644 --- a/tests/util.rs +++ b/tests/util.rs @@ -8,7 +8,7 @@ use psbt::bitcoin::hex::{self, FromHex}; use psbt::{v0, v2}; #[track_caller] -pub fn hex_psbt_v0(s: &str) -> Result { +pub fn hex_psbt_v0(s: &str) -> Result { let r: Result, hex::HexToBytesError> = Vec::from_hex(s); match r { Err(_e) => panic!("unable to parse PSBT v0 from hex string {}", s), @@ -17,7 +17,7 @@ pub fn hex_psbt_v0(s: &str) -> Result { } #[track_caller] -pub fn hex_psbt_v2(s: &str) -> Result { +pub fn hex_psbt_v2(s: &str) -> Result { let r: Result, hex::HexToBytesError> = Vec::from_hex(s); match r { Err(_e) => panic!("unable to parse PSBT v2 from hex string {}", s), From b23e38a2d6b2d0b5bdbc6ad144978782bb9af7e6 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 10:04:47 +1100 Subject: [PATCH 11/15] Improve TODO comment about sighash single --- src/v2/map/global.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/v2/map/global.rs b/src/v2/map/global.rs index ec42caa..8b0379b 100644 --- a/src/v2/map/global.rs +++ b/src/v2/map/global.rs @@ -127,7 +127,7 @@ impl Global { self.tx_modifiable_flags & OUTPUTS_MODIFIABLE > 0 } - // TODO: Use this function? + // TODO: Investigate if we should be using this function? #[allow(dead_code)] pub(crate) fn has_sighash_single(&self) -> bool { self.tx_modifiable_flags & SIGHASH_SINGLE > 0 From 392684bab33ddcb89989ab3ce7ab104bf8277f0f Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 10:13:47 +1100 Subject: [PATCH 12/15] Update TODO about memory exhaustion --- src/v2/map/global.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/v2/map/global.rs b/src/v2/map/global.rs index 8b0379b..8038962 100644 --- a/src/v2/map/global.rs +++ b/src/v2/map/global.rs @@ -4,7 +4,6 @@ use core::convert::TryFrom; use core::fmt; use bitcoin::bip32::{ChildNumber, DerivationPath, Fingerprint, KeySource, Xpub}; -use bitcoin::consensus::encode::MAX_VEC_SIZE; use bitcoin::consensus::{encode as consensus, Decodable}; use bitcoin::locktime::absolute; use bitcoin::{bip32, transaction, Transaction, VarInt}; @@ -134,9 +133,8 @@ impl Global { } pub(crate) fn decode(r: &mut R) -> Result { - // TODO(tobin): Work out why do we do this take, its not done in input or output modules. - let mut r = r.take(MAX_VEC_SIZE as u64); - + // TODO: Consider adding protection against memory exhaustion here by defining a maximum + // PBST size and using `take` as we do in rust-bitcoin consensus decoding. let mut version: Option = None; let mut tx_version: Option = None; let mut fallback_lock_time: Option = None; @@ -312,7 +310,7 @@ impl Global { }; loop { - match raw::Pair::decode(&mut r) { + match raw::Pair::decode(r) { Ok(pair) => insert_pair(pair)?, Err(serialize::Error::NoMorePairs) => break, Err(e) => return Err(DecodeError::DeserPair(e)), From 07960e89a654c630f0a4d0e3bd32ff4ba9f9579b Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 10:40:37 +1100 Subject: [PATCH 13/15] Move sighash type tests to the sighash_type module --- src/lib.rs | 2 +- src/sighash_type.rs | 64 +++++++++++++++++++++++++++++++++++++++++++-- src/v2/map/input.rs | 52 ------------------------------------ 3 files changed, 63 insertions(+), 55 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 1dc7b0d..a4733df 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -57,7 +57,7 @@ use core2::io; #[rustfmt::skip] // Keep pubic re-exports separate #[doc(inline)] pub use crate::{ - sighash_type::PsbtSighashType, + sighash_type::{PsbtSighashType, InvalidSighashTypeError}, version::Version, }; diff --git a/src/sighash_type.rs b/src/sighash_type.rs index cba2c32..97e9bb9 100644 --- a/src/sighash_type.rs +++ b/src/sighash_type.rs @@ -113,13 +113,15 @@ impl std::error::Error for SighashTypeParseError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None } } +// TODO: Remove this error after issue resolves. +// https://github.com/rust-bitcoin/rust-bitcoin/issues/2423 /// Integer is not a consensus valid sighash type. #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub enum InvalidSighashTypeError { - /// TODO: + /// The real invalid sighash type error. Bitcoin(sighash::InvalidSighashTypeError), - /// TODO: + /// Hack required because of non_exhaustive on the real error. Invalid(u32), } @@ -149,3 +151,61 @@ impl std::error::Error for InvalidSighashTypeError { impl From for InvalidSighashTypeError { fn from(e: sighash::InvalidSighashTypeError) -> Self { Self::Bitcoin(e) } } + +#[cfg(test)] +mod tests { + use core::str::FromStr; + + use super::*; + use crate::sighash_type::InvalidSighashTypeError; + + #[test] + fn psbt_sighash_type_ecdsa() { + for ecdsa in &[ + EcdsaSighashType::All, + EcdsaSighashType::None, + EcdsaSighashType::Single, + EcdsaSighashType::AllPlusAnyoneCanPay, + EcdsaSighashType::NonePlusAnyoneCanPay, + EcdsaSighashType::SinglePlusAnyoneCanPay, + ] { + let sighash = PsbtSighashType::from(*ecdsa); + let s = format!("{}", sighash); + let back = PsbtSighashType::from_str(&s).unwrap(); + assert_eq!(back, sighash); + assert_eq!(back.ecdsa_hash_ty().unwrap(), *ecdsa); + } + } + + #[test] + fn psbt_sighash_type_taproot() { + for tap in &[ + TapSighashType::Default, + TapSighashType::All, + TapSighashType::None, + TapSighashType::Single, + TapSighashType::AllPlusAnyoneCanPay, + TapSighashType::NonePlusAnyoneCanPay, + TapSighashType::SinglePlusAnyoneCanPay, + ] { + let sighash = PsbtSighashType::from(*tap); + let s = format!("{}", sighash); + let back = PsbtSighashType::from_str(&s).unwrap(); + assert_eq!(back, sighash); + assert_eq!(back.taproot_hash_ty().unwrap(), *tap); + } + } + + #[test] + fn psbt_sighash_type_notstd() { + let nonstd = 0xdddddddd; + let sighash = PsbtSighashType { inner: nonstd }; + let s = format!("{}", sighash); + let back = PsbtSighashType::from_str(&s).unwrap(); + + assert_eq!(back, sighash); + // TODO: Add this assertion once we remove InvalidSighashTypeError + // assert_eq!(back.ecdsa_hash_ty(), Err(NonStandardSighashTypeError(nonstd))); + assert_eq!(back.taproot_hash_ty(), Err(InvalidSighashTypeError::Invalid(nonstd))); + } +} diff --git a/src/v2/map/input.rs b/src/v2/map/input.rs index 8b8259d..c359df4 100644 --- a/src/v2/map/input.rs +++ b/src/v2/map/input.rs @@ -997,60 +997,8 @@ impl std::error::Error for CombineError { #[cfg(test)] mod test { - use core::str::FromStr; - use super::*; - #[test] - fn psbt_sighash_type_ecdsa() { - for ecdsa in &[ - EcdsaSighashType::All, - EcdsaSighashType::None, - EcdsaSighashType::Single, - EcdsaSighashType::AllPlusAnyoneCanPay, - EcdsaSighashType::NonePlusAnyoneCanPay, - EcdsaSighashType::SinglePlusAnyoneCanPay, - ] { - let sighash = PsbtSighashType::from(*ecdsa); - let s = format!("{}", sighash); - let back = PsbtSighashType::from_str(&s).unwrap(); - assert_eq!(back, sighash); - assert_eq!(back.ecdsa_hash_ty().unwrap(), *ecdsa); - } - } - - #[test] - fn psbt_sighash_type_taproot() { - for tap in &[ - TapSighashType::Default, - TapSighashType::All, - TapSighashType::None, - TapSighashType::Single, - TapSighashType::AllPlusAnyoneCanPay, - TapSighashType::NonePlusAnyoneCanPay, - TapSighashType::SinglePlusAnyoneCanPay, - ] { - let sighash = PsbtSighashType::from(*tap); - let s = format!("{}", sighash); - let back = PsbtSighashType::from_str(&s).unwrap(); - assert_eq!(back, sighash); - assert_eq!(back.taproot_hash_ty().unwrap(), *tap); - } - } - - #[test] - fn psbt_sighash_type_notstd() { - let nonstd = 0xdddddddd; - let sighash = PsbtSighashType { inner: nonstd }; - let s = format!("{}", sighash); - let back = PsbtSighashType::from_str(&s).unwrap(); - - assert_eq!(back, sighash); - // TODO: Uncomment this stuff. - // assert_eq!(back.ecdsa_hash_ty(), Err(NonStandardSighashTypeError(nonstd))); - // assert_eq!(back.taproot_hash_ty(), Err(InvalidSighashTypeError(nonstd))); - } - #[cfg(feature = "std")] fn out_point() -> OutPoint { let txid = Txid::hash(b"some arbitrary bytes"); From 3e757ed86822f33b9c0182cc6f897c39660c4c46 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 10:54:12 +1100 Subject: [PATCH 14/15] Make function private --- src/v2/mod.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/v2/mod.rs b/src/v2/mod.rs index 8e3182e..adab0e0 100644 --- a/src/v2/mod.rs +++ b/src/v2/mod.rs @@ -955,9 +955,8 @@ impl Psbt { /// Checks the sighash types of input partial sigs (ECDSA). /// /// This can be used at anytime but is primarily used during PSBT finalizing. - // TODO: Would pub(crate) be better? - // TODO: It would be nice if this was enforced by the typesystem and fields if the `Input`. - pub fn check_partial_sigs_sighash_type(&self) -> Result<(), PartialSigsSighashTypeError> { + #[cfg(feature = "miniscript")] + pub(crate) fn check_partial_sigs_sighash_type(&self) -> Result<(), PartialSigsSighashTypeError> { use PartialSigsSighashTypeError::*; for (input_index, input) in self.inputs.iter().enumerate() { From 66abe2c0839281193cfe75b39ee7761bde378535 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Feb 2024 11:06:41 +1100 Subject: [PATCH 15/15] Remove TODO, determine lock time is useful --- src/v2/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/v2/mod.rs b/src/v2/mod.rs index adab0e0..69cc251 100644 --- a/src/v2/mod.rs +++ b/src/v2/mod.rs @@ -526,7 +526,6 @@ impl Psbt { /// Determines the lock time as specified in [BIP-370] if it is possible to do so. /// /// [BIP-370]: - // TODO: Does this need to be public? pub fn determine_lock_time(&self) -> Result { let require_time_based_lock_time = self.inputs.iter().any(|input| input.requires_time_based_lock_time());