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

rpcwallet: default include_watchonly to true for watchonly wallets #16383

Merged
merged 4 commits into from Aug 16, 2019

Conversation

@jb55
Copy link
Contributor

commented Jul 13, 2019

Right now it's a bit annoying to deal with watchonly wallets, many rpc commands have an include_watchonly argument that needs to be explicitly set.

Wallets created with createwallet can have a disable_private_keys parameter, for those wallets we already know that they are watchonly, so there's no reason to have to explicitly ask for it for every command. Instead we check this wallet flag when the include_watchonly parameter isn't set.

@MarcoFalke
Copy link
Member

left a comment

Would have to update the documentation as well for the "smart" default value.

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

@jb55 jb55 force-pushed the jb55:20190713-watchonly-defaults branch from 5bd7c0c to 2370175 Jul 13, 2019

@achow101

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

Concept ACK. I think having include_watchonly default to true for actual watchonly wallets is super useful.

@jb55 jb55 force-pushed the jb55:20190713-watchonly-defaults branch from 2370175 to d60e77c Jul 13, 2019

@jb55

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

@MarcoFalke I'm a bit new, which docs specifically would need to be updated?

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

@MarcoFalke I'm a bit new, which docs specifically would need to be updated?

-                    {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Whether to include watch-only addresses in balance calculation and details[]"},
+                    {"include_watchonly", RPCArg::Type::BOOL, /* default */ ">>>>>>>>>>> update here<<<<<<<<", "Whether to include watch-only addresses in balance calculation and details[]"},
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@jonatack
Copy link
Contributor

left a comment

Concept ACK. Will test when help docs are updated. Seems this change would merit test coverage?

@jb55 jb55 force-pushed the jb55:20190713-watchonly-defaults branch 2 times, most recently from 9a33c13 to 3c145c3 Jul 13, 2019

@jb55

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

@MarcoFalke I wasn't quite sure what to do for documenting the smart default, let me know if that looks ok

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 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:

  • #16397 (Clarify includeWatching for fundrawtransaction by stevenroose)
  • #16185 (gettransaction: add an argument to decode the transaction by darosior)
  • #15729 (rpc: Raise error in getbalance if minconf is not zero by promag)
  • #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)

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.

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

@jb55 jb55 force-pushed the jb55:20190713-watchonly-defaults branch from 3c145c3 to cc0d834 Jul 14, 2019

@promag
Copy link
Member

left a comment

Concept ACK

Seems this change would merit test coverage?

Agree with @jonatack. Also add a minor release note?

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

Concept ACK, this definitely seems like the only sensible default to have in that case.

@instagibbs

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

concept ACK, needs tests(as others have said)

@jb55

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

One thing I missed was rpcs with the includeWatching option such as fundrawtransaction and walletcreatefundedpsbt. I think it makes sense to add the default on those as well.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Concept ACK

@jb55 jb55 force-pushed the jb55:20190713-watchonly-defaults branch from cc0d834 to efbe2c9 Jul 18, 2019

jb55 added 2 commits Jul 13, 2019
rpcwallet: default include_watchonly to true for watchonly wallets
The logic before would only include watchonly addresses if it was
explicitly set in the rpc argument.

This changes the logic like so:

If the include_watchonly argument is missing, check the
WALLET_FLAG_DISABLE_PRIVATE_KEYS flag to determine if we're working
with a watchonly wallet. If so, default include_watchonly to true.

If the include_watchonly argument is explicit set to false, we still
disable them from the listing. Although this would always return
nothing, it might be still useful in situations where you want to
explicitly filter out watchonly addresses regardless of what wallet
you are dealing with.

Signed-off-by: William Casarin <jb55@jb55.com>
rpcwallet: document include_watchonly default for watchonly wallets
Signed-off-by: William Casarin <jb55@jb55.com>

@jb55 jb55 force-pushed the jb55:20190713-watchonly-defaults branch 2 times, most recently from 4caca1d to c7cbf29 Jul 18, 2019

@jb55

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Latest updates:

  • Added the new defaulting logic to walletcreatefundedpsbt and fundrawtransaction's includeWatching option.
  • Fixed @promag's nits
  • Added release notes
  • Added functional tests for the new defaults

This is the first time I've done the last two things, so let me know if I did anything wrong.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Concept ACK

Good idea!

jb55 added 2 commits Jul 16, 2019
doc: add release note for include_watchonly default changes
Signed-off-by: William Casarin <jb55@jb55.com>
tests: functional watch-only wallet tests
These test the new watch-only defaults for rpcs with include_watchonly
and includeWatching options.

Signed-off-by: William Casarin <jb55@jb55.com>

@jb55 jb55 force-pushed the jb55:20190713-watchonly-defaults branch from c7cbf29 to 72eaab0 Jul 24, 2019

@achow101

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Code review ACK 72eaab0

@promag
Copy link
Member

left a comment

ACK 72eaab0, code review only, didn't look closely to the test.


RPCs which have an `include_watchonly` argument or `includeWatching`
option now default to `true` for watch-only wallets. Affected RPCs
are: `getbalance`, `listreceivedbyaddress`, `listreceivedbylabel`,

This comment has been minimized.

Copy link
@promag

promag Jul 31, 2019

Member

nit, sort.

assert_raises_rpc_error(-4, "Insufficient funds", wo_wallet.fundrawtransaction, rawtx, no_wo_options)



This comment has been minimized.

Copy link
@promag

promag Jul 31, 2019

Member

nit, remove extra line.

txid = def_wallet.sendtoaddress(wo_addr, 1)
self.nodes[0].generate(1)

# getbalance

This comment has been minimized.

Copy link
@promag

promag Jul 31, 2019

Member

nit, drop comment.

@meshcollider

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

This makes a lot of sense, Concept ACK

@Sjors

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

ACK 72eaab0

@kallewoof
Copy link
Member

left a comment

ACK 72eaab0

Lots of unnecessary newlines all over, and a few nits, but merge-worthy IMO!

@@ -52,6 +52,23 @@ static inline bool GetAvoidReuseFlag(CWallet * const pwallet, const UniValue& pa
return avoid_reuse;
}


This comment has been minimized.

Copy link
@kallewoof

kallewoof Aug 16, 2019

Member

Nit: Extraneous newline.


// otherwise return whatever include_watchonly was set to
return include_watchonly.get_bool();
}

This comment has been minimized.

Copy link
@kallewoof

kallewoof Aug 16, 2019

Member

Can do

return include_watchonly.isNull()
    ? pwallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)
    : include_watchonly.get_bool();
return include_watchonly.get_bool();
}


This comment has been minimized.

Copy link
@kallewoof

kallewoof Aug 16, 2019

Member

Nit: Extraneous newline.

@fanquake
Copy link
Member

left a comment

ACK 72eaab0 - I've looked over the changes, they make sense to me. Compiled and ran the tests etc.

Thanks to @kallewoof and @ajtowns (he said he sanity checked on his lunch break) for following up my review prod.

I know there are some nits here, however this has ACKs, and has been sitting for a while, so I'm going to merge it. The nits can be scooped up by someone in either a follow up PR, or a related change.

@fanquake fanquake merged commit 72eaab0 into bitcoin:master Aug 16, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fanquake added a commit that referenced this pull request Aug 16, 2019
Merge #16383: rpcwallet: default include_watchonly to true for watcho…
…nly wallets

72eaab0 tests: functional watch-only wallet tests (William Casarin)
72ffbdc doc: add release note for include_watchonly default changes (William Casarin)
003a3c7 rpcwallet: document include_watchonly default for watchonly wallets (William Casarin)
a50d9e6 rpcwallet: default include_watchonly to true for watchonly wallets (William Casarin)

Pull request description:

  Right now it's a bit annoying to deal with watchonly wallets, many rpc commands have an `include_watchonly` argument that needs to be explicitly set.

  Wallets created with `createwallet` can have a `disable_private_keys` parameter, for those wallets we already know that they are watchonly, so there's no reason to have to explicitly ask for it for every command. Instead we check this wallet flag when the `include_watchonly` parameter isn't set.

ACKs for top commit:
  achow101:
    Code review ACK 72eaab0
  Sjors:
    ACK 72eaab0
  promag:
    ACK 72eaab0, code review only, didn't look closely to the test.
  kallewoof:
    ACK 72eaab0
  fanquake:
    ACK 72eaab0 - I've looked over the changes, they make sense to me. Compiled and ran the tests etc.

Tree-SHA512: d3646b55e97f386594d7efc994f0712f3888475c6a5dc7f131ac9f8c49bf5d4677182b88f42b34152abe1ad101ecadd152b4c20e9d3c1267190db36f77ab8bd7
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 17, 2019
Merge bitcoin#16383: rpcwallet: default include_watchonly to true for…
… watchonly wallets

72eaab0 tests: functional watch-only wallet tests (William Casarin)
72ffbdc doc: add release note for include_watchonly default changes (William Casarin)
003a3c7 rpcwallet: document include_watchonly default for watchonly wallets (William Casarin)
a50d9e6 rpcwallet: default include_watchonly to true for watchonly wallets (William Casarin)

Pull request description:

  Right now it's a bit annoying to deal with watchonly wallets, many rpc commands have an `include_watchonly` argument that needs to be explicitly set.

  Wallets created with `createwallet` can have a `disable_private_keys` parameter, for those wallets we already know that they are watchonly, so there's no reason to have to explicitly ask for it for every command. Instead we check this wallet flag when the `include_watchonly` parameter isn't set.

ACKs for top commit:
  achow101:
    Code review ACK 72eaab0
  Sjors:
    ACK 72eaab0
  promag:
    ACK 72eaab0, code review only, didn't look closely to the test.
  kallewoof:
    ACK 72eaab0
  fanquake:
    ACK 72eaab0 - I've looked over the changes, they make sense to me. Compiled and ran the tests etc.

Tree-SHA512: d3646b55e97f386594d7efc994f0712f3888475c6a5dc7f131ac9f8c49bf5d4677182b88f42b34152abe1ad101ecadd152b4c20e9d3c1267190db36f77ab8bd7
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.