Miniscript tapscript support (Pr 2 of 3) #275

sanket1729 · 2021-10-21T11:42:42Z

Adds support for MultiA script fragment. Builds on top of #255 .

Do not merge this until we have a rust-bitcoin release

JeremyRubin · 2021-10-27T04:01:40Z

src/descriptor/sh.rs

+                    .expect("Size checked in Miniscript"))
+            }
+            ShInner::Ms(ref ms) => {
+                Ok(bitcoin::Address::p2sh(&ms.encode(), network)


why not propagate the error?

Agreed, it is much safer to do so. This does not rely on size check being the only error.

apoelstra · 2021-11-12T19:59:28Z

src/interpreter/mod.rs

@@ -189,34 +189,34 @@ impl<'txin> Interpreter<'txin> {
        unsigned_tx: &'a bitcoin::Transaction,
        input_idx: usize,
        amount: u64,
-    ) -> impl Fn(&bitcoin::PublicKey, BitcoinSig) -> bool + 'a {
+    ) -> Result<impl Fn(&bitcoin::PublicKey, BitcoinSig) -> bool + 'a, Error> {


In e458d98:

I think, rather than returning an error here, we should return a closure that always returns false. But I'm not sure.

maybe you want a closure that always returns a Result<bool, Error>?

This should return an error if the input_idx is out of bounds.

On related note, I think we need a major redesign of this for taproot support anyways. This is currently hacked together that supports segwitv0 and legacy sighash. For taproot, we need entire sets of prevouts, it may be worth considering redesigning this entire API (yet) again :P

apoelstra · 2021-11-12T20:10:00Z

src/miniscript/satisfy.rs

+                            // This a privacy issue, we are only selecting the first available
+                            // sigs. Incase pk at pos 1 is not selected, we know we did not have access to it
+                            // bitcoin core also implements the same logic for MULTISIG, so I am not bothering
+                            // permuting the sigs for now


In c05eb7c:

Ok to leave this comment here, but TBH I don't think there's any way we could eliminate this privacy leak. If the first signature is missing then the resulting witnesses will have a different distribution than if it's not ... which will become apparent over time.

Although, I guess, in practice users are unlikely to reuse keys in visible ways, so an adversary will have only a one-shot view of the signature distribution.

dr-orlovsky

utACK mod some code style comments and proposal to re-use types which will be introduced in rust-bitcoin (once they got merged).

Also think that Tap context should be renamed to TapScript, since taproot may have more scripts in the future and Tap context requirements cover only leaf hash version 0xC0, but not other future versions with their own contexts

dr-orlovsky · 2021-11-14T08:09:46Z

src/miniscript/astelem.rs

+                if frag_name == "multi" {
+                    pks.map(|pks| Terminal::Multi(k, pks))
+                } else {
+                    // must be multi_a
+                    pks.map(|pks| Terminal::MultiA(k, pks))
+                }


I do believe this should be match; otherwise with future TapLeaf version-defined post-TapScripts we will not have a compiler error here

There is a match check that frag_name is either multi or multi_a. I don't see an issue with doing an if-else here.

("multi", n) | ("multi_a", n) => { // code.. if frag_name == "multi" { pks.map(|pks| Terminal::Multi(k, pks)) } else { // must be multi_a pks.map(|pks| Terminal::MultiA(k, pks)) } }

src/miniscript/satisfy.rs

sanket1729 · 2021-12-26T01:15:19Z

Pushed another branch. Some portions of this PR were merged in rust-bitcoin, so the review here should be (slightly) easy. Since this depends on a specific from rust-bitcoin, it is not possible to run integration tests on these because the dependencies operate on rust-bitcoin release.

I think I should have considered this before merging the integration tests PR, but now we have to wait for a rust-bitcoincore-rpc release too.

sanket1729 · 2022-01-11T15:45:42Z

The last commit message has a description for why it was needed

Iter keys in `MultiA` Fix witness generation for `MultiA`

NoChecks context needs to know how to serialize the given public key or where to lookup for it's signature (schnorr or ecdsa). The simplest way to convey this information is to separate out NoChecks for ecdsa and schnorr. When using NoChecksEcdsa the associated type bitcoin::PublicKey or when using NoChecksSchnorr, the associated type XOnlyPublicKey should take care of it.

sanket1729 · 2022-01-12T01:28:47Z

Rebased on ready for review.

apoelstra

ACK 42e4c50

I'm a little unsure about the last commit - splitting NoChecks. In this PR, NoChecksSchnorr is not actually used and the interpreter continues to be ECDSA-only. It's not obvious how to taproot-enable the interpreter, so I trust you have a plan in mind that will make use of the new split NoChecks.

sanket1729 · 2022-01-13T21:31:20Z

In this PR, NoChecksSchnorr is not actually used and the interpreter continues to be ECDSA-only. It's not obvious how to taproot-enable the interpreter, so I trust you have a plan in mind that will make use of the new split NoChecks.

I do have a plan for it. Our test-cases actually fail if we don't have this. We do a fancy interpreter check after we finalize a psbt to see that everything is good before we spend it. In that we need to serialize a script, We need some way to figure out Miniscript<Pk, NoChecks> should serialize key as 33 byte or 32 byte one or which signature algorithm to use for verification. All other contexts have this information implicitly in them because of associated consensus key type.

This was also done partly to address #275 (comment) in this PR.

apoelstra · 2022-01-14T00:36:01Z

Ah, I forgot about the PSBT sanity check. Makes sense!

sanket1729 force-pushed the multi_a branch 3 times, most recently from b61d72a to b7961c4 Compare October 21, 2021 22:56

sanket1729 changed the title ~~Miniscript tapscript support Pr 2 of 3~~ Miniscript tapscript support (Pr 2 of 3) Oct 21, 2021

sanket1729 marked this pull request as ready for review October 21, 2021 23:05

sanket1729 force-pushed the multi_a branch 2 times, most recently from a1b1d09 to cc6786d Compare October 26, 2021 16:08

JeremyRubin reviewed Oct 27, 2021

View reviewed changes

sanket1729 changed the title ~~Miniscript tapscript support (Pr 2 of 3)~~ [DO NOT MERGE] Miniscript tapscript support (Pr 2 of 3) Oct 27, 2021

notmandatory mentioned this pull request Nov 2, 2021

Taproot / Schnorr bitcoindevkit/bdk#63

Closed

8 tasks

apoelstra reviewed Nov 12, 2021

View reviewed changes

dr-orlovsky reviewed Nov 14, 2021

View reviewed changes

sanket1729 mentioned this pull request Nov 16, 2021

Rename Tap context to TapScript #280

Closed

sanket1729 force-pushed the multi_a branch from cc6786d to ac46eb9 Compare November 21, 2021 23:10

sanket1729 force-pushed the multi_a branch 2 times, most recently from 6257229 to 6dc83da Compare December 26, 2021 01:05

sanket1729 force-pushed the multi_a branch from 6dc83da to d6ca7b9 Compare January 11, 2022 07:57

sanket1729 changed the title ~~[DO NOT MERGE] Miniscript tapscript support (Pr 2 of 3)~~ Miniscript tapscript support (Pr 2 of 3) Jan 11, 2022

sanket1729 force-pushed the multi_a branch 2 times, most recently from ed385fc to 7070067 Compare January 11, 2022 15:35

sanket1729 added 7 commits January 12, 2022 06:57

add multi_a

89e7cb3

Iter keys in `MultiA` Fix witness generation for `MultiA`

Use bitcoin::EcdsaSig from rust-bitcoin

e26d4e6

Use bitcoin::SchnorrSig type from rust-bitcoin

2c32c03

Change satisfier to support xonly sigs

1a7e779

Add satisfaction tests

f6b2536

Fix fuzzer crash while allocating keys for multi_a

e70e6a3

sanket1729 force-pushed the multi_a branch from 7070067 to 42e4c50 Compare January 12, 2022 01:27

apoelstra approved these changes Jan 12, 2022

View reviewed changes

sanket1729 merged commit b8d04ac into rust-bitcoin:master Jan 13, 2022

Miniscript tapscript support (Pr 2 of 3) #275

Miniscript tapscript support (Pr 2 of 3) #275

Uh oh!

Conversation

sanket1729 commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

sanket1729 Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

apoelstra Nov 12, 2021 • 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!

JeremyRubin Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

sanket1729 Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

apoelstra Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Nov 14, 2021

Choose a reason for hiding this comment

Uh oh!

sanket1729 Nov 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sanket1729 commented Dec 26, 2021

Uh oh!

sanket1729 commented Jan 11, 2022

Uh oh!

sanket1729 commented Jan 12, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented Jan 13, 2022

Uh oh!

apoelstra commented Jan 14, 2022

Uh oh!

Uh oh!

sanket1729 commented Oct 21, 2021 •

edited

Loading

apoelstra Nov 12, 2021 •

edited

Loading