Add a method to PSBT to compute sighash::Prevouts

[sanket1729]

The most important feature of Psbt's is that they have all the information to compute the sighash. Our new APIs for taproot_key_spend_signature_hash and taproot_script_spend_signature_hash require a sighash::Prevouts struct. This is essentially a collection of all utxos the transaction is spending.

It would be great to have the following API:

impl PartialSignedTransaction {
	pub fn to_prevouts(&self) -> sighash::Prevouts {
		/// Always return the  Prevouts::All variant as psbts must have UTXO information
	}
}

[violet360]

Hey @sanket1729, I am a participant of summer of bitcoin, new to rust and this project, but this seems very interesting one to start my journey with, would like to work on it....looking forward to raise a PR.

[violet360]

Hello,
So I read a bit about psbts, ( here, here, and here )started by tracing the class PartiallySignedTransaction in mod.rs, I think this is where I will add the function and then tried to map a rough schema of the class dependencies.
so based on this I wrote some rough snippet, just wanted to know am I approaching it right ?

impl PartiallySignedTransaction {
	pub fn to_prevouts(&self) -> sighash::Prevouts {
        	let mut inputs : Vec<TxIn> = self.unsigned_tx.input // input array of the psbt struct
        	let mut prevOuts : Vec<OutPoint> = Vec::new(); 
        	for x in 0..inputs.len() {
            		prevOuts.push(inputs[x].previous_output) // just pushing the previous_output of each input in the empty output array
        	}
        	return prevOuts;
    	}
}

[sanket1729]

@violet360, the type of prevouts is

rust-bitcoin/src/util/sighash.rs

Line 84 in 2c1077e

pub enum Prevouts<'u, T> where T: 'u + Borrow<TxOut> {

.

You would need a Vec<&TxOut> from the psbts. There are two cases:
If the Inputs has WITNESS_UTXO, use it directly.
Otherwise, look at the spending txout for the non_witness_utxo and lookup the utxo at the spending index determined by the outpoint information.

Here is some code that I am using downstream to get the non_witness_utxo

        let vout = psbt.unsigned_tx.input[index].previous_output.vout;
        let non_witness_utxo = &non_witness_utxo.output[vout as usize]

[sanket1729]

Some spoiler code that I am using downstream: https://github.com/sanket1729/rust-miniscript/blob/685d374fe4758d067439962e9b6bb4a3118ff430/src/psbt/finalizer.rs#L104-L126

This is (almost) exactly what we need, but I want to avoid the .clone() call there. After #835, we no longer need the allocation.

[violet360]

Hey, thanks for pointing out the cases, made the changes.
But I have one basic doubt (mentioned below).

If we just need a list of funding utxos for a psbt then why sighash::Prevouts instead of Result<Vec<&TxOut>, Error> as return type ?

I think the answer lies in scope of ownership and lifetime annotation but not sure...unable to give proper logical reason to myself.
My initial implementation used sighash::Prevouts as return type, but the code exited with error on line 3.

missing generics for enum `Prevouts`
expected 1 generic argumentrustc[E0107](https://doc.rust-lang.org/error-index.html#E0107)
[sighash.rs(84, 10): ]()enum defined here, with 1 generic parameter: `T`
[mod.rs(76, 50): ]()add missing generic argument: `Prevouts<T>`

The return type Result<Vec<&TxOut>, Error> seems more intuitive to me, because it not only returns the value but also handles error, below is the current code I wrote, unlike the previous code this one had no compilation error, can you tell me the missing pieces ?

impl PartiallySignedTransaction {
...
...
    /// returns the vector of utxos
    pub fn to_prevouts(&self) -> Result<Vec<&TxOut>, Error> {
        let mut prev_outs : Vec<&TxOut> = Vec::new(); 
        for i in 0..self.inputs.len() {
            let psbt_input = &self.inputs[i];
             // check for witness utxos
                if let Some(ref witness_utxo) = psbt_input.witness_utxo {
                    prev_outs.push(&witness_utxo)
                     // if no witness utxos then catch the non witness part of the input
                } else if let Some(ref non_witness_utxo) = psbt_input.non_witness_utxo {
                     // get output index (vout) from the outpoint and use it to get the non_witness_utxo
                    let vout = self.unsigned_tx.input[i].previous_output.vout as usize;
                    prev_outs.push(&non_witness_utxo.output[vout])
                } else {
                    // created MissingUtxo error enum in errors.rs
                    return Err(Error::MissingUtxo);
                }
            }
            Ok(prev_outs)
        }
...
...
}

Pardon if my understanding seems too naive, still learning, but will improve with time for sure.

[tcharding]

So this is a bit of a curly one because of Rust borrowing rules. I played around with this and was able to return a Result<Prevouts<&TxOut>, Error> by first changing the Prevouts::All variant to use vec instead of a slice (removes the explicit lifetime also).

-    All(&'u [T]),
+    All(Vec<T>),

I was unable to work out why we use a slice currently or what the implications of changing this were? (If you choose to follow this route you could do this as an initial patch so that folks could review and discuss it.)

I was then able to implement the logic by combining your linked initial implementation with the code linked by @sanket1729 (I particularly like his separation into two functions).

I've intentionally not posted too much code so you can have fun playing with it.

[Kixunil]

Perhaps it should return iterator instead? If Vec is really required, then Cow<'u, [T]> would be a reasonable compromise IMO.

@violet360 please prefer to use iterators, something like this:

self.unsigned_tx.input.iter().zip(&self.inputs).map(|tx_input, psbt_input| {
// Your code here
})
// collect only if Vec should really be returned (IMO it shouldn't)
.collect::<Result<Vec<_>, _>>()

Note this code seems to have significant similarities with one in my bip78 crate I think we should really try to migrate it here.

[violet360]

Hey @Kixunil , Ok, let me try this, I don't really understand iterators that much, last time used them in c++, I guess RUST has it's own ways maybe...so here is my chance to get RUSTy with iterators.
but I want to know how will returning iterators allow me to handle errors and what about sighash::Prevouts ?

Hi @tcharding , thanks for pointing it out....yes I tried that too but my concern was same as yours, what are the implications of changing this....hence thought of not going in that direction.

[violet360]

I thought generating a PR would make it easier to comment on my code..... so did that. #853

[Kixunil]

Yeah iterators in Rust are different (better) and very important because of performance and correctness implications. Definitely try to learn and use them.

You can simply return impl Iterator<Item=Result<&TxOut, Error>> and let the caller handle it with collect() or such.

[violet360]

yeah, ok, thank you, before I start preparing my next commit I just have one question.
If the return type becomes impl Iterator<Item=Result<&TxOut, Error>> (as I am returning an iterator), then what about sighash::Prevouts return type as mentioned in the issue ?

[Kixunil]

I think it's OK to not use it since the caller can easily create it if actually required. E.g. it's not required in my crate. @sanket1729 feel free to dispute.

[sanket1729]

I think it's okay to create an iterator. My crate(the caller) can create the variant by collecting it.

…

On Mon, Feb 28, 2022, 17:03 Martin Habovštiak ***@***.***> wrote: I think it's OK to not use it since the caller can easily create it if actually required. E.g. it's not required in my crate. @sanket1729 <https://github.com/sanket1729> feel free to dispute. — Reply to this email directly, view it on GitHub <#849 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABUQEOPLVFOMWN5LEUHBGATU5OTHRANCNFSM5PJ63NHQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[DanGould]

Was this issue closed by #853 @sanket1729?

Also, wouldn't bip 370 PSBTv2 make this obsolete?

[Kixunil]

@DanGould yeah, it was.

PSBTv2 doesn't make it obsolete but our new Rut API should lead to removal of the panic.