Descriptor: add GetAddressType() and IsSegWit()

Sjors commented at 12:19 pm on March 13, 2019: member

In preparation for descriptor based wallets (e.g. #15487) it’s useful to distinguish between base58 and base32 addresses. One descriptor could be associated with getnewaddress "" bech32 and another with getnewaddress "" legacy (which is interpreted as “base58”, rather than as non-SegWit).

This PR introduces a new enum AddressType, along with a parser that returns an Optional. A new method GetAddressType() is added to Descriptor instances. The only purpose of AddressType is compatibility with other wallets, not a way to toggle SegWit.

Reusing OutputType would not be ideal for this, because the distinction between OutputType::LEGACY (-addresstype=legacy) and OutputType::P2SH_SEGWIT (-addresstype=p2sh-segwit) is irrelevant in descriptor based wallets. Instead, if someone does not wish to use SegWit, they can create a wallet with only a non-SegWit descriptor.

This is also useful for importing keys from a hardware wallet. In the current WIP #14912 there is a method signerfetchkeys which asks the device (driver) to return a set of descriptors. This returns e.g. three descriptors: one for BIP44 (legacy), one for BIP49 (p2sh-segwit) and one for BIP84 (native segwit). It can then associate the BIP84 descriptor with getnewaddress "" bech32.

It can then use either the BIP44 or BIP49 descriptor with getnewaddress "" legacy. To distinguish those, this PR adds IsSegWit() to Descriptor instances.

The boost/optional/optional_io.hpp IO include allows for a more compact notation in the tests: BOOST_CHECK_EQUAL(ParseAddressType("bech32"), AddressType::BECH32).

Sjors force-pushed on Mar 13, 2019

fanquake added the label Refactoring on Mar 13, 2019

fanquake added the label Wallet on Mar 13, 2019

in src/addresstype.cpp:25 in 3d63978d58 outdated

20+const std::string& FormatAddressType(AddressType type)
21+{
22+    switch (type) {
23+    case AddressType::BASE58: return ADDRESS_TYPE_STRING_BASE58;
24+    case AddressType::BECH32: return ADDRESS_TYPE_STRING_BECH32;
25+    default: assert(false);

MarcoFalke commented at 1:07 pm on March 13, 2019:

Could remove the default case and move the assert after the switch, so that the compiler will issue a warning when the assert could be hit

Sjors commented at 9:57 am on March 14, 2019:

The compiler throws plenty of warnings even if I remove the assert:

Sjors force-pushed on Mar 14, 2019

Sjors commented at 10:21 am on March 14, 2019: member

The IsSegWit() check mentioned in the description can be found here, but I’ll hold off on a PR for now: https://github.com/Sjors/bitcoin/compare/2019/03/descriptor-address-type...Sjors:2019/03/descriptor-is-segwit

sipa commented at 10:03 pm on March 16, 2019: member

I don’t think this makes sense. The address type is a property of a scriptPubKey, not of a descriptor (in particular, it’s not well defined for combo descriptors).

You can already determine the address type of a scriptPubKey using ExtractDestination().

Sjors renamed this:
~~Descriptor: add GetAddressType() -> base58 / bech32~~
Descriptor: add GetAddressType() and IsSegWit()
on May 14, 2019

Sjors force-pushed on May 14, 2019

DrahtBot commented at 5:15 am on May 30, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18034 (Get the OutputType for a descriptor by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Aug 18, 2019

Sjors force-pushed on Aug 19, 2019

DrahtBot removed the label Needs rebase on Aug 19, 2019

Sjors force-pushed on Sep 4, 2019

DrahtBot added the label Needs rebase on Oct 8, 2019

[build] add IO support for Boost::Optional 25e015721e

Add AddressType (base58, bech32) a4e4604fb5

Descriptor: add GetAddressType() 8e5dbc5a87

Add IsSegWit() to Descriptor 73d62d8b5c

Sjors force-pushed on Nov 7, 2019

DrahtBot removed the label Needs rebase on Nov 7, 2019

meshcollider commented at 7:56 pm on November 22, 2019: contributor

@Sjors can you respond to sipa? I agree with him that this isn’t really a property of descriptors, but I haven’t looked into whether this makes your future work heaps easier or not. If not, let’s close this :)

Sjors commented at 8:38 am on November 23, 2019: member

I still use this inside #16546 for external signer support. However that PR is built on top of descriptor wallet support #15764 which is still in flux. So it’s probably more useful to discuss again when that’s ready.

The use case here is that we obtain descriptors from a hardware wallet, e.g. via the HWI getdescriptors call. This might be a mix of BIP44/49/84 descriptors. We then need to match those to the right output type in our wallet. That way if a user requests a legacy address, we give them a BIP44 derived address, vs. when they request a native segwit address, we’ll give them a BIP84 derived address.

In order to match the right descriptor with the the right output type we need to parse it.

I don’t think it makes sense to start with a descriptor string, derive a random destination, get its script and then analyse that script to figure out if it’s SegWit. The information is already in the descriptor string, so I think we should just use that.

meshcollider commented at 5:19 am on November 24, 2019: contributor

Perhaps in that case, it would be better to make a “CanProvideAddressTypeX” which takes a descriptor and an address type and returns a bool. Roughly provides the same functionality but makes more logical sense to me.

Can’t HWI just tell the wallet which descriptor it wants to use for which address type though?

Sjors commented at 9:13 am on November 24, 2019: member

The first suggestions seems doable, but not sure why it’s better. By address type you mean OutputType? It seems less flexible though, as in the longer run I don’t know if we really want to hold on to OutputType.

E.g. multisig would likely only use bech32 and wrapped segwit, not legacy p2sh or bare multisig. All we need is the new AddressType to distinguish those.

I can also imagine adding IsTaproot later, at which point our OutputType bech32 would be ambiguous.

I’d like to keep HWI universal, not too Core specific. The only universal way we currently have to describe an address type is a descriptor.

achow101 commented at 11:04 pm on January 30, 2020: member

What exactly is the utility of knowing whether the address type is base58 or bech32? I don’t think we have many things that depend on the encoding. Rather what we care about is the OutputType.

Sjors commented at 10:08 am on January 31, 2020: member

@achow101 my initial thinking was to stuff OutputType in the legacy box, along with its GetDestinationForKey(), AddAndGetDestinationForScript, etc, and switch to the simpler AddressType. However it’s still part of the ScriptPubKeyMan interface, so maybe not worth changing now.

When sharing an address, the distinction between base58 and bech32 is all that matters. So ideally I’d like to see a world where getnewaddress only takes a boolean argument to indicate the user needs base58. The GUI already behaves that way.

A standard wallet would have two descriptors instead of three: native segwit and wrapped segwit. If someone doesn’t want to use SegWit, they can create a wallet with a single P2PKH descriptor; such a wallet can only generate a base58 address.

achow101 commented at 6:11 pm on January 31, 2020: member

While it would be nice to just have the distinction be between base58 and bech32, I don’t think we are going to lose the “legacy” distinction any time soon. Besides the whole backwards compatibility thing, there are still holdouts who insist on legacy addresses.

This also gets more complicated with taproot which don’t allow for P2SH wrapped addresses. Even with just a base58 or bech32 address distinction, we would still need to distinguish between segwit v0 and segwit v1 addresses because inevitably someone will have implemented segwit incorrectly and can’t send to v1 addresses.

Sjors commented at 6:13 pm on January 31, 2020: member

there are still holdouts who insist on legacy addresses

Yikes, there are services that refuse to send to p2sh? Do you have an example?

inevitably someone will have implemented segwit incorrectly and can’t send to v1 addresses

Sadly that’s conceivable… Especially since early on there was a recommendation to only allow v0.

Sjors commented at 1:25 pm on February 10, 2020: member

Closing in favor of #18034

Sjors closed this on Feb 10, 2020

DrahtBot locked this on Feb 15, 2022

Descriptor: add GetAddressType() and IsSegWit() #15590

Conflicts