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

kallewoof commented at 2:13 am on May 11, 2017: member

(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 commented at 2:48 am on May 11, 2017: member

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 force-pushed on May 11, 2017

kallewoof commented at 2:52 am on May 11, 2017: member

@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?”

kallewoof force-pushed on May 11, 2017

jonasschnelli added the label Wallet on May 11, 2017

in src/wallet/wallet.cpp:878 in a05d03a7e8 outdated

874@@ -846,6 +875,14 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
875 
876     uint256 hash = wtxIn.GetHash();
877 
878+    if (GetBoolArg("-avoidreuse", false)) {

jonasschnelli commented at 6:18 am on May 11, 2017:

Maybe use a constant for the default value here.

in src/wallet/wallet.cpp:3642 in a05d03a7e8 outdated

3638@@ -3598,6 +3639,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
3639     strUsage += HelpMessageOpt("-walletnotify=<cmd>", _("Execute command when a wallet transaction changes (%s in cmd is replaced by TxID)"));
3640     strUsage += HelpMessageOpt("-zapwallettxes=<mode>", _("Delete all wallet transactions and only recover those parts of the blockchain through -rescan on startup") +
3641                                " " + _("(1 = keep tx meta data e.g. account owner and payment request information, 2 = drop tx meta data)"));
3642+    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"));

jonasschnelli commented at 6:19 am on May 11, 2017:

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

in src/wallet/wallet.cpp:934 in a05d03a7e8 outdated

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

jonasschnelli commented at 6:22 am on May 11, 2017:

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

in src/wallet/rpcwallet.cpp:422 in a05d03a7e8 outdated

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

jonasschnelli commented at 6:26 am on May 11, 2017:

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.

kallewoof commented at 6:50 am on May 11, 2017:

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.

kallewoof force-pushed on May 11, 2017

gmaxwell commented at 10:09 am on May 18, 2017: contributor

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 commented at 1:49 pm on May 18, 2017: member

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 force-pushed on May 19, 2017

kallewoof force-pushed on Jul 13, 2017

kallewoof commented at 2:00 am on July 13, 2017: member

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

kallewoof force-pushed on Sep 28, 2017

kallewoof force-pushed on Oct 11, 2017

kallewoof force-pushed on Dec 5, 2017

in src/wallet/coincontrol.h:31 in f596a41ca2 outdated

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

promag commented at 12:46 pm on January 9, 2018:

bool allow_dirty_addresses.

kallewoof commented at 7:33 am on January 11, 2018:

Missed this comment earlier; fixed.

in src/wallet/rpcwallet.cpp:507 in f596a41ca2 outdated

503@@ -500,6 +504,7 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
504         }
505     }
506 
507+    coin_control.fAllowDirtyAddresses = !request.params[8].isNull() && request.params[8].get_bool();

promag commented at 12:50 pm on January 9, 2018:

This does not default to DEFAULT_AVOIDREUSE.

promag commented at 1:04 pm on January 9, 2018: member

Should creating transactions to dirty addresses fail?

Needs rebase.

Nit, commit messages don’t have period.

kallewoof force-pushed on Jan 10, 2018

kallewoof commented at 1:04 am on January 10, 2018: member

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

Edit: and the commit message dots.

kallewoof force-pushed on Jan 11, 2018

kallewoof force-pushed on Jan 19, 2018

kallewoof renamed this:
~~[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
on Feb 20, 2018

kallewoof closed this on Mar 15, 2018

laanwj referenced this in commit 5f7575e263 on Jul 24, 2018

kallewoof reopened this on Jul 24, 2018

DrahtBot commented at 6:52 pm on July 24, 2018: member

DrahtBot added the label Needs rebase on Jul 24, 2018

[wallet] Set 'dirty' DestData for used addresses ccd5326d3b

[wallet] Exclude dirty coins in AvailableCoins results b032f70473

[wallet] Add allow_dirty_addresses to CCoinControl and adhere to this in AvailableCoins cab7197213

[wallet] [rpc] Add allowdirty option to sendtoaddress RPC and make dirty tracking optional as -avoidreuse flag 8297ae1611

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

[wallet] Make GetAvailableCredit (and thus getbalance RPC) adhere to dirty state 6c178d0908

[test] Add test for avoidreuse feature c353877abd

kallewoof force-pushed on Jul 25, 2018

kallewoof closed this on Jul 25, 2018

kallewoof deleted the branch on Jul 25, 2018

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

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

laanwj removed the label Needs rebase on Oct 24, 2019

Munkybooty referenced this in commit 3a9c562e6e on Jun 15, 2021

Munkybooty referenced this in commit c40f154def on Jun 15, 2021

Munkybooty referenced this in commit 0f644d5f5f on Jun 16, 2021

Munkybooty referenced this in commit e2bf529053 on Jun 22, 2021

Munkybooty referenced this in commit 82659a4eb8 on Jun 24, 2021

Munkybooty referenced this in commit 640b1ceea2 on Jun 24, 2021

MarcoFalke locked this on Dec 16, 2021

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