don't touch addressbook when using secure payment-requests #3069

Diapolo commented at 12:31 PM on October 8, 2013: none

fixes #3006 by preventing addressbook changes when using secure payment-requests

in src/qt/sendcoinsdialog.cpp:None in d00a07381d outdated

 102 | @@ -103,7 +103,7 @@ void SendCoinsDialog::on_sendButton_clicked()
 103 |  
 104 |          QString recipientElement;
 105 |  
 106 | -        if (rcp.authenticatedMerchant.isEmpty())
 107 | +        if (!rcp.paymentRequest.IsInitialized())

laanwj commented at 6:59 PM on October 8, 2013:

I'm not sure that is correct; should this be done for every payment request, or just when the merchant is authenticated (ie "secure payment")? Same for the other similar changes below.

Diapolo commented at 7:52 PM on October 8, 2013:

I need to re-think that change... could well be you are correct. Edit: This one is wrong, I'll check one by one now :).

in src/qt/walletmodel.cpp:None in d00a07381d outdated

 282 | @@ -276,7 +283,6 @@ bool WalletModel::validateAddress(const QString &address)
 283 |                  wallet->SetAddressBook(dest, strLabel, ""); // "" means don't change purpose
 284 |              }
 285 |          }
 286 | -        emit coinsSent(wallet, rcp, transaction_array);

laanwj commented at 7:02 PM on October 8, 2013:

When you remove this, coinsSent is no longer emitted for non-payment requests, is that on purpose?

Diapolo commented at 7:47 PM on October 8, 2013:

Yes it is, as only payment-requests should trigger the PaymentServer::fetchPaymentACK slot AFAIK. @gavinandresen Can you verify this?

gavinandresen commented at 7:46 AM on October 9, 2013:

Seems to me we'll want the coinsSent signal sent any time coins are sent. And the fetchPaymentACK code should (does?) just ignore any non-payment-request-triggered signals (there isn't a paymentACK URL to fetch from in that case).

Diapolo commented at 10:28 AM on October 9, 2013:

As the coinsSent signal is currently only connected to fetchPaymentACK I thought it would be nice to only emit it for payment-requests. You are right that nothing bad is happening currently in fetchPaymentACK for non payment-requests, but who knows ^^.

laanwj commented at 11:14 AM on October 9, 2013:

IMO it's better to leave out the special case here and always emit if it doesn't hurt. The name of the signal implies nothing about payment request or not.

Diapolo commented at 12:02 PM on October 9, 2013:

Alright, should be fine now...

laanwj commented at 7:18 AM on October 10, 2013:

We could even do the 'add to address book' part of the function as another signal handler for coinsSent.

Diapolo commented at 9:55 AM on October 10, 2013:

I don't see the benefit of your suggestion (yet), we need to access wallet for processing the address stuff, so that code belongs to WalletModel IMO.

in src/qt/sendcoinsdialog.cpp:None in d00a07381d outdated

 351 | @@ -352,7 +352,7 @@ void SendCoinsDialog::pasteEntry(const SendCoinsRecipient &rv)
 352 |  bool SendCoinsDialog::handlePaymentRequest(const SendCoinsRecipient &rv)
 353 |  {
 354 |      QString strSendCoins = tr("Send Coins");
 355 | -    if (!rv.authenticatedMerchant.isEmpty()) {
 356 | +    if (rv.paymentRequest.IsInitialized()) {

Diapolo commented at 7:58 PM on October 8, 2013:

This one is correct, as secure and insecure payment-requests can expire. Edit: But it triggers a bug, which only blocks the first to-address in a multi-address insecure payment-request :-/.

Diapolo commented at 6:43 AM on October 9, 2013: none

@TheBlueMatt Can you take a look at the @BitcoinPullTester failure, seems unrelated to my pull.

laanwj commented at 6:52 AM on October 9, 2013: member

Thanks for cutting this down to one change

in src/qt/walletmodel.cpp:None in 6d8352889a outdated

 257 | @@ -258,6 +258,13 @@ bool WalletModel::validateAddress(const QString &address)
 258 |      // and emit coinsSent signal for each recipient
 259 |      foreach(const SendCoinsRecipient &rcp, transaction.getRecipients())
 260 |      {
 261 | +        // Don't touch the address book when we have a payment request

Diapolo commented at 7:09 AM on October 9, 2013:

@laanwj I'm asking myself, if I should change the behaviour once more and just prevent touching the address book for secure payment-requests (green sendcoins UI), but allow it for insecure payment-requests (normal sendcoins UI). coinsSent would still be emited for both types, as paymentACK messages are received for secure and insecure ones.

Diapolo commented at 7:02 AM on October 10, 2013: none

Is @BitcoinPullTester currently broken again? Would be nice if this (now small ^^) pull can come to an end.

gavinandresen commented at 11:42 PM on October 10, 2013: contributor

Pull-tester is broken, I'm poking at it to figure out why.

RE: coinsSent: I think @laanwj's intuition is right, adding to the address book should ideally be done as a coinsSent signal handler. I can imagine some future change where we want to add a coinsSent signal handler that does... something... that would like to know if the address we just sent to is in the address book. And the way you've rewritten the code, that potential future programmer will spend a bunch of time in a debugger trying to figure out why their code is broken (it'll be broken because their signal handler will be called BEFORE the address is added to the address book).

don't touch addressbook when using secure payment-requests

- fixes #3006 by preventing addressbook changes when using
  secure payment-requests

sq

48c011489b

Diapolo commented at 1:36 PM on October 11, 2013: none

I changed the code back to emit coinsSent() AFTER the addressbook code was done or skipped, so this can be merged soon.

As for the other suggestions to implement another coinsSent signal handler for adding to the addressbook, I'm still not sure how do do this, but can now be done in the future anyway :).

BitcoinPullTester commented at 2:09 PM on October 11, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/48c011489b047389e7c1c3aa8634de5133ec5b5c for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj commented at 1:43 PM on October 14, 2013: member

ACK

laanwj referenced this in commit a2bb571c4f on Oct 16, 2013

laanwj merged this on Oct 16, 2013

laanwj closed this on Oct 16, 2013

Bushstar referenced this in commit 0e94e97cc5 on Apr 8, 2020

DrahtBot locked this on Sep 8, 2021