Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Mar 17, 2017

Conversation

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Jan 19, 2017

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

Copy link
Member

@jonasschnelli jonasschnelli left a comment

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

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 20, 2017
Member

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

This comment has been minimized.

@morcos

morcos Jan 20, 2017
Member

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.

This comment has been minimized.

@luke-jr

luke-jr Jan 20, 2017
Member

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

This comment has been minimized.

@ryanofsky

ryanofsky Jan 23, 2017
Author Contributor

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

@@ -112,6 +112,7 @@ SendCoinsDialog::SendCoinsDialog(const PlatformStyle *_platformStyle, QWidget *p
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);

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 20, 2017
Member

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

This comment has been minimized.

@ryanofsky

ryanofsky Jan 23, 2017
Author Contributor

Done in f8193e2.

@morcos
Copy link
Member

@morcos morcos commented Jan 20, 2017

concept ACK
and lightly tested without problems

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

This comment has been minimized.

@luke-jr

luke-jr Jan 20, 2017
Member

s/Allow/Request/

This comment has been minimized.

@ryanofsky

ryanofsky Jan 23, 2017
Author Contributor

Done in 0936ced.

<string>Allow Replace-By-Fee</string>
</property>
<property name="toolTip">
<string>Signals that this transaction can be replaced with a transation paying higher fees (as long as the transaction is NOT confirmed).</string>

This comment has been minimized.

@luke-jr

luke-jr Jan 20, 2017
Member

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

This comment has been minimized.

@ryanofsky

ryanofsky Jan 23, 2017
Author Contributor

Done in 36548fc.

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

This comment has been minimized.

@luke-jr

luke-jr Jan 20, 2017
Member

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

@@ -154,7 +154,7 @@ class WalletModel : public QObject
};

// 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 comment has been minimized.

@luke-jr

luke-jr Jan 20, 2017
Member

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?

This comment has been minimized.

@ryanofsky

ryanofsky Jan 23, 2017
Author Contributor

Added to coincontrol in 74a594d.

@@ -2566,9 +2566,10 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
// 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 comment has been minimized.

@luke-jr

luke-jr Jan 20, 2017
Member

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;
}

This comment has been minimized.

@ryanofsky

ryanofsky Jan 23, 2017
Author Contributor

Simplified with switch to coincontrol in 74a594d.

@@ -553,6 +553,12 @@ class CAccountingEntry
std::vector<char> _ssExtra;
};

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

This comment has been minimized.

@luke-jr

luke-jr Jan 20, 2017
Member

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,

This comment has been minimized.

@ryanofsky

ryanofsky Jan 23, 2017
Author Contributor

Reverted for switch to coin control in 2fce406.

@jtimon
Copy link
Member

@jtimon jtimon commented Jan 23, 2017

Concept ACK

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 23, 2017
Add signalRbf option to CCoinControl as suggested by
Luke Dashjr <luke-jr+git@utopios.org> in
bitcoin#9592 (comment)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 23, 2017
Set signalRbf via CCoinControl as suggested by
Luke Dashjr <luke-jr+git@utopios.org> in
bitcoin#9592 (comment)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 23, 2017
s/Allow/Request/ as suggested by Luke Dashjr <luke-jr+git@utopios.org> in
bitcoin#9592 (comment)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 23, 2017
Change RBF tooltip as suggested by Luke Dashjr <luke-jr+git@utopios.org> in
bitcoin#9592 (comment)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 23, 2017
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 23, 2017
Access fWalletRbf global through WalletModel as suggested by
Jonas Schnelli <dev@jonasschnelli.ch> in
bitcoin#9592 (comment)
@ryanofsky ryanofsky force-pushed the ryanofsky:pr/grbf branch from 95bec7a to 92b9ff6 Jan 23, 2017
@ryanofsky
Copy link
Contributor Author

@ryanofsky ryanofsky commented Jan 23, 2017

Squashed 95bec7a -> 92b9ff6.

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 24, 2017
Member

What about using This transaction signals replaceability?

This comment has been minimized.

@ryanofsky

ryanofsky Jan 24, 2017
Author Contributor

Updated in 8f728d0.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Jan 24, 2017

Tested ACK 92b9ff6.

With a German UI, the text can look a bit strange (see screenshot).
Maybe we can add some pixels right margin?
bildschirmfoto 2017-01-24 um 09 28 57

Screenshots:

bildschirmfoto 2017-01-24 um 09 38 32

bildschirmfoto 2017-01-24 um 09 38 37

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
Copy link

@aesedepece aesedepece commented Jan 24, 2017

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
Copy link
Member

@jonasschnelli jonasschnelli commented Jan 24, 2017

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

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

@ryanofsky ryanofsky left a comment

Moved checkbox into fee section in fac1f09.

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

This comment has been minimized.

@ryanofsky

ryanofsky Jan 24, 2017
Author Contributor

Updated in 8f728d0.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Jan 24, 2017

Now the labels misses some bottom margin.

bildschirmfoto 2017-01-24 um 21 28 42

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>
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 25, 2017
jonasschnelli and others added 2 commits Apr 5, 2016
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 ryanofsky force-pushed the ryanofsky:pr/grbf branch from f4aac9e to c4e4792 Jan 25, 2017
@ryanofsky
Copy link
Contributor Author

@ryanofsky ryanofsky commented Jan 25, 2017

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

Copy link
Member

@jonasschnelli jonasschnelli left a comment

(again) Tested ACK c4e4792

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

This comment has been minimized.

@luke-jr

luke-jr Feb 2, 2017
Member

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

This comment has been minimized.

@ryanofsky

ryanofsky Feb 3, 2017
Author Contributor

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Feb 3, 2017
Member

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
Copy link
Contributor Author

@ryanofsky ryanofsky commented Mar 8, 2017

This PR has several ACKs. Should it be merged?

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Mar 8, 2017

Probably needs a GUI way to actually use it first.

@ryanofsky
Copy link
Contributor Author

@ryanofsky ryanofsky commented Mar 8, 2017

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
Copy link
Member

@luke-jr luke-jr commented Mar 8, 2017

Merging them at the same time seems logical.

@laanwj
Copy link
Member

@laanwj laanwj commented Mar 14, 2017

Adding 0.15 milestone

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Mar 17, 2017

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 jonasschnelli merged commit c4e4792 into bitcoin:master Mar 17, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jonasschnelli added a commit that referenced this pull request Mar 17, 2017
…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
@MarcoFalke MarcoFalke mentioned this pull request Mar 19, 2017
12 of 12 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.