Wallet, GUI: Warn when sending to already-used Bitcoin addresses

luke-jr commented at 9:42 pm on May 8, 2019: member

An in-memory bloom filter is used to detect potential address reuse, avoiding wasting unnecessary memory with large wallets.
Entering a used address in the GUI Send tab makes the field turn yellow.
Sending to a used address from the GUI prompts with detailed information about prior usage, as well as a note about best practices to avoid address reuse.
(I also fixed GUIUtil::dateTimeStr to not overflow with 64-bit timestamps.)

gmaxwell commented at 9:45 pm on May 8, 2019: contributor

Meta-concept-ack! we should absolutely do something like this (I haven’t looked at the specifics of what this does yet)

in src/qt/sendcoinsdialog.cpp:324 in 3a6d7ce1dd outdated

292+            if (!prior_usage_info.contains(rcp.address)) continue;
293+            const auto& rcp_prior_usage_info = prior_usage_info.value(rcp.address);
294+            const QString label_and_address = rcp.label.isEmpty() ? rcp.address : (rcp.label + " (" + rcp.address + ")");
295+            reuse_question.append("<br />");
296+            if (rcp_prior_usage_info.num_txs == 1) {
297+                //: %1 is an amount (eg, "1 BTC"); %2 is a Bitcoin address and its label; %3 is a date (eg, "2019-05-08")

luke-jr commented at 9:45 pm on May 8, 2019:

Note: //: is how Qt lets us add notes for translators. (I’m not sure if it survives to Transifex?)

hebasto commented at 0:48 am on March 26, 2021:

Note: //: is how Qt lets us add notes for translators. (I’m not sure if it survives to Transifex?)

Smth like #21465 is required for that.

DrahtBot added the label GUI on May 8, 2019

DrahtBot added the label RPC/REST/ZMQ on May 8, 2019

DrahtBot added the label Wallet on May 8, 2019

DrahtBot commented at 10:45 pm on May 8, 2019: 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:

#17513 (refactor, qt: Nuke some circular dependencies by hebasto)
#17509 (gui: save and load PSBT by Sjors)
#17492 (QT: bump fee returns PSBT on clipboard for watchonly-only wallets by instagibbs)
#17463 (Bugfix: GUI: Restore SendConfirmationDialog button default to “Yes” by luke-jr)
#17457 (gui: Fix manual coin control with multiple wallets loaded by promag)
#16944 (gui: create PSBT with watch-only wallet by Sjors)

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.

fanquake deleted a comment on May 9, 2019

meshcollider commented at 5:48 am on May 9, 2019: contributor

Concept ACK

This addresses #3266 I assume

luke-jr commented at 6:13 am on May 9, 2019: member

It doesn’t take into consideration all the ideas/advice (even my own!) on #3266, but yes, it implements the general idea I think.

jonasschnelli commented at 7:11 am on May 9, 2019: contributor

Concept ACK

hebasto commented at 2:24 pm on May 9, 2019: member

Concept ACK

in src/wallet/wallet.cpp:735 in 667583e786 outdated

730+    }
731+}
732+
733+void CWallet::BuildAddressBloomFilter()
734+{
735+    m_address_bloom_filter_elems = std::max<size_t>(100, mapWallet.size() * 2);

sipa commented at 9:13 pm on May 22, 2019:

Is this code assuming that there are two outputs per transaction? Bloom filter performance is very much dependent on this estimate being correct (if you take a filter designed for N elements with fprate=0.000001, and put 2N elements in it, the resulting fprate is only 0.0032).

luke-jr commented at 1:22 am on May 23, 2019:

The intent was to not need rebuilding until the wallet doubled in size, but I guess it fails to consider transactions having multiple outputs.

Would another factor (or variable) be better here?

luke-jr commented at 2:12 am on August 29, 2019:

Made it 6x for now.

Sjors commented at 6:09 pm on November 15, 2019:

I’m still seeing * 2. Move the constants ADDR_BLOOM_FILTER_TX_TO_ELEMENTS_FACTOR and ADDR_BLOOM_FILTER_FP_RATE defined in 39f49f653541ce172a91a8d6996f44831f9a8df2 to a13c30296f0507b1c6cdd70e42e693fe38d3c9df?

DrahtBot added the label Needs rebase on Jun 6, 2019

promag commented at 10:32 pm on June 12, 2019: member

Concept ACK, didn’t see the code but maybe you could split RPC changes to other PR?

Sjors commented at 5:11 pm on August 15, 2019: member

Concept ACK, but agree with @promag on splitting getaddressinfo into a seperate PR, so we can review that and the bloom filter stuff first.

When I enter a duplicate address the field becomes yellow as expected, but when I also enter an absurdly high amount, it no longer shows the “insufficient balance error”, but instead ignores the amount I entered and falls back to the previous amount. Also use_txids was empty for me with the same address in getaddressinfo. (I tried this on a rebased branch, so maybe I broke it myself)

luke-jr renamed this:
~~Wallet, GUI: Warn when sending to already-used Bitcoin addresses (also RPC: include such information in getaddressinfo)~~
Wallet, GUI: Warn when sending to already-used Bitcoin addresses
on Aug 29, 2019

luke-jr force-pushed on Aug 29, 2019

luke-jr commented at 2:11 am on August 29, 2019: member

Rebased, fixed issues, and moved RPC change to a new rpc_gai_txids branch that can be PR’d after this.

DrahtBot removed the label Needs rebase on Aug 29, 2019

luke-jr force-pushed on Sep 1, 2019

instagibbs commented at 7:03 pm on September 13, 2019: member

concept ACK

laanwj added the label Feature on Sep 30, 2019

DrahtBot added the label Needs rebase on Oct 2, 2019

Wallet: Add memory-only bloom filter for addresses used in wallet transactions a13c30296f

Wallet: Add efficient [negative] check that an address is not known to be used 39f49f6535

interfaces/wallet: Add checkAddressForUsage and findAddressUsage 7a452c5e91

GUI: WalletModel: Wrap checkAddressForUsage and findAddressUsage db6c738382

GUI: QValidatedLineEdit: Add support for a warning-but-valid state b62c4342c1

GUI: Implement BitcoinAddressUnusedInWalletValidator a524075721

GUI: Use warning indicator for send coins entries with reused addresses 400850c782

GUI: Use 64-bit safe QDateTime::fromMSecsSinceEpoch for dateTimeStr

Partially reverts 0030c1bd6cdee44d9f59af9a023cd39cbd335bd5, which was presumably done for compatibility with Qt<4.7.

d08eb8eb1b

GUI: Add GUIUtil::dateStr 051ed3c72b

GUI: SendConfirmationDialog: Enable customisation of dialog

- Dialog icon can be changed
- Both buttons can be replaced with other standard buttons
- "Yes" button can be renamed

4e10bdd433

GUI: Add a warning prompt when sending to an already-used address e9bd26532e

GUI/Send: Use detailedText for address reuse error 391c5d9a97

luke-jr force-pushed on Oct 25, 2019

DrahtBot removed the label Needs rebase on Oct 25, 2019

in src/wallet/wallet.cpp:845 in a13c30296f outdated

840@@ -841,6 +841,23 @@ void CWallet::AddToSpends(const uint256& wtxid)
841         AddToSpends(txin.prevout, wtxid);
842 }
843 
844+void CWallet::AddToAddressBloomFilter(const CWalletTx& wtx)
845+{

Sjors commented at 6:07 pm on November 15, 2019:

Assert that m_address_bloom_filter has been built?

in src/wallet/wallet.h:1022 in 39f49f6535 outdated

1018@@ -1019,6 +1019,8 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
1019     void UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
1020     void ListLockedCoins(std::vector<COutPoint>& vOutpts) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
1021 
1022+    bool FindScriptPubKeyUsed(const std::set<CScript>& keys, const boost::variant<boost::blank, std::function<void(const CWalletTx&)>, std::function<void(const CWalletTx&, uint32_t)>>& callback = boost::blank()) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

Sjors commented at 6:18 pm on November 15, 2019:

Can you document the params here? Also maybe explicitly state that this function corrects for any false positives found with the bloom filter.

I think using std::vector<CWalletTx&>& transactions instead of a callback is more readable (and avoids Boost). Or do you need this to be asynchronous?

in src/wallet/wallet.cpp:877 in 39f49f6535 outdated

874+        }
875+    }
876+    if (!found_any) {
877+        return false;
878+    }
879+

Sjors commented at 6:24 pm on November 15, 2019:

Suggested comment: // Bloom filters can return false positives, so iterate through all wallet addresses

promag commented at 10:23 pm on December 16, 2019:

Could early return if no callback?

in src/interfaces/wallet.cpp:34 in 7a452c5e91 outdated

30@@ -30,6 +31,16 @@
31 namespace interfaces {
32 namespace {
33 
34+std::set<CScript> AddressesToKeys(std::vector<std::string> addresses)

Sjors commented at 6:31 pm on November 15, 2019:

AddressesToScriptPubKeys?

in src/qt/walletmodel.h:163 in db6c738382 outdated

159@@ -158,6 +160,8 @@ class WalletModel : public QObject
160 
161     // Check address for validity
162     bool validateAddress(const QString &address);
163+    bool checkAddressForUsage(const std::vector<std::string>& addresses) const;

Sjors commented at 6:37 pm on November 15, 2019:

checkAddressesForUsage?

in src/qt/sendcoinsdialog.cpp:291 in e9bd26532e outdated

286+        QMap<QString, prior_usage_info_t> prior_usage_info;
287+        {
288+            QStringList addresses;
289+            for (const auto& recipient : recipients) {
290+#ifdef ENABLE_BIP70
291+                if (recipient.paymentRequest.IsInitialized()) continue;

Sjors commented at 6:45 pm on November 15, 2019:

Maybe add a comment here. Why not put ifndef ENABLE_BIP70 before the entire { block? Can users manually add destinations to a BIP70 payment?

fanquake commented at 6:56 pm on November 15, 2019:

Can users manually add destinations to a BIP70 payment?

BIP70 is no longer supported. Looks like this needs to be rebased on master and any ENABLE_BIP70 additions needs to be removed.

Sjors commented at 6:51 pm on November 15, 2019: member

391c5d9a97211b46bc6b4d701e98bf10aae197fa looks good, except the triplet of choices is confusing:

try renaming OK to Cancel and making that the default action
Override should be Send or Send anyway (I initially thought Override meant overriding the address)
Consider dropping Show Details and instead put Sent ... BTC to this address across 2 transactions from 15-11-2019 through 15-11-2019 as a text under the address. Being able to put an error message right under the address is useful for a #16807 GUI followup too.

Given 391c5d9a97211b46bc6b4d701e98bf10aae197fa, I think #17463 should be merged first.

DrahtBot commented at 7:12 pm on November 21, 2019: member

DrahtBot added the label Needs rebase on Nov 21, 2019

promag commented at 10:30 pm on December 16, 2019: member

Would it be too bad to index all wallet’s scriptPubKey?

laanwj added this to the milestone 0.20.0 on Mar 5, 2020

laanwj removed this from the milestone 0.20.0 on Mar 5, 2020

luke-jr commented at 7:34 pm on March 5, 2020: member

Planning to redo this as an addressbook bool once #18192 gets merged…

luke-jr closed this on Mar 5, 2020

hebasto commented at 5:59 am on May 4, 2020: member

Planning to redo this as an addressbook bool once #18192 gets merged…

#18192 and #18546 have been merged already :D

rebroad commented at 5:15 am on August 20, 2020: contributor

@luke-jr re-open?

hebasto commented at 9:01 am on October 3, 2021: member

Up for grabs?

luke-jr commented at 9:24 pm on October 3, 2021: member

It’s maintained. PR is pending on https://github.com/bitcoin-core/gui/pull/404

hebasto referenced this in commit b7942c9482 on Feb 9, 2022

sidhujag referenced this in commit f2bd0d1359 on Feb 9, 2022

luke-jr commented at 7:52 am on February 9, 2022: member

Now blocking on PR #22693 for wallet changes

DrahtBot locked this on Feb 9, 2023

Wallet, GUI: Warn when sending to already-used Bitcoin addresses #15987

Conflicts