GUI: Display warning when attempting address reuse #9722

CryptAxe commented at 6:48 AM on February 8, 2017: contributor

Added address reuse key 'au' to destdata

Added informative QMessageBox popup warning when user attempts address reuse

GUI: Display warning when attempting address reuse

Added address reuse key 'au' to destdata

Added informative QMessageBox popup warning when user attempts address reuse

9113784bd6

CryptAxe commented at 6:48 AM on February 8, 2017: contributor

fanquake added the label GUI on Feb 8, 2017

in src/qt/sendcoinsdialog.cpp:None in 9113784bd6 outdated

 262 | @@ -263,6 +263,27 @@ void SendCoinsDialog::on_sendButton_clicked()
 263 |      QStringList formatted;
 264 |      Q_FOREACH(const SendCoinsRecipient &rcp, currentTransaction.getRecipients())
 265 |      {
 266 | +        // If user is attempting address reuse, show an informative warning
 267 | +        if (model->isUsed(rcp.address.toStdString()))
 268 | +        {
 269 | +            QString reuseWarningText = tr(
 270 | +                        "You have sent to the Bitcoin address %1 already!\n\n"
 271 | +                        "Sending to the same address twice is technically not supported and "

jonasschnelli commented at 4:17 PM on February 8, 2017:

I think the part technically not supported is not correct.

TheBlueMatt commented at 4:48 PM on February 8, 2017:

Indeed, it is supported fine, but it is highly discouraged.

luke-jr commented at 5:35 PM on February 8, 2017:

It would not be a bug to stop scanning for transactions paying to an address once it has been used. It "works" only by accident.

in src/qt/sendcoinsdialog.cpp:None in 9113784bd6 outdated

 270 | +                        "You have sent to the Bitcoin address %1 already!\n\n"
 271 | +                        "Sending to the same address twice is technically not supported and "
 272 | +                        "will reduce your own security as well as the privacy of all Bitcoin users."
 273 | +                    ).arg(rcp.address) + "\n";
 274 | +
 275 | +            QMessageBox reuseMessage("Warning: address reuse!",

luke-jr commented at 4:18 PM on February 8, 2017:

I can't find this constructor in the Qt docs...

in src/qt/sendcoinsdialog.cpp:None in 9113784bd6 outdated

 273 | +                    ).arg(rcp.address) + "\n";
 274 | +
 275 | +            QMessageBox reuseMessage("Warning: address reuse!",
 276 | +                                     reuseWarningText,
 277 | +                                     QMessageBox::Warning,
 278 | +                                     QMessageBox::Yes | QMessageBox::Escape,

luke-jr commented at 4:19 PM on February 8, 2017:

Yes | Escape doesn't look like a normal combination?

in src/qt/walletmodel.cpp:None in 9113784bd6 outdated

 354 | @@ -355,6 +355,9 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran
 355 |  
 356 |                  std::map<CTxDestination, CAddressBookData>::iterator mi = wallet->mapAddressBook.find(dest);
 357 |  
 358 | +                std::string key = "au"; // "au" prefix = "address used" in destdata

luke-jr commented at 4:19 PM on February 8, 2017:

"used" seems more straightforward, but more importantly, this should be moved to a static const in a src/wallet/ header.

in src/qt/walletmodel.cpp:None in 9113784bd6 outdated

 593 | @@ -591,6 +594,24 @@ bool WalletModel::isSpent(const COutPoint& outpoint) const
 594 |      return wallet->IsSpent(outpoint.hash, outpoint.n);
 595 |  }
 596 |  
 597 | +bool WalletModel::isUsed(const std::string& address) const
 598 | +{
 599 | +    LOCK2(cs_main, wallet->cs_wallet);
 600 | +    for (const std::pair<CTxDestination, CAddressBookData>& item : wallet->mapAddressBook)

jonasschnelli commented at 4:20 PM on February 8, 2017:

Instead of looping, would a std::map::find be possible (std::map<CTxDestination, CAddressBookData>::iterator mi = wallet->mapAddressBook.find(dest);?

jonasschnelli commented at 4:25 PM on February 8, 2017: contributor

General concept ACK. Am I correct that this patch does not warn for "send-agains" to addresses sent before this patch was active?

MarcoFalke commented at 6:01 PM on February 8, 2017: member

Agree that the "technically not supported" is confusing to users. We should just mention "is (highly) discouraged".

Update isUsed, destdata used key, warning text 8f01b1f8f6

CryptAxe commented at 6:01 PM on February 9, 2017: contributor

Making another update, but in the meantime, I changed the warning message because if there is confusion even here then it needs to be more clear. I also updated the message box code so that it isn't using an obsolete constructor and removed the extra escape.

This warning will only be displayed for addresses which are used after the software has been updated. It would be possible though for someone to add an update later that would scan through the wallet transactions and mark the destinations of those as used. Also the wallet already had a function to read the destdata by using std::map find, but it wasn't being used anywhere as far as I can tell. I replaced the loop with that.

Add destdata after updating address book 18c1325dcd

CryptAxe commented at 6:52 PM on February 9, 2017: contributor

I am going to go through some tests now, but everything is ready to be reviewed again.

CryptAxe commented at 12:09 AM on February 10, 2017: contributor

Alright I'm happy with the code. Anyone have a problem with the updated warning message?

jonasschnelli commented at 8:32 AM on February 10, 2017: contributor

Code looks good to me. I dislike the "1", but I guess this is just how we have to do it with the flexible per wtx map.

Though I'm unsure about the update path of this feature. If we only mark new destinations, this may result in a false awareness of address reuse because some addresses you may reuse (the old ones before the upgrade) won't show the warning and users may think it's okay to reuse them.

A solution would be, to either A) loop through the transactions that where made before the feature was active or to B) migrate at first start with the new feature (loop through all wtxs, set the used dest data.

I'd like to hear more opinions on that.

luke-jr commented at 9:07 AM on February 10, 2017: member

I'd prefer B, but it would only allow one reuse before it'd get flagged, right?

CryptAxe commented at 10:59 PM on February 10, 2017: contributor

Yeah the way it works right now, and with B, the code will only allow tagging on first use. I'm fine switching to B or whatever makes it more clear but 1 = true made sense to me.

On Feb 10, 2017 1:07 AM, "Luke Dashjr" notifications@github.com wrote:

I'd prefer B, but it would only allow one reuse before it'd get flagged, right?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9722#issuecomment-278892806, or mute the thread https://github.com/notifications/unsubscribe-auth/AHu1NptENp_ak43bJO0zlnipHctySWBlks5rbCjVgaJpZM4L6ce2 .

jonasschnelli commented at 1:46 PM on March 17, 2017: contributor

Any intentions to work this towards the solution "B" (migrate at first start with the new feature (loop through all wtxs, set the used dest data.)?

CryptAxe commented at 4:33 PM on March 19, 2017: contributor

Yes, I was just waiting for some feedback on that before I went ahead and added the code. Since I think it's a good idea and nobody seems to take issue I'll finish this up next time I have a break.

On Mar 17, 2017 6:47 AM, "Jonas Schnelli" notifications@github.com wrote:

Any intentions to work this towards the solution "B" (migrate at first start with the new feature (loop through all wtxs, set the used dest data.)?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9722#issuecomment-287357677, or mute the thread https://github.com/notifications/unsubscribe-auth/AHu1Nm20_a6Eq_-MxgEfKp5zebvJ9MU2ks5rmo7ggaJpZM4L6ce2 .

jonasschnelli commented at 6:40 AM on June 29, 2017: contributor

Closing due to inactivity (feel free to reopen once someone takle option B or there is consensus about doing solution A).

jonasschnelli closed this on Jun 29, 2017

meshcollider added the label Up for grabs on Mar 7, 2018

MarcoFalke locked this on Sep 8, 2021

fanquake removed the label Up for grabs on Aug 4, 2022