From ca38dbd16de769841a3b68770dbcc0fd50aad265 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 23 Jun 2023 15:13:52 +1000 Subject: [PATCH] transaction:: Return custom error from verify function There is not need to return the general `script::Error` from the transaction verify functions. We can better describe the error path by returning a custom error. --- bitcoin/src/blockdata/script/mod.rs | 54 ----------------- bitcoin/src/blockdata/transaction.rs | 90 +++++++++++++++++++++++++--- 2 files changed, 83 insertions(+), 61 deletions(-) diff --git a/bitcoin/src/blockdata/script/mod.rs b/bitcoin/src/blockdata/script/mod.rs index bc4c41a6..ecd68547 100644 --- a/bitcoin/src/blockdata/script/mod.rs +++ b/bitcoin/src/blockdata/script/mod.rs @@ -680,66 +680,19 @@ pub enum Error { EarlyEndOfScript, /// Tried to read an array off the stack as a number when it was more than 4 bytes. NumericOverflow, - /// Error validating the script with bitcoinconsensus library. - #[cfg(feature = "bitcoinconsensus")] - BitcoinConsensus(bitcoinconsensus::Error), /// Can not find the spent output. UnknownSpentOutput(OutPoint), /// Can not serialize the spending transaction. Serialization, } -// If bitcoinonsensus-std is off but bitcoinconsensus is present we patch the error type to -// implement `std::error::Error`. -#[cfg(all(feature = "std", feature = "bitcoinconsensus", not(feature = "bitcoinconsensus-std")))] -mod bitcoinconsensus_hack { - use core::fmt; - - #[repr(transparent)] - pub(crate) struct Error(bitcoinconsensus::Error); - - impl fmt::Debug for Error { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Debug::fmt(&self.0, f) } - } - - impl fmt::Display for Error { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) } - } - - // bitcoinconsensus::Error has no sources at this time - impl std::error::Error for Error {} - - pub(crate) fn wrap_error(error: &bitcoinconsensus::Error) -> &Error { - // Unfortunately, we cannot have the reference inside `Error` struct because of the 'static - // bound on `source` return type, so we have to use unsafe to overcome the limitation. - // SAFETY: the type is repr(transparent) and the lifetimes match - unsafe { &*(error as *const _ as *const Error) } - } -} - -#[cfg(not(all( - feature = "std", - feature = "bitcoinconsensus", - not(feature = "bitcoinconsensus-std") -)))] -mod bitcoinconsensus_hack { - #[allow(unused_imports)] // conditionally used - pub(crate) use core::convert::identity as wrap_error; -} - impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - #[cfg(feature = "bitcoinconsensus")] - use internals::write_err; - match *self { Error::NonMinimalPush => f.write_str("non-minimal datapush"), Error::EarlyEndOfScript => f.write_str("unexpected end of script"), Error::NumericOverflow => f.write_str("numeric overflow (number on stack larger than 4 bytes)"), - #[cfg(feature = "bitcoinconsensus")] - Error::BitcoinConsensus(ref e) => - write_err!(f, "bitcoinconsensus verification failed"; bitcoinconsensus_hack::wrap_error(e)), Error::UnknownSpentOutput(ref point) => write!(f, "unknown spent output: {}", point), Error::Serialization => f.write_str("can not serialize the spending transaction in Transaction::verify()"), @@ -758,8 +711,6 @@ impl std::error::Error for Error { | NumericOverflow | UnknownSpentOutput(_) | Serialization => None, - #[cfg(feature = "bitcoinconsensus")] - BitcoinConsensus(ref e) => Some(bitcoinconsensus_hack::wrap_error(e)), } } } @@ -779,8 +730,3 @@ impl From for Error { } } } - -#[cfg(feature = "bitcoinconsensus")] -impl From for Error { - fn from(err: bitcoinconsensus::Error) -> Error { Error::BitcoinConsensus(err) } -} diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index 8418fbe6..dad96ccc 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -21,8 +21,6 @@ use internals::write_err; use super::Weight; use crate::blockdata::locktime::absolute::{self, Height, Time}; use crate::blockdata::locktime::relative; -#[cfg(feature = "bitcoinconsensus")] -use crate::blockdata::script; use crate::blockdata::script::{Script, ScriptBuf}; use crate::blockdata::witness::Witness; use crate::consensus::{encode, Decodable, Encodable}; @@ -931,7 +929,7 @@ impl Transaction { /// Shorthand for [`Self::verify_with_flags`] with flag [`bitcoinconsensus::VERIFY_ALL`]. #[cfg(feature = "bitcoinconsensus")] - pub fn verify(&self, spent: S) -> Result<(), script::Error> + pub fn verify(&self, spent: S) -> Result<(), TxVerifyError> where S: FnMut(&OutPoint) -> Option, { @@ -942,7 +940,7 @@ impl Transaction { /// /// The `spent` closure should not return the same [`TxOut`] twice! #[cfg(feature = "bitcoinconsensus")] - pub fn verify_with_flags(&self, mut spent: S, flags: F) -> Result<(), script::Error> + pub fn verify_with_flags(&self, mut spent: S, flags: F) -> Result<(), TxVerifyError> where S: FnMut(&OutPoint) -> Option, F: Into, @@ -953,7 +951,7 @@ impl Transaction { if let Some(output) = spent(&input.previous_output) { output.script_pubkey.verify_with_flags(idx, output.value, tx.as_slice(), flags)?; } else { - return Err(script::Error::UnknownSpentOutput(input.previous_output)); + return Err(TxVerifyError::UnknownSpentOutput(input.previous_output)); } } Ok(()) @@ -1416,6 +1414,85 @@ impl InputWeightPrediction { } } +/// An error during transaction validation. +#[cfg(feature = "bitcoinconsensus")] +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum TxVerifyError { + /// Error validating the script with bitcoinconsensus library. + ScriptVerification(bitcoinconsensus::Error), + /// Can not find the spent output. + UnknownSpentOutput(OutPoint), +} + +#[cfg(feature = "bitcoinconsensus")] +impl fmt::Display for TxVerifyError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use TxVerifyError::*; + + match *self { + ScriptVerification(ref e) => { + write_err!(f, "bitcoinconsensus verification failed"; bitcoinconsensus_hack::wrap_error(e)) + } + UnknownSpentOutput(ref p) => write!(f, "unknown spent output: {}", p), + } + } +} + +#[cfg(all(feature = "std", feature = "bitcoinconsensus"))] +impl std::error::Error for TxVerifyError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use TxVerifyError::*; + + match *self { + ScriptVerification(ref e) => Some(bitcoinconsensus_hack::wrap_error(e)), + UnknownSpentOutput(_) => None, + } + } +} + +#[cfg(feature = "bitcoinconsensus")] +impl From for TxVerifyError { + fn from(e: bitcoinconsensus::Error) -> Self { TxVerifyError::ScriptVerification(e) } +} + +// If bitcoinonsensus-std is off but bitcoinconsensus is present we patch the error type to +// implement `std::error::Error`. +#[cfg(all(feature = "std", feature = "bitcoinconsensus", not(feature = "bitcoinconsensus-std")))] +mod bitcoinconsensus_hack { + use core::fmt; + + #[repr(transparent)] + pub(crate) struct Error(bitcoinconsensus::Error); + + impl fmt::Debug for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Debug::fmt(&self.0, f) } + } + + impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) } + } + + // bitcoinconsensus::Error has no sources at this time + impl std::error::Error for Error {} + + pub(crate) fn wrap_error(error: &bitcoinconsensus::Error) -> &Error { + // Unfortunately, we cannot have the reference inside `Error` struct because of the 'static + // bound on `source` return type, so we have to use unsafe to overcome the limitation. + // SAFETY: the type is repr(transparent) and the lifetimes match + unsafe { &*(error as *const _ as *const Error) } + } +} + +#[cfg(not(all( + feature = "std", + feature = "bitcoinconsensus", + not(feature = "bitcoinconsensus-std") +)))] +mod bitcoinconsensus_hack { + #[allow(unused_imports)] // conditionally used + pub(crate) use core::convert::identity as wrap_error; +} + #[cfg(test)] mod tests { use core::str::FromStr; @@ -1838,7 +1915,6 @@ mod tests { fn test_transaction_verify() { use std::collections::HashMap; - use crate::blockdata::script; use crate::blockdata::witness::Witness; // a random recent segwit transaction from blockchain using both old and segwit inputs @@ -1895,7 +1971,7 @@ mod tests { .err() .unwrap() { - script::Error::BitcoinConsensus(_) => {} + TxVerifyError::ScriptVerification(_) => {} _ => panic!("Wrong error type"), } }