Add tr descriptor #278

sanket1729 · 2021-11-02T15:56:44Z

No description provided.

dr-orlovsky

Concept ACK. Left two API improvement suggestions

dr-orlovsky · 2021-11-14T08:57:14Z

src/descriptor/tr.rs

+    // A new leaf version would require a new Context, therefore there is no point
+    // in adding a LeafVersion with Leaf type here. All Miniscripts right now
+    // are of Leafversion::default
+    Leaf(Arc<Miniscript<Pk, Tap>>),


I think the problem may be solved by having a dedicated enum variant for each Leaf version. So this will become LeafTapScript, the enum will become #[non_exhaustive] and there will be no problem to add more contexts with more enum variants with post-0xC0 leafs

Yeah, marking everything non-exhaustive is a big step. I suppose we will have a big breaking as soon as the MSRV lands

dr-orlovsky · 2021-11-14T08:59:33Z

src/descriptor/bare.rs

@@ -63,6 +63,26 @@ impl<Pk: MiniscriptKey> Bare<Pk> {
    }
 }

+impl<Pk: MiniscriptKey + ToPublicKey> Bare<Pk> {


I think all this non-failing methods can be re-organized into an additional trait, let's say PreTaprootDescriptor: Descriptor, such that all pre-taproot descriptor types may be used as a generalized function arguments

We could do for the release. But I think that the effort is not worth it considering everything will go back to non-fallible once we have a new major breaking release of rust-secp that has static contexts

dr-orlovsky

Spotted one bug

dr-orlovsky · 2021-11-27T18:23:42Z

src/util.rs

+// This belongs in rust-bitcoin, make this upstream later
+pub(crate) fn script_is_v1_tr(s: &Script) -> bool {
+    let mut iter = s.instructions_minimal();
+    if s.len() != 32


This is bug: the key is 32 bytes indeed, but the script s 34.

Suggested change

if s.len() != 32

if s.len() != 34

dr-orlovsky · 2021-11-27T18:36:50Z

src/util.rs

+    let mut iter = s.instructions_minimal();
+    if s.len() != 32
+        || iter.next() != Some(Ok(Instruction::Op(opcodes::all::OP_PUSHNUM_1)))
+        || iter.next() != Some(Ok(Instruction::Op(opcodes::all::OP_PUSHBYTES_32)))


This does not work neither, I do not know why. But using spk.is_v1_p2tr() makes things working

LLFourn · 2022-01-15T03:02:22Z

src/descriptor/tr.rs

+        let spend_info = self
+            .spend_info
+            .as_ref()
+            .ok_or(Error::TaprootSpendInfoUnavialable)?;


This error case is the thing that forces us to make script_pubkey() return a Result right? Wouldn't it be preferable to make caching spend_info optional. So make a public API for caching it but if it hasn't been cached just compute it on the fly. For me, the risk of possibly doing some extra (usually minor) computation seems better than giving me a Result that I may arrogantly .unwrap().

This is not possible because we require a secp parameter to do the tweaking. Creating a new context inside a method is expensive and causes severe performance issues. This would go away once we get static verification tables in rust-secp, but still a while to go.

I am also unhappy with the current PR :( . Some rough thoughts,

I am considering more changes in the type system to distinguish between Derived and UnDerived descriptors. We would then implement DescriptorTriat only for Derived descriptors. Or perhaps having two traits.

I also don't like that address() fails for raw descriptors which are not that common. Maybe DescriptorTrait methods are not that useful.

I will make another candidate PR by tomorrow with the goal of avoiding unwraps() for common case like script_pubkey and address etc

sanket1729 · 2022-01-18T01:58:34Z

@apoelstra, the last commit is a suggested way to avoid calling .compute_spend_info everytime. The code naming/quality is not perfect, but this is an example design that would allow us to remove the TaprootCacheNotPresent error. It basically forces the user to call a function to convert Descriptor<Pk, Uncached> to Descriptor<Pk, Cached>.

It would be great if we can get rust-secp release with static contexts, then we don't have to worry about caching at all. In the interest of moving things forward, I will be separating the PR into two parts. Adding a 1) native Tr descriptor and 2) Integrating it with the DescriptorTrait.

LLFourn · 2022-01-31T06:56:34Z

hey @sanket1729 -- I've been testing out this branch. FWIW I found the Cached an Uncached distinction easy to work with. I just did the caching when going from abstract descriptor (e.g. with wildcards) to concrete descriptor. I think this allows you remove the Results on the trait but you just haven't gotten around to it yet?

One thing that I found to be inconvenient was that the DescriptorTrait was only implemented on Cached Descriptors but max_satisfaction_weight could theoretically be answered by an uncached descriptor as well (I think). So to figure out the max_satisfaction_weight of a more abstract descriptor I had to first make it concrete and then cache the spending info and then ask it its max satisfaction weight.

sanket1729 · 2022-02-01T22:15:12Z

Hey @LLFourn, good news is that we now would not even have to do the Cache part. Creating new verification-contexts would be cheap as secp new release (rust-bitcoin/rust-secp256k1#384 has support for static contexts). Eventually (maybe in the next release of a rust-bitcoin ecosystem cycle), we could rid of all verification context parameters altogether. Static Signing context is a bit more complicated, but fortunately, we don't really do it in this lib.

but you just haven't gotten around to it yet?

I stopped working on this, because the cheap context creation means that we should be back to the old API :)

sanket1729 · 2022-02-01T23:06:58Z

I did some benchmarking on the rust-bitcoin/rust-secp256k1#384 for the tweaking.

Currently

test benches::bench_context_create_and_tweak ... bench:   5,183,500 ns/iter (+/- 42,121)

After the update:

test benches::bench_context_create_and_tweak ... bench:      54,330 ns/iter (+/- 301)

Ideally (with full static context support), meaning no calls to context_create.

test benches::bench_context_create_and_tweak ... bench:      23,253 ns/iter (+/- 62)

A 100 times increase from 5ms to 50mirco s. I think (for now), it is acceptable to create a new context with a double overhead. This is a massive improvement over the previous 100 times overhead. This can be improved later when we have an API without secp context like xpk.tweak(&tweak). This allows us to have a cleaner API for now, with a performance hit at verification time.

I will be working on a follow-up PR where we use interior mutability to cache the spend_data so that only the first invocation would be costly. Subsequent invocations can use the cached value of SpendInfo

apoelstra · 2022-02-04T14:15:57Z

The 50us delay should also go away in the near(ish) future -- I believe it is caused by our current logic re-randomizing even static contexts on creation, which (a) ought to be done at compile-time, it doesn't use fresh randomness, and (b) doesn't need to be done at all for verification-only contexts.

See rust-bitcoin/rust-secp256k1#388 for further discussion

nickfarrow · 2022-02-08T07:38:57Z

I really like the removal of Cached and the unwraps. I've also been using this branch and found that the taproot spend_info reading from cache can hang. Idea for fix sanket1729#4

fixups to Tr descriptor code from SarcasticNastik

Co-authored-by: SarcasticNastik <aman.rojjha@research.iiit.ac.in>

Fix TapTree iter depth Fix tr spend_info lock * Attempt to read cache prior to matching on spend_info Option * This bug would cause function to hang * Also added test tr_script_pubkey Co-authored-by: nickfarrow <nicholas.w.farrow@gmail.com>

sanket1729 · 2022-02-08T10:43:43Z

Thanks @nickfarrow, merged, credited, and squashed your commit into 7eac908.

apoelstra · 2022-02-08T19:28:05Z

Done reviewing f5c8d7f

Overall looks great. A few comments, which I think should be addressed in future PRs:

I think we should split off a new struct RawTr from the Tr descriptor, and require that Tr have a script path (or hidden tree or whatever). The current situation has Tr::internal_key confusingly returning the external key in the case that there is no script path.
I think we should have a PreTaprootDescrtiptor and TaprootDescriptor trait which are subtraits of Descriptor and have additional methods, rather than making many of the existing methods fallible. Then the new ecdsa_sighash_script_code can just be called script_code again.
I'm confused by the rename of lookup_schnorr_sig to lookup_tap_leaf_script_sig. Also what if we made the signature type a type parameter of Satisfier then we could just use lookup_sig for both signature types?
I'm not thrilled with the new expression parsing code; it seems like we could reduce the duplicated code.

Finally, I have some fixups at https://github.com/apoelstra/rust-miniscript/tree/2022-02--278-fixups -- mostly minor stuff, but I also cleaned up the locking code.

sanket1729 · 2022-02-09T12:34:30Z

The current situation has Tr::internal_key confusingly returning the external key in the case that there is no script path.

I don't follow this. Every Tr descriptor must have an internal key. There is no confusion with the external_key(or output_key) here.

I think we should have a PreTaprootDescrtiptor and TaprootDescriptor trait which are subtraits of Descriptor and have additional methods, rather than making many of the existing methods fallible. Then the new ecdsa_sighash_script_code can just be called script_code again.

Agreed.

I'm confused by the rename of lookup_schnorr_sig to lookup_tap_leaf_script_sig. Also what if we made the signature type a type parameter of Satisfier then we could just use lookup_sig for both signature types?

The naming was mostly done to mimic the psbt naming. This is slightly more complicated because there are two different uses of Schnorr sigs. Overall, we have three main types of sigs:

Regular ECDSA sig lookup (pk -> ecdsa_sig)
Schnorr keyspend lookup (this does not have any notion of leaf_hash) (xonly_pk -> schnorr_sig)
Schnorr leaf sig that has key (xonly_key, leaf_hash) to value -> schnorr_sig

So, really need different types here :( because the function signatures vastly differ for key_spend and script_spend.

I'm not thrilled with the new expression parsing code; it seems like we could reduce the duplicated code.

I am also not happy with the current state of parsing. I will dedup it in a follow-up PR.

Finally, I have some fixups at https://github.com/apoelstra/rust-miniscript/tree/2022-02--278-fixups -- mostly minor stuff, but I also cleaned up the locking code.

Any special reason why you prefer .clone() on Arc instead of Arc::clone. All examples prefer in rust std docs the latter.

apoelstra · 2022-02-09T18:52:47Z

I don't follow this. Every Tr descriptor must have an internal key. There is no confusion with the external_key(or output_key) here.

I may be confused. What happens if you create a Tr with an internal key but None as the script tree?

Overall, we have three main types of sigs:

Can you elaborate on the difference between the latter two cases?

Any special reason why you prefer .clone() on Arc instead of Arc::clone. All examples prefer in rust std docs the latter.

No special reason. The docs on Arc say Some people prefer to use fully qualified syntax, while others prefer using method-call syntax. and this is how I've always done it. I'm happy to change it if you prefer the fully-qualified way.

sanket1729 · 2022-02-09T21:27:43Z

I may be confused. What happens if you create a Tr with an internal key but None as the script tree?

The internal key would return the internal key correctly. The script_pubkey()/spk() API calculation would calculate the output key as per BIP341.

Can you elaborate on the difference between the latter two cases?

One needs to know the leaf_hash under which the signature is applicable, the key spend one does not

No special reason. The docs on Arc say Some people prefer to use fully qualified syntax, while others prefer using method-call syntax. and this is how I've always done it. I'm happy to change it if you prefer the fully-qualified way.

I can do it in the follow-up PR. I like calling Arc::clone() because it clearly highlights that it is a cheap clone.

apoelstra · 2022-02-09T21:30:33Z

@sanket1729 ah! Ok, I missed the BIP341 logic. Alright, I'm happy.

If you cherry-pick my commits I'll ACK this. (Or I can change the Arc::clones if you'd like me to do that first)

apoelstra · 2022-02-09T21:48:55Z

@sanket1729 there are four extra commits on my branch

sanket1729 · 2022-02-09T21:54:48Z

@apoelstra, oops. Missed those

There are multiple `Tree`s in this file and the expression Tree is a somewhat obscure one. It is confusing to have it referenced without any qualification.

1. `RwLock` is typically much slower than `Mutex`, so the Rust developers discourage its use unless you really need lots of simultaneous readers. We do not, we only need to unlock this mutex for tiny amounts of time locking logic 2. The `clone` implementation missed an opportunity to clone an internal `Arc`, instead using `Mutex::new(None)` and forcing the clone to recompute the spend info. 3. Minor idiomatic cleanups.

apoelstra

ACK 7195bd7

Good to merge, I think.

sanket1729 · 2022-02-09T22:14:51Z

Awesome. Working on fixups based on this

2b13694 Add non-trivial multi-node example (Aman Rojjha) 1c2a80e Add validity and malleability checks. (Aman Rojjha) 0866807 Add P2Tr compiler (Aman Rojjha) 285207e Internal-key extraction done (Aman Rojjha) 26fc574 Policy to single-leaf TapTree compilation done (Aman Rojjha) Pull request description: This PR builds on top of #278. This is one in a series of PRs aimed to implement a feature-complete *Pay-to-Taproot* **P2Tr** compiler. Specifically, this introduces a basic compilation for a given policy by collecting the leaf nodes of the *tree* generated by considering root-level disjunctive `OR`and using this to generate a `TapTree`. > Assuming that _duplicate keys_ are **NOT** possible even in different branches of the TapTree. # Follow Up PRs - #342 - Uses heuristic for tree-generation/ _merging_ algorithm while buillding `TapTree` to optimize over the *expected average total cost*. - #352 - A future PR implementing enumerative strategies for optimizing `thresh` `k`-of-`n` and similar structures. ACKs for top commit: apoelstra: ACK 2b13694 sanket1729: ACK 2b13694 . Let's push forward on this. In the interest of ACKs and multiple rebases already done here, I am merging this. We recently shifted to rust 2018. And this code has a bunch of warnings because of it. Tree-SHA512: 4ceca51a383f5d52b572a16937bbcc3a9c53f0247e4b6df57a6547fd0b1c7cc33ff04dd9a476914bcf6d0a09e255918b8f7ebfe176c839d6ae31c84613dce67f

notmandatory mentioned this pull request Nov 2, 2021

Taproot / Schnorr bitcoindevkit/bdk#63

Closed

8 tasks

dr-orlovsky reviewed Nov 14, 2021

View reviewed changes

sanket1729 force-pushed the tr_descriptor branch from cbe744e to 9581f2e Compare November 21, 2021 23:01

dr-orlovsky suggested changes Nov 27, 2021

View reviewed changes

dr-orlovsky reviewed Nov 27, 2021

View reviewed changes

sanket1729 force-pushed the tr_descriptor branch 2 times, most recently from 1a63641 to 90fabf6 Compare December 27, 2021 22:10

sanket1729 mentioned this pull request Jan 11, 2022

Taproot descriptor #267

Closed

4 tasks

sanket1729 force-pushed the tr_descriptor branch from 90fabf6 to f11baaa Compare January 15, 2022 01:50

sanket1729 changed the title ~~[DO NOT MERGE] Add tr descriptor~~ Add tr descriptor Jan 15, 2022

sanket1729 force-pushed the tr_descriptor branch from f11baaa to 6d920ed Compare January 15, 2022 02:20

sanket1729 marked this pull request as ready for review January 15, 2022 02:20

LLFourn reviewed Jan 15, 2022

View reviewed changes

SarcasticNastik mentioned this pull request Jan 16, 2022

Taproot Compiler #291

Merged

sanket1729 marked this pull request as draft January 18, 2022 01:52

sanket1729 changed the title ~~Add tr descriptor~~ [DO NOT MERGE]Add tr descriptor Jan 18, 2022

sanket1729 force-pushed the tr_descriptor branch 2 times, most recently from f83f62b to abe64dd Compare February 4, 2022 12:26

sanket1729 force-pushed the tr_descriptor branch 3 times, most recently from e210332 to c81d9ab Compare February 6, 2022 02:53

sanket1729 force-pushed the tr_descriptor branch from 97bab62 to f3f37b0 Compare February 8, 2022 10:38

SarcasticNastik and others added 8 commits February 8, 2022 02:40

Implement Taproot descriptor tree parsing

6a717bf

fixups to Tr descriptor code from SarcasticNastik

Add roundtrip tests

81d1658

Co-authored-by: SarcasticNastik <aman.rojjha@research.iiit.ac.in>

Add iterator for taptree

a0ef37f

Update descriptor trait

74460e7

Update satisfy API for taproot support

dc21a79

Add Tr descriptor

48f6bb0

Fix TapTree iter depth Fix tr spend_info lock * Attempt to read cache prior to matching on spend_info Option * This bug would cause function to hang * Also added test tr_script_pubkey Co-authored-by: nickfarrow <nicholas.w.farrow@gmail.com>

Add psbt finalizer support

82ab565

implement satisfier for psbt

f5c8d7f

sanket1729 force-pushed the tr_descriptor branch from f3f37b0 to f5c8d7f Compare February 8, 2022 10:40

apoelstra added 4 commits February 9, 2022 13:55

replace length checks with is_empty

b1cb8e0

replace manual 'ch > 0x7f' checks with is_ascii

5b5bf46

qualify Tree symbol

954b12f

There are multiple `Tree`s in this file and the expression Tree is a somewhat obscure one. It is confusing to have it referenced without any qualification.

sanket1729 force-pushed the tr_descriptor branch from 170bc3e to 7195bd7 Compare February 9, 2022 21:55

apoelstra approved these changes Feb 9, 2022

View reviewed changes

sanket1729 merged commit f621bbf into rust-bitcoin:master Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tr descriptor #278

Add tr descriptor #278

sanket1729 commented Nov 2, 2021

dr-orlovsky left a comment

dr-orlovsky Nov 14, 2021

sanket1729 Jan 15, 2022

dr-orlovsky Nov 14, 2021

sanket1729 Jan 15, 2022

dr-orlovsky left a comment

dr-orlovsky Nov 27, 2021

dr-orlovsky Nov 27, 2021

LLFourn Jan 15, 2022 •

edited

Loading

sanket1729 Jan 15, 2022

sanket1729 commented Jan 18, 2022

LLFourn commented Jan 31, 2022 •

edited

Loading

sanket1729 commented Feb 1, 2022

sanket1729 commented Feb 1, 2022

apoelstra commented Feb 4, 2022 •

edited

Loading

nickfarrow commented Feb 8, 2022 •

edited

Loading

sanket1729 commented Feb 8, 2022

apoelstra commented Feb 8, 2022

sanket1729 commented Feb 9, 2022

apoelstra commented Feb 9, 2022

sanket1729 commented Feb 9, 2022 •

edited

Loading

apoelstra commented Feb 9, 2022

apoelstra commented Feb 9, 2022

sanket1729 commented Feb 9, 2022

apoelstra left a comment

sanket1729 commented Feb 9, 2022

Add tr descriptor #278

Add tr descriptor #278

Conversation

sanket1729 commented Nov 2, 2021

dr-orlovsky left a comment

Choose a reason for hiding this comment

dr-orlovsky Nov 14, 2021

Choose a reason for hiding this comment

sanket1729 Jan 15, 2022

Choose a reason for hiding this comment

dr-orlovsky Nov 14, 2021

Choose a reason for hiding this comment

sanket1729 Jan 15, 2022

Choose a reason for hiding this comment

dr-orlovsky left a comment

Choose a reason for hiding this comment

dr-orlovsky Nov 27, 2021

Choose a reason for hiding this comment

dr-orlovsky Nov 27, 2021

Choose a reason for hiding this comment

LLFourn Jan 15, 2022 • edited Loading

Choose a reason for hiding this comment

sanket1729 Jan 15, 2022

Choose a reason for hiding this comment

sanket1729 commented Jan 18, 2022

LLFourn commented Jan 31, 2022 • edited Loading

sanket1729 commented Feb 1, 2022

sanket1729 commented Feb 1, 2022

apoelstra commented Feb 4, 2022 • edited Loading

nickfarrow commented Feb 8, 2022 • edited Loading

sanket1729 commented Feb 8, 2022

apoelstra commented Feb 8, 2022

sanket1729 commented Feb 9, 2022

apoelstra commented Feb 9, 2022

sanket1729 commented Feb 9, 2022 • edited Loading

apoelstra commented Feb 9, 2022

apoelstra commented Feb 9, 2022

sanket1729 commented Feb 9, 2022

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 commented Feb 9, 2022

LLFourn Jan 15, 2022 •

edited

Loading

LLFourn commented Jan 31, 2022 •

edited

Loading

apoelstra commented Feb 4, 2022 •

edited

Loading

nickfarrow commented Feb 8, 2022 •

edited

Loading

sanket1729 commented Feb 9, 2022 •

edited

Loading