gui: Add transaction record type Fee #12578

1. 
   promag commented at 6:31 pm on March 1, 2018: member

   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:

    0gettransaction 56d4c03f65f32acecd80ec6ec1b5cd12eddcd35d705f2d4c82a90c85b1cf872a
    1
    2{
    3  ...
    4  "details": [
    5    {
    6      "account": "",
    7      "address": "2N3AFooPuYvN6zWPpThPB8w8V48Ph31PuKH",
    8      "category": "send",
    9      "amount": -1,
   10      "label": "",
   11      "vout": 0,
   12      "fee": -0.0000022,
   13      "abandoned": false
   14    },
   15    {
   16      "account": "",
   17      "address": "2N42JYZHhwRXgrpuztod6CcEXfUDLTqbhY7",
   18      "category": "send",
   19      "amount": -1,
   20      "label": "",
   21      "vout": 1,
   22      "fee": -0.0000022,
   23      "abandoned": false
   24    }
   25  ],
   26  ...
   27}
   

   Before: After: The row background now alternates based on txid, so in the capture above, the first 3 rows refer to the same transaction.

2. fanquake added the label GUI on Mar 1, 2018
3. 
   laanwj commented at 6:42 pm on March 5, 2018: member

   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.
4. 
   promag commented at 6:55 pm on March 5, 2018: member

   @laanwj that’s an existing problem isn’t?
5. 
   laanwj commented at 7:29 pm on March 5, 2018: member

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

6. 
   promag commented at 8:22 pm on March 5, 2018: member

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

7. 
   laanwj commented at 8:45 pm on March 5, 2018: member

   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.

8. 
   luke-jr commented at 10:45 pm on March 5, 2018: member

   Concept ACK. It might (or might not) make sense to only do it for multi-debits…
9. promag force-pushed on Mar 6, 2018
10. 
    promag commented at 2:02 pm on March 6, 2018: member

    @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.
11. promag force-pushed on Mar 6, 2018
12. 
    promag commented at 8:07 pm on March 6, 2018: member

    Added release notes.
13. 
    jonasschnelli commented at 6:32 pm on April 10, 2018: contributor

    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)
14. 
    promag commented at 7:00 pm on April 12, 2018: member

    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?

15. 
    jonasschnelli commented at 7:40 pm on April 12, 2018: contributor

    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.

16. promag force-pushed on Apr 13, 2018
17. 
    promag commented at 9:02 pm on April 13, 2018: member

    Rebased.
18. promag force-pushed on Apr 13, 2018
19. MarcoFalke renamed this:
    Add transaction record type Fee
    gui: Add transaction record type Fee
    on May 10, 2018
20. 
    laanwj commented at 8:20 pm on May 10, 2018: member

    Concept ACK
21. 
    jonasschnelli commented at 8:25 pm on May 10, 2018: contributor

    Build: https://bitcoin.jonasschnelli.ch/build/603
22. 
    jonasschnelli commented at 6:28 am on May 14, 2018: contributor

    Tested ACK ae7a25fafc08482ba8af2174c3e8002bbe5a3bac

    Before / After:

23. promag force-pushed on May 24, 2018
24. 
    promag commented at 3:47 pm on May 24, 2018: member

    Rebased due to conflict in src/qt/test/wallettests.cpp.
25. DrahtBot added the label Needs rebase on Jun 11, 2018
26. promag force-pushed on Jun 24, 2018
27. 
    promag commented at 3:05 pm on June 24, 2018: member

    Rebased due to release notes conflict.
28. DrahtBot removed the label Needs rebase on Jun 24, 2018
29. DrahtBot added the label Needs rebase on Aug 13, 2018
30. 
    jb55 commented at 8:29 pm on August 24, 2018: member

    Concept ACK
31. gui: Avoid TransactionRecord instance for IsMine outputs 82dd3e0ffa
32. gui: Add transaction record type Fee 550f1f3d34
33. gui: Alternate row background based on txid 264f9c4d4f
34. promag force-pushed on Sep 9, 2018
35. DrahtBot removed the label Needs rebase on Sep 9, 2018
36. 
    DrahtBot commented at 5:38 pm on December 29, 2018: member

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

    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.

37. 
    practicalswift commented at 0:03 am on December 30, 2018: contributor

    Please mark all single parameter ctors explicit :-)
38. 
    hebasto commented at 7:27 pm on January 4, 2019: member

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

39. 
    promag commented at 8:54 pm on January 4, 2019: member

    @hebasto the idea is to understand the breakdown of the transfer value. Do you agree with the current implementation?
40. 
    hebasto commented at 9:16 pm on January 4, 2019: member

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

41. 
    luke-jr commented at 2:18 am on January 5, 2019: member

    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.

42. 
    hebasto commented at 7:38 am on January 5, 2019: member

    @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?~

43. 
    hebasto commented at 10:45 am on January 6, 2019: member

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

44. 
    hebasto commented at 11:14 am on January 6, 2019: member

    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.

45. 
    hebasto commented at 8:49 am on January 7, 2019: member

    Does this approach work for special transactions (e.g., CoinJoin, Pay-to-Endpoint etc)?
46. in src/qt/transactionview.cpp:70 in 264f9c4d4f

    65+
    66+        QItemDelegate::paint(painter, option, index);
    67+    }
    68+
    69+public:
    70+    TransactionRecordDelegate(QSortFilterProxyModel* proxy) : m_proxy(proxy) {}
    

    ---

    ---

    

    practicalswift commented at 8:41 pm on January 9, 2019:

    This should be explicit :-)
47. DrahtBot closed this on May 7, 2019

    ---

48. 
    DrahtBot commented at 5:07 pm on May 7, 2019: member
49. DrahtBot reopened this on May 7, 2019

    ---

50. DrahtBot added the label Needs rebase on May 9, 2019
51. 
    DrahtBot commented at 5:49 pm on May 9, 2019: member
52. 
    promag commented at 9:02 am on June 30, 2019: member

    Closing for now. The implementation is not ideal anyway.
53. promag closed this on Jun 30, 2019

    ---

54. kallewoof referenced this in commit 41c8d4c89c on Oct 4, 2019
55. kallewoof referenced this in commit 3edcdf8f0b on Oct 4, 2019
56. kallewoof referenced this in commit 1655947c15 on Oct 4, 2019
57. laanwj removed the label Needs rebase on Oct 24, 2019
58. apoelstra referenced this in commit b314a222b9 on Dec 12, 2020
59. apoelstra referenced this in commit e56825b3bb on Dec 13, 2020
60. apoelstra referenced this in commit 2351324302 on Dec 13, 2020
61. apoelstra referenced this in commit 9e75d2b63d on Dec 13, 2020
62. apoelstra referenced this in commit c2041a4364 on Dec 13, 2020
63. apoelstra referenced this in commit 51ec128897 on Dec 13, 2020
64. apoelstra referenced this in commit a8a23f6205 on Dec 13, 2020
65. apoelstra referenced this in commit 9d6b8c75e4 on Dec 13, 2020
66. apoelstra referenced this in commit 246aa9d59e on Dec 14, 2020
67. apoelstra referenced this in commit 578377a9af on Dec 14, 2020
68. apoelstra referenced this in commit 11e80537f8 on Dec 14, 2020
69. apoelstra referenced this in commit 2e46cecf35 on Dec 14, 2020
70. apoelstra referenced this in commit 1af4e96ce0 on Dec 16, 2020
71. apoelstra referenced this in commit fe4ca2ee58 on Dec 16, 2020
72. apoelstra referenced this in commit 832990c146 on Dec 16, 2020
73. apoelstra referenced this in commit f912cb8e2e on Mar 26, 2021
74. DrahtBot locked this on Dec 16, 2021

Contributors
promag laanwj luke-jr jonasschnelli jb55 DrahtBot practicalswift hebasto

Labels
GUI