Taproot P2TR address #563

dr-orlovsky · 2021-01-30T20:42:16Z

Part of Taproot project https://github.com/orgs/rust-bitcoin/projects/3 and epic #503. Depends on #617

sgeisler · 2021-02-05T18:03:58Z

One thing to keep in mind here: you need the new bech32 version for the Display implementation for version>0 segwit addresses. So this is blocked on rust-bitcoin/rust-bech32#48.

dr-orlovsky · 2021-02-11T22:09:38Z

@apoelstra what do you think on refactoring Address type given the opportunity by Taproot to do a API breaking change? Why not make it Copy and instead of WitnessProgramm provide explicit variants for WPubkeyHash, WScriptHash and Taproot with fixed-length arrays? witness_program() can be a method returning &[u8] then. Each new witness version will probably require address change anyway (like taproot), so we are not creating more problems for the future by doing that.

clarkmoody · 2021-02-12T07:21:10Z

I've opened rust-bitcoin/rust-bech32#50 to add Bech32m support to the bech32 library.

sgeisler · 2021-02-12T12:21:27Z

Each new witness version will probably require address change anyway (like taproot)

Ideally that would not be the case. Bech32m was only necessary due to a bug in bech32. BIP173 also states

Version 0 witness addresses are always 42 or 62 characters, but implementations MUST allow the use of any version.

Which seems to indicate that we'd still need an "unknown" variant.

clarkmoody · 2021-02-18T04:14:17Z

Here is my take on implementation for Bech32m checksums, based on BIP-0350 and bech32 0.8.0.

dr-orlovsky · 2021-02-19T11:21:29Z

Thank you, @clarkmoody, your changes are merged

dr-orlovsky · 2021-02-22T12:56:36Z

Which seems to indicate that we'd still need an "unknown" variant.

@sgeisler not necessary. We are talking about addresses, not descriptors; and not each script can be represented by an address (payload). I think it's absolutely fine not to support future unknown address formats and not provide a full witness program for them: ppl needing this will be able to use a special descriptor. In return, we will have a copy'able type like

#[derive(Copy, Clone, Ord, PartialOrd, Eq, PartialEq, Debug)]
pub enum AddressPayload {
    PubkeyHash(PubkeyHash),
    ScriptHash(ScriptHash),
    WPubkeyHash(WPubkeyHash),
    WScriptHash(WScriptHash),
    Taproot(secp256k1::schnorrsig::PublicKey),
}

and unability to parse the address will be denoted by Option::None, as it is happens today for custom script pubkeys.

sgeisler · 2021-02-22T15:14:49Z

Imo "[…] but implementations MUST allow the use of any version." is intended to provide forwards compatibility. We don't need to understand a witness script to send money to it and future users shouldn't have to upgrade to send money to version >=2 witness addresses. That upgrading is necessary for sending funds to Taproot addresses is a very unfortunate mishap and not the norm.

dr-orlovsky · 2021-02-22T15:51:30Z

However you are sending funds to a scriptPubkey, not an address. So Bech32m have not broken the funds which were stent to witness v1 outputs, and not supporting parsing of an address does not implies preventing users from sending funds wherever they want to. Anyway, it's not that important so I'm fine if it is not needed in the library.

apoelstra · 2021-02-22T16:33:00Z

We absolutely should support versions >1. The whole point of bech32(m) is that you can support future versions.

Users send money to addresses, not scriptPubKeys.

clarkmoody · 2021-02-22T17:06:49Z

The main bitcoin repo has an enum variant for unknown witness version that it uses when classifying outputs.

enum class TxoutType {
    NONSTANDARD,
    // 'standard' transaction types:
    PUBKEY,
    PUBKEYHASH,
    SCRIPTHASH,
    MULTISIG,
    NULL_DATA, //!< unspendable OP_RETURN script that carries data
    WITNESS_V0_SCRIPTHASH,
    WITNESS_V0_KEYHASH,
    WITNESS_V1_TAPROOT,
    WITNESS_UNKNOWN, //!< Only for Witness versions not already defined above
};

stevenroose

LGTM it seems that this version does support segwit versions > 1, right?

I would suggest a follow-up PR to add an extra AddressType::UnknownSegwitVersion(SegwitVersion) or something.

dr-orlovsky · 2021-03-14T12:23:06Z

it seems that this version does support segwit versions > 1, right?

in which sense? We have enum values for all allowed versions >1

sgeisler

utACK, looks good from a high level, would be nice if someone (possibly myself, but later) could take a more detailed look.

sgeisler · 2021-03-14T12:54:41Z

in which sense?

That we can a) parse and format and b) send transactions to addresses of unknown witness version.

We have enum values for all allowed versions >1

Yes, this makes it possible.

sanket1729

Some nits and a bug where we don't check program length for taproot witness programs.

Also added another point for discussion about having a different type for tweaked keys. Curious to see feedback from other reviewers on it.

src/util/address.rs

sanket1729 · 2021-03-17T08:27:12Z

@dr-orlovsky if your concern is waiting for 2 reviewer ACKs again, we can also create a follow-up PR with the changes.

dr-orlovsky · 2021-03-21T14:16:45Z

Thanks @sanket1729, I will add commits to this PR. Just need more time to finish other stuff before switching to this one

dr-orlovsky · 2021-04-06T15:09:30Z

@sanket1729 addressed all of your suggestions in the new commit, aside from the first one (for which I gave rationale why I think the current code is better).

@stevenroose, @sgeisler now this will be pending your re-ACK

UPD: Review suggestions addressing #563 issues went into there (and this PR is rebased on top of them)

sgeisler

tACK 7c32c7f

Test log:

Apr 06 17:26:33.766  INFO testinator: Installing rust toolchain 'nightly'
Apr 06 17:26:36.389  INFO testinator: Installing rust toolchain 'stable'
Apr 06 17:26:36.643  INFO testinator: Installing rust toolchain '1.29.0'
Apr 06 17:26:36.873  INFO testinator: Preparing environment for rust nightly tests (9 configurations)
Apr 06 17:26:36.874  INFO testinator: Preparing environment for rust stable tests (8 configurations)
Apr 06 17:26:36.874  INFO testinator: Preparing environment for rust 1.29.0 tests (8 configurations)
Apr 06 17:27:44.850  INFO testinator: Running rust nightly tests in /tmp/rust-bitcoin-nightly.q0GCXPPVY41z/rust-bitcoin
Apr 06 17:27:44.854  INFO testinator: Running rust 1.29.0 tests in /tmp/rust-bitcoin-1.29.0.fJk1f8F3peCL/rust-bitcoin
Apr 06 17:27:44.854 DEBUG testinator: Generating lock file with rust=1.29.0
Apr 06 17:27:44.858  INFO testinator: Running rust stable tests in /tmp/rust-bitcoin-stable.ChE2yLlxWOgK/rust-bitcoin
Apr 06 17:27:46.656 DEBUG testinator: Pinning cc to 1.0.41
Apr 06 17:27:46.839 DEBUG testinator: Pinning serde to 1.0.98
Apr 06 17:27:47.030 DEBUG testinator: Pinning serde_derive to 1.0.98
Apr 06 17:27:47.305 DEBUG testinator: Pinning byteorder to 1.3.4
Apr 06 17:28:28.440  INFO testinator: Test rust=nightly, features=[secp-recovery] succeeded!
Apr 06 17:28:37.784  INFO testinator: Test rust=stable, features=[secp-recovery] succeeded!
Apr 06 17:29:03.816  INFO testinator: Test rust=1.29.0, features=[secp-recovery] succeeded!
Apr 06 17:29:13.877  INFO testinator: Test rust=nightly, features=[use-serde] succeeded!
Apr 06 17:29:33.909  INFO testinator: Test rust=stable, features=[use-serde] succeeded!
Apr 06 17:29:35.676  INFO testinator: Test rust=nightly, features=[base64] succeeded!
Apr 06 17:30:05.646  INFO testinator: Test rust=stable, features=[base64] succeeded!
Apr 06 17:30:08.549  INFO testinator: Test rust=nightly, features=[secp-lowmemory] succeeded!
Apr 06 17:30:29.381  INFO testinator: Test rust=nightly, features=[rand] succeeded!
Apr 06 17:30:36.467  INFO testinator: Test rust=stable, features=[secp-lowmemory] succeeded!
Apr 06 17:30:51.175  INFO testinator: Test rust=1.29.0, features=[use-serde] succeeded!
Apr 06 17:30:52.423  INFO testinator: Test rust=nightly, features=[unstable] succeeded!
Apr 06 17:31:09.924  INFO testinator: Test rust=stable, features=[rand] succeeded!
Apr 06 17:31:42.735  INFO testinator: Test rust=nightly, features=[bitcoinconsensus] succeeded!
Apr 06 17:32:01.278  INFO testinator: Test rust=1.29.0, features=[base64] succeeded!
Apr 06 17:32:07.630  INFO testinator: Test rust=nightly, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,unstable,bitcoinconsensus] succeeded!                                                                                                  
Apr 06 17:32:12.992  INFO testinator: Test rust=stable, features=[bitcoinconsensus] succeeded!
Apr 06 17:32:48.787  INFO testinator: Test rust=nightly, features=[] succeeded!
Apr 06 17:32:57.434  INFO testinator: Test rust=1.29.0, features=[secp-lowmemory] succeeded!
Apr 06 17:33:10.115  INFO testinator: Test rust=stable, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!                                                                                                            
Apr 06 17:33:51.636  INFO testinator: Test rust=1.29.0, features=[rand] succeeded!
Apr 06 17:33:51.638  INFO testinator: Test rust=stable, features=[] succeeded!
Apr 06 17:34:40.626  INFO testinator: Test rust=1.29.0, features=[bitcoinconsensus] succeeded!
Apr 06 17:35:28.786  INFO testinator: Test rust=1.29.0, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!                                                                                                            
Apr 06 17:36:10.203  INFO testinator: Test rust=1.29.0, features=[] succeeded!
Apr 06 17:36:14.164  INFO testinator: Fuzzing deserialize_script
Apr 06 17:37:25.152  INFO testinator: Successfully fuzzed deserialize_script
Apr 06 17:37:25.152  INFO testinator: Fuzzing uint128_fuzz
Apr 06 17:38:27.100  INFO testinator: Successfully fuzzed uint128_fuzz
Apr 06 17:38:27.100  INFO testinator: Fuzzing deserialize_amount
Apr 06 17:39:28.101  INFO testinator: Successfully fuzzed deserialize_amount
Apr 06 17:39:28.101  INFO testinator: Fuzzing deserialize_transaction
Apr 06 17:40:30.258  INFO testinator: Successfully fuzzed deserialize_transaction
Apr 06 17:40:30.258  INFO testinator: Fuzzing deser_net_msg
Apr 06 17:41:33.076  INFO testinator: Successfully fuzzed deser_net_msg
Apr 06 17:41:33.076  INFO testinator: Fuzzing deserialize_address
Apr 06 17:42:34.071  INFO testinator: Successfully fuzzed deserialize_address
Apr 06 17:42:34.071  INFO testinator: Fuzzing deserialize_block
Apr 06 17:43:36.235  INFO testinator: Successfully fuzzed deserialize_block
Apr 06 17:43:36.235  INFO testinator: Fuzzing outpoint_string
Apr 06 17:44:38.151  INFO testinator: Successfully fuzzed outpoint_string
Apr 06 17:44:38.151  INFO testinator: Fuzzing deserialize_psbt
Apr 06 17:45:41.146  INFO testinator: Successfully fuzzed deserialize_psbt

src/util/address.rs

sanket1729

ACK 3d23ac7

sgeisler

utACK 3d23ac7

I left a few nits, but none were blocking in the end imo. I could understand if you don't want to invalidate reviews. Haven't had much time for it recently :( hope it gets better

src/util/address.rs

sgeisler · 2021-09-16T20:53:27Z

src/util/address.rs

+                match (version, prog.len()) {
+                    (WitnessVersion::V0, 20) => Ok(AddressType::P2wpkh),
+                    (WitnessVersion::V0, 32) => Ok(AddressType::P2wsh),
+                    (WitnessVersion::V0, len) => Err(Error::InvalidWitnessProgramLength(len)),


Shouldn't this be InvalidSegwitV0ProgramLength (if we want to keep that type)?

Still relevant: why not InvalidSegwitV0ProgramLength? Seems to be made for exactly this case.

src/util/address.rs

dr-orlovsky · 2021-09-16T21:41:54Z

@sgeisler thank you for the review! We have two ACKs from maintainers and one ACK from community, so I will merge it for now. But anyway lets finish the discussion of your nits in the comments and I will do another PR addressing them

dr-orlovsky · 2021-09-16T21:42:44Z

Ok, we have version conflicts so re-review will be required anyway. I will address your nits with it - we just need to arrive to some end result with them

dr-orlovsky · 2021-09-17T07:03:37Z

Rebased. @sgeisler I see your point with the UnsupportedWitnessVersion error, however since it requires more discussion and not a part of Taproot-specific update, I propose to leave it for a separate PR. It seems it is directly related to #648 discussion and we have to decide there on the overall API for Address type guarantees

Kixunil

utACK

src/util/address.rs

Kixunil · 2021-09-17T12:11:03Z

src/util/address.rs

-            Error::InvalidWitnessProgramLength(l) => write!(f,
-                "the witness program must be between 2 and 40 bytes in length: length={}", l,
+            Error::InvalidWitnessProgramLength(len) => write!(f,
+                "the witness program must be between 2 and 40 bytes in length: length={}", len,


One more general thought on error design, not related to this PR specifically: something like serde::de::Visitor::expecting method or parse_arg::ParseArg::describe_type could be useful.

Not sure I entirely get what do you mean by that design

Separate what is expected from what is the error. The intention is mainly to allow nicer/more flexible formatting of error messages.

dr-orlovsky · 2021-09-19T06:48:26Z

@Kixunil my point is that solving this issue will require more discussion and must involve many more changes from other PRs and issues (like #648, #642, #643, #646). The primary focus of this PR is to enable P2TR addresses at the current master, and we've being discussing and working on this trivial task already for 8 months having more than a hundred comments.

So while I am definitely up for a much better API for addresses, such task is more generic that just adding support for P2TR proposed in this PR, and will require (potentially many more) month(s) of discussion – so I propose to finally merge this one and move forward, unlocking a way for Taproot-supporting release.

Kixunil · 2021-09-19T12:11:04Z

@dr-orlovsky Either I misclicked or GitHub have screwed up my comment, it was referencing line https://github.com/rust-bitcoin/rust-bitcoin/pull/563/files#diff-c55d0186f3d5990b5e223c1a8aa1bbe60988201c452dd13b3324e892bc33119eR88

I believe the documentation should say "but such address, while parsed, may return errors from some methods". (Parsing == converting string to internal representation)

This is unrelated to future designs as the documentation should reflect the state of things in code as written right now.

dr-orlovsky · 2021-09-20T06:45:59Z

@Kixunil updated both error docs and display with the most verbose form I was able to compose. Pls review

Kixunil · 2021-09-20T08:29:37Z

I mean, I specifically have issue with the word "parsing", which means converting string to internal representation. The literal parse() method is implemented on &str via FromStr but that impl does not return FutureWitnessVersion. There's no other parsing method. All other methods do various other processing, not parsing. So the way the doc is written implies that such error is returned from from_str/parse which is not the case. It's the other way around - parsing does not return that variant, but other methods do.

apoelstra · 2021-09-20T14:38:11Z

src/util/address.rs

+        version.bech32_variant()
+    }
+}
+


I don't like this commit. It seems to violate the spirit of From.

Is it because you find those types unrelated? That's what I feel but curious if you have another reason.

Yeah, I think that's what it is. It seems like this is changing, conceptually, what the data is.

apoelstra · 2021-09-20T14:42:50Z

src/util/address.rs

+                    (WitnessVersion::V0, 32) => Ok(AddressType::P2wsh),
+                    (WitnessVersion::V0, len) => Err(Error::InvalidWitnessProgramLength(len)),
+                    (WitnessVersion::V1, 32) => Ok(AddressType::P2tr),
+                    (version, length) => Err(Error::FutureWitnessVariant { version, program_len: length }),


Why is this an error? I don't like this commit either.

In the context of address type it does make sense (we don't know the address type) but maybe it enourages users to reject future versions, so should be a variant of AddressType instead?

@apoelstra you can't make it not a error unless we go further than just enabling P2TR addresses and will modify AddressType to support arbitrary future softforks - which will be a case for debate, already started in #648, #642, #643, #646...

Pls also see my explanations in #563 (comment)

Replace BIP-0173 test vectors with those in BIP-0350.

dr-orlovsky · 2021-09-21T11:11:58Z

@apoelstra, @Kixunil, I have removed both controversial commits and made this PR to be only about P2TR address support. So now:

no From<WitnessVersion> for bech32::Variant
address_type reverted to return Option and not Result, as was before

The support of future Witness versions is independent from P2TR addresses and will be addressed in a separate PR(s), together with the rest of required address refactoring (#648, #642, #643, #646...).

apoelstra

ACK c1991d7

sanket1729

crACK c1991d7

dr-orlovsky · 2021-09-25T14:09:51Z

@Kixunil eaiting for you approval to merge this. I have removed all controversial part which was not directly related to P2TR address

Kixunil

Sorry for holding up, didn't make my mind when I saw your change and didn't know you want my review too.

ACK from me for this lighter change, I look forward into resolving the questions about invariants in the future.

dr-orlovsky added this to Ready for Review in Taproot Jan 30, 2021

dr-orlovsky mentioned this pull request Jan 30, 2021

Tracking issue for Taproot #503

Closed

20 tasks

dr-orlovsky moved this from Ready for Review to In review in Taproot Feb 6, 2021

dr-orlovsky added the API break This PR requires a version bump for the next release label Feb 19, 2021

stevenroose previously approved these changes Mar 13, 2021

View reviewed changes

Taproot automation moved this from In review to Ready for merge Mar 14, 2021

sgeisler previously approved these changes Mar 14, 2021

View reviewed changes

sgeisler mentioned this pull request Mar 14, 2021

WitnessVersion type #562

Closed

Taproot automation moved this from Ready for merge to In review Mar 14, 2021

sanket1729 requested changes Mar 14, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Show resolved Hide resolved

dr-orlovsky mentioned this pull request Apr 6, 2021

API for taproot tweaked keys #585

Closed

dr-orlovsky dismissed stale reviews from sgeisler and stevenroose via 71556d3 April 6, 2021 15:08

sgeisler previously approved these changes Apr 6, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

sanket1729 previously approved these changes Sep 14, 2021

View reviewed changes

Taproot automation moved this from In review to Ready for merge Sep 16, 2021

sgeisler previously approved these changes Sep 16, 2021

View reviewed changes

dr-orlovsky dismissed stale reviews from sgeisler and sanket1729 via 1767bcc September 17, 2021 07:02

Taproot automation moved this from Ready for merge to In review Sep 17, 2021

Kixunil previously approved these changes Sep 17, 2021

View reviewed changes

dr-orlovsky dismissed Kixunil’s stale review via 8d72ee3 September 17, 2021 13:14

apoelstra reviewed Sep 20, 2021

View reviewed changes

dr-orlovsky and others added 3 commits September 21, 2021 12:45

Taproot P2TR address

5573a54

Bump bech32 to 0.8.0 and use BIP-0350 Bech32m checksum

eeeb722

Replace BIP-0173 test vectors with those in BIP-0350.

Improving error information for address parser

c1991d7

apoelstra approved these changes Sep 24, 2021

View reviewed changes

sanket1729 approved these changes Sep 24, 2021

View reviewed changes

Taproot automation moved this from In review to Ready for merge Sep 24, 2021

Kixunil approved these changes Sep 25, 2021

View reviewed changes

dr-orlovsky merged commit e49cdbd into rust-bitcoin:master Sep 25, 2021

Taproot automation moved this from Ready for merge to Done Sep 25, 2021

sanket1729 mentioned this pull request Nov 3, 2021

Change Address::p2tr #687

Closed

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

Taproot P2TR address #563

Taproot P2TR address #563

Conversation

dr-orlovsky commented Jan 30, 2021 • edited

sgeisler commented Feb 5, 2021

dr-orlovsky commented Feb 11, 2021

clarkmoody commented Feb 12, 2021

sgeisler commented Feb 12, 2021 • edited

clarkmoody commented Feb 18, 2021

dr-orlovsky commented Feb 19, 2021

dr-orlovsky commented Feb 22, 2021 • edited

sgeisler commented Feb 22, 2021

dr-orlovsky commented Feb 22, 2021 • edited

apoelstra commented Feb 22, 2021

clarkmoody commented Feb 22, 2021

stevenroose left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Mar 14, 2021

sgeisler left a comment

Choose a reason for hiding this comment

sgeisler commented Mar 14, 2021

sanket1729 left a comment

Choose a reason for hiding this comment

sanket1729 commented Mar 17, 2021

dr-orlovsky commented Mar 21, 2021

dr-orlovsky commented Apr 6, 2021 • edited

sgeisler left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

sgeisler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky commented Sep 16, 2021

dr-orlovsky commented Sep 16, 2021

dr-orlovsky commented Sep 17, 2021

Kixunil left a 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

dr-orlovsky commented Sep 19, 2021 • edited

Kixunil commented Sep 19, 2021

dr-orlovsky commented Sep 20, 2021

Kixunil commented Sep 20, 2021

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

dr-orlovsky commented Sep 21, 2021 • edited

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Sep 25, 2021

Kixunil left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Jan 30, 2021 •

edited

sgeisler commented Feb 12, 2021 •

edited

dr-orlovsky commented Feb 22, 2021 •

edited

dr-orlovsky commented Feb 22, 2021 •

edited

dr-orlovsky commented Apr 6, 2021 •

edited

dr-orlovsky commented Sep 19, 2021 •

edited

dr-orlovsky commented Sep 21, 2021 •

edited