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
API to find funding utxos in psbt #853
Conversation
Nit: Next time you may avoid adding/removing blank lines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High level review only, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the code looks almost good but we really have to fix two more issues I found.
There's another thing: should we use a dedicated error type? I tend towards yes but it can be done later as well. I suggest @violet360 if it's too overwhelming to you to add another error type leave it for later, if you feel like adding it you can do it within this PR.
FTR, this PR will most likely not go to the Taproot release anyway.
src/util/psbt/mod.rs
Outdated
@@ -72,6 +72,24 @@ pub struct PartiallySignedTransaction { | |||
} | |||
|
|||
impl PartiallySignedTransaction { | |||
/// Returns an iterator for the funding UTXOs of the psbt | |||
/// function throws error if transaction inputs have unequal lengths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't "throw" and error though. It panics. In Rust the usual style of documenting panics is like this:
/// Returns an iterator for the funding UTXOs of the psbt
///
/// For each PSBT input that contains UTXO information `Ok` is returned containing that information.
/// The order of returned items is same as the order of inputs.
///
/// ## Errors
///
/// The function returns error when UTXO information is not present or is invalid.
///
/// ## Panics
///
/// The function panics if the length of transaction inputs is not equal to the length of PSBT inputs.
(See below for "or invalid" case.)
src/util/psbt/mod.rs
Outdated
pub fn get_funding_utxos(&self) -> impl Iterator<Item = Result<&TxOut, Error>> { | ||
assert_eq!(self.inputs.len(), self.unsigned_tx.input.len()); | ||
self.unsigned_tx.input.iter().zip(&self.inputs).map(|(tx_input, psbt_input)| { | ||
if let Some(ref witness_utxo) = psbt_input.witness_utxo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: you can avoid ref
by borrowing: if let Some(witness_utxo) = &psbt_input.witness_utxo
I personally prefer this style and I think it's used in this library but not a big deal. ref
has it's own appeal too.
Also perhaps using match on the whole thing would be clearer:
match (&psbt_input.witness_utxo, &psbt_input.non_witness_utxo) {
(Some(witness_utxo), _) => Ok(witness_utxo),
(None, Some(non_witness_utxo) => {
...
},
(None, None) => Err(Error::MissingUtxo),
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the code looks almost good but we really have to fix two more issues I found.
There's another thing: should we use a dedicated error type? I tend towards yes but it can be done later as well. I suggest @violet360 if it's too overwhelming to you to add another error type leave it for later, if you feel like adding it you can do it within this PR.
FTR, this PR will most likely not go to the Taproot release anyway.
yes I too was thinking about generating a seperate error type for psbts but thought it would be an overkill, will try committing that in this PR itself.....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that it still makes sense to also have the variants in Error
and From<PsbtUtxoError> for Error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is acceptable now. Just some minor details.
Because it's a new non-essential feature this should be merged after 0.28 release.
src/util/psbt/mod.rs
Outdated
@@ -171,6 +197,7 @@ mod display_from_str { | |||
#[cfg_attr(docsrs, doc(cfg(feature = "base64")))] | |||
pub use self::display_from_str::PsbtParseError; | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This empty line seems unrelated. It'd be nice to remove it during next push.
src/util/psbt/error.rs
Outdated
/// The separator for a PSBT must be `0xff`. | ||
InvalidSeparator, | ||
/// Triggered when output index goes out of bounds of when trying to get |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem grammatically correct but as a non-native I'll defer this to @tcharding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, you just need to squash the commits into one.
src/util/psbt/error.rs
Outdated
@@ -98,6 +102,8 @@ impl fmt::Display for Error { | |||
} | |||
Error::NoMorePairs => f.write_str("no more key-value pairs for this psbt map"), | |||
Error::HashParseError(e) => write!(f, "Hash Parse Error: {}", e), | |||
Error::MissingUtxo => f.write_str("UTXO information is not present in PSBT"), | |||
Error::PsbtUtxoOutOfbounds => f.write_str("Output index is out of bounds of non witness script output array"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, forgot to comment before that "Output index
should probably be "output index
to be consistent with other messages. Sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hash Parse Error:
is wrong too, a patch put at the front of this PR cleaning that all up would be super useful if you can be bothered. Don't feel obliged though. You will likely touch this line when the error comment is changed anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments, thanks.
src/util/psbt/error.rs
Outdated
@@ -37,8 +37,12 @@ pub enum Error { | |||
/// Magic bytes for a PSBT must be the ASCII for "psbt" serialized in most | |||
/// significant byte order. | |||
InvalidMagic, | |||
///Missing both the witness and non-witness utxo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
///Missing both the witness and non-witness utxo | |
/// Missing both the witness and non-witness utxo. |
src/util/psbt/error.rs
Outdated
/// The separator for a PSBT must be `0xff`. | ||
InvalidSeparator, | ||
/// Triggered when output index goes out of bounds of the output in non witness utxo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Triggered when output index goes out of bounds of the output in non witness utxo | |
/// Triggered when output index does not exist in PSBT input (non-witness) UTXO. |
Not sure if the content of this comment is correct but it shows a grammatical improvement, please modify as you see fit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While strictly speaking correct, thinking of it in terms of "out of bounds" feels more natural to me. Won't block if others prefer your suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair comment, this is a tricky little rustdoc to write succinctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of bound seemed natural to me too, but whatever you guys prefer at the end though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How's this?
/// Triggered when output index is out of bounds in relation to the output in non-witness UTXO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. But should it be "triggered"? Maybe "returned is better"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of error, words like triggered, panic, etc. Sound more suitable than "returned".
Maybe "Triggered" would be more suitable here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mental model in Rust is that errors are returned (well, they actually are :)). But honestly, I don't care much if others find "triggered" better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW we have 'triggered' in one other place in the repo
git grep trig
src/blockdata/script.rs:1279: // which triggerred overflow bugs on 32-bit machines in script formatting in the past.
I'm not fussed either way.
src/util/psbt/error.rs
Outdated
@@ -98,6 +102,8 @@ impl fmt::Display for Error { | |||
} | |||
Error::NoMorePairs => f.write_str("no more key-value pairs for this psbt map"), | |||
Error::HashParseError(e) => write!(f, "Hash Parse Error: {}", e), | |||
Error::MissingUtxo => f.write_str("UTXO information is not present in PSBT"), | |||
Error::PsbtUtxoOutOfbounds => f.write_str("Output index is out of bounds of non witness script output array"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hash Parse Error:
is wrong too, a patch put at the front of this PR cleaning that all up would be super useful if you can be bothered. Don't feel obliged though. You will likely touch this line when the error comment is changed anyways.
|
||
use prelude::*; | ||
|
||
use io; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please no 'random' whitespace changes. The reason is that it makes reviewing the PR harder because reviewers must stop to think what is this, why is it being done, is it correct, is it a step backwards in readability. All easy questions but unnecessary for getting your work merged. The easier you make it for reviewers to ack your work the easier you'll get PR's merged, that's a win for everyone :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, silly mistake of mine ... will take care of it from next commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers mate, no stress. If I had a dollar for ever mistake I made in this repo in the last three months I'd be fiat rich :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with other comments, just adding my 5 cents
/// | ||
/// The function panics if the length of transaction inputs is not equal to the length of PSBT inputs. | ||
pub fn get_funding_utxos(&self) -> impl Iterator<Item = Result<&TxOut, Error>> { | ||
assert_eq!(self.inputs.len(), self.unsigned_tx.input.len()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_eq!(self.inputs.len(), self.unsigned_tx.input.len()); | |
assert_debug_eq!(self.inputs.len(), self.unsigned_tx.input.len()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi !
you meant debug_assert_eq!
instead of assert_debug_eq!
right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delayed review. This is a great shape after several iterations and thanks for addressing all the feedback @violet360 .
Everything is good from my side, just the final API naming.
src/util/psbt/mod.rs
Outdated
/// ## Panics | ||
/// | ||
/// The function panics if the length of transaction inputs is not equal to the length of PSBT inputs. | ||
pub fn get_funding_utxos(&self) -> impl Iterator<Item = Result<&TxOut, Error>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a similar theme for #861, I think we rename this to iter_funding_utxos
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, consider it done 👍🏻
@dr-orlovsky I disagree with it being debug assert. If the two have different length it's a clear inconsistency of input data and there's no good solution. Debug assert will just allow silent weird bugs in release. Regarding naming, |
I also want to improve the error management of psbt, I think there is a lot of room for that, planning to get that done in this PR itself, or maybe a new PR.
pub enum psbt {
...
PsbtHash, // psbthash child enum of psbt
/// Missing both the witness and non-witness utxo.
MissingUtxo,
/// Returned when output index is out of bounds in relation to the output in non-witness UTXO.
PsbtUtxoOutOfbounds,
..
}
...
pub enum Error {
...
psbt,
...
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 9f523e0. If there are further iterations, feel free to request review from me.
@dr-orlovsky I disagree with it being debug assert. If the two have different length it's a clear inconsistency of input data and there's no good solution. Debug assert will just allow silent weird bugs in release.
Indeed, asserts should be assert!
instead of debug_assert!
unless it benchmarked that it affects performance. I had a similar issue in taproot review.
#677 (comment)
Perhaps, that should also be updated to assert!
and we can inspect the codebase for misc debug_assert!
and change those to assert!
That should be a separate PR. @violet360, we should take this discussion to #837 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 9f523e0
@sanket1729 sorry, IDK why but your comment does not displays to me. @Kixunil Here, if the supplied PSBT (as an input) have different number of inputs in tx and map, it must error at the moment it was unparsed into |
@dr-orlovsky See: https://doc.rust-lang.org/std/macro.debug_assert.html Quote from the docs:
|
@Kixunil seems like all your comments were addressed. Can you confirm so we can merge this one? |
I view the logic bug similar to indexing out of bounds or calling You could argue this is not memory-safety issue but I view bugs in bitcoin-related code as seriously as memory safety. People could lose sats. V2 API will remove this by construction but until then |
sorry for the late comment...was busy with some other stuff lately |
FWIW, from my reading of this thread there is consensus on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5afb0ea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5afb0ea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5afb0ea. Thanks for being patient with this.
This interface does not return complete information about the funding utxo. it only returns psbt v0 input map info: It would be nice in the PSBTv2 implementation to return an |
5afb0ea API to get an iterator for funding utxos in psbt (violet360) Pull request description: ### Current status The API returns a vector of UTXOs and has return type `Result<Vec<&TxOut>, Error>` ### Expected The return statement should be of type `sighash::Prevouts` as pointed in #849 ACKs for top commit: Kixunil: ACK 5afb0ea tcharding: ACK 5afb0ea sanket1729: ACK 5afb0ea. Thanks for being patient with this. Tree-SHA512: 724fc3dffdbb1331584f89bbe84527e1af0d193a344fe43b36f2f2a628652d259001a3abf6b3909df53524cd3fbdbe3af760b7004d40d3bee1848fbb83efff5b
Current status
The API returns a vector of UTXOs and has return type
Result<Vec<&TxOut>, Error>
Expected
The return statement should be of type
sighash::Prevouts
as pointed in #849