Update Psbt APIs #305

sanket1729 · 2022-03-08T22:20:10Z

Based on #301

apoelstra · 2022-03-09T16:20:18Z

src/psbt/mod.rs

+/// Additional operations for miniscript descriptors for various psbt roles.
+/// Note that these APIs would generally error when used on scripts that are not
+/// miniscripts.
+pub trait PbstOps {


In 27679de:

I think this should be called PsbtExt

apoelstra · 2022-03-09T16:20:44Z

src/psbt/mod.rs

+
+/// Sighash message(signing data) for a given psbt transaction input.
+#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy)]
+pub enum PsbtSigHashMsg {


In 27679de:

I think these variants should also hold the sighash type. Thoughts?

Why do you think this might be useful? The sighash type is directly available in psbt APIs by rust-bitcoin/rust-bitcoin#847 . The signer only needs to know the message in order to sign with keys

Ah, good point. I forgot about the other accessor function.

apoelstra · 2022-03-09T16:24:13Z

src/psbt/mod.rs

+}
+
+impl PbstOps for Psbt {
+    /// See the triat level documentation for details


In 27679de:

Typo triat in several places

Actually I don't think any of these doccomments are needed at all. rustdoc by default will show the trait-level documentation.

apoelstra · 2022-03-09T16:26:49Z

src/psbt/mod.rs

+                .map_err(|_e| UxtoUpdateError::MissingInputUxto)?;
+            let secp = secp256k1::Secp256k1::verification_only();
+
+            if !(range.start < range.end) {


In 27679de:

I'm confused about the purpose of this if-clause. If the user provides an empty range shouldn't we just do nothing?

Ohh, is the issue that if you try to iterate on a bad range it'll run (effectively) forever? I think you should comment this, and also change the if condition to if range.start > range.end and also I continue to think this should be empty, rather than deriving the 0 index. Users who explicitly want 0 can provide 0..0.

If the descriptor is already is derived and contains no wildcards, we still need to process it. Will add a comment here.

I wanted to use the range.is_empty() API, but that some weird behavior in rust 1.29. Specifically, I can't select candidate 1 amongst the following:

511 | if Range::<u32>::is_empty(&range) { // required for MSRV | ^^^^^^^^^^^^^^^^^^^^^^ multiple `is_empty` found | = note: candidate #1 is defined in an impl for the type `std::ops::Range<_>` = note: candidate #2 is defined in an impl of the trait `std::iter::ExactSizeIterator`

I have tried a lot of ways to do this and rust IRC/discord could not help here.

I think it might make more sense to take an Option<Range>?

apoelstra · 2022-03-09T16:32:49Z

Done reviewing 27679de. No serious comments. This is spectacular, it's what we imagined years ago when PSBT and Miniscript were first coming into focus.

It should probably have more extensive testing but I'm not gonna hold up the PR over that. I'm sure we'll have some API iteration to do when people start using this in the wild.

JeremyRubin · 2022-03-09T18:06:26Z

looks pretty good!

One comment:

It would be great if we had a couple more APIs released:

fn sign_with<S: SigningProvider>(&mut self, s: S) that signs the psbt with some sort of provider (e.g., then you could implement a HWI based SigningProvider and everything ends up in the right place)
fn partially_finalize(&mut self) where it tries to finalize all the inputs it can understand, but allows the others to fail/not finalize. This makes it less brittle if PSBTs include inputs that are unintelligible that you might do a round of partial finalization on each. E.g., imagine i want to prototype a MuSig finalizer thingy.

apoelstra · 2022-03-09T18:48:33Z

@JeremyRubin the original intention of the Satisfier trait was that it be a SigningProvider. Would you be ok with using that trait? And maybe the method name should be update_sign to go along with update_desc? I dunno.

+1 to partially_finalize. This is pretty close to Core's walletprocesspsbt behavior.

JeremyRubin · 2022-03-09T19:17:24Z

Hmmmmm....

I think it makes more sense to rearchitect Satisfier to be

trait Satisfier : ECDSASigner + SchnorrSigner + Preimage.... {}

becuase it seems weird to have all these "baggage" methods when really you just want to provide a HWI that can sign a schnorr thing.

+ the methods for create_sig / update_sig need to take PSBT since otherwise we can't verify on HWI what we are signing?

apoelstra · 2022-03-09T19:23:59Z

All the methods on Satisfier have default implementations which simply don't provide the expected data. And I think they would all make sense for (some) HWWs to provide.

JeremyRubin · 2022-03-09T19:41:12Z

ah ok...

so i guess you'd do something like

struct HWI<'a> {
    psbt: &'a Psbt
    usb_device: Path
}
impl Satisfier for HWI {
    fn lookup_sig(&self, _: &Pk) -> Option<BitcoinSig> {
        todo!();
    }
}

it still seems off, since in theory our HWI can analyze, generate sigs, finalize inputs, etc, whereas Satisfier is really for pluggin the data into our own Psbt solver.

dr-orlovsky · 2022-03-10T09:36:14Z

utACK 27679de

apoelstra · 2022-03-10T14:28:35Z

@JeremyRubin I don't understand why a HWI would be used for finalization, when this requires only public data and any choices made could be overwritten by a host PC anyway.

But I agree it would be nice if the Satisfier API had some way to receive PSBT data so it could correspondingly change future behavior. We could add such a method to Satisfier which took a &mut self and defaulted to doing nothing.

JeremyRubin · 2022-03-10T14:57:41Z

My point is more that the correct API to have with a HWI is not to "lookup signatures" from it for a given Pk, but rather to ask it "can you do anything to this PSBT".

W.r.t. finalizing, they might do that if one of the inputs has some sort of proprietary script type that is specific to that HWW.

apoelstra · 2022-03-10T15:12:19Z

I think the API you're looking for would make more sense as part of a HWI object (in a HWI library) rather than as part of the rust-miniscript PSBT API.

JeremyRubin · 2022-03-10T16:21:00Z

For example, on thing I'm tinkering with is a virtual HWI. This is an application which runs on the same computer, but you have to e.g. copy/paste PSBTs into it to get them signed. It's good for putting the key somewhere else (imagine a core wallet where you watchonly the VHWI xpub instead of having core actually have the key).

I would like to send a PSBT with rust-miniscript to such a VHWI, and it makes sense to me that the rust-miniscript library would provide a fn sign_with<S: SigningProvider>(&mut self, s: S) that I could offer the keys I have in this isolated context & modify the PSBT by inserting signatures.

Basically similar to the logic I have here: https://github.com/sapio-lang/sapio/blob/072b8835dcf4ba6f8f00f3a5d9034ef8e021e0a7/ctv_emulators/src/servers/hd.rs#L63, but this is a special case of CTV oracle where it's safe to sign without parsing.

I think this is the sort of thing miniscript should cover, v.s. external signers having to write their own logic for it since it makes the library a bit more self contained "this is how you should sign a miniscript". And you shouldn't sign for things you don't understand, so the thing you understand is the miniscript (hence making it this libraries responsibility kind-of?).

JeremyRubin · 2022-03-10T16:22:39Z

p.s. this totally could be a new crate, rust-miniscript-psbttools, but then I'd suggest extracting all the psbt code into such a target anyways (why should PSBT finalization stuff need priveledged access to the miniscript internals? if it does, then we need a better abstracted API).

apoelstra · 2022-03-10T19:09:46Z

Can you propose a SigningProvider API? I still don't get how it differs from Satisfier plus a couple extra methods.

You may be correct that your proposal belongs in a different crate (I'm pretty sure none of this PSBT stuff needs access to minscript internals). But the support that we have here is pretty powerful and general, and I'd like to keep it in this crate even if it were redundant with a more advanced library.

apoelstra · 2022-03-11T17:37:03Z

src/psbt/finalizer.rs

+            if !allow_mall {
+                desc.get_satisfaction(PsbtInputSatisfier::new(&psbt, index))
+            } else {
+                desc.get_satisfaction_mall(PsbtInputSatisfier::new(&psbt, index))


In 815d012:

Can't these use sat as well rather than creating new input satisfiers?

seems like it should work, no borrowchecker issues i can think of.

JeremyRubin · 2022-03-12T02:27:06Z

Before I get too far into the gritty details, does something like this seem to make sense for a SigningRole trait?

diff --git a/src/psbt/finalizer.rs b/src/psbt/finalizer.rs
index 66466ed..94adc99 100644
--- a/src/psbt/finalizer.rs
+++ b/src/psbt/finalizer.rs
@@ -27,6 +27,7 @@ use super::{Error, InputError, PsbtInputSatisfier};
 use bitcoin::blockdata::witness::Witness;
 use bitcoin::secp256k1::{self, Secp256k1};
 use bitcoin::util::key::XOnlyPublicKey;
+use bitcoin::util::bip32::ExtendedPrivKey;
 use bitcoin::util::taproot::LeafVersion;
 use bitcoin::{self, PublicKey, Script};
 use descriptor::DescriptorTrait;
@@ -369,8 +370,86 @@ pub fn finalize_helper<C: secp256k1::Verification>(
     Ok(())
 }
 
+/// Trait which handles correctly the BIP-174 defined Signer role.
+pub trait SignerRole {
+    /// sign the provided PSBT in place.
+    /// we use a concrete type to allow singers to be represented as trait objects.
+    fn sign_mut(&self, psbt: &mut Psbt, secp: &Secp256k1<secp256k1::All>) -> Result<(), super::Error>;
+    /// sign the provided PSBT and put the data into a copy that we return.
+    /// The returned PSBT should be combine with the input.
+    ///
+    /// we use a concrete type to allow singers to be represented as trait objects.
+    /// Implementors may wish to optimize this implementation to write to a new PSBT which can be
+    /// combined.
+    fn sign(&self, psbt: &Psbt, secp: &Secp256k1<secp256k1::All>) -> Result<Psbt, super::Error> {
+        let mut ret = psbt.clone();
+        self.sign_mut(&mut ret, secp)?;
+        Ok(ret)
+    }
+}
+
+impl SignerRole for ExtendedPrivKey {
+    fn sign_mut(&self, psbt: &mut Psbt, secp: &Secp256k1<secp256k1::All>) -> Result<(), super::Error> {
+        for (index, input) in psbt.inputs.iter_mut().enumerate() {
+            if let Some(non_witness_utxo) =  input.non_witness_utxo {
+                let txid_expected = psbt.unsigned_tx.input.get(index).map(|t|t.previous_output.txid);
+                if Some(non_witness_utxo.txid()) == txid_expected {
+                    if let Some(redeem_script) = input.redeem_script {
+                        // TODO: fix indexing to .at
+                        let p2sh_expected = non_witness_utxo.output[psbt.unsigned_tx.input[index].previous_output.vout as usize].script_pubkey;
+                        if  p2sh_expected == redeem_script.to_p2sh() {
+                        //            sign_non_witness(redeemScript, i)
+                        } else {
+                            return Err(Error::InputError(InputError::InvalidRedeemScript { redeem:redeem_script:, p2sh_expected}, index));
+                        }
+                    } else {
+                //            sign_non_witness(non_witness_utxo.vout[psbt.tx.input[i].prevout.n].scriptPubKey, i)
+                    }
+                } else {
+                    if let Some(txid_expected) = txid_expected {
+                        return Err(Error::InputError(InputError::InvalidUTXO{txid_expected,
+                            tx_provided: non_witness_utxo }, index));
+                    } else {
+                        return Err(Error::InputError(InputError::InvalidUnsignedTx{tx_provided: non_witness_utxo}, index));
+                    }
+                }
+            } else if let Some(witness_utxo) = input.witness_utxo {
+                let script = if let Some(redeem_script) = input.redeem_script {
+                    if witness_utxo.script_pubkey != redeem_script.to_p2sh() {
+                        Err(InputError::InvalidRedeemScript{redeem: redeem_script, p2sh_expected: witness_utxo.script_pubkey})
+                    } else {
+                        Ok(redeem_script)
+                    }
+                } else {
+                    Ok(witness_utxo.script_pubkey)
+                }.map_err(|e| Error::InputError(e, index))?;
+                if script.is_v0_p2wpkh() {
+                    // sign_witness(script[2:22], index)
+                } else if script.is_v0_p2wsh() {
+                    if let Some(witness_script) = input.witness_script {
+                        if script == witness_script.to_v0_p2wsh() {
+                            // sign_witness(witnessScript, i)
+                        } else {
+                            return Err(Error::InputError(InputError::InvalidWitnessScript
+                                                         {
+                                                             witness_script: witness_script.clone(),
+                                                             p2wsh_expected: script.clone(),
+                                                         }, index));
+                        }
+                    } else {
+                        return Err(Error::InputError(InputError::MissingWitnessScript, index));
+                    }
+                } else if script.is_v1_p2tr() {
+                    // sign Tr
+                }
+            }
+        }
+        Ok(())
+    }
+}
+
diff --git a/src/psbt/mod.rs b/src/psbt/mod.rs
index 52b9e90..2265b4f 100644
--- a/src/psbt/mod.rs
+++ b/src/psbt/mod.rs
@@ -79,6 +79,13 @@ pub enum InputError {
         /// The (incorrect) signature
         sig: Vec<u8>,
     },
+    InvalidUTXO {
+        tx_provided: bitcoin::Transaction,
+        txid_expected: bitcoin::Txid, 
+    },
+    InvalidUnsignedTx {
+        tx_provided: bitcoin::Transaction,
+    },
     /// Pass through the underlying errors in miniscript
     MiniscriptError(super::Error),
     /// Missing redeem script for p2sh
@@ -132,6 +139,13 @@ pub enum Error {
 impl fmt::Display for InputError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
+            InputError::InvalidUTXO {
+                ref tx_provided,
+                ref txid_expected,
+            } => write!(f, "PSBT: Expected txid  {:x} got txn {:?}", txid_expected, tx_provided),
+            InputError::InvalidUnsignedTX {
+                ref tx_provided,
+            } => write!(f, "PSBT: incorrect unsigned txn {:?}", tx_provided),
             InputError::InvalidSignature {
                 ref pubkey,
                 ref sig,

apoelstra · 2022-03-12T15:01:26Z

@JeremyRubin it makes sense to me but it's not clear that it actually uses Miniscript -- could you just as easily propose this for rust-bitcoin?

sanket1729 · 2022-03-13T18:09:49Z

In principle, the signers need not understand anything about miniscript. As @apoelstra said, I think this is better suited for rust-bitcoin directly as the method you suggest really does not require knowledge of descriptors or miniscript.

In the psbt role workflow, at minimum, only updaters and finalizers need to understand miniscript. For completeness, I will list my understanding of psbt workflow should fit in rust-bitcoin ecosystem.

Creator: Does not need to understand miniscript. Only creates the unsigned transaction. Psbt::from_tx in rust-bitcoin
Updater: This role has two broad purposes.
- Look at the Blockchain and fill in all the previous output data. Users have to manually query the blockchain and fill in this information.
- Fill in information from the descriptor. Look at the descriptor and fill in information about redeem scripts, witness scripts, taproot tree information. Key origin/bip-32 information. This requires understanding descriptors and in scope of rust-miniscript. (And is implemented in this PR).
Signer: Technically does not understand anything about descriptors. It should look at BIP 32 paths, find out if can sign for it and then sign it. This can be implemented in rust-bitcoin with the method you suggested.
Combiner: does not understand anything. Implemented in rust-bitcoin.
Finalizer: Needs to understand descriptors and miniscript in order to construct the proper final witness. Implemented in rust-miniscript
Extractor: Does not need to understand miniscript/descriptors. Implemented in rust-bitcoin.

Adds support for finalizing individual inputs and an additional AP that does not mutuate the original Psbt, but returns a new one

apoelstra

ACK 472f803

I'm not sure that _mut is the correct way to name this sort of method variant, but I'm not sure it's not, so I'm ok with it.

The APIs unsigned_script_sig give a single script element push <elem>. But what Psbt redeem script, we need the script without the prefixing the op pushbytes opcode

sanket1729 · 2022-03-14T15:19:32Z

@apoelstra , sorry for revoking your ACK. I added two more commits to this PR: One is API modification and another is a bug fix caught by integration tests(in a final upcoming PR).

apoelstra

ACK b3f4c18

JeremyRubin · 2022-03-14T17:15:47Z

src/psbt/mod.rs

+        secp: &secp256k1::Secp256k1<C>,
+    ) -> Result<(), Vec<Error>>;
+
+    /// Same as [`PsbtExt::finalize_mut`], but does not mutate the input psbt and


doc issue: it does mutate the input, it owns it?

what's different here is that it's clear that unless you're the owner of the psbt, you don't clobber it and return the psbt either in the Ok or Err.

sanket1729 force-pushed the update_psbt_apis branch 2 times, most recently from 0c27a44 to 5072177 Compare March 9, 2022 10:20

sanket1729 mentioned this pull request Mar 9, 2022

7.0 RC1 release #292

Closed

sanket1729 force-pushed the update_psbt_apis branch 3 times, most recently from afdc2c7 to 27679de Compare March 9, 2022 14:12

sanket1729 marked this pull request as ready for review March 9, 2022 14:12

sanket1729 mentioned this pull request Mar 9, 2022

Fix the length of the taproot output to 34 bytes in script_is_v1_tr #304

Closed

apoelstra reviewed Mar 9, 2022

View reviewed changes

dr-orlovsky added this to In review in Taproot Mar 10, 2022

sanket1729 force-pushed the update_psbt_apis branch 3 times, most recently from f5efd00 to 815d012 Compare March 11, 2022 12:01

apoelstra reviewed Mar 11, 2022

View reviewed changes

sanket1729 force-pushed the update_psbt_apis branch from 815d012 to d331bf6 Compare March 13, 2022 16:56

sanket1729 mentioned this pull request Mar 13, 2022

Make Tr::spend_info public #307

Merged

sanket1729 added 5 commits March 13, 2022 11:37

Remove local function script_is_v1_p2tr

30fa409

Add more auto-derives to conversion error

0c64a70

Introduce PsbtExt trait

49fe1ca

Move input sighash type match checks into sanity_check

5fa86b2

Add support for finalize input

68cccb3

sanket1729 force-pushed the update_psbt_apis branch from d331bf6 to 9d995a4 Compare March 13, 2022 20:10

Update finalize API to return vector of errors

daa80b7

sanket1729 force-pushed the update_psbt_apis branch 3 times, most recently from b08938c to 6eefb03 Compare March 14, 2022 12:22

Update Psbt APIs with _mut and _input support

472f803

Adds support for finalizing individual inputs and an additional AP that does not mutuate the original Psbt, but returns a new one

sanket1729 force-pushed the update_psbt_apis branch from 6eefb03 to 472f803 Compare March 14, 2022 12:23

apoelstra previously approved these changes Mar 14, 2022

View reviewed changes

Taproot automation moved this from In review to Ready for merge Mar 14, 2022

sanket1729 added 2 commits March 14, 2022 08:16

Update psbt finalize_* APIs to consume self

e9b53e9

Correct redeem script/script code caluclation

b3f4c18

The APIs unsigned_script_sig give a single script element push <elem>. But what Psbt redeem script, we need the script without the prefixing the op pushbytes opcode

sanket1729 dismissed apoelstra’s stale review via b3f4c18 March 14, 2022 15:17

Taproot automation moved this from Ready for merge to In review Mar 14, 2022

apoelstra approved these changes Mar 14, 2022

View reviewed changes

Taproot automation moved this from In review to Ready for merge Mar 14, 2022

sanket1729 merged commit 6cd1fb6 into rust-bitcoin:master Mar 14, 2022

Taproot automation moved this from Ready for merge to Done Mar 14, 2022

JeremyRubin reviewed Mar 14, 2022

View reviewed changes

sanket1729 mentioned this pull request Apr 10, 2022

Upstream bdk sighash calculation code rust-bitcoin/rust-bitcoin#946

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Psbt APIs #305

Update Psbt APIs #305

sanket1729 commented Mar 8, 2022

apoelstra Mar 9, 2022

apoelstra Mar 9, 2022

sanket1729 Mar 10, 2022

apoelstra Mar 11, 2022

apoelstra Mar 9, 2022

apoelstra Mar 9, 2022

apoelstra Mar 9, 2022

apoelstra Mar 9, 2022

sanket1729 Mar 10, 2022

sanket1729 Mar 10, 2022

apoelstra Mar 11, 2022

apoelstra commented Mar 9, 2022

JeremyRubin commented Mar 9, 2022

apoelstra commented Mar 9, 2022

JeremyRubin commented Mar 9, 2022 •

edited

Loading

apoelstra commented Mar 9, 2022

JeremyRubin commented Mar 9, 2022

dr-orlovsky commented Mar 10, 2022

apoelstra commented Mar 10, 2022

JeremyRubin commented Mar 10, 2022

apoelstra commented Mar 10, 2022

JeremyRubin commented Mar 10, 2022

JeremyRubin commented Mar 10, 2022

apoelstra commented Mar 10, 2022

apoelstra Mar 11, 2022

JeremyRubin Mar 11, 2022

JeremyRubin commented Mar 12, 2022

apoelstra commented Mar 12, 2022

sanket1729 commented Mar 13, 2022

apoelstra left a comment

sanket1729 commented Mar 14, 2022

apoelstra left a comment

JeremyRubin Mar 14, 2022

Update Psbt APIs #305

Update Psbt APIs #305

Conversation

sanket1729 commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Mar 9, 2022

JeremyRubin commented Mar 9, 2022

apoelstra commented Mar 9, 2022

JeremyRubin commented Mar 9, 2022 • edited Loading

apoelstra commented Mar 9, 2022

JeremyRubin commented Mar 9, 2022

dr-orlovsky commented Mar 10, 2022

apoelstra commented Mar 10, 2022

JeremyRubin commented Mar 10, 2022

apoelstra commented Mar 10, 2022

JeremyRubin commented Mar 10, 2022

JeremyRubin commented Mar 10, 2022

apoelstra commented Mar 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyRubin commented Mar 12, 2022

apoelstra commented Mar 12, 2022

sanket1729 commented Mar 13, 2022

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 commented Mar 14, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyRubin commented Mar 9, 2022 •

edited

Loading