Bech32 addresses in dumpwallet #12315

fivepiece commented at 5:10 PM on January 31, 2018: contributor

Output bech32 addresses in dumpwallet if address type is not as legacy

fivepiece commented at 5:22 PM on January 31, 2018: contributor

p2sh-p2wpkh addresses along with the p2wpkh raw script are still visible in the scripts area in dumpwallet. importwallet behaves the same (will import all scriptpubkey types) I've set the address to be written in the dump to be [unknown address type] if neither legacy, p2sh-segwit or bech32 are set in the client, but I'm not sure if it could ever happen. In any case, it doesn't affect importwallet which will still import such dumps successfully.

fivepiece commented at 5:52 PM on January 31, 2018: contributor

Actually, this is not good enough. If an uncompressed privkey is being dumped, the address should be a p2pkh one regardless of the global setting.

fivepiece commented at 6:07 PM on January 31, 2018: contributor

Moved the decision as to which address we should output to after CKey key is set so we can check if it's compressed or not. A mixed output looks like :

# extended private masterkey: tprv8ZgxMBicQKsPe1mq9Pnix5Ui8nJAFhT4CTcMSK881Nyg1H6AF4zyASDK55NtwbG7mW811RXsLZpJCzN1wLDF8CN2urZprbTUL5inwqKNMFG

93VarZKnS5AGpxz86jLJ3amfXyVqY5eAjQ93noAshDYCbj55RW8 1970-01-01T00:00:01Z label= # addr=mzaNtEKZ62cqBUo3vNyYMURCzgqzFd9aGU
cNQYFxubhW4PhxubRZzDhPnaU3vFifU2XoohqfQyFuCSNcrwJsTQ 2018-01-31T18:00:52Z reserve=1 # addr=bcrt1qpm5uqjvw6mqmqgdanu7q248pppcpec9ascwhle hdkeypath=m/0'/0'/8'
cQp1biWyU45diaMQNpjgnHpVKutNMWJwwu1h9MrNRQLHVenAGu74 2018-01-31T18:00:52Z reserve=1 # addr=bcrt1qz5rqcfpzwh4hmugdrgjytwkv42xf9ymajuq9hq hdkeypath=m/0'/0'/0'

Segwit scriptpubkeys are not created for this uncompressed key on importwallet.

fanquake added the label RPC/REST/ZMQ on Feb 1, 2018

devrandom commented at 1:15 AM on February 1, 2018: none

I tested this by:

creating a wallet using v0.14.2, and generating one address
re-opening the wallet using bitcoind from this PR and dumping

The resulting dump has bech32 output for the old generated address. I assume this wasn't the intention?

Here are the dumps. With 0.14.2:

cNfTtkT9q15ozS2ESSVcTEXaZorQwrDkwioFZgniwrqVJzs6DFAJ 2018-02-01T00:08:58Z label= # addr=mv7rgSAEUbJe3gnAsEmdSZS8koSYwD2S7G hdkeypath=m/0'/0'/1'

and with this PR:

cNfTtkT9q15ozS2ESSVcTEXaZorQwrDkwioFZgniwrqVJzs6DFAJ 2018-02-01T00:08:58Z label= # addr=bcrt1q5q46yxf6gaelwlx8253py0qdyqzst32dtpf04e hdkeypath=m/0'/0'/1'

fivepiece commented at 1:39 AM on February 1, 2018: contributor

Right, I haven't identified a way of telling whether keys were added before or after the wallet upgrade for bech32. In this PR the output address type is only decided by what's set in the global setting. The only way to get the base58 addresses back is by changing the setting to addresstype=legacy. Maybe I missed some marker in the wallet that suggests the point of upgrade?

sipa commented at 1:44 AM on February 1, 2018: member

You can check which of the corresponding addresses has a label set. Typically this will only be one of the P2PKH, P2SH-P2WPKH, or P2WPKH ones. If so, you could just output that exact one. Unfortunately this won't work for imported keys, as they get the label assigned to all 3.

fivepiece commented at 2:42 AM on February 1, 2018: contributor

Thanks! I'll see what I can do.

devrandom commented at 3:05 AM on February 1, 2018: none

So perhaps the thing to do is to output any of the address types that have a label, and if none have a label output the global type. So up to three addresses per line.

fivepiece commented at 3:32 AM on February 1, 2018: contributor

@devrandom sorry the page didn't auto update with your comment. So you're saying that multiple addresses per entry is better? With this push, the first hit is used. I guess I see your point now though.

devrandom commented at 6:44 AM on February 1, 2018: none

Given Sipa's point, with imported keys you'd outputting the first address type in the if statement, not necessarily the type that the user is actually using or expecting.

fivepiece commented at 4:26 PM on February 1, 2018: contributor

(sorry for mass pushing, I'll push to a side branch for now)

fivepiece commented at 5:49 PM on February 1, 2018: contributor

I believe this push does things more along the lines that you guys suggested. A couple things :

If you set a label on an imported privkey, then try to set different labels to either its p2pkh or p2wpkh addresses, they will not show up in dumpwallet. The label set to the privkey will show up instead.
If you set a label on an imported privkey, then try to set a different label on its p2sh-p2wpkh address, its label will be used in the umpwallet entry.
Change addresses, even if generated by getrawchangeaddress will not retain their original address type, that's because they have no label.

Here's an example dump : https://gist.github.com/fivepiece/fe8e708264c390e0e943e54cec6d1162

You can see imported compressed and uncompressed keys and keys that were generated by getnewaddress retain their original type. Things like reserve , change and hdmaster (and other non labels) will have their output address set to the user's global setting. I've also added the case of outputting p2sh-p2wpkh addresses in the dump for non address book keys (in the switch statement) as the user might actually want to dump reserved keys as p2sh-p2wpkh.

In the gist, the line :

0014d6e506edcaf58910c27c4d8cbe94014552297808 1970-01-01T00:00:00Z script=1 # addr=2N1csGz3tHSBwXaLsHULSWj2PSphzoWvFPX

is a result of importpubkey with an compressed pubkey. It's the p2sh-p2wpkh script for it. I think this is new. It doesn't happen for importpubkey with an uncompressed pubkey.

I'll go over the tests now. It's getting an address entry that looks like mx5AyoDPJViVFaAu9VFMQ1MbwiUtTq8qJg,2MxskE9rmHDyK6pwdwM55ZFdAk3HtkQiwNW so ~it currently breaks.~ I added a change to the test runner for this case.

Cheers

in src/wallet/rpcdump.cpp:734 in 5baed68a31 outdated

 728 | @@ -729,12 +729,60 @@ UniValue dumpwallet(const JSONRPCRequest& request)
 729 |      for (std::vector<std::pair<int64_t, CKeyID> >::const_iterator it = vKeyBirth.begin(); it != vKeyBirth.end(); it++) {
 730 |          const CKeyID &keyid = it->second;
 731 |          std::string strTime = EncodeDumpTime(it->first);
 732 | -        std::string strAddr = EncodeDestination(keyid);
 733 | +        std::string strAddr;
 734 | +        std::string strLabel;
 735 | +        bool fLabelFound = false;

devrandom commented at 1:46 AM on February 2, 2018:

Can fLabelFound declaration be moved inside the if statement?

fivepiece commented at 2:07 PM on February 2, 2018:

Yep, will do.

in src/wallet/rpcdump.cpp:740 in 5baed68a31 outdated

 736 |          CKey key;
 737 |          if (pwallet->GetKey(keyid, key)) {
 738 | +            if (!key.IsCompressed()) {
 739 | +                strAddr = EncodeDestination(keyid);
 740 | +                strLabel = EncodeDumpString(pwallet->mapAddressBook[keyid].name);
 741 | +                fLabelFound = true;

devrandom commented at 1:48 AM on February 2, 2018:

Should this be set true only if it's found in address book?

fivepiece commented at 2:07 PM on February 2, 2018:

Ah good call. The uncompressed key might belong to an internal entry like change.

in test/functional/wallet_dump.py:55 in 5baed68a31 outdated

  52 | +                        if addrObj['address'] == addr.split(",")[0] and addrObj['hdkeypath'] == keypath and keytype == "label=":
  53 | +                            # ensure we only add the length of the set of addresses in an entry
  54 | +                            # containing multiple addresses if it is the expected p2wsh-p2wpkh
  55 | +                            # we added in the test
  56 | +                            if len(set(addr.split(","))) > 1 and addr.split(",")[1] == script_addrs[1]:
  57 | +                                found_addr += len(set(addr.split(",")))

devrandom commented at 2:33 AM on February 2, 2018:

this line seems to be unreached when this test is actually run

fivepiece commented at 2:09 PM on February 2, 2018:

Good catch :) the test was passing for because I broke it. I'll set the test to check for the p2sh-p2wpkh address specifically for the key it was added to.

fivepiece commented at 8:30 PM on February 2, 2018: contributor

~I think that I'm getting the wrong addresses for p2wpkh and probably p2sh-p2wpkh too. Looking into it.~ Nope, false alarm.

in src/wallet/rpcdump.cpp:737 in c844debcb1 outdated

 733 | +        std::string strAddr;
 734 | +        std::string strLabel;
 735 |          CKey key;
 736 |          if (pwallet->GetKey(keyid, key)) {
 737 | +            bool fLabelFound = false;
 738 | +            if (!key.IsCompressed()) {

laanwj commented at 12:31 PM on February 6, 2018:

Would make sense to encapsulate this (go from key/keyid to label,addr) out to a function.

fivepiece commented at 5:10 PM on February 6, 2018:

Thanks for having a look. I'm not sure what I should be passing to that function though, seems that I would have to pass a pointer to the wallet itself along with the keyid in order to know whether a label exists for an address type for that keyid. I might be missing something obvious though.

laanwj commented at 12:38 PM on February 6, 2018: member

Concept ACK

fivepiece commented at 7:33 PM on February 6, 2018: contributor

Moved things to a function per @laanwj review, I'm not sure if any locks or further wallet availability checks are needed though.

in src/wallet/rpcdump.cpp:91 in 03c04c609f outdated

  86 | +        CTxDestination p2wpkhAddr;
  87 | +        CTxDestination p2shAddr;
  88 | +        CScript p2wpkhScr = GetScriptForDestination(WitnessV0KeyHash(keyid));
  89 | +        CScript p2shScr = GetScriptForDestination(CScriptID(p2wpkhScr));
  90 | +        ExtractDestination(p2wpkhScr, p2wpkhAddr);
  91 | +        ExtractDestination(p2shScr, p2shAddr);

sipa commented at 7:36 PM on February 6, 2018:

You should iterate the result of GetAllDestinationsForKey(key) here instead of replicating the logic to compute the destinations.

fivepiece commented at 8:25 PM on February 6, 2018:

That... makes a lot more sense :)

fivepiece commented at 10:27 PM on February 6, 2018: contributor

Actually, per @sipa's suggestion to use GetAllDestinationsForKey() for the labeled addresses, we can use GetDestinationForKey(), requesting a g_address_type for the unlabeled ones and remove the switch-case stuff altogether.

in src/wallet/rpcdump.cpp:81 in b7f99e67a8 outdated

  76 | +    bool fLabelFound = false;
  77 | +    CKey key;
  78 | +    pwallet->GetKey(keyid, key);
  79 | +    for (const auto& dest : GetAllDestinationsForKey(key.GetPubKey())) {
  80 | +        if (pwallet->mapAddressBook.count(dest)) {
  81 | +            if (!strAddr.empty())

sipa commented at 10:39 PM on February 6, 2018:

Style nit: braces when indenting.

fivepiece commented at 11:03 PM on February 6, 2018:

Fixed

sipa commented at 10:53 PM on February 6, 2018: member

Lightly tested ACK, needs squashing.

This notably does not report the "correct" address for change addresses created through getrawchangeaddress or otherwise, though that's hard to deal with regardless as we don't keep a record of which type was used.

Bech32 addresses in dumpwallet

Output bech32 addresses in dumpwallet if address type is not as legacy

45eea40aa8

fivepiece force-pushed on Feb 6, 2018

fivepiece commented at 11:04 PM on February 6, 2018: contributor

One commit now

sipa commented at 2:02 AM on February 7, 2018: member

Lightly tested ACK 45eea40aa88f047111a9b1151fe4d1bad5c560e2

laanwj added this to the milestone 0.16.0 on Feb 7, 2018

laanwj added the label Needs backport on Feb 7, 2018

meshcollider commented at 9:50 PM on February 7, 2018: contributor

LGTM, thanks for doing this :) utACK https://github.com/bitcoin/bitcoin/pull/12315/commits/45eea40aa88f047111a9b1151fe4d1bad5c560e2

laanwj commented at 8:55 AM on February 8, 2018: member

utACK 45eea40

laanwj merged this on Feb 8, 2018

laanwj closed this on Feb 8, 2018

laanwj referenced this in commit ab4ee6e692 on Feb 8, 2018

laanwj referenced this in commit 758a41e100 on Feb 8, 2018

laanwj removed the label Needs backport on Feb 8, 2018

HashUnlimited referenced this in commit 4928002a8d on Mar 16, 2018

ccebrecos referenced this in commit 4ef0c4c8e0 on Sep 14, 2018

DrahtBot locked this on Sep 8, 2021