[Qt] attribute replaceable (RBF) transactions #7817

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2016/04/qt_rbf changing 10 files +26 −2

jonasschnelli commented at 2:14 PM on April 5, 2016: contributor

This attributes transactiontable entries (one or more per wtx) if the transactions signals opt-in-RBF. A light orange color together with a new icon (for "incoming" and "outgoing" transactions).

<img width="1056" alt="bildschirmfoto 2016-04-05 um 16 05 34" src="https://cloud.githubusercontent.com/assets/178464/14284713/005ad530-fb49-11e5-9292-3c10be3a45c7.png">
jonasschnelli added the label GUI on Apr 5, 2016
in src/qt/transactionrecord.cpp:None in 36e467aa2d outdated
```
   6 | @@ -7,6 +7,7 @@
   7 |  #include "base58.h"
   8 |  #include "consensus/consensus.h"
   9 |  #include "main.h"
  10 | +#include "policy/rbf.h"
```
paveljanik commented at 5:21 PM on April 5, 2016:

Do you need this here?

jonasschnelli commented at 8:56 AM on April 6, 2016:

Right. This is not required. Fixed.
MarcoFalke commented at 6:21 PM on April 5, 2016: member

Concept ACK

~~Note: Please don't retrigger travis, as I reported the issue to them.~~

in src/qt/transactiondesc.cpp:None in 36e467aa2d outdated

  38 | @@ -39,7 +39,7 @@ QString TransactionDesc::FormatTxStatus(const CWalletTx& wtx)
  39 |          else if (GetAdjustedTime() - wtx.nTimeReceived > 2 * 60 && wtx.GetRequestCount() == 0)
  40 |              return tr("%1/offline").arg(nDepth);
  41 |          else if (nDepth == 0)
  42 | -            return tr("0/unconfirmed, %1").arg((wtx.InMempool() ? tr("in memory pool") : tr("not in memory pool"))) + (wtx.isAbandoned() ? ", "+tr("abandoned") : "");
  43 | +            return tr("0/unconfirmed, %1").arg((wtx.InMempool() ? tr("in memory pool") : tr("not in memory pool"))) + (wtx.signalsOptInRBF() ? ", "+tr("replaceable") : "") + (wtx.isAbandoned() ? ", "+tr("abandoned") : "");

paveljanik commented at 6:36 PM on April 5, 2016:

We will present transactions signalling opt-in RBF as "replaceable". Users will ask, what it means.

Edit: Greg asked the same question in other PR. We should probably clarify this. What about this as a topic for Thu meeting?

paveljanik commented at 6:37 PM on April 5, 2016: contributor

Concept ACK
jonasschnelli force-pushed on Apr 28, 2016
btcdrak commented at 9:11 PM on May 17, 2016: contributor

Travis needs a restart now.
petertodd commented at 9:18 PM on May 17, 2016: contributor

Concept ACK for outgoing.

Concept NACK for incoming - I'm worried about giving a false sense of security given the multitude of other ways of doublespending.
luke-jr commented at 10:08 PM on May 17, 2016: member

Mostly agreed with @petertodd

Probably the new icon/colour should be set for all unconfirmed transactions signed by a third party, whether incoming or outgoing.
paveljanik commented at 5:09 AM on May 18, 2016: contributor

@petertodd You mean that marking "incoming" tx signalling RBF with this icon can bring false sense of security to non-RBF transactions not marked with this icon? Or? Can you compare this situation with the current status quo?
petertodd commented at 8:18 AM on May 20, 2016: contributor

@paveljanik So the status quo, is that all incoming unconfirmed txs look the same; I'm concerned that showing some types of unconfirmed incoming txs differently than others in UI's intended for general users gives the false impression that Bitcoin Core can reliably tell you about the risk of any particular unconfirmed incoming txs; expert-level UI's like the RPC interface are much less of a concern to me.
paveljanik commented at 8:23 AM on May 20, 2016: contributor

@petertodd In such case, we should not show Unconfirmed at all, add a number input item defaulting to 6 into options and only show transactions with such number of confirmations.
paveljanik commented at 8:25 AM on May 20, 2016: contributor

As all unconfirmed txs are marked with ? in the UI, I think this is safe as long as there is a ? on RBF signalling txs.
petertodd commented at 8:25 AM on May 20, 2016: contributor

@paveljanik But unconfirmed is a state that I think users understand just fine: "Someone intends to send me a transaction, and if they're honest it'll probably confirm (eventually)." There's nothing wrong with showing that.
paveljanik commented at 8:55 AM on May 20, 2016: contributor
Interesting Travis failures - qInitResources_bitcoin not found. The log contains:
```
  GEN      qt/qrc_bitcoin.cpp
RCC: Error in 'qt/bitcoin.qrc': Cannot find file 'res/icons/transaction_replaceable.png'
```
laanwj commented at 6:35 AM on June 9, 2016: member

Probably you need to add the file in a Makefile so that it is included in the distribution
jonasschnelli force-pushed on Jun 9, 2016
jonasschnelli commented at 9:26 AM on June 9, 2016: contributor

Fixed nit and added missing file to Makefile.
jonasschnelli added this to the milestone 0.13.0 on Jun 9, 2016
jonasschnelli commented at 9:27 AM on June 9, 2016: contributor

@petertodd @luke-jr : What about keeping the icon but not the orangish color for incoming RBF transactions?
[Qt] attribute replaceable (RBF) transactions 7a30ed6478
jonasschnelli force-pushed on Jun 10, 2016
gmaxwell commented at 10:29 AM on June 15, 2016: contributor

@petertodd ?

I like it with just the icon myself.
petertodd commented at 4:54 PM on June 15, 2016: contributor

@gmaxwell @jonasschnelli Eh, fine, go with that.
laanwj commented at 12:59 PM on June 20, 2016: member

utACK 7a30ed6

(also agree on removing the orange color but keeping the icon)
MarcoFalke commented at 2:30 PM on June 20, 2016: member

Looks like getting rid of the orange color is preferred. @jonasschnelli Mind to take a look?

Also the nit still holds: #7817 (review)
laanwj removed this from the milestone 0.13.0 on Jun 21, 2016
laanwj commented at 9:51 AM on June 21, 2016: member

Removing 0.13 milestone, this missed the feature freeze.
btcdrak commented at 4:48 PM on August 19, 2016: contributor

needs rebase
jonasschnelli commented at 4:49 PM on August 19, 2016: contributor

I think we should first focus on #8456 (before we continue with the GUI level).
luke-jr commented at 6:41 AM on September 10, 2016: member

@jonasschnelli AFAICT it still has the same problem with just the icon. It gives a false sense of security; there is literally no rational distinction between them for incoming transactions. But maybe I'm missing some use case?
paveljanik commented at 7:54 PM on December 10, 2016: contributor

Rebase needed.
jonasschnelli commented at 9:49 AM on January 6, 2017: contributor

Closing for now due to controversy and inactivity...
jonasschnelli closed this on Jan 6, 2017
laanwj referenced this in commit 6516b36731 on Aug 25, 2018
Munkybooty referenced this in commit c0e8721372 on Jun 30, 2021
Munkybooty referenced this in commit 8f0f802c48 on Jun 30, 2021
Munkybooty referenced this in commit 9fc66137f7 on Jun 30, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#8182 [Qt] Add simple opt-in-RBF support #12676 Show "bip125-replaceable" flag, when retrieving mempool entries