Add psbt BIP174 test vectors tests #935

DanGould · 2022-04-03T14:43:59Z

resolves #892

Initial patch adds OP_0 alias for OP_PUSHBYTES_0 as approved here
Second patch is the bulk of this work. It tests BIP 174 PSBT integration defined at The BIP's Test Vectors Section using Add API to PSBT to enable signing inputs #957
Third patch points users to rust-miniscript for a more useful finalizer

tcharding · 2022-04-03T22:16:41Z

what do you mean by "Clean up this error" on SignError?

I stole that error type, and a bunch of the code for signing PSBTs from the bdk project. By 'clean up' I meant remove all the variants that were not being used. Also, in general, all the error handling code is incomplete i.e., usage of expect throughout. I was planning on re-doing all that once everything worked.

One suggestion is to alias PartiallySignedTransaction as Psbt in rust-bitcoin

That's a PR you can do if you would like to. Conceptually simple but likely to get some argument/discussion if you like that sort of thing :) Otherwise I'll do it.

Cheers, will review.

EDIT: By the way, this should probably be a draft PR, its not ready for merge yet.

EDIT EDIT: So cool to wake up on Monday morning and find all your problems from last week solved! Thanks man.

tcharding

I'm going to push a commit to this branch @DanGould, want to take a look at it and squash it in if you agree with the changes?

tcharding · 2022-04-03T23:26:52Z

examples/psbt.rs

+        // OP_CHECKMULTISIG bug pops +1 value when evaluating. Need extra OP_0 byte in front
+        // Why is OP_0 called `OP_PUSHBYTES_0` in rust-bitcoin?


I did some digging, this is because in rust-bitcoin we map OP_1 - OP_16 to all::OP_PUSHNUM_0 - all::OP_PUSHNUM_16. Bitcoin Core does not define opcodes for the byte values 0x01 - 0x4b (at least not in enum opcodetype) but in rust-bitcoin we define all::OP_PUSHBYTES_1 - all::OP_PUSHBYTES_75, when these are serialized they are simply the byte values 0x01 - 0x4b. So, OP_0 could have been defined as OP_PUSHNUM_0 I guess but OP_PUSHBYTES_0 was chosen.

@apoelstra do you remember why this was done or why we did not explicitly have an OP_0 defined or aliased as we do for OP_FALSE?

This seem so obvious I feel like I'm missing something?

/// OP_0, the zero byte, the empty stack. pub static OP_ZERO: All = all::OP_PUSHBYTES_0;

I think it just didn't occur to me. I have never needed to push a zero to the stack, only FALSE 🤷

I'd be happy to accept a new alias.

tcharding · 2022-04-04T00:22:45Z

Nice work man! We have the seed passing issue to resolve still. Also, I'm starting to think that this should not be an example but rather should be an integration test. Then, the remaining TODOs are changes to rust-bitcoin. If you want to do them I'm happy to let you run with them :)

tcharding · 2022-04-04T00:32:57Z

Locally this builds fine but CI is including recent changes to the sighash types, no idea what is going on with that. Its your branch @DanGould so I won't mess with it but my blunt guess would be update your local master branch then rebase this and change SigHash -> Sighash and force push.

Draft because I'm stuck. We are currently working on PSBT workflow that uses the test vectors from BIP174, issue rust-bitcoin#892 describes the problem. PR rust-bitcoin#935 implements a solution using the test vectors but the code is cluttered because of all the hex strings, also the code does not really show how a user of `rust-bitcoin` would go about using the `psbt` module. I propose that rust-bitcoin#935 goes into `src/tests`, this PR adds `src/examples/psbt.rs`. Add an example PSBT workflow based on a cold-storage signer and a watch-only online wallet. The watch-only wallet is the Creator, Updater, and Finalizer. The cold-storage wallet is the Signer.

examples/psbt.rs

DanGould · 2022-04-05T06:51:02Z

I'm starting to think that this should not be an example but rather should be an integration test —@tcharding

concept ack to that and your psbt watch-only example as an example. I'll turn these TODOs into PRs.

examples/psbt.rs

tcharding · 2022-04-06T03:13:30Z

For your CI fixes you can check out #940 or

Use BTreeMap:new() and then call insert instead of using BTreeMap::from
Add an explicit scope around the immutable/mutable borrow issue (if you hit it)

DanGould · 2022-04-07T04:48:43Z

Why the heck aren't I seeing these errors when running act locally? I feel like a pleb pushing CI failing builds

tcharding · 2022-04-07T04:57:02Z

Don't beat yourself up about it, it took me months on this project to work out how to see the Rust 1.29 errors. I don't use act, in case it helps I can get these errors locally using

RUST_LOG=warn cargo-gen-lockfile-rust-1.29-bitcoin; RUST_LOG=warn cargo +1.29 run --example psbt

And the shell function

function cargo-gen-lockfile-rust-1.29-bitcoin {
    RUST_LOG=warn cargo clean
    rm Cargo.lock

    cargo +1.29 generate-lockfile --verbose && \
        cargo +1.29 update -p cc --precise "1.0.41" --verbose && \
        cargo +1.29 update --package "serde" --precise "1.0.98" --verbose && \
        cargo +1.29 update -p "serde_derive" --precise "1.0.98" --verbose

    export RUST_LOG=$saved_log
}

DanGould · 2022-04-07T05:26:40Z

@tcharding would it make sense to add this script to contributing.md? I wonder why this doesn't show up with act on its own. I get that this will change when we upgrade rust. this would have saved me 3 pushes at this point

tcharding · 2022-04-07T06:20:18Z

Yeah go for it.

tcharding · 2022-04-07T06:34:30Z

All green! I rekon this is pretty close to ready-for-review. I still rekon we should be asserting against PSBTs not against hex, do you have an opinion on that?

DanGould · 2022-04-07T07:51:34Z

We're asserting against PSBTs now 👍

Sincere thanks for all the guidance. I'm thrilled to be able to contribute :D

edit: if this is going to be an integration test it looks like we'll need a home for them.

tcharding · 2022-04-07T22:05:38Z

We're asserting against PSBTs now

There were a few missed, I pushed a wip patch, you can squash if you like it.

I also pushed a commit that moves the example to tests/ as is customary for Rust integration tests, run it with cargo test bip174_psbt_workflow.

tcharding · 2022-04-07T22:10:06Z

I'm thrilled to be able to contribute

I'm glad you are enjoying it, this is a mad project with good people working on it. I'm learning heaps hacking on this repo.

tcharding · 2022-04-07T22:12:09Z

Oh and incase you want to know why its in tests/: https://doc.rust-lang.org/rust-by-example/testing/integration_testing.html

DanGould · 2022-08-28T21:04:02Z

Now this uses the same signer as #940, is rebased on master, and satisfies the clippy gods.

I pair programmed the latest with @0xBEEFCAF3 and added him as a co-author.

tcharding

This looks good, a few trivial whitespace issues to fix.

With the whitespace fixes: ACK 210f5ec

src/blockdata/opcodes.rs

tests/psbt.rs

tcharding · 2022-08-30T01:22:10Z

tests/psbt.rs

+    psbt
+}
+
+/// Finalizes a PSBT accord to the Input Finalizer role described in BIP 174.


Perhaps we should comment here that finalizing can better be done with miniscript?

If we do we should add the same comment in examples/ecdsa-psbt.rs.

tcharding · 2022-08-30T01:31:51Z

tests/psbt.rs

+    let mut inputs = psbt.inputs.clone();
+
+    for (i, input) in inputs.iter_mut().enumerate() {
+        for sk in keys {
+            let mut required_pks = input.bip32_derivation.keys();
+            let pk = sk.public_key(secp);
+            if required_pks.any(|required| (*required) == pk.inner) {
+                psbt_sign::sign(&mut psbt, sk, i, secp).expect("failed to sign input with key");
+            }
+        }
+    }


I applied the patches from this PR to the sign PSBT (#957) PR and was able to implement GetKey for &[PriavateKey] based on this code. If/when both this PR and #957 merges this function can become

/// Signs `psbt` with `keys` if required. fn sign(mut psbt: Psbt, keys: &[PrivateKey]) -> Psbt { let secp = SECP256K1; psbt.sign(&keys, &secp); psbt }

which smells like success :)

This took a little massaging since &keys needs to be GetKeys. I think it works now

tcharding · 2022-08-30T01:36:11Z

Thanks for sticking at this one @DanGould!

tcharding · 2022-10-24T22:50:55Z

Hey @DanGould, now that the ecdsa-psbt module merged and the PSBT signing API improvements, this one should be able to merge. Have you time to check it over and rebase?

tcharding · 2022-10-24T22:51:53Z

I re-named the PR to remove the word "example" because this adds tests not example.

tcharding

I've said it before but this test is hot. All my suggestions are just nits though I'd like to see util removed from the import paths if that's ok.

ACK 421fe92

tcharding · 2022-10-26T00:50:38Z

bitcoin/tests/psbt.rs

+                    vout: input_0.index,
+                },
+                script_sig: Script::new(),
+                sequence: bitcoin::Sequence(0xFFFFFFFF), // Disable nSequence.


Suggested change

sequence: bitcoin::Sequence(0xFFFFFFFF), // Disable nSequence.

sequence: Sequence::MAX, // Disable nSequence.

tcharding · 2022-10-26T00:51:21Z

bitcoin/tests/psbt.rs

+
+    Transaction {
+        version: 2,
+        lock_time: bitcoin::absolute::PackedLockTime::ZERO,


nit: With the addition of use bitcoin::locktime::absolute;

Suggested change

lock_time: bitcoin::absolute::PackedLockTime::ZERO,

lock_time: absolute::PackedLockTime::ZERO,

tcharding · 2022-10-26T00:53:34Z

bitcoin/tests/psbt.rs

+use bitcoin::util::bip32::{ExtendedPrivKey, ExtendedPubKey, Fingerprint, IntoDerivationPath, KeySource};
+use bitcoin::util::psbt::{Psbt, PsbtSighashType};


nit: We are in the process of flattening util, would be better if the import path did not use util (both psbt and bip32 are in scope at the crate root).

tcharding · 2022-10-26T00:56:04Z

Will need to be rebased once #1345 goes in.

DanGould · 2022-10-26T01:50:27Z

added nits & rustfmt +nightly to use our fancy rustfmt.toml

tcharding

formatted, awesome. Thanks man.

ACK 5459c4f

(Rebase after we merge fix to master still to come.)

In preparation for adding integration tests in the standard Rust `tests/` directroy; move the contents of `test_data` to `tests/data`.

Implement Test Vectors from BIP 174 Co-authored-by: Tobin Harding <me@tobin.cc> Co-authored-by: Armin Sabouri <armins88@gmail.com>

DanGould · 2022-10-27T14:44:15Z

cherry-picked #1347 so we have a clean history and pass CI before @tcharding wakes up

tcharding · 2022-10-28T00:47:30Z

ACK b8bd31d

Thanks man!

sanket1729

ACK b8bd31d. Thanks for sticking with this PR. This looks awesome after #957

tcharding reviewed Apr 4, 2022

View reviewed changes

DanGould marked this pull request as draft April 4, 2022 04:25

tcharding mentioned this pull request Apr 5, 2022

Add psbt watch-only/cold-storage example #940

Merged

tcharding changed the title ~~add psbt example~~ Add psbt test vectors example Apr 5, 2022

DanGould force-pushed the psbt-example branch from 22224c3 to 0454b80 Compare April 5, 2022 06:43

DanGould commented Apr 5, 2022

View reviewed changes

examples/psbt.rs Outdated Show resolved Hide resolved

tcharding reviewed Apr 5, 2022

View reviewed changes

examples/psbt.rs Outdated Show resolved Hide resolved

examples/psbt.rs Outdated Show resolved Hide resolved

examples/psbt.rs Outdated Show resolved Hide resolved

examples/psbt.rs Outdated Show resolved Hide resolved

examples/psbt.rs Outdated Show resolved Hide resolved

DanGould force-pushed the psbt-example branch 2 times, most recently from dea0872 to 4bde605 Compare April 7, 2022 04:28

DanGould force-pushed the psbt-example branch from 4bde605 to 0302cf3 Compare April 7, 2022 05:32

DanGould force-pushed the psbt-example branch 2 times, most recently from 31da281 to eff0d9a Compare April 7, 2022 07:50

DanGould marked this pull request as ready for review April 7, 2022 07:53

DanGould force-pushed the psbt-example branch from ec1900e to bd8a3c3 Compare April 9, 2022 04:01

DanGould force-pushed the psbt-example branch from bd3e22e to 210f5ec Compare August 29, 2022 11:42

tcharding reviewed Aug 30, 2022

View reviewed changes

tcharding changed the title ~~Add psbt test vectors example~~ Add psbt BIP174 test vectors tests Oct 24, 2022

DanGould force-pushed the psbt-example branch 2 times, most recently from 62259a2 to 421fe92 Compare October 25, 2022 22:35

tcharding mentioned this pull request Oct 25, 2022

Move test_data/* to tests/data #1347

Closed

tcharding previously approved these changes Oct 26, 2022

View reviewed changes

DanGould dismissed tcharding’s stale review via 5459c4f October 26, 2022 01:49

DanGould force-pushed the psbt-example branch from 421fe92 to 5459c4f Compare October 26, 2022 01:49

tcharding previously approved these changes Oct 26, 2022

View reviewed changes

DanGould dismissed tcharding’s stale review via 3ad55bf October 27, 2022 13:49

DanGould force-pushed the psbt-example branch 2 times, most recently from 3ad55bf to 12714f8 Compare October 27, 2022 14:23

tcharding and others added 4 commits October 27, 2022 10:40

Move test_data/* tests/data

72935a0

In preparation for adding integration tests in the standard Rust `tests/` directroy; move the contents of `test_data` to `tests/data`.

Add OP_0 alias for OP_PUSHBYTES_0

6b6ef52

Test PSBT integration

16bf6f6

Implement Test Vectors from BIP 174 Co-authored-by: Tobin Harding <me@tobin.cc> Co-authored-by: Armin Sabouri <armins88@gmail.com>

Promote rust-miniscript finalizer

b8bd31d

DanGould force-pushed the psbt-example branch from 12714f8 to b8bd31d Compare October 27, 2022 14:43

tcharding approved these changes Oct 28, 2022

View reviewed changes

sanket1729 approved these changes Oct 28, 2022

View reviewed changes

sanket1729 merged commit 21ae79b into rust-bitcoin:master Oct 28, 2022

DanGould deleted the psbt-example branch October 28, 2022 13:57

		// OP_CHECKMULTISIG bug pops +1 value when evaluating. Need extra OP_0 byte in front
		// Why is OP_0 called `OP_PUSHBYTES_0` in rust-bitcoin?

	sequence: bitcoin::Sequence(0xFFFFFFFF), // Disable nSequence.
	sequence: Sequence::MAX, // Disable nSequence.

	lock_time: bitcoin::absolute::PackedLockTime::ZERO,
	lock_time: absolute::PackedLockTime::ZERO,

		use bitcoin::util::bip32::{ExtendedPrivKey, ExtendedPubKey, Fingerprint, IntoDerivationPath, KeySource};
		use bitcoin::util::psbt::{Psbt, PsbtSighashType};

Add psbt BIP174 test vectors tests #935

Add psbt BIP174 test vectors tests #935

Conversation

DanGould commented Apr 3, 2022 • edited

tcharding commented Apr 3, 2022 • edited

tcharding left a comment

Choose a reason for hiding this comment

tcharding Apr 3, 2022

Choose a reason for hiding this comment

apoelstra Apr 20, 2022

Choose a reason for hiding this comment

tcharding commented Apr 4, 2022

tcharding commented Apr 4, 2022

DanGould commented Apr 5, 2022

tcharding commented Apr 6, 2022

DanGould commented Apr 7, 2022

tcharding commented Apr 7, 2022

DanGould commented Apr 7, 2022

tcharding commented Apr 7, 2022

tcharding commented Apr 7, 2022

DanGould commented Apr 7, 2022 • edited

tcharding commented Apr 7, 2022

tcharding commented Apr 7, 2022

tcharding commented Apr 7, 2022

DanGould commented Aug 28, 2022 • edited

tcharding left a comment

Choose a reason for hiding this comment

tcharding Aug 30, 2022

Choose a reason for hiding this comment

tcharding Aug 30, 2022

Choose a reason for hiding this comment

tcharding Aug 30, 2022 • edited

Choose a reason for hiding this comment

DanGould Oct 25, 2022

Choose a reason for hiding this comment

tcharding commented Aug 30, 2022

tcharding commented Oct 24, 2022

tcharding commented Oct 24, 2022

tcharding left a comment • edited

Choose a reason for hiding this comment

tcharding Oct 26, 2022

Choose a reason for hiding this comment

tcharding Oct 26, 2022 • edited

Choose a reason for hiding this comment

tcharding Oct 26, 2022

Choose a reason for hiding this comment

tcharding commented Oct 26, 2022

DanGould commented Oct 26, 2022

tcharding left a comment

Choose a reason for hiding this comment

DanGould commented Oct 27, 2022

tcharding commented Oct 28, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

DanGould commented Apr 3, 2022 •

edited

tcharding commented Apr 3, 2022 •

edited

DanGould commented Apr 7, 2022 •

edited

DanGould commented Aug 28, 2022 •

edited

tcharding Aug 30, 2022 •

edited

tcharding left a comment •

edited

tcharding Oct 26, 2022 •

edited