Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
[wallet] Optional '-avoidreuse' flag which defaults to not reusing addresses in sends #10386
Conversation
|
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? |
|
@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?
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
added the
Wallet
label
May 11, 2017
| + 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) |
jonasschnelli
May 11, 2017
Member
Maybe add a AssertLockHeld(cs_wallet); somewhere above GetDestData?
| @@ -846,6 +875,14 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) | ||
| uint256 hash = wtxIn.GetHash(); | ||
| + if (GetBoolArg("-avoidreuse", false)) { |
| @@ -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")); |
jonasschnelli
May 11, 2017
Member
Missing default value (something like (strprintf(_("(default: %u)"), DEFAULT_AVOIDREUSE));).
| @@ -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" |
jonasschnelli
May 11, 2017
Member
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
May 11, 2017
Contributor
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.
|
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 ) |
|
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
added some commits
May 10, 2017
|
kallewoof |
This commit was signed with a verified signature.
|
64c78c5
|
||
|
|
kallewoof |
This commit was signed with a verified signature.
|
e12eb6f
|
||
|
|
kallewoof |
This commit was signed with a verified signature.
|
e030160
|
||
|
|
kallewoof |
This commit was signed with a verified signature.
|
e2fa7d8
|
||
|
|
kallewoof |
This commit was signed with a verified signature.
|
ee5f61c
|
||
|
|
kallewoof |
This commit was signed with a verified signature.
|
7d1ed07
|
||
|
|
kallewoof |
This commit was signed with a verified signature.
|
1d7f054
|
|
Rebased (and code became slightly more clean thanks to new coin control being present). |
kallewoof commentedMay 11, 2017
•
edited
#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.