Page MenuHomePhabricator

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

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

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCd36def6deacb: Merge #9662: Add createwallet "disableprivatekeys" option: a sane mode for…
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.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nakihito created this revision.Tue, Oct 29, 19:25
Owners added a reviewer: Restricted Owners Package.Tue, Oct 29, 19:25
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Oct 29, 19:25
nakihito planned changes to this revision.Tue, Oct 29, 19:25
nakihito requested review of this revision.Tue, Oct 29, 19:26
nakihito edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Wed, Oct 30, 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.Wed, Oct 30, 16:36
nakihito updated this revision to Diff 13793.Wed, Oct 30, 20:38

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

deadalnix requested changes to this revision.Sat, Nov 2, 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.Sat, Nov 2, 18:49
nakihito planned changes to this revision.Mon, Nov 4, 07:38
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.

nakihito updated this revision to Diff 13883.Mon, Nov 4, 07:39

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

nakihito planned changes to this revision.Mon, Nov 4, 07:39
nakihito requested review of this revision.Mon, Nov 4, 19:34
deadalnix added inline comments.Mon, Nov 18, 00:23
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.Tue, Nov 19, 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.Tue, Nov 19, 15:04
nakihito updated this revision to Diff 14210.Tue, Nov 19, 19:26

Fixed comment formatting.

nakihito added inline comments.Tue, Nov 19, 19:28
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

Fabien accepted this revision.Tue, Nov 19, 20:19
deadalnix accepted this revision.Wed, Nov 20, 14:30
This revision is now accepted and ready to land.Wed, Nov 20, 14:30