Fix PSBT Deserialization to 174 Compliance over empty Taproot Tree #1194
Conversation
JeremyRubin
force-pushed
the
fix-taptree-ser
branch
from
9dd74e7
to
638f8ed
Compare
| @@ -127,7 +132,8 @@ impl std::error::Error for Error { | |||
| | NonStandardSighashType(_) | |||
| | InvalidPreimageHashPair{ .. } | |||
| | CombineInconsistentKeySources(_) | |||
| | ConsensusEncoding => None, | |||
| | ConsensusEncoding | |||
| | IncompleteTapTree(_) => None, | |||
There was a problem hiding this comment.
The inner IncompleteTapTree implements error::Error so this should return Some(e) instead of None, same as for HashParse.
| crate::util::psbt::Error::IncompleteTapTree(i) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: Perhaps this could go in psbt::error belowe the other From impls.
|
I would like to investigate this a bit more. |
|
Looks like we are allowing empty trees. It was hard to think about this while merging #936 as we were indirectly reliant on the assumption of there being at least one leaf. |
| @@ -127,7 +132,8 @@ impl std::error::Error for Error { | |||
| | NonStandardSighashType(_) | |||
| | InvalidPreimageHashPair{ .. } | |||
| | CombineInconsistentKeySources(_) | |||
| | ConsensusEncoding => None, | |||
| | ConsensusEncoding | |||
| | IncompleteTapTree(_) => None, | |||
| 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"))))); |
There was a problem hiding this comment.
matches! is not available in our MSRV. How the hell does this pass CI?!
There was a problem hiding this comment.
it fails one case; will change it.
| fn from(i: IncompleteTapTree) -> Self { | ||
| crate::util::psbt::Error::IncompleteTapTree(i) |
There was a problem hiding this comment.
Please use more readable names.
| fn from(i: IncompleteTapTree) -> Self { | |
| crate::util::psbt::Error::IncompleteTapTree(i) | |
| fn from(error: IncompleteTapTree) -> Self { | |
| crate::util::psbt::Error::IncompleteTapTree(error) |
I don't think that's the source of the error, the source of the error is that we directly construct TapTree here rather than using TapTree try_from. |
|
Hmm, maybe it's time already to open a Rust 2021 tracking issue to list stuff like |
|
@apoelstra #1060 is already open. :) Also |
Co-authored-by: Martin Habovštiak <martin.habovstiak@gmail.com>
|
Note that the related upstream Core PR is now: bitcoin/bitcoin#25858, if anyone here is interested in reviewing. |
|
up for grabs. |
Fixes #1192, partly (may also be a bug in Core it seems, so workflows involving core are stil broken?)