Skip to content

Taproot-related data types#666

Closed
dr-orlovsky wants to merge 1 commit intorust-bitcoin:masterfrom
BP-WG:taproot/script
Closed

Taproot-related data types#666
dr-orlovsky wants to merge 1 commit intorust-bitcoin:masterfrom
BP-WG:taproot/script

Conversation

@dr-orlovsky
Copy link
Copy Markdown
Collaborator

This PR will introduce types required for Taproot API for output construction from tapscript and internal keys. If the approach will be concept ACK, I will work on the implementation (merklization of script data + using these types in address construction)

@dr-orlovsky dr-orlovsky added this to the 0.28.0 milestone Sep 27, 2021
@dr-orlovsky dr-orlovsky changed the title Taproot-related data types (looking for concept ACK) Taproot-related data types Sep 27, 2021
@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

Same CI error as in #646 is fuzztests unrelated to the code

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
pub struct ScriptedPubkey {
pub struct ScriptPubkey {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just naming this Taproot or Tap? ScriptedPubkey sounds strange.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is neither Taproot or Tap pubkey since this data type only covers variant of pubkey which has an associated script and does not cover self-tweaked single pubkey which can be used in taproot. I am also not happy with ScriptedPubkey name, but we need to somehow highlight that this is one of two options for source data in final taproot generation process from the internal key.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little more bikeshedding: TapScriptPubkey? (since it would only be used in the script path)

Fine with whatever name, after more thought: ScriptPubkey might be confused with scriptPubkey from BIPs

@GeneFerneau
Copy link
Copy Markdown

Concept ACK

Copy link
Copy Markdown
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dr-orlovsky, I have a more detailed implementation of the TapTree structure that I did for rust-miniscript. I can port it over the data structures to rust-bitcoin, but I feel that logic for Tree parsing, and construction utilities do belong there.

What do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaves themselves are not enough to construct a full tree. You also need to know how they are arranged.

I have a local implementation of TapTree structure for rust-miniscript that I can port to rust-bitcoin. I don't know if this belongs in rust-bitcoin or in rust-miniscript(descriptors).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just naming this Taproot or Tap? ScriptedPubkey sounds strange.

@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

dr-orlovsky commented Sep 28, 2021

@sanket1729 I also thought that constructing outputs with tapscript should belong to rust-miniscript, but lately changed my opinion.

Rust-bitcoin should clearly allow creation of tapscript-based transaction outputs, even if these outputs are not checked against validity rules (like if they have OP_CHECKMULTISIG opcode) - the same way it allows creation of SegWit outputs which can be incorrect. Otherwise we need to remove whole support for scripted outputs (including P2WSH) and move it to rust-miniscript.

Next, if we plan to split rust-bitcoin into multiple crates and have some sort of bitcoin_scripts moved/updated for that matter, all this type system will make even more sense in scripts rather than in miniscript. For my understanding even context objects checking script validity should not be a part of miniscript but rather should be a part of bitcoin. For my understanding, miniscript is a way to formalize script, but it should not be the only way to construct scripts (at least otherwise we will not have LN support and other old protocols from pre-miniscript era).

So yes, I think that it will be nice to have your tree script implementation to move here from miniscript (and, probably in the future, to scripts subcrate).

@dr-orlovsky dr-orlovsky mentioned this pull request Sep 29, 2021
20 tasks
@sanket1729
Copy link
Copy Markdown
Member

@dr-orlovsky, I have upcoming PR(coming in the near future) that would address creating TapTree from scripts with odds, computing script pubkeys, constructing/verifying Merkle proofs, control blocks etc. I think that will supersede this PR. Just keeping you in loop incase you are working more on this PR

@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

@sanket1729 Thank you for the update, I am mot working on this PR for now

@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

Closing in favor of #677

@dr-orlovsky dr-orlovsky closed this Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants