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

Do not import private keys to wallets with private keys disabled #15235

Merged
merged 3 commits into from Feb 1, 2019

Conversation

Projects
None yet
9 participants
@achow101
Copy link
Member

commented Jan 23, 2019

Fixes a bug where private keys could be imported to wallets with private keys disabled. Now every RPC which can import private keys checks for whether the wallet has private keys are disabled and errors if it is. Also added an belt-and-suspenders check to AddKeyPubkeyWithDB to have it assert that the wallet has private keys enabled.

@fanquake fanquake added the Wallet label Jan 23, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15226 (Allow creating blank (empty) wallets (alternative) by achow101)
  • #15093 (rpc: Change importwallet to return additional errors by marcinja)
  • #15032 (Nit fixes for PR 14565 by MeshCollider)
  • #15024 (Allow specific private keys to be derived from descriptor by MeshCollider)
  • #15006 (Add option to create an encrypted wallet by achow101)
  • #14491 (Allow descriptor imports with importmulti by MeshCollider)
  • #14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)
  • #14021 (Import key origin data through descriptors in importmulti by achow101)

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.

@Sjors
Copy link
Member

left a comment

Concept ACK. Travis is unhappy. On my own machine test/functional/wallet_disableprivatekeys.py failed when it was part of the full test suite, but not when run individually. Odd...

schermafbeelding 2019-01-23 om 11 50 58

sethdseed should also have this check, since it currently throws a confusing "Cannot set a HD seed on a non-HD wallet."

Maybe add the same check to addmultisigaddress, since the help says "This functionality is only intended for use with non-watchonly addresses."?

ab677ed looks good otherwise

Suggest tagging this for v0.18 and 0.17 backport.

By the way, we never elaborated on disableprivatekey in the release notes.

src/wallet/rpcdump.cpp Outdated
@@ -549,6 +552,10 @@ UniValue importwallet(const JSONRPCRequest& request)
+ HelpExampleRpc("importwallet", "\"test\"")
);

if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled when private keys are disabled");

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 23, 2019

Member

Add todo that this should permit importing a wallet that also has private keys disabled? (I assume currently that information isn't stored in the dump)

This comment has been minimized.

Copy link
@achow101

achow101 Jan 24, 2019

Author Member

Done

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 24, 2019

Member

Please open an issue for this as well, a TODO in the source code hasn't historically turned out very effective.

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 25, 2019

Member

@laanwj wrote:

Please open an issue for this as well, a TODO in the source code hasn't historically turned out very effective.

Although it says "todo", I see it more as an explanation that this is strictly speaking a bug, and that we should pay attention to this if we move to descriptor based wallets and (presumably) overhaul the dump and import commands.

@achow101

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

The test failure is running wallet_disableprivatekeys.py with --usecli. It seems that bitcoin-cli is erroring, giving: error: Error parsing JSON:[{'timestamp': 'now', 'scriptPubKey': {'address': 'n2xmfzEaZi2XyjFYCXxfMbUHyKGk2PaL2k'}, 'keys': ['cQdMcowhL91CXuSRs7FSaujMPNArV8dLZcjq16kHDSDgNaAVTB99']}] (tested on my own machine with some debugging changes)

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

So that's the importmulti call? Check if CRPCConvertParam in src/rpc/client.cpp contains all the non-string params; I think there's a bunch of RPC calls where that's not up to date.

@achow101 achow101 force-pushed the achow101:fix-noprivs-import branch to 0efac0c Jan 23, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

I fixed the test failure. It's because --usecli did not properly handle arguments that are dicts and lists.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

utACK d8ac347
Somehow this was missed in #9662.

IMO this is a bugfix and should be backported.

@achow101 achow101 force-pushed the achow101:fix-noprivs-import branch from 0efac0c to 2fa2a44 Jan 24, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2019

sethdseed should also have this check, since it currently throws a confusing "Cannot set a HD seed on a non-HD wallet."

Done

Maybe add the same check to addmultisigaddress, since the help says "This functionality is only intended for use with non-watchonly addresses."?

I don't think that's necessary. IMO we should only have this warning for anything where private keys can be imported.

(Missed these in the earlier push)

@Sjors

Sjors approved these changes Jan 24, 2019

Copy link
Member

left a comment

tACK 2fa2a44

pos_args = [str(arg).lower() if type(arg) is bool else str(arg) for arg in args]
named_args = [str(key) + "=" + str(value) for (key, value) in kwargs.items()]
pos_args = [str(arg).lower() if type(arg) is bool else (json.dumps(arg) if (isinstance(arg, dict) or isinstance(arg, list)) else str(arg)) for arg in args]
named_args = [str(key) + "=" + (json.dumps(value) if (isinstance(value, dict) or isinstance(value, list)) else str(value)) for (key, value) in kwargs.items()]

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 24, 2019

Member

paging @jnewbery, looks ok?

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 24, 2019

Member

Might want to use named function, say value_to_cli, here for clarity, and avoid repetition; this duplicated expression keeps getting longer

Edit: I also think it's wrong that booleans are handled differently for pos and named args.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jan 24, 2019

Member

Definitely agree with pulling this monstrosity out into a function :)

bools in named args were fixed here: #14938 (comment) but that PR wasn't merged.

This comment has been minimized.

Copy link
@achow101

achow101 Jan 24, 2019

Author Member

I've pulled this into a new function which is easier to read. It also fixes the bools in named args issue.

This comment has been minimized.

Copy link
@achow101

achow101 Jan 31, 2019

Author Member

Since this issue affects multiple PRs, I've split it into a separate PR: #15301

@achow101 achow101 force-pushed the achow101:fix-noprivs-import branch 2 times, most recently from bc3d735 to 553d599 Jan 24, 2019

tests: unify RPC argument to cli argument conversion and handle dicts…
… and lists

When running tests with --usecli, unify the conversion from argument objects to
strings using a new function arg_to_cli(). This fixes boolean arguments when
using named arguments.

Also use json.dumps() to get the string values for arguments that are dicts and
lists so that bitcoind's JSON parser does not become confused.

@achow101 achow101 force-pushed the achow101:fix-noprivs-import branch from 553d599 to 7b70a14 Jan 24, 2019

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

Concept ACK. (maybe we should have a warning that prints in the debug log if you load a non-private-key wallet with private keys in it, like we do for overlong multisig.)

@MarcoFalke MarcoFalke added this to the 0.17.2 milestone Jan 24, 2019

@Sjors

Sjors approved these changes Jan 25, 2019

Copy link
Member

left a comment

re-tACK 7b70a14 on macOS (without --enable-debug, because that will crash until the next rebase)

src/wallet/rpcdump.cpp Outdated
@@ -549,6 +552,10 @@ UniValue importwallet(const JSONRPCRequest& request)
+ HelpExampleRpc("importwallet", "\"test\"")
);

if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled when private keys are disabled");

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 25, 2019

Member

@laanwj wrote:

Please open an issue for this as well, a TODO in the source code hasn't historically turned out very effective.

Although it says "todo", I see it more as an explanation that this is strictly speaking a bug, and that we should pay attention to this if we move to descriptor based wallets and (presumably) overhaul the dump and import commands.

@achow101 achow101 force-pushed the achow101:fix-noprivs-import branch from 7b70a14 to d9d6bed Jan 25, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

I've gotten rid of the todo in importwallet and just implemented it. The latest push refactors importwallet so that the import happens in two steps. First the import file is read and the keys and scripts extracted. Then the check for private keys being imported and private keys disabled is done. Then it imports. This will avoid importing wallet dump file that have any private keys and ensure that nothing is imported at all if the file has private keys.

@Sjors
Copy link
Member

left a comment

I'm not sure if d9d6bed is working correctly, or if I'm seeing an already existing bug in dumpwallet:

When I dump a wallet that I created a few months ago, the oldest key in the dump file is 2018-08-01T14:51:24Z, but all the scripts have 0 in the time column. If I then import that into a fresh wallet, it triggers a rescan from genesis.

The line if (time > 0) should prevent nTimeBegin from becoming 0 this way, but see my comment where I think this goes wrong.

}
}
file.close();
for (unsigned int i = 0; i < keys.size(); ++i) {

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 31, 2019

Member

nit: calculate total = keys.size() + scripts.size() before the loop, then set progress=0

for ( const auto& key_tuple : keys ) {
... ShowProgress(... progress / total * 100)
...
progress++

This comment has been minimized.

Copy link
@achow101

achow101 Jan 31, 2019

Author Member

Done

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 31, 2019

Member

re-nit: still missing the for ( const auto& key_tuple : keys ) bit :-)

This comment has been minimized.

Copy link
@achow101

achow101 Jan 31, 2019

Author Member

Oops fixed

}
if (time > 0) {
pwallet->m_script_metadata[id].nCreateTime = time;
nTimeBegin = std::min(nTimeBegin, time);

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 31, 2019

Member

Shouldn't this be birthtime? int64_t birth_time = DecodeDumpTime(vstr[1]) (because all script timestamps are 0 in the dump). This line should be moved outside the loop probably.

This comment has been minimized.

Copy link
@achow101

achow101 Jan 31, 2019

Author Member

No because birth_time just becomes time in this loop.

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 31, 2019

Member

Ok, but then what's causing the full rescan?

Just tried on HEAD~2 and getting the same thing, so I'm blaming my own wallet. Just tried this again with a fresh wallet that only has one transaction. Now the rescan is a modest 10623 blocks (I guess that's a built in safety margin?).

I also got a ton of Skipping import of ... (script already present) messages both my original weird wallet and on the fresh one. That doesn't make sense to me, because I'm importing into a brand new wallet. But it's not new behavior.

This comment has been minimized.

Copy link
@achow101

achow101 Jan 31, 2019

Author Member

I'm not sure what might be causing that problem. However the Skipping import messages for scripts is expected since segwit scripts are in the dump, but already automatically imported when the keys were imported.

@achow101 achow101 force-pushed the achow101:fix-noprivs-import branch 2 times, most recently from afcbee8 to 2911ef6 Jan 31, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

tACK 2911ef6

achow101 added some commits Jan 25, 2019

Refactor importwallet to extract data from the file and then import
Instead of importing keys and scripts as each line in the file is
read, first extract the data then import them.

@achow101 achow101 force-pushed the achow101:fix-noprivs-import branch from 2911ef6 to e6c58d3 Jan 31, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

re-tACK 2911ef6

@laanwj laanwj merged commit e6c58d3 into bitcoin:master Feb 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 1, 2019

Merge #15235: Do not import private keys to wallets with private keys…
… disabled

e6c58d3 Do not import private keys to wallets with private keys disabled (Andrew Chow)
b5c5021 Refactor importwallet to extract data from the file and then import (Andrew Chow)
1f77f67 tests: unify RPC argument to cli argument conversion and handle dicts and lists (Andrew Chow)

Pull request description:

  Fixes a bug where private keys could be imported to wallets with private keys disabled. Now every RPC which can import private keys checks for whether the wallet has private keys are disabled and errors if it is. Also added an belt-and-suspenders check to `AddKeyPubkeyWithDB` to have it assert that the wallet has private keys enabled.

Tree-SHA512: 5cd04febce9aa2bd9bfd02f312c6ff8705e37278cae59efd3895f6d6e2f1b477aefd297e2dd0860791bdd3d4f3cad8eb1a404f8f3d4e2035b91314ad2c1028ae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.