Psbt: Add finalizer and extractor by sanket1729 · Pull Request #119 · rust-bitcoin/rust-miniscript

sanket1729 · 2020-08-05T09:59:27Z

Implemented psbt finalizer and extractor. Fixes #117 .

This PR also has some small refactors.

Decouples satisfaction API for reusing most code in psbt input and Tx input
Completed psbt finalizer. Miniscript primarily relies on three types of constraints: timelocks, hashlocks and sigs.

Timelocks: There is no special witness required from psbt. The transaction input contains sequence and transaction has the corresponding locktime.
Hashlocks: Awaiting on BIP174: add hash preimage fields to inputs bitcoin/bips#955 and to be merged, psbt support in rust-bitcoin.
Sigs: Partial sigs contain the data required for the finalizer.

Once the finalizer fills in the required fields, we double-check everything by running the interpreter and checking that there are no errors.
Similarly, before extracting we double-check everything with the interpreter.

The interpreter signature checks rely on sighash calculation from #116. Once the sighash functionality is added this PR can be updated to use it. Currently, this returns true. We can even merge this first and add the check in a followup PR once #116 is in.

apoelstra · 2020-08-16T12:58:08Z

src/miniscript/satisfy.rs

+pub fn bitcoinsig_from_rawsig(rawsig: &[u8]) -> Result<BitcoinSig, Error> {
+    let (flag, sig) = rawsig.split_last().unwrap();
+    let flag = bitcoin::SigHashType::from_u32(*flag as u32);
+    let sig = match secp256k1::Signature::from_der(sig) {


Will

let sig = secp256k1::Signature::from_der(sig)?;

work?

Yup, that should be cleaner. We already have a From implementation from the Secp error

apoelstra · 2020-08-16T13:33:01Z

src/psbt/mod.rs

+#[derive(Debug)]
+pub enum InputError {
+    /// Public key parsing error
+    InvalidPubkey { pubkey: Vec<u8> },


We should retain the secp256k1::Error as well here

apoelstra · 2020-08-16T13:33:52Z

src/psbt/mod.rs

+    /// Public key parsing error
+    InvalidPubkey { pubkey: Vec<u8> },
+    /// Redeem script does not match the p2sh hash
+    InvalidRedeemScript,


We should retain the expected and actual hashes here

apoelstra · 2020-08-16T13:34:03Z

src/psbt/mod.rs

+    /// Redeem script does not match the p2sh hash
+    InvalidRedeemScript,
+    /// Witness script does not match the p2wsh hash
+    InvalidWitnessScript,


We should retain the expected and actual hashes here

apoelstra · 2020-08-16T13:35:32Z

src/psbt/mod.rs

+            InputError::MissingWitness => write!(f, "PSBT is missing witness"),
+            InputError::MissingRedeemScript => write!(f, "PSBT is Redeem script"),
+            InputError::MissingUtxo => {
+                write!(f, "PSBT is missing both witness and non-witnes UTXO")


"witness" spelled wrong

dr-orlovsky · 2020-08-09T14:12:49Z

src/miniscript/satisfy.rs

 use Terminal;

 /// Type alias for a signature/hashtype pair
 pub type BitcoinSig = (secp256k1::Signature, bitcoin::SigHashType);


Shouldn't BitcoinSig type and bitcoinsig_from_rawsig function belong to rust-bitcoin, and not miniscript lib?

Conceptually yes. But I think along with that we would also want to add new APIs to use this type instead.

dr-orlovsky · 2020-08-09T14:14:55Z

src/miniscript/satisfy.rs

+/// Helper function to create BitcoinSig from Rawsig
+/// Useful for downstream when implementing Satisfier.
+/// Returns underlying secp if the Signature is not of correct formart
+pub fn bitcoinsig_from_rawsig(rawsig: &[u8]) -> Result<BitcoinSig, Error> {


It's a pity we do not have TryFrom in rust 1.22... Otherwise impl TryFrom<&[u8]> for BitcoinSig would look more native.

sgeisler · 2020-09-12T16:32:53Z

src/psbt/finalizer.rs

+    Ok(())
+}
+
+pub fn finalize(psbt: &mut Psbt) -> Result<(), super::Error> {


Mutably borrowing the PSBT will leave it in a potentially invalid state in case of an error. I'd rather consume and return the PSBT.

We do check the validity of the psbt before returning.

Somehow, it seems that allocating a new psbt when we are only adding two fields per input(final sciptsig and final witness) seems wrong.

While working on PSBTs in rust-bitcoin, I am not getting why finalize shouldn't be part of PartiallySignedTransaction structure with access to self
https://github.com/rust-bitcoin/rust-bitcoin/pull/472/files#diff-77e0d8cfeb8d4f84f6f901f6fc14139fR337

I think the method finalize could belong there, but rust-miniscript offers a way to construct the final witness and final scriptsig in a generic way by the satisfier trait.

We don't want to introduce a rust-miniscript dependancy to rust-bitcoin.

Maybe one way might be to have the method at both places, the generic finalizer in rust-miniscript and one method in rust-bitcoin that only fills in the required fields.

The more I think about this, I think there definitely should be a finalize method in rust-bitcoin that works for all scripts. Miniscript is more of helper to the finalizer. The job of a finalizer is to fill in the final values for witness and script_sig even if they are not miniscripts.

I think the API should be as follows:

rust-miniscript:

struct finalized_witness((Script, Vec<Vec<u8>>)); fn finalize_miniscript(&psb, S: Satisfier) -> Vec<finalized_witness> // Final script_sig and witness // Only works when all scripts used are miniscripts

This should also address @sgeisler concern about taking in mutable reference.
rust-bitcoin:

fn finalize(&mut self, final_witness: Vec<finalized_witness>) // This fills in the values in psbt.

cc @apoelstra

fn finalize(&mut self, witness: Vec<u8>, script_sig: Script)

Do I understand it correctly that this finalize method would work on per-input basis? Otherwise the witness/script sig had to be matched to an input which would make the finalization at least O(n^2). This also means that miniscript finalize had to return multiple (Script, Vec<u8>) tuples (one for each input).

EDIT: and isn't the witness Vec<Vec<u8>>?

Yup, my bad. Edited the original post. We can discuss the API even further. Maybe it should only work on a per input basis, but I wanted to share my opinion on how to the split between rust-miniscript's role and rust-bitcoin's role on finalizing.

Wrap errors from miniscript into psst

src/psbt/finalizer.rs

apoelstra · 2020-10-01T19:27:48Z

src/psbt/finalizer.rs

+            }
+            None => {
+                return Err(InputError::MissingPubkey);
+            }


more idiomatic (and readable) to move the return out of the match so it is clear there are no non-returning branches

return match partial_sig_contains_pk { Some((pk, _sig)) => Ok(Descriptor::Pkh(pk.to_owned())), None => Err(InputError::MissingPubkey), };

also every single return in this function could be removed since they appear at the end of the block (you need to change a couple ifs to else ifs). This would also be clearer since it emphasizes that this giant if statement is the only thing going on.

Removing all returns as it is the last statement

Rather, should I keep the short circuit return paths? Maybe those are more clear

I only saw one that I'd consider a "short-circuit return path", which is the one I complained about below because it was immediately followed by an .unwrap().

apoelstra · 2020-10-01T19:33:03Z

src/psbt/finalizer.rs

+        if inp.redeem_script.is_none() {
+            return Err(InputError::MissingRedeemScript);
+        }
+        let redeem_script = inp.redeem_script.as_ref().unwrap();


This is_none followed by unwrap should be replaced with a match (which will add an indentation level but will eliminate a panic case)

apoelstra · 2020-10-01T19:37:08Z

src/psbt/finalizer.rs

+        let script_pubkey =
+            get_scriptpubkey(psbt, index).map_err(|e| Error::InputError(e, index))?;
+        let empty_script_sig = Script::new();
+        let empty_witness = Vec::new();


nit: i would drop both of these variables, they are only used once and both have longer names than the expressions they replace :P

I think I need a let binding for the following?

fn_name(&Script::new())

apoelstra

c2b1fa7

sanket1729 force-pushed the psbt_fixes branch 2 times, most recently from 5dd1e1f to 1b8fbd2 Compare August 5, 2020 18:40

sanket1729 changed the title ~~Psbt fixes~~ Psbt: Add finalizer and extractor Aug 5, 2020

dr-orlovsky mentioned this pull request Aug 9, 2020

PSBT responsibilities API according to BIP-174 rust-bitcoin/rust-bitcoin#455

Closed

apoelstra reviewed Aug 16, 2020

View reviewed changes

sanket1729 force-pushed the psbt_fixes branch 5 times, most recently from b2e9d43 to c60f158 Compare August 28, 2020 23:28

sanket1729 mentioned this pull request Aug 31, 2020

Added hash Preimages to psbt rust-bitcoin/rust-bitcoin#465

Merged

dr-orlovsky reviewed Sep 6, 2020

View reviewed changes

sgeisler mentioned this pull request Sep 12, 2020

PSBT interface mockup rust-bitcoin/rust-bitcoin#457

Closed

6 tasks

sgeisler reviewed Sep 12, 2020

View reviewed changes

sanket1729 added 5 commits October 1, 2020 12:56

Helper for conversion of bitcoinsig from rawsig

70f5490

Decouple satisfaction API

8e7410b

Added from impl interpreter error

0fb11c0

Rework error types. Complete extractor

5c16f2f

Wrap errors from miniscript into psst

Added pkh lookup for psbt

a71025f

sanket1729 force-pushed the psbt_fixes branch from c60f158 to 3c58906 Compare October 1, 2020 18:16

sanket1729 mentioned this pull request Oct 1, 2020

Complete implementation of psbt Satisfier for hashlocks and timelocks #140

Closed

apoelstra reviewed Oct 1, 2020

View reviewed changes

src/psbt/finalizer.rs Outdated Show resolved Hide resolved

apoelstra reviewed Oct 1, 2020

View reviewed changes

src/psbt/finalizer.rs Outdated Show resolved Hide resolved

apoelstra reviewed Oct 1, 2020

View reviewed changes

sanket1729 force-pushed the psbt_fixes branch 2 times, most recently from 2c20d3d to 1a3db30 Compare October 1, 2020 20:02

sanket1729 added 2 commits October 1, 2020 15:07

Implemnented finalizer

099d68a

Added psbt example

c2b1fa7

sanket1729 force-pushed the psbt_fixes branch from 1a3db30 to c2b1fa7 Compare October 1, 2020 20:07

apoelstra approved these changes Oct 1, 2020

View reviewed changes

apoelstra merged commit eae1634 into rust-bitcoin:master Oct 1, 2020

sanket1729 mentioned this pull request Oct 1, 2020

Release 2.0.0 #143

Merged

sanket1729 mentioned this pull request Oct 13, 2020

Feature request: a PSBT stack (or at least a finalizer) #58

Closed

Conversation

sanket1729 commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 Sep 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgeisler Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sanket1729 commented Aug 5, 2020 •

edited

Loading

sanket1729 Sep 12, 2020 •

edited

Loading

sanket1729 Sep 14, 2020 •

edited

Loading

sgeisler Sep 14, 2020 •

edited

Loading