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

RPC: creates possibility to preserve labels on importprivkey #13381

Merged
merged 1 commit into from Nov 13, 2018
Merged

RPC: creates possibility to preserve labels on importprivkey #13381

merged 1 commit into from Nov 13, 2018

Conversation

@marcoagner
Copy link
Contributor

@marcoagner marcoagner commented Jun 3, 2018

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.

@promag
Copy link
Member

@promag promag commented Jun 3, 2018

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

@promag promag commented Jun 3, 2018

Concept ACK, a test would be nice.

@marcoagner
Copy link
Contributor Author

@marcoagner marcoagner commented Jun 3, 2018

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.

@marcoagner marcoagner changed the title [WIP] RPC: creates preserve parameter for importprivkey call RPC: creates preserve parameter for importprivkey call Jun 3, 2018
for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
pwallet->SetAddressBook(dest, strLabel, "receive");
bool has_label = pwallet->mapAddressBook.find(dest) != pwallet->mapAddressBook.end() && !pwallet->mapAddressBook[dest].name.empty();
Copy link
Member

@promag promag Jun 3, 2018

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");

Copy link
Contributor Author

@marcoagner marcoagner Jun 3, 2018

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

@marcoagner
Copy link
Contributor Author

@marcoagner marcoagner commented Jun 3, 2018

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

Copy link
Member

@promag promag left a comment

Update PR title.

@@ -0,0 +1,93 @@
#!/usr/bin/env python3
# Copyright (c) 2017 The Bitcoin Core developers
Copy link
Member

@promag promag Jun 4, 2018

2018

@@ -151,8 +151,13 @@ UniValue importprivkey(const JSONRPCRequest& request)
CKeyID vchAddress = pubkey.GetID();
{
pwallet->MarkDirty();
// We don't know which corresponding address will be used; label them all

// We don't know which corresponding address will be used; label all new addresses and preserve the rest
Copy link
Member

@promag promag Jun 4, 2018

Comment is inacurate.

@marcoagner marcoagner changed the title RPC: creates preserve parameter for importprivkey call RPC: creates possibility to preserve labels on importprivkey Jun 4, 2018
@harding
Copy link
Member

@harding harding commented Jun 4, 2018

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

for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
if (request.params[1].isNull()) {
const auto& it = pwallet->mapAddressBook.find(dest);
if (it != pwallet->mapAddressBook.end()) strLabel = it->second.name;
Copy link
Member

@Empact Empact Jun 8, 2018

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.

Copy link
Member

@promag promag Jun 8, 2018

Good catch! Suggestion:

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

Copy link
Member

@promag promag Jun 8, 2018

There should be a test ensuring the correct behaviour.

Copy link
Contributor Author

@marcoagner marcoagner Jun 8, 2018

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

@marcoagner
Copy link
Contributor Author

@marcoagner marcoagner commented Jun 10, 2018

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

@luke-jr luke-jr commented Jun 12, 2018

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

@promag promag commented Jun 13, 2018

Agree with @luke-jr regarding the behaviour change.

@marcoagner
Copy link
Contributor Author

@marcoagner marcoagner commented Jun 13, 2018

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

@harding harding commented Jun 13, 2018

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

@marcoagner marcoagner commented Jun 13, 2018

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

@harding harding commented Jun 13, 2018

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

@marcoagner marcoagner commented Jun 13, 2018

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

@marcoagner marcoagner commented Jul 19, 2018

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

Copy link
Member

@sipa sipa left a comment

utACK 6115b5231954a38c400fa4808627dbb8e239ea68. Didn't review the tests.

Non-blocking nit only.

for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
pwallet->SetAddressBook(dest, strLabel, "receive");
const auto& it = pwallet->mapAddressBook.find(dest);
Copy link
Member

@sipa sipa Jul 19, 2018

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

-------------------------------------

Previously, `importprivkey` automatically added the default empty label
("") to all addresses associated with the imported private keys. Now it
Copy link
Member

@promag promag Jul 20, 2018

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

@@ -102,7 +102,7 @@ UniValue importprivkey(const JSONRPCRequest& request)
"Hint: use importmulti to import more than one private key.\n"
"\nArguments:\n"
"1. \"privkey\" (string, required) The private key (see dumpprivkey)\n"
"2. \"label\" (string, optional, default=\"\") An optional label\n"
"2. \"label\" (string, optional, default=current label if watch-only address exists, otherwise \"\") An optional label\n"
Copy link
Member

@promag promag Jul 20, 2018

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 \"\".

Copy link
Contributor Author

@marcoagner marcoagner Jul 20, 2018

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)

for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
pwallet->SetAddressBook(dest, strLabel, "receive");
const auto& it = pwallet->mapAddressBook.find(dest);
if (it == pwallet->mapAddressBook.end() || !request.params[1].isNull()) {
Copy link
Member

@promag promag Jul 20, 2018

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");
    }
}

Copy link
Contributor Author

@marcoagner marcoagner Jul 20, 2018

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.

# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test the behavior of RPC importprivkey on set and unset labels of
watch-only addresses.
Copy link
Member

@promag promag Jul 20, 2018

Drop watch-only?

self.num_nodes = 2
self.setup_clean_chain = True

def setup_network(self):
Copy link
Member

@promag promag Jul 20, 2018

Can be removed.

@marcoagner
Copy link
Contributor Author

@marcoagner marcoagner commented Jul 20, 2018

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

@promag
Copy link
Member

@promag promag commented Jul 20, 2018

Maybe @harding could re-test?

Copy link
Member

@sipa sipa left a comment

utACK 1d4fdb33349a60cac1926f83586826ba62b1a451

pwallet->SetAddressBook(dest, strLabel, "receive");
if (!request.params[1].isNull()) {
pwallet->SetAddressBook(dest, strLabel, "receive");
} else if (pwallet->mapAddressBook.count(dest) == 0) {
Copy link
Member

@sipa sipa Jul 21, 2018

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)) { ?

Copy link
Contributor Author

@marcoagner marcoagner Jul 21, 2018

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

Copy link
Contributor Author

@marcoagner marcoagner Jul 21, 2018

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.

self.num_nodes = 2
self.setup_clean_chain = True

def run_test(self):
Copy link
Member

@MarcoFalke MarcoFalke Sep 13, 2018

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

Copy link
Contributor Author

@marcoagner marcoagner Sep 28, 2018

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Sep 13, 2018

Lets see what travis says

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Sep 15, 2018

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

Conflicts

No conflicts as of last run.


bcaddress_assert = self.nodes[1].getaddressinfo(
address_assert4['embedded']['address']
)
Copy link
Contributor

@practicalswift practicalswift Oct 5, 2018

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

Copy link
Contributor Author

@marcoagner marcoagner Oct 5, 2018

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

@marcoagner marcoagner commented Oct 5, 2018

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

@meshcollider
Copy link
Member

@meshcollider meshcollider commented Nov 10, 2018

utACK a6b5ec1

@sipa
Copy link
Member

@sipa sipa commented Nov 11, 2018

utACK a6b5ec1

@Empact
Copy link
Member

@Empact Empact commented Nov 13, 2018

utACK a6b5ec1

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Nov 13, 2018

utACK a6b5ec1

@jonasschnelli jonasschnelli merged commit a6b5ec1 into bitcoin:master Nov 13, 2018
2 checks passed
jonasschnelli added a commit that referenced this issue Nov 13, 2018
…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
deadalnix added a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue May 21, 2020
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.