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

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

Merged
merged 10 commits into from Jun 18, 2019

Conversation

@kallewoof
Copy link
Member

commented Jul 25, 2018

Add a new wallet flag called avoid_reuse which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination.

This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments.

This replaces #10386 and together with the (now merged) #12257 it addresses #10065 in full. The concerns raised in #10386 (comment) are also addressed due to #12257.

Note: this builds on top of #15780. (merged)

@fanquake fanquake added the Wallet label Jul 25, 2018

@promag
Copy link
Member

left a comment

Concept ACK.

Should we also disallow sending to a dirty address? If we don't then those coins can't be spend if the flag is set, or am I missing something?

Show resolved Hide resolved test/functional/feature_avoidreuse.py Outdated
Show resolved Hide resolved src/wallet/init.cpp Outdated
Show resolved Hide resolved src/wallet/coincontrol.h Outdated
Show resolved Hide resolved src/wallet/wallet.cpp Outdated
Show resolved Hide resolved src/wallet/wallet.cpp Outdated
Show resolved Hide resolved src/wallet/wallet.cpp Outdated
@practicalswift

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

Concept ACK

Nice work!

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

Perhaps fodder for a separate PR, but listtransactions and the GUI should get indicators that an incoming payment was dirty when received. By having that parties that reuse addresses will start getting a frowny-recycle-icon or whatever on their payments, which will increase awareness that their behaviour has downsides.

Edit: I see I'm repeating myself from an earlier PR. :P

@kallewoof kallewoof force-pushed the kallewoof:feature-avoidreuse branch 3 times, most recently Jul 26, 2018

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2018

@promag

Should we also disallow sending to a dirty address? If we don't then those coins can't be spend if the flag is set, or am I missing something?

You can always spend them by using the allowdirty flag or by manually selecting them using coin control (via createrawtx or via the GUI).

I do think we should at least warn users about that kind of behaviour though, but it feels like a UI fix that can come later.

@gmaxwell

Perhaps fodder for a separate PR, but listtransactions and the GUI should get indicators that an incoming payment was dirty when received. By having that parties that reuse addresses will start getting a frowny-recycle-icon or whatever on their payments, which will increase awareness that their behaviour has downsides.

I will see about making that the case for the CLI side of things. I think a GUI side fix would be great, but unfortunately I don't seem to be talented at writing QT code. (Practice, I guess.)

Edit: I see I'm repeating myself from an earlier PR. :P

Sorry about that. I felt like it was worthwhile to re-PR since the old PR has a lot of outdated talk that is mitigated by #12257.

@kallewoof kallewoof force-pushed the kallewoof:feature-avoidreuse branch Jul 26, 2018

@DrahtBot DrahtBot removed the Needs rebase label Jul 26, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16215 (gui: Refactor wallet controller activities by promag)
  • #15937 (WIP: Add loadwallet and createwallet load_on_startup options by ryanofsky)
  • #15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
  • #15906 ([wallet] Remove AvailableCoins nMinDepth argument by amitiuttarwar)
  • #15729 (rpc: Raise error in getbalance if minconf is not zero by promag)
  • #15450 ([GUI] Create wallet menu option by achow101)
  • #15064 ([PoC] GUI: Migrate BIP70 merchant info to mapValue["to"] by luke-jr)
  • #14533 ([WIP] wallet: ensure wallet files are not reused across chains by mrwhythat)
  • #11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)
  • #10615 (RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet by luke-jr)
  • #9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

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

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

@gmaxwell:

the GUI should get indicators that an incoming payment was dirty when received

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".

Detective icon is a nice hint that there's privacy related information, but an exclamation mark can also be used to explain other potential problems with an incoming transaction, e.g. if the fee is extremely low.

A parallel measure, that doesn't involve UI changes, could be for coin selection to not spend from dirty addresses as long as possible. And then try to spend them if there's an exact match (no other outputs, no change), perhaps even only if there's no other exact match, depending on how strongly you want to avoid spending these things.

When the user wants to "spend all" funds and the dirty amount is less than x%, a dialog could popup and suggest to "exclude a small amount for privacy reasons".

This is the kind of behavior that if we want people to use it, should be on by default. That can wait for another PR, but it's useful to think about it a bit when designing the RPC. -avoidreuse doesn't really capture the above. It's too aggressive / binary an option to be useful in a GUI IMO.

Telling people to manually go into coin selection is not a good idea. If a user receives a non-trivial amount, they expect to be able to just spend it with no additional hoops. That means in the current form it's probably never a user friendly default.

@promag

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

Maybe add an exclamation mark

@Sjors I like that, and then a tooltip/popup could explain the warning.

-avoidreuse doesn't really capture the above. It's too aggressive / binary an option to be useful in a GUI IMO.

"Avoid" is not binary right?

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2018

I think "dirty" is a confusing concept.

It's called "dirty" in the CLI as well, but there's also a different concept in the wallet code called 'dirty' which is completely unrelated. In short, it may be a good idea to rename this feature, but I can't think of a good name. "Compromised" is too long. "Seen" is too vague. @luke-jr any ideas?

Detective icon is a nice hint that there's privacy related information, but an exclamation mark can also be used to explain other potential problems with an incoming transaction, e.g. if the fee is extremely low.

I like "!" too. If it is used for multiple things, it could take away from the importance though (e.g. user ignores it thinking it's 'just cause of the 1 sat/b fee').

As a sidenote, we also should mark UTXOs in coin selection dialog somehow. Same approach? Maybe need to include a warning popup if they mix dirty and clean.

A parallel measure, that doesn't involve UI changes, could be for coin selection to not spend from dirty addresses as long as possible. And then try to spend them if there's an exact match (no other outputs, no change), perhaps even only if there's no other exact match, depending on how strongly you want to avoid spending these things.

I don't know, I think we should always avoid dirty inputs unless the user explicitly marks them as 'clean' or picks them directly using manual coin control.

When the user wants to "spend all" funds and the dirty amount is less than x%, a dialog could popup and suggest to "exclude a small amount for privacy reasons".

I didn't think about this case, and it's probably fairly common. Then again, I don't think I can do a lot from the CLI side, so this is probably GUI level stuff.

This is the kind of behavior that if we want people to use it, should be on by default. That can wait for another PR, but it's useful to think about it a bit when designing the RPC. -avoidreuse doesn't really capture the above. It's too aggressive / binary an option to be useful in a GUI IMO.

What about it is too aggressive?

Telling people to manually go into coin selection is not a good idea. If a user receives a non-trivial amount, they expect to be able to just spend it with no additional hoops. That means in the current form it's probably never a user friendly default.

Maybe it should be sensitive to the amount. A good middle ground may be to, for any input that is X times higher than current dust threshold, that goes to an already spent-from address, to show the user a dialog saying the input is dirty and ask them whether to force-mark it as clean or keep it as dirty.

@Sjors

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

What about it is too aggressive?

A user might receive their entire salary on a reused address. If the current implementation of -avoidreuse=1 were to become the default, then the UI would need to honor that setting. That means at minimum asking the user for confirmation when they're about to spend these funds. But that would be a very unintuitive question. The Spend screen is not the right place to handle this. Warning the user when they receive such funds is more appropriate.

This is where the threshold comes in, but I think it's non-trivial to figure out what the right threshold is, and it might be game-able.

Perhaps the ! / detective icon in the transaction view could offer the user a choice to manually quarantine funds. Something like "If you did not expect this transaction someone may be spying on you, waiting for you to spend the coins. Would you like to quarantine them?"

You could even quarantine funds by default based on some heuristic as long as the UI makes that very clear and it's easy for a user to unquarantine it (similar to firewall and anti-virus popups). But maybe try the opt-in quarantine approach first.

So then there's dirty with a subset of quarantined. -avoidreuse=1 could e.g. not spend quarantined without an override, give other dirty coins special treatment, but still spend them if needed. That way the setting can be made a default in a future version, in way that the GUI can honor it.

Depending on what ends up happening, the documentation should perhaps point out that the GUI ignores this setting. Alternatively (for the current implementation) dirty coins could be hidden in the GUI, not shown in coin selection and not used when spending all, when this setting is enabled. Both options require a warning, and neither seems ideal, but updating the GUI in a more sophisticated way is pretty time consuming.

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2018

@Sjors That makes sense to me. It seems like adding this default off (as is the current proposal) makes the most sense.

GUI work seems like it will be a good deal of work to get right, but in the meantime, there are real (advanced) users who would benefit from having this feature now, even without the GUI/intuitive component.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Jul 29, 2018

N.B. This doesn't actually fully solve #10065, since transactions received with a dirty address are still shown in the GUI / RPC.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Jul 29, 2018

(as for a term... "reused" perhaps? I don't know that this flag should be exposed as-is, but more like a "will never confirm until spent" status)

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2018

I think "reused" is a bit vague, and doesn't convey the fact it's considered a thing to be avoided if possible, in the way that "dirty" does.

@kallewoof kallewoof force-pushed the kallewoof:feature-avoidreuse branch 2 times, most recently Jul 30, 2018

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2018

Note: I realized the test was invalid, as getbalance would return only clean amount, so I added support to getbalance and fixed the tests.

@kallewoof kallewoof force-pushed the kallewoof:feature-avoidreuse branch 2 times, most recently Jul 30, 2018

@jnewbery

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

It feels to me that this should be a persistent per-wallet setting, rather than a global config option (I think we should be eliminating the global wallet config options wherever possible. See #13044)

@kallewoof kallewoof force-pushed the kallewoof:feature-avoidreuse branch from 0f8b25d to 5ebc6b0 May 29, 2019

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

Removed secondary parameter to SetWalletFlag and restored the UnsetWalletFlag method, and using that instead now.

@DrahtBot DrahtBot removed the Needs rebase label May 29, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

ACK 5ebc6b0

(Reran the rebase from 66f3e97 myself and checked the diff)

@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Concept and code-review ACK 5ebc6b0

Would be nice if this had final sign-off by @meshcollider as wallet maintainer before merge.

Thanks for working on privacy!

//! Forbids inclusion of dirty (previously used) addresses

Hehe. If this use of "dirty addresses" would catch on, maybe people would start avoiding re-using addresses out of a sense of hygiene. One can dream, right.
But agree with @Sjors that "dirty payments" is better to avoid—it's kind of loaded.

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

@laanwj f385578 is not the right commit, FYI!

@jtimon

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Since I don't know the wallet code all that well, it would take a lot of time to be able to properly review this, but concept ACK.

@fanquake fanquake requested a review from meshcollider Jun 18, 2019

@fanquake

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@achow101 Might also want to take a final look?

@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@laanwj f385578 is not the right commit, FYI!

Thanks for noticing, fixed, that was a copy/paste error I was copying a lot of commit hashes for #16200.

@meshcollider

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Code review ACK 5ebc6b0

Only lightly reviewed the test (8f2e208) but the actual code looks good, thank you @kallewoof for being so proactive in addressing review comments and rebasing! About time this finally gets merged. I'll wait a day for Andrew to review but otherwise I'll merge this tomorrow.

assert_raises_rpc_error(-8, "Wallet flag is already set to false", self.nodes[0].setwalletflag, 'avoid_reuse', False)
assert_raises_rpc_error(-8, "Wallet flag is already set to true", self.nodes[1].setwalletflag, 'avoid_reuse', True)

def test_immutable(self):

This comment has been minimized.

Copy link
@meshcollider

meshcollider Jun 18, 2019

Member

In addition to the above TODO, I also suggest this be moved to a different file later (its independent of the avoid-reuse tests)

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 18, 2019

Author Member

Makes sense to me.

ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
ISMINE_ALL_USED = ISMINE_ALL | ISMINE_USED,

This comment has been minimized.

Copy link
@achow101

achow101 Jun 18, 2019

Member

Is it really necessary to add new ismine types? AFAICT, these are only used as ismine filters which I think could just as easily be done with a boolean. IsMine() isn't returning these values, and ISMINE_ALL_USED isn't used anywhere.

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 18, 2019

Author Member

You get most of the combinations with the new "used" flag, because a bunch of calls have a with and without "avoid reuse" state... so I think this is the cleanest approach.

ISMINE_ALL_USED is indeed not used; I added it to indicate that ISMINE_ALL is in fact not include the used ones. It's sort of a comment, with the added side effect that smart IDEs will show it when giving suggestions.

return NullUniValue;
}

if (request.fHelp || request.params.size() < 1 || request.params.size() > 2) {

This comment has been minimized.

Copy link
@achow101

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 18, 2019

Author Member

Will address if rebase becomes necessary, otherwise will try to preserve ACKs and do a quick follow-up post-merge.

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 19, 2019

Author Member

I can do this but it would mean some unnecessary calculations when users are not asking for help:

std::string flags = "";
for (auto& it : WALLET_FLAG_MAP)
if (it.second & MUTABLE_WALLET_FLAGS)
flags += (flags == "" ? "" : ", ") + it.first;

Still worth it?

@@ -2830,6 +2928,9 @@ static UniValue listunspent(const JSONRPCRequest& request)
" \"witnessScript\" : \"script\" (string) witnessScript if the scriptPubKey is P2WSH or P2SH-P2WSH\n"
" \"spendable\" : xxx, (bool) Whether we have the private keys to spend this output\n"
" \"solvable\" : xxx, (bool) Whether we know how to spend this output, ignoring the lack of keys\n"
+ (avoid_reuse ?

This comment has been minimized.

Copy link
@achow101

achow101 Jun 18, 2019

Member

I don't think we should have help output that switches on the active wallet for that command. This breaks help listunspent.

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 18, 2019

Author Member

Why does it break?

This comment has been minimized.

Copy link
@achow101

achow101 Jun 18, 2019

Member

You can do help listunspent without specifying -rpcwallet in order to just get the help for listunspent (and other commands). By making the help dependent on -rpcwallet, this makes the output of help listunspent less helpful because then the help sometimes will include reused and other times not. The help text should be consistent regardless of the other options specified. In general, all options and output should be displayed in the help even if some of those results will be omitted in actual command output. For example, getaddressinfo has a ton of extra fields that may or may not be part of the actual result, but all of them are listed in the help output.

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 18, 2019

Author Member

Ahh, I see what you're saying. Will rework into static output post-merge and/or if rebase becomes necessary.

@achow101

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

ACK 5ebc6b0 modulo above nits

Reviewed the code and did a couple of manual tests.

Still not a fan of adding new isminetypes but I won't block this on that.

@meshcollider

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

The few remaining nits are not worth blocking merge on this, it has waited long enough. A follow-up PR to address them would be good.

@meshcollider meshcollider merged commit 5ebc6b0 into bitcoin:master Jun 18, 2019

2 checks passed

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

meshcollider added a commit that referenced this pull request Jun 18, 2019

Merge #13756: wallet: "avoid_reuse" wallet flag for improved privacy
5ebc6b0 bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets (Karl-Johan Alm)
ada258f doc: release notes for avoid_reuse (Karl-Johan Alm)
2766955 wallet: enable avoid_partial_spends by default if avoid_reuse is set (Karl-Johan Alm)
8f2e208 test: add test for avoidreuse feature (Karl-Johan Alm)
0bdfbd3 wallet/rpc: add 'avoid_reuse' option to RPC commands (Karl-Johan Alm)
f904723 wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS (Karl-Johan Alm)
8247a0d wallet: enable avoid_reuse feature (Karl-Johan Alm)
eec1566 wallet: avoid reuse flags (Karl-Johan Alm)
5892809 wallet: make IsWalletFlagSet() const (Karl-Johan Alm)
129a5ba wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS (Karl-Johan Alm)

Pull request description:

  Add a new wallet flag called `avoid_reuse` which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination.

  This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments.

  This replaces #10386 and together with the (now merged) #12257 it addresses #10065 in full. The concerns raised in #10386 (comment) are also addressed due to #12257.

  ~~Note: this builds on top of #15780.~~ (merged)

ACKs for commit 5ebc6b:
  jnewbery:
    ACK 5ebc6b0
  laanwj:
    Concept and code-review ACK 5ebc6b0
  meshcollider:
    Code review ACK 5ebc6b0
  achow101:
    ACK 5ebc6b0 modulo above nits

Tree-SHA512: fdef45826af544cbbb45634ac367852cc467ec87081d86d08b53ca849e588617e9a0a255b7e7bb28692d15332de58d6c3d274ac003355220e4213d7d9070742e
@Sjors

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

I took a very brief range-diff look at the changes since my last review 69ec084. Not too many changes. Congrats on the merge!

@kallewoof kallewoof deleted the kallewoof:feature-avoidreuse branch Jun 19, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 19, 2019

Merge bitcoin#13756: wallet: "avoid_reuse" wallet flag for improved p…
…rivacy

5ebc6b0 bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets (Karl-Johan Alm)
ada258f doc: release notes for avoid_reuse (Karl-Johan Alm)
2766955 wallet: enable avoid_partial_spends by default if avoid_reuse is set (Karl-Johan Alm)
8f2e208 test: add test for avoidreuse feature (Karl-Johan Alm)
0bdfbd3 wallet/rpc: add 'avoid_reuse' option to RPC commands (Karl-Johan Alm)
f904723 wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS (Karl-Johan Alm)
8247a0d wallet: enable avoid_reuse feature (Karl-Johan Alm)
eec1566 wallet: avoid reuse flags (Karl-Johan Alm)
5892809 wallet: make IsWalletFlagSet() const (Karl-Johan Alm)
129a5ba wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS (Karl-Johan Alm)

Pull request description:

  Add a new wallet flag called `avoid_reuse` which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination.

  This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments.

  This replaces bitcoin#10386 and together with the (now merged) bitcoin#12257 it addresses bitcoin#10065 in full. The concerns raised in bitcoin#10386 (comment) are also addressed due to bitcoin#12257.

  ~~Note: this builds on top of bitcoin#15780.~~ (merged)

ACKs for commit 5ebc6b:
  jnewbery:
    ACK 5ebc6b0
  laanwj:
    Concept and code-review ACK 5ebc6b0
  meshcollider:
    Code review ACK bitcoin@5ebc6b0
  achow101:
    ACK 5ebc6b0 modulo above nits

Tree-SHA512: fdef45826af544cbbb45634ac367852cc467ec87081d86d08b53ca849e588617e9a0a255b7e7bb28692d15332de58d6c3d274ac003355220e4213d7d9070742e
@MarcoFalke
Copy link
Member

left a comment

Looked at the documentation and left some comments

- getbalance
- sendtoaddress

In addition, `sendtoaddress` has been changed to enable `-avoidpartialspends` when

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jun 19, 2019

Member

Can you explain what that means? sendtoaddress is an RPC call, how would it enable a command line flag (-avoidpartialspends) of a running program?

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 19, 2019

Author Member

Changing wording to

In addition, `sendtoaddress` has been changed to always use `-avoidpartialspends`
when `avoid_reuse` is enabled, as it would otherwise risk using up the "wrong" UTXO
for an address reuse case.

Does that look better?

These include:

- createwallet
- getbalance

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jun 19, 2019

Member

What about getbalances?

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 19, 2019

Author Member

In #16239.

a wallet will distinguish between used and unused addresses, and default to not
use the former in coin selection.

(Note: rescanning the blockchain is required, to correctly mark previously

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jun 19, 2019

Member

Could remove (Note: , since all sentences in this doc are release notes

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 19, 2019

Author Member

Removed. I wanted to use it as in "Do note that [word of caution]", but it's no big deal.

@@ -378,6 +389,8 @@ static UniValue sendtoaddress(const JSONRPCRequest& request)
" \"UNSET\"\n"
" \"ECONOMICAL\"\n"
" \"CONSERVATIVE\""},
{"avoid_reuse", RPCArg::Type::BOOL, /* default */ pwallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE) ? "true" : "unavailable", "Avoid spending from dirty addresses; addresses are considered\n"
" dirty if they have previously been used in a transaction."},

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jun 19, 2019

Member

This makes the help text change at run time. I'd prefer to make it static and explain it with something like "true if wallet falg is set, otherwise unavailable"

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 19, 2019

Author Member

Yep, fixed in #16239.

@@ -733,6 +749,7 @@ static UniValue getbalance(const JSONRPCRequest& request)
{"dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Remains for backward compatibility. Must be excluded or set to \"*\"."},
{"minconf", RPCArg::Type::NUM, /* default */ "0", "Only include transactions confirmed at least this many times."},
{"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Also include balance in watch-only addresses (see 'importaddress')"},
{"avoid_reuse", RPCArg::Type::BOOL, /* default */ pwallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE) ? "true" : "unavailable", "Do not include balance in dirty outputs; addresses are considered dirty if they have previously been used in a transaction."},

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jun 19, 2019

Member

Same here

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 19, 2019

Author Member

Good catch. Added to #16239.

meshcollider added a commit that referenced this pull request Jun 22, 2019

Merge #16239: wallet/rpc: follow-up clean-up/fixes to avoid_reuse
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

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 22, 2019

Merge bitcoin#16239: wallet/rpc: follow-up clean-up/fixes to avoid_reuse
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

fanquake added a commit that referenced this pull request Jun 26, 2019

Merge #16286: refactoring: wallet: Fix GCC 7.4.0 warning
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
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.