From a6254212dc90fc07ee2ae1119efd386225d497f4 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 18 Oct 2024 09:55:00 +1100 Subject: [PATCH 1/8] Move consensus error code to submodule The `consensus` module has a bunch of error types, move them all to a separate module. Add re-exports so the types are still available at the same place they were. Make the `error` module private and re-export all errors from the `consensus` module root. --- bitcoin/src/consensus/encode.rs | 118 ++---------------------- bitcoin/src/consensus/error.rs | 157 ++++++++++++++++++++++++++++++++ bitcoin/src/consensus/mod.rs | 42 +-------- 3 files changed, 167 insertions(+), 150 deletions(-) create mode 100644 bitcoin/src/consensus/error.rs diff --git a/bitcoin/src/consensus/encode.rs b/bitcoin/src/consensus/encode.rs index c4dad4f76..cf427199f 100644 --- a/bitcoin/src/consensus/encode.rs +++ b/bitcoin/src/consensus/encode.rs @@ -14,132 +14,29 @@ //! scripts come with an opcode decode, hashes are big-endian, numbers are //! typically big-endian decimals, etc.) -use core::{fmt, mem}; +use core::mem; use hashes::{sha256, sha256d, GeneralHash, Hash}; -use hex::error::{InvalidCharError, OddLengthStringError}; -use internals::{compact_size, write_err, ToU64}; +use hex::DisplayHex as _; +use internals::{compact_size, ToU64}; use io::{BufRead, Cursor, Read, Write}; +use super::IterReader; use crate::bip152::{PrefilledTransaction, ShortId}; use crate::bip158::{FilterHash, FilterHeader}; use crate::block::{self, BlockHash}; -use crate::consensus::{DecodeError, IterReader}; use crate::merkle_tree::TxMerkleNode; #[cfg(feature = "std")] use crate::p2p::{ address::{AddrV2Message, Address}, message_blockdata::Inventory, }; -use crate::prelude::{rc, sync, Box, Cow, DisplayHex, String, Vec}; +use crate::prelude::{rc, sync, Box, Cow, String, Vec}; use crate::taproot::TapLeafHash; use crate::transaction::{Transaction, TxIn, TxOut}; -/// Encoding error. -#[derive(Debug)] -#[non_exhaustive] -pub enum Error { - /// And I/O error. - Io(io::Error), - /// Tried to allocate an oversized vector. - OversizedVectorAllocation { - /// The capacity requested. - requested: usize, - /// The maximum capacity. - max: usize, - }, - /// Checksum was invalid. - InvalidChecksum { - /// The expected checksum. - expected: [u8; 4], - /// The invalid checksum. - actual: [u8; 4], - }, - /// VarInt was encoded in a non-minimal way. - NonMinimalVarInt, - /// Parsing error. - ParseFailed(&'static str), - /// Unsupported Segwit flag. - UnsupportedSegwitFlag(u8), -} - -internals::impl_from_infallible!(Error); - -impl fmt::Display for Error { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - use Error::*; - - match *self { - Io(ref e) => write_err!(f, "IO error"; e), - OversizedVectorAllocation { requested: ref r, max: ref m } => - write!(f, "allocation of oversized vector: requested {}, maximum {}", r, m), - InvalidChecksum { expected: ref e, actual: ref a } => - write!(f, "invalid checksum: expected {:x}, actual {:x}", e.as_hex(), a.as_hex()), - NonMinimalVarInt => write!(f, "non-minimal varint"), - ParseFailed(ref s) => write!(f, "parse failed: {}", s), - UnsupportedSegwitFlag(ref swflag) => - write!(f, "unsupported segwit version: {}", swflag), - } - } -} - -#[cfg(feature = "std")] -impl std::error::Error for Error { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - use Error::*; - - match self { - Io(e) => Some(e), - OversizedVectorAllocation { .. } - | InvalidChecksum { .. } - | NonMinimalVarInt - | ParseFailed(_) - | UnsupportedSegwitFlag(_) => None, - } - } -} - -impl From for Error { - fn from(error: io::Error) -> Self { Error::Io(error) } -} - -/// Hex deserialization error. -#[derive(Debug)] -pub enum FromHexError { - /// Purported hex string had odd length. - OddLengthString(OddLengthStringError), - /// Decoding error. - Decode(DecodeError), -} - -impl fmt::Display for FromHexError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - use FromHexError::*; - - match *self { - OddLengthString(ref e) => - write_err!(f, "odd length, failed to create bytes from hex"; e), - Decode(ref e) => write_err!(f, "decoding error"; e), - } - } -} - -#[cfg(feature = "std")] -impl std::error::Error for FromHexError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - use FromHexError::*; - - match *self { - OddLengthString(ref e) => Some(e), - Decode(ref e) => Some(e), - } - } -} - -impl From for FromHexError { - #[inline] - fn from(e: OddLengthStringError) -> Self { Self::OddLengthString(e) } -} +#[rustfmt::skip] // Keep public re-exports separate. +pub use super::{Error, FromHexError}; /// Encodes an object into a vector. pub fn serialize(data: &T) -> Vec { @@ -859,6 +756,7 @@ impl Decodable for TapLeafHash { #[cfg(test)] mod tests { + use core::fmt; use core::mem::discriminant; use super::*; diff --git a/bitcoin/src/consensus/error.rs b/bitcoin/src/consensus/error.rs new file mode 100644 index 000000000..181b8d310 --- /dev/null +++ b/bitcoin/src/consensus/error.rs @@ -0,0 +1,157 @@ +// SPDX-License-Identifier: CC0-1.0 + +//! Consensus encoding errors. + +use core::fmt; + +use hex::error::{InvalidCharError, OddLengthStringError}; +use hex::DisplayHex as _; +use internals::write_err; + +#[cfg(doc)] +use super::IterReader; + +/// Error when consensus decoding from an `[IterReader]`. +#[derive(Debug)] +pub enum DecodeError { + /// Attempted to decode an object from an iterator that yielded too many bytes. + TooManyBytes, + /// Invalid consensus encoding. + Consensus(Error), + /// Other decoding error. + Other(E), +} + +internals::impl_from_infallible!(DecodeError); + +impl fmt::Display for DecodeError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use DecodeError::*; + + match *self { + TooManyBytes => + write!(f, "attempted to decode object from an iterator that yielded too many bytes"), + Consensus(ref e) => write_err!(f, "invalid consensus encoding"; e), + Other(ref other) => write!(f, "other decoding error: {:?}", other), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for DecodeError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use DecodeError::*; + + match *self { + TooManyBytes => None, + Consensus(ref e) => Some(e), + Other(_) => None, // TODO: Is this correct? + } + } +} + +/// Encoding error. +#[derive(Debug)] +#[non_exhaustive] +pub enum Error { + /// And I/O error. + Io(io::Error), + /// Tried to allocate an oversized vector. + OversizedVectorAllocation { + /// The capacity requested. + requested: usize, + /// The maximum capacity. + max: usize, + }, + /// Checksum was invalid. + InvalidChecksum { + /// The expected checksum. + expected: [u8; 4], + /// The invalid checksum. + actual: [u8; 4], + }, + /// VarInt was encoded in a non-minimal way. + NonMinimalVarInt, + /// Parsing error. + ParseFailed(&'static str), + /// Unsupported Segwit flag. + UnsupportedSegwitFlag(u8), +} + +internals::impl_from_infallible!(Error); + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use Error::*; + + match *self { + Io(ref e) => write_err!(f, "IO error"; e), + OversizedVectorAllocation { requested: ref r, max: ref m } => + write!(f, "allocation of oversized vector: requested {}, maximum {}", r, m), + InvalidChecksum { expected: ref e, actual: ref a } => + write!(f, "invalid checksum: expected {:x}, actual {:x}", e.as_hex(), a.as_hex()), + NonMinimalVarInt => write!(f, "non-minimal varint"), + ParseFailed(ref s) => write!(f, "parse failed: {}", s), + UnsupportedSegwitFlag(ref swflag) => + write!(f, "unsupported segwit version: {}", swflag), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use Error::*; + + match self { + Io(e) => Some(e), + OversizedVectorAllocation { .. } + | InvalidChecksum { .. } + | NonMinimalVarInt + | ParseFailed(_) + | UnsupportedSegwitFlag(_) => None, + } + } +} + +impl From for Error { + fn from(error: io::Error) -> Self { Error::Io(error) } +} + +/// Hex deserialization error. +#[derive(Debug)] +pub enum FromHexError { + /// Purported hex string had odd length. + OddLengthString(OddLengthStringError), + /// Decoding error. + Decode(DecodeError), +} + +impl fmt::Display for FromHexError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use FromHexError::*; + + match *self { + OddLengthString(ref e) => + write_err!(f, "odd length, failed to create bytes from hex"; e), + Decode(ref e) => write_err!(f, "decoding error"; e), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for FromHexError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use FromHexError::*; + + match *self { + OddLengthString(ref e) => Some(e), + Decode(ref e) => Some(e), + } + } +} + +impl From for FromHexError { + #[inline] + fn from(e: OddLengthStringError) -> Self { Self::OddLengthString(e) } +} diff --git a/bitcoin/src/consensus/mod.rs b/bitcoin/src/consensus/mod.rs index 1150af014..a12b22e13 100644 --- a/bitcoin/src/consensus/mod.rs +++ b/bitcoin/src/consensus/mod.rs @@ -6,12 +6,12 @@ //! conform to Bitcoin consensus. pub mod encode; +mod error; #[cfg(feature = "serde")] pub mod serde; use core::fmt; -use internals::write_err; use io::{BufRead, Read}; use crate::consensus; @@ -20,6 +20,7 @@ use crate::consensus; #[doc(inline)] pub use self::{ encode::{deserialize, deserialize_partial, serialize, Decodable, Encodable, ReadExt, WriteExt}, + error::{Error, FromHexError, DecodeError}, }; struct IterReader>> { @@ -102,42 +103,3 @@ impl>> BufRead for IterReader { - /// Attempted to decode an object from an iterator that yielded too many bytes. - TooManyBytes, - /// Invalid consensus encoding. - Consensus(consensus::encode::Error), - /// Other decoding error. - Other(E), -} - -internals::impl_from_infallible!(DecodeError); - -impl fmt::Display for DecodeError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - use DecodeError::*; - - match *self { - TooManyBytes => - write!(f, "attempted to decode object from an iterator that yielded too many bytes"), - Consensus(ref e) => write_err!(f, "invalid consensus encoding"; e), - Other(ref other) => write!(f, "other decoding error: {:?}", other), - } - } -} - -#[cfg(feature = "std")] -impl std::error::Error for DecodeError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - use DecodeError::*; - - match *self { - TooManyBytes => None, - Consensus(ref e) => Some(e), - Other(_) => None, // TODO: Is this correct? - } - } -} From ebfef3f114c459f3767ab594f4bc0870388ccd8b Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 18 Oct 2024 09:58:41 +1100 Subject: [PATCH 2/8] Return generic error as Some The `std::error::Error` impl for `consensus::DecodeError` should return the inner generic error. --- bitcoin/src/consensus/error.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bitcoin/src/consensus/error.rs b/bitcoin/src/consensus/error.rs index 181b8d310..e1e0e2d1a 100644 --- a/bitcoin/src/consensus/error.rs +++ b/bitcoin/src/consensus/error.rs @@ -38,14 +38,14 @@ impl fmt::Display for DecodeError { } #[cfg(feature = "std")] -impl std::error::Error for DecodeError { +impl std::error::Error for DecodeError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { use DecodeError::*; match *self { TooManyBytes => None, Consensus(ref e) => Some(e), - Other(_) => None, // TODO: Is this correct? + Other(ref e) => Some(e), } } } From efd7f9f06cf9156517bef775545da713d982945a Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 18 Oct 2024 10:38:50 +1100 Subject: [PATCH 3/8] Add error constructor parse_failed_error The `encode::Error::ParseFailed` variant holds an inner string, this is suboptimal. In an effort to patch the `encode::Error` while mimizing the diffs required add a helper function that creates the variant. The benefit is that later patches that effect this variant will only need to update the constructor function and not every call site. Internal change only. --- bitcoin/src/bip152.rs | 15 ++++++++------- bitcoin/src/blockdata/transaction.rs | 6 ++++-- bitcoin/src/consensus/encode.rs | 6 +++--- bitcoin/src/consensus/error.rs | 4 ++++ bitcoin/src/consensus/mod.rs | 1 + bitcoin/src/p2p/address.rs | 21 +++++++++++---------- bitcoin/src/p2p/message.rs | 4 ++-- bitcoin/src/p2p/message_bloom.rs | 4 ++-- bitcoin/src/p2p/message_network.rs | 4 ++-- 9 files changed, 37 insertions(+), 28 deletions(-) diff --git a/bitcoin/src/bip152.rs b/bitcoin/src/bip152.rs index 96ffcdc79..c3ccee11c 100644 --- a/bitcoin/src/bip152.rs +++ b/bitcoin/src/bip152.rs @@ -16,7 +16,7 @@ use crate::consensus::encode::{self, Decodable, Encodable, ReadExt, WriteExt}; use crate::internal_macros::{impl_array_newtype_stringify, impl_consensus_encoding}; use crate::prelude::Vec; use crate::transaction::TxIdentifier; -use crate::{block, Block, BlockHash, Transaction}; +use crate::{block, consensus, Block, BlockHash, Transaction}; /// A BIP-152 error #[derive(Debug, Clone, PartialEq, Eq)] @@ -84,8 +84,9 @@ impl Decodable for PrefilledTransaction { #[inline] fn consensus_decode(r: &mut R) -> Result { let idx = r.read_compact_size()?; - let idx = u16::try_from(idx) - .map_err(|_| encode::Error::ParseFailed("BIP152 prefilled tx index out of bounds"))?; + let idx = u16::try_from(idx).map_err(|_| { + consensus::parse_failed_error("BIP152 prefilled tx index out of bounds") + })?; let tx = Transaction::consensus_decode(r)?; Ok(PrefilledTransaction { idx, tx }) } @@ -172,7 +173,7 @@ impl Decodable for HeaderAndShortIds { }; match header_short_ids.short_ids.len().checked_add(header_short_ids.prefilled_txs.len()) { Some(x) if x <= u16::MAX.into() => Ok(header_short_ids), - _ => Err(encode::Error::ParseFailed("indexes overflowed 16 bits")), + _ => Err(consensus::parse_failed_error("indexes overflowed 16 bits")), } } } @@ -312,7 +313,7 @@ impl Decodable for BlockTransactionsRequest { // transactions that would be allowed in a vector. let byte_size = nb_indexes .checked_mul(mem::size_of::()) - .ok_or(encode::Error::ParseFailed("invalid length"))?; + .ok_or(consensus::parse_failed_error("invalid length"))?; if byte_size > encode::MAX_VEC_SIZE { return Err(encode::Error::OversizedVectorAllocation { requested: byte_size, @@ -326,12 +327,12 @@ impl Decodable for BlockTransactionsRequest { let differential = r.read_compact_size()?; last_index = match last_index.checked_add(differential) { Some(i) => i, - None => return Err(encode::Error::ParseFailed("block index overflow")), + None => return Err(consensus::parse_failed_error("block index overflow")), }; indexes.push(last_index); last_index = match last_index.checked_add(1) { Some(i) => i, - None => return Err(encode::Error::ParseFailed("block index overflow")), + None => return Err(consensus::parse_failed_error("block index overflow")), }; } indexes diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index 469a9a70c..009605baf 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -20,7 +20,7 @@ use io::{BufRead, Write}; use primitives::Sequence; use super::Weight; -use crate::consensus::{encode, Decodable, Encodable}; +use crate::consensus::{self, encode, Decodable, Encodable}; use crate::internal_macros::{impl_consensus_encoding, impl_hashencode}; use crate::locktime::absolute::{self, Height, Time}; use crate::prelude::{Borrow, Vec}; @@ -900,7 +900,9 @@ impl Decodable for Transaction { txin.witness = Decodable::consensus_decode_from_finite_reader(r)?; } if !input.is_empty() && input.iter().all(|input| input.witness.is_empty()) { - Err(encode::Error::ParseFailed("witness flag set but no witnesses present")) + Err(consensus::parse_failed_error( + "witness flag set but no witnesses present", + )) } else { Ok(Transaction { version, diff --git a/bitcoin/src/consensus/encode.rs b/bitcoin/src/consensus/encode.rs index cf427199f..eb89dfe11 100644 --- a/bitcoin/src/consensus/encode.rs +++ b/bitcoin/src/consensus/encode.rs @@ -60,7 +60,7 @@ pub fn deserialize(data: &[u8]) -> Result { if consumed == data.len() { Ok(rv) } else { - Err(Error::ParseFailed("data not consumed entirely when explicitly deserializing")) + Err(super::parse_failed_error("data not consumed entirely when explicitly deserializing")) } } @@ -418,7 +418,7 @@ impl Decodable for String { #[inline] fn consensus_decode(r: &mut R) -> Result { String::from_utf8(Decodable::consensus_decode(r)?) - .map_err(|_| self::Error::ParseFailed("String was not valid UTF8")) + .map_err(|_| super::parse_failed_error("String was not valid UTF8")) } } @@ -433,7 +433,7 @@ impl Decodable for Cow<'static, str> { #[inline] fn consensus_decode(r: &mut R) -> Result, Error> { String::from_utf8(Decodable::consensus_decode(r)?) - .map_err(|_| self::Error::ParseFailed("String was not valid UTF8")) + .map_err(|_| super::parse_failed_error("String was not valid UTF8")) .map(Cow::Owned) } } diff --git a/bitcoin/src/consensus/error.rs b/bitcoin/src/consensus/error.rs index e1e0e2d1a..e74c54ec8 100644 --- a/bitcoin/src/consensus/error.rs +++ b/bitcoin/src/consensus/error.rs @@ -155,3 +155,7 @@ impl From for FromHexError { #[inline] fn from(e: OddLengthStringError) -> Self { Self::OddLengthString(e) } } + +/// Constructs a `Error::ParseFailed` error. +// This whole variant should go away because of the inner string. +pub(crate) fn parse_failed_error(msg: &'static str) -> Error { Error::ParseFailed(msg) } diff --git a/bitcoin/src/consensus/mod.rs b/bitcoin/src/consensus/mod.rs index a12b22e13..8a43ae329 100644 --- a/bitcoin/src/consensus/mod.rs +++ b/bitcoin/src/consensus/mod.rs @@ -22,6 +22,7 @@ pub use self::{ encode::{deserialize, deserialize_partial, serialize, Decodable, Encodable, ReadExt, WriteExt}, error::{Error, FromHexError, DecodeError}, }; +pub(crate) use self::error::parse_failed_error; struct IterReader>> { iterator: core::iter::Fuse, diff --git a/bitcoin/src/p2p/address.rs b/bitcoin/src/p2p/address.rs index a531618e0..d90992001 100644 --- a/bitcoin/src/p2p/address.rs +++ b/bitcoin/src/p2p/address.rs @@ -10,6 +10,7 @@ use std::net::{Ipv4Addr, Ipv6Addr, SocketAddr, SocketAddrV4, SocketAddrV6, ToSoc use io::{BufRead, Read, Write}; +use crate::consensus; use crate::consensus::encode::{self, Decodable, Encodable, ReadExt, WriteExt}; use crate::p2p::ServiceFlags; @@ -165,28 +166,28 @@ impl Decodable for AddrV2 { let network_id = u8::consensus_decode(r)?; let len = r.read_compact_size()?; if len > 512 { - return Err(encode::Error::ParseFailed("IP must be <= 512 bytes")); + return Err(consensus::parse_failed_error("IP must be <= 512 bytes")); } Ok(match network_id { 1 => { if len != 4 { - return Err(encode::Error::ParseFailed("invalid IPv4 address")); + return Err(consensus::parse_failed_error("invalid IPv4 address")); } let addr: [u8; 4] = Decodable::consensus_decode(r)?; AddrV2::Ipv4(Ipv4Addr::new(addr[0], addr[1], addr[2], addr[3])) } 2 => { if len != 16 { - return Err(encode::Error::ParseFailed("invalid IPv6 address")); + return Err(consensus::parse_failed_error("invalid IPv6 address")); } let addr: [u16; 8] = read_be_address(r)?; if addr[0..3] == ONION { - return Err(encode::Error::ParseFailed( + return Err(consensus::parse_failed_error( "OnionCat address sent with IPv6 network id", )); } if addr[0..6] == [0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0xFFFF] { - return Err(encode::Error::ParseFailed( + return Err(consensus::parse_failed_error( "IPV4 wrapped address sent with IPv6 network id", )); } @@ -196,33 +197,33 @@ impl Decodable for AddrV2 { } 3 => { if len != 10 { - return Err(encode::Error::ParseFailed("invalid TorV2 address")); + return Err(consensus::parse_failed_error("invalid TorV2 address")); } let id = Decodable::consensus_decode(r)?; AddrV2::TorV2(id) } 4 => { if len != 32 { - return Err(encode::Error::ParseFailed("invalid TorV3 address")); + return Err(consensus::parse_failed_error("invalid TorV3 address")); } let pubkey = Decodable::consensus_decode(r)?; AddrV2::TorV3(pubkey) } 5 => { if len != 32 { - return Err(encode::Error::ParseFailed("invalid I2P address")); + return Err(consensus::parse_failed_error("invalid I2P address")); } let hash = Decodable::consensus_decode(r)?; AddrV2::I2p(hash) } 6 => { if len != 16 { - return Err(encode::Error::ParseFailed("invalid CJDNS address")); + return Err(consensus::parse_failed_error("invalid CJDNS address")); } let addr: [u16; 8] = read_be_address(r)?; // check the first byte for the CJDNS marker if addr[0] >> 8 != 0xFC { - return Err(encode::Error::ParseFailed("invalid CJDNS address")); + return Err(consensus::parse_failed_error("invalid CJDNS address")); } AddrV2::Cjdns(Ipv6Addr::new( addr[0], addr[1], addr[2], addr[3], addr[4], addr[5], addr[6], addr[7], diff --git a/bitcoin/src/p2p/message.rs b/bitcoin/src/p2p/message.rs index 8776aa96a..9ae2aa0a2 100644 --- a/bitcoin/src/p2p/message.rs +++ b/bitcoin/src/p2p/message.rs @@ -19,7 +19,7 @@ use crate::p2p::{ Magic, }; use crate::prelude::{Box, Cow, String, ToOwned, Vec}; -use crate::{block, transaction}; +use crate::{block, consensus, transaction}; /// The maximum number of [super::message_blockdata::Inventory] items in an `inv` message. /// @@ -418,7 +418,7 @@ impl Decodable for HeaderDeserializationWrapper { for _ in 0..len { ret.push(Decodable::consensus_decode(r)?); if u8::consensus_decode(r)? != 0u8 { - return Err(encode::Error::ParseFailed( + return Err(consensus::parse_failed_error( "Headers message should not contain transactions", )); } diff --git a/bitcoin/src/p2p/message_bloom.rs b/bitcoin/src/p2p/message_bloom.rs index 1e6b5e235..2a75f9a64 100644 --- a/bitcoin/src/p2p/message_bloom.rs +++ b/bitcoin/src/p2p/message_bloom.rs @@ -6,7 +6,7 @@ use io::{BufRead, Write}; -use crate::consensus::{encode, Decodable, Encodable, ReadExt}; +use crate::consensus::{self, encode, Decodable, Encodable, ReadExt}; use crate::internal_macros::impl_consensus_encoding; /// `filterload` message sets the current bloom filter @@ -52,7 +52,7 @@ impl Decodable for BloomFlags { 0 => BloomFlags::None, 1 => BloomFlags::All, 2 => BloomFlags::PubkeyOnly, - _ => return Err(encode::Error::ParseFailed("unknown bloom flag")), + _ => return Err(consensus::parse_failed_error("unknown bloom flag")), }) } } diff --git a/bitcoin/src/p2p/message_network.rs b/bitcoin/src/p2p/message_network.rs index 470a07dbb..942578148 100644 --- a/bitcoin/src/p2p/message_network.rs +++ b/bitcoin/src/p2p/message_network.rs @@ -8,7 +8,7 @@ use hashes::sha256d; use io::{BufRead, Write}; -use crate::consensus::{encode, Decodable, Encodable, ReadExt}; +use crate::consensus::{self, encode, Decodable, Encodable, ReadExt}; use crate::internal_macros::impl_consensus_encoding; use crate::p2p; use crate::p2p::address::Address; @@ -126,7 +126,7 @@ impl Decodable for RejectReason { 0x41 => RejectReason::Dust, 0x42 => RejectReason::Fee, 0x43 => RejectReason::Checkpoint, - _ => return Err(encode::Error::ParseFailed("unknown reject code")), + _ => return Err(consensus::parse_failed_error("unknown reject code")), }) } } From 5a42ef28507e365ef7c29096bfe794e063791979 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 18 Oct 2024 11:54:45 +1100 Subject: [PATCH 4/8] Do not manually map IO error We have a `From` impl for IO error but we are manually mapping. Done in preparation for patching the `From` impl. --- bitcoin/src/consensus/encode.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/bitcoin/src/consensus/encode.rs b/bitcoin/src/consensus/encode.rs index eb89dfe11..c64868ffe 100644 --- a/bitcoin/src/consensus/encode.rs +++ b/bitcoin/src/consensus/encode.rs @@ -160,7 +160,7 @@ macro_rules! decoder_fn { #[inline] fn $name(&mut self) -> core::result::Result<$val_type, Error> { let mut val = [0; $byte_len]; - self.read_exact(&mut val[..]).map_err(Error::Io)?; + self.read_exact(&mut val[..])?; Ok(<$val_type>::from_le_bytes(val)) } }; @@ -216,9 +216,7 @@ impl ReadExt for R { #[inline] fn read_bool(&mut self) -> Result { ReadExt::read_i8(self).map(|bit| bit != 0) } #[inline] - fn read_slice(&mut self, slice: &mut [u8]) -> Result<(), Error> { - self.read_exact(slice).map_err(Error::Io) - } + fn read_slice(&mut self, slice: &mut [u8]) -> Result<(), Error> { Ok(self.read_exact(slice)?) } #[inline] #[rustfmt::skip] // Formatter munges code comments below. fn read_compact_size(&mut self) -> Result { From b04142c7459853e4e550224ad52d00f43709babb Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 18 Oct 2024 10:11:54 +1100 Subject: [PATCH 5/8] Add encode::Error::MissingData variant The `io::Error` is troublesome because it contains a bunch of stuff that never happens when reading from a buffer. However the EOF variant can occur if the buffer is too short. As an initial step towards reducing usage of the `io::Error` add a `MissingData` variant to the `encode::Error` and when converting from an IO error map to `MissingData` if EOF is encountered. --- bitcoin/src/consensus/encode.rs | 9 +++------ bitcoin/src/consensus/error.rs | 15 +++++++++++++-- bitcoin/src/consensus/serde.rs | 2 ++ 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/bitcoin/src/consensus/encode.rs b/bitcoin/src/consensus/encode.rs index c64868ffe..e5aa9fdf9 100644 --- a/bitcoin/src/consensus/encode.rs +++ b/bitcoin/src/consensus/encode.rs @@ -1027,13 +1027,11 @@ mod tests { ]) .is_err()); - let rand_io_err = Error::Io(io::Error::new(io::ErrorKind::Other, "")); - // Check serialization that `if len > MAX_VEC_SIZE {return err}` isn't inclusive, - // by making sure it fails with IO Error and not an `OversizedVectorAllocation` Error. + // by making sure it fails with `MissingData` and not an `OversizedVectorAllocation` Error. let err = deserialize::(&serialize(&(super::MAX_VEC_SIZE as u32))).unwrap_err(); - assert_eq!(discriminant(&err), discriminant(&rand_io_err)); + assert!(matches!(err, Error::MissingData)); test_len_is_max_vec::(); test_len_is_max_vec::(); @@ -1055,11 +1053,10 @@ mod tests { Vec: Decodable, T: fmt::Debug, { - let rand_io_err = Error::Io(io::Error::new(io::ErrorKind::Other, "")); let mut buf = Vec::new(); buf.emit_compact_size(super::MAX_VEC_SIZE / mem::size_of::()).unwrap(); let err = deserialize::>(&buf).unwrap_err(); - assert_eq!(discriminant(&err), discriminant(&rand_io_err)); + assert!(matches!(err, Error::MissingData)); } #[test] diff --git a/bitcoin/src/consensus/error.rs b/bitcoin/src/consensus/error.rs index e74c54ec8..d4f16fe1e 100644 --- a/bitcoin/src/consensus/error.rs +++ b/bitcoin/src/consensus/error.rs @@ -56,6 +56,8 @@ impl std::error::Error for DecodeEr pub enum Error { /// And I/O error. Io(io::Error), + /// Missing data (early end of file or slice too short). + MissingData, // TODO: Can we add more context? /// Tried to allocate an oversized vector. OversizedVectorAllocation { /// The capacity requested. @@ -86,6 +88,7 @@ impl fmt::Display for Error { match *self { Io(ref e) => write_err!(f, "IO error"; e), + MissingData => write!(f, "missing data (early end of file or slice too short)"), OversizedVectorAllocation { requested: ref r, max: ref m } => write!(f, "allocation of oversized vector: requested {}, maximum {}", r, m), InvalidChecksum { expected: ref e, actual: ref a } => @@ -105,7 +108,8 @@ impl std::error::Error for Error { match self { Io(e) => Some(e), - OversizedVectorAllocation { .. } + MissingData + | OversizedVectorAllocation { .. } | InvalidChecksum { .. } | NonMinimalVarInt | ParseFailed(_) @@ -115,7 +119,14 @@ impl std::error::Error for Error { } impl From for Error { - fn from(error: io::Error) -> Self { Error::Io(error) } + fn from(e: io::Error) -> Self { + use io::ErrorKind; + + match e.kind() { + ErrorKind::UnexpectedEof => Error::MissingData, + _ => Error::Io(e), + } + } } /// Hex deserialization error. diff --git a/bitcoin/src/consensus/serde.rs b/bitcoin/src/consensus/serde.rs index 790dd5781..f96363f18 100644 --- a/bitcoin/src/consensus/serde.rs +++ b/bitcoin/src/consensus/serde.rs @@ -361,6 +361,8 @@ impl serde::de::Expected for DisplayExpected { fn consensus_error_into_serde(error: ConsensusError) -> E { match error { ConsensusError::Io(error) => panic!("unexpected IO error {:?}", error), + ConsensusError::MissingData => + E::custom("missing data (early end of file or slice too short)"), ConsensusError::OversizedVectorAllocation { requested, max } => E::custom(format_args!( "the requested allocation of {} items exceeds maximum of {}", requested, max From 33566ac58c347f51bcb922e71a9a71dca7d916a0 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 18 Oct 2024 12:08:27 +1100 Subject: [PATCH 6/8] Split encode::Error into two parts The `consensus::encode::Error` contains an IO error but reading from a buffer only ever errors for EOF. We converted all instances of EOF to `MissingData` already so now we can split the IO error apart from the actual encoding errors variants. --- bitcoin/src/bip152.rs | 5 +- bitcoin/src/blockdata/transaction.rs | 4 +- bitcoin/src/blockdata/witness.rs | 30 ++++++----- bitcoin/src/consensus/encode.rs | 42 +++++++++------- bitcoin/src/consensus/error.rs | 75 +++++++++++++++++++++------- bitcoin/src/consensus/mod.rs | 2 +- bitcoin/src/consensus/serde.rs | 26 +++++----- bitcoin/src/merkle_tree/block.rs | 5 +- bitcoin/src/psbt/raw.rs | 4 +- 9 files changed, 124 insertions(+), 69 deletions(-) diff --git a/bitcoin/src/bip152.rs b/bitcoin/src/bip152.rs index c3ccee11c..f7f79cf43 100644 --- a/bitcoin/src/bip152.rs +++ b/bitcoin/src/bip152.rs @@ -315,10 +315,11 @@ impl Decodable for BlockTransactionsRequest { .checked_mul(mem::size_of::()) .ok_or(consensus::parse_failed_error("invalid length"))?; if byte_size > encode::MAX_VEC_SIZE { - return Err(encode::Error::OversizedVectorAllocation { + return Err(encode::ParseError::OversizedVectorAllocation { requested: byte_size, max: encode::MAX_VEC_SIZE, - }); + } + .into()); } let mut indexes = Vec::with_capacity(nb_indexes); diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index 009605baf..8eabc3054 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -913,7 +913,7 @@ impl Decodable for Transaction { } } // We don't support anything else - x => Err(encode::Error::UnsupportedSegwitFlag(x)), + x => Err(encode::ParseError::UnsupportedSegwitFlag(x).into()), } // non-segwit } else { @@ -1486,7 +1486,7 @@ mod tests { let tx_bytes = hex!("0000fd000001021921212121212121212121f8b372b0239cc1dff600000000004f4f4f4f4f4f4f4f000000000000000000000000000000333732343133380d000000000000000000000000000000ff000000000009000dff000000000000000800000000000000000d"); let tx: Result = deserialize(&tx_bytes); assert!(tx.is_err()); - assert!(tx.unwrap_err().to_string().contains("witness flag set but no witnesses present")); + assert!(matches!(tx.unwrap_err(), crate::consensus::Error::Parse(_))); } #[test] diff --git a/bitcoin/src/blockdata/witness.rs b/bitcoin/src/blockdata/witness.rs index 08e39d483..d5ca14db0 100644 --- a/bitcoin/src/blockdata/witness.rs +++ b/bitcoin/src/blockdata/witness.rs @@ -26,10 +26,11 @@ impl Decodable for Witness { // Minimum size of witness element is 1 byte, so if the count is // greater than MAX_VEC_SIZE we must return an error. if witness_elements > MAX_VEC_SIZE { - return Err(self::Error::OversizedVectorAllocation { + return Err(encode::ParseError::OversizedVectorAllocation { requested: witness_elements, max: MAX_VEC_SIZE, - }); + } + .into()); } if witness_elements == 0 { Ok(Witness::default()) @@ -48,21 +49,26 @@ impl Decodable for Witness { let element_size_len = compact_size::encoded_size(element_size); let required_len = cursor .checked_add(element_size) - .ok_or(self::Error::OversizedVectorAllocation { - requested: usize::MAX, - max: MAX_VEC_SIZE, - })? + .ok_or(encode::Error::Parse( + encode::ParseError::OversizedVectorAllocation { + requested: usize::MAX, + max: MAX_VEC_SIZE, + }, + ))? .checked_add(element_size_len) - .ok_or(self::Error::OversizedVectorAllocation { - requested: usize::MAX, - max: MAX_VEC_SIZE, - })?; + .ok_or(encode::Error::Parse( + encode::ParseError::OversizedVectorAllocation { + requested: usize::MAX, + max: MAX_VEC_SIZE, + }, + ))?; if required_len > MAX_VEC_SIZE + witness_index_space { - return Err(self::Error::OversizedVectorAllocation { + return Err(encode::ParseError::OversizedVectorAllocation { requested: required_len, max: MAX_VEC_SIZE, - }); + } + .into()); } // We will do content.rotate_left(witness_index_space) later. diff --git a/bitcoin/src/consensus/encode.rs b/bitcoin/src/consensus/encode.rs index e5aa9fdf9..5b9902f01 100644 --- a/bitcoin/src/consensus/encode.rs +++ b/bitcoin/src/consensus/encode.rs @@ -36,7 +36,7 @@ use crate::taproot::TapLeafHash; use crate::transaction::{Transaction, TxIn, TxOut}; #[rustfmt::skip] // Keep public re-exports separate. -pub use super::{Error, FromHexError}; +pub use super::{Error, FromHexError, ParseError}; /// Encodes an object into a vector. pub fn serialize(data: &T) -> Vec { @@ -76,6 +76,7 @@ pub fn deserialize_hex(hex: &str) -> Result { /// doesn't consume the entire vector. pub fn deserialize_partial(data: &[u8]) -> Result<(T, usize), Error> { let mut decoder = Cursor::new(data); + let rv = Decodable::consensus_decode_from_finite_reader(&mut decoder)?; let consumed = decoder.position() as usize; @@ -224,7 +225,7 @@ impl ReadExt for R { 0xFF => { let x = self.read_u64()?; if x < 0x1_0000_0000 { // I.e., would have fit in a `u32`. - Err(Error::NonMinimalVarInt) + Err(ParseError::NonMinimalVarInt.into()) } else { Ok(x) } @@ -232,7 +233,7 @@ impl ReadExt for R { 0xFE => { let x = self.read_u32()?; if x < 0x1_0000 { // I.e., would have fit in a `u16`. - Err(Error::NonMinimalVarInt) + Err(ParseError::NonMinimalVarInt.into()) } else { Ok(x as u64) } @@ -240,7 +241,7 @@ impl ReadExt for R { 0xFD => { let x = self.read_u16()?; if x < 0xFD { // Could have been encoded as a `u8`. - Err(Error::NonMinimalVarInt) + Err(ParseError::NonMinimalVarInt.into()) } else { Ok(x as u64) } @@ -649,7 +650,8 @@ impl Decodable for CheckedData { let data = read_bytes_from_finite_reader(r, opts)?; let expected_checksum = sha2_checksum(&data); if expected_checksum != checksum { - Err(self::Error::InvalidChecksum { expected: expected_checksum, actual: checksum }) + Err(ParseError::InvalidChecksum { expected: expected_checksum, actual: checksum } + .into()) } else { Ok(CheckedData { data, checksum }) } @@ -858,50 +860,50 @@ mod tests { discriminant( &test_varint_encode(0xFF, &(0x100000000_u64 - 1).to_le_bytes()).unwrap_err() ), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&test_varint_encode(0xFE, &(0x10000_u64 - 1).to_le_bytes()).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&test_varint_encode(0xFD, &(0xFD_u64 - 1).to_le_bytes()).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&deserialize::>(&[0xfd, 0x00, 0x00]).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&deserialize::>(&[0xfd, 0xfc, 0x00]).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&deserialize::>(&[0xfd, 0xfc, 0x00]).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&deserialize::>(&[0xfe, 0xff, 0x00, 0x00, 0x00]).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&deserialize::>(&[0xfe, 0xff, 0xff, 0x00, 0x00]).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant( &deserialize::>(&[0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]) .unwrap_err() ), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant( &deserialize::>(&[0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00]) .unwrap_err() ), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); let mut vec_256 = vec![0; 259]; @@ -1031,7 +1033,10 @@ mod tests { // by making sure it fails with `MissingData` and not an `OversizedVectorAllocation` Error. let err = deserialize::(&serialize(&(super::MAX_VEC_SIZE as u32))).unwrap_err(); - assert!(matches!(err, Error::MissingData)); + match err { + Error::Io(e) => panic!("unexpected I/O error {}", e), + Error::Parse(e) => assert_eq!(e, ParseError::MissingData), + } test_len_is_max_vec::(); test_len_is_max_vec::(); @@ -1056,7 +1061,10 @@ mod tests { let mut buf = Vec::new(); buf.emit_compact_size(super::MAX_VEC_SIZE / mem::size_of::()).unwrap(); let err = deserialize::>(&buf).unwrap_err(); - assert!(matches!(err, Error::MissingData)); + match err { + Error::Io(e) => panic!("unexpected I/O error {}", e), + Error::Parse(e) => assert_eq!(e, ParseError::MissingData), + } } #[test] diff --git a/bitcoin/src/consensus/error.rs b/bitcoin/src/consensus/error.rs index d4f16fe1e..75dad6f69 100644 --- a/bitcoin/src/consensus/error.rs +++ b/bitcoin/src/consensus/error.rs @@ -56,6 +56,54 @@ impl std::error::Error for DecodeEr pub enum Error { /// And I/O error. Io(io::Error), + /// Error parsing encoded object. + Parse(ParseError), +} + +internals::impl_from_infallible!(Error); + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use Error::*; + + match *self { + Io(ref e) => write_err!(f, "IO error"; e), + Parse(ref e) => write_err!(f, "error parsing encoded object"; e), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use Error::*; + + match *self { + Io(ref e) => Some(e), + Parse(ref e) => Some(e), + } + } +} + +impl From for Error { + fn from(e: io::Error) -> Self { + use io::ErrorKind; + + match e.kind() { + ErrorKind::UnexpectedEof => Error::Parse(ParseError::MissingData), + _ => Error::Io(e), + } + } +} + +impl From for Error { + fn from(e: ParseError) -> Self { Error::Parse(e) } +} + +/// Encoding is invalid. +#[derive(Debug, PartialEq, Eq)] +#[non_exhaustive] +pub enum ParseError { /// Missing data (early end of file or slice too short). MissingData, // TODO: Can we add more context? /// Tried to allocate an oversized vector. @@ -80,14 +128,13 @@ pub enum Error { UnsupportedSegwitFlag(u8), } -internals::impl_from_infallible!(Error); +internals::impl_from_infallible!(ParseError); -impl fmt::Display for Error { +impl fmt::Display for ParseError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - use Error::*; + use ParseError::*; match *self { - Io(ref e) => write_err!(f, "IO error"; e), MissingData => write!(f, "missing data (early end of file or slice too short)"), OversizedVectorAllocation { requested: ref r, max: ref m } => write!(f, "allocation of oversized vector: requested {}, maximum {}", r, m), @@ -102,12 +149,11 @@ impl fmt::Display for Error { } #[cfg(feature = "std")] -impl std::error::Error for Error { +impl std::error::Error for ParseError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - use Error::*; + use ParseError::*; match self { - Io(e) => Some(e), MissingData | OversizedVectorAllocation { .. } | InvalidChecksum { .. } @@ -118,17 +164,6 @@ impl std::error::Error for Error { } } -impl From for Error { - fn from(e: io::Error) -> Self { - use io::ErrorKind; - - match e.kind() { - ErrorKind::UnexpectedEof => Error::MissingData, - _ => Error::Io(e), - } - } -} - /// Hex deserialization error. #[derive(Debug)] pub enum FromHexError { @@ -169,4 +204,6 @@ impl From for FromHexError { /// Constructs a `Error::ParseFailed` error. // This whole variant should go away because of the inner string. -pub(crate) fn parse_failed_error(msg: &'static str) -> Error { Error::ParseFailed(msg) } +pub(crate) fn parse_failed_error(msg: &'static str) -> Error { + Error::Parse(ParseError::ParseFailed(msg)) +} diff --git a/bitcoin/src/consensus/mod.rs b/bitcoin/src/consensus/mod.rs index 8a43ae329..7ef30ada6 100644 --- a/bitcoin/src/consensus/mod.rs +++ b/bitcoin/src/consensus/mod.rs @@ -20,7 +20,7 @@ use crate::consensus; #[doc(inline)] pub use self::{ encode::{deserialize, deserialize_partial, serialize, Decodable, Encodable, ReadExt, WriteExt}, - error::{Error, FromHexError, DecodeError}, + error::{Error, FromHexError, DecodeError, ParseError}, }; pub(crate) use self::error::parse_failed_error; diff --git a/bitcoin/src/consensus/serde.rs b/bitcoin/src/consensus/serde.rs index f96363f18..2751098fb 100644 --- a/bitcoin/src/consensus/serde.rs +++ b/bitcoin/src/consensus/serde.rs @@ -17,8 +17,7 @@ use serde::de::{SeqAccess, Unexpected, Visitor}; use serde::ser::SerializeSeq; use serde::{Deserializer, Serializer}; -use super::encode::Error as ConsensusError; -use super::{Decodable, Encodable}; +use super::{Decodable, Encodable, Error, ParseError}; use crate::consensus::{DecodeError, IterReader}; /// Hex-encoding strategy @@ -358,26 +357,25 @@ impl serde::de::Expected for DisplayExpected { } // not a trait impl because we panic on some variants -fn consensus_error_into_serde(error: ConsensusError) -> E { +fn consensus_error_into_serde(error: ParseError) -> E { match error { - ConsensusError::Io(error) => panic!("unexpected IO error {:?}", error), - ConsensusError::MissingData => + ParseError::MissingData => E::custom("missing data (early end of file or slice too short)"), - ConsensusError::OversizedVectorAllocation { requested, max } => E::custom(format_args!( + ParseError::OversizedVectorAllocation { requested, max } => E::custom(format_args!( "the requested allocation of {} items exceeds maximum of {}", requested, max )), - ConsensusError::InvalidChecksum { expected, actual } => E::invalid_value( + ParseError::InvalidChecksum { expected, actual } => E::invalid_value( Unexpected::Bytes(&actual), &DisplayExpected(format_args!( "checksum {:02x}{:02x}{:02x}{:02x}", expected[0], expected[1], expected[2], expected[3] )), ), - ConsensusError::NonMinimalVarInt => + ParseError::NonMinimalVarInt => E::custom(format_args!("compact size was not encoded minimally")), - ConsensusError::ParseFailed(msg) => E::custom(msg), - ConsensusError::UnsupportedSegwitFlag(flag) => + ParseError::ParseFailed(msg) => E::custom(msg), + ParseError::UnsupportedSegwitFlag(flag) => E::invalid_value(Unexpected::Unsigned(flag.into()), &"segwit version 1 flag"), } } @@ -390,7 +388,9 @@ where match self { DecodeError::Other(error) => error, DecodeError::TooManyBytes => E::custom(format_args!("got more bytes than expected")), - DecodeError::Consensus(error) => consensus_error_into_serde(error), + DecodeError::Consensus(Error::Parse(e)) => consensus_error_into_serde(e), + DecodeError::Consensus(Error::Io(_)) => + unreachable!("iterator never returns I/O error"), } } } @@ -403,7 +403,9 @@ where match self { DecodeError::Other(error) => error.into_de_error(), DecodeError::TooManyBytes => DE::custom(format_args!("got more bytes than expected")), - DecodeError::Consensus(error) => consensus_error_into_serde(error), + DecodeError::Consensus(Error::Parse(e)) => consensus_error_into_serde(e), + DecodeError::Consensus(Error::Io(_)) => + unreachable!("iterator never returns I/O error"), } } } diff --git a/bitcoin/src/merkle_tree/block.rs b/bitcoin/src/merkle_tree/block.rs index 1b96efe12..ef363d643 100644 --- a/bitcoin/src/merkle_tree/block.rs +++ b/bitcoin/src/merkle_tree/block.rs @@ -426,10 +426,11 @@ impl Decodable for PartialMerkleTree { let nb_bytes_for_bits = r.read_compact_size()? as usize; if nb_bytes_for_bits > MAX_VEC_SIZE { - return Err(encode::Error::OversizedVectorAllocation { + return Err(encode::ParseError::OversizedVectorAllocation { requested: nb_bytes_for_bits, max: MAX_VEC_SIZE, - }); + } + .into()); } let mut bits = vec![false; nb_bytes_for_bits * 8]; for chunk in bits.chunks_mut(8) { diff --git a/bitcoin/src/psbt/raw.rs b/bitcoin/src/psbt/raw.rs index 906af3b63..110624b58 100644 --- a/bitcoin/src/psbt/raw.rs +++ b/bitcoin/src/psbt/raw.rs @@ -82,10 +82,10 @@ impl Key { let key_byte_size: u64 = byte_size - 1; if key_byte_size > MAX_VEC_SIZE.to_u64() { - return Err(encode::Error::OversizedVectorAllocation { + return Err(encode::Error::Parse(encode::ParseError::OversizedVectorAllocation { requested: key_byte_size as usize, max: MAX_VEC_SIZE, - } + }) .into()); } From 713196be0d61313bd1873c4ec366efb4e43f6892 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 18 Oct 2024 12:27:35 +1100 Subject: [PATCH 7/8] Return DeserError from encode::deserialize The `encode::deserialize` function never returns an I/O error. Add a new error type that expresses this. --- bitcoin/src/blockdata/transaction.rs | 2 +- bitcoin/src/consensus/encode.rs | 25 +++++++++--------- bitcoin/src/consensus/error.rs | 39 ++++++++++++++++++++++++++++ bitcoin/src/consensus/mod.rs | 2 +- bitcoin/src/psbt/error.rs | 17 ++++++++++++ 5 files changed, 70 insertions(+), 15 deletions(-) diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index 8eabc3054..48e68161b 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -1486,7 +1486,7 @@ mod tests { let tx_bytes = hex!("0000fd000001021921212121212121212121f8b372b0239cc1dff600000000004f4f4f4f4f4f4f4f000000000000000000000000000000333732343133380d000000000000000000000000000000ff000000000009000dff000000000000000800000000000000000d"); let tx: Result = deserialize(&tx_bytes); assert!(tx.is_err()); - assert!(matches!(tx.unwrap_err(), crate::consensus::Error::Parse(_))); + assert!(matches!(tx.unwrap_err(), crate::consensus::DeserializeError::Parse(_))); } #[test] diff --git a/bitcoin/src/consensus/encode.rs b/bitcoin/src/consensus/encode.rs index 5b9902f01..462e68a31 100644 --- a/bitcoin/src/consensus/encode.rs +++ b/bitcoin/src/consensus/encode.rs @@ -36,7 +36,7 @@ use crate::taproot::TapLeafHash; use crate::transaction::{Transaction, TxIn, TxOut}; #[rustfmt::skip] // Keep public re-exports separate. -pub use super::{Error, FromHexError, ParseError}; +pub use super::{Error, FromHexError, ParseError, DeserializeError}; /// Encodes an object into a vector. pub fn serialize(data: &T) -> Vec { @@ -53,14 +53,14 @@ pub fn serialize_hex(data: &T) -> String { /// Deserializes an object from a vector, will error if said deserialization /// doesn't consume the entire vector. -pub fn deserialize(data: &[u8]) -> Result { +pub fn deserialize(data: &[u8]) -> Result { let (rv, consumed) = deserialize_partial(data)?; // Fail if data are not consumed entirely. if consumed == data.len() { Ok(rv) } else { - Err(super::parse_failed_error("data not consumed entirely when explicitly deserializing")) + Err(DeserializeError::Unconsumed) } } @@ -74,10 +74,15 @@ pub fn deserialize_hex(hex: &str) -> Result { /// Deserializes an object from a vector, but will not report an error if said deserialization /// doesn't consume the entire vector. -pub fn deserialize_partial(data: &[u8]) -> Result<(T, usize), Error> { +pub fn deserialize_partial(data: &[u8]) -> Result<(T, usize), ParseError> { let mut decoder = Cursor::new(data); - let rv = Decodable::consensus_decode_from_finite_reader(&mut decoder)?; + let rv = match Decodable::consensus_decode_from_finite_reader(&mut decoder) { + Ok(rv) => rv, + Err(Error::Parse(e)) => return Err(e), + Err(Error::Io(_)) => + unreachable!("consensus_decode code never returns an I/O error for in-memory reads"), + }; let consumed = decoder.position() as usize; Ok((rv, consumed)) @@ -1033,10 +1038,7 @@ mod tests { // by making sure it fails with `MissingData` and not an `OversizedVectorAllocation` Error. let err = deserialize::(&serialize(&(super::MAX_VEC_SIZE as u32))).unwrap_err(); - match err { - Error::Io(e) => panic!("unexpected I/O error {}", e), - Error::Parse(e) => assert_eq!(e, ParseError::MissingData), - } + assert_eq!(err, DeserializeError::Parse(ParseError::MissingData)); test_len_is_max_vec::(); test_len_is_max_vec::(); @@ -1061,10 +1063,7 @@ mod tests { let mut buf = Vec::new(); buf.emit_compact_size(super::MAX_VEC_SIZE / mem::size_of::()).unwrap(); let err = deserialize::>(&buf).unwrap_err(); - match err { - Error::Io(e) => panic!("unexpected I/O error {}", e), - Error::Parse(e) => assert_eq!(e, ParseError::MissingData), - } + assert_eq!(err, DeserializeError::Parse(ParseError::MissingData)); } #[test] diff --git a/bitcoin/src/consensus/error.rs b/bitcoin/src/consensus/error.rs index 75dad6f69..4321cf776 100644 --- a/bitcoin/src/consensus/error.rs +++ b/bitcoin/src/consensus/error.rs @@ -11,6 +11,45 @@ use internals::write_err; #[cfg(doc)] use super::IterReader; +/// Error deserializing from a slice. +#[derive(Debug, PartialEq, Eq)] +#[non_exhaustive] +pub enum DeserializeError { + /// Error parsing encoded object. + Parse(ParseError), + /// Data unconsumed error. + Unconsumed, +} + +internals::impl_from_infallible!(DeserializeError); + +impl fmt::Display for DeserializeError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use DeserializeError::*; + + match *self { + Parse(ref e) => write_err!(f, "error parsing encoded object"; e), + Unconsumed => write!(f, "data not consumed entirely when deserializing"), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for DeserializeError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use DeserializeError::*; + + match *self { + Parse(ref e) => Some(e), + Unconsumed => None, + } + } +} + +impl From for DeserializeError { + fn from(e: ParseError) -> Self { Self::Parse(e) } +} + /// Error when consensus decoding from an `[IterReader]`. #[derive(Debug)] pub enum DecodeError { diff --git a/bitcoin/src/consensus/mod.rs b/bitcoin/src/consensus/mod.rs index 7ef30ada6..04dbd5479 100644 --- a/bitcoin/src/consensus/mod.rs +++ b/bitcoin/src/consensus/mod.rs @@ -20,7 +20,7 @@ use crate::consensus; #[doc(inline)] pub use self::{ encode::{deserialize, deserialize_partial, serialize, Decodable, Encodable, ReadExt, WriteExt}, - error::{Error, FromHexError, DecodeError, ParseError}, + error::{Error, FromHexError, DecodeError, ParseError, DeserializeError}, }; pub(crate) use self::error::parse_failed_error; diff --git a/bitcoin/src/psbt/error.rs b/bitcoin/src/psbt/error.rs index 1804459e4..e1d266650 100644 --- a/bitcoin/src/psbt/error.rs +++ b/bitcoin/src/psbt/error.rs @@ -71,6 +71,10 @@ pub enum Error { CombineInconsistentKeySources(Box), /// Serialization error in bitcoin consensus-encoded structures ConsensusEncoding(encode::Error), + /// Deserialization error in bitcoin consensus-encoded structures. + ConsensusDeserialize(encode::DeserializeError), + /// Error parsing bitcoin consensus-encoded object. + ConsensusParse(encode::ParseError), /// Negative fee NegativeFee, /// Integer overflow in fee calculation @@ -141,6 +145,9 @@ impl fmt::Display for Error { write!(f, "combine conflict: {}", s) } ConsensusEncoding(ref e) => write_err!(f, "bitcoin consensus encoding error"; e), + ConsensusDeserialize(ref e) => + write_err!(f, "bitcoin consensus deserializaton error"; e), + ConsensusParse(ref e) => write_err!(f, "error parsing bitcoin consensus encoded object"; e), NegativeFee => f.write_str("PSBT has a negative fee which is not allowed"), FeeOverflow => f.write_str("integer overflow in fee calculation"), InvalidPublicKey(ref e) => write_err!(f, "invalid public key"; e), @@ -169,6 +176,8 @@ impl std::error::Error for Error { match *self { InvalidHash(ref e) => Some(e), ConsensusEncoding(ref e) => Some(e), + ConsensusDeserialize(ref e) => Some(e), + ConsensusParse(ref e) => Some(e), Io(ref e) => Some(e), InvalidMagic | MissingUtxo @@ -211,6 +220,14 @@ impl From for Error { fn from(e: encode::Error) -> Self { Error::ConsensusEncoding(e) } } +impl From for Error { + fn from(e: encode::DeserializeError) -> Self { Error::ConsensusDeserialize(e) } +} + +impl From for Error { + fn from(e: encode::ParseError) -> Self { Error::ConsensusParse(e) } +} + impl From for Error { fn from(e: io::Error) -> Self { Error::Io(e) } } From bbffa3db43802b30d23259c0372f16a877a0ef8b Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 22 Oct 2024 12:39:07 +1100 Subject: [PATCH 8/8] Remove the IO error from DecodeError The `DecodeError` (badly named) consensus decodes an object from an iterator that implements `Read`. The `Read` impl never returns a real IO error, we use the `io::Error` to temporarily wrap the error returned by the inner iterator and unwrap it in `IterReader::decode`. As such there is no reason for the `DecodeError` to hold an `encode::Error`, it can hold an `encode::ParseError`. The value of this change is easily seen in the removal of calls to `unreachable`. --- bitcoin/src/consensus/encode.rs | 2 +- bitcoin/src/consensus/error.rs | 20 +++++++++++--------- bitcoin/src/consensus/mod.rs | 4 ++-- bitcoin/src/consensus/serde.rs | 14 +++++--------- 4 files changed, 19 insertions(+), 21 deletions(-) diff --git a/bitcoin/src/consensus/encode.rs b/bitcoin/src/consensus/encode.rs index 462e68a31..3af089b30 100644 --- a/bitcoin/src/consensus/encode.rs +++ b/bitcoin/src/consensus/encode.rs @@ -1164,7 +1164,7 @@ mod tests { hex.push_str("abcdef"); assert!(matches!( deserialize_hex::(&hex).unwrap_err(), - FromHexError::Decode(DecodeError::TooManyBytes) + FromHexError::Decode(DecodeError::Unconsumed) )); } } diff --git a/bitcoin/src/consensus/error.rs b/bitcoin/src/consensus/error.rs index 4321cf776..cfd9f256a 100644 --- a/bitcoin/src/consensus/error.rs +++ b/bitcoin/src/consensus/error.rs @@ -51,14 +51,17 @@ impl From for DeserializeError { } /// Error when consensus decoding from an `[IterReader]`. +/// +/// This is the same as a `DeserializeError` with an additional variant to return any error yealded +/// by the inner bytes iterator. #[derive(Debug)] pub enum DecodeError { - /// Attempted to decode an object from an iterator that yielded too many bytes. - TooManyBytes, /// Invalid consensus encoding. - Consensus(Error), + Parse(ParseError), + /// Data unconsumed error. + Unconsumed, /// Other decoding error. - Other(E), + Other(E), // Yielded by the inner iterator. } internals::impl_from_infallible!(DecodeError); @@ -68,9 +71,8 @@ impl fmt::Display for DecodeError { use DecodeError::*; match *self { - TooManyBytes => - write!(f, "attempted to decode object from an iterator that yielded too many bytes"), - Consensus(ref e) => write_err!(f, "invalid consensus encoding"; e), + Parse(ref e) => write_err!(f, "error parsing encoded object"; e), + Unconsumed => write!(f, "data not consumed entirely when deserializing"), Other(ref other) => write!(f, "other decoding error: {:?}", other), } } @@ -82,8 +84,8 @@ impl std::error::Error for DecodeEr use DecodeError::*; match *self { - TooManyBytes => None, - Consensus(ref e) => Some(e), + Parse(ref e) => Some(e), + Unconsumed => None, Other(ref e) => Some(e), } } diff --git a/bitcoin/src/consensus/mod.rs b/bitcoin/src/consensus/mod.rs index 04dbd5479..708ff6347 100644 --- a/bitcoin/src/consensus/mod.rs +++ b/bitcoin/src/consensus/mod.rs @@ -38,12 +38,12 @@ impl>> IterReader { fn decode(mut self) -> Result> { let result = T::consensus_decode(&mut self); match (result, self.error) { - (Ok(_), None) if self.iterator.next().is_some() => Err(DecodeError::TooManyBytes), + (Ok(_), None) if self.iterator.next().is_some() => Err(DecodeError::Unconsumed), (Ok(value), None) => Ok(value), (Ok(_), Some(error)) => panic!("{} silently ate the error: {:?}", core::any::type_name::(), error), (Err(consensus::encode::Error::Io(io_error)), Some(de_error)) if io_error.kind() == io::ErrorKind::Other && io_error.get_ref().is_none() => Err(DecodeError::Other(de_error)), - (Err(consensus_error), None) => Err(DecodeError::Consensus(consensus_error)), + (Err(consensus::encode::Error::Parse(parse_error)), None) => Err(DecodeError::Parse(parse_error)), (Err(consensus::encode::Error::Io(io_error)), de_error) => panic!("unexpected IO error {:?} returned from {}::consensus_decode(), deserialization error: {:?}", io_error, core::any::type_name::(), de_error), (Err(consensus_error), Some(de_error)) => panic!("{} should've returned `Other` IO error because of deserialization error {:?} but it returned consensus error {:?} instead", core::any::type_name::(), de_error, consensus_error), } diff --git a/bitcoin/src/consensus/serde.rs b/bitcoin/src/consensus/serde.rs index 2751098fb..8ef5e275e 100644 --- a/bitcoin/src/consensus/serde.rs +++ b/bitcoin/src/consensus/serde.rs @@ -17,7 +17,7 @@ use serde::de::{SeqAccess, Unexpected, Visitor}; use serde::ser::SerializeSeq; use serde::{Deserializer, Serializer}; -use super::{Decodable, Encodable, Error, ParseError}; +use super::{Decodable, Encodable, ParseError}; use crate::consensus::{DecodeError, IterReader}; /// Hex-encoding strategy @@ -387,10 +387,8 @@ where fn unify(self) -> E { match self { DecodeError::Other(error) => error, - DecodeError::TooManyBytes => E::custom(format_args!("got more bytes than expected")), - DecodeError::Consensus(Error::Parse(e)) => consensus_error_into_serde(e), - DecodeError::Consensus(Error::Io(_)) => - unreachable!("iterator never returns I/O error"), + DecodeError::Unconsumed => E::custom(format_args!("got more bytes than expected")), + DecodeError::Parse(e) => consensus_error_into_serde(e), } } } @@ -402,10 +400,8 @@ where fn into_de_error(self) -> DE { match self { DecodeError::Other(error) => error.into_de_error(), - DecodeError::TooManyBytes => DE::custom(format_args!("got more bytes than expected")), - DecodeError::Consensus(Error::Parse(e)) => consensus_error_into_serde(e), - DecodeError::Consensus(Error::Io(_)) => - unreachable!("iterator never returns I/O error"), + DecodeError::Unconsumed => DE::custom(format_args!("got more bytes than expected")), + DecodeError::Parse(e) => consensus_error_into_serde(e), } } }