Taproot interpreter support #301

sanket1729 · 2022-02-25T12:34:13Z

Add taproot interpreter support. This is the second to last PR for taproot miniscript support. The last PR will add some more high-level APIs for taproot psbt and integration tests.

apoelstra · 2022-02-27T22:32:48Z

src/interpreter/mod.rs

+    }
+}
+
+// While parsing we need to remember how to the hash was parsed so that we can


In 374ab9a:

so that we can what?

apoelstra · 2022-02-27T22:34:36Z

src/interpreter/inner.rs

@@ -211,7 +211,7 @@ pub(super) fn from_txdata<'txin>(
                // Key spend
                // let sig =
                // Tr inference to be done in future commit
-                todo!();
+                panic!("TODO");


In 277c783:

lol does this change anything?

Nope. Just allows compiling on MSRV :P . I was testing whether that particular commit compiles on rust 1.29

apoelstra

ACK 4e6352a

I have a single fixup commit in https://github.com/apoelstra/rust-miniscript/tree/2022-03--301-fixup which you can take or leave; it just cleans up one comment that I didn't really understand.

sanket1729 · 2022-03-07T13:13:46Z

Cherry-picked your commit

JeremyRubin · 2022-03-07T23:16:04Z

src/interpreter/inner.rs

    /// The script being evaluated is an actual script
-    Script(Miniscript<bitcoin::PublicKey, NoChecksEcdsa>, ScriptType),
+    Script(Miniscript<super::BitcoinKey, NoChecksEcdsa>, ScriptType),


When would we want a TR/XOnly with an ECDSA here?

In a later commit, the NoChecksEcdsa is replaced by NoChecks

JeremyRubin · 2022-03-07T23:18:35Z

src/interpreter/inner.rs

    spk: &bitcoin::Script,
    script_sig: &'txin bitcoin::Script,
    witness: &'txin Witness,
-) -> Result<(Inner, Stack<'txin>, bitcoin::Script), Error> {
+) -> Result<(Inner, Stack<'txin>, Option<bitcoin::Script>), Error> {


should the script_sig also be optional here then?

That should be provided as Script::new. Every segwti tx has an empty script sig instead of the None script sig.

JeremyRubin · 2022-03-07T23:20:50Z

src/interpreter/inner.rs

+                // Key spend
+                // let sig =
+                // Tr inference to be done in future commit
+                todo!();


n.b. might be better not to commit a todo! here for better bisecting sometime down the line?

Is this a comment about todo! not working on MSRV? Or that it might panic? If it is the latter, I think it is safe to do here because this is a new feature being added and no code path should reach here.
This also helps in making commits smaller/more atomic and easy to review.

not sure about that, I just meant it's nice if I want to test the git history against a taproot fragment that I can see where a bug in the interpreter gets added (e.g., like the bug I pointed out below about stack sizes would not be catchable in a bisect here).

JeremyRubin · 2022-03-07T23:24:49Z

src/interpreter/inner.rs

                    let script = miniscript.encode();
+                    let miniscript = pre_taproot_to_no_checks(&miniscript);


can you add a comment here?

JeremyRubin · 2022-03-07T23:25:07Z

src/interpreter/inner.rs

                                    let script = miniscript.encode();
+                                    let miniscript = pre_taproot_to_no_checks(&miniscript);


can you add a comment on this line? why

JeremyRubin · 2022-03-07T23:27:35Z

src/interpreter/inner.rs

+// Convert a miniscript from a specified context into an insane miniscript
+// We need to remember how the hash160 was translated because while doing a checksig
+// we need to know whether to parse the public key provided in witness as x-only or full
+pub(super) fn pre_taproot_to_no_checks<Ctx: ScriptContext>(


this function / its name is kinda confusing; maybe you can clarify the comment to discuss what the goal is (parse the public key provided in witness as x-only or full) and then what we do & why it accomplishes that?

Maybe you can also add a crate local trait IsPreTaproot to restrict what Ctx it can be called with.

or a trait crate local ToNoChecks and you can impl it default, and change just Tap CTX's impl?

The interpreter API operates on a NoChecks context because making it context-specific would involve lots of duplicate code for each context. The interpreter is for the use case when you already have a transaction and want to figure out what constraints were satisfied. The primary use-case for this feature was miniscript friendly block explorers that would output what constraints were satisfied for this given transaction. Hence, the insane scripts.

As for this PR, I have added a ToNoChecksTrait that makes the code look cleaner.

JeremyRubin · 2022-03-07T23:28:02Z

src/interpreter/inner.rs

+
+// Convert a miniscript from a specified context into an insane miniscript
+#[allow(dead_code)]
+pub(super) fn taproot_to_no_checks<Ctx: ScriptContext>(


why is this conversion insane? why do we do this?

Will add a comment elaborating this.

JeremyRubin · 2022-03-07T23:30:58Z

src/interpreter/mod.rs

+// - This does not implement ToPublicKey to avoid context dependant encoding/decoding of 33/32
+//   byte keys. This allows us to keep a single NoChecks context instead of a context for
+//   for NoChecksSchnorr/NoChecksEcdsa.
+// Long term TODO: There really should be any need for Miniscript<Pk: MiniscriptKey> struct


nit; typo missing "not be any need"

JeremyRubin · 2022-03-07T23:32:59Z

src/interpreter/mod.rs

+
+// While parsing we need to remember how to the hash was parsed so that we can
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+enum TaggedHash160 {


what does "tagged" mean in this context?

just that we track what kind of key?

May call it TypedHash because TaggedHash now means something else

Thanks, I had the same confusion!

JeremyRubin · 2022-03-07T23:39:38Z

src/interpreter/mod.rs

        &'iter self,
-        verify_sig: F,
-    ) -> Iter<'txin, 'iter, F> {
+        verify_sig: Box<dyn FnMut(&KeySigPair) -> bool + 'iter>,


i think you can make F: dyn FnMut... + 'iter without forcing it to be Boxed explicitly

JeremyRubin · 2022-03-07T23:44:45Z

looks pretty good to me!

JeremyRubin · 2022-03-08T01:21:34Z

n.b. rust-bitcoin/rust-bitcoin@04787d4 will seem to break some of the code using Prevouts, not clear if that's going into a 0.28 or not.

is this commit going into RC2?

JeremyRubin · 2022-03-08T01:57:57Z

src/interpreter/inner.rs

+                    // Our sighash structure and signature verification
+                    // does not support annex, return error
+                    return Err(Error::TapAnnexUnsupported);
+                } else if wit_stack.len() >= 2 {


this line is incorrect -- wit_stack can be 1 element (or in the case of CTV, empty!)

I think the confusion comes from the fact that the tap_script and ctrl_blk are popped off the wit_stack at this point.

Yup, the script spend must contain at least two witness elements that are being popped here. The additional witness elements can be 0

nope; I still think it's incorrect.

referencing: https://github.com/bitcoin/bitcoin/blob/64a4483dc6798a9a7d8327d320a17b3c4d7d4ee0/src/script/interpreter.cpp#L1919

if stack empty, fail

Check if there is 2 things and one of the things is an annex, remove it

If the stack is length 1 do keypath

If the stack is length 2 do script spend

Whereas your code says:

if stack empty, fail

If the length is 1 do keyspend

If the length is >= 2, pop the stack for CTRL and pop for tap script (stack length now 0 to N elemented):

If CTRL is actually an annex (0x50), fail.

Now control is actually a control, and script is a script for sure

check that there are >= 2 elements on the stack; if not fail

Execute the script

Clearly, the last step should not be happending! For example, the following witness fails:

<sig> <pk checksig> <ctrl>

because the script only needs one argument.

(the right number of arguments is zero)

Clearly, the last step should not be happending! For example, the following witness fails:

Are you missing adding an example?

If I am understanding this correctly, this will fail when the key spend has annex? To be clear, my only concern here is we should fail cleanly on the annex, and not the exact emulation of bitcoin consensus.

Right, the 0 is a bug, my integration tests are failing on it

Added a commit fixing this

lol -- example was there, but markdown was hiding it because it looked like html

sanket1729 · 2022-03-08T16:56:59Z

n.b. rust-bitcoin/rust-bitcoin@04787d4 will seem to break some of the code using Prevouts, not clear if that's going into a 0.28 or not.

I am not sure if it will be 0.28. It is a good to have for rust-miniscript, but I think we may have to wait 0.28.1

JeremyRubin · 2022-03-08T18:34:22Z

src/interpreter/inner.rs

+                    .last()
+                    .and_then(|x| x.as_push().ok())
+                    .map(|x| x.len() > 0 && x[0] == TAPROOT_ANNEX_PREFIX)
+                    .unwrap_or(false);


i think slice patterns with .. are not stable yet, but it'd be great if this could be done with pattern matches...

Is it rust 2021 :P ? We are still with 1.29 MSRV :(

https://adventures.michaelfbryan.com/posts/daily/slice-patterns/

1.26 for slice patterns, 1.42 for the "everything else .." matching within them.

so i think it's only really viable once we're at 1.42

JeremyRubin · 2022-03-08T18:34:47Z

src/interpreter/inner.rs

-                    // Annex is non-standard, bitcoin consensus rules ignore it.
-                    // Our sighash structure and signature verification
-                    // does not support annex, return error
-                    return Err(Error::TapAnnexUnsupported);


you should keep this, no?

Yes, indeed. Initially, I was thinking that we can just pop and ignore it. But as the comment says here, since we don't support signature verification with Annex, it is correct to Error

JeremyRubin · 2022-03-08T18:35:02Z

src/interpreter/inner.rs

+                    .map(|x| x.len() > 0 && x[0] == TAPROOT_ANNEX_PREFIX)
+                    .unwrap_or(false);
+                if has_annex {
+                    wit_stack.pop();


return error here.

JeremyRubin · 2022-03-08T18:44:19Z

src/interpreter/inner.rs

@@ -216,47 +228,36 @@ pub(super) fn from_txdata<'txin>(
                    None, // Tr script code None
                ))
            } else {
-                // wit_stack.len() >=2
-                // Check for annex
                let ctrl_blk = wit_stack.pop().ok_or(Error::UnexpectedStackEnd)?;


nit: mild preference to have an explicity match over wit_stack.len() for 0, 1, _ and return Error::UnexpectedStackEnd for 0, and then make a new error Errror::ImplementationInvariantBug or something for the pops here (so that we're not using expect or unwrap) -- for readability it makes it more clear to me.

Changed to match over len().

I don't see any expects or unwraps in this code? Where are we using those?

we're not, i was just saying that now the ok_or is guaranteed to succeed, so it might make sense to make it an assertion via unwrap or expect (since it should be unreachable) or to add a Error::ImplementationError instead of unwraps or UnexpectedStackEnd (because the user should know they didn't do submit something invalid if it ever hits).

JeremyRubin · 2022-03-08T22:02:11Z

src/interpreter/inner.rs

+                .map(|x| x.len() > 0 && x[0] == TAPROOT_ANNEX_PREFIX)
+                .unwrap_or(false);
+            let has_annex = has_annex && (wit_stack.len() >= 2);
+            if has_annex {


nit: may save a future developer some heart ache if you pop the annex still / comment that it should be popped. not required.

JeremyRubin · 2022-03-08T22:02:58Z

Verification of TR looks much cleaner now (dare I say correct?), thanks for the fixups.

sanket1729 · 2022-03-08T22:16:28Z

@apoelstra , ready for your review again

There are a few wierd changes that are waiting on upstream changes, but we should be good for 0.28.0 release. These changes would be caught by compiler after they are fixed upstream

I realize that introducing NoChecksEcdsa/NoChecksSchnorr was the wrong way to go. - Remove lifetimes from return types - Change script from Script to Option<Script> - Makes all the awkward hacks non-public - This makes the tests for internal non-exposed types really ackward with .into() etc, but I don't think this matters much as it is not a public API

This is an unituitive API that mutates interpreter for minor efficiency gains

This was a mistake. In the new API, we never decode/encode from NoChecks. We don't need a tag to know whether we want to parse 32/33 byte keys. We parse script with the correct context and then create another instance NoChecks

…preter

Rename TaggedHash160 -> TypedHash160

apoelstra

ACK be83ac7

b3f4c18 Correct redeem script/script code caluclation (sanket1729) e9b53e9 Update psbt finalize_* APIs to consume self (sanket1729) 472f803 Update Psbt APIs with _mut and _input support (sanket1729) daa80b7 Update finalize API to return vector of errors (sanket1729) 68cccb3 Add support for finalize input (sanket1729) 5fa86b2 Move input sighash type match checks into sanity_check (sanket1729) 49fe1ca Introduce PsbtExt trait (sanket1729) 0c64a70 Add more auto-derives to conversion error (sanket1729) 30fa409 Remove local function script_is_v1_p2tr (sanket1729) Pull request description: Based on #301 ACKs for top commit: apoelstra: ACK b3f4c18 Tree-SHA512: eaee7b50357c8dad94f11baf6f7116fe982c17b284f3d10f64d85cf4cd91b1c696d46f50b2f8b223dd5abd6d1243e77fa1f57f19a4253c78df747f90b423e3d9

sanket1729 force-pushed the tap_interpreter branch from 8e5defe to 685d374 Compare February 25, 2022 12:51

sanket1729 changed the title ~~Test CI fix~~ Taproot interpreter support Feb 26, 2022

apoelstra reviewed Feb 27, 2022

View reviewed changes

apoelstra previously approved these changes Mar 5, 2022

View reviewed changes

sanket1729 dismissed apoelstra’s stale review via a6b9eb4 March 7, 2022 13:13

sanket1729 force-pushed the tap_interpreter branch from 4e6352a to a6b9eb4 Compare March 7, 2022 13:13

JeremyRubin reviewed Mar 7, 2022

View reviewed changes

JeremyRubin reviewed Mar 8, 2022

View reviewed changes

sanket1729 force-pushed the tap_interpreter branch from 537f123 to cd2ae51 Compare March 8, 2022 22:00

JeremyRubin reviewed Mar 8, 2022

View reviewed changes

sanket1729 mentioned this pull request Mar 8, 2022

Update Psbt APIs #305

Merged

sanket1729 force-pushed the tap_interpreter branch from cd2ae51 to 4d112c9 Compare March 9, 2022 10:13

sanket1729 mentioned this pull request Mar 9, 2022

7.0 RC1 release #292

Closed

sanket1729 and others added 13 commits March 9, 2022 03:42

Update to 0.28.0-rc.1

73cd5a8

There are a few wierd changes that are waiting on upstream changes, but we should be good for 0.28.0 release. These changes would be caught by compiler after they are fixed upstream

Remove mut from Interpreter type for iterator

c28ac99

This is an unituitive API that mutates interpreter for minor efficiency gains

Overall Iter struct so that there is no closure

d3512bc

Add taproot Inner inference

21d182a

Interpreter sighash API support, no more closures

6c9e436

Remove NoChecksSchnorr

16d0830

This was a mistake. In the new API, we never decode/encode from NoChecks. We don't need a tag to know whether we want to parse 32/33 byte keys. We parse script with the correct context and then create another instance NoChecks

Cleanup iter APIs, retintroduce examples

a5b0b28

Update interpreter API in psbt

a9c7f41

Integration test fixups

be5a536

comments: clean up the comments around the "scriptcode" hack in Inter…

8ef682f

…preter

Add ToNoChecks trait

1929518

Rename TaggedHash160 -> TypedHash160

Update Control block verification

be83ac7

sanket1729 force-pushed the tap_interpreter branch from 4d112c9 to be83ac7 Compare March 9, 2022 11:42

apoelstra approved these changes Mar 9, 2022

View reviewed changes

sanket1729 merged commit 9d67260 into rust-bitcoin:master Mar 9, 2022

sanket1729 mentioned this pull request Mar 9, 2022

Remove fuzztaget and cleanup psbt finalizer #290

Merged

		let script = miniscript.encode();
		let miniscript = pre_taproot_to_no_checks(&miniscript);

Taproot interpreter support #301

Taproot interpreter support #301

Conversation

sanket1729 commented Feb 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 commented Mar 7, 2022

JeremyRubin Mar 7, 2022 • edited

Choose a reason for hiding this comment

sanket1729 Mar 8, 2022 • edited

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

sanket1729 Mar 8, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyRubin commented Mar 7, 2022

JeremyRubin 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

JeremyRubin Mar 8, 2022 • edited

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

sanket1729 Mar 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

JeremyRubin commented Mar 8, 2022 • edited

sanket1729 commented Mar 8, 2022

apoelstra left a comment

Choose a reason for hiding this comment

JeremyRubin Mar 7, 2022 •

edited

sanket1729 Mar 8, 2022 •

edited

sanket1729 Mar 8, 2022 •

edited

JeremyRubin Mar 8, 2022 •

edited

sanket1729 Mar 8, 2022 •

edited

JeremyRubin commented Mar 8, 2022 •

edited