Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Mar 13, 2019

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).

src/addresstype.cpp Outdated Show resolved Hide resolved
@Sjors Sjors force-pushed the 2019/03/descriptor-address-type branch from 3d63978 to 4371f3a Compare March 14, 2019 09:57
@Sjors
Copy link
Member Author

Sjors commented Mar 14, 2019

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

@sipa
Copy link
Member

sipa commented Mar 16, 2019

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 Sjors changed the title Descriptor: add GetAddressType() -> base58 / bech32 Descriptor: add GetAddressType() and IsSegWit() May 14, 2019
@Sjors Sjors force-pushed the 2019/03/descriptor-address-type branch 2 times, most recently from c73ff9f to 5efb75e Compare May 14, 2019 19:37
@DrahtBot
Copy link
Contributor

DrahtBot commented May 30, 2019

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.

@Sjors Sjors force-pushed the 2019/03/descriptor-address-type branch from 44a2e1f to 73d62d8 Compare November 7, 2019 19:06
@meshcollider
Copy link
Contributor

meshcollider commented Nov 22, 2019

@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
Copy link
Member Author

Sjors commented Nov 23, 2019

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
Copy link
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
Copy link
Member Author

Sjors commented Nov 24, 2019

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
Copy link
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
Copy link
Member Author

Sjors commented Jan 31, 2020

@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
Copy link
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
Copy link
Member Author

Sjors commented Jan 31, 2020

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
Copy link
Member Author

Sjors commented Feb 10, 2020

Closing in favor of #18034

@Sjors Sjors closed this Feb 10, 2020
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants