Wallet, GUI: Warn when sending to already-used Bitcoin addresses by luke-jr · Pull Request #15987 · bitcoin/bitcoin

luke-jr · 2019-05-08T21:42:10Z

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 · 2019-05-08T21:45:47Z

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

luke-jr · 2019-05-08T21:45:47Z

src/qt/sendcoinsdialog.cpp

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

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 · 2019-05-08T22:45:30Z

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.

meshcollider · 2019-05-09T05:48:17Z

Concept ACK

This addresses #3266 I assume

luke-jr · 2019-05-09T06:13:13Z

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 · 2019-05-09T07:11:39Z

Concept ACK

hebasto · 2019-05-09T14:24:17Z

Concept ACK

src/wallet/wallet.cpp

promag · 2019-06-12T22:32:13Z

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

Sjors · 2019-08-15T17:11:33Z

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 · 2019-08-29T02:11:45Z

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

instagibbs · 2019-09-13T19:03:56Z

concept ACK

…nsactions

…o be used

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

Sjors

391c5d9 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 391c5d9, I think #17463 should be merged first.

Sjors · 2019-11-15T18:07:39Z

src/wallet/wallet.cpp

 }

+void CWallet::AddToAddressBloomFilter(const CWalletTx& wtx)
+{


Assert that m_address_bloom_filter has been built?

src/wallet/wallet.cpp

Sjors · 2019-11-15T18:18:25Z

src/wallet/wallet.h

    void UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    void ListLockedCoins(std::vector<COutPoint>& vOutpts) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

+    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);


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?

Sjors · 2019-11-15T18:24:16Z

src/wallet/wallet.cpp

+    if (!found_any) {
+        return false;
+    }
+


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

Could early return if no callback?

Sjors · 2019-11-15T18:31:53Z

src/interfaces/wallet.cpp

 namespace interfaces {
 namespace {

+std::set<CScript> AddressesToKeys(std::vector<std::string> addresses)


AddressesToScriptPubKeys?

Sjors · 2019-11-15T18:37:20Z

src/qt/walletmodel.h


    // Check address for validity
    bool validateAddress(const QString &address);
+    bool checkAddressForUsage(const std::vector<std::string>& addresses) const;


checkAddressesForUsage?

Sjors · 2019-11-15T18:45:10Z

src/qt/sendcoinsdialog.cpp

+            QStringList addresses;
+            for (const auto& recipient : recipients) {
+#ifdef ENABLE_BIP70
+                if (recipient.paymentRequest.IsInitialized()) continue;


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

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.

DrahtBot · 2019-11-21T19:12:38Z

Needs rebase

promag

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

promag · 2019-12-16T22:23:53Z

src/wallet/wallet.cpp

+    if (!found_any) {
+        return false;
+    }
+


Could early return if no callback?

luke-jr · 2020-03-05T19:34:10Z

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

hebasto · 2020-05-04T05:59:01Z

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

#18192 and #18546 have been merged already :D

rebroad · 2020-08-20T05:15:38Z

@luke-jr re-open?

hebasto · 2021-10-03T09:01:25Z

Up for grabs?

luke-jr · 2021-10-03T21:24:02Z

It's maintained. PR is pending on bitcoin-core/gui#404

aeb18b6 Bugfix: GUI: Check validity when QValidatedLineEdit::setText is called (Luke Dashjr) b1a544b Bugfix: GUI: Re-check validity after QValidatedLineEdit::setCheckValidator (Luke Dashjr) 2385b50 Bugfix: GUI: Only apply invalid style to QValidatedLineEdit, not its tooltip (Luke Dashjr) Pull request description: 1. Use a CSS selector to avoid changing the background colour of the tooltip. 2. Re-check validity of input when we first set the validator (probably a no-op in practice). 3. Check validity of input when it is set programmatically via `setText` (eg, via the address book). Probably no-op in practice UNTIL merging bitcoin/bitcoin#15987 or any other PR that adds a warning for valid addresses. Moved from bitcoin/bitcoin#18133 (just concept ACKs) ACKs for top commit: Sjors: tACK aeb18b6 hebasto: ACK aeb18b6, tested on Linux Mint 20.3 (Qt 5.12.8). Tree-SHA512: b6fa8ee4dec76e1c759095721240e6fa5071a02993cb28406e96a0fa2e819f5dddc03d2e7c9073354d7865c2b09eb263afaf853ecba42e9fc4f50ef4ae20bf0f

…ineEdit aeb18b6 Bugfix: GUI: Check validity when QValidatedLineEdit::setText is called (Luke Dashjr) b1a544b Bugfix: GUI: Re-check validity after QValidatedLineEdit::setCheckValidator (Luke Dashjr) 2385b50 Bugfix: GUI: Only apply invalid style to QValidatedLineEdit, not its tooltip (Luke Dashjr) Pull request description: 1. Use a CSS selector to avoid changing the background colour of the tooltip. 2. Re-check validity of input when we first set the validator (probably a no-op in practice). 3. Check validity of input when it is set programmatically via `setText` (eg, via the address book). Probably no-op in practice UNTIL merging bitcoin#15987 or any other PR that adds a warning for valid addresses. Moved from bitcoin#18133 (just concept ACKs) ACKs for top commit: Sjors: tACK aeb18b6 hebasto: ACK aeb18b6, tested on Linux Mint 20.3 (Qt 5.12.8). Tree-SHA512: b6fa8ee4dec76e1c759095721240e6fa5071a02993cb28406e96a0fa2e819f5dddc03d2e7c9073354d7865c2b09eb263afaf853ecba42e9fc4f50ef4ae20bf0f

luke-jr · 2022-02-09T07:52:49Z

Now blocking on PR #22693 for wallet changes

luke-jr commented May 8, 2019

View reviewed changes

DrahtBot added GUI RPC/REST/ZMQ Wallet labels May 8, 2019

bitcoin deleted a comment from Rockstarrecords11 May 9, 2019

sipa reviewed May 22, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jun 6, 2019

kallewoof mentioned this pull request Jun 19, 2019

Support not reusing addresses #10065

Closed

luke-jr changed the title ~~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 Aug 29, 2019

luke-jr force-pushed the wallet_no_reuse branch from 3a6d7ce to c37f306 Compare August 29, 2019 02:10

DrahtBot removed the Needs rebase label Aug 29, 2019

luke-jr force-pushed the wallet_no_reuse branch 2 times, most recently from 65c3c70 to 545af21 Compare September 1, 2019 16:46

laanwj added the Feature label Sep 30, 2019

DrahtBot added the Needs rebase label Oct 2, 2019

luke-jr added 4 commits October 25, 2019 21:38

Wallet: Add memory-only bloom filter for addresses used in wallet tra…

a13c302

…nsactions

Wallet: Add efficient [negative] check that an address is not known t…

39f49f6

…o be used

interfaces/wallet: Add checkAddressForUsage and findAddressUsage

7a452c5

GUI: WalletModel: Wrap checkAddressForUsage and findAddressUsage

db6c738

luke-jr added 4 commits October 25, 2019 21:39

GUI: Add GUIUtil::dateStr

051ed3c

GUI: SendConfirmationDialog: Enable customisation of dialog

4e10bdd

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

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

e9bd265

GUI/Send: Use detailedText for address reuse error

391c5d9

luke-jr force-pushed the wallet_no_reuse branch from 545af21 to 391c5d9 Compare October 25, 2019 21:41

DrahtBot removed the Needs rebase label Oct 25, 2019

luke-jr mentioned this pull request Nov 4, 2019

gui: grey out used address in address book #17355

Closed

This was referenced Nov 13, 2019

gui: create PSBT with watch-only wallet #16944

Merged

Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" #17463

Closed

Sjors reviewed Nov 15, 2019

View reviewed changes

DrahtBot added the Needs rebase label Nov 21, 2019

promag reviewed Dec 16, 2019

View reviewed changes

src/wallet/wallet.cpp

if (!found_any) {

return false;

}

Copy link

Contributor

promag Dec 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could early return if no callback?

luke-jr mentioned this pull request Feb 13, 2020

Fix various edge case bugs in QValidatedLineEdit #18133

Closed

laanwj modified the milestone: 0.20.0 Mar 5, 2020

luke-jr closed this Mar 5, 2020

luke-jr mentioned this pull request Apr 7, 2020

Store destdata for change in separate key for backward compatibility #18550

Closed

This was referenced Aug 12, 2021

Fix various edge case bugs in QValidatedLineEdit bitcoin-core/gui#404

Merged

RPC/Wallet: Add "use_txids" to output of getaddressinfo #22693

Closed

luke-jr mentioned this pull request Mar 1, 2022

Warn when sending to already-used Bitcoin addresses bitcoin-core/gui#562

Closed

bitcoin locked and limited conversation to collaborators Feb 9, 2023

Conversation

luke-jr commented May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented May 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

meshcollider commented May 9, 2019

Uh oh!

luke-jr commented May 9, 2019

Uh oh!

jonasschnelli commented May 9, 2019

Uh oh!

hebasto commented May 9, 2019

Uh oh!

Uh oh!

promag commented Jun 12, 2019

Uh oh!

Sjors commented Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Aug 29, 2019

Uh oh!

instagibbs commented Sep 13, 2019

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 21, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Mar 5, 2020

Uh oh!

hebasto commented May 4, 2020

Uh oh!

rebroad commented Aug 20, 2020

Uh oh!

hebasto commented Oct 3, 2021

Uh oh!

luke-jr commented Oct 3, 2021

Uh oh!

luke-jr commented Feb 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

luke-jr commented May 8, 2019 •

edited

Loading

DrahtBot commented May 8, 2019 •

edited

Loading

Sjors commented Aug 15, 2019 •

edited

Loading