Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 12 commits into from

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented May 8, 2019

  • 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
Copy link
Contributor

@gmaxwell gmaxwell commented May 8, 2019

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

const QString label_and_address = rcp.label.isEmpty() ? rcp.address : (rcp.label + " (" + rcp.address + ")");
reuse_question.append("<br />");
if (rcp_prior_usage_info.num_txs == 1) {
//: %1 is an amount (eg, "1 BTC"); %2 is a Bitcoin address and its label; %3 is a date (eg, "2019-05-08")
Copy link
Member Author

@luke-jr luke-jr May 8, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

@hebasto hebasto Mar 26, 2021

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@DrahtBot DrahtBot commented May 8, 2019

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.

@bitcoin bitcoin deleted a comment from Rockstarrecords11 May 9, 2019
@bitcoin bitcoin deleted a comment from Rockstarrecords11 May 9, 2019
@meshcollider
Copy link
Member

@meshcollider meshcollider commented May 9, 2019

Concept ACK

This addresses #3266 I assume

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented May 9, 2019

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
Copy link
Contributor

@jonasschnelli jonasschnelli commented May 9, 2019

Concept ACK

1 similar comment
@hebasto
Copy link
Member

@hebasto hebasto commented May 9, 2019

Concept ACK

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@promag
Copy link
Member

@promag promag commented Jun 12, 2019

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

@Sjors
Copy link
Member

@Sjors Sjors commented Aug 15, 2019

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 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 luke-jr force-pushed the wallet_no_reuse branch from 3a6d7ce to c37f306 Aug 29, 2019
@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Aug 29, 2019

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

@luke-jr luke-jr force-pushed the wallet_no_reuse branch 2 times, most recently from 65c3c70 to 545af21 Sep 1, 2019
@instagibbs
Copy link
Member

@instagibbs instagibbs commented Sep 13, 2019

concept ACK

Copy link
Member

@Sjors Sjors left a comment

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

Schermafbeelding 2019-11-15 om 18 18 22

  • 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.

@@ -841,6 +841,23 @@ void CWallet::AddToSpends(const uint256& wtxid)
AddToSpends(txin.prevout, wtxid);
}

void CWallet::AddToAddressBloomFilter(const CWalletTx& wtx)
{
Copy link
Member

@Sjors Sjors Nov 15, 2019

Choose a reason for hiding this comment

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

Assert that m_address_bloom_filter has been built?

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@@ -1019,6 +1019,8 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
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);
Copy link
Member

@Sjors Sjors Nov 15, 2019

Choose a reason for hiding this comment

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

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?

if (!found_any) {
return false;
}

Copy link
Member

@Sjors Sjors Nov 15, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

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

@@ -30,6 +31,16 @@
namespace interfaces {
namespace {

std::set<CScript> AddressesToKeys(std::vector<std::string> addresses)
Copy link
Member

@Sjors Sjors Nov 15, 2019

Choose a reason for hiding this comment

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

AddressesToScriptPubKeys?

@@ -158,6 +160,8 @@ class WalletModel : public QObject

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

@Sjors Sjors Nov 15, 2019

Choose a reason for hiding this comment

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

checkAddressesForUsage?

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

@Sjors Sjors Nov 15, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

@fanquake fanquake Nov 15, 2019

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@DrahtBot DrahtBot commented Nov 21, 2019

Needs rebase

Copy link
Member

@promag promag left a comment

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

if (!found_any) {
return false;
}

Copy link
Member

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

@laanwj laanwj added this to the 0.20.0 milestone Mar 5, 2020
@laanwj laanwj removed this from the 0.20.0 milestone Mar 5, 2020
@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Mar 5, 2020

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

@hebasto
Copy link
Member

@hebasto hebasto commented May 4, 2020

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

#18192 and #18546 have been merged already :D

@rebroad
Copy link
Contributor

@rebroad rebroad commented Aug 20, 2020

@luke-jr re-open?

@hebasto
Copy link
Member

@hebasto hebasto commented Oct 3, 2021

Up for grabs?

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Oct 3, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet