display txfee in first sendCoinsDialog message box (issue #1715)

jonasschnelli commented at 1:36 pm on May 13, 2013: contributor

People companying about txfees are not shown in the first question (#1715, #1714) if funds should be sent. This pull will calculate and include the txfee in the sendcoins question messagebox.

in src/qt/sendcoinsdialog.cpp: in 095a25539e outdated

 97@@ -98,8 +98,11 @@ void SendCoinsDialog::on_sendButton_clicked()
 98 
 99     fNewRecipientAllowed = false;
100 
101+    // calculate fee for displaying
102+    qint64 txFeeRequired = model->calculateFee(recipients);
103+
104     QMessageBox::StandardButton retval = QMessageBox::question(this, tr("Confirm send coins"),
105-                          tr("Are you sure you want to send %1?").arg(formatted.join(tr(" and "))),
106+                          tr("Are you sure you want to send %1 and %2 to transaction fees?").arg(formatted.join(tr(" and ")), BitcoinUnits::formatWithUnit(BitcoinUnits::BTC, txFeeRequired)),

Diapolo commented at 1:55 pm on May 13, 2013:

Perhaps we should also offer a total here? Are you sure you want to send %1 (%2 debit including %3 transaction fee)?

And while you are on, perhaps replace the BitcoinUnits::BTC via OptionsDialog::getDisplayUnit() to show the unit chosen in the settings?

jonasschnelli commented at 1:58 pm on May 13, 2013:

Okay. I’ve also thought about a total. But the alert screen as a text-only display will might not fit for a proper display. We could also think to make a extra confirmation view.

getDisplayUnit: will do that.

BitcoinPullTester commented at 2:12 pm on May 13, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/095a25539e9d243160a1e5ee5dec79d789ea3a27 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.

in src/qt/walletmodel.cpp: in 095a25539e outdated

145+
146+    CWalletTx wtx;
147+    CReserveKey keyChange(wallet);
148+    int64 nFeeRequired = 0;
149+    std::string strFailReason;
150+    bool fCreated = wallet->CreateTransaction(vecSend, wtx, keyChange, nFeeRequired, strFailReason);

laanwj commented at 8:00 pm on May 19, 2013:

As CreateTransaction is called twice, once to compute the fee, once to send the transaction, I wonder if the randomization inside CreateTransaction (the knapsack algorithm) won’t give trouble here. It may pick a different set of inputs the second time and thus come up with a slightly different suggested fee.

I’m not sure how much of a problem this is in practice.

jonasschnelli commented at 6:57 am on May 20, 2013:

Yes. I’m also not sure. But i thought it must act the same as you would do “send coins” -> “cancel” -> “send coins”. The only difference is maybe that we won’t go back to the main-runloop between “cancel” and the 2nd “send coins”.

laanwj commented at 6:49 am on May 21, 2013:

This is exactly the same as clicking cancel and send coins again. Usually this gets the same fee but it’s not guearanteed 100%.

This also builds the transaction twice, which for large wallets may cause some delay.

The “perfect” way would be to build the transaction (but not sign it), determine the fee and only then show the confirmation dialog. If confirmed, send exactly this transaction. But this would require some back/forth signalling in WalletModel:sendCoins.

jonasschnelli commented at 6:59 am on May 21, 2013:

@laanwj let me analyze the code… i have some fears changing to much in core files. :)

laanwj commented at 7:10 am on May 21, 2013:

@jonasschnelli You don’t need to change anything in core files. src/qt/WalletModel.cpp at most

Another thing to consider: how does this interact with encrypted wallets. Will it ask the passphrase an extra time?

jonasschnelli commented at 7:44 am on May 21, 2013:

Yes. Good question. Let me also check this.

jonasschnelli commented at 2:55 pm on May 24, 2013: contributor

@laanwj can you review again? Now the transaction gets prepared and the instance will be used to commit the transaction.

in src/qt/walletmodel.h: in 150111c53f outdated

69@@ -69,15 +70,20 @@ class WalletModel : public QObject
70     {
71         SendCoinsReturn(StatusCode status,
72                          qint64 fee=0,
73-                         QString hex=QString()):
74-            status(status), fee(fee), hex(hex) {}
75+                         QString hex=QString(),
76+                         CReserveKey keyChange=NULL):
77+            status(status), fee(fee), hex(hex), keyChange(keyChange) {}

jonasschnelli commented at 2:55 pm on May 24, 2013:

i think we need to keep the possible keychange. don’t we?

Diapolo commented at 9:45 pm on May 24, 2013:

Also can you format this a little nicer than it was before, I think it’s hard to read.

BitcoinPullTester commented at 3:43 pm on May 24, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/150111c53fc08db0ecc2fb894f80e17d80e37c39 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.

in src/qt/sendcoinsdialog.cpp: in 150111c53f outdated

152+        return;
153+    }
154+
155+    //TODO before pull merge: nice string, i18n
156+    QMessageBox::StandardButton retval = QMessageBox::question(this, tr("Confirm send coins"),
157+                          tr("Are you sure you want to send %1 and %2 to transaction fees?").arg(formatted.join(tr(" and ")), BitcoinUnits::formatWithUnit(BitcoinUnits::BTC, prepareStatus.fee)),

Diapolo commented at 9:43 pm on May 24, 2013:

I suggest: Are you sure you want to send %1 (this transaction requires a fee of %2)?

in src/qt/walletmodel.cpp: in 150111c53f outdated

122@@ -124,11 +123,13 @@ bool WalletModel::validateAddress(const QString &address)
123     return addressParsed.IsValid();
124 }
125 
126-WalletModel::SendCoinsReturn WalletModel::sendCoins(const QList<SendCoinsRecipient> &recipients)
127+WalletModel::SendCoinsReturn WalletModel::prepareTransaction(const QList<SendCoinsRecipient> &recipients, CWalletTx &preparedTransaction)
128 {
129+

Diapolo commented at 9:43 pm on May 24, 2013:

Why a new-line at the start of the function ;)?

in src/qt/walletmodel.h: in 508caa7bc2 outdated

67@@ -67,17 +68,19 @@ class WalletModel : public QObject
68     // Return status record for SendCoins, contains error id + information
69     struct SendCoinsReturn
70     {
71-        SendCoinsReturn(StatusCode status,
72-                         qint64 fee=0,
73-                         QString hex=QString()):
74-            status(status), fee(fee), hex(hex) {}
75+        SendCoinsReturn(StatusCode status, qint64 fee=0, QString hex=QString(), CReserveKey keyChange=NULL):

jonasschnelli commented at 10:22 am on May 26, 2013:

For me this is now better readable.

Diapolo commented at 11:28 am on May 26, 2013:

Yes it is :), thanks.

in src/qt/sendcoinsdialog.cpp: in 508caa7bc2 outdated

152+        return;
153+    }
154+
155+    //TODO before pull merge: nice string, i18n
156+    QMessageBox::StandardButton retval = QMessageBox::question(this, tr("Confirm send coins"),
157+                          tr("Are you sure you want to send %1 (this transaction requires a fee of %2)?").arg(formatted.join(tr(" and ")), BitcoinUnits::formatWithUnit(BitcoinUnits::BTC, prepareStatus.fee)),

jonasschnelli commented at 10:24 am on May 26, 2013:

This text needs a better formatting later. Like the case when txfee is 0 (then the text must not refer to a tx fee). But i would recommend to fix the code issues, get reviews, get some ACK’s (ok no okays) and move on with the text-display-layer (to save time if people don’t ACK the change).

BitcoinPullTester commented at 11:16 am on May 26, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/508caa7bc26a1b06fc193892098dbc58f183df15 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 10:56 pm on May 31, 2013: member

Looks like the correct way forward!

I wonder about one thing, though: As the wallet lock is released in the time between the creation of the prepared transaction and committing it (which in principle is a good thing, otherwise everything using the wallet will hang), it seems possible for another spend in the background (for example, through RPC) to spend the inputs used by the transaction. I’m not sure what would happen in this case. An error would be fine, but crashing out or creating an invalid/double spend transaction is not. We’ll have to check and test this case carefully.

jonasschnelli commented at 7:06 am on June 3, 2013: contributor

@laanwj good catch. The QMessageBox can stay open till infinity (it needs user action). So,… the missing wallet locking can be quite a problem. I do test now what happens when i keep open the message box and trigger another payment with rpc.

jonasschnelli commented at 8:15 am on June 3, 2013: contributor

Did some testing. When spending the coins (whole wallet balance) in background using RPC sendtoaddress during the open QMessageBox (asking for confirmation), i get a correct error when trying to spend the prepared transaction (WalletModel:sendCoins) TransactionCommitFailed. Error handling is fine in my eye.

I focus now on the string generation for informing the user about the fee.

jonasschnelli commented at 8:45 am on June 3, 2013: contributor

squashed and ready for ACK’s :)

in src/qt/sendcoinsdialog.cpp: in 0e2c7f1923 outdated

154+
155+    QString questionString = tr("Are you sure you want to send %1?");
156+    if(prepareStatus.fee > 0)
157+    {
158+        // change the string if there is a fee required
159+        questionString = tr("Are you sure you want to send %1<br /><hr />\nthis transaction requires a fee of <span style='color:#aa0000;'>%2</span>?");

Diapolo commented at 9:20 am on June 3, 2013:

I would rather use a fixed HTML color value like just red (symbolic name AFAIK) to be sure we don’t mess with the users OS theme.

jonasschnelli commented at 9:49 am on June 3, 2013:

But #aa0000 is not red. :) Red is somehow to strong and looks more like an error. #a00 is a darker red. Maybe there’s a equivalent symbolic color. let me check…

sipa commented at 9:50 am on June 3, 2013:

I don’t see how using a color name or its hex representation would in any way impact interaction with OS themes; they’re both a static color.

jonasschnelli commented at 10:09 am on June 3, 2013:

Yes. Agree. In theory a color name like “darkRed” could be mapped to a different color hex code. But this would be against W3C. #aa00000 is fine for me and as far as i see there is no theme like code setup where i could place/get existing colors.

Diapolo commented at 9:25 am on June 3, 2013: none

I think the dialog boxes look a little disturbed and should be perhaps a little more tidy. My idea would be to design it as a list to be more easily readable and the fee display as an addittional sentence at the end.

BitcoinPullTester commented at 9:28 am on June 3, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/7b5936bce40141a0e838294bce37a0e1d544912e 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.

BitcoinPullTester commented at 10:33 am on June 3, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/0e2c7f1923754159fa0f6f8d7f6430b0ef00f906 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.

rebroad commented at 11:31 am on June 3, 2013: contributor

Please bear in mind people who use high contrast themes who are visually impaired. The OS needs to be able to use the correct colour in this situation.

BitcoinPullTester commented at 11:40 am on June 3, 2013: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/149973ef0214d9aa2d3d83838eb2bdba8942b5ac for test log.

This pull does not merge cleanly onto current master 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.

Diapolo commented at 11:54 am on June 3, 2013: none

Looks very nice :), if you only could change this transaction requires a fee of XYZ BTC into This transaction requires a fee of XYZ BTC. (uppercase start and . to make it a sentence) ^^.

I hope people don’t mess with the HTML stuff when translating on Transifex…

jonasschnelli commented at 12:11 pm on June 3, 2013: contributor

Will fix the typo. @Diapolo: what do you think if we take the html stuff into separates stings. Goal: exclude html from tr stuff. It might be then a bit a string gluing battle. But prevents us from messed up transiflex sources.

Diapolo commented at 12:17 pm on June 3, 2013: none

Let’s see what @laanwj thinks, but if possible I would prevent big HTML stuff in translatable strings if possible, like you suggest.

laanwj commented at 12:31 pm on June 3, 2013: member

I agree, better to let people translate only the messages and keep the surrounding html fixed.

jonasschnelli commented at 12:34 pm on June 3, 2013: contributor

okay. Let me slice the strings…

jonasschnelli commented at 1:16 pm on June 3, 2013: contributor

did some string slicing. But i’m not sure if it’s still readable like this and might also leads to transiflex problems. I’d prefer the previous version…

laanwj commented at 1:32 pm on June 3, 2013: member

IMO this is not much better, instead of html the translation messages now contain lots of %%%. If you want to slice the strings, why not use concatenation (+) instead of all the interpolation? Or maybe we’ll just have to accept a little html in the translation strings if it’s the least worst option…

in src/qt/sendcoinsdialog.cpp: in 6a529b9312 outdated

111+            formatted.append(tr("<b>%1</b> to %2 %4(%3)%5%6").arg(BitcoinUnits::formatWithUnit(BitcoinUnits::BTC, rcp.amount), rcp.label.toHtmlEscaped(), rcp.address, btcAddressSpanO, formattedSpanClose, formattedLineBreak));
112 #endif
113+        }
114+        else
115+        {
116+            formatted.append(tr("<b>%1</b> to %2%3").arg(BitcoinUnits::formatWithUnit(BitcoinUnits::BTC, rcp.amount), rcp.address, formattedLineBreak));

Diapolo commented at 1:38 pm on June 3, 2013:

I thought of it the other way around.

QString strTo = tr(“to”); and so on and use this in the .append() function and don’t use tr() there.

jonasschnelli commented at 6:54 am on June 4, 2013:

Hmm.. i was also thinking that way. But as far as i remember other i18n projects, sometimes a simple “to” can not just be exchanged because other languages need changes at other places (like swapping words, etc.). Do you know what i mean? Who is now for which solution?

original solution with html in tr-strings
current solution with %X to avoid html in tr-strings
solution from @Diapolo where we go to other way around (just tr some basic words like “to”)

Diapolo commented at 9:54 am on June 4, 2013:

I had a similar problem while doing the german translations one time, with "Last received block was generated %1 ago., in combination with the plural of %n day(s), which was solveable via chosing a similar meaning but not doing an exact translation. I can’t really tell if this will work here, but I vote number 3.

Also it looks like we have 2 complete sentences and only the to is a little special.

BitcoinPullTester commented at 2:57 pm on June 3, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/6a529b9312b981df9852d24ea5907a3187e3244f 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.

wtogami commented at 7:22 pm on June 5, 2013: contributor

Could you please squash the commits, rebasing on top of master?

in src/qt/sendcoinsdialog.cpp: in 6a529b9312 outdated

174+
175+    QString questionString = tr("Are you sure you want to send %1?");
176+    if(prepareStatus.fee > 0)
177+    {
178+        // change the string if there is a fee required
179+        questionString = tr("Are you sure you want to send?%7%7%1%6This transaction requires a fee of %4%2%5%6Total Amount %3");

PRabahy commented at 8:53 pm on June 6, 2013:

I would reword “This transaction requires …” to “123 BTC required as transaction fee”. That follows the same flow of the list of outputs (amount –> recipient).

jonasschnelli commented at 10:39 am on June 7, 2013: contributor

rebased, squashed, updated.

Added new string recommended by @PRab
overhauled string gluing like @Diapolo recommended.

Now it looks like:

single no fee (std case): bildschirmfoto 2013-06-07 um 12 31 40

single with fee: bildschirmfoto 2013-06-07 um 12 32 13

multiple recp. no fee: bildschirmfoto 2013-06-07 um 12 31 48

multiple recp. with fee: bildschirmfoto 2013-06-07 um 12 32 22

Diapolo commented at 10:47 am on June 7, 2013: none

Looks really nice now, just add a little ! after fee to make it a sentence and you have my design ACK (I’m not deep enough into Tx-code to ACK that part)!

jonasschnelli commented at 10:53 am on June 7, 2013: contributor

@Diapolo agree for the “!”. Added, commited, pushed.

Diapolo commented at 10:55 am on June 7, 2013: none

ACK

sipa commented at 4:30 pm on June 7, 2013: member

Looks good. Haven’t checked the code or tested.

One question: what does a voluntary fee look like?

jonasschnelli commented at 6:04 pm on June 7, 2013: contributor

@sipa: It does look the same (check screenshot with voluntary 0.1BTC fee): bildschirmfoto 2013-06-07 um 20 03 04

Maybe the sentence need to be changed. What do you think?

PRabahy commented at 6:07 pm on June 7, 2013: contributor

Just changing “required” to “added” makes it sound good to me.

jonasschnelli commented at 6:07 pm on June 7, 2013: contributor

@PRab okay. sounds good. Let me check how i can detect if vol. or req. fee.

wtogami commented at 6:30 pm on June 7, 2013: contributor

+1 to “required” or “added” depending on if it is required or voluntary.

cozz commented at 8:01 pm on June 7, 2013: contributor

I am trying to merge this in coin control, there seems to be a serious bug in how you handle CReserveKey keyChange. If you send a transaction which creates change, the wallet gets permanently locked. Simply send 2 transactions in a row, to test this, but you must have change. You can also send a transaction and then try to create a new address in the receive tab -> runtime error. bitcoin-qt: /usr/include/boost/thread/pthread/recursive_mutex.hpp:107: void boost::recursive_mutex::lock(): Assertion `!pthread_mutex_lock(&m)’ failed.

Would you mind to take a look at this? Besides that, good job man, looking great.

laanwj commented at 11:39 pm on June 7, 2013: member

I like this.

One nit though: I think highlighting the fee in red as well as adding an exclamation mark is over the top. That’s only warranted if the fee is high compared the the amount to be sent. Otherwise, just use a period, no need to scream.

rebroad commented at 2:48 am on June 8, 2013: contributor

I agree. It also potentially discriminates against visually impaired people if it cannot be overridden by the operating system’s theme. On Jun 8, 2013 6:40 AM, “Wladimir J. van der Laan” notifications@github.com wrote:

I like this.

One nit though: I think highlighting the fee in red as well as adding an exclamation mark is over the top. That’s only warranted if the fee is high compared the the amount to be sent.

— Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/2651#issuecomment-19138661 .

laanwj commented at 6:52 am on June 8, 2013: member

That’s a subject for another pull request. We have quite a few hardcoded colors at the moment. It would be nice to be able to override them with a theme for example with a css file. But to keep the scope of this pull request limited, red will do for now.

wtogami commented at 6:59 am on June 8, 2013: contributor

Although please look into the issue identified by cozz.

jonasschnelli commented at 11:56 am on June 9, 2013: contributor

@cozz Thanks for the report. The problem might be, that the return of the new prepareTransaction method is within the LOCK block. I will clean this up and move the return out of the LOCK block.

jonasschnelli commented at 12:02 pm on June 9, 2013: contributor

Updated. Exclamation mark is removed. Voluntary gets detected. LOCK problematic “could” be fixed. @cozz can you retest? Can somebody have a closer look into the prepareTransaction and sendCoins methods? Especially the locking, etc.?

cozz commented at 0:20 am on June 11, 2013: contributor

I still have the lock-problem. I have tested on linux and windows. I think the problem is that CReserveKey keyChange(wallet); is defined in the function. I can fix the problem when I define CReserveKey as a pointer, but then it may be memory leaked I guess, if not deletes added. So you could do this or you could define CReserveKey in sendcoinsdialog and pass it as a parameter. But there you cant call CReserveKey keyChange(wallet); because wallet does not exist. So you could define a pointer here and then call “new CReserveKey” in prepareTransaction. And then cleanup CReserveKey in sendcoinsdialog after it is not needed anymore.

Maybe its just me, but whenever I try to send 2 transactions with change in a row it crashes.

cozz commented at 2:42 am on June 11, 2013: contributor

To determine whether a fee is added or required in the case where you have set a voluntary fee is not possible, createTransaction does not tell you that. It tells you how much fee has been added, but not why. So to provide really accurate “added/required” message, we would have to add a boolean return fForcedFee to createTransaction. Or we could simply write “added” in all cases to make it simple here.

sipa commented at 8:48 am on June 11, 2013: member

I’m fine with just saying “added” here.

About CReserveKey: would it help to remove its dependency on CWallet, and have the CWallet passed explicitly when needed (when getting a pubkey, or when keeping/returning)?

jonasschnelli commented at 8:51 am on June 11, 2013: contributor

@cozz: I do compare the requiresFee from createTransaction with the possible voluntary fee from the settings. If the value is the same, it must be a voluntary fee. Or not?

laanwj commented at 9:32 am on June 11, 2013: member

It’s a possible check - but just that the value matches does not provide certainty.

For now I strongly prefer always using “added” to prevent unneccesary complexity.

Such checking could always be added in a later pull and reviewed and tested separately.

jonasschnelli commented at 10:17 am on June 11, 2013: contributor

@laanwj We could change to “added” in all cases. But my understanding is that the user should be happy when he set (example) a 0.001BTC vol. fee in settings and then read “added” (even when the tx would require 0.001BTC as fee). When the tx fee would be higher, it would then be “required”.

laanwj commented at 1:12 pm on June 11, 2013: member

Yes – but mind that the fee set in configuration is per KB, so at least you’d have to do some computations as well. I’m not comfortable with the UI making guesses based on the amount. That logic doesn’t belong there.

On the other hand I’m fine with a solution in which CreateTransaction returns a flag whether the fee was voluntary or not (as @cozz suggest), but it should not be in this pull request.

Let’s just try to get this merged with the current functionality, as it’s very nice to have.

jonasschnelli commented at 1:17 pm on June 11, 2013: contributor

Okay. Let’s move the added/required distinction to a possible upcoming pull. I will just change it to “added”. I try now to fix the CReserveKey thing…

BitcoinPullTester commented at 4:18 am on June 13, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/d01361332e958e880805c2a604b651248dc5eac8 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.

jonasschnelli commented at 7:57 am on June 13, 2013: contributor

@Cozz, @sipa: i’ve updated the CReserveKey handling. The CReserveKey will now no longer be passed around. I decided to keep it as a instance var of walletmodel (including a new cleanup transaction method). If you could review it.. would be appreciated. @Cozz: if you find time: Could you do a retest (it sounds that you have a proper test-setup).

BitcoinPullTester commented at 8:45 am on June 13, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/d2b910cc472e43dfb23fe24566c1cc020acdbd2b 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 9:36 am on June 13, 2013: member

I don’t like the temporary state in WalletModel. This is a similar concern as I have with the coin control patch (which, last time I checked, added output selection state). Such state causes possible concurrency issues, what if a new transaction is created before the old one is cleaned up. At least a leak.

I’d prefer the transaction object returned from preparetransaction to be self-contained,and not leak its implementation to clients. Maybe create a WalletModelTransaction class?

jonasschnelli commented at 9:50 am on June 13, 2013: contributor

@laanwj yes. These state’s are really not easy to handle. They need a very clear work to avoid leaks, etc. The problem is, that i’m going deeper than i should. :) I do not see the root cause of the CReserveKey problem as i also do not have a test setup to debug it. The WalletModelTransaction class sounds after a good idea. For now it looks a bit overheaded but it’s extendable and it’s a good base for other “changes” (CoinControl) in that area.

laanwj commented at 11:03 am on June 13, 2013: member

Yeah, point on overdesign taken. My initial design idea was to keep the UI and core as separare as possible, so all communication and usage of core data structures happens through models (signmessafe was the first to break this). It thus makes me a bit sad to see more and more includes of core headers leaking into non-model UI files, and core data structures there being used directly.

In any case, good work here!

cozz commented at 6:37 pm on June 13, 2013: contributor

It works for me now.

jonasschnelli commented at 7:28 am on June 17, 2013: contributor

Update code, moved transaction-stuff to a own class. @Cozz: could you please re-test one more time? @laanwj: code-review?

BitcoinPullTester commented at 0:56 am on June 19, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/aff22cd76ebf4fbe7a11875d17e6b932b2b1d8e8 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.

cozz commented at 11:50 pm on June 29, 2013: contributor

tested again, seems to work, just some minor things I have noticed:

required/added still there
remove file src/walletmodeltransaction.h, it doesnt belong there, accident I guess. model and qt code should not be in src-folder anyway
you forgot transaction.setTransactionFee(nFeeRequired); in walletmodel.cpp line 196 before the return same thing line 168 transaction.setTransactionFee(nTransactionFee); before the return
uint64 totalTransactionAmount in sendcoinsdialog.cpp, I would remove this and make a get-method in class WalletModelTransaction instead, thats what the class is there for

jonasschnelli commented at 5:54 pm on July 1, 2013: contributor

Thanks @cozz will have a look at it soon/tmr

jonasschnelli commented at 6:52 am on July 10, 2013: contributor

cleaned up and fixed issues reported by @cozz. Ready for some final ACKs.

BitcoinPullTester commented at 7:23 am on July 10, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a027df6fa8e6ebf1cb221e3ef93977c85efddb5a 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.

in src/qt/walletmodel.cpp: in a027df6fa8 outdated

136     if(recipients.empty())
137     {
138         return OK;
139     }
140 
141+    transaction.setTransactionFee(nFeeRequired);

cozz commented at 5:32 pm on July 10, 2013:

nFeeRequired must be replaced with nTransactionFee here, I would remove this line and put it below before the return: if((total + nTransactionFee) > getBalance()) { transaction.setTransactionFee(nTransactionFee); return SendCoinsReturn(AmountWithFeeExceedsBalance); }

jonasschnelli commented at 7:19 am on July 11, 2013:

Okay. Your right. Changed.

in src/qt/sendcoinsdialog.cpp: in a027df6fa8 outdated

170+    default:
171+        break;
172+    }
173+
174+    if(prepareStatus.status != WalletModel::OK) {
175+        return;

cozz commented at 5:34 pm on July 10, 2013:

I think there is a fNewRecipientAllowed = true; missing before this return

jonasschnelli commented at 7:19 am on July 11, 2013:

Good catch! Thanks. changed.

in src/qt/walletmodel.cpp: in aedfd8dd48 outdated

164@@ -163,12 +165,15 @@ bool WalletModel::validateAddress(const QString &address)
165 
166     if((total + nTransactionFee) > getBalance())
167     {
168-        return SendCoinsReturn(AmountWithFeeExceedsBalance, nTransactionFee);
169+        transaction.setTransactionFee(nFeeRequired);

cozz commented at 12:48 pm on July 11, 2013:

you still say nFeeRequired, you must replace this with nTransactionFee: transaction.setTransactionFee(nTransactionFee);

laanwj commented at 8:28 am on August 10, 2013:

Agreed, the “amount plus fee exceeds balance” check fails, yet we still set the transaction’s fee to nFeeRequired which is 0 at this point.

BitcoinPullTester commented at 6:07 am on July 20, 2013: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/aedfd8dd4892cb79b33a13c9c9db1c188be6aca9 for test log.

This pull does not merge cleanly onto current master 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.

Diapolo commented at 9:31 am on August 5, 2013: none

@jonasschnelli What is the current state for this pull?

jonasschnelli commented at 10:14 am on August 5, 2013: contributor

Sorry. Didn’t realize that my turn is missing… will check it and give response soon. @Diapolo thanks for the reminder!

Diapolo commented at 11:51 am on August 5, 2013: none

No problem, I was just wondering, why that one isn’t merged yet and saw there is an open feedback from @cozz and you surely need to rebase.

laanwj commented at 8:28 am on August 10, 2013: member

Yes, let’s try to get this merged.

jonasschnelli commented at 12:53 pm on August 10, 2013: contributor

Okay. Fixed the “amount plus fee exceeds balance” issue while setting the transaction fee in the transaction model to always 0.

Some final ACK’s?

BitcoinPullTester commented at 1:20 pm on August 10, 2013: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/bb8da7fd3ce971714ec9af34010d53b51a4d45e2 for test log.

This pull does not merge cleanly onto current master 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.

jonasschnelli commented at 1:35 pm on August 10, 2013: contributor

hmm… need to rebase.

BitcoinPullTester commented at 2:37 pm on August 10, 2013: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/237edd014bf4017fdcb756f6f8c8001a573531c4 for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in contrib/test-scripts)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

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.

in src/qt/sendcoinsdialog.cpp: in 237edd014b outdated

60@@ -60,6 +61,11 @@ void SendCoinsDialog::setModel(WalletModel *model)
61     delete ui;
62 }
63 
64+void SendCoinsDialog::setOptionsModel(OptionsModel *optionsModel)
65+{
66+    this->optionsModel = optionsModel;

Diapolo commented at 3:11 pm on August 10, 2013:

You added this class attribute, but it seems it’s not used. IMHO you could replace all model->getOptionsModel() calls with optionsModel.

jonasschnelli commented at 8:20 pm on August 11, 2013:

Right. The optionsModal was for the (now removed) check if the fee is voluntary. Will remove this soon.

in src/qt/walletmodel.cpp: in 237edd014b outdated

182@@ -178,35 +183,50 @@ bool WalletModel::validateAddress(const QString &address)
183             vecSend.push_back(make_pair(scriptPubKey, rcp.amount));
184         }
185 
186-        CWalletTx wtx;
187-        CReserveKey keyChange(wallet);
188-        int64 nFeeRequired = 0;
189+

Diapolo commented at 3:16 pm on August 10, 2013:

unneeded new-line

in src/qt/walletmodeltransaction.h: in 237edd014b outdated

15+    QList<SendCoinsRecipient> getRecipients();
16+
17+    CWalletTx *getTransaction();
18+
19+    void setTransactionFee(int64 newFee);
20+    int64 getTransactionFee();

Diapolo commented at 3:19 pm on August 10, 2013:

Should we use qint64 and quint64 below as this is Qt code (same above int64 newFee)?

jonasschnelli commented at 6:18 am on August 12, 2013: contributor

@Diapolo did cleanup the code (fixed your pointing) and also got rid of the setOptionsModel in sendCoinsDialog.

in src/qt/walletmodel.cpp: in b24cb4ceea outdated

127+WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransaction &transaction)
128 {
129     qint64 total = 0;
130     QSet<QString> setAddress;
131     QString hex;
132+    int64 nFeeRequired = 0;

Diapolo commented at 6:40 am on August 12, 2013:

int64 -> qint64

jonasschnelli commented at 6:42 am on August 12, 2013:

fixed.

in src/qt/walletmodeltransaction.cpp: in b24cb4ceea outdated

28+quint64 WalletModelTransaction::getTransactionFee()
29+{
30+    return fee;
31+}
32+
33+void WalletModelTransaction::setTransactionFee(int64 newFee)

Diapolo commented at 6:42 am on August 12, 2013:

int64 -> quint64, as fee is a quint64

in src/qt/walletmodel.cpp: in b24cb4ceea outdated

174         LOCK2(cs_main, wallet->cs_wallet);
175 
176+        transaction.newPossibleKeyChange(wallet);
177+
178         // Sendmany
179         std::vector<std::pair<CScript, int64> > vecSend;

Diapolo commented at 6:43 am on August 12, 2013:

@laanwj Perhaps even here int64 should be changed to qint64?

in src/qt/walletmodeltransaction.h: in 7931cc6df9 outdated

14+
15+    QList<SendCoinsRecipient> getRecipients();
16+
17+    CWalletTx *getTransaction();
18+
19+    void setTransactionFee(int64 newFee);

Diapolo commented at 6:47 am on August 12, 2013:

int64 -> quint64, to match the function definition :)

jonasschnelli commented at 6:48 am on August 12, 2013:

fixed.! :)

Diapolo commented at 6:51 am on August 12, 2013: none

@jonasschnelli Thanks for your hard work here, I have to say sorry, but IMHO we need to check the data types for fee variables once more. In the core we use int64 nTransactionFee, so AFAIK we should then match this in our Qt code by using qint64, right? So places which you recently changed to quint64 should be qint64 to match core.

jonasschnelli commented at 6:56 am on August 12, 2013: contributor

@Diapolo yes. I’ve also thought we should not use unsigned vars. I also prefer qint64. Let me change this.

display txfee in first sendCoinsDialog message box

Conflicts while rebasing with master:
	bitcoin-qt.pro

Signed-off-by: Jonas Schnelli <jonas.schnelli@include7.ch>

2cb460966a

jonasschnelli commented at 6:58 am on August 12, 2013: contributor

Now i use qint64 for fee “holders”. Should now corespondent with the non-qt code where we use int64.

BitcoinPullTester commented at 7:10 am on August 12, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b24cb4ceeaefce25993a27badea740a922d4090c 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.

Diapolo commented at 7:14 am on August 12, 2013: none

Looks good now, let’s see what @laanwj thinks about the general idea now (use qint64 etc. in Qt code).

BitcoinPullTester commented at 8:10 am on August 12, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/2cb460966a15595b8eb3c393a4ff47b7ab1011f8 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.

wtogami commented at 11:38 am on August 23, 2013: contributor

ping @laanwj

Diapolo commented at 1:42 pm on August 29, 2013: none

Dunno if it needs another rebase, but ping @laanwj :).

laanwj commented at 5:53 pm on August 29, 2013: member

Looks good, ACK. Seems there are some minor conflicts with the payment request changes.

jonasschnelli commented at 7:21 am on August 30, 2013: contributor

I’ll do a rebase soon…

laanwj commented at 9:04 am on August 30, 2013: member

I can do it too if you want

jonasschnelli commented at 9:07 am on August 30, 2013: contributor

If you have time, would be nice! I’m busy with other stuff right now…

laanwj commented at 12:16 pm on August 30, 2013: member

Will give it a try, though it seems more work than expected. Some areas (such as on_sendbutton_clicked) are pretty heavily hit by both your patch and paymentrequest.

laanwj commented at 6:10 pm on August 30, 2013: member

See #2958 for rebased version.

laanwj closed this on Aug 30, 2013

wtogami referenced this in commit b050c136ec on Jan 4, 2014

DrahtBot locked this on Sep 8, 2021

display txfee in first sendCoinsDialog message box (issue #1715) #2651