[Qt] Add checkbox in the GUI to opt-in to RBF when creating a transaction

ryanofsky commented at 11:44 pm on January 19, 2017: member

The first three commits come from @jonasschnelli’s PR https://github.com/bitcoin/bitcoin/pull/8182

MarcoFalke added the label GUI on Jan 20, 2017

MarcoFalke added the label Wallet on Jan 20, 2017

in src/qt/sendcoinsdialog.cpp: in 054264a3a5 outdated

324@@ -324,6 +325,13 @@ void SendCoinsDialog::on_sendButton_clicked()
325     questionString.append(QString("<span style='font-size:10pt;font-weight:normal;'><br />(=%2)</span>")
326         .arg(alternativeUnits.join(" " + tr("or") + "<br />")));
327 
328+    if (ui->optInRBF->isChecked())
329+    {
330+        questionString.append("<hr /><span style='color:#aa0000;'>");
331+        questionString.append(tr("This transaction is replacable (optin-RBF)!"));

jonasschnelli commented at 7:49 am on January 20, 2017:

For clarity, we should probably use This transactions signals replaceability (optin-RBF)

morcos commented at 7:30 pm on January 20, 2017:

This red warning with exclamation point is overly alarming. It almost implies it’s dangerous to you that its replaceable. Transactions you send that are replaceable are not something to be warned about. Worst case someone doesn’t accept them, and you can replace it.

luke-jr commented at 11:23 pm on January 20, 2017:

I’m not sure we need to specially say anything here, but something more like “This transaction may be replaced.” sounds nicer.

ryanofsky commented at 8:01 pm on January 23, 2017:

Fixed spelling of replaceable, removed exclamation point and red highlight in 8924813e6b4271b5f1865e4fd7d8938490e6bd5a.

in src/qt/sendcoinsdialog.cpp: in 054264a3a5 outdated

111@@ -112,6 +112,7 @@ SendCoinsDialog::SendCoinsDialog(const PlatformStyle *_platformStyle, QWidget *p
112     ui->groupCustomFee->button((int)std::max(0, std::min(1, settings.value("nCustomFeeRadio").toInt())))->setChecked(true);
113     ui->customFee->setValue(settings.value("nTransactionFee").toLongLong());
114     ui->checkBoxMinimumFee->setChecked(settings.value("fPayOnlyMinFee").toBool());
115+    ui->optInRBF->setCheckState(fWalletRbf ? Qt::Checked : Qt::Unchecked);

jonasschnelli commented at 7:51 am on January 20, 2017:

nit: we should probably access fWalletRbf over WalletModel (not directly over the global space).

ryanofsky commented at 7:42 pm on January 23, 2017:

Done in f8193e2851eaedf1e9f63f9e2a72bafefde1971e.

jonasschnelli approved

jonasschnelli commented at 7:51 am on January 20, 2017: contributor

utACK 054264a3a504108380bd4d29d9aee4d92660305f modulo send-dialog-confirmation text overhaul.

morcos commented at 7:31 pm on January 20, 2017: member

concept ACK and lightly tested without problems

in src/qt/forms/sendcoinsdialog.ui: in 054264a3a5 outdated

1247@@ -1248,6 +1248,16 @@
1248       </widget>
1249      </item>
1250      <item>
1251+      <widget class="QCheckBox" name="optInRBF">
1252+       <property name="text">
1253+        <string>Allow Replace-By-Fee</string>

luke-jr commented at 11:20 pm on January 20, 2017:

s/Allow/Request/

ryanofsky commented at 8:07 pm on January 23, 2017:

Done in 0936ced43ba9861a1fb02e0c28341b5daad3c307.

in src/qt/forms/sendcoinsdialog.ui: in 054264a3a5 outdated

1247@@ -1248,6 +1248,16 @@
1248       </widget>
1249      </item>
1250      <item>
1251+      <widget class="QCheckBox" name="optInRBF">
1252+       <property name="text">
1253+        <string>Allow Replace-By-Fee</string>
1254+       </property>
1255+       <property name="toolTip">
1256+        <string>Signals that this transaction can be replaced with a transation paying higher fees (as long as the transaction is NOT confirmed).</string>

luke-jr commented at 11:22 pm on January 20, 2017:

“Indicates that the sender may wish to replace this transaction with a new one paying higher fees (prior to being confirmed).”

ryanofsky commented at 8:10 pm on January 23, 2017:

Done in 36548fcc3518f8f1f639cb83b8423602028edaa6.

in src/qt/walletmodel.h: in 054264a3a5 outdated

153@@ -154,7 +154,7 @@ class WalletModel : public QObject
154     };
155 
156     // prepare transaction for getting txfee before sending coins
157-    SendCoinsReturn prepareTransaction(WalletModelTransaction &transaction, const CCoinControl *coinControl = NULL);
158+    SendCoinsReturn prepareTransaction(WalletModelTransaction &transaction, const CCoinControl *coinControl = NULL, bool optInRBF = false);

luke-jr commented at 11:24 pm on January 20, 2017:

This changes the default behaviour. Presently, it uses fWalletRbf, but now it will become !RBF always.

Is there a reason not to pass this through CCoinControl?

ryanofsky commented at 8:59 pm on January 23, 2017:

Added to coincontrol in 74a594d331cc615175e55ea3a095df641ac8265a.

in src/wallet/wallet.cpp: in 054264a3a5 outdated

2565@@ -2566,9 +2566,10 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2566                 // to avoid conflicting with other possible uses of nSequence,
2567                 // and in the spirit of "smallest posible change from prior
2568                 // behavior."
2569+                bool rbf = (flags & CREATE_TX_ENABLE_RBF || ::fWalletRbf) && !(flags & CREATE_TX_DISABLE_RBF);

luke-jr commented at 11:26 pm on January 20, 2017:

This would be more readable as:

0bool rbf = ::fWalletRbf;
1if (flags & CREATE_TX_ENABLE_RBF) {
2    rbf = true;
3} else if (flags & CREATE_TX_DISABLE_RBF) {
4    rbf = false;
5}

ryanofsky commented at 9:21 pm on January 23, 2017:

Simplified with switch to coincontrol in 74a594d331cc615175e55ea3a095df641ac8265a.

in src/wallet/wallet.h: in 054264a3a5 outdated

552@@ -553,6 +553,12 @@ class CAccountingEntry
553     std::vector<char> _ssExtra;
554 };
555 
556+enum CreateTransactionFlags {
557+    CREATE_TX_DEFAULT     = 0,
558+    CREATE_TX_DONT_SIGN   = (1U << 0),

luke-jr commented at 11:29 pm on January 20, 2017:

Please invert this. Not only is it unintuitive to have it backward, it also contradicts the current function signature (that is, passing true or false will silently get the opposite behaviour).

0    CREATE_TX_DEFAULT = 1,
1    CREATE_TX_SIGN = 1,

ryanofsky commented at 9:22 pm on January 23, 2017:

Reverted for switch to coin control in 2fce406a978cfc391aa3cc3d66ff4810d48c99e0.

luke-jr changes_requested

jtimon commented at 8:26 pm on January 23, 2017: contributor

Concept ACK

Allow to opt-into RBF when creating a transaction 568c05a591

ryanofsky referenced this in commit 74a594d331 on Jan 23, 2017

ryanofsky referenced this in commit bfdb09fe69 on Jan 23, 2017

ryanofsky referenced this in commit 0936ced43b on Jan 23, 2017

ryanofsky referenced this in commit 36548fcc35 on Jan 23, 2017

ryanofsky referenced this in commit 8924813e6b on Jan 23, 2017

ryanofsky referenced this in commit f8193e2851 on Jan 23, 2017

ryanofsky force-pushed on Jan 23, 2017

ryanofsky commented at 9:37 pm on January 23, 2017: member

Squashed 95bec7a7563f416799475d03e152d826216d907b -> 92b9ff6d85819cd82ffb5bba8d3953e1725507d4.

in src/qt/sendcoinsdialog.cpp: in 92b9ff6d85 outdated

326@@ -324,6 +327,13 @@ void SendCoinsDialog::on_sendButton_clicked()
327     questionString.append(QString("<span style='font-size:10pt;font-weight:normal;'><br />(=%2)</span>")
328         .arg(alternativeUnits.join(" " + tr("or") + "<br />")));
329 
330+    if (ui->optInRBF->isChecked())
331+    {
332+        questionString.append("<hr /><span>");
333+        questionString.append(tr("This transaction is replaceable (optin-RBF)."));

jonasschnelli commented at 8:19 am on January 24, 2017:

What about using This transaction signals replaceability?

ryanofsky commented at 4:38 pm on January 24, 2017:

Updated in 8f728d0490caca49b503220e51e325a7057a7b8d.

jonasschnelli commented at 8:41 am on January 24, 2017: contributor

Tested ACK 92b9ff6d85819cd82ffb5bba8d3953e1725507d4.

With a German UI, the text can look a bit strange (see screenshot). Maybe we can add some pixels right margin?

Screenshots:

Possible follow-up work: -> Add a UI option for the global -walletrbf flag. -> Maybe persist the checkbox state (state should probably survives restarts, = strong -walletrbf in GUI settings)

aesedepece commented at 11:54 am on January 24, 2017: none

I’m wondering whether it would make more sense to put the RBF checkbox next to the fee options instead. That would definitely avoid the layout issue with “verbose” languages like German or Spanish.

Think about it. We all know that RBF allows replacing transaction A with transaction B as long as A has no confirmations yet and B includes a higher fee. Nevertheless, as far as I know, the intended use case for RBF in Core is only to allow the user to increase the fee afterwards from the transactions history view by right-clicking, pressing “Increase fee…” and selecting a higher fee.

My point is that the user will not perceive the transaction being replaced but rather being “upgraded”. That’s why I believe that from an UX point of view, RBF is more related to fees than to a transaction as a whole, and therefore putting the checkbox in the fees frame makes much more sense to me.

jonasschnelli commented at 12:55 pm on January 24, 2017: contributor

I think @aesedepece made a good point. Moving it into the fee section makes sense.

ryanofsky referenced this in commit fac1f092cd on Jan 24, 2017

ryanofsky referenced this in commit 8f728d0490 on Jan 24, 2017

ryanofsky commented at 4:39 pm on January 24, 2017: member

Moved checkbox into fee section in fac1f092cdc496fec5d1e5e5d2fde1d032cbf032.

jonasschnelli commented at 8:30 pm on January 24, 2017: contributor

Now the labels misses some bottom margin.

This should fix it:

 0diff --git a/src/qt/forms/sendcoinsdialog.ui b/src/qt/forms/sendcoinsdialog.ui
 1index a633478..e25fe05 100644
 2--- a/src/qt/forms/sendcoinsdialog.ui
 3+++ b/src/qt/forms/sendcoinsdialog.ui
 4@@ -1178,8 +1178,8 @@
 5           </property>
 6           <property name="sizeHint" stdset="0">
 7            <size>
 8-            <width>800</width>
 9-            <height>1</height>
10+            <width>40</width>
11+            <height>5</height>
12            </size>
13           </property>
14          </spacer>

ryanofsky referenced this in commit f4aac9e25c on Jan 25, 2017

[Qt] Add simple optin-RBF checkbox and confirmation info 838a58e7ca

[Qt] Change RBF checkbox to reflect -walletrbf setting

Before this commit, the checkbox would always start off unchecked. After this
commit it will respect the -walletrbf setting (which is currently false by
default).

c4e4792c53

ryanofsky force-pushed on Jan 25, 2017

ryanofsky commented at 5:57 pm on January 25, 2017: member

Thanks added in f4aac9e25c293cff9c608959c73dd44c5a3083cc, confirmed the change does improve the spacing, and squashed f4aac9e25c293cff9c608959c73dd44c5a3083cc -> c4e4792c537bae6228fc3438dd971634985922f4 (grbf.5 -> grbf.6)

jonasschnelli approved

jonasschnelli commented at 7:58 am on January 27, 2017: contributor

(again) Tested ACK c4e4792c537bae6228fc3438dd971634985922f4

in src/qt/sendcoinsdialog.cpp: in c4e4792c53

326@@ -324,6 +327,13 @@ void SendCoinsDialog::on_sendButton_clicked()
327     questionString.append(QString("<span style='font-size:10pt;font-weight:normal;'><br />(=%2)</span>")
328         .arg(alternativeUnits.join(" " + tr("or") + "<br />")));
329 
330+    if (ui->optInRBF->isChecked())
331+    {
332+        questionString.append("<hr /><span>");
333+        questionString.append(tr("This transaction signals replaceability (optin-RBF)."));

luke-jr commented at 9:26 pm on February 2, 2017:

I still think this is likely to confuse users. Why must we say so here? (The alternative case seems much more of a liability…)

ryanofsky commented at 1:37 pm on February 3, 2017:

@jonasschnelli or others, you should comment if you think the “This transaction signals replaceability” text is useful, otherwise I’m fine with removing it.

jonasschnelli commented at 8:38 pm on February 3, 2017:

I don’t know whats best here. Somehow I agree with @luke-jr that it would probably be better the label non-RBF transactions (something like “this transaction signals to be final”), but meh.

We also need to respect that RBF is deployed as a new feature and maybe users are expecting to see wether the new features is enabled or not.

So, no strong opinion. The PRs current solution seems acceptable to me.

ryanofsky commented at 5:11 pm on March 8, 2017: member

This PR has several ACKs. Should it be merged?

luke-jr commented at 5:14 pm on March 8, 2017: member

Probably needs a GUI way to actually use it first.

ryanofsky commented at 5:22 pm on March 8, 2017: member

Probably needs a GUI way to actually use it first.

In that case, this should not be merged until #9697 is merged. Would note though that there is no “GUI way” to use #9697 without this PR, so the dependency is somewhat circular.

luke-jr commented at 5:29 pm on March 8, 2017: member

Merging them at the same time seems logical.

laanwj added this to the milestone 0.15.0 on Mar 14, 2017

laanwj commented at 8:12 am on March 14, 2017: member

Adding 0.15 milestone

jonasschnelli commented at 2:30 pm on March 17, 2017: contributor

Going to merge this (even without #9697). A) it can make sense without a Qt bumper (at least you can bump over the console) and B) I don’t want to kick this back to a rebase phase.

jonasschnelli merged this on Mar 17, 2017

jonasschnelli closed this on Mar 17, 2017

jonasschnelli referenced this in commit 9c7b7cf0bb on Mar 17, 2017

MarcoFalke locked this on Sep 8, 2021

[Qt] Add checkbox in the GUI to opt-in to RBF when creating a transaction #9592