Make OutputType consistent with Descriptor and return it #15567

1. 
   Sjors commented at 11:47 AM on March 9, 2019: member

   This PR renames OutputType::LEGACY to OutputType::PKH, OutputType::P2SH_SEGWIT to OutputType::P2SH_WPKH and OutputType::BECH32 to OutputType::WPKH in order to be consistent with the terminology used in Output Descriptors and in order to more accurately describe what the current wallet uses.

   It introduces two new OutputTypes P2SH, P2SH_WSH and WSH to distinguish legacy P2SH from SegWit wrapped P2SH, and to distinguish SegWit Pay-to-Public-Key from Pay-to-Script-Hash.

   Finally it adds GetAddressType() to Descriptor, which returns the reintroduced (#12729) OutputType::NONE for descriptors that we currently can't convert to an address (pk(...), raw(), etc), those whose address we don't interpret (addr()) and the quirky combo().

   This allows a future descriptor based wallet (e.g. #15487) to find the correct descriptor for a given -addresstype and -changetype setting when generating new addresses. Similarly it allows importing the appropriate descriptor from a hardware wallet (e.g. #14912).

2. Add OutputType::NONE fbf86b82fc
3. Make OutputType consistent with Descriptor b2da4c74d8
4. fanquake added the label Refactoring on Mar 9, 2019
5. fanquake added the label Wallet on Mar 9, 2019
6. 
   DrahtBot commented at 12:40 PM on March 9, 2019: member

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #15452 (Replace CScriptID and CKeyID in CTxDestination with dedicated types by instagibbs)

   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.

7. Add OutputType P2SH, WSH and P2SH_WSH 7a5fe7f36a
8. Descriptor: add OutputType GetAddressType() d3585d157a
9. Sjors force-pushed on Mar 9, 2019
10. fanquake requested review from meshcollider on Mar 10, 2019
11. 
    meshcollider commented at 12:23 AM on March 10, 2019: contributor

    Concept ACK, will take a look :)

12. 
    ryanofsky commented at 5:06 PM on March 11, 2019: member

    It would be better to use Optional<OutputType> than to add an OutputType::NONE enum. There may be some contexts where it's valid for the output type to be unset, but in most places it's not valid, and using the optional type would do a better job communicating and enforcing this.

13. in src/outputtype.cpp:23 in b2da4c74d8 outdated

      19 | @@ -20,13 +20,13 @@ static const std::string OUTPUT_TYPE_STRING_BECH32 = "bech32";
      20 |  bool ParseOutputType(const std::string& type, OutputType& output_type)
      21 |  {
      22 |      if (type == OUTPUT_TYPE_STRING_LEGACY) {
      23 | -        output_type = OutputType::LEGACY;
      24 | +        output_type = OutputType::PKH;
    

    ---

    ---

    

    MarcoFalke commented at 5:19 PM on March 11, 2019:

    style-nit: in commit b2da4c74d891fe719b21411f692e3edd4b968c

    Do as scripted diff?

14. 
    sipa commented at 5:33 PM on March 11, 2019: member

    I don't understand the motivation for splitting up OutputTypes further.

    When you request a Bech32 address from the wallet, getting a P2WPKH or P2WSH are both fine, and which one depends on the context currently (getnewaddress vs addmultisigaddress). In a future descriptor-based design there should just be one default source for all Bech32 addresses - which can be either P2WPKH or P2WSH (which depends on the setup and spending policy you want, not on what you're requesting.

15. 
    Sjors commented at 5:15 PM on March 12, 2019: member

    @ryanofsky I also thought about using Optional. I can switch to that unless others see a problem. @sipa we current use OutputType for three different tasks

    1. distinguish between PKH, SH-WPKH and WPKH. That distinction is historical, because it happens to be the three different output types the wallet supports. Most other (non-multisig) wallets also use these three types.
    2. distinguish what we expect other wallets can handle. All existing wallets can send to both PKH and SH-WPKH, but not all wallets can send to WPKH.
    3. keep support for generating fresh PKH addresses

    I think we should introduce a new type for task (2): AddressType: bech32 or base58. Then there could be a simple mapping from OutputType to AddressType. For backwards compatibility all we need for OutputType is PKH, SH-WPKH and WPKH, so I could remove the others, although they're trivial.

    As you pointed out, for descriptor based wallets, we only need to care about AddressType. The wallet can link to one base58 and one bech32 descriptor. Those descriptors would be set upon wallet creation, by default they would be SH-WPKH (bech32) and SH-WPKH (base58). Someone can also create a legacy wallet with only PKH (base58) and no bech32. (this assumes that we encourage users to create multiple wallets if they want both PKH and SH-WPKH)

    For a descriptor wallet that relies on external keys, e.g. BIP44/49/84, we could fetch the BIP49 (base58) and BIP84 (bech32) derivation xpubs from the device.

    For legacy wallets we would keep using OutputType, and by making that more precise we hopefully avoid problems later.

16. 
    sipa commented at 5:46 PM on March 12, 2019: member

    @Sjors Not really. LEGACY refers to pkh or p2sh-multisig; P2SH_SEGWIT refers to p2sh-pwpkh or p2sh-p2wsh-multisig; BECH32 refers to p2wpkh or p2wsh-multisig.

    They indeed serve two different goals (a) selecting between bech32 support in the receiver or not and (b) choosing non-segwit outputs.

    I still don't see what in de codebase needs further differentiation between multisig or not etc.

17. 
    Sjors commented at 9:33 AM on March 13, 2019: member

    IIUC the only interaction the user has with OutputType is through getnewaddress and addmultisigaddress. So far I've only considered getnewaddress, where legacy refers to pkh.

    With addmultisigaddress the address_type argument would map to different output types, i.e. P2SH, P2SH_WSH and WSH. That's still compatible with the idea of a simple OutputType -> AddressType mapping in descriptor wallets,

    However, interpreting bitcoind -addresstype=... as OutputType::PKH (etc) makes no sense in that case, so I'd have to rethink this PR.

    I'm not advocating OutputType to be even more fine grained. E.g. MultisigDescriptor would have OutputType::WSH.

    An alternative could be to "deprecate" OutputType, i.e. only use it in the context of a legacy wallet. The bitcoind -address_type parameter could be ignored for descriptor wallets, and the address_type argument for getnewaddress could be constrained to bech32 and base58 for descriptor wallets. (addmultisigaddress is not needed for descriptor wallets)

    In that case this PR won't be necessary, but then I do have to find another solution for deciding which descriptor to import from a hardware wallet into a legacy wallet (#14912). Right now it considers the current address_type and then matches the descriptor string prefix. However it might be better to make that PR contingent on descriptor wallets anyway, because if the user switch -addresstype or calls getnewaddress with a different address type than during key import, they'll end up generating addresses on the wrong derivation path. That breaks compatibility with the manufacturer software wallets which assume BIP44/49/84. Descriptor wallets solve that problem, because each descriptor can have its own derivation path.

18. Sjors closed this on Mar 13, 2019

    ---

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

    I'm trying a different approach in #15590.

20. DrahtBot locked this on Dec 16, 2021

Review Requested
meshcollider