Adding taptree_of_horror example.

[miketwenty1]

Adding taptree_of_horror example.
Adding bitcoin_hashes as dev dependency.
Made some formatting consistency corrections in Cargo.toml.

[miketwenty1]

@apoelstra @sanket1729

Rebased the last commit into my PR. Looking for review/feedback. I laid out a few talking points for discussion so I can get an idea of how to get this merged in:

1. Does the setup of the example work for you?
2. Are you okay with my linter’s consistency edits to Cargo.toml?
3. Ok that I'm bringing in bitcoin_hashes as a dev-dep?
4. Would you prefer a themed example referencing TABConf 6's "Taptree of Horror," or a non-themed one? I was thinking themed, and eventually adding the review Youtube video and the Sats Conf video when they are ready https://agenda.satsconf.com.br/satsconf2024/talk/GQC8YK/ , if you would prefer non-themed, let me know what you're thinking for a example and folder name.
5. I included the .excalidraw file in case the tree graphic needed to be edited, but I can remove it if not needed.

[apoelstra]

• Are you okay with my linter’s consistency edits to Cargo.toml?

I'd rather not. It seems like you just need to add some lines to the bottom for the new example and that's it. (Also ironically, your new source files have some trailing spaces in them, so maybe these need to have a linter/formatter run on them.)

• Ok that I'm bringing in bitcoin_hashes as a dev-dep?

No, this doesn't seem necessary to me. I tried removing this and replacing bitcoin_hashes::Hash with bitcoin::hashes::Hash in your code and everything seemed to compile fine.

• Would you prefer a themed example referencing TABConf 6's "Taptree of Horror," or a non-themed one? I was thinking themed

Yeah, I like the theme.

[miketwenty1]

I'd rather not.

Fixed

(Also ironically, your new source files have some trailing spaces in them, so maybe these need to have a linter/formatter run on them.)

I'm not sure what you mean. Can you give me a specific example.

No, this doesn't seem necessary to me. I tried removing this and replacing bitcoin_hashes::Hash with bitcoin::hashes::Hash in your code and everything seemed to compile fine.

Fixed

Thanks for the feedback, fixed the issues you pointed. Let me know what else you'd like to see in this PR.

[apoelstra]

In 1282d30

Can replace all these with one-liners, e.g.

let [a0, a1, a2, a3, a4, a5, a6, a7, a8] = a_pks[..].try_into().unwrap();

I would also drop the Define individual variables with pattern matching comment since it's just a repetition of the code.

[miketwenty1]

fyi, I'm also be including the explicit type to make the compiler/linter happy
[PublicKey; KEYS_PER_PERSONA]

[apoelstra]

If you think that's necessary -- though I tried it without type annotations and it seemed to work. (Maybe you need the annotations once you convert all of them; I only did one.)

[miketwenty1]

I tried it again and removed the type for the "alice"/"a" keys and got this:

cargo +1.63.0 run --example taptree_of_horror --features compiler
   Compiling miniscript v12.2.0 (/Users/tzez/git/github/rust-bitcoin/rust-miniscript)
error[E0282]: type annotations needed
  --> examples/taptree_of_horror/taptree_of_horror.rs:88:9
   |
88 |     let [a0, a1, a2, a3, a4, a5, a6, a7, a8] = a_pks[..].try_into().unwrap();
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: type must be known at this point
help: consider giving this pattern a type
   |
88 |     let [a0, a1, a2, a3, a4, a5, a6, a7, a8]: _ = a_pks[..].try_into().unwrap();
   |                                             +++

For more information about this error, try `rustc --explain E0282`.
error: could not compile `miniscript` due to previous error

having the specific type solves this error, if you want to address it in a different way let me know.

[apoelstra]

Super cool! Done reviewing c1282d30653a3cb4e188e59d5a3dd8de909aa9477. Lots of comments but they are all super minor things.

[miketwenty1]

@apoelstra
I made the corrections you suggested, some notes:

• The new const KEYS_PER_PERSONA still has some try_into() ideas because some places it needs to be a usize and some places it needs to be a u32.
• There were a few places where the compiler/linter wanted me to define a specific type for some things. (just fyi if you were wondering why I defined some types).
• Decided to give the commented out code for internal keys full type paths incase someone is confused where it's coming from.
• I decided to also add in some header comments for the various section of the example.

// ====== 1. Setup Hardcoded Values for all of the Personas ======
// ====== 2. Derive Keys, Preimages, Hashes, and Timelocks for Policy and Signing ======
// ====== 3. Create Taptree Policy and Descriptor ======
// ====== 4. Construct an Unsigned Transaction from the Tapscript ======
// ====== 5. Sign and Create a Spending Transaction ======

Thanks for the first review, I'm ready for anything else you want to change.

[apoelstra]

89cdeca looks great to me! CI is showing a clippy lint you can address.

[apoelstra]

utack 08cea1c

[apoelstra]

utack 6910eb4

[apoelstra]

ACK 6910eb4; successfully ran local tests

[apoelstra]

cc @sanket1729 do you want to review this? I'll hold off on merging til tomorrow afternoon but I think this is good to go in.

[sanket1729]

ACK 6910eb4

[sanket1729]

Interesting, do we need explicit dependency for this? Do we not have a feature for this @apoelstra . Been some times since I looked at this.

[sanket1729]

Oops. I had a partial review from last week. Looks like this was resolved.

[sanket1729]

Did not a careful code review. The psbt APIs are used correctly and will serve as a good reference point for future users.

Maybe, we have a good first issue to simplify this example and create a simple psbt sign/update/finalize flow for a simpler policy :) .

[apoelstra]

We need to update contrib/test_vars.sh to include this example. (But really, we need to update CI to just read this stuff out of the toml file.)