Bech32 addresses in dumpwallet #12315

fivepiece · 2018-01-31T17:10:53Z

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

fivepiece · 2018-01-31T17:22:23Z

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 · 2018-01-31T17:52:30Z

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 · 2018-01-31T18:07:41Z

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.

devrandom · 2018-02-01T01:15:28Z

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 · 2018-02-01T01:39:28Z

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 · 2018-02-01T01:44:22Z

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 · 2018-02-01T02:42:50Z

Thanks! I'll see what I can do.

devrandom · 2018-02-01T03:05:08Z

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 · 2018-02-01T03:32:54Z

@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 · 2018-02-01T06:44:37Z

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 · 2018-02-01T16:26:21Z

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

fivepiece · 2018-02-01T17:49:21Z

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

devrandom · 2018-02-02T01:46:47Z

src/wallet/rpcdump.cpp

-        std::string strAddr = EncodeDestination(keyid);
+        std::string strAddr;
+        std::string strLabel;
+        bool fLabelFound = false;


Can fLabelFound declaration be moved inside the if statement?

Yep, will do.

devrandom · 2018-02-02T01:48:59Z

src/wallet/rpcdump.cpp

+            if (!key.IsCompressed()) {
+                strAddr = EncodeDestination(keyid);
+                strLabel = EncodeDumpString(pwallet->mapAddressBook[keyid].name);
+                fLabelFound = true;


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

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

devrandom · 2018-02-02T02:33:52Z

test/functional/wallet_dump.py

+                            # containing multiple addresses if it is the expected p2wsh-p2wpkh
+                            # we added in the test
+                            if len(set(addr.split(","))) > 1 and addr.split(",")[1] == script_addrs[1]:
+                                found_addr += len(set(addr.split(",")))


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

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 · 2018-02-02T20:30:14Z

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

laanwj · 2018-02-06T12:31:32Z

src/wallet/rpcdump.cpp

        CKey key;
        if (pwallet->GetKey(keyid, key)) {
+            bool fLabelFound = false;
+            if (!key.IsCompressed()) {


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

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 · 2018-02-06T12:38:40Z

Concept ACK

fivepiece · 2018-02-06T19:33:54Z

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

sipa · 2018-02-06T19:36:58Z

src/wallet/rpcdump.cpp

+        CScript p2wpkhScr = GetScriptForDestination(WitnessV0KeyHash(keyid));
+        CScript p2shScr = GetScriptForDestination(CScriptID(p2wpkhScr));
+        ExtractDestination(p2wpkhScr, p2wpkhAddr);
+        ExtractDestination(p2shScr, p2shAddr);


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

That... makes a lot more sense :)

fivepiece · 2018-02-06T22:27:22Z

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.

sipa

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.

sipa · 2018-02-06T22:39:17Z

src/wallet/rpcdump.cpp

+    pwallet->GetKey(keyid, key);
+    for (const auto& dest : GetAllDestinationsForKey(key.GetPubKey())) {
+        if (pwallet->mapAddressBook.count(dest)) {
+            if (!strAddr.empty())


Style nit: braces when indenting.

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

fivepiece · 2018-02-06T23:04:55Z

One commit now

sipa · 2018-02-07T02:02:46Z

Lightly tested ACK 45eea40

meshcollider · 2018-02-07T21:50:16Z

LGTM, thanks for doing this :)
utACK 45eea40

laanwj · 2018-02-08T08:55:52Z

utACK 45eea40

45eea40 Bech32 addresses in dumpwallet (fivepiece) Pull request description: Output bech32 addresses in dumpwallet if address type is not as legacy Tree-SHA512: f6b6f788293779fe6339b94d9b792180e1d1dcb9c8e826caef8693557e1710213ba57891981c17505ace8d67b407eeca6fd9a8825757dd292cca2aa12575d15c

Output bech32 addresses in dumpwallet if address type is not as legacy Github-Pull: #12315 Rebased-From: 45eea40 Tree-SHA512: 3426292ddaeaafebc25fe84802011f5569a0cbb47fcc3209e7f00f64fc6eb1e637732722bbd02dce8d46be87d0f3687ce8370e71e9286bf7d00afc0a895faecb

Output bech32 addresses in dumpwallet if address type is not as legacy Github-Pull: bitcoin#12315 Rebased-From: 45eea40 Tree-SHA512: 3426292ddaeaafebc25fe84802011f5569a0cbb47fcc3209e7f00f64fc6eb1e637732722bbd02dce8d46be87d0f3687ce8370e71e9286bf7d00afc0a895faecb

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

devrandom reviewed Feb 2, 2018

View reviewed changes

sipa mentioned this pull request Feb 5, 2018

dumpprivkey and dumpwallet give different adress #12358

Closed

laanwj reviewed Feb 6, 2018

View reviewed changes

sipa reviewed Feb 6, 2018

View reviewed changes

Bech32 addresses in dumpwallet

45eea40

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

fivepiece force-pushed the dumpwallet-bech32 branch from b7f99e6 to 45eea40 Compare February 6, 2018 23:03

laanwj added this to the 0.16.0 milestone Feb 7, 2018

laanwj added the Needs backport label Feb 7, 2018

laanwj merged commit 45eea40 into bitcoin:master Feb 8, 2018

laanwj removed the Needs backport label Feb 8, 2018

str4d mentioned this pull request Mar 6, 2018

Add Sapling keys and addresses to the wallet zcash/zcash#3059

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Bech32 addresses in dumpwallet #12315

Bech32 addresses in dumpwallet #12315

Uh oh!

Conversation

fivepiece commented Jan 31, 2018

Uh oh!

fivepiece commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fivepiece commented Jan 31, 2018

Uh oh!

fivepiece commented Jan 31, 2018

Uh oh!

devrandom commented Feb 1, 2018

Uh oh!

fivepiece commented Feb 1, 2018

Uh oh!

sipa commented Feb 1, 2018

Uh oh!

fivepiece commented Feb 1, 2018

Uh oh!

devrandom commented Feb 1, 2018

Uh oh!

fivepiece commented Feb 1, 2018

Uh oh!

devrandom commented Feb 1, 2018

Uh oh!

fivepiece commented Feb 1, 2018

Uh oh!

fivepiece commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devrandom Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devrandom Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fivepiece commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 6, 2018

Uh oh!

fivepiece commented Feb 6, 2018

Uh oh!

sipa Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fivepiece commented Feb 6, 2018

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fivepiece commented Feb 6, 2018

Uh oh!

sipa commented Feb 7, 2018

Uh oh!

fivepiece commented Jan 31, 2018 •

edited

Loading

fivepiece commented Feb 1, 2018 •

edited

Loading

devrandom Feb 2, 2018 •

edited

Loading

devrandom Feb 2, 2018 •

edited

Loading

fivepiece commented Feb 2, 2018 •

edited

Loading

laanwj Feb 6, 2018 •

edited

Loading

sipa Feb 6, 2018 •

edited

Loading