RPC: creates possibility to preserve labels on importprivkey #13381

marcoagner · 2018-06-03T14:59:52Z

As discussed in the issue, this is a feature request instead of a bug report since the behaviour was as intended (i.e. label with default: ''). With this, the old behaviour is kept while the possibility to achieve the preservation of labels, as expected in the open issue, is added.

promag · 2018-06-03T16:30:46Z

There is no need for a new parameter, check if label is null instead - this means that the default is to preserve if it exists otherwise "".

promag · 2018-06-03T17:29:07Z

Concept ACK, a test would be nice.

marcoagner · 2018-06-03T21:16:35Z

Thank you, @promag.
I added some functional tests for this specific behavior between importaddress and importprivkey.
If everything is alright, I'll squash and rebase as necessary.

promag · 2018-06-03T21:42:00Z

src/wallet/rpcdump.cpp

I was thinking in something like

if (request.params[1].isNull()) { const auto& it = pwallet->mapAddressBook.find(dest); if (it != pwallet->mapAddressBook.end()) strLabel = it->second; } pwallet->SetAddressBook(dest, strLabel, "receive");

Hm, I see. Yeah.. this behavior seems better. Thanks!
Updating this and the test.

marcoagner · 2018-06-03T23:57:46Z

Updated with the right behavior.
I will squash and rebase it as necessary in the morning, thanks.

promag

Update PR title.

promag · 2018-06-04T00:12:03Z

test/functional/wallet_import_with_label.py

promag · 2018-06-04T00:17:48Z

src/wallet/rpcdump.cpp

Comment is inacurate.

harding · 2018-06-04T17:58:01Z

Tested ACK 74877f3b10072e1975c931bb296212e008cd8c58 . This is exactly the behavior I desired, thanks @marcoagner!

Empact · 2018-06-08T07:24:13Z

src/wallet/rpcdump.cpp

Is it problematic that this will apply the label for a given destination over all following destinations, provided they aren't found on lookup? I suspect you'll want another destination-specific local to prevent that.

Good catch! Suggestion:

if (it == pwallet->mapAddressBook.end() || !request.params[1].isNull()) { pwallet->SetAddressBook(dest, strLabel, "receive") }

There should be a test ensuring the correct behaviour.

Yes, makes sense. I will update it and add a test for the correct behaviour as soon as I can.

marcoagner · 2018-06-10T11:16:38Z

I'm back to this. The new commit keeps the already tested behaviour (passing tests added on this PR et al) and passes new test for the multiple destinations bug spotted by @Empact.

luke-jr · 2018-06-12T21:44:50Z

This still changes behaviour: the default is now null (ie, current label) instead of "".

I don't particularly care what the default is (null makes more sense), but the change should be documented and noted in release notes.

promag · 2018-06-13T00:01:06Z

Agree with @luke-jr regarding the behaviour change.

marcoagner · 2018-06-13T12:03:37Z

I added a release-notes-pr13381.md file documenting the change.
Let me know if I missed something or should document this somewhere else.
Thanks!

harding · 2018-06-13T13:31:40Z

@marcoagner Thanks for adding the release notes! I thought maybe the behavior change could be described more simply with the addition of a few examples; see below from some proposed text. I also changed the heading so that it's clear that only a small part of the importprivkey API is being changed (I also removed the code style from the heading as those don't display well in headings on parts of BitcoinCore.org, but that's a local problem so feel free to add back the backtics if you like them).

RPC importprivkey: new label behavior
-------------------------------------

Previously, `importprivkey`automatically added the default empty label
("") to all addresses associated with imported private keys.  Now it
defaults to using any existing label for those addresses.  For example:

- Old behavior: you import a watch-only address with the label "cold
  wallet".  Later, you import the corresponding private key using the
  default settings.  The address's label is changed from "cold wallet"
  to "".

- New behavior: you import a watch-only address with the label "cold
  wallet".  Later, you import the corresponding private key using the
  default settings.  The address's label remains "cold wallet".

In both the previous and current case, if you directly specify a label
during the import, that label will override whatever previous label the
addresses may have had.  Also in both cases, if none of the addresses
previously had a label, they will still receive the default empty label
("").  Examples:

- You import a watch-only address with the label "temporary".  Later you
  import the corresponding private key with the label "final".  The
  address's label will be changed to "final".

- You import a private key for an address that was not previously in the
  wallet.  It's addresses will receive the default empty label ("").

marcoagner · 2018-06-13T13:57:18Z

@harding That's much better, thanks.
I'm changing to that on the next commit with some small (nit) differences.
Here's the diff so it's easier to spot my edits:

4,5c4,5
< Previously, `importprivkey`automatically added the default empty label
< ("") to all addresses associated with imported private keys.  Now it
---
> Previously, `importprivkey` automatically added the default empty label
> ("") to all addresses associated with the imported private keys.  Now it
21c21
< ("").  Examples:
---
> ("") as default.  Examples:
28c28,29
<   wallet.  It's addresses will receive the default empty label ("").
---
>   wallet using the default settings.  Its addresses will receive the
>   default empty label ("").

Seems good?

harding · 2018-06-13T14:30:46Z

@marcoagner LGTM, although the two uses of 'default' in this clause is a tiny bit awkward IMO (but, it's clear, which is all that really matters):

they will still receive the default empty label ("") as default.

Maybe that second-to-last sentence could have its adverbial phrase "using the default settings" moved next to the verb it modifies, "import", e.g.:

You use the default settings to import a private key for an address that was not previously in the wallet.

Thanks for updating, and thanks especially for catching my misuse of the apostrophe in the possessive its. 🤦‍♂️

marcoagner · 2018-06-13T15:22:34Z

@harding I changed both parts pointed in your last comment. Thanks for the attention and help with this; I think we've now nailed this part. :)

marcoagner · 2018-07-19T22:12:06Z

Could anybody review 6115b5231954a38c400fa4808627dbb8e239ea68, please? One month in the freezer and is (hopefully) simple. :) @promag @Empact

sipa

utACK 6115b5231954a38c400fa4808627dbb8e239ea68. Didn't review the tests.

Non-blocking nit only.

sipa · 2018-07-19T22:59:21Z

src/wallet/rpcdump.cpp

Nit: It's slightly confusing to use a reference type here; find returns an iterator by value.

promag · 2018-07-20T00:37:47Z

doc/release-notes-pr13381.md

Should be ... imported private key. (singular)?

promag · 2018-07-20T00:43:00Z

src/wallet/rpcdump.cpp

Could drop watch-only since the code below does't validate that, it only checks if the label exists for each associated address. So default=current label if exists, otherwise \"\".

Yes. Just leaving address between if and exists: default=current label if address exists, otherwise \"\". (I know it was probably just a typo; I'm commenting to make sure)

promag · 2018-07-20T00:59:41Z

src/wallet/rpcdump.cpp

Nit, maybe make it clear that params[1] has precedence — prevents a lookup if it does? Also, updating with the same label could be avoided, or am I missing something? Suggestion:

for (const auto& dest : GetAllDestinationsForKey(pubkey)) { if (!request.params[1].isNull()) { pwallet->SetAddressBook(dest, strLabel, "receive") } else if (pwallet->mapAddressBook.count(dest) == 0) { pwallet->SetAddressBook(dest, strLabel, "receive"); } }

Yes, it seems right to me. Thank you again!
I thought I had to somehow guarantee purpose was now "receive" for all dests, but it doesn't seem to be the case now I looked.

promag · 2018-07-20T01:00:33Z

test/functional/wallet_import_with_label.py

Drop watch-only?

promag · 2018-07-20T01:02:10Z

test/functional/wallet_import_with_label.py

Can be removed.

marcoagner · 2018-07-20T10:20:48Z

Thank you all for reviewing again. Updated with 1d4fdb33349a60cac1926f83586826ba62b1a451.

promag · 2018-07-20T10:27:23Z

Maybe @harding could re-test?

sipa

utACK 1d4fdb33349a60cac1926f83586826ba62b1a451

sipa · 2018-07-21T06:46:35Z

src/wallet/rpcdump.cpp

Given that the branches for the two if statements are identical, why not write it as a single if (!request.params[1].isNull() || pwallet->mapAddressBook.count(dest)) { ?

Just to try to make it obvious in the code that the request.params[1] has precedence. Bad idea? Both work.

No reason besides it since AFAIK putting the look up as second condition in the || statement (like your suggestion) will not call it if there is a params[1] in the first condition.

maflcko · 2018-09-13T22:16:14Z

test/functional/wallet_import_with_label.py

The skip-if-no-wallet might have to be added to this test?

You're right. I was not aware, thanks.

maflcko · 2018-09-13T22:16:24Z

Lets see what travis says

DrahtBot · 2018-09-15T14:21:59Z

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

Conflicts

No conflicts as of last run.

practicalswift · 2018-10-05T07:25:25Z

test/functional/wallet_import_with_label.py

Since this is a new file - please run it through black to get it auto-formatted in accordance with PEP-8 :-)

Hm, nice. Didn't know this black, thanks.
I was using Flake8. Besides the double-quotes for strings, it seems that black has a longer default line length, but I'm okay with (actually prefer) that. :)
I will just rebase, re-run tests, and provide a new commit.

- changes importprivkey behavior to overwrite existent label if one is passed and keep existing ones if no label is passed - tests behavior of importprivkey on existing address labels and different same key destination

marcoagner · 2018-10-05T13:39:36Z

wallet_basic.py passed locally but seems to have failed on appveyor 🤔

meshcollider · 2018-11-10T12:58:54Z

utACK a6b5ec1

sipa · 2018-11-11T03:10:22Z

utACK a6b5ec1

Empact · 2018-11-13T06:25:28Z

utACK a6b5ec1

jonasschnelli · 2018-11-13T07:51:06Z

utACK a6b5ec1

…ivkey a6b5ec1 rpc: creates possibility to preserve labels on importprivkey (marcoagner) Pull request description: Closes #13087. As discussed in the issue, this is a feature request instead of a bug report since the behaviour was as intended (i.e. label with default: `''`). With this, the old behaviour is kept while the possibility to achieve the preservation of labels, as expected in the open issue, is added. Tree-SHA512: b33be50e1e7f62f7ddfae953177ba0926e2d848961f9fac7501c2b513322c0cb95787745d07d137488267bad1104ecfdbe800c6747f94162eb07c976835c1386

Summary: - changes importprivkey behavior to overwrite existent label if one is passed and keep existing ones if no label is passed - tests behavior of importprivkey on existing address labels and different same key destination This is a backport of Core [[bitcoin/bitcoin#13381 | PR13381]] . I had to modify heavily the last test to create a P2SH manually. Bitcoin Core does produce a segwit wrapped into P2SH address by default so this isn't a problem for them. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D6185

marcoagner mentioned this pull request Jun 3, 2018

importprivkey unsets labels set with importaddress and importpubkey #13087

Closed

fanquake added Wallet RPC/REST/ZMQ labels Jun 3, 2018

marcoagner changed the title ~~[WIP] RPC: creates preserve parameter for importprivkey call~~ RPC: creates preserve parameter for importprivkey call Jun 3, 2018

promag reviewed Jun 3, 2018

View reviewed changes

yuntai mentioned this pull request Jun 4, 2018

RPC/REST/ZMQ, Wallet: Set label with importprivkey only requested #13365

Closed

promag reviewed Jun 4, 2018

View reviewed changes

marcoagner changed the title ~~RPC: creates preserve parameter for importprivkey call~~ RPC: creates possibility to preserve labels on importprivkey Jun 4, 2018

Empact reviewed Jun 8, 2018

View reviewed changes

sipa reviewed Jul 19, 2018

View reviewed changes

src/wallet/rpcdump.cpp Outdated

Copy link

Member

sipa Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It's slightly confusing to use a reference type here; find returns an iterator by value.

promag reviewed Jul 20, 2018

View reviewed changes

sipa reviewed Jul 21, 2018

View reviewed changes

maflcko reviewed Sep 13, 2018

View reviewed changes

maflcko closed this Sep 13, 2018

maflcko reopened this Sep 13, 2018

DrahtBot mentioned this pull request Sep 15, 2018

tests: Reorder tests and move most of extended tests up to normal tests #14225

Merged

practicalswift reviewed Oct 5, 2018

View reviewed changes

rpc: creates possibility to preserve labels on importprivkey

a6b5ec1

- changes importprivkey behavior to overwrite existent label if one is passed and keep existing ones if no label is passed - tests behavior of importprivkey on existing address labels and different same key destination

DrahtBot mentioned this pull request Oct 20, 2018

Rpc help helper class #14502

Closed

practicalswift mentioned this pull request Oct 22, 2018

Enable flake8 rule E225 which checks for missing whitespace around op… #14534

Closed

jonasschnelli merged commit a6b5ec1 into bitcoin:master Nov 13, 2018

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

RPC: creates possibility to preserve labels on importprivkey #13381

RPC: creates possibility to preserve labels on importprivkey #13381

Uh oh!

Conversation

marcoagner commented Jun 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jun 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jun 3, 2018

Uh oh!

marcoagner commented Jun 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoagner commented Jun 3, 2018

Uh oh!

promag 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!

harding commented Jun 4, 2018

Uh oh!

Empact Jun 8, 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!

promag Jun 8, 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!

Choose a reason for hiding this comment

Uh oh!

marcoagner commented Jun 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Jun 12, 2018

Uh oh!

promag commented Jun 13, 2018

Uh oh!

marcoagner commented Jun 13, 2018

Uh oh!

harding commented Jun 13, 2018

Uh oh!

marcoagner commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harding commented Jun 13, 2018

Uh oh!

marcoagner commented Jun 13, 2018

Uh oh!

marcoagner commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Jul 20, 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!

promag Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

marcoagner commented Jun 3, 2018 •

edited

Loading

promag commented Jun 3, 2018 •

edited

Loading

Empact Jun 8, 2018 •

edited

Loading

promag Jun 8, 2018 •

edited

Loading

marcoagner commented Jun 10, 2018 •

edited

Loading

marcoagner commented Jun 13, 2018 •

edited

Loading

marcoagner commented Jul 19, 2018 •

edited

Loading

promag Jul 20, 2018 •

edited

Loading

promag Jul 20, 2018 •

edited

Loading

sipa Jul 21, 2018 •

edited

Loading

marcoagner Jul 21, 2018 •

edited

Loading

marcoagner Jul 21, 2018 •

edited

Loading

DrahtBot commented Sep 15, 2018 •

edited

Loading