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

kallewoof commented at 7:34 AM on July 25, 2018: member

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 added the label Wallet on Jul 25, 2018

in test/functional/feature_avoidreuse.py:24 in cd6e85574e outdated

  19 | +        '''
  20 | +        Throw away all owned coins by the node so it gets a balance of 0.
  21 | +        '''
  22 | +        balance = node.getbalance()
  23 | +        if balance > 0.5:
  24 | +            tosend = '%.5f' % (balance - Decimal(0.01))

promag commented at 1:28 PM on July 25, 2018:

Commit "test: Add test for avoidreuse feature"

Use subtractfeefromamount=true below instead?

in src/wallet/init.cpp:58 in 8bb7905a9e outdated

  54 | @@ -55,6 +55,7 @@ void WalletInit::AddWalletOptions() const
  55 |  {
  56 |      gArgs.AddArg("-addresstype", strprintf("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")", FormatOutputType(DEFAULT_ADDRESS_TYPE)), false, OptionsCategory::WALLET);
  57 |      gArgs.AddArg("-avoidpartialspends", strprintf(_("Group outputs by address, selecting all or none, instead of selecting on a per-output basis. Privacy is improved as an address is only used once (unless someone sends to it after spending from it), but may result in slightly higher fees as suboptimal coin selection may result due to the added limitation (default: %u)"), DEFAULT_AVOIDPARTIALSPENDS), false, OptionsCategory::WALLET);
  58 | +    gArgs.AddArg("-avoidreuse", "Mark addresses which have been used to fund transactions in the past, and avoid reusing these in future funding, except when explicitly requested " + strprintf(_("(default: %u)"), DEFAULT_AVOIDREUSE), false, OptionsCategory::WALLET);

promag commented at 1:31 PM on July 25, 2018:

IMO could be something like -avoidaddressreuse.

in src/wallet/coincontrol.h:38 in bb8ea84a93 outdated

  33 | @@ -34,6 +34,8 @@ class CCoinControl
  34 |      boost::optional<bool> m_signal_bip125_rbf;
  35 |      //! Avoid partial use of funds sent to a given address
  36 |      bool m_avoid_partial_spends;
  37 | +    //! Allows inclusion of dirty (previously used) addresses
  38 | +    bool m_allow_dirty_addresses;

promag commented at 1:31 PM on July 25, 2018:

Commit "wallet: add m_allow_dirty_addresses flag to coin control object"

Could initialize here?

kallewoof commented at 8:10 AM on July 26, 2018:

Everything is initialized in SetNull(). In some cases, you can't initialize in the .h file because no gArgs available at compile time.

in src/wallet/wallet.cpp:931 in f042d92b9e outdated

 926 | +{
 927 | +    const CWalletTx* srctx = GetWalletTx(hash);
 928 | +    if (srctx) {
 929 | +        CTxDestination dst;
 930 | +        if (ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst)) {
 931 | +            AssertLockHeld(cs_wallet);

promag commented at 1:32 PM on July 25, 2018:

Commit "wallet: enable avoidreuse feature"

Why is this here? Move up?

Same in CWallet::IsDirty below.

kallewoof commented at 8:10 AM on July 26, 2018:

This may be artifact from previous iteration. Removing.

in src/wallet/wallet.cpp:928 in f042d92b9e outdated

 921 | @@ -922,6 +922,37 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash)
 922 |      return success;
 923 |  }
 924 |  
 925 | +void CWallet::SetDirtyState(const uint256& hash, unsigned int n, bool dirty)
 926 | +{
 927 | +    const CWalletTx* srctx = GetWalletTx(hash);
 928 | +    if (srctx) {

promag commented at 1:33 PM on July 25, 2018:

Commit "wallet: enable avoidreuse feature"

Instead of nesting, could early return here:

if (!srctx) return;

Same in CWallet::IsDirty below.

kallewoof commented at 8:29 AM on July 26, 2018:

Fine for SetDirtyState but IsDirty returns false in two cases; (1) !srctx and (2) !ExtractDestination(..), so fall-through feels cleaner.

in src/wallet/wallet.cpp:1980 in f042d92b9e outdated

1978 | @@ -1940,6 +1979,7 @@ CAmount CWalletTx::GetImmatureCredit(bool fUseCache) const
1979 |  
1980 |  CAmount CWalletTx::GetAvailableCredit(bool fUseCache, const isminefilter& filter) const
1981 |  {
1982 | +    bool allow_dirty_addresses = !gArgs.GetBoolArg("-avoidreuse", DEFAULT_AVOIDREUSE);

promag commented at 1:36 PM on July 25, 2018:

Commit "wallet: enable avoidreuse feature"

Could add a g_allow_dirty_addresses instead?

kallewoof commented at 8:11 AM on July 26, 2018:

Might be worth it, yeah.

promag commented at 1:38 PM on July 25, 2018: member

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?

DrahtBot added the label Needs rebase on Jul 25, 2018

practicalswift commented at 3:58 PM on July 25, 2018: contributor

Concept ACK

Nice work!

gmaxwell commented at 5:53 PM on July 25, 2018: contributor

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 force-pushed on Jul 26, 2018

kallewoof commented at 8:39 AM on July 26, 2018: member

@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 force-pushed on Jul 26, 2018

DrahtBot removed the label Needs rebase on Jul 26, 2018

DrahtBot commented at 9:27 AM on July 26, 2018: member

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.

laanwj added the label Feature on Jul 26, 2018

jonasschnelli assigned Sjors on Jul 26, 2018

Sjors commented at 7:47 PM on July 26, 2018: member

@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 commented at 7:58 PM on July 26, 2018: member

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 commented at 2:15 AM on July 27, 2018: member

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 commented at 9:45 AM on July 27, 2018: member

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 commented at 3:00 AM on July 28, 2018: member

@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 commented at 10:02 PM on July 29, 2018: member

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 commented at 10:05 PM on July 29, 2018: member

(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 commented at 4:20 AM on July 30, 2018: member

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 force-pushed on Jul 30, 2018

kallewoof commented at 6:20 AM on July 30, 2018: member

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 force-pushed on Jul 30, 2018

jnewbery commented at 4:33 PM on July 30, 2018: member

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 commented at 8:23 PM on July 30, 2018: member

I thought that was a PR @jonasschnelli was working on that wasn't merged yet (the no private keys one), but I could be mistaken. Either way, makes sense to make it persistent-per-wallet.

jnewbery commented at 9:13 PM on July 30, 2018: member

a PR @jonasschnelli was working on that wasn't merged yet (the no private keys one)

#9662 - merged!

kallewoof commented at 9:14 PM on July 30, 2018: member

Cool! I was trying to find that one.

kallewoof force-pushed on Aug 1, 2018

kallewoof commented at 4:55 AM on August 2, 2018: member

After @gmaxwell comment here, the new funding approach would work as follows:

Create output groups of all dirty coins
See if any of them fulfil the desired payment amount including fees up to maybe 3x the amount, and return that group if found. MARK CHANGE AS DIRTY
Try coin select using only clean coins

If user used -allowdirty flag, it would simplify to

Try coin select using all coins. (MARK CHANGE AS DIRTY IF 1+ DIRTY COINS WERE USED)

Currently the code does step 3 of the first case and partially step 1 of the -allowdirty case, but does not mark change as dirty. So:

Switch to using persistent per-wallet mode
Add check to see if dirty output groups would satisfy payment and use that if so
Mark change outputs with 1+ dirty coins as their origin as also dirty

kallewoof force-pushed on Aug 3, 2018

kallewoof commented at 6:57 AM on August 3, 2018: member

@jnewbery Code is now using the wallet flag feature. The -avoidreuse flag now determines the default value for new wallets instead. (Also ping @jonasschnelli regarding wallet flag stuff.)

kallewoof force-pushed on Aug 3, 2018

DrahtBot added the label Needs rebase on Aug 25, 2018

in test/functional/feature_avoidreuse.py:76 in 483482b35d outdated

  71 | +
  72 | +    def test_fund_send_fund_send(self):
  73 | +        '''
  74 | +        Test the simple case where [1] generates a new address A, then
  75 | +        [0] sends 10 BTC to A.
  76 | +        [1] spends 5 BTC from A. (leaving roughly 4 BTC useable)

practicalswift commented at 9:10 PM on September 1, 2018:

Typo found by codespell: useable

kallewoof commented at 12:49 AM on September 10, 2018:

Usable can also be spelled useable, with an e in the middle: both are correct.

https://english.stackexchange.com/questions/69709/is-useable-preferred-in-certain-regions-or-just-an-alternate-spelling-of-usa

kallewoof force-pushed on Sep 10, 2018

DrahtBot removed the label Needs rebase on Sep 10, 2018

in src/wallet/init.cpp:58 in 3ab698c072 outdated

  54 | @@ -55,6 +55,7 @@ void WalletInit::AddWalletOptions() const
  55 |  {
  56 |      gArgs.AddArg("-addresstype", strprintf("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")", FormatOutputType(DEFAULT_ADDRESS_TYPE)), false, OptionsCategory::WALLET);
  57 |      gArgs.AddArg("-avoidpartialspends", strprintf(_("Group outputs by address, selecting all or none, instead of selecting on a per-output basis. Privacy is improved as an address is only used once (unless someone sends to it after spending from it), but may result in slightly higher fees as suboptimal coin selection may result due to the added limitation (default: %u)"), DEFAULT_AVOIDPARTIALSPENDS), false, OptionsCategory::WALLET);
  58 | +    gArgs.AddArg("-avoidreuse", "Enable the 'avoid reuse' flag for new wallets created. When enabled, wallets will mark addresses which have been used to fund transactions in the past, and avoid reusing these in future funding, except when explicitly requested " + strprintf(_("(default: %u)"), DEFAULT_AVOIDREUSE), false, OptionsCategory::WALLET);

jnewbery commented at 7:43 PM on September 10, 2018:

My personal preference would be to remove this command line argument, since it's possible to create a wallet with this flag using the createwallet RPC, or update an existing wallet's flag with setwalletflag. I think it's better to remove it from here because:

it's better to have one way to do things
in general, I think we should be aiming to reduce the number of command line arguments, especially in the wallet (see #13044)

kallewoof commented at 7:07 AM on September 11, 2018:

Makes sense. And resolves one TODO (the startup option is ignored for "fresh" nodes in the test framework).

in src/wallet/init.cpp:231 in 3ab698c072 outdated

 227 | @@ -227,8 +228,11 @@ bool WalletInit::Open() const
 228 |          return true;
 229 |      }
 230 |  
 231 | +    uint64_t wallet_creation_flags =

jnewbery commented at 7:44 PM on September 10, 2018:

I'd prefer to remove this, and make the flag settable through the createwallet and setwalletflag options (see how WALLET_FLAG_DISABLE_PRIVATE_KEYS is set)

kallewoof commented at 7:07 AM on September 11, 2018:

Above change resulted in its removal.

in src/wallet/rpcwallet.cpp:295 in 3ab698c072 outdated

 291 | @@ -292,7 +292,7 @@ static UniValue setlabel(const JSONRPCRequest& request)
 292 |  
 293 |  static CTransactionRef SendMoney(CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue)
 294 |  {
 295 | -    CAmount curBalance = pwallet->GetBalance();
 296 | +    CAmount curBalance = pwallet->GetBalance(ISMINE_SPENDABLE, 0, coin_control.m_allow_dirty_addresses);

jnewbery commented at 7:46 PM on September 10, 2018:

I think this change should be in the wallet/rpc: add allow_dirty option to sendtoaddress commit (not the wallet/rpc: add include_dirty flag to getbalance command commit)

kallewoof commented at 7:08 AM on September 11, 2018:

Oops, yep. Thanks.

in src/wallet/rpcwallet.cpp:2557 in 3ab698c072 outdated

2553 | @@ -2540,15 +2554,113 @@ static UniValue loadwallet(const JSONRPCRequest& request)
2554 |      return obj;
2555 |  }
2556 |  
2557 | -static UniValue createwallet(const JSONRPCRequest& request)
2558 | +static std::map<std::string,WalletFlags> walletflagmap{

jnewbery commented at 7:47 PM on September 10, 2018:

Can we define these in wallet.h, so there's only one place to change when adding wallet flags?

in src/wallet/rpcwallet.cpp:2562 in 3ab698c072 outdated

2558 | +static std::map<std::string,WalletFlags> walletflagmap{
2559 | +    {"avoidreuse", WALLET_FLAG_AVOIDREUSE},
2560 | +    {"disable_private_keys", WALLET_FLAG_DISABLE_PRIVATE_KEYS},
2561 | +};
2562 | +
2563 | +static UniValue getwalletflags(const JSONRPCRequest& request)

jnewbery commented at 7:48 PM on September 10, 2018:

I'd prefer not to add this new method. Can we just add a wallet_flags field to the getwalletinfo return object?

kallewoof commented at 7:12 AM on September 11, 2018:

Removing it. There's already a "private_keys_enabled" and "avoid_reuse" entry in the returned value, so I don't think a wallet_flags is even necessary.

in src/wallet/rpcwallet.cpp:2642 in 3ab698c072 outdated

2638 | +
2639 | +    if (!walletflagmap.count(flag)) {
2640 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Unknown wallet flag %s", flag));
2641 | +    }
2642 | +
2643 | +    if (walletflagmap.at(flag) >= (1ULL << 32)) {

jnewbery commented at 7:49 PM on September 10, 2018:

I don't think this is always necessarily going to be true. The meaning of the upper section/lower section flags are which we can safely ignore if we don't know the meaning of (ie if we open a newer wallet on an older client), not whether the flags are mutable or not.

kallewoof commented at 7:11 AM on September 11, 2018:

Ahh. Got it.

in test/functional/test_runner.py:97 in 3ab698c072 outdated

  93 | @@ -94,6 +94,7 @@
  94 |      'rpc_getchaintips.py',
  95 |      'interface_rest.py',
  96 |      'mempool_spend_coinbase.py',
  97 | +    'feature_avoidreuse.py',

jnewbery commented at 7:54 PM on September 10, 2018:

Should be wallet_avoidreuse.py

kallewoof commented at 7:40 AM on September 11, 2018:

Renamed

jnewbery commented at 7:55 PM on September 10, 2018: member

Concept ACK. Some additional high-level feedback inline.

kallewoof force-pushed on Sep 11, 2018

kallewoof commented at 8:12 AM on September 11, 2018: member

@jnewbery Thanks for review! I believe I've addressed all your nits.

in src/wallet/wallet.cpp:1583 in 177c4115b7 outdated

1451 | @@ -1415,15 +1452,15 @@ bool CWallet::IsHDEnabled() const
1452 |      return !hdChain.seed_id.IsNull();
1453 |  }
1454 |  
1455 | -void CWallet::SetWalletFlag(uint64_t flags)
1456 | +void CWallet::SetWalletFlag(uint64_t flag, bool value)

jnewbery commented at 3:00 PM on September 12, 2018:

flags -> flag is a good name change, since SetWalletFlag() suggests this function should only be used to change a single flag.

However, note that there's nothing in this function to enforce that. This doesn't necessarily need to go in your PR, but we could be more defensive by adding something like:

if (^flag || (flag & (flag-1)) {
    # flag has no bits set or more than one bit set
    throw ... 
}

in src/wallet/wallet.h:119 in 177c4115b7 outdated

 115 | -static constexpr uint64_t g_known_wallet_flags = WALLET_FLAG_DISABLE_PRIVATE_KEYS;
 116 | +static constexpr uint64_t g_known_wallet_flags =
 117 | +        WALLET_FLAG_AVOIDREUSE
 118 | +    |   WALLET_FLAG_DISABLE_PRIVATE_KEYS;
 119 | +
 120 | +static const std::map<std::string,WalletFlags> walletflagmap{

jnewbery commented at 3:09 PM on September 12, 2018:

nit: this is a constant, so naming convention says this should be WALLET_FLAG_MAP.

(g_known_wallet_flags above should also be KNOWN_WALLET_FLAG. Perhaps that should be changed as part of this PR?)

in src/wallet/rpcwallet.cpp:344 in 177c4115b7 outdated

 338 | @@ -339,9 +339,10 @@ static UniValue sendtoaddress(const JSONRPCRequest& request)
 339 |          return NullUniValue;
 340 |      }
 341 |  
 342 | -    if (request.fHelp || request.params.size() < 2 || request.params.size() > 8)
 343 | +    if (request.fHelp || request.params.size() < 2 || request.params.size() > 9)
 344 |          throw std::runtime_error(
 345 | -            "sendtoaddress \"address\" amount ( \"comment\" \"comment_to\" subtractfeefromamount replaceable conf_target \"estimate_mode\")\n"
 346 | +            std::string() +
 347 | +            "sendtoaddress \"address\" amount ( \"comment\" \"comment_to\" subtractfeefromamount replaceable conf_target \"estimate_mode\" allow_dirty )\n"

jnewbery commented at 3:20 PM on September 12, 2018:

(same comment goes for all "dirty" text in RPC help/interface)

Are we sure we want to introduce the new terminology "dirty" into the public-facing API? To me, "dirty" doesn't seem intuitive for users. I can imagine lots of users thinking "why are these coins that I have dirty?", "Has someone sent me dirty coins?", etc.

Is it possible to describe these concepts using the word 'reused' or something that conveys the meaning more clearly to users?

kallewoof commented at 4:28 AM on September 13, 2018:

The reason why we've stuck with dirty so far is because of the negative connotation. Note that a coin only becomes dirty if

you received to an address
you spent everything from that address
you received to the same address afterwards

Imagine walking through mud on the way home. As you go inside, you leave a trail behind on the walkway up to and to the inside of your house, even if you prudently take the shoes off at the entrance.

in src/wallet/rpcwallet.cpp:2581 in 177c4115b7 outdated

2570 | +        throw std::runtime_error(
2571 | +            "setwalletflag flag ( state )\n"
2572 | +            "\nChange the state of the given wallet flag for a wallet.\n"
2573 | +            "\nArguments:\n"
2574 | +            "1. flag    (string, required) The name of the flag to change. Current available flags: " + flags + "\n"
2575 | +            "2. value   (boolean, optional, default=true) The new state.\n"

jnewbery commented at 3:22 PM on September 12, 2018:

naming mismatch: you've called it "value" here, but "state" in the RPC table. I slightly prefer "value", but either is fine.

kallewoof commented at 4:29 AM on September 13, 2018:

Oops, thanks! Fixed.

in src/wallet/rpcwallet.cpp:2542 in 177c4115b7 outdated

2572 | +            "\nChange the state of the given wallet flag for a wallet.\n"
2573 | +            "\nArguments:\n"
2574 | +            "1. flag    (string, required) The name of the flag to change. Current available flags: " + flags + "\n"
2575 | +            "2. value   (boolean, optional, default=true) The new state.\n"
2576 | +            "\nResult:\n"
2577 | +            "true|false (boolean) True or false, reflecting whether the flag was modified (true), or (false) whether it was already set to the given state\n"

jnewbery commented at 3:29 PM on September 12, 2018:

I'm not sure whether this is the most useful return object. The user wants to know whether the flag has been set or not, not whether the flag was updated or not (they're interested in the final state, not whether it changed).

Would returning an array of flags set or the result of getwalletinfo be more helpful?

(just a suggestion - I'm not sure what the best approach is here)

kallewoof commented at 4:31 AM on September 13, 2018:

I think being told that your action did nothing vs your action did something is a good UI, personally.

Perhaps it should actually throw instead of returning false, though...

Sjors commented at 8:23 AM on October 25, 2018:

Update: it throws now

in src/wallet/rpcwallet.cpp:2590 in 177c4115b7 outdated

2586 | +
2587 | +    if (!walletflagmap.count(flag)) {
2588 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Unknown wallet flag %s", flag));
2589 | +    }
2590 | +
2591 | +    if (pwallet->IsWalletFlagSet(walletflagmap.at(flag))) return false;

jnewbery commented at 3:32 PM on September 12, 2018:

This early exit prevents the user from being able to unset a flag. If the flag is already set, we'll exit before trying to unset it.

kallewoof commented at 4:32 AM on September 13, 2018:

D'oh, thanks. Fixed.

in src/wallet/rpcwallet.cpp:2592 in 177c4115b7 outdated

2588 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Unknown wallet flag %s", flag));
2589 | +    }
2590 | +
2591 | +    if (pwallet->IsWalletFlagSet(walletflagmap.at(flag))) return false;
2592 | +
2593 | +    pwallet->SetWalletFlag(walletflagmap.at(flag), value);

jnewbery commented at 3:34 PM on September 12, 2018:

It should not be possible to update the disable_private_keys flag after the wallet has been created (because by that time, there are already private keys in the wallet)

jnewbery commented at 4:00 PM on September 12, 2018:

I don't think this is sufficient for enabling 'avoid_reuse'. Consider:

wallet is created without the avoid_reuse flag
wallet sends from address
wallet receives new coins to used address. These are NOT marked as dirty because the avoid_reuse flag is not set
avoid_reuse is enabled with the setwalletflag RPC
getbalance, sendtoaddress, etc will all use the reused address coins because they're not set as dirty.

kallewoof commented at 4:37 AM on September 13, 2018:

Should it be possible to disable it?

kallewoof commented at 5:05 AM on September 13, 2018:

Right ... the idea is that the user has to actually call 'rescan' for it to become retroactive. This was explained when this was a command line option, I think, but the explanation vanished with the change to remove the option. Not sure where would be a good place to explain this. Perhaps there should be a 'caveats' map for flags which are returned when setting flags?

in src/wallet/wallet.h:368 in 177c4115b7 outdated

 364 | @@ -354,6 +365,8 @@ class CWalletTx : public CMerkleTx
 365 |      mutable bool fAvailableWatchCreditCached;
 366 |      mutable bool fChangeCached;
 367 |      mutable bool fInMempool;
 368 | +    mutable bool available_dirty_credit_cached;

jnewbery commented at 3:36 PM on September 12, 2018:

naming convention: mamber variable names should be prefixed m_

in src/wallet/rpcwallet.cpp:344 in 177c4115b7 outdated

 338 | @@ -339,9 +339,10 @@ static UniValue sendtoaddress(const JSONRPCRequest& request)
 339 |          return NullUniValue;
 340 |      }
 341 |  
 342 | -    if (request.fHelp || request.params.size() < 2 || request.params.size() > 8)
 343 | +    if (request.fHelp || request.params.size() < 2 || request.params.size() > 9)
 344 |          throw std::runtime_error(
 345 | -            "sendtoaddress \"address\" amount ( \"comment\" \"comment_to\" subtractfeefromamount replaceable conf_target \"estimate_mode\")\n"
 346 | +            std::string() +

jnewbery commented at 3:55 PM on September 12, 2018:

I don't understand why this std::string() + is required

in test/functional/wallet_avoidreuse.py:98 in 177c4115b7 outdated

  56 | +        # Flags should be node1.avoid_reuse=false, node2.avoid_reuse=true
  57 | +        assert_equal(self.nodes[0].getwalletinfo()["avoid_reuse"], False)
  58 | +        assert_equal(self.nodes[1].getwalletinfo()["avoid_reuse"], True)
  59 | +
  60 | +        # Stop and restart node 1
  61 | +        self.stop_node(1)

jnewbery commented at 3:57 PM on September 12, 2018:

Do you need to stop-start the node here? Can you just unloadwallet then reloadwallet?

kallewoof commented at 5:03 AM on September 13, 2018:

I kind of want the node to go down completely so we don't get some weird case where a flag persists when reloading wallet but not when restarting node. Even if that sounds like an impossibility.

in test/functional/wallet_avoidreuse.py:5 in 177c4115b7 outdated

   0 | @@ -0,0 +1,126 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2018 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +"""Test the avoid_reuse feature."""

jnewbery commented at 4:01 PM on September 12, 2018:

It'd be nice if this tested the setwalletflag RPC too.

kallewoof commented at 5:06 AM on September 13, 2018:

It sort of does, actually. I assumed there were tests elsewhere, but apparently not. I will add complementary tests for setwalletflag.

jnewbery commented at 4:02 PM on September 12, 2018: member

Lots of comments.

I haven't thought too much about the logic for making coins dirty/checking dirtiness or the test coverage.

kallewoof force-pushed on Sep 13, 2018

kallewoof commented at 5:19 AM on September 13, 2018: member

@jnewbery Thanks a lot for all the feedback. I have addressed most of your nits (a few remain up in the air).

kallewoof force-pushed on Sep 13, 2018

kallewoof force-pushed on Sep 14, 2018

in test/functional/wallet_avoidreuse.py:27 in 62b5893d98 outdated

  22 | +
  23 | +    def set_test_params(self):
  24 | +        self.setup_clean_chain = False
  25 | +        self.num_nodes = 2
  26 | +
  27 | +    def reset_balance(self, node, discardaddr):

ken2812221 commented at 6:00 AM on September 14, 2018:

Should skip this if wallet disabled.

    def skip_test_if_missing_module(self):
        self.skip_if_no_wallet()

kallewoof force-pushed on Sep 14, 2018

DrahtBot added the label Needs rebase on Sep 18, 2018

kallewoof force-pushed on Sep 19, 2018

DrahtBot removed the label Needs rebase on Sep 19, 2018

DrahtBot added the label Needs rebase on Oct 24, 2018

kallewoof force-pushed on Oct 25, 2018

DrahtBot removed the label Needs rebase on Oct 25, 2018

in test/functional/wallet_avoidreuse.py:48 in 2ba2afcc4e outdated

  43 | +        self.test_persistence()
  44 | +        self.test_immutable()
  45 | +
  46 | +        self.nodes[0].generate(110)
  47 | +        self.sync_all()
  48 | +        self.reset_balance(self.nodes[1], self.nodes[0].getnewaddress())

Sjors commented at 8:30 AM on October 25, 2018:

If you use getreceivedbyaddress instead of getbalance you don't need this reset_balance and assert_approx stuff.

kallewoof commented at 7:29 AM on October 26, 2018:

That's a good point, but getreceivedbyaddress has not been updated to handle the avoid_reuse flag yet. Perhaps it's worth doing that and switch, but it's also reassuring to ensure no weird balance appeared from some unexpected address.

Sjors commented at 9:10 AM on October 26, 2018:

Multiple wallets per node might be another route.

in test/functional/wallet_avoidreuse.py:92 in 2ba2afcc4e outdated

  87 | +        assert_raises_rpc_error(-8, "Wallet flag is immutable", w.setwalletflag, 'disable_private_keys', False)
  88 | +
  89 | +        # Unload temp wallet
  90 | +        self.nodes[1].unloadwallet(tempwallet)
  91 | +
  92 | +    def test_fund_send_fund_send(self):

Sjors commented at 8:34 AM on October 25, 2018:

Move this test up, since it's more interesting.

kallewoof commented at 7:30 AM on October 26, 2018:

The way I see it is, the two tests above it will catch weird issues that could cause the test to fail in random places, so those tests are run first. The intention is to order tests chronologically.

in test/functional/wallet_avoidreuse.py:125 in 2ba2afcc4e outdated

 120 | +
 121 | +        assert_raises_rpc_error(-6, "Insufficient funds", self.nodes[1].sendtoaddress, retaddr, 10)
 122 | +
 123 | +        self.nodes[1].sendtoaddress(retaddr, 4)
 124 | +
 125 | +    def test_fund_send_fund_senddirty(self):

Sjors commented at 8:37 AM on October 25, 2018:

This should probably be the first test, since it describes the behavior without the new flag.

kallewoof commented at 7:32 AM on October 26, 2018:

OK!

Sjors commented at 9:29 AM on October 25, 2018: member

Concept ACK, code is getting close.

Can you update the PR name and description to describe the latest behavior? My understanding:

in order to avoid reuse for a fresh wallet, use createwallet false true
for an existing wallet use setwalletflag avoid_reuse true
- setwalletflag can't be used on disable_private_keys because it's immutable
sendtoaddress can override the flag using allow_dirty

Issues:

the flag name disable_private_keys in WALLET_FLAG_MAP is inconsistent with the output of getwalletinfo (private_keys_enabled)
the use of both allow_dirty and avoid_reuse is confusing. I find avoid_reuse more clear.
why does sendtoaddress have an allow_dirty option, but e.g. sendmany doesn't?
why does getbalance have include_dirty, but similar calls don't? (in particular getunconfirmedbalance and getreceivedbylabel)
setwalletflag avoid_reuse false should mark used destinations as dirty or warn that it doesn't

Suggestions:

don't tease the user by listing disable_private_keys in the help, or add a separate "Flags which can't be changed: "
as pointed out earlier, listtransactions and friends should include the dirty flag and allow excluding those, for UI purposes, but this can wait

kallewoof renamed this:
~~wallet: -avoidreuse feature for improved privacy~~
wallet: "avoid_reuse" wallet flag for improved privacy
on Oct 26, 2018

in test/functional/wallet_avoidreuse.py:27 in 2ba2afcc4e outdated

  22 | +
  23 | +    def set_test_params(self):
  24 | +        self.setup_clean_chain = False
  25 | +        self.num_nodes = 2
  26 | +
  27 | +    def reset_balance(self, node, discardaddr):

practicalswift commented at 7:12 AM on October 26, 2018:

Nit: This method doesn't use self and could be a function instead?

kallewoof commented at 7:32 AM on October 26, 2018:

Good point. Turned into function.

kallewoof commented at 7:23 AM on October 26, 2018: member

@Sjors Thanks a lot for the detailed review.

the flag name disable_private_keys in WALLET_FLAG_MAP is inconsistent with the output of getwalletinfo (private_keys_enabled)

Yeah. This is remnant from a different PR which uses both variants. I think it should be resolved, but probably in its own PR.

the use of both allow_dirty and avoid_reuse is confusing. I find avoid_reuse more clear.

Makes sense. Switched to using that.

why does sendtoaddress have an allow_dirty option, but e.g. sendmany doesn't? why does getbalance have include_dirty, but similar calls don't? (in particular getunconfirmedbalance and getreceivedbylabel)

I mentioned this in the original PR, but I am basically trying to keep the PR as small as I can, so I am leaving out some RPC tweaks and such. I intend to do a follow-up PR to address these things.

setwalletflag avoid_reuse false should mark used destinations as dirty or warn that it doesn't

You mean true I think? false would disable the feature. It may be worth it to add a "warn user if turning <ON|OFF> flag <X>" mapping to the wallet flags, as I expect other wallet flags may also want to give hints when toggled.

don't tease the user by listing disable_private_keys in the help, or add a separate "Flags which can't be changed: "

I guess, yeah. Doesn't feel like a super big deal, and it could be useful to see a list even if you can't change them, but maybe it'll just confuse people. No longer displaying immutable flags in list.

kallewoof commented at 7:25 AM on October 26, 2018: member

@Sjors

I can't comment on the "Update: it throws now" comment for some reason. It does throw, but it also returns true/false for successful calls, where false means "flag unchanged" and true means "flag changed".

kallewoof force-pushed on Oct 26, 2018

kallewoof force-pushed on Oct 29, 2018

DrahtBot added the label Needs rebase on Nov 5, 2018

kallewoof force-pushed on Nov 6, 2018

DrahtBot removed the label Needs rebase on Nov 6, 2018

DrahtBot added the label Needs rebase on Nov 9, 2018

luke-jr commented at 5:29 PM on November 9, 2018: member

Dirty should be set when receiving, not when the received coins are then spent.

kallewoof force-pushed on Nov 12, 2018

DrahtBot removed the label Needs rebase on Nov 12, 2018

kallewoof force-pushed on Nov 12, 2018

kallewoof commented at 5:48 AM on November 12, 2018: member

@luke-jr

Dirty should be set when receiving, not when the received coins are then spent.

This is done in AddToWallet, so it looks like it is doing what you are asking for:

https://github.com/bitcoin/bitcoin/blob/dbfb209900cb6d4e6af3c0eb6ffa96f20f1dd123/src/wallet/wallet.cpp#L880-L886

The test test_fund_send_fund_send ensures that this behavior is the case, I think. Is it missing something?

kallewoof force-pushed on Nov 12, 2018

DrahtBot added the label Needs rebase on Nov 13, 2018

kallewoof force-pushed on Nov 14, 2018

DrahtBot removed the label Needs rebase on Nov 14, 2018

in src/wallet/wallet.h:130 in 602bd48de3 outdated

 124 | @@ -125,11 +125,22 @@ enum WalletFlags : uint64_t {
 125 |      // wallet flags in the upper section (> 1 << 31) will lead to not opening the wallet if flag is unknown
 126 |      // unknown wallet flags in the lower section <= (1 << 31) will be tolerated
 127 |  
 128 | +    // will categorize coins as clean (not reused) and dirty (reused), and handle
 129 | +    // them with privacy considerations in mind
 130 | +    WALLET_FLAG_AVOIDREUSE = (1ULL << 0),

meshcollider commented at 9:23 AM on November 18, 2018:

nit: WALLET_FLAG_AVOID_REUSE instead of WALLET_FLAG_AVOIDREUSE

Sjors commented at 2:10 PM on November 27, 2018:

Agree with the nit, and also that it's a nit :-)

in src/wallet/rpcwallet.cpp:740 in e633e181f1 outdated

 734 | @@ -734,7 +735,11 @@ static UniValue getbalance(const JSONRPCRequest& request)
 735 |          filter = filter | ISMINE_WATCH_ONLY;
 736 |      }
 737 |  
 738 | -    return ValueFromAmount(pwallet->GetBalance(filter, min_depth));
 739 | +    bool avoid_reuse = request.params[3].isNull()
 740 | +        ? pwallet->IsWalletFlagSet(WALLET_FLAG_AVOIDREUSE)
 741 | +        : request.params[3].get_bool();

meshcollider commented at 9:34 AM on November 18, 2018:

Above help text says (only applicable if the \"avoid reuse\" wallet flag is enabled), so why do you have this case? Shouldn't we just error immediately if it is set without pwallet->IsWalletFlagSet(WALLET_FLAG_AVOIDREUSE)? Same with the other RPCs

kallewoof commented at 4:11 AM on November 19, 2018:

Code now throws an error if user does avoidreuse=true for a wallet which does not have WALLET_FLAG_AVOID_REUSE enabled.

in src/wallet/rpcwallet.cpp:571 in afc146d69a outdated

 568 | +            "getreceivedbyaddress \"address\" ( minconf avoid_reuse )\n"
 569 |              "\nReturns the total amount received by the given address in transactions with at least minconf confirmations.\n"
 570 |              "\nArguments:\n"
 571 |              "1. \"address\"         (string, required) The bitcoin address for transactions.\n"
 572 |              "2. minconf             (numeric, optional, default=1) Only include transactions confirmed at least this many times.\n"
 573 | +            "3. avoid_reuse         (boolean, optional) Forbid inclusion of addresses which have been marked as 'dirty'.\n"

meshcollider commented at 9:46 AM on November 18, 2018:

Help texts for all the RPC changes in this PR could do with some more consistency, e.g. in how the default is output (this one doesn't output the default at all, whereas sendtoaddress will decide whether to output default=true or unavailable)

meshcollider commented at 9:59 AM on November 18, 2018: contributor

Concept ACK / light code review https://github.com/bitcoin/bitcoin/pull/13756/commits/a2d136dc46593f6e15c038f2fb497596617ee73e

Haven't looked at the test yet. A couple of comments inline.

The split into multiple commits is nice but perhaps a little too much, some squashing would be good e.g. "wallet: add MUTABLE_WALLET_FLAGS" -> "wallet/rpc: add setwalletflag RPC command for modifying wallet flag post creation", etc.

kallewoof commented at 4:16 AM on November 19, 2018: member

@MeshCollider Thanks much for the review!

The split into multiple commits is nice but perhaps a little too much, some squashing would be good e.g. "wallet: add MUTABLE_WALLET_FLAGS" -> "wallet/rpc: add setwalletflag RPC command for modifying wallet flag post creation", etc.

I will try to squash stuff. Agree about the two commits you mentioned.

kallewoof force-pushed on Nov 19, 2018

Sjors commented at 3:17 PM on November 19, 2018: member

I wrote earlier:

setwalletflag avoid_reuse false should mark used destinations as dirty or warn that it doesn't

I meant to say setwalletflag avoid_reuse true should go through existing transactions in the wallet and mark things dirty as needed. Or the help text and/or release notes should warn that it doesn't.

getbalance "*" 0 false false ignores unconfirmed transactions (pre-existing bug?)

getreceivedbyaddress tb1q... 0 true discards the first transaction on the dirty address. That seems non-intuitive.

A followup PR could add a "Avoid Reuse" checkbox to the coin selection dialog in the GUI to reveal dirty coins. Another PR could add a Avoid Reuse checkbox to the Options->Wallet dialog, which would perform setwalletflag avoid_reuse true (it would not use QT persisted settings).

meshcollider commented at 7:47 PM on November 19, 2018: contributor

@Sjors getbalance is preexisting yep, see #14602

kallewoof force-pushed on Nov 20, 2018

kallewoof commented at 4:29 AM on November 20, 2018: member

@Sjors Thanks for testing and for valuable feedback.

setwalletflag avoid_reuse false should mark used destinations as dirty or warn that it doesn't

I meant to say setwalletflag avoid_reuse true should go through existing transactions in the wallet and mark things dirty as needed. Or the help text and/or release notes should warn that it doesn't.

I somehow forgot about this, apologies -- I've added "caveats" to the setwalletflags, and also added release notes indicating a rescan is required.

getreceivedbyaddress tb1q... 0 true discards the first transaction on the dirty address. That seems non-intuitive.

Thinking about this, I actually don't really see why getreceivedbyaddress should even care about reuse. It should just show everything that was received. Dropping that commit.

kallewoof force-pushed on Nov 20, 2018

DrahtBot added the label Needs rebase on Nov 23, 2018

kallewoof force-pushed on Nov 26, 2018

DrahtBot removed the label Needs rebase on Nov 26, 2018

Sjors approved

Sjors commented at 2:16 PM on November 27, 2018: member

tACK 3bff2a5 on macOS 10.14.1

I would be good to get someone with deep wallet knowledge like @sipa to specifically check d4e108f6bbd99264a739455f9e46f7068acb01d0, which is where the used tracking happens.

kallewoof commented at 1:43 AM on November 28, 2018: member

@Sjors #13756 (review) I can't see what nit this is referring to -- is it the "use AVOID_REUSE, not AVOIDREUSE" one? Then cool, as I believe I addressed that.

Sjors commented at 2:25 PM on November 28, 2018: member

@kallewoof correct, looks like I responded to an outdated comment somehow.

kallewoof force-pushed on Nov 30, 2018

kallewoof force-pushed on Dec 3, 2018

DrahtBot added the label Needs rebase on Dec 5, 2018

kallewoof force-pushed on Dec 10, 2018

DrahtBot removed the label Needs rebase on Dec 10, 2018

kallewoof force-pushed on Dec 10, 2018

DrahtBot added the label Needs rebase on Jan 29, 2019

kallewoof force-pushed on Jan 30, 2019

DrahtBot removed the label Needs rebase on Jan 30, 2019

Sjors commented at 11:54 AM on January 30, 2019: member

utACK 23a15c5

Changes since my last review: rebase, some commits reorganised, new RPC help syntax, cosmetic changes:

PREV=3bff2a5 N=14 && git range-diff `git merge-base --all HEAD $PREV`...$PREV HEAD~$N...HEAD

sharidz313 approved

DrahtBot added the label Needs rebase on Feb 10, 2019

charlesrutabanzibwa unassigned Sjors on Feb 11, 2019

kallewoof force-pushed on Feb 13, 2019

DrahtBot removed the label Needs rebase on Feb 13, 2019

in src/wallet/wallet.cpp:4223 in ac89bbac87 outdated

4224 | -        } else if (wallet_creation_flags & WALLET_FLAG_BLANK_WALLET) {
4225 | -            walletInstance->SetWalletFlag(WALLET_FLAG_BLANK_WALLET);
4226 | -        } else {
4227 | +        if ((wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) &&
4228 | +            (wallet_creation_flags & WALLET_FLAG_BLANK_WALLET)) {
4229 | +            InitError(_("Wallet cannot have both DISABLE_PRIVATE_KEYS and BLANK_WALLET flags"));

Sjors commented at 8:47 AM on February 13, 2019:

Nit: this new check (if you keep it, see ff7fe3cf817f787ee8d929b73522d2e9c9da5681), as well as setting all flags in one go (below), deserves a separate commit. I do like the cleanup.

kallewoof commented at 9:10 AM on February 13, 2019:

I assumed setting both was an error, but realized tests rely on being able to set both. I think I screwed up the logic though (the original code did if/else, which would actually not set one of the flags if the other one was set). Will address now.

Sjors commented at 9:20 AM on February 13, 2019:

Actually setting both is fine, so this check needs to be dropped.

a blank wallet with private keys enabled can import or generate new private keys
a blank wallet with private keys disabled can only import public keys

The other way around:

a wallet without private keys is blank upon creation, but that might change, so I don't think we should enforce that

kallewoof commented at 5:54 AM on February 14, 2019:

Yes, setting both is fine, but the code pre-this-PR will actually ignore the blank wallet flag if the disable private keys flag is set:

https://github.com/bitcoin/bitcoin/blob/9c93f5d9fc93df2120998e8383bc972b738f3ff5/src/wallet/wallet.cpp#L4142-L4151

(As such, the updated code will explicitly disable the blank wallet flag if both are set, before setting all flags.)

Sjors approved

Sjors commented at 8:56 AM on February 13, 2019: member

~re-utACK 79bf9f6~, modulo squashing the new fixup commits:

rebased on blank wallet #15226
slight cleanup of CreateWalletFromFile
modernised RPC help syntax

kallewoof force-pushed on Feb 14, 2019

kallewoof commented at 6:00 AM on February 14, 2019: member

Squashed. @Sjors I believe the code now acts the same way as master, i.e.

blank_wallet → blank_wallet
disable_privkeys → disable_privkeys
blank_wallet | disable_privkeys → disable_privkeys

kallewoof force-pushed on Feb 14, 2019

Sjors commented at 10:05 AM on February 14, 2019: member

@kallewoof you're right, it's the same as on master. utACK ec26b2e

kallewoof commented at 11:25 AM on February 14, 2019: member

@Sjors The big question is, does it even matter if both flags are set? I would love to not have those 5 lines of cruft in there, if they don't make a difference. I should hunt down whoever wrote that code and ask, I guess. (And/or verify myself.)

Sjors commented at 3:53 PM on February 14, 2019: member

I'm not sure, but perhaps it's better dealt with in a followup to this as well as #14021. cc @achow101

It seems to me that when you create a wallet without private keys, as long as there's no additional argument to feed the new wallet with public keys, it's by definition blank. Problem is that the RPC needs a default and we probably don't want to make that too complicated. A sane default for blank is true when private keys are disabled, whereas otherwise the sane default for blank is false.

DrahtBot added the label Needs rebase on Feb 15, 2019

kallewoof force-pushed on Feb 15, 2019

DrahtBot removed the label Needs rebase on Feb 15, 2019

DrahtBot added the label Needs rebase on Mar 4, 2019

kallewoof force-pushed on Mar 5, 2019

DrahtBot removed the label Needs rebase on Mar 5, 2019

Sjors commented at 5:42 PM on March 5, 2019: member

utACK rebased 74581ff

DrahtBot added the label Needs rebase on Apr 2, 2019

kallewoof force-pushed on Apr 3, 2019

DrahtBot removed the label Needs rebase on Apr 3, 2019

DrahtBot added the label Needs rebase on Apr 4, 2019

kallewoof force-pushed on Apr 4, 2019

DrahtBot removed the label Needs rebase on Apr 4, 2019

DrahtBot added the label Needs rebase on Apr 9, 2019

kallewoof force-pushed on Apr 10, 2019

kallewoof renamed this:
~~wallet: "avoid_reuse" wallet flag for improved privacy~~
[WIP] wallet: "avoid_reuse" wallet flag for improved privacy
on Apr 10, 2019

DrahtBot removed the label Needs rebase on Apr 10, 2019

kallewoof force-pushed on Apr 10, 2019

kallewoof renamed this:
~~[WIP] wallet: "avoid_reuse" wallet flag for improved privacy~~
wallet: "avoid_reuse" wallet flag for improved privacy
on Apr 10, 2019

kallewoof force-pushed on Apr 10, 2019

kallewoof commented at 4:22 AM on April 10, 2019: member

~~Rebased, and refactored code based on #15780 cachable accounts (note: first commit here is from that PR).~~ (merged)

There is now a isminetype called "ISMINE_USED" which is set for the 'allow reuse' caches.

kallewoof force-pushed on Apr 10, 2019

kallewoof force-pushed on Apr 11, 2019

laanwj referenced this in commit 2d5419feed on Apr 23, 2019

DrahtBot added the label Needs rebase on Apr 23, 2019

kallewoof force-pushed on Apr 23, 2019

DrahtBot removed the label Needs rebase on Apr 23, 2019

in src/wallet/wallet.h:823 in 823205aab5 outdated

 819 | @@ -800,6 +820,9 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
 820 |          std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool& bnb_used) const;
 821 |  
 822 |      bool IsSpent(interfaces::Chain::Lock& locked_chain, const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 823 | +    bool IsUsed(const uint256& hash, unsigned int n) const;

Sjors commented at 11:35 AM on April 24, 2019:

nit: rename to IsUsedDestination

Needs comment, e.g.:

// Whether this or any UTXO with the same CTxDestination has been spent.

in src/wallet/wallet.h:824 in 823205aab5 outdated

 819 | @@ -800,6 +820,9 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
 820 |          std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool& bnb_used) const;
 821 |  
 822 |      bool IsSpent(interfaces::Chain::Lock& locked_chain, const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 823 | +    bool IsUsed(const uint256& hash, unsigned int n) const;
 824 | +    void SetUsedState(const uint256& hash, unsigned int n, bool used);

Sjors commented at 11:37 AM on April 24, 2019:

nit: rename to SetUsedStateDestination, to make it more clear that it's the CTxDestination we're interested in

in src/wallet/wallet.cpp:987 in 823205aab5 outdated

 976 | @@ -938,6 +977,14 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
 977 |  
 978 |      uint256 hash = wtxIn.GetHash();
 979 |  
 980 | +    if (IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE)) {

Sjors commented at 11:42 AM on April 24, 2019:

Shouldn't we mark destinations as used regardless of the flag? Or does this flag imply both a default preference and the entire functionality?

kallewoof commented at 12:21 AM on April 25, 2019:

We do not mark destinations as used unless the user turns on avoid_reuse. This is the reason why people must rescan the chain if they turn it on.

in src/wallet/rpcwallet.cpp:2687 in 823205aab5 outdated

2683 | @@ -2596,6 +2684,10 @@ static UniValue createwallet(const JSONRPCRequest& request)
2684 |          flags |= WALLET_FLAG_BLANK_WALLET;
2685 |      }
2686 |  
2687 | +    if (!request.params[2].isNull() && request.params[2].get_bool()) {

Sjors commented at 11:49 AM on April 24, 2019:

Yikes, not sure how I missed this in earlier reviews: needs to be params[3]

kallewoof commented at 12:22 AM on April 25, 2019:

Wow. I need to write better tests, it looks like.

Edit: the tests look like they should have caught this issue. Weirdness.

Sjors commented at 7:48 AM on April 25, 2019:

It was probably a rebase glitch. The tests often use named params.

Sjors changes_requested

Sjors commented at 12:28 PM on April 24, 2019: member

tACK 823205a modulo params bug (inline). Compared to my previous tACK 74581ff:

some commits were combined
ISMINE_USED added (the cache logic in GetAvailableCredit could use extra eyes)

For a followup: this flag works best when combined with -avoidpartialspends, as suggested by @kallewoof earlier. There's no harm as long as they are combined in a single transaction. If the user forgets to set -avoidpartialspends they'll miss the opportunity to spend duplicates in a safe way. We could interpret -avoidpartialspends (#12257) as true for wallets with avoid_reuse flag.

kallewoof force-pushed on Apr 25, 2019

kallewoof commented at 4:46 AM on April 25, 2019: member

@Sjors Very good point about -avoidpartialspends. I can't believe I forgot about that. I think as you say this flag should be enabled by default if avoid_reuse is set in the wallet.

Edit: enabled, see c831a40.

Sjors commented at 7:50 AM on April 25, 2019: member

Don't forget to update the bitcoind docs for this new param behavior.

kallewoof force-pushed on Apr 26, 2019

kallewoof commented at 5:22 AM on April 26, 2019: member

@Sjors Right, thanks. Done. I think a follow-up PR should convert -avoidpartialspends into a wallet flag that is auto-enabled when avoid_reuse flag is set/enabled.

DrahtBot added the label Needs rebase on May 6, 2019

kallewoof force-pushed on May 7, 2019

DrahtBot removed the label Needs rebase on May 7, 2019

Sjors commented at 1:18 PM on May 12, 2019: member

utACK 69ec084, compared to my previous utACK:

renamed SetUsedState to SetUsedDestinationState, and IsUsed to IsUsedDestination
param index fixed (@MarcoFalke it would be nice if we didn't need brittle stuff like request.params[3] in the RPC)
improved documentation

DrahtBot added the label Needs rebase on May 16, 2019

in src/wallet/wallet.cpp:1588 in 69ec084ace outdated

1585 |  {
1586 |      LOCK(cs_wallet);
1587 | -    m_wallet_flags |= flags;
1588 | +    if (!flag || (flag & (flag-1))) {
1589 | +        // flag has no bits set or more than one bit set
1590 | +        throw std::runtime_error(std::string(__func__) + ": flag format invalid (not a single bit set)");

jnewbery commented at 8:00 PM on May 16, 2019:

This should really be a std::logic_error(). The arguments passed to SetWalletFlag() are known by the programmer.

kallewoof commented at 2:39 AM on May 20, 2019:

Makes sense, changed.

in src/wallet/wallet.cpp:2239 in 69ec084ace outdated

2235 | @@ -2184,6 +2236,9 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector<
2236 |  
2237 |      vCoins.clear();
2238 |      CAmount nTotal = 0;
2239 | +    // Either AVOIDREUSE flag is not set (in which case we always allow), or we default to avoiding, and only in the case where

jnewbery commented at 8:49 PM on May 16, 2019:

nit: s/AVOIDREUSE/WALLET_FLAG_AVOID_REUSE so the comment matches the code

in src/wallet/wallet.cpp:4111 in 69ec084ace outdated

4142 | -            //selective allow to set flags
4143 | -            walletInstance->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
4144 | -        } else if (wallet_creation_flags & WALLET_FLAG_BLANK_WALLET) {
4145 | -            walletInstance->SetWalletFlag(WALLET_FLAG_BLANK_WALLET);
4146 | -        } else {
4147 | +        if ((wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) &&

jnewbery commented at 9:00 PM on May 16, 2019:

I don't understand how this change is related to the other changes in 56f7c53925c5f5069b325515e1f61251f8c16971 wallet: enable avoid_reuse feature or to any of the other changes in this PR.

kallewoof commented at 2:45 AM on May 20, 2019:

@jnewbery Existing code would set at most one of the two flags (disable privkeys or blank_wallet), and would not set any other wallet flags. As I am introducing more wallet flags, I needed those to be set, but because the existing code will ignore the blank_wallet flag if the disable privkeys flag is set, I had to do some tweakery to not modify the existing behavior.

If everyone agrees that suddenly setting both disable_privkeys AND blank_wallet is harmless, this code can be simplified down to just doing ->SetWalletFlags(...) without the if block, but I didn't feel confident making that decision myself.

jnewbery commented at 9:03 PM on May 20, 2019:

Ah. I see now. Yes, this makes sense, although it's a little confusing the way that the generic setWalletFlags() is called between two blocks of blank wallet specific code.

I've done some grepping and it seems to me that it's fine for WALLET_FLAG_BLANK_WALLET and WALLET_FLAG_DISABLE_PRIVATE_KEYS to be set on the same wallet. @achow101 should know for sure.

achow101 commented at 3:40 PM on May 24, 2019:

It's fine to set both WALLET_FLAG_BLANK_WALLET and WALLET_FLAG_DISABLE_PRIVATE_KEYS on a wallet, though redundant. WALLET_FLAG_BLANK_WALLET will be unset whenever something is imported.

ryanofsky commented at 4:08 PM on May 24, 2019:

I wish the BLANK_WALLET flag was called something more like WALLET_IS_ALREADY_INITIALIZED or WALLET_DO_NOT_REINITIALIZE since the only effect of the flag is to skip the fFirstRun code path when the wallet is loaded. I think the term "blank wallet" is good as a createwallet option name, but less good as a flag name.

kallewoof commented at 1:01 AM on May 25, 2019:

Removed the redundant if block.

in src/wallet/wallet.cpp:3121 in 69ec084ace outdated

3115 | @@ -3043,6 +3116,14 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
3116 |                  coin.BindWallet(this);
3117 |                  NotifyTransactionChanged(this, coin.GetHash(), CT_UPDATED);
3118 |              }
3119 | +
3120 | +            if (used) {
3121 | +                // resulting outputs are all used (if an output is not ours, it

jnewbery commented at 9:01 PM on May 16, 2019:

Can you explain a bit more why you're setting the outputs to dirty if one of the inputs is dirty?

kallewoof commented at 2:53 AM on May 20, 2019:

This is just a convenient place to mark-up your own UTXO:s, e.g. change address and the like. This part can be removed and the AddToWallet* methods will handle it a little later. I'm actually going to remove it, as it's confused myself more than once in the past, and now, you as well.

Edit: I don't think the above response is correct. I have removed the code in question, but will dig more to see why it was there. It may have been a mistake that caused change addresses that were not reused to be marked as reused, but that should have been reflected in the node balance in tests.

kallewoof commented at 3:22 AM on May 20, 2019:

I investigated further, and this is indeed a bug. I updated the tests to ensure that the balance was updated correctly, and for the case where this if block was present, the non-dirty balance would become 0 when it should be 5.

diff --git a/test/functional/wallet_avoidreuse.py b/test/functional/wallet_avoidreuse.py
index b7b3eae34..2964d0868 100755
--- a/test/functional/wallet_avoidreuse.py
+++ b/test/functional/wallet_avoidreuse.py
@@ -105,11 +105,15 @@ class AvoidReuseTest(BitcoinTestFramework):

         self.nodes[1].sendtoaddress(address=retaddr, amount=10, avoid_reuse=False)

+        # node 1 should now have about 5 btc left (for both cases)
+        assert_approx(self.nodes[1].getbalance(), 5, 0.001)
+        assert_approx(self.nodes[1].getbalance(avoid_reuse=False), 5, 0.001)
+
     def test_fund_send_fund_send(self):
         '''
         Test the simple case where [1] generates a new address A, then
         [0] sends 10 BTC to A.
-        [1] spends 5 BTC from A. (leaving roughly 4 BTC useable)
+        [1] spends 5 BTC from A. (leaving roughly 5 BTC useable)
         [0] sends 10 BTC to A again.
         [1] tries to spend 10 BTC (fails; dirty).
         [1] tries to spend 4 BTC (succeeds; change address sufficient)
@@ -138,5 +142,9 @@ class AvoidReuseTest(BitcoinTestFramework):

         self.nodes[1].sendtoaddress(retaddr, 4)

+        # node 1 should now have about 1 btc left (no dirty) and 11 (including dirty)
+        assert_approx(self.nodes[1].getbalance(), 1, 0.001)
+        assert_approx(self.nodes[1].getbalance(avoid_reuse=False), 11, 0.001)
+
 if __name__ == '__main__':
     AvoidReuseTest().main()

For case with if (used) [...] present:

2019-05-20T03:18:33.609000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/Users/me/path/bitcoin/test/functional/test_framework/test_framework.py", line 194, in main
    self.run_test()
  File "./wallet_avoidreuse.py", line 45, in run_test
    self.test_fund_send_fund_senddirty()
  File "./wallet_avoidreuse.py", line 109, in test_fund_send_fund_senddirty
    assert_approx(self.nodes[1].getbalance(), 5, 0.001)
  File "./wallet_avoidreuse.py", line 17, in assert_approx
    raise AssertionError("%s < [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan)))
AssertionError: 0E-8 < [4.999..5.001]

jnewbery commented at 9:51 PM on May 20, 2019:

oh, I think I understand why this code was here now. In the test you've just shown a way of turning coins from dirty to clean by creating a tx that uses up the dirty input and makes a clean change output.

I can see arguments for why you might want to consider it dirty, but on balance I think you've got the right logic now. The change output is a fresh address, so shouldn't be considered dirty.

kallewoof commented at 2:53 AM on May 21, 2019:

Yeah, I think I got the logic mixed up in there when I wrote that block. Thanks for pointing that out!

in src/wallet/rpcwallet.cpp:2724 in 69ec084ace outdated

2719 | +    res.pushKV("success", true);
2720 | +
2721 | +    pwallet->SetWalletFlag(flag, value);
2722 | +
2723 | +    if (flag && WALLET_FLAG_CAVEATS.count(flag)) {
2724 | +        res.pushKV("caveats", WALLET_FLAG_CAVEATS.at(flag));

jnewbery commented at 9:11 PM on May 16, 2019:

I thought this return format was confusing:

→ bcli setwalletflag avoid_reuse true
{
  "success": true,
  "caveats": "You need to rescan the blockchain in order to correctly mark used destinations in the past. Until this is done, some destinations may be considered unused, even if the opposite is the case."
}

→ bcli setwalletflag avoid_reuse false
{
  "success": true,
  "caveats": "You need to rescan the blockchain in order to correctly mark used destinations in the past. Until this is done, some destinations may be considered unused, even if the opposite is the case."
}

I think it'd be clearer if:

instead of success it returned whether the flag is now set
it didn't print the caveat when setting the flag to false
you changed the word "caveat" to "warning", which seems much easier to understand (especially for non-native speakers).

kallewoof commented at 3:02 AM on May 20, 2019:

success flag

I like success better than simply saying what the flag was changed to, because it better addresses the case where the user is targeting the wrong wallet. For example, I may ssh into a server with a node on it, and my intention is to turn a wallet flag on, but in this scenario, I ssh'd into the wrong machine and/or picked the wrong wallet (in the case of a node with multiple wallets).

With success=<flag was actually changed>, I would get a success=false here, which would hopefully lead me to discover my error. With new_flag_value=true here, I would get no such indication.

Perhaps both would be a good compromise here? Add flag_name, flag_old_value, flag_new_value to the results. Too verbose?

caveat for true only

A caveat could apply to turning off a flag as well as turning it on, though, but as this is not the case right now, I'm going with "only show when turning flag to ON" approach.

the word "caveat"

As a non-native speaker, I don't think caveat is that weird a word, but will change it if people think it's hard to understand.

jnewbery commented at 9:14 PM on May 20, 2019:

Add flag_name, flag_old_value, flag_new_value to the results. Too verbose?

Seems ok to me. Alternatively, you could just throw a JSONRPCError if the flag is already set to what was requested. That's the way we usually deal with input errors in the RPCs.

As a non-native speaker, I don't think caveat is that weird a word

I'm going to claim that 'warning' is a far more common word, and conveys the same meaning.

kallewoof commented at 3:19 AM on May 21, 2019:

Throwing seems sensible. Going with that and adding flag name and value (but skipping old value as it's now implied). Switching caveat to warning.

in src/wallet/rpcwallet.cpp:402 in 69ec084ace outdated

 398 | @@ -386,6 +399,7 @@ static UniValue sendtoaddress(const JSONRPCRequest& request)
 399 |                      HelpExampleCli("sendtoaddress", "\"1M72Sfpbz1BPpXFHz9m3CdqATR44Jvaydd\" 0.1")
 400 |              + HelpExampleCli("sendtoaddress", "\"1M72Sfpbz1BPpXFHz9m3CdqATR44Jvaydd\" 0.1 \"donation\" \"seans outpost\"")
 401 |              + HelpExampleCli("sendtoaddress", "\"1M72Sfpbz1BPpXFHz9m3CdqATR44Jvaydd\" 0.1 \"\" \"\" true")
 402 | +            + HelpExampleCli("sendtoaddress", "\"1M72Sfpbz1BPpXFHz9m3CdqATR44Jvaydd\" 0.1 \"\" \"\" false true")

jnewbery commented at 9:31 PM on May 16, 2019:

I don't think this is necessary (and doesn't actually demonstrate the new parameter).

kallewoof commented at 3:05 AM on May 20, 2019:

Yeah. Removed.

in src/wallet/rpcwallet.cpp:742 in 69ec084ace outdated

 738 | @@ -722,7 +739,7 @@ static UniValue getbalance(const JSONRPCRequest& request)
 739 |          return NullUniValue;
 740 |      }
 741 |  
 742 | -    if (request.fHelp || (request.params.size() > 3 ))
 743 | +    if (request.fHelp || request.params.size() > 4)

jnewbery commented at 9:34 PM on May 16, 2019:

Consider also updating the getbalances RPC method (although that could be done in a follow-up PR)

kallewoof commented at 3:11 AM on May 20, 2019:

I think a follow-up is a good idea here. I'll make an effort to write one soon after merge so this is fresh in people's heads.

in src/wallet/init.cpp:42 in 69ec084ace outdated

  38 | @@ -39,7 +39,7 @@ const WalletInitInterface& g_wallet_init_interface = WalletInit();
  39 |  void WalletInit::AddWalletOptions() const
  40 |  {
  41 |      gArgs.AddArg("-addresstype", strprintf("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")", FormatOutputType(DEFAULT_ADDRESS_TYPE)), false, OptionsCategory::WALLET);
  42 | -    gArgs.AddArg("-avoidpartialspends", strprintf("Group outputs by address, selecting all or none, instead of selecting on a per-output basis. Privacy is improved as an address is only used once (unless someone sends to it after spending from it), but may result in slightly higher fees as suboptimal coin selection may result due to the added limitation (default: %u)", DEFAULT_AVOIDPARTIALSPENDS), false, OptionsCategory::WALLET);
  43 | +    gArgs.AddArg("-avoidpartialspends", strprintf("Group outputs by address, selecting all or none, instead of selecting on a per-output basis. Privacy is improved as an address is only used once (unless someone sends to it after spending from it), but may result in slightly higher fees as suboptimal coin selection may result due to the added limitation (default: %u%s)", DEFAULT_AVOIDPARTIALSPENDS, DEFAULT_AVOIDPARTIALSPENDS ? "" : ", but enabled for wallets with \"avoid_reuse\" enabled"), false, OptionsCategory::WALLET);

jnewbery commented at 9:43 PM on May 16, 2019:

I don't understand this. We know at compile time that DEFAULT_AVOIDPARTIALSPENDS is false, so why do you need this ternary operator?

Sjors commented at 2:49 PM on May 17, 2019:

Are you suggesting #ifdef? Because we might switch this default at some point.

jnewbery commented at 9:24 PM on May 20, 2019:

I just think it's weird to have runtime programming logic to construct a string where we already know what that string should be. If the concern is about this help text not being updated if DEFAULT_AVOIDPARTIALSPENDS changes, then I think it would be better to just leave a comment where that constant is defined saying "If this is changed to true, update the help text for -avoidpartialspends"

kallewoof commented at 3:22 AM on May 21, 2019:

I am going to just remove the conditional part of this and have it always say "(always enabled for wallets with \"avoid_reuse\" enabled)". That should resolve the issue, I think?

in test/functional/wallet_avoidreuse.py:22 in 69ec084ace outdated

  17 | +        raise AssertionError("%s < [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan)))
  18 | +    if v > vexp + vspan:
  19 | +        raise AssertionError("%s > [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan)))
  20 | +
  21 | +def reset_balance(node, discardaddr):
  22 | +    '''

jnewbery commented at 9:47 PM on May 16, 2019:

micronit: one-line docstrings should be on one line with the quotes: https://www.python.org/dev/peps/pep-0257/#one-line-docstrings

in test/functional/wallet_avoidreuse.py:120 in 69ec084ace outdated

 115 | +
 116 | +    def test_fund_send_fund_send(self):
 117 | +        '''
 118 | +        Test the simple case where [1] generates a new address A, then
 119 | +        [0] sends 10 BTC to A.
 120 | +        [1] spends 5 BTC from A. (leaving roughly 4 BTC useable)

jnewbery commented at 9:53 PM on May 16, 2019:

roughly 5 BTC usable?

in test/functional/wallet_avoidreuse.py:127 in 69ec084ace outdated

  87 | +        assert_raises_rpc_error(-8, "Wallet flag is immutable", w.setwalletflag, 'disable_private_keys', False)
  88 | +
  89 | +        # Unload temp wallet
  90 | +        self.nodes[1].unloadwallet(tempwallet)
  91 | +
  92 | +    def test_fund_send_fund_senddirty(self):

jnewbery commented at 9:55 PM on May 16, 2019:

Can you just combine this with test_fund_send_fund_send() by adding the final line:

self.nodes[1].sendtoaddress(address=retaddr, amount=10, avoid_reuse=False)

to that test?

kallewoof commented at 3:24 AM on May 20, 2019:

I think I like the two separate, especially now that they ensure the balance updates are correct in each case. Look at the updated tests, and if you still think they should be merged, I can do so!

in doc/release-notes-13756.md:33 in 69ec084ace outdated

  28 | +whether already used addresses should be left out or included in the operation.
  29 | +These include:
  30 | +
  31 | +- sendtoaddress
  32 | +- getbalance
  33 | +- getunconfirmedbalance

jnewbery commented at 9:58 PM on May 16, 2019:

remove getunconfirmedbalance

jnewbery commented at 10:00 PM on May 16, 2019: member

Sorry for being so negligent in reviewing this. I've gone through all the commits again and have a few comments.

I think the avoid_reuse parameter should also be added to sendmany, but that could be done in a follow-up PR and shouldn't hold this up any more.

kallewoof force-pushed on May 20, 2019

kallewoof commented at 3:42 AM on May 20, 2019: member

@jnewbery Thanks for the review! It's 100% my responsibility to get people to review, so no worries at all. :)

kallewoof force-pushed on May 20, 2019

DrahtBot removed the label Needs rebase on May 20, 2019

in test/functional/wallet_avoidreuse.py:87 in 14b08a27b3 outdated

  82 | +        self.nodes[1].unloadwallet(tempwallet)
  83 | +
  84 | +    def test_fund_send_fund_senddirty(self):
  85 | +        '''
  86 | +        Test the same as test_fund_send_fund_send, except send the 10 BTC with
  87 | +        the avoid_reuse flag set. This means the 10 BTC send should succeed,

jnewbery commented at 9:52 PM on May 20, 2019:

I think this needs to be updated to say "node 1 sends the 10BTC with the avoid_reuse flag set to false"

jnewbery commented at 9:53 PM on May 20, 2019: member

Looking better. Thanks for the quick response to my review. I had just a few more comments inline.

kallewoof force-pushed on May 21, 2019

kallewoof commented at 3:36 AM on May 21, 2019: member

@jnewbery Thanks for follow-up review! I believe I addressed everything you brought up.

Edit: will address the flag comment after @achow101 or someone confirms that it's OK. I also left "caveat" as is internally. I can switch that to warning too, but it didn't feel super necessary.

in src/wallet/wallet.cpp:3101 in f932ca0d8e outdated

3096 | +            // as used as appropriate
3097 | +            bool used = false;
3098 | +            if (IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE)) {
3099 | +                for (const auto& vin : wtxNew.tx->vin) {
3100 | +                    if (IsUsedDestination(vin.prevout.hash, vin.prevout.n)) {
3101 | +                        used = true;

jnewbery commented at 4:43 PM on May 22, 2019:

I don't understand what this code block is doing. You have a boolean used which is initially set to false and then may be updated to true, but you never use the value of used anywhere.

kallewoof commented at 4:21 AM on May 23, 2019:

That was leftovers from the incorrect code that I removed earlier. Thanks, removed!

kallewoof force-pushed on May 23, 2019

in src/wallet/rpcwallet.cpp:2682 in f27b3c8df8 outdated

2677 | +        throw std::runtime_error(
2678 | +            RPCHelpMan{"setwalletflag",
2679 | +                "\nChange the state of the given wallet flag for a wallet.\n",
2680 | +                {
2681 | +                    {"flag", RPCArg::Type::STR, RPCArg::Optional::NO, "The name of the flag to change. Current available flags: " + flags},
2682 | +                    {"state", RPCArg::Type::BOOL, /* default */ "true", "The new state."},

jnewbery commented at 7:57 PM on May 23, 2019:

This needs to match the name in rpc/client.cpp and the CRPCCommand table below (which is currently value).

kallewoof commented at 5:44 AM on May 24, 2019:

Changed to value.

jnewbery commented at 8:39 PM on May 23, 2019: member

Getting close!

I think we should update listunspent to take a avoid_reuse flag, or at the very least default to showing coins that are reusing addresses. I was testing this manually and got very confused when listunspent stopped showing coins in my wallet that I knew existed.

kallewoof force-pushed on May 24, 2019

kallewoof commented at 5:45 AM on May 24, 2019: member

@jnewbery Thanks a lot for all the feedback. I updated listunspent to (1) show reused UTXOs and (2) show a flag "reused" if the wallet has "avoid_reuse" enabled.

I also updated the tests to check that the listunspent results match what you would expect after each send.

~~Edit: I realized that someone added an UnsetWalletFlag method at some point. I've removed that and switched the 4 uses of it in wallet.cpp to call SetWalletFlag(.., false) instead.~~ (reverted 2019-05-29 18:30 JST)

kallewoof force-pushed on May 24, 2019

jnewbery commented at 4:00 PM on May 24, 2019: member

utACK 3ae67c42386c7c34b1e4d6bafe0d2233ae4523ea @achow101's comment here #13756 (review) could be addressed in a follow-up PR.

kallewoof force-pushed on May 25, 2019

jnewbery commented at 2:21 PM on May 28, 2019: member

utACK 66f3e9780d916d9983bab1c153ab4f303d55bbd0

DrahtBot added the label Needs rebase on May 29, 2019

wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS 129a5bafd9

kallewoof force-pushed on May 29, 2019

wallet: make IsWalletFlagSet() const 58928098c2

wallet: avoid reuse flags

Add m_avoid_address_reuse flag to coin control object.
Add avoid_reuse wallet flag and accompanying strings/caveats.

eec15662fa

wallet: enable avoid_reuse feature 8247a0da3a

wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS f904723e0d

wallet/rpc: add 'avoid_reuse' option to RPC commands

createwallet, getbalance, getwalletinfo, listunspent, sendtoaddress

rpc/wallet: listunspent include reused flag and show reused utxos by default

0bdfbd34cf

test: add test for avoidreuse feature 8f2e208f7c

wallet: enable avoid_partial_spends by default if avoid_reuse is set 27669551da

doc: release notes for avoid_reuse ada258f8c8

bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets 5ebc6b0eb2

kallewoof force-pushed on May 29, 2019

kallewoof commented at 9:50 AM on May 29, 2019: member

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

DrahtBot removed the label Needs rebase on May 29, 2019

jnewbery commented at 7:34 AM on June 6, 2019: member

ACK 5ebc6b0eb

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

meshcollider added this to the "Blockers" column in a project

laanwj commented at 8:58 AM on June 17, 2019: member

Concept and code-review ACK 5ebc6b0eb267e0552c66fffc5e5afe7df8becf80

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 commented at 11:03 AM on June 17, 2019: member

@laanwj https://github.com/bitcoin/bitcoin/commit/f3855781fdc52382b7ccfd0060623fa8ab1541b5 is not the right commit, FYI!

jtimon commented at 4:11 PM on June 17, 2019: contributor

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 requested review from meshcollider on Jun 18, 2019

fanquake commented at 12:22 AM on June 18, 2019: member

@achow101 Might also want to take a final look?

laanwj commented at 7:54 AM on June 18, 2019: member

@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 commented at 12:36 PM on June 18, 2019: contributor

Code review ACK https://github.com/bitcoin/bitcoin/pull/13756/commits/5ebc6b0eb267e0552c66fffc5e5afe7df8becf80

Only lightly reviewed the test (8f2e208f7c0468f9ba92bc789a698281b1c81284) 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.

in test/functional/wallet_avoidreuse.py:110 in 8f2e208f7c outdated

 105 | +
 106 | +        # Attempting to set flag to its current state should throw
 107 | +        assert_raises_rpc_error(-8, "Wallet flag is already set to false", self.nodes[0].setwalletflag, 'avoid_reuse', False)
 108 | +        assert_raises_rpc_error(-8, "Wallet flag is already set to true", self.nodes[1].setwalletflag, 'avoid_reuse', True)
 109 | +
 110 | +    def test_immutable(self):

meshcollider commented at 12:41 PM on June 18, 2019:

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

kallewoof commented at 2:26 PM on June 18, 2019:

Makes sense to me.

meshcollider approved

in src/script/ismine.h:25 in 8247a0da3a outdated

  19 | @@ -20,7 +20,9 @@ enum isminetype
  20 |      ISMINE_NO         = 0,
  21 |      ISMINE_WATCH_ONLY = 1 << 0,
  22 |      ISMINE_SPENDABLE  = 1 << 1,
  23 | +    ISMINE_USED       = 1 << 2,
  24 |      ISMINE_ALL        = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
  25 | +    ISMINE_ALL_USED   = ISMINE_ALL | ISMINE_USED,

achow101 commented at 2:14 PM on June 18, 2019:

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.

kallewoof commented at 2:29 PM on June 18, 2019:

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.

in src/wallet/rpcwallet.cpp:2672 in 5ebc6b0eb2

2667 | +
2668 | +    if (!EnsureWalletIsAvailable(pwallet, request.fHelp)) {
2669 | +        return NullUniValue;
2670 | +    }
2671 | +
2672 | +    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2) {

achow101 commented at 2:20 PM on June 18, 2019:

nit: Use RPCHelpMan help{...}. See https://github.com/bitcoin/bitcoin/blob/master/src/rpc/rawtransaction.cpp#L74 for an example.

kallewoof commented at 3:26 PM on June 18, 2019:

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

kallewoof commented at 2:47 AM on June 19, 2019:

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

https://github.com/bitcoin/bitcoin/blob/44d81723236114f9370f386f3b3310477a6dde43/src/wallet/rpcwallet.cpp#L2671-L2674

Still worth it?

in src/wallet/rpcwallet.cpp:2931 in 5ebc6b0eb2

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

achow101 commented at 2:56 PM on June 18, 2019:

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

kallewoof commented at 3:25 PM on June 18, 2019:

Why does it break?

achow101 commented at 3:35 PM on June 18, 2019:

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.

kallewoof commented at 3:41 PM on June 18, 2019:

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

achow101 commented at 3:09 PM on June 18, 2019: member

ACK 5ebc6b0eb267e0552c66fffc5e5afe7df8becf80 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 commented at 11:30 PM on June 18, 2019: contributor

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 merged this on Jun 18, 2019

meshcollider closed this on Jun 18, 2019

meshcollider referenced this in commit 44d8172323 on Jun 18, 2019

Sjors commented at 11:52 PM on June 18, 2019: member

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

kallewoof deleted the branch on Jun 19, 2019

sidhujag referenced this in commit 6bcaa95d9d on Jun 19, 2019

in doc/release-notes-13756.md:35 in 5ebc6b0eb2

  30 | +
  31 | +- createwallet
  32 | +- getbalance
  33 | +- sendtoaddress
  34 | +
  35 | +In addition, `sendtoaddress` has been changed to enable `-avoidpartialspends` when

MarcoFalke commented at 2:41 PM on June 19, 2019:

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

kallewoof commented at 4:03 PM on June 19, 2019:

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?

in doc/release-notes-13756.md:32 in 5ebc6b0eb2

  27 | +Several RPCs have been updated to include an "avoid_reuse" flag, used to control
  28 | +whether already used addresses should be left out or included in the operation.
  29 | +These include:
  30 | +
  31 | +- createwallet
  32 | +- getbalance

MarcoFalke commented at 2:42 PM on June 19, 2019:

What about getbalances?

kallewoof commented at 4:04 PM on June 19, 2019:

In #16239.

in doc/release-notes-13756.md:10 in 5ebc6b0eb2

   5 | +
   6 | +A new wallet flag `avoid_reuse` has been added (default off). When enabled,
   7 | +a wallet will distinguish between used and unused addresses, and default to not
   8 | +use the former in coin selection.
   9 | +
  10 | +(Note: rescanning the blockchain is required, to correctly mark previously

MarcoFalke commented at 2:43 PM on June 19, 2019:

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

kallewoof commented at 4:05 PM on June 19, 2019:

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

in src/wallet/rpcwallet.cpp:393 in 5ebc6b0eb2

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

MarcoFalke commented at 2:47 PM on June 19, 2019:

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"

kallewoof commented at 4:05 PM on June 19, 2019:

Yep, fixed in #16239.

in src/wallet/rpcwallet.cpp:752 in 5ebc6b0eb2

 748 | @@ -733,6 +749,7 @@ static UniValue getbalance(const JSONRPCRequest& request)
 749 |                      {"dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Remains for backward compatibility. Must be excluded or set to \"*\"."},
 750 |                      {"minconf", RPCArg::Type::NUM, /* default */ "0", "Only include transactions confirmed at least this many times."},
 751 |                      {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Also include balance in watch-only addresses (see 'importaddress')"},
 752 | +                    {"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."},

MarcoFalke commented at 2:48 PM on June 19, 2019:

Same here

kallewoof commented at 4:09 PM on June 19, 2019:

Good catch. Added to #16239.

MarcoFalke commented at 2:51 PM on June 19, 2019: member

Looked at the documentation and left some comments

meshcollider referenced this in commit 2cbcc55ba6 on Jun 22, 2019

sidhujag referenced this in commit ef2a6872bc on Jun 22, 2019

fanquake referenced this in commit ca80fec973 on Jun 26, 2019

jnewbery removed this from the "Blockers" column in a project

luke-jr referenced this in commit 92e048d412 on Feb 22, 2020

luke-jr referenced this in commit 5e8217691e on Feb 22, 2020

luke-jr referenced this in commit e9aec6ef89 on Mar 5, 2020

vansergen referenced this in commit 86c643717b on Mar 26, 2020

MarcoFalke referenced this in commit c5966a87d1 on Apr 6, 2020

sidhujag referenced this in commit 9fe8a19478 on Apr 8, 2020

ryanofsky referenced this in commit 02c9d2c78d on Apr 12, 2020

ryanofsky referenced this in commit 5a9bbfe5e7 on Apr 13, 2020

sidhujag referenced this in commit c064c8cba7 on Apr 19, 2020

ryanofsky referenced this in commit 6847561ce1 on Apr 27, 2020

ryanofsky referenced this in commit 0502b35473 on Apr 27, 2020

ryanofsky referenced this in commit 4eea4ac8f3 on May 4, 2020

ryanofsky referenced this in commit 4855c6271c on May 4, 2020

luke-jr referenced this in commit 0e9e25ade0 on May 6, 2020

ryanofsky referenced this in commit 6e48213820 on May 27, 2020

ryanofsky referenced this in commit 3a58f2ae17 on May 27, 2020

deadalnix referenced this in commit 7d44fb717d on Jun 4, 2020

deadalnix referenced this in commit d8fefbbec6 on Jun 4, 2020

deadalnix referenced this in commit fe7e6dcdc4 on Jun 4, 2020

deadalnix referenced this in commit 71e5ab2112 on Jun 5, 2020

deadalnix referenced this in commit 162108998d on Jun 9, 2020

deadalnix referenced this in commit 3f601d7850 on Jun 9, 2020

deadalnix referenced this in commit fc648c84ba on Jun 9, 2020

deadalnix referenced this in commit e471189790 on Jun 9, 2020

deadalnix referenced this in commit 449e061db0 on Jun 9, 2020

deadalnix referenced this in commit 5e09061160 on Jun 9, 2020

deadalnix referenced this in commit f3e4c24c98 on Jun 9, 2020

ryanofsky referenced this in commit f8d7abef76 on Jun 17, 2020

ryanofsky referenced this in commit 005f0dfd16 on Jun 17, 2020

ryanofsky referenced this in commit e44e5a2e33 on Jul 1, 2020

ryanofsky referenced this in commit 6fe1199ee5 on Jul 1, 2020

ryanofsky referenced this in commit 36aa5fee72 on Jul 10, 2020

ryanofsky referenced this in commit 11b08858f3 on Jul 10, 2020

ryanofsky referenced this in commit 487684727b on Jul 13, 2020

ryanofsky referenced this in commit da12e67616 on Jul 14, 2020

ryanofsky referenced this in commit 7aaf995e81 on Jul 14, 2020

ftrader referenced this in commit e75ea52d1f on Aug 17, 2020

ryanofsky referenced this in commit 4ca322b86d on Aug 28, 2020

ryanofsky referenced this in commit 12462bcd54 on Aug 28, 2020

ryanofsky referenced this in commit e04daaeb92 on Sep 13, 2020

ryanofsky referenced this in commit 7fbfefab87 on Sep 13, 2020

ryanofsky referenced this in commit 793f7e8118 on Jan 21, 2021

ryanofsky referenced this in commit 75897d46a5 on Jan 21, 2021

ryanofsky referenced this in commit 7a05b1dee2 on Mar 3, 2021

ryanofsky referenced this in commit 14b1d5f3e9 on Mar 3, 2021

Munkybooty referenced this in commit 5ca34f593d on Oct 1, 2021

vijaydasmp referenced this in commit aad0ab973e on Dec 11, 2021

vijaydasmp referenced this in commit 24abaed5fa on Dec 13, 2021

vijaydasmp referenced this in commit de9acf52a0 on Dec 13, 2021

vijaydasmp referenced this in commit f5bd9f93f2 on Dec 13, 2021

vijaydasmp referenced this in commit ad94a6be0a on Dec 14, 2021

vijaydasmp referenced this in commit f6ae538cef on Dec 14, 2021

vijaydasmp referenced this in commit b16bf93aa8 on Dec 14, 2021

vijaydasmp referenced this in commit d6fb51e33d on Dec 15, 2021

vijaydasmp referenced this in commit 80fa0fa2f7 on Dec 15, 2021

vijaydasmp referenced this in commit 74e61761e2 on Dec 15, 2021

vijaydasmp referenced this in commit ffc3102ce5 on Dec 15, 2021

MarcoFalke locked this on Dec 16, 2021