gui: Add transaction record type Fee #12578

Currently, in the transactions table, the fee is added as debit to the first output. It's a different behaviour from gettransaction RPC where the values are seen separately:

gettransaction 56d4c03f65f32acecd80ec6ec1b5cd12eddcd35d705f2d4c82a90c85b1cf872a

{
  ...
  "details": [
    {
      "account": "",
      "address": "2N3AFooPuYvN6zWPpThPB8w8V48Ph31PuKH",
      "category": "send",
      "amount": -1,
      "label": "",
      "vout": 0,
      "fee": -0.0000022,
      "abandoned": false
    },
    {
      "account": "",
      "address": "2N42JYZHhwRXgrpuztod6CcEXfUDLTqbhY7",
      "category": "send",
      "amount": -1,
      "label": "",
      "vout": 1,
      "fee": -0.0000022,
      "abandoned": false
    }
  ],
  ...
}

Before: <img width="848" alt="screen shot 2018-03-01 at 18 44 20" src="https://user-images.githubusercontent.com/3534524/36863063-940a2b86-1d80-11e8-95ff-a2cb39174fc1.png"> After: <img width="850" alt="screen shot 2018-03-06 at 13 55 08" src="https://user-images.githubusercontent.com/3534524/37035998-43b6a5f0-2146-11e8-9729-9f79956b2808.png"> The row background now alternates based on txid, so in the capture above, the first 3 rows refer to the same transaction.

Concept ACK, I think it is clearer to list the fee separately. On the other hand, this compounds the issue that it's not possible to see in the list what transaction output records group to one transaction.

@laanwj that's an existing problem isn't?

@laanwj that's an existing problem isn't?

Yes, that's what I said. However this compounds it, because it means every transaction will get multiple records, whereas it used to be that this only happens for (rarer) multi-sends. Not an argument against doing this, but something to be aware of, as people will ask "what does this fee belong to". They can find out by looking at the transaction details.

Yeah, in the typical listing it will show an alternating "Fee" row.

I was thinking in adding a checkbox to show/hide individual fee rows.

Another thing that could help the grouping is to change from alternate rows into "alternate txids", but that is something more complicated to do (as it depends on the proxy model parameters).

Another thing that could help the grouping is to change from alternate rows into "alternate txids", but that is something more complicated to do (as it depends on the proxy model parameters).

Yeah that can be done in a future PR. Sounds like a good idea. I wonder how that will/should interact with sorting on alternative columns, though.

I was thinking in adding a checkbox to show/hide individual fee rows.

Might be overkill, just adds an extra configuration that needs to be supported. But no strong opinion.

Do add it to the release notes, this is something that users are bound to notice.

Concept ACK. It might (or might not) make sense to only do it for multi-debits...

@laanwj I've implemented alternating rows based on transaction id (see after image above). The implementation is not efficient but it's enough to evaluate the concept. @luke-jr even for single-debit it's weird to have the fee associated to that address, sounds like you sent that value there.

Added release notes.

Concept ACK especially with the grouping (odd/even background alternative). IMO the grouping could be more visible (group entries from a transaction with an outline border line or similar)

group entries from a transaction with an outline border line or similar

I'll try something along that, but it's not something usual right?

I'll try something along that, but it's not something usual right?

I guess that is something that could be improved in a follow up PR.

Rebased.

Concept ACK

Build: https://bitcoin.jonasschnelli.ch/build/603

Tested ACK ae7a25fafc08482ba8af2174c3e8002bbe5a3bac

Before / After:

<img width="1292" alt="bildschirmfoto 2018-05-14 um 08 26 42" src="https://user-images.githubusercontent.com/178464/39981411-cafa940a-5750-11e8-9697-c2080ca7cd9b.png">

<img width="1292" alt="bildschirmfoto 2018-05-14 um 08 27 56" src="https://user-images.githubusercontent.com/178464/39981410-cadc3258-5750-11e8-9a34-20ac760d5e5b.png">

Rebased due to conflict in src/qt/test/wallettests.cpp.

Rebased due to release notes conflict.

Concept ACK

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #15452 (Replace CScriptID and CKeyID in CTxDestination with dedicated types by instagibbs)
• #15115 (GUI: Replace send-to-self with dual send+receive entries by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Please mark all single parameter ctors explicit :-)

https://en.bitcoin.it/wiki/Transaction:

A transaction is a transfer of Bitcoin value...

So, the "Transaction" table should present the value transfer history and not be littered with "Fee" type output rows.

Moreover, there is no such thing as "Fee" transaction.

If one wants display fees in the "Transaction" table this could be done with a separate "Fee" column. @promag

... the first 3 rows refer to the same transaction.

~I'm strongly disagree with such approach.~

@hebasto the idea is to understand the breakdown of the transfer value. Do you agree with the current implementation?

@promag

the idea is to understand the breakdown of the transfer value.

There is a "Details ..." window to find out and understand the breakdown of the transfer value.

Do you agree with the current implementation?

~No. I believe we should stick to "one transaction - one row" ~rule~ in the "Transactions" table.~ "Fee" rows will produce a mess while user tries sorting within columns (e.g. by "Amount").

https://en.bitcoin.it/wiki/Transaction

If the wiki is confusing, it would be good to improve it.

I believe we should stick to "one transaction - one row" rule in the "Transactions" table. @promag's point is that we have NEVER had such a "rule"...

It would also be very user unfriendly.

@luke-jr

https://en.bitcoin.it/wiki/Transaction

If the wiki is confusing, it would be good to improve it.

There is nothing confusing in the wiki, IMO. A bitcoin transaction is a well-defined technical notion since 2008 :)

@promag's point is that we have NEVER had such a "rule"...

~A "rule" is a bad wording from my side; I mean "de facto".~

It would also be very user unfriendly.

~You consider "one transaction - one row" presentation of the "Transaction" table very user unfriendly. Did I understand you correctly?~

@promag @luke-jr on second thought I've realized the "one transaction - one row" presentation is overkill. Moreover, multiple recipients transactions (multi-sends) currently are presented with appropriate number of rows.

Concept ACK.

Tested on Linux:

Note "Fee" rows are listed at the end of transaction groups. When they somehow change rows order and revert it back, "Fee" rows can be listed at the beginning of transaction groups:

Although, this behavior can be fixed in another PR. @promag

I was thinking in adding a checkbox to show/hide individual fee rows.

It will significantly improve UX, IMO.

Also, a "Fee" row does not appear for "Payment to yourself" transactions.

Does this approach work for special transactions (e.g., CoinJoin, Pay-to-Endpoint etc)?

<!--5d09a71f8925f3f132321140b44b946d-->The last travis run for this pull request was 239 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

Closing for now. The implementation is not ideal anyway.