Use CWallet::Import* functions in all import* RPCs #16301

achow101 · 2019-06-28T00:53:20Z

#15741 introduced ImportPrivKeys, ImportPubKeys, ImportScripts, and ImportScriptPubKeys in CWallet which are used by importmulti. This PR changes the remaining import* RPCs (importaddress, importprivkey, importpubkey, and importwallet) to use these functions as well instead of directly adding the imported items to the wallet.

achow101 · 2019-06-28T00:55:42Z

This also makes the CWallet restructure simpler as the changes to CWallet's import functions will then apply to all the import* RPCs.

DrahtBot · 2019-06-28T04:19:20Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16341 (WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
#16037 (rpc: Early fail import(wallet,multi) when a required block is pruned by promag)
#15414 ([wallet] allow adding pubkeys from imported private keys to keypool by Sjors)
#15093 (rpc: Change importwallet to return additional errors by marcinja)

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.

maflcko · 2019-06-28T20:59:33Z

Approach ACK

promag

Concept ACK. Spent some time reviewing the changes commit by commit and this mostly looks good to me considering latests refactors to the wallet. IMO last commit should be reworked or add a explanation as to why it's safe to ignore errors/return values.

promag · 2019-06-29T22:42:36Z

src/wallet/rpcdump.cpp

e66ed3b84593bebd3550e18b5b3be3605e08ced1

These changes are kind of breaking change, the caller has no way to know if something went bad - as it is will always succeed.Is this the case just because dumpwallet/importwallet should not be used?

This change was because it ImportScripts and ImportPrivKeys doesn't return why it failed. It could fail because the script or privkey already exists in the wallet which would still be fine, or it could cail because something went wrong with writing the data to the wallet file which is bad. I suppose I could make them return an enum, but that's a larger change that I didn't really want to do.

The errors reported as-is appear to at least be somewhat helpful.

It could fail because the script or privkey already exists in the wallet

I don't think that's true. These two checks appear to be only reporting real errors. Perhaps I missed something.

Yes, but these checks are part of ImportPrivKeys and ImportScripts both of which can return false in non-error cases. Both have HaveKey and/or HaveCScript checks that can fail, but not cause the import to be bad. But there could also be an actual error which would result in the import being bad.

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (991ecfdbbb289bd6e4c50fc63f9201edbca14f70)

Both have HaveKey and/or HaveCScript checks that can fail, but not cause the import to be bad.

What's this referring to? Import methods seem to return true if imported data was already found. But maybe I'm misinterpreting. Might help to refer to a specific example.

Turns out I was having a hard time with the negations.

I've pushed another commit that makes it more explicit when something is being skipped because we already have it, as well as logging when this happens.

ryanofsky

Really nice changes. Almost finished reviewing (will update list below with progress).

12fd3a849c4be0879e48e65661ca1628c21308b2 Optionally apply labels to imported scriptPubKeys (1/6)
ea665e323f76766def6c4f46a46c53e38365d179 Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys (2/6)
94ad6ad965ba6dfffaf7387b15ac659323fc85d0 Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions (3/6)
2fb78f8baad0778de81dc81209340e04ccef4df6 Have importaddress use ImportScripts and ImportScriptPubKeys (4/6)
92863d7de8cf0d7432bc47f36bf1aa4de257c548 Optionally allow ImportScripts to set script creation timestamp (5/6)
e66ed3b84593bebd3550e18b5b3be3605e08ced1 Have importwallet use ImportPrivKeys and ImportScripts (6/6)

src/wallet/rpcdump.cpp

src/wallet/wallet.cpp

ryanofsky · 2019-07-01T15:47:05Z

src/wallet/rpcdump.cpp

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (ea665e323f76766def6c4f46a46c53e38365d179)

Just curious, but why is this replacing the LearnAllRelatedScripts() call with inlined code? The change seems fine but is duplicative, makes code longer, and seems like the opposite of the other changes here switching to higher level ImportPrivKeys() and ImportScriptPubKeys() calls.

This was done in preparation for the wallet restructure which causes LearnAllRelatedScripts to no longer be part of CWallet.

I actually find this slightly more readable than the previous LearnAllRelatedScripts -> LearnRelatedScripts(key, OutputType::P2SH_SEGWIT);. .... However it's hard to tell if it's really doing the same thing. Maybe add an extra commit to make that more clear?

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (5776e6d72e76e03251d346588035fcdb0423440c)

Just for the record, I like Sjors idea of putting the AddKeyPubKey->ImportPrivKeys and LearnAllRelatedScripts->ImportScripts in two separate commits, since they are independent changes. But it's not very important.

src/wallet/rpcdump.cpp

ryanofsky · 2019-07-01T16:14:16Z

src/wallet/rpcdump.cpp

In commit "Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions" (94ad6ad965ba6dfffaf7387b15ac659323fc85d0)

Just a note to other reviewers: this seems to happen in ImportPubKeys now

more specifically here's the rough logic of ImportScript:

mark dirty addwatchonly if is redeem script: addcscript ImportAddress else SetAddressBook

And in this case it's not a redeemscript(last arg is false), so it marks dirty, adds watchonly, and sets address book.

Mark dirty is top-level now, ImportScriptPubKeys takes care of the SetAddressBookWithDB, and both ImportScriptPubKeys and ImportPubKeys adds watch only in specific situations(when pubkey is not already in wallet, or the script is not in the wallet yet). Previously the watchonly script was added when ISMINE was not spendable. I'll leave it up to other reviewers to figure if this is desired/same semantics.

I'll leave it up to other reviewers to figure if this is desired/same semantics.

@achow101 could you clarify in commit message whether this causes a change in behavior or not? If you aren't sure, it'd at least be good to say if it potentially changes behavior. This is hurting my head to think about but it seems only potential difference is whether timestamp is overridden when key already exists?

For all these commits, it'd be really helpful to note in commit messages whether they do or don't or maybe change behavior and what the scope of the change is.

I've added a note to each commit indicating what behavior changes in it.

Here, the only behavior change is that the timestamp for the key will be set to 1 if we already have it.

src/wallet/wallet.h

src/wallet/wallet.cpp

src/wallet/rpcdump.cpp

Sjors

Concept ACK. I have some questions about the first commit, will study the rest later.

Sjors · 2019-07-09T16:19:38Z

src/wallet/rpcdump.cpp

I actually find this slightly more readable than the previous LearnAllRelatedScripts -> LearnRelatedScripts(key, OutputType::P2SH_SEGWIT);. .... However it's hard to tell if it's really doing the same thing. Maybe add an extra commit to make that more clear?

Sjors · 2019-07-09T16:23:31Z

src/wallet/rpcdump.cpp

It's not obvious to me how dropping SetAddressBook doesn't change behavior, e.g. we're no longer calling NotifyAddressBookChanged.

SetAddressBook happens in ImportScriptPubKeys

As long as the imported key is !internal and is valid destination, it does appear to SetAddressBookWithDB which in this RPC is true.

instagibbs · 2019-07-10T13:42:43Z

src/wallet/rpcdump.cpp

why 1 and not 0? No internal documentation I can find on this.

Dunno, it's just adapted from the changed code.

1 was introduced in #7551...

why 1 and not 0? No internal documentation I can find on this.

As far as I know, there is no substantive difference anymore between 0 and 1, but I think it's good to keep using 1 here so this is a refactoring change, not a change in behavior.

I do think in general it's better to use 1 as an explicit request to rescan from the beginning of the chain as opposed to 0 which could indidcate we're dealing with an old wallet that didn't save metadata (see #9108, #9682), or an uninitialized variable, or some other undefined craziness from all the spaghetti code.

instagibbs · 2019-07-10T13:46:06Z

src/wallet/rpcdump.cpp

As long as the imported key is !internal and is valid destination, it does appear to SetAddressBookWithDB which in this RPC is true.

instagibbs · 2019-07-10T13:51:30Z

src/wallet/rpcdump.cpp

(request.params[1].isNull()) && pwallet->mapAddressBook.count(dest) != 0 /* internal */

This seems like an extreme abuse of the flag based on the name and confused me. This should be renamed add_to_book_if_valid or maybe even better just have the validity check inside SetToAddressBook(do we ever want to import invalid destinations?) and call it add_to_addrbook.

I've renamed the flag to apply_label.

instagibbs · 2019-07-10T13:56:23Z

src/wallet/rpcdump.cpp

? This is already done by GetAllDestinationsForKey

ImportScriptPubKeys doesn't actually add the scripts to the wallet, so we add the script here.

Might be useful to say as much

instagibbs · 2019-07-10T14:01:48Z

src/wallet/rpcdump.cpp

more specifically here's the rough logic of ImportScript:

mark dirty addwatchonly if is redeem script: addcscript ImportAddress else SetAddressBook

And in this case it's not a redeemscript(last arg is false), so it marks dirty, adds watchonly, and sets address book.

Mark dirty is top-level now, ImportScriptPubKeys takes care of the SetAddressBookWithDB, and both ImportScriptPubKeys and ImportPubKeys adds watch only in specific situations(when pubkey is not already in wallet, or the script is not in the wallet yet). Previously the watchonly script was added when ISMINE was not spendable. I'll leave it up to other reviewers to figure if this is desired/same semantics.

instagibbs · 2019-07-10T14:05:05Z

src/wallet/rpcdump.cpp

can you annotate key_origins too?

instagibbs · 2019-07-10T14:25:26Z

src/wallet/wallet.h

nit: timestamp since you're calling it that in other places and it matches better other args

instagibbs · 2019-07-10T14:28:46Z

src/wallet/rpcdump.cpp

isn't this backwards?

Because it's reporting errors when it returns true?

Oh, whoops. Fixed.

instagibbs · 2019-07-10T14:39:20Z

src/wallet/rpcdump.cpp

The errors reported as-is appear to at least be somewhat helpful.

It could fail because the script or privkey already exists in the wallet

I don't think that's true. These two checks appear to be only reporting real errors. Perhaps I missed something.

The internal bool was only to indicate whether the given label should be applied as things that are internal should not have a label. To make this clearer, we change internal to apply_label and invert its usage so things that have labels set this to true in order to have their labels applied.

Sjors · 2019-07-12T14:11:35Z

Travis sad

achow101 · 2019-07-12T20:32:02Z

Fixed travis hopefully.

instagibbs · 2019-07-16T01:58:09Z

utACK 991ecfdbbb289bd6e4c50fc63f9201edbca14f70

ryanofsky

I reviewed the whole PR, and I think it's close, but am unsure about some issues. Details are in code comments, but my main concerns were:

It seems bad that importprivkey might be calling ImportScriptPubKeys instead of SetAddressBook just to set the label.
There might be some unintended changes around updating nCreateTime. In general, commits are unclear about how and whether they are intended to change behavior.
Error handling in the importwallet commit seems wrong

But aside from these specific concerns, the change seem very good and moves in a welcome direction.

ryanofsky · 2019-07-02T17:28:07Z

src/wallet/rpcdump.cpp

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (74ec96b601dd6417f01f4801d83e01eeb7dded18)

It's not really clear from context that this comment is referring to the apply_label argument passed to ImportScriptPubKeys below. Would suggest moving the comment or mentioning this explicitly.

Reverted the below change entirely.

ryanofsky · 2019-07-02T17:41:35Z

src/wallet/rpcdump.cpp

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (e3cc8e102fc2736b3871869646f8d60599988f82)

Is this ever going to add watch only scripts given the !IsMine check in ImportScriptPubKeys?

It'd be nice to have a succinct comment here saying when this will add a watch only script, and why it's necessary.

It won't ever add the scripts as watch only because we import the private keys earlier.

Reverted this change

ryanofsky · 2019-07-16T17:22:03Z

src/wallet/rpcdump.cpp

why 1 and not 0? No internal documentation I can find on this.

As far as I know, there is no substantive difference anymore between 0 and 1, but I think it's good to keep using 1 here so this is a refactoring change, not a change in behavior.

I do think in general it's better to use 1 as an explicit request to rescan from the beginning of the chain as opposed to 0 which could indidcate we're dealing with an old wallet that didn't save metadata (see #9108, #9682), or an uninitialized variable, or some other undefined craziness from all the spaghetti code.

ryanofsky · 2019-07-16T18:45:16Z

src/wallet/rpcdump.cpp

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (74ec96b601dd6417f01f4801d83e01eeb7dded18)

I don't understand why this is calling ImportScriptPubKeys instead of just SetAddressBook like before. This is confusing because the code is first going out of its way to remove associated watch-only scripts from setWatchOnly (in AddKeyPubKeyWithDB via ImportPrivKeys):

https://github.com/bitcoin/bitcoin/blob/74ec96b601dd6417f01f4801d83e01eeb7dded18/src/wallet/wallet.cpp#L310-L319

And then after removing the scripts it looks like it goes out of its way to add them by calling ImportScriptPubKeys. But then the ImportScriptPubKeys call doesn't add the scripts because have_solving_data and IsMine are both true in the following check?

https://github.com/bitcoin/bitcoin/blob/74ec96b601dd6417f01f4801d83e01eeb7dded18/src/wallet/wallet.cpp#L1724-L1727

Something seems wrong here. If scripts are meant to be added to setWatchOnly it seems like the removal code shouldn't be called and the adding code might need to be fixed. Of the scripts are meant to be removed, then it's misleading to call ImportScriptPubKeys instead of just SetAddressBook like before.

Reverted this change

ryanofsky · 2019-07-16T19:11:50Z

src/wallet/rpcdump.cpp

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (74ec96b601dd6417f01f4801d83e01eeb7dded18)

It looks like there is a minor change in behavior here due to implementation of ImportPrivKeys:

https://github.com/bitcoin/bitcoin/blob/74ec96b601dd6417f01f4801d83e01eeb7dded18/src/wallet/wallet.cpp#L1683

Previously nCreateTime would not be updated if key already existed, and now it will be (in memory only, apparently the value is not written to disk on the next line).

Would suggesting fixing up ImportPrivKeys to avoid this change in behavior, or documenting in the commit message here that there is a slight change in behavior.

Fixed ImportPrivKeys

ryanofsky · 2019-07-16T20:00:30Z

src/wallet/rpcdump.cpp

In commit "Optionally allow ImportScripts to set script creation timestamp" (ace5d082034cbaa52f596689433381a2226a21dc)

Can you clairfy in the commit message whether this is a change in behavior for the importmulti RPC. Seems like a minor bugfix?

ryanofsky · 2019-07-16T20:14:23Z

src/wallet/rpcdump.cpp

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (991ecfdbbb289bd6e4c50fc63f9201edbca14f70)

IMO, would be clearer to call ImportPrivKeys({keyid, key}}, time) than have the extra map variable.

ryanofsky · 2019-07-16T20:15:50Z

src/wallet/rpcdump.cpp

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (991ecfdbbb289bd6e4c50fc63f9201edbca14f70)

Ths doesn't seem right, if HaveKey is true, ImportPrivKeys returns true.

~~No?~~

nvm, looking at a change that I had locally

I've restored fGood and the error messages.

ryanofsky · 2019-07-16T20:17:24Z

src/wallet/rpcdump.cpp

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (991ecfdbbb289bd6e4c50fc63f9201edbca14f70)

This print statement should precede the import above...

ryanofsky · 2019-07-16T20:22:13Z

src/wallet/rpcdump.cpp

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (991ecfdbbb289bd6e4c50fc63f9201edbca14f70)

Both have HaveKey and/or HaveCScript checks that can fail, but not cause the import to be bad.

What's this referring to? Import methods seem to return true if imported data was already found. But maybe I'm misinterpreting. Might help to refer to a specific example.

promag · 2019-07-17T09:54:08Z

src/wallet/rpcdump.cpp

7a58ad5624b2ea9c2d2c661446cc6e046a222fca

nTimeBegin is not being updated, the rescan below could miss wallet transactions?

Added the nTimeBegin update back.

promag · 2019-07-17T10:01:49Z

src/wallet/rpcdump.cpp

7a58ad5624b2ea9c2d2c661446cc6e046a222fca

nit, this wouldn't be logged if the key is present.

Added a note in the commit message that this happens.

ryanofsky

utACK 35805ce74a780674c2a04259f98c8888906d7e91. Thanks for the previous fixes, and feel free to ignore my questions / comments, but I hope one you'll consider is restoring the direct SetAddressBook call in importwallet instead of calling it indirectly through ImportScriptPubKeys, because it's confusing how ImportScriptPubKeys appears to add watch only scripts.

ryanofsky · 2019-07-17T17:08:25Z

src/wallet/wallet.cpp

In commit "Log when an import is being skipped because we already have it" (8bd426d238a526f8cfa4e8dd567fb3ccadb40cf7)

Note: Beyond adding the log, this commit also changes behavior of ImportPrivKeys to not update the nCreateTime when the key already exists. This is a good change, so just wanted to point it out.

ryanofsky · 2019-07-17T17:15:34Z

src/wallet/rpcdump.cpp

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (5776e6d72e76e03251d346588035fcdb0423440c)

Nice! From ea665e323f76766def6c4f46a46c53e38365d179 to 5776e6d72e76e03251d346588035fcdb0423440c this commit is so much simpler now. The ImportScriptPubKeys stuff was so confusing to me before.

ryanofsky · 2019-07-17T17:20:42Z

src/wallet/rpcdump.cpp

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (5776e6d72e76e03251d346588035fcdb0423440c)

Just for the record, I like Sjors idea of putting the AddKeyPubKey->ImportPrivKeys and LearnAllRelatedScripts->ImportScripts in two separate commits, since they are independent changes. But it's not very important.

ryanofsky · 2019-07-18T17:08:57Z

src/wallet/rpcdump.cpp

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (35805ce74a780674c2a04259f98c8888906d7e91)

Any particular reason this this is calling ImportScriptPubKeys to indirectly set the labels instead of just directly calling SetAddressBook like before? This seems to create same confusion around removing watch only scripts, and then appearing to add them again, but then not really adding them due to ismine, that was in a prior version of the importprivkey commit #16301 (comment).

Would suggest reverting here and just calling SetAddressBook instead of ImportScriptPubKeys like before. Or adding a comment about what ImportScriptPubKeys is intended to do in this context.

ryanofsky · 2019-07-18T17:19:41Z

src/wallet/rpcdump.cpp

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (35805ce74a780674c2a04259f98c8888906d7e91)

Interesting there is still a case that treats 0 and 1 key times differently. It might a be nice idea in a followup PR to replace all the 0's and 1's in the code with SKIP_RESCAN = 0, FULL_RESCAN = 1 constants.

I think the use of 0 and 1 is kind of inconsistent and all of the the timestamp stuff needs to be cleaned up. But that should be done in a followup PR.

ryanofsky · 2019-07-18T20:13:12Z

src/wallet/wallet.cpp

In commit "Log when an import is being skipped because we already have it" (8bd426d238a526f8cfa4e8dd567fb3ccadb40cf7)

Noting another change in behavior: This will now avoid adding the key to the keypool if add_keypool is true and the public key already exists. Could potentially be documented in the commit message or add_keypool description.

Added a note in the commit message

Behavior Changes: * Those pubkeys being imported with add_keypool set and are already in the wallet will no longer be added to the keypool

…mportScriptPubKeys Behavior changes: * If we already have the key, it's wpkh script will still be added, although it should already be there

ryanofsky

utACK bc8ecc056ffa42c758b04d5bcae32c1354c3fbe1. Only change since last review was reverting the importwallet ImportScriptPubKeys change I felt was confusing, and updating one of the commit log messages to note a change in behavior.

Really nice job with this PR, and thanks for being so responsive!

instagibbs · 2019-07-20T01:35:54Z

utACK bc8ecc0

Checked diff from 991ecfd , was reversions that were discussed and some logging/error reporting improvements.

maflcko · 2019-07-24T13:18:22Z

partial ACK up to af6dced944

Some questions:

In commit af6dced944, there seems to be a behavior change that is not
mentioned in the commit body: Previously, if the privkey already existed,
we'd error out. Now, we'd overwrite the label?
In commit af6dced944. Previously, there was a call to
AddCScript(GetScriptForDestination(WitnessV0KeyHash(pubKey.GetId())));
(via LearnAllRelatedScripts). I don't see why this is no longer required
or where an equivalent call is made. Mind to explain?

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

partial ACK up to af6dced944

Some questions:
* In commit af6dced944, there seems to be a behavior change that is not
  mentioned in the commit body: Previously, if the privkey already existed,
  we'd error out. Now, we'd overwrite the label?

* In commit af6dced944. Previously, there was a call to
  AddCScript(GetScriptForDestination(WitnessV0KeyHash(pubKey.GetId())));
  (via LearnAllRelatedScripts). I don't see why this is no longer required
  or where an equivalent call is made. Mind to explain?
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiFywv+Oc9kV/FPwgRVxvIEVgUQpEE4d/DcPiZ205RzdTEOXh7DxzootgnwZSzC
S8obDJGqDy26JSpCr4R6JevUl2gTYVeKMph9G2hlEHZW4pdiAxAY4/xQ8KvoAwBZ
b+u2ovlq6A8uTCQzPYkJ1KAZHHKVwd32PAYllJshilLpdVo5rqXbDwoR322/XoV2
mEeKCjnHBhQDRY4wFQ5g+2onN1oqf2Rw9fELtXqbBR7SM5EjTF79rTfkwLWmDReu
iK92/AQHfQGSrdbILz9VJ9gJE+OCco7dvi/sqMQmXKk9q3aNAY+zDgnQRvvbKkUX
p/yZrBXH8ExN7Fd89D8sb6L/h/floJM0EMQ8Ccc7cVEou77VHLC08NRiLTP5lTzH
nmmd7Br4LqZOQuVPZ9iZCRbUpu+jGPbM0uIqtYDeqO+elSnepWYTwZlCVEfjCwpX
RSaimg4I08v0cpWicoVQcFAB3DxpU0OCJZZEy4/rS5ZwkxVmyorFSy6+Hvh97oCJ
964kzylV
=2JyL
-----END PGP SIGNATURE-----

Timestamp of file with hash 14e6ca5a72d68fc23e91e6eecdf4294ca99ad0e7ce1478ca529dfbbda4ec2d21 -

… functions Behavior changes: * If any scripts for the pubkey were already in the wallet, their timestamps will be set to 1 and label updated

Also removes the now unused ImportAddress and ImportScript from rpcdump.cpp Behavior changes: * No errors will be thrown when the script or key already exists in the wallet. * If the key or script is already in the wallet, their labels will be updated.

Behavior changes: * scripts imported in importmulti that are not explicilty scriptPubKeys will have timestamps set for them

Behavior changes: * An "Importing ..." line is logged for every key, even ones that are skipped

achow101 · 2019-07-24T15:44:57Z

* In commit [af6dced](https://github.com/bitcoin/bitcoin/commit/af6dced9446eca4a67429e07b3216ae7f3d08c16), there seems to be a behavior change that is not
  mentioned in the commit body: Previously, if the privkey already existed,
  we'd error out. Now, we'd overwrite the label?

Updated the commit message

* In commit [af6dced](https://github.com/bitcoin/bitcoin/commit/af6dced9446eca4a67429e07b3216ae7f3d08c16). Previously, there was a call to
  AddCScript(GetScriptForDestination(WitnessV0KeyHash(pubKey.GetId())));
  (via LearnAllRelatedScripts). I don't see why this is no longer required
  or where an equivalent call is made. Mind to explain?

ImportPubKeys will add the public key to mapWatchKeys. In doing so, it calls ImplicitlyLearnRelatedKeyScripts which will find that script and add it to mapScripts in memory. Every time this key is loaded, this script will always be found and loaded into memory. The only difference is that that script is not persisted to disk.

ryanofsky

utACK 40ad2f6. Only change since last review is a tweaked commit message (mentioning label update in importpubkey commit)

maflcko · 2019-07-25T13:41:55Z

ACK 40ad2f6 (checked that behavior changes are mentioned in the commit body)

Show signature and timestamp

Signature:

Timestamp of file with hash bb77b117482794207771d1571ca4e73ac19a12b52c18a1c489b86f8600c76c0b -

maflcko · 2019-07-25T21:06:16Z

I've written some tests in #16465

Sjors

ACK 40ad2f6. Those extra tests also pass.

40ad2f6 Have importwallet use ImportPrivKeys and ImportScripts (Andrew Chow) 78941da Optionally allow ImportScripts to set script creation timestamp (Andrew Chow) 94bf156 Have importaddress use ImportScripts and ImportScriptPubKeys (Andrew Chow) a00d1e5 Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions (Andrew Chow) c6a8274 Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys (Andrew Chow) fae7a5b Log when an import is being skipped because we already have it (Andrew Chow) ab28e31 Change ImportScriptPubKeys' internal to apply_label (Andrew Chow) Pull request description: #15741 introduced `ImportPrivKeys`, `ImportPubKeys`, `ImportScripts`, and `ImportScriptPubKeys` in `CWallet` which are used by `importmulti`. This PR changes the remaining `import*` RPCs (`importaddress`, `importprivkey`, `importpubkey`, and `importwallet`) to use these functions as well instead of directly adding the imported items to the wallet. ACKs for top commit: MarcoFalke: ACK 40ad2f6 (checked that behavior changes are mentioned in the commit body) ryanofsky: utACK 40ad2f6. Only change since last review is a tweaked commit message (mentioning label update in importpubkey commit) Sjors: ACK 40ad2f6. Those extra tests also pass. Tree-SHA512: 910e3bbe20b6f8809a47b7293775db234125615d886c7fd99c194f4cdf00c765eb1e24b1799260f1213b98c88f9bbe696796f36087c182925e567d44e9194c98

40ad2f6 Have importwallet use ImportPrivKeys and ImportScripts (Andrew Chow) 78941da Optionally allow ImportScripts to set script creation timestamp (Andrew Chow) 94bf156 Have importaddress use ImportScripts and ImportScriptPubKeys (Andrew Chow) a00d1e5 Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions (Andrew Chow) c6a8274 Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys (Andrew Chow) fae7a5b Log when an import is being skipped because we already have it (Andrew Chow) ab28e31 Change ImportScriptPubKeys' internal to apply_label (Andrew Chow) Pull request description: bitcoin#15741 introduced `ImportPrivKeys`, `ImportPubKeys`, `ImportScripts`, and `ImportScriptPubKeys` in `CWallet` which are used by `importmulti`. This PR changes the remaining `import*` RPCs (`importaddress`, `importprivkey`, `importpubkey`, and `importwallet`) to use these functions as well instead of directly adding the imported items to the wallet. ACKs for top commit: MarcoFalke: ACK 40ad2f6 (checked that behavior changes are mentioned in the commit body) ryanofsky: utACK 40ad2f6. Only change since last review is a tweaked commit message (mentioning label update in importpubkey commit) Sjors: ACK 40ad2f6. Those extra tests also pass. Tree-SHA512: 910e3bbe20b6f8809a47b7293775db234125615d886c7fd99c194f4cdf00c765eb1e24b1799260f1213b98c88f9bbe696796f36087c182925e567d44e9194c98

Summary: Behavior Changes: * Those pubkeys being imported with add_keypool set and are already in the wallet will no longer be added to the keypool This is a partial backport of Core [[bitcoin/bitcoin#16301 | PR16301]] : bitcoin/bitcoin@fae7a5b Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6411

Summary: The internal bool was only to indicate whether the given label should be applied as things that are internal should not have a label. To make this clearer, we change internal to apply_label and invert its usage so things that have labels set this to true in order to have their labels applied. This is a partial backport of Core [[bitcoin/bitcoin#16301 | PR16301]] : bitcoin/bitcoin@ab28e31 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6410

…mportScriptPubKeys Summary: Behavior changes: * If we already have the key, it's wpkh script will still be added, although it should already be there Tis is a partial backport of Core [[bitcoin/bitcoin#16301 | PR16301]] : bitcoin/bitcoin@c6a8274 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6412

… functions Summary: Behavior changes: * If any scripts for the pubkey were already in the wallet, their timestamps will be set to 1 and label updated This is a partial backport of Core [[bitcoin/bitcoin#16301 | PR16301]] : bitcoin/bitcoin@a00d1e5 Depends on D6410 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6413

Summary: Also removes the now unused ImportAddress and ImportScript from rpcdump.cpp Behavior changes: * No errors will be thrown when the script or key already exists in the wallet. * If the key or script is already in the wallet, their labels will be updated. This is a partial backport of Core [[bitcoin/bitcoin#16301 | PR16301]] : bitcoin/bitcoin@94bf156 Depends on D6413 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6414

Summary: Behavior changes: * scripts imported in importmulti that are not explicilty scriptPubKeys will have timestamps set for them This is a partial backport of Core [[bitcoin/bitcoin#16301 | PR16301]] : bitcoin/bitcoin@78941da Depends on D6414 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6415

Summary: Behavior changes: * An "Importing ..." line is logged for every key, even ones that are skipped This is a partial backport of Core [[bitcoin/bitcoin#16301 | PR16301]] : bitcoin/bitcoin@40ad2f6 Depends on D6415 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6416

achow101 force-pushed the imports-use-cwallet-funcs branch from 8ae9638 to 2b0033b Compare June 28, 2019 00:53

fanquake added the Wallet label Jun 28, 2019

achow101 force-pushed the imports-use-cwallet-funcs branch 2 times, most recently from 30ca265 to e66ed3b Compare June 28, 2019 01:18

maflcko added the Refactoring label Jun 28, 2019

promag reviewed Jun 29, 2019

View reviewed changes

ryanofsky reviewed Jul 1, 2019

View reviewed changes

achow101 force-pushed the imports-use-cwallet-funcs branch from e66ed3b to cf2d294 Compare July 2, 2019 00:06

practicalswift reviewed Jul 2, 2019

View reviewed changes

src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved

achow101 mentioned this pull request Jul 5, 2019

Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) #16341

Closed

Sjors reviewed Jul 9, 2019

View reviewed changes

achow101 force-pushed the imports-use-cwallet-funcs branch from cf2d294 to 041e654 Compare July 9, 2019 19:05

instagibbs reviewed Jul 10, 2019

View reviewed changes

achow101 force-pushed the imports-use-cwallet-funcs branch from 041e654 to 0a5b973 Compare July 12, 2019 01:44

achow101 force-pushed the imports-use-cwallet-funcs branch from 0a5b973 to 991ecfd Compare July 12, 2019 20:32

ryanofsky reviewed Jul 16, 2019

View reviewed changes

achow101 force-pushed the imports-use-cwallet-funcs branch 2 times, most recently from 1af1689 to 7a58ad5 Compare July 16, 2019 23:16

promag reviewed Jul 17, 2019

View reviewed changes

achow101 force-pushed the imports-use-cwallet-funcs branch from 7a58ad5 to 35805ce Compare July 17, 2019 15:21

ryanofsky approved these changes Jul 18, 2019

View reviewed changes

achow101 added 2 commits July 18, 2019 20:34

Log when an import is being skipped because we already have it

fae7a5b

Behavior Changes: * Those pubkeys being imported with add_keypool set and are already in the wallet will no longer be added to the keypool

Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and I…

c6a8274

…mportScriptPubKeys Behavior changes: * If we already have the key, it's wpkh script will still be added, although it should already be there

achow101 force-pushed the imports-use-cwallet-funcs branch from 35805ce to bc8ecc0 Compare July 19, 2019 00:36

ryanofsky approved these changes Jul 19, 2019

View reviewed changes

achow101 added 4 commits July 24, 2019 11:42

Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys…

a00d1e5

… functions Behavior changes: * If any scripts for the pubkey were already in the wallet, their timestamps will be set to 1 and label updated

Optionally allow ImportScripts to set script creation timestamp

78941da

Behavior changes: * scripts imported in importmulti that are not explicilty scriptPubKeys will have timestamps set for them

Have importwallet use ImportPrivKeys and ImportScripts

40ad2f6

Behavior changes: * An "Importing ..." line is logged for every key, even ones that are skipped

achow101 force-pushed the imports-use-cwallet-funcs branch from bc8ecc0 to 40ad2f6 Compare July 24, 2019 15:43

ryanofsky approved these changes Jul 24, 2019

View reviewed changes

maflcko added this to the 0.19.0 milestone Jul 25, 2019

Sjors mentioned this pull request Jul 26, 2019

test: Test p2sh-witness and bech32 in wallet_import_rescan #16465

Merged

Sjors approved these changes Jul 26, 2019

View reviewed changes

maflcko merged commit 40ad2f6 into bitcoin:master Jul 26, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Use CWallet::Import* functions in all import* RPCs #16301

Use CWallet::Import* functions in all import* RPCs #16301

Conversation

achow101 commented Jun 28, 2019

Uh oh!

achow101 commented Jun 28, 2019

Uh oh!

DrahtBot commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Jun 28, 2019

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!

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!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DrahtBot commented Jun 28, 2019 •

edited

Loading