wallet/rpc: follow-up clean-up/fixes to avoid_reuse by kallewoof · Pull Request #16239 · bitcoin/bitcoin

kallewoof · 2019-06-19T03:01:33Z

This addresses a few remaining issues pointed out in #13756:

First commit addresses wallet: "avoid_reuse" wallet flag for improved privacy #13756 (comment)
Second commit addresses wallet: "avoid_reuse" wallet flag for improved privacy #13756 (comment)

Ping jnewbery and achow101 as they pointed out these issues.

DrahtBot · 2019-06-19T07:20:14Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16240 (JSONRPCRequest-aware RPCHelpMan by kallewoof)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

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.

promag

Concept ACK.

src/wallet/rpcwallet.cpp

kallewoof · 2019-06-19T14:26:48Z

@promag Thanks a lot for review! I addressed everything you noted.

promag

Sorry for the 2nd round, just 2 nits.

src/wallet/rpcwallet.cpp

test/functional/wallet_avoidreuse.py

kallewoof · 2019-06-19T15:46:33Z

@promag Thanks for thoroughness, it is very appreciated. Addressed all.

maflcko · 2019-06-19T15:49:45Z

I left some review in the other pull, which could be addressed here?

kallewoof · 2019-06-19T16:09:55Z

Addressed @MarcoFalke comments in #13756. Thanks for feedback!

maflcko

ACK, but what about #13756 (comment)

doc/release-notes-13756.md

kallewoof · 2019-06-19T16:55:07Z

@MarcoFalke Thanks, I missed that one. I believe I got all of them this time. Also updated release notes based on your suggestions.

maflcko · 2019-06-19T16:59:19Z

ACK b59d4c7da1b886a0834211e70850127faf3e121e (scrolled through the diff on GitHub)

test/functional/wallet_avoidreuse.py

kallewoof · 2019-06-21T02:38:30Z

@jnewbery thanks! Trimmed tests as suggested.

meshcollider · 2019-06-21T07:55:16Z

Code looks good, code review ACK 7f39c40

I'm just slightly unsure about the word "used" in the getbalance RPC, could seem like the balance which has been used (spent) rather than the balance which is "dirty" or "reused" or something

kallewoof · 2019-06-21T12:29:28Z

I initially used the word "dirty", but then realized that is internally used for something else in the wallet, and there was a comment about "dirty" being confusing in #13756 (comment)

I think "dirty" is a confusing concept. Maybe add an exclamation mark (or a detective icon) next to the transaction and when the user clicks on that, say something like "This address has been used before, for privacy reasons it's better to create a new address each time you wish to receive coins, even from the same person".

(in reference to a GUI comment).

I'm open for changing it, but I'm not sure what would be a good name for it.

jnewbery · 2019-06-21T13:29:34Z

I'm open for changing it, but I'm not sure what would be a good name for it.

How about "used address balance" or "reused address balance"? Bit of a mouthful, but at least it's explicit.

achow101

ACK 7f39c40a332824043e3bef282580daf748131bb4 Read through the diff, looks good.

kallewoof · 2019-06-21T13:54:20Z

@jnewbery A bit long, maybe.. I mean, if we change it I would ideally like to change it everywhere, including the "avoid_reuse" flags in RPC commands and such.

Even "used"/"reused" does not accurately describe what this is referring to, as evident by the fact I as the author screwed it up (thankfully @jnewbery caught that one).

"UTXO/balance that was sent to an address after it was spent from"

jnewbery

ACK 7f39c40a332824043e3bef282580daf748131bb4 (reviewed code and ran tests locally)

I've left a couple of style nits inline. Feel free to take or leave.

src/wallet/rpcwallet.cpp

test/functional/wallet_avoidreuse.py

Always show the same help topic regardless of wallet flags, and explain that something is not always available, rather than runtime-modifying the help output.

jnewbery · 2019-06-21T18:28:33Z

ACK 71d0344

Thanks for quick turnaround!

meshcollider · 2019-06-22T09:58:06Z

re-utACK 71d0344

71d0344 docs: release note wording (Karl-Johan Alm) 3d2ff37 wallet/rpc: use static help text (Karl-Johan Alm) 53c3c1e wallet/rpc/getbalances: add entry for 'mine.used' balance in results (Karl-Johan Alm) Pull request description: This addresses a few remaining issues pointed out in #13756: * First commit addresses #13756 (comment) * Second commit addresses #13756 (comment) Ping jnewbery and achow101 as they pointed out these issues. ACKs for commit 71d034: jnewbery: ACK 71d0344 meshcollider: re-utACK 71d0344 Tree-SHA512: 5e28822af0574ad07dbbed21aa2fe7866bf5770b4c0a1c150ad0da8af3152bcfb7170330a7497fa500326c594740ecf63733cf58325821e2811d7b911d5783a0

71d0344 docs: release note wording (Karl-Johan Alm) 3d2ff37 wallet/rpc: use static help text (Karl-Johan Alm) 53c3c1e wallet/rpc/getbalances: add entry for 'mine.used' balance in results (Karl-Johan Alm) Pull request description: This addresses a few remaining issues pointed out in bitcoin#13756: * First commit addresses bitcoin#13756 (comment) * Second commit addresses bitcoin#13756 (comment) Ping jnewbery and achow101 as they pointed out these issues. ACKs for commit 71d034: jnewbery: ACK 71d0344 meshcollider: re-utACK bitcoin@71d0344 Tree-SHA512: 5e28822af0574ad07dbbed21aa2fe7866bf5770b4c0a1c150ad0da8af3152bcfb7170330a7497fa500326c594740ecf63733cf58325821e2811d7b911d5783a0

d8bd97d Fix GCC 7.4.0 warning (Hennadii Stepanov) Pull request description: Having #13756 and #16239 merged cause GCC warning: ``` $ gcc --version gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0 ... $ make -j 4 > /dev/null wallet/wallet.cpp: In member function ‘CWallet::Balance CWallet::GetBalance(int, bool) const’: wallet/wallet.cpp:2269:45: warning: enumeral and non-enumeral type in conditional expression [-Wextra] isminefilter reuse_filter = avoid_reuse ? 0 : ISMINE_USED; ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~ ``` Fixed with this PR. ACKs for top commit: practicalswift: utACK d8bd97d promag: ACK d8bd97d. kallewoof: utACK d8bd97d meshcollider: Trivial utACK d8bd97d Tree-SHA512: 2fb315ac82f290c8b9f4e48d1b4526b9babe0717c68593c7bc55cd6c289e64b6322aba72984f39291a9254b57d3f6ba8dbfe03799f510c0c1dc108b21b286732

…lance in results Summary: bitcoin/bitcoin@53c3c1e --- Partial backport of Core [[bitcoin/bitcoin#16239 | PR16239]] Test Plan: ninja ./src/bitcoind -regtest -daemon ./src/bitcoin-cli -regtest getbalances help check that it looks right test_runner.py wallet_avoidreuse Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6482

Summary: Always show the same help topic regardless of wallet flags, and explain that something is not always available, rather than runtime-modifying the help output. bitcoin/bitcoin@3d2ff37 --- Partial backport of Core [[bitcoin/bitcoin#16239 | PR16239]] Test Plan: ninja ./src/bitcoind -regtest -daemon ./src/bitcoin-cli -regtest help sendtoaddress ./src/bitcoin-cli -regtest help getbalance ./src/bitcoin-cli -regtest help listunspent check help output looks spiffy test_runner.py wallet_avoidreuse Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6486

Summary: bitcoin/bitcoin@3d2ff37 --- Concludes backport of Core [[bitcoin/bitcoin#16239 | PR16239]] Test Plan: none Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6487

fanquake added Wallet RPC/REST/ZMQ labels Jun 19, 2019

fanquake requested review from achow101 and jnewbery June 19, 2019 03:02

promag reviewed Jun 19, 2019

View reviewed changes

kallewoof force-pushed the 2019-06-followup-avoidreuse branch from 5e704ab to c8568bb Compare June 19, 2019 14:26

promag reviewed Jun 19, 2019

View reviewed changes

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

test/functional/wallet_avoidreuse.py Outdated Show resolved Hide resolved

kallewoof force-pushed the 2019-06-followup-avoidreuse branch from c8568bb to e6564cb Compare June 19, 2019 15:45

kallewoof mentioned this pull request Jun 19, 2019

wallet: "avoid_reuse" wallet flag for improved privacy #13756

Merged

kallewoof force-pushed the 2019-06-followup-avoidreuse branch from e6564cb to 5ff6aaa Compare June 19, 2019 16:09

maflcko reviewed Jun 19, 2019

View reviewed changes

doc/release-notes-13756.md Outdated Show resolved Hide resolved

doc/release-notes-13756.md Outdated Show resolved Hide resolved

kallewoof force-pushed the 2019-06-followup-avoidreuse branch from 5ff6aaa to b59d4c7 Compare June 19, 2019 16:54

jnewbery reviewed Jun 20, 2019

View reviewed changes

test/functional/wallet_avoidreuse.py Outdated Show resolved Hide resolved

test/functional/wallet_avoidreuse.py Outdated Show resolved Hide resolved

kallewoof force-pushed the 2019-06-followup-avoidreuse branch from b59d4c7 to 7f39c40 Compare June 21, 2019 02:17

achow101 approved these changes Jun 21, 2019

View reviewed changes

jnewbery reviewed Jun 21, 2019

View reviewed changes

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

test/functional/wallet_avoidreuse.py Outdated Show resolved Hide resolved

kallewoof added 2 commits June 22, 2019 02:45

wallet/rpc/getbalances: add entry for 'mine.used' balance in results

53c3c1e

wallet/rpc: use static help text

3d2ff37

Always show the same help topic regardless of wallet flags, and explain that something is not always available, rather than runtime-modifying the help output.

docs: release note wording

71d0344

kallewoof force-pushed the 2019-06-followup-avoidreuse branch from 7f39c40 to 71d0344 Compare June 21, 2019 17:46

meshcollider merged commit 71d0344 into bitcoin:master Jun 22, 2019

kallewoof deleted the 2019-06-followup-avoidreuse branch June 24, 2019 04:38

hebasto mentioned this pull request Jun 25, 2019

refactoring: wallet: Fix GCC 7.4.0 warning #16286

Merged

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

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kallewoof commented Jun 19, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kallewoof commented Jun 19, 2019

Uh oh!

maflcko commented Jun 19, 2019

Uh oh!

kallewoof commented Jun 19, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kallewoof commented Jun 19, 2019

Uh oh!

maflcko commented Jun 19, 2019

Uh oh!

Uh oh!

Uh oh!

kallewoof commented Jun 21, 2019

Uh oh!

meshcollider commented Jun 21, 2019

Uh oh!

kallewoof commented Jun 21, 2019

Uh oh!

jnewbery commented Jun 21, 2019

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Jun 21, 2019

Uh oh!

meshcollider commented Jun 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kallewoof commented Jun 19, 2019 •

edited

Loading

DrahtBot commented Jun 19, 2019 •

edited

Loading

kallewoof commented Jun 21, 2019 •

edited

Loading