Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PSBT Deserialization to 174 Compliance over empty Taproot Tree #1194

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/util/psbt/error.rs
Expand Up @@ -7,6 +7,7 @@ use core::fmt;
use crate::blockdata::transaction::Transaction;
use crate::consensus::encode;
use crate::util::psbt::raw;
use crate::util::psbt::map::IncompleteTapTree;

use crate::hashes;
use crate::util::bip32::ExtendedPubKey;
Expand Down Expand Up @@ -73,6 +74,9 @@ pub enum Error {
CombineInconsistentKeySources(Box<ExtendedPubKey>),
/// Serialization error in bitcoin consensus-encoded structures
ConsensusEncoding,
/// Incomplete TapTree Error
// N.B. No need to Box as it's just a vector internally.
IncompleteTapTree(IncompleteTapTree)
}

impl fmt::Display for Error {
Expand Down Expand Up @@ -100,6 +104,7 @@ impl fmt::Display for Error {
},
Error::CombineInconsistentKeySources(ref s) => { write!(f, "combine conflict: {}", s) },
Error::ConsensusEncoding => f.write_str("bitcoin consensus or BIP-174 encoding error"),
Error::IncompleteTapTree(ref error) => write_err!(f, "incomplete Taproot tree", error),
}
}
}
Expand Down Expand Up @@ -127,7 +132,8 @@ impl std::error::Error for Error {
| NonStandardSighashType(_)
| InvalidPreimageHashPair{ .. }
| CombineInconsistentKeySources(_)
| ConsensusEncoding => None,
| ConsensusEncoding
| IncompleteTapTree(_) => None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inner IncompleteTapTree implements error::Error so this should return Some(e) instead of None, same as for HashParse.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/should/must/

}
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/util/psbt/map/output.rs
Expand Up @@ -99,6 +99,12 @@ impl core::fmt::Display for IncompleteTapTree {
}
}

impl From<IncompleteTapTree> for crate::util::psbt::Error {
fn from(i: IncompleteTapTree) -> Self {
crate::util::psbt::Error::IncompleteTapTree(i)
Comment on lines +103 to +104
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use more readable names.

Suggested change
fn from(i: IncompleteTapTree) -> Self {
crate::util::psbt::Error::IncompleteTapTree(i)
fn from(error: IncompleteTapTree) -> Self {
crate::util::psbt::Error::IncompleteTapTree(error)

}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Perhaps this could go in psbt::error belowe the other From impls.

#[cfg(feature = "std")]
#[cfg_attr(docsrs, doc(cfg(feature = "std")))]
impl std::error::Error for IncompleteTapTree {
Expand Down
15 changes: 14 additions & 1 deletion src/util/psbt/serialize.rs
Expand Up @@ -7,6 +7,8 @@
//!

use crate::prelude::*;
use core;
use core::convert::TryFrom;

use crate::io;

Expand Down Expand Up @@ -344,7 +346,7 @@ impl Deserialize for TapTree {
.map_err(|_| encode::Error::ParseFailed("Tree not in DFS order"))?;
}
if builder.is_finalizable() && !builder.has_hidden_nodes() {
Ok(TapTree(builder))
Ok(TapTree::try_from(builder).map_err(psbt::Error::from)?)
} else {
Err(encode::Error::ParseFailed("Incomplete taproot Tree"))
}
Expand Down Expand Up @@ -395,6 +397,17 @@ mod tests {
assert_eq!(tree, tree_prime);
}

#[cfg(feature = "base64")]
#[test]
fn taptree_empty() {
use core::str::FromStr;
use crate::psbt::PartiallySignedTransaction;
let test_data_containing_empty =
"cHNidP8BAIkCAAAAAWlZ9F5/nRjE0quO8bE8cfKIiVTKHa/Zu6PKwWfpZDmpAAAAAAD9////Anz3nHYAAAAAIlEg7T17MEFOm47gN7FhZB5g4HjviGW4YxZWReJr1c/w0I+AlpgAAAAAACJRIDspRhxBQj5+aJw088IczFSbyRKB7wofy/SQXnErwCcQAAAAAAABASsAlDV3AAAAACJRIBMUosKNoVNU5tEW2Q/tylLRzabMGIprODHM0LbB8h1/IRaXdf3KRBBZMc8epg7uuU3pr42i3YMFnheNDW10F0cpnxkAHzYIzlYAAIABAACAAAAAgAAAAABRAAAAARcgl3X9ykQQWTHPHqYO7rlN6a+Not2DBZ4XjQ1tdBdHKZ8AAQUgT+NhtL213EJgJ51h6FiZYhPgUv48YQKACnnMYpOZPMEBBgAhB0/jYbS9tdxCYCedYehYmWIT4FL+PGECgAp5zGKTmTzBGQAfNgjOVgAAgAEAAIAAAACAAQAAADEBAAAAAQUgrUXPD+fhaOeY7GlxJDZ0pAgUZ0reW+A3wmxMwBAIt14BBgAhB61Fzw/n4WjnmOxpcSQ2dKQIFGdK3lvgN8JsTMAQCLdeGQAfNgjOVgAAgAEAAIAAAACAAAAAAFIAAAAA";
let psbt = PartiallySignedTransaction::from_str(test_data_containing_empty);
assert!(matches!(psbt, Err(psbt::PsbtParseError::PsbtEncoding(encode::Error::ParseFailed("Incomplete taproot Tree")))));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matches! is not available in our MSRV. How the hell does this pass CI?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it fails one case; will change it.

}

#[test]
fn can_deserialize_non_standard_psbt_sighash_type() {
let non_standard_sighash = [222u8, 0u8, 0u8, 0u8]; // 32 byte value.
Expand Down