[Qt] Add checkbox in the GUI to opt-in to RBF when creating a transaction by ryanofsky · Pull Request #9592 · bitcoin/bitcoin

ryanofsky · 2017-01-19T23:44:00Z

The first three commits come from @jonasschnelli's PR #8182

jonasschnelli

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

jonasschnelli · 2017-01-20T07:49:16Z

src/qt/sendcoinsdialog.cpp

+    if (ui->optInRBF->isChecked())
+    {
+        questionString.append("<hr /><span style='color:#aa0000;'>");
+        questionString.append(tr("This transaction is replacable (optin-RBF)!"));


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

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.

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

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

jonasschnelli · 2017-01-20T07:51:15Z

src/qt/sendcoinsdialog.cpp

    ui->groupCustomFee->button((int)std::max(0, std::min(1, settings.value("nCustomFeeRadio").toInt())))->setChecked(true);
    ui->customFee->setValue(settings.value("nTransactionFee").toLongLong());
    ui->checkBoxMinimumFee->setChecked(settings.value("fPayOnlyMinFee").toBool());
+    ui->optInRBF->setCheckState(fWalletRbf ? Qt::Checked : Qt::Unchecked);


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

Done in f8193e2.

morcos · 2017-01-20T19:31:43Z

concept ACK
and lightly tested without problems

luke-jr · 2017-01-20T23:20:56Z

src/qt/forms/sendcoinsdialog.ui

     </item>
+     <item>
+      <widget class="QCheckBox" name="optInRBF">
+       <property name="text">


s/Allow/Request/

Done in 0936ced.

luke-jr · 2017-01-20T23:22:06Z

src/qt/forms/sendcoinsdialog.ui

+       <property name="text">
+        <string>Allow Replace-By-Fee</string>
+       </property>
+       <property name="toolTip">


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

Done in 36548fc.

luke-jr · 2017-01-20T23:23:44Z

src/qt/sendcoinsdialog.cpp

+    if (ui->optInRBF->isChecked())
+    {
+        questionString.append("<hr /><span style='color:#aa0000;'>");
+        questionString.append(tr("This transaction is replacable (optin-RBF)!"));


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

luke-jr · 2017-01-20T23:24:58Z

src/qt/walletmodel.h


    // prepare transaction for getting txfee before sending coins
-    SendCoinsReturn prepareTransaction(WalletModelTransaction &transaction, const CCoinControl *coinControl = NULL);
+    SendCoinsReturn prepareTransaction(WalletModelTransaction &transaction, const CCoinControl *coinControl = NULL, bool optInRBF = false);


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?

Added to coincontrol in 74a594d.

luke-jr · 2017-01-20T23:26:59Z

src/wallet/wallet.cpp

                // to avoid conflicting with other possible uses of nSequence,
                // and in the spirit of "smallest posible change from prior
                // behavior."
+                bool rbf = (flags & CREATE_TX_ENABLE_RBF || ::fWalletRbf) && !(flags & CREATE_TX_DISABLE_RBF);


This would be more readable as:

bool rbf = ::fWalletRbf; if (flags & CREATE_TX_ENABLE_RBF) { rbf = true; } else if (flags & CREATE_TX_DISABLE_RBF) { rbf = false; }

Simplified with switch to coincontrol in 74a594d.

luke-jr · 2017-01-20T23:29:32Z

src/wallet/wallet.h


+enum CreateTransactionFlags {
+    CREATE_TX_DEFAULT     = 0,
+    CREATE_TX_DONT_SIGN   = (1U << 0),


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

CREATE_TX_DEFAULT = 1, CREATE_TX_SIGN = 1,

Reverted for switch to coin control in 2fce406.

jtimon · 2017-01-23T20:26:23Z

Concept ACK

Add signalRbf option to CCoinControl as suggested by Luke Dashjr <[email protected]> in bitcoin#9592 (comment)

Set signalRbf via CCoinControl as suggested by Luke Dashjr <[email protected]> in bitcoin#9592 (comment)

s/Allow/Request/ as suggested by Luke Dashjr <[email protected]> in bitcoin#9592 (comment)

Change RBF tooltip as suggested by Luke Dashjr <[email protected]> in bitcoin#9592 (comment)

Update RBF status line with comments in bitcoin#9592 (comment)

Access fWalletRbf global through WalletModel as suggested by Jonas Schnelli <[email protected]> in bitcoin#9592 (comment)

ryanofsky · 2017-01-23T21:37:26Z

Squashed 95bec7a -> 92b9ff6.

jonasschnelli · 2017-01-24T08:19:08Z

src/qt/sendcoinsdialog.cpp

+    if (ui->optInRBF->isChecked())
+    {
+        questionString.append("<hr /><span>");
+        questionString.append(tr("This transaction is replaceable (optin-RBF)."));


What about using This transaction signals replaceability?

Updated in 8f728d0.

jonasschnelli · 2017-01-24T08:41:57Z

Tested ACK 92b9ff6.

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 · 2017-01-24T11:54:32Z

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 · 2017-01-24T12:55:59Z

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

Move RBF checkbox to fee section as suggested in bitcoin#9592 (comment) and bitcoin#9592 (comment)

Change RBF status line as suggested in bitcoin#9592 (comment) and bitcoin#9592 (comment)

ryanofsky

Moved checkbox into fee section in fac1f09.

ryanofsky · 2017-01-24T16:38:32Z

src/qt/sendcoinsdialog.cpp

+    if (ui->optInRBF->isChecked())
+    {
+        questionString.append("<hr /><span>");
+        questionString.append(tr("This transaction is replaceable (optin-RBF)."));


Updated in 8f728d0.

jonasschnelli · 2017-01-24T20:30:13Z

Now the labels misses some bottom margin.

This should fix it:

diff --git a/src/qt/forms/sendcoinsdialog.ui b/src/qt/forms/sendcoinsdialog.ui
index a633478..e25fe05 100644
--- a/src/qt/forms/sendcoinsdialog.ui
+++ b/src/qt/forms/sendcoinsdialog.ui
@@ -1178,8 +1178,8 @@
           </property>
           <property name="sizeHint" stdset="0">
            <size>
-            <width>800</width>
-            <height>1</height>
+            <width>40</width>
+            <height>5</height>
            </size>
           </property>
          </spacer>

Increase spacing below RBF checkbox as suggested bitcoin#9592 (comment)

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

ryanofsky · 2017-01-25T17:57:14Z

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

jonasschnelli

(again) Tested ACK c4e4792

luke-jr · 2017-02-02T21:26:15Z

src/qt/sendcoinsdialog.cpp

+    if (ui->optInRBF->isChecked())
+    {
+        questionString.append("<hr /><span>");
+        questionString.append(tr("This transaction signals replaceability (optin-RBF)."));


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

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

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 · 2017-03-08T17:11:42Z

This PR has several ACKs. Should it be merged?

luke-jr · 2017-03-08T17:14:42Z

Probably needs a GUI way to actually use it first.

ryanofsky · 2017-03-08T17:22:25Z

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 · 2017-03-08T17:29:16Z

Merging them at the same time seems logical.

laanwj · 2017-03-14T08:12:53Z

Adding 0.15 milestone

jonasschnelli · 2017-03-17T14:30:31Z

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.

…ing a transaction c4e4792 [Qt] Change RBF checkbox to reflect -walletrbf setting (Russell Yanofsky) 838a58e [Qt] Add simple optin-RBF checkbox and confirmation info (Jonas Schnelli) 568c05a Allow to opt-into RBF when creating a transaction (Jonas Schnelli) Tree-SHA512: 3d52dcd4e44da8aed4d631748074afef78d38c860f2a8b95323f4801a989d6599a3498a753fc10daba4098c527ef5a0eb942e5b3f1bfd656e1a6bd272b8e6c57

ryanofsky mentioned this pull request Jan 19, 2017

Enable RBF transactions in wallet by default #9527

Closed

maflcko added GUI Wallet labels Jan 20, 2017

ryanofsky mentioned this pull request Jan 20, 2017

[RPC] Simplified bumpfee command. #8456

Merged

jonasschnelli approved these changes Jan 20, 2017

View reviewed changes

luke-jr suggested changes Jan 20, 2017

View reviewed changes

Allow to opt-into RBF when creating a transaction

568c05a

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 23, 2017

fixup! Allow to opt-into RBF when creating a transaction

74a594d

Add signalRbf option to CCoinControl as suggested by Luke Dashjr <[email protected]> in bitcoin#9592 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 23, 2017

fixup! [Qt] Add simple optin-RBF checkbox and confirmation info

bfdb09f

Set signalRbf via CCoinControl as suggested by Luke Dashjr <[email protected]> in bitcoin#9592 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 23, 2017

fixup! [Qt] Add simple optin-RBF checkbox and confirmation info

0936ced

s/Allow/Request/ as suggested by Luke Dashjr <[email protected]> in bitcoin#9592 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 23, 2017

fixup! [Qt] Add simple optin-RBF checkbox and confirmation info

36548fc

Change RBF tooltip as suggested by Luke Dashjr <[email protected]> in bitcoin#9592 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 23, 2017

fixup! [Qt] Add simple optin-RBF checkbox and confirmation info

8924813

Update RBF status line with comments in bitcoin#9592 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 23, 2017

fixup! [Qt] Change RBF checkbox to reflect -walletrbf setting

f8193e2

Access fWalletRbf global through WalletModel as suggested by Jonas Schnelli <[email protected]> in bitcoin#9592 (comment)

ryanofsky force-pushed the pr/grbf branch from 95bec7a to 92b9ff6 Compare January 23, 2017 21:35

jonasschnelli reviewed Jan 24, 2017

View reviewed changes

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 24, 2017

fixup! [Qt] Add simple optin-RBF checkbox and confirmation info

fac1f09

Move RBF checkbox to fee section as suggested in bitcoin#9592 (comment) and bitcoin#9592 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 24, 2017

fixup! [Qt] Add simple optin-RBF checkbox and confirmation info

8f728d0

Change RBF status line as suggested in bitcoin#9592 (comment) and bitcoin#9592 (comment)

ryanofsky commented Jan 24, 2017

View reviewed changes

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 25, 2017

fixup! [Qt] Add simple optin-RBF checkbox and confirmation info

f4aac9e

Increase spacing below RBF checkbox as suggested bitcoin#9592 (comment)

jonasschnelli and others added 2 commits January 25, 2017 12:51

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

838a58e

[Qt] Change RBF checkbox to reflect -walletrbf setting

c4e4792

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

ryanofsky force-pushed the pr/grbf branch from f4aac9e to c4e4792 Compare January 25, 2017 17:51

jonasschnelli approved these changes Jan 27, 2017

View reviewed changes

luke-jr reviewed Feb 2, 2017

View reviewed changes

luke-jr mentioned this pull request Feb 2, 2017

Opt-into-RBF for RPC & bitcoin-tx #9672

Merged

ryanofsky mentioned this pull request Mar 9, 2017

Refactor Bumpfee, move core functionality to CWallet #9681

Merged

laanwj added this to the 0.15.0 milestone Mar 14, 2017

jonasschnelli merged commit c4e4792 into bitcoin:master Mar 17, 2017

maflcko mentioned this pull request Mar 19, 2017

TODO for release notes 0.15.0 #9889

Closed

12 tasks

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

ryanofsky commented Jan 19, 2017

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jan 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jan 23, 2017

Uh oh!

ryanofsky commented Jan 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jan 24, 2017

Uh oh!

aesedepece commented Jan 24, 2017

Uh oh!

jonasschnelli commented Jan 24, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jan 24, 2017

Uh oh!

ryanofsky commented Jan 25, 2017

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 23, 2017 •

edited

Loading

ryanofsky Jan 23, 2017 •

edited

Loading

ryanofsky Jan 23, 2017 •

edited

Loading