[WIP] [wallet] Optional '-avoidreuse' flag which defaults to not reusing addresses in sends #10386

kallewoof · 2017-05-11T02:13:32Z

(Note: putting this on hold until #12257 is merged.)

#10065 brings up a privacy issue where a user can send a bunch of near-dust transactions to an address, which would be picked up by the coin select code when the owner funded transactions, connecting multiple transactions and addresses to the same user.

This adds a (by default turned off) flag -avoidreuse. When enabled, the wallet will mark any addresses that were used to fund a transaction as "dirty" and will avoid using them in funding additional transactions, unless an "allow dirty" flag is set.

It also adds support to allow dirty addresses in sendtoaddress. More tweaks to other RPC commands is necessary but I wanted to keep the PR as small as possible.

Retroactive flagging of dirty addresses can be done by rescanning the chain.

luke-jr · 2017-05-11T02:48:58Z

IMO this isn't complete unless incoming transactions to dirty addresses (even before being used!) are hidden as well (or at least flagged in some visible manner).

Also, shouldn't rescanning be sufficient?

kallewoof · 2017-05-11T02:52:48Z

@luke-jr: Maybe I am misunderstanding you, but incoming transactions are irrelevant. The only thing that matters is when you spend from an address. Each time you do, that address is marked dirty and any UTXOs pointing to it are automatically considered dirty in this implementation. Am I missing a case?

Also, shouldn't rescanning be sufficient?

Oops - yes, rescanning is sufficient. Updated OP, thanks.

Edit: my definition of address reuse has always been "spending from the same address 2+ times", whereas @luke-jr's definition seems to be "any UTXOs which send to an address that has already been sent to". Both definitions would solve the issue in question, but the latter would mean people could no longer say "to support my work send BTC to [static address]" if they wished to use this feature. The question ultimately is "which one of the two definitions makes the most sense?"

jonasschnelli · 2017-05-11T06:18:43Z

src/wallet/wallet.cpp

@@ -846,6 +875,14 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)

    uint256 hash = wtxIn.GetHash();

+    if (GetBoolArg("-avoidreuse", false)) {


Maybe use a constant for the default value here.

jonasschnelli · 2017-05-11T06:19:36Z

src/wallet/wallet.cpp

@@ -3598,6 +3639,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
    strUsage += HelpMessageOpt("-walletnotify=<cmd>", _("Execute command when a wallet transaction changes (%s in cmd is replaced by TxID)"));
    strUsage += HelpMessageOpt("-zapwallettxes=<mode>", _("Delete all wallet transactions and only recover those parts of the blockchain through -rescan on startup") +
                               " " + _("(1 = keep tx meta data e.g. account owner and payment request information, 2 = drop tx meta data)"));
+    strUsage += HelpMessageOpt("-avoidreuse", _("Mark addresses which have been used to fund transactions in the past, and avoid reusing these in future funding, except when explicitly requested"));


Missing default value (something like (strprintf(_("(default: %u)"), DEFAULT_AVOIDREUSE));).

jonasschnelli · 2017-05-11T06:22:26Z

src/wallet/wallet.cpp

+        if (ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst)) {
+            if (::IsMine(*this, dst)) {
+                if (dirty && !GetDestData(dst, "dirty", NULL)) {
+                    AddDestData(dst, "dirty", "p"); // p for "present", opposite of absent (null)


Maybe add a AssertLockHeld(cs_wallet); somewhere above GetDestData?

jonasschnelli · 2017-05-11T06:26:28Z

src/wallet/rpcwallet.cpp

@@ -416,12 +419,15 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
            "                             transaction, just kept in your wallet.\n"
            "5. subtractfeefromamount  (boolean, optional, default=false) The fee will be deducted from the amount being sent.\n"
            "                             The recipient will receive less bitcoins than you enter in the amount field.\n"
+            "6. allowdirty             (boolean, optional, " + (GetBoolArg("-avoidreuse", false) ? "default=false" : "unavailable") + ") Allows spending from dirty addresses; addresses are considered\n"


Why unavailable? Someone may just have enabled -avoidreuse but hasn't rescanned. Maybe keep a state somewhere if we have scanned with -avoidreuse up to the wallets bestblock.

It returns "unavailable" if you do not have the feature turned on. I don't want people to think they can avoid using dirty coins if they are running without the flag. I.e. even by explicitly saying allowdirty=false, it will still use dirty coins when -avoidreuse is not enabled.

You are right though, that it will still use dirty coins unless you rescan. Feels like that should be mentioned somewhere, but not sure where.

gmaxwell · 2017-05-18T10:09:43Z

I support the goal, but: If an address is paid 10 btc then 0.0001 btc and then a transaction spends the latter, A would be dirty and the 10 BTC are stuck. That seems sub-optimal.

The best idea I had to deal with that previously is that whenever you spend from A you make an effort to spend all payments to A or at least all non-dust payments to A. Then the only time you get dirty funds is when someone pays a non-negligible amount to an address after you've already spent to it.

If something is going to cause transactions to fail, we probably will need another kind of balance display to make the behavior explicable.

Independently from this (but also useful for it) we probably should have an icon in the transaction list for reused addresses. (Trefoil made of arrows? :P )

kallewoof · 2017-05-18T13:49:27Z

You are able to spend by using the allowdirty flag in sendtoaddress, and you can always make a raw transaction yourself. The intention of this is to give expert users a way to plug the gaping security hole that exists in the system -- to make it intuitive and wonderful for the average user is not an aspiration at this point (as mentioned, in particular RPC commands need some work before this could ever be made a default-on feature).

The solutions you present are all solutions an expert user could make use of with this implementation. It would simply stop their clients from shooting them (privacy wise) in the foot automatically.

Edit: Re-reading, I realize you are talking about coin select algorithm. That's an interesting idea. It would make sense to consider all "coins" going to A as a single coin for as long as you haven't spent from A yet. That way you select on an all-or-nothing basis (perhaps excluding dust).

kallewoof · 2017-07-13T02:00:17Z

Rebased (and code became slightly more clean thanks to new coin control being present).

promag

Should creating transactions to dirty addresses fail?

Needs rebase.

Nit, commit messages don't have period.

promag · 2018-01-09T12:46:44Z

src/wallet/coincontrol.h

@@ -27,6 +27,8 @@ class CCoinControl
    boost::optional<CFeeRate> m_feerate;
    //! Override the default confirmation target if set
    boost::optional<unsigned int> m_confirm_target;
+    //! Allows inclusion of dirty (previously used) addresses
+    bool fAllowDirtyAddresses;


bool allow_dirty_addresses.

Missed this comment earlier; fixed.

promag · 2018-01-09T12:50:16Z

src/wallet/rpcwallet.cpp

@@ -500,6 +504,7 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
        }
    }

+    coin_control.fAllowDirtyAddresses = !request.params[8].isNull() && request.params[8].get_bool();


This does not default to DEFAULT_AVOIDREUSE.

kallewoof · 2018-01-10T01:04:18Z

@promag Thanks for the review! Fixed the default usage issue.

Edit: and the commit message dots.

@luke-jr

…n select 232f96f doc: Add release notes for -avoidpartialspends (Karl-Johan Alm) e00b469 clean-up: Remove no longer used ivars from CInputCoin (Karl-Johan Alm) 43e04d1 wallet: Remove deprecated OutputEligibleForSpending (Karl-Johan Alm) 0128121 test: Add basic testing for wallet groups (Karl-Johan Alm) 59d6f7b wallet: Switch to using output groups instead of coins in coin selection (Karl-Johan Alm) 87ebce2 wallet: Add output grouping (Karl-Johan Alm) bb629cb Add -avoidpartialspends and m_avoid_partial_spends (Karl-Johan Alm) 65b3eda wallet: Add input bytes to CInputCoin (Karl-Johan Alm) a443d7a moveonly: CoinElegibilityFilter into coinselection.h (Karl-Johan Alm) 173e18a utils: Add insert() convenience templates (Karl-Johan Alm) Pull request description: This PR adds an optional (off by default) `-avoidpartialspends` flag, which changes coin select to use output groups rather than outputs, where each output group corresponds to all outputs with the same destination. It is a privacy improvement, as each time you spend some output, any other output that is publicly associated with the destination (address) will also be spent at the same time, at the cost of fee increase for cases where coin select without group restriction would find a more optimal set of coins (see example below). For regular use without address reuse, this PR should have no effect on the user experience whatsoever; it only affects users who, for some reason, have multiple outputs with the same destination (i.e. address reuse). Nodes with this turned off will still try to avoid partial spending, if the fee of the resulting transaction is not greater than the fee of the original transaction. Example: a node has four outputs linked to two addresses `A` and `B`: * 1.0 btc to `A` * 0.5 btc to `A` * 1.0 btc to `B` * 0.5 btc to `B` The node sends 0.2 btc to `C`. Without `-avoidpartialspends`, the following coin selection will occur: * 0.5 btc to `A` or `B` is picked * 0.2 btc is output to `C` * 0.3 - fee is output to (unique change address) With `-avoidpartialspends`, the following will instead happen: * Both of (0.5, 1.0) btc to `A` or `B` is picked (one or the other pair) * 0.2 btc is output to `C` * 1.3 - fee is output to (unique change address) As noted, the pro here is that, assuming nobody sends to the address after you spend from it, you will only ever use one address once. The con is that the transaction becomes slightly larger in this case, because it is overpicking outputs to adhere to the no partial spending rule. This complements #10386, in particular it addresses @luke-jr and @gmaxwell's concerns in #10386 (comment) and #10386 (comment). Together with `-avoidreuse`, this fully addresses the concerns in #10065 I believe. Tree-SHA512: 24687a4490ba59cf4198ed90052944ff4996653a4257833bb52ed24d058b3e924800c9b3790aeb6be6385b653b49e304453e5d7ff960e64c682fc23bfc447621

DrahtBot · 2018-07-24T18:52:05Z

Needs rebase

… in AvailableCoins

…rty tracking optional as -avoidreuse flag

…dirty state

5ebc6b0 bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets (Karl-Johan Alm) ada258f doc: release notes for avoid_reuse (Karl-Johan Alm) 2766955 wallet: enable avoid_partial_spends by default if avoid_reuse is set (Karl-Johan Alm) 8f2e208 test: add test for avoidreuse feature (Karl-Johan Alm) 0bdfbd3 wallet/rpc: add 'avoid_reuse' option to RPC commands (Karl-Johan Alm) f904723 wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS (Karl-Johan Alm) 8247a0d wallet: enable avoid_reuse feature (Karl-Johan Alm) eec1566 wallet: avoid reuse flags (Karl-Johan Alm) 5892809 wallet: make IsWalletFlagSet() const (Karl-Johan Alm) 129a5ba wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS (Karl-Johan Alm) Pull request description: Add a new wallet flag called `avoid_reuse` which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination. This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments. This replaces #10386 and together with the (now merged) #12257 it addresses #10065 in full. The concerns raised in #10386 (comment) are also addressed due to #12257. ~~Note: this builds on top of #15780.~~ (merged) ACKs for commit 5ebc6b: jnewbery: ACK 5ebc6b0 laanwj: Concept and code-review ACK 5ebc6b0 meshcollider: Code review ACK 5ebc6b0 achow101: ACK 5ebc6b0 modulo above nits Tree-SHA512: fdef45826af544cbbb45634ac367852cc467ec87081d86d08b53ca849e588617e9a0a255b7e7bb28692d15332de58d6c3d274ac003355220e4213d7d9070742e

…rivacy 5ebc6b0 bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets (Karl-Johan Alm) ada258f doc: release notes for avoid_reuse (Karl-Johan Alm) 2766955 wallet: enable avoid_partial_spends by default if avoid_reuse is set (Karl-Johan Alm) 8f2e208 test: add test for avoidreuse feature (Karl-Johan Alm) 0bdfbd3 wallet/rpc: add 'avoid_reuse' option to RPC commands (Karl-Johan Alm) f904723 wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS (Karl-Johan Alm) 8247a0d wallet: enable avoid_reuse feature (Karl-Johan Alm) eec1566 wallet: avoid reuse flags (Karl-Johan Alm) 5892809 wallet: make IsWalletFlagSet() const (Karl-Johan Alm) 129a5ba wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS (Karl-Johan Alm) Pull request description: Add a new wallet flag called `avoid_reuse` which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination. This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments. This replaces bitcoin#10386 and together with the (now merged) bitcoin#12257 it addresses bitcoin#10065 in full. The concerns raised in bitcoin#10386 (comment) are also addressed due to bitcoin#12257. ~~Note: this builds on top of bitcoin#15780.~~ (merged) ACKs for commit 5ebc6b: jnewbery: ACK 5ebc6b0 laanwj: Concept and code-review ACK 5ebc6b0 meshcollider: Code review ACK bitcoin@5ebc6b0 achow101: ACK 5ebc6b0 modulo above nits Tree-SHA512: fdef45826af544cbbb45634ac367852cc467ec87081d86d08b53ca849e588617e9a0a255b7e7bb28692d15332de58d6c3d274ac003355220e4213d7d9070742e

@luke-jr

… in coin select 232f96f doc: Add release notes for -avoidpartialspends (Karl-Johan Alm) e00b469 clean-up: Remove no longer used ivars from CInputCoin (Karl-Johan Alm) 43e04d1 wallet: Remove deprecated OutputEligibleForSpending (Karl-Johan Alm) 0128121 test: Add basic testing for wallet groups (Karl-Johan Alm) 59d6f7b wallet: Switch to using output groups instead of coins in coin selection (Karl-Johan Alm) 87ebce2 wallet: Add output grouping (Karl-Johan Alm) bb629cb Add -avoidpartialspends and m_avoid_partial_spends (Karl-Johan Alm) 65b3eda wallet: Add input bytes to CInputCoin (Karl-Johan Alm) a443d7a moveonly: CoinElegibilityFilter into coinselection.h (Karl-Johan Alm) 173e18a utils: Add insert() convenience templates (Karl-Johan Alm) Pull request description: This PR adds an optional (off by default) `-avoidpartialspends` flag, which changes coin select to use output groups rather than outputs, where each output group corresponds to all outputs with the same destination. It is a privacy improvement, as each time you spend some output, any other output that is publicly associated with the destination (address) will also be spent at the same time, at the cost of fee increase for cases where coin select without group restriction would find a more optimal set of coins (see example below). For regular use without address reuse, this PR should have no effect on the user experience whatsoever; it only affects users who, for some reason, have multiple outputs with the same destination (i.e. address reuse). Nodes with this turned off will still try to avoid partial spending, if the fee of the resulting transaction is not greater than the fee of the original transaction. Example: a node has four outputs linked to two addresses `A` and `B`: * 1.0 btc to `A` * 0.5 btc to `A` * 1.0 btc to `B` * 0.5 btc to `B` The node sends 0.2 btc to `C`. Without `-avoidpartialspends`, the following coin selection will occur: * 0.5 btc to `A` or `B` is picked * 0.2 btc is output to `C` * 0.3 - fee is output to (unique change address) With `-avoidpartialspends`, the following will instead happen: * Both of (0.5, 1.0) btc to `A` or `B` is picked (one or the other pair) * 0.2 btc is output to `C` * 1.3 - fee is output to (unique change address) As noted, the pro here is that, assuming nobody sends to the address after you spend from it, you will only ever use one address once. The con is that the transaction becomes slightly larger in this case, because it is overpicking outputs to adhere to the no partial spending rule. This complements bitcoin#10386, in particular it addresses @luke-jr and @gmaxwell's concerns in bitcoin#10386 (comment) and bitcoin#10386 (comment). Together with `-avoidreuse`, this fully addresses the concerns in bitcoin#10065 I believe. Tree-SHA512: 24687a4490ba59cf4198ed90052944ff4996653a4257833bb52ed24d058b3e924800c9b3790aeb6be6385b653b49e304453e5d7ff960e64c682fc23bfc447621 # Conflicts: # src/Makefile.am # src/bench/coin_selection.cpp # src/wallet/coincontrol.h # src/wallet/coinselection.cpp # src/wallet/coinselection.h # src/wallet/init.cpp # src/wallet/test/coinselector_tests.cpp # src/wallet/wallet.cpp # src/wallet/wallet.h # test/functional/test_runner.py

@luke-jr

… in coin select 232f96f doc: Add release notes for -avoidpartialspends (Karl-Johan Alm) e00b469 clean-up: Remove no longer used ivars from CInputCoin (Karl-Johan Alm) 43e04d1 wallet: Remove deprecated OutputEligibleForSpending (Karl-Johan Alm) 0128121 test: Add basic testing for wallet groups (Karl-Johan Alm) 59d6f7b wallet: Switch to using output groups instead of coins in coin selection (Karl-Johan Alm) 87ebce2 wallet: Add output grouping (Karl-Johan Alm) bb629cb Add -avoidpartialspends and m_avoid_partial_spends (Karl-Johan Alm) 65b3eda wallet: Add input bytes to CInputCoin (Karl-Johan Alm) a443d7a moveonly: CoinElegibilityFilter into coinselection.h (Karl-Johan Alm) 173e18a utils: Add insert() convenience templates (Karl-Johan Alm) Pull request description: This PR adds an optional (off by default) `-avoidpartialspends` flag, which changes coin select to use output groups rather than outputs, where each output group corresponds to all outputs with the same destination. It is a privacy improvement, as each time you spend some output, any other output that is publicly associated with the destination (address) will also be spent at the same time, at the cost of fee increase for cases where coin select without group restriction would find a more optimal set of coins (see example below). For regular use without address reuse, this PR should have no effect on the user experience whatsoever; it only affects users who, for some reason, have multiple outputs with the same destination (i.e. address reuse). Nodes with this turned off will still try to avoid partial spending, if the fee of the resulting transaction is not greater than the fee of the original transaction. Example: a node has four outputs linked to two addresses `A` and `B`: * 1.0 btc to `A` * 0.5 btc to `A` * 1.0 btc to `B` * 0.5 btc to `B` The node sends 0.2 btc to `C`. Without `-avoidpartialspends`, the following coin selection will occur: * 0.5 btc to `A` or `B` is picked * 0.2 btc is output to `C` * 0.3 - fee is output to (unique change address) With `-avoidpartialspends`, the following will instead happen: * Both of (0.5, 1.0) btc to `A` or `B` is picked (one or the other pair) * 0.2 btc is output to `C` * 1.3 - fee is output to (unique change address) As noted, the pro here is that, assuming nobody sends to the address after you spend from it, you will only ever use one address once. The con is that the transaction becomes slightly larger in this case, because it is overpicking outputs to adhere to the no partial spending rule. This complements bitcoin#10386, in particular it addresses @luke-jr and @gmaxwell's concerns in bitcoin#10386 (comment) and bitcoin#10386 (comment). Together with `-avoidreuse`, this fully addresses the concerns in bitcoin#10065 I believe. Tree-SHA512: 24687a4490ba59cf4198ed90052944ff4996653a4257833bb52ed24d058b3e924800c9b3790aeb6be6385b653b49e304453e5d7ff960e64c682fc23bfc447621 # Conflicts: # src/Makefile.am # src/bench/coin_selection.cpp # src/wallet/coincontrol.h # src/wallet/coinselection.cpp # src/wallet/coinselection.h # src/wallet/init.cpp # src/wallet/test/coinselector_tests.cpp # src/wallet/wallet.cpp # src/wallet/wallet.h # test/functional/test_runner.py

@luke-jr

… in coin select 232f96f doc: Add release notes for -avoidpartialspends (Karl-Johan Alm) e00b469 clean-up: Remove no longer used ivars from CInputCoin (Karl-Johan Alm) 43e04d1 wallet: Remove deprecated OutputEligibleForSpending (Karl-Johan Alm) 0128121 test: Add basic testing for wallet groups (Karl-Johan Alm) 59d6f7b wallet: Switch to using output groups instead of coins in coin selection (Karl-Johan Alm) 87ebce2 wallet: Add output grouping (Karl-Johan Alm) bb629cb Add -avoidpartialspends and m_avoid_partial_spends (Karl-Johan Alm) 65b3eda wallet: Add input bytes to CInputCoin (Karl-Johan Alm) a443d7a moveonly: CoinElegibilityFilter into coinselection.h (Karl-Johan Alm) 173e18a utils: Add insert() convenience templates (Karl-Johan Alm) Pull request description: This PR adds an optional (off by default) `-avoidpartialspends` flag, which changes coin select to use output groups rather than outputs, where each output group corresponds to all outputs with the same destination. It is a privacy improvement, as each time you spend some output, any other output that is publicly associated with the destination (address) will also be spent at the same time, at the cost of fee increase for cases where coin select without group restriction would find a more optimal set of coins (see example below). For regular use without address reuse, this PR should have no effect on the user experience whatsoever; it only affects users who, for some reason, have multiple outputs with the same destination (i.e. address reuse). Nodes with this turned off will still try to avoid partial spending, if the fee of the resulting transaction is not greater than the fee of the original transaction. Example: a node has four outputs linked to two addresses `A` and `B`: * 1.0 btc to `A` * 0.5 btc to `A` * 1.0 btc to `B` * 0.5 btc to `B` The node sends 0.2 btc to `C`. Without `-avoidpartialspends`, the following coin selection will occur: * 0.5 btc to `A` or `B` is picked * 0.2 btc is output to `C` * 0.3 - fee is output to (unique change address) With `-avoidpartialspends`, the following will instead happen: * Both of (0.5, 1.0) btc to `A` or `B` is picked (one or the other pair) * 0.2 btc is output to `C` * 1.3 - fee is output to (unique change address) As noted, the pro here is that, assuming nobody sends to the address after you spend from it, you will only ever use one address once. The con is that the transaction becomes slightly larger in this case, because it is overpicking outputs to adhere to the no partial spending rule. This complements bitcoin#10386, in particular it addresses @luke-jr and @gmaxwell's concerns in bitcoin#10386 (comment) and bitcoin#10386 (comment). Together with `-avoidreuse`, this fully addresses the concerns in bitcoin#10065 I believe. Tree-SHA512: 24687a4490ba59cf4198ed90052944ff4996653a4257833bb52ed24d058b3e924800c9b3790aeb6be6385b653b49e304453e5d7ff960e64c682fc23bfc447621 # Conflicts: # src/Makefile.am # src/bench/coin_selection.cpp # src/wallet/coincontrol.h # src/wallet/coinselection.cpp # src/wallet/coinselection.h # src/wallet/init.cpp # src/wallet/test/coinselector_tests.cpp # src/wallet/wallet.cpp # src/wallet/wallet.h # test/functional/test_runner.py

@luke-jr

… in coin select 232f96f doc: Add release notes for -avoidpartialspends (Karl-Johan Alm) e00b469 clean-up: Remove no longer used ivars from CInputCoin (Karl-Johan Alm) 43e04d1 wallet: Remove deprecated OutputEligibleForSpending (Karl-Johan Alm) 0128121 test: Add basic testing for wallet groups (Karl-Johan Alm) 59d6f7b wallet: Switch to using output groups instead of coins in coin selection (Karl-Johan Alm) 87ebce2 wallet: Add output grouping (Karl-Johan Alm) bb629cb Add -avoidpartialspends and m_avoid_partial_spends (Karl-Johan Alm) 65b3eda wallet: Add input bytes to CInputCoin (Karl-Johan Alm) a443d7a moveonly: CoinElegibilityFilter into coinselection.h (Karl-Johan Alm) 173e18a utils: Add insert() convenience templates (Karl-Johan Alm) Pull request description: This PR adds an optional (off by default) `-avoidpartialspends` flag, which changes coin select to use output groups rather than outputs, where each output group corresponds to all outputs with the same destination. It is a privacy improvement, as each time you spend some output, any other output that is publicly associated with the destination (address) will also be spent at the same time, at the cost of fee increase for cases where coin select without group restriction would find a more optimal set of coins (see example below). For regular use without address reuse, this PR should have no effect on the user experience whatsoever; it only affects users who, for some reason, have multiple outputs with the same destination (i.e. address reuse). Nodes with this turned off will still try to avoid partial spending, if the fee of the resulting transaction is not greater than the fee of the original transaction. Example: a node has four outputs linked to two addresses `A` and `B`: * 1.0 btc to `A` * 0.5 btc to `A` * 1.0 btc to `B` * 0.5 btc to `B` The node sends 0.2 btc to `C`. Without `-avoidpartialspends`, the following coin selection will occur: * 0.5 btc to `A` or `B` is picked * 0.2 btc is output to `C` * 0.3 - fee is output to (unique change address) With `-avoidpartialspends`, the following will instead happen: * Both of (0.5, 1.0) btc to `A` or `B` is picked (one or the other pair) * 0.2 btc is output to `C` * 1.3 - fee is output to (unique change address) As noted, the pro here is that, assuming nobody sends to the address after you spend from it, you will only ever use one address once. The con is that the transaction becomes slightly larger in this case, because it is overpicking outputs to adhere to the no partial spending rule. This complements bitcoin#10386, in particular it addresses @luke-jr and @gmaxwell's concerns in bitcoin#10386 (comment) and bitcoin#10386 (comment). Together with `-avoidreuse`, this fully addresses the concerns in bitcoin#10065 I believe. Tree-SHA512: 24687a4490ba59cf4198ed90052944ff4996653a4257833bb52ed24d058b3e924800c9b3790aeb6be6385b653b49e304453e5d7ff960e64c682fc23bfc447621 # Conflicts: # src/Makefile.am # src/bench/coin_selection.cpp # src/wallet/coincontrol.h # src/wallet/coinselection.cpp # src/wallet/coinselection.h # src/wallet/init.cpp # src/wallet/test/coinselector_tests.cpp # src/wallet/wallet.cpp # src/wallet/wallet.h # test/functional/test_runner.py

@luke-jr

… in coin select 232f96f doc: Add release notes for -avoidpartialspends (Karl-Johan Alm) e00b469 clean-up: Remove no longer used ivars from CInputCoin (Karl-Johan Alm) 43e04d1 wallet: Remove deprecated OutputEligibleForSpending (Karl-Johan Alm) 0128121 test: Add basic testing for wallet groups (Karl-Johan Alm) 59d6f7b wallet: Switch to using output groups instead of coins in coin selection (Karl-Johan Alm) 87ebce2 wallet: Add output grouping (Karl-Johan Alm) bb629cb Add -avoidpartialspends and m_avoid_partial_spends (Karl-Johan Alm) 65b3eda wallet: Add input bytes to CInputCoin (Karl-Johan Alm) a443d7a moveonly: CoinElegibilityFilter into coinselection.h (Karl-Johan Alm) 173e18a utils: Add insert() convenience templates (Karl-Johan Alm) Pull request description: This PR adds an optional (off by default) `-avoidpartialspends` flag, which changes coin select to use output groups rather than outputs, where each output group corresponds to all outputs with the same destination. It is a privacy improvement, as each time you spend some output, any other output that is publicly associated with the destination (address) will also be spent at the same time, at the cost of fee increase for cases where coin select without group restriction would find a more optimal set of coins (see example below). For regular use without address reuse, this PR should have no effect on the user experience whatsoever; it only affects users who, for some reason, have multiple outputs with the same destination (i.e. address reuse). Nodes with this turned off will still try to avoid partial spending, if the fee of the resulting transaction is not greater than the fee of the original transaction. Example: a node has four outputs linked to two addresses `A` and `B`: * 1.0 btc to `A` * 0.5 btc to `A` * 1.0 btc to `B` * 0.5 btc to `B` The node sends 0.2 btc to `C`. Without `-avoidpartialspends`, the following coin selection will occur: * 0.5 btc to `A` or `B` is picked * 0.2 btc is output to `C` * 0.3 - fee is output to (unique change address) With `-avoidpartialspends`, the following will instead happen: * Both of (0.5, 1.0) btc to `A` or `B` is picked (one or the other pair) * 0.2 btc is output to `C` * 1.3 - fee is output to (unique change address) As noted, the pro here is that, assuming nobody sends to the address after you spend from it, you will only ever use one address once. The con is that the transaction becomes slightly larger in this case, because it is overpicking outputs to adhere to the no partial spending rule. This complements bitcoin#10386, in particular it addresses @luke-jr and @gmaxwell's concerns in bitcoin#10386 (comment) and bitcoin#10386 (comment). Together with `-avoidreuse`, this fully addresses the concerns in bitcoin#10065 I believe. Tree-SHA512: 24687a4490ba59cf4198ed90052944ff4996653a4257833bb52ed24d058b3e924800c9b3790aeb6be6385b653b49e304453e5d7ff960e64c682fc23bfc447621 # Conflicts: # src/Makefile.am # src/bench/coin_selection.cpp # src/wallet/coincontrol.h # src/wallet/coinselection.cpp # src/wallet/coinselection.h # src/wallet/init.cpp # src/wallet/test/coinselector_tests.cpp # src/wallet/wallet.cpp # src/wallet/wallet.h # test/functional/test_runner.py

@luke-jr

… in coin select 232f96f doc: Add release notes for -avoidpartialspends (Karl-Johan Alm) e00b469 clean-up: Remove no longer used ivars from CInputCoin (Karl-Johan Alm) 43e04d1 wallet: Remove deprecated OutputEligibleForSpending (Karl-Johan Alm) 0128121 test: Add basic testing for wallet groups (Karl-Johan Alm) 59d6f7b wallet: Switch to using output groups instead of coins in coin selection (Karl-Johan Alm) 87ebce2 wallet: Add output grouping (Karl-Johan Alm) bb629cb Add -avoidpartialspends and m_avoid_partial_spends (Karl-Johan Alm) 65b3eda wallet: Add input bytes to CInputCoin (Karl-Johan Alm) a443d7a moveonly: CoinElegibilityFilter into coinselection.h (Karl-Johan Alm) 173e18a utils: Add insert() convenience templates (Karl-Johan Alm) Pull request description: This PR adds an optional (off by default) `-avoidpartialspends` flag, which changes coin select to use output groups rather than outputs, where each output group corresponds to all outputs with the same destination. It is a privacy improvement, as each time you spend some output, any other output that is publicly associated with the destination (address) will also be spent at the same time, at the cost of fee increase for cases where coin select without group restriction would find a more optimal set of coins (see example below). For regular use without address reuse, this PR should have no effect on the user experience whatsoever; it only affects users who, for some reason, have multiple outputs with the same destination (i.e. address reuse). Nodes with this turned off will still try to avoid partial spending, if the fee of the resulting transaction is not greater than the fee of the original transaction. Example: a node has four outputs linked to two addresses `A` and `B`: * 1.0 btc to `A` * 0.5 btc to `A` * 1.0 btc to `B` * 0.5 btc to `B` The node sends 0.2 btc to `C`. Without `-avoidpartialspends`, the following coin selection will occur: * 0.5 btc to `A` or `B` is picked * 0.2 btc is output to `C` * 0.3 - fee is output to (unique change address) With `-avoidpartialspends`, the following will instead happen: * Both of (0.5, 1.0) btc to `A` or `B` is picked (one or the other pair) * 0.2 btc is output to `C` * 1.3 - fee is output to (unique change address) As noted, the pro here is that, assuming nobody sends to the address after you spend from it, you will only ever use one address once. The con is that the transaction becomes slightly larger in this case, because it is overpicking outputs to adhere to the no partial spending rule. This complements bitcoin#10386, in particular it addresses @luke-jr and @gmaxwell's concerns in bitcoin#10386 (comment) and bitcoin#10386 (comment). Together with `-avoidreuse`, this fully addresses the concerns in bitcoin#10065 I believe. Tree-SHA512: 24687a4490ba59cf4198ed90052944ff4996653a4257833bb52ed24d058b3e924800c9b3790aeb6be6385b653b49e304453e5d7ff960e64c682fc23bfc447621 # Conflicts: # src/Makefile.am # src/bench/coin_selection.cpp # src/wallet/coincontrol.h # src/wallet/coinselection.cpp # src/wallet/coinselection.h # src/wallet/init.cpp # src/wallet/test/coinselector_tests.cpp # src/wallet/wallet.cpp # src/wallet/wallet.h # test/functional/test_runner.py

kallewoof force-pushed the feature-white-black-address branch from 794beca to 2e85d49 Compare May 11, 2017 02:50

kallewoof force-pushed the feature-white-black-address branch 2 times, most recently from 66a68dc to a05d03a Compare May 11, 2017 04:21

jonasschnelli added the Wallet label May 11, 2017

jonasschnelli reviewed May 11, 2017

View reviewed changes

kallewoof force-pushed the feature-white-black-address branch 2 times, most recently from dd58577 to daf9e57 Compare May 11, 2017 08:11

kallewoof force-pushed the feature-white-black-address branch from daf9e57 to bc9084c Compare May 19, 2017 00:47

laanwj mentioned this pull request Jun 5, 2017

remove some unused functions #10501

Merged

kallewoof force-pushed the feature-white-black-address branch from bc9084c to 1d7f054 Compare July 13, 2017 01:59

kallewoof force-pushed the feature-white-black-address branch 4 times, most recently from d3385e3 to 7c7268b Compare September 28, 2017 09:05

kallewoof force-pushed the feature-white-black-address branch 2 times, most recently from 8ff3de5 to c3226d4 Compare October 11, 2017 09:19

kallewoof force-pushed the feature-white-black-address branch from c3226d4 to f596a41 Compare December 5, 2017 04:48

promag reviewed Jan 9, 2018

View reviewed changes

kallewoof force-pushed the feature-white-black-address branch from f596a41 to 1f7bc58 Compare January 10, 2018 01:03

kallewoof force-pushed the feature-white-black-address branch from 1f7bc58 to 511234a Compare January 11, 2018 07:32

ajtowns mentioned this pull request Jan 16, 2018

[tests] Rename functional tests #11774

Merged

kallewoof force-pushed the feature-white-black-address branch from 511234a to 1f99433 Compare January 19, 2018 00:53

kallewoof mentioned this pull request Jan 24, 2018

[wallet] Use destination groups instead of coins in coin select #12257

Merged

kallewoof changed the title ~~[wallet] Optional '-avoidreuse' flag which defaults to not reusing addresses in sends~~ [WIP] [wallet] Optional '-avoidreuse' flag which defaults to not reusing addresses in sends Feb 20, 2018

kallewoof closed this Mar 15, 2018

kallewoof reopened this Jul 24, 2018

DrahtBot added the Needs rebase label Jul 24, 2018

kallewoof added 7 commits July 25, 2018 15:05

[wallet] Set 'dirty' DestData for used addresses

ccd5326

[wallet] Exclude dirty coins in AvailableCoins results

b032f70

[wallet] Add allow_dirty_addresses to CCoinControl and adhere to this…

cab7197

… in AvailableCoins

[wallet] [rpc] Add allowdirty option to sendtoaddress RPC and make di…

8297ae1

…rty tracking optional as -avoidreuse flag

[doc] Add -avoidreuse documentation to -help and manpages

fd910c6

[wallet] Make GetAvailableCredit (and thus getbalance RPC) adhere to …

6c178d0

…dirty state

[test] Add test for avoidreuse feature

c353877

kallewoof force-pushed the feature-white-black-address branch from 1f99433 to c353877 Compare July 25, 2018 06:40

kallewoof closed this Jul 25, 2018

kallewoof mentioned this pull request Jul 25, 2018

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

Merged

kallewoof deleted the feature-white-black-address branch July 25, 2018 07:34

HashUnlimited mentioned this pull request Sep 12, 2018

Skip is_closing() check when not available. chaincoin/chaincoin#569

Merged

laanwj removed the Needs rebase label Oct 24, 2019

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

		@@ -846,6 +875,14 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)

		uint256 hash = wtxIn.GetHash();

		if (GetBoolArg("-avoidreuse", false)) {

[WIP] [wallet] Optional '-avoidreuse' flag which defaults to not reusing addresses in sends #10386

[WIP] [wallet] Optional '-avoidreuse' flag which defaults to not reusing addresses in sends #10386

Uh oh!

Conversation

kallewoof commented May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented May 11, 2017

Uh oh!

kallewoof commented May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented May 18, 2017

Uh oh!

kallewoof commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Jul 13, 2017

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 24, 2018

Uh oh!

Uh oh!

kallewoof commented May 11, 2017 •

edited

Loading

kallewoof commented May 11, 2017 •

edited

Loading

kallewoof commented May 18, 2017 •

edited

Loading

promag left a comment •

edited

Loading

kallewoof commented Jan 10, 2018 •

edited

Loading