Page MenuHomePhabricator

Merge #9662: Add createwallet "disableprivatekeys" option: a sane mode for watchonly-wallets
ClosedPublic

Authored by nakihito on Oct 29 2019, 19:25.

Details

Summary

a3fa4d6a6acf19d640a1d5879a00aa1f059e2380 QA: Fix bug in -usecli logic that converts booleans to non-lowercase strings (Jonas Schnelli)
4704e5f074e57782d058404a594a7313cf170cf0 [QA] add createwallet disableprivatekey test (Jonas Schnelli)
c7b8f343e99d9d53ea353ddce9a977f1886caf30 [Qt] Disable creating receive addresses when private keys are disabled (Jonas Schnelli)
2f15c2bc20d583b4c1788da78c9c635c36e03ed0 Add disable privatekeys option to createwallet (Jonas Schnelli)
cebefba0855cee7fbcb9474b34e6779369e8e9ce Add option to disable private keys during internal wallet creation (Jonas Schnelli)
9995a602a639b64a749545b7c3bafbf67f97324f Add facility to store wallet flags (64 bits) (Jonas Schnelli)

Pull request description:

This mode ('createwallet {"disableprivatekeys": true}') is intended for a sane pure watch-only mode, ideal for a use-case where one likes to use Bitcoin-Core in conjunction with a hardware-wallet or another solutions for cold-storage.

Since we have support for custom change addresses in `fundrawtransaction`, pure watch-only wallets including coin-selection are possible and do make sense for some use cases.

This new mode disables all forms of private key generation and ensure that no mix between hot and cold keys are possible.

Tree-SHA512: 3ebe7e8d54c4d4e5f790c348d4c292d456f573960a5b04d69ca5ef43a9217c7e7671761c6968cdc56f9a8bc235f3badd358576651af9f10855a0eb731f3fc508

Backport of Core PR9662
https://github.com/bitcoin/bitcoin/pull/9662/

Depends on D4175

Test Plan
make check
test_runner.py
bitcoin-qt
Help -> Debug -> Console
createwallet noprivkey true

Select noprivkey wallet in the main window
In the Receive menu, the Request Payment button should be greyed out and not clickable.

pr9662post.png (530×846 px, 25 KB)

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a reviewer: Restricted Owners Package.Oct 29 2019, 19:25
nakihito edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Oct 30 2019, 16:36

You are missing the usecli version of the test.

src/wallet/rpcwallet.cpp
3510 ↗(On Diff #13745)

C++ style cast

src/wallet/wallet.cpp
1616 ↗(On Diff #13745)

Fix braces

4564 ↗(On Diff #13745)

...

This revision now requires changes to proceed.Oct 30 2019, 16:36

Fixed nits, added --usecli version of new test, and updated timing.json with new tests.

deadalnix requested changes to this revision.Nov 2 2019, 18:49
deadalnix added inline comments.
src/wallet/rpcwallet.cpp
3511 ↗(On Diff #13793)

Why is that a static cast ? uint64_t(WALLET_FLAG_DISABLE_PRIVATE_KEYS) doesn't work ?

src/wallet/wallet.h
1366 ↗(On Diff #13793)

Relayout

This revision now requires changes to proceed.Nov 2 2019, 18:49
nakihito added inline comments.
src/wallet/rpcwallet.cpp
3511 ↗(On Diff #13793)

The functional cast does work, but to my knowledge its a generally less safe casting method because it tries all cast types and uses which ever one works first (I believe the first three it tries are const, static, and then dynamic casts). In this case, it would likely end up as a static cast. I will be reverting to the functional cast because this is a rather trivial change and there is no need to give ourselves ownership of this code on the off chance that Core makes some changes that affect this code.

Reverted static_cast to functional cast to prevent issues of code ownership and reformatted a comment.

nakihito requested review of this revision.Nov 4 2019, 19:34
src/wallet/rpcwallet.cpp
3511 ↗(On Diff #13793)

Your answer is applicable to C-style casts. That is not what I suggest you to use.

Fabien requested changes to this revision.Nov 19 2019, 15:04
Fabien added inline comments.
src/wallet/wallet.cpp
4548 ↗(On Diff #13883)

It's not what is in the PR.

src/wallet/wallet.h
108 ↗(On Diff #13883)

This block of comments and the one above need some love: sentences start capitalized and end with a dot.

Edit: there a several places like this in this diff.

1359 ↗(On Diff #13883)

Dito.

1362 ↗(On Diff #13883)

Dito.

1368 ↗(On Diff #13883)

Dito.

This revision now requires changes to proceed.Nov 19 2019, 15:04

Fixed comment formatting.

src/wallet/wallet.cpp
4548 ↗(On Diff #13883)

Sorry, forgot to make a note about this one.
This change was made in D4179 (out of order backport). See https://github.com/bitcoin/bitcoin/pull/13774/files#diff-b2bb174788c7409b671c46ccc86034bdR4082

This revision is now accepted and ready to land.Nov 20 2019, 14:30