Correctly limit overview transaction list #704

1. 
   john-moffett commented at 8:50 pm on January 26, 2023: contributor

   Fixes #703

   The way the main overview page limits the number of transactions displayed (currently 5) is not an appropriate use of Qt. Our subclassed transaction sort/filter proxy model returns a maximum of 5 in rowCount(). However, the model itself actually may hold significantly more. While this has worked, it breaks the contract of rowCount().

   If bitcoin-qt is run with a DEBUG build of Qt, it’ll result in an assert-crash in certain relatively common situations (see #703 for details). Instead of artificially limiting the rowCount() in the subclassed filter, we can hide/unhide the rows in the displaying QListView upon any changes in the sorted proxy filter.

   I loaded a wallet with 20,000 transactions and did not notice any performance differences between master and this branch.

   For reference, this is the list I’m referring to:

2. Correctly limit overview transaction list

   The way that the main overview page limits the number
   of transactions displayed (currently 5) is not
   an appropriate use of Qt. If it's run with a DEBUG
   build of Qt, it'll result in a segfault in certain
   relatively common situations. Instead of artificially
   limiting the rowCount() in the subclassed proxy
   filter, we hide/unhide the rows in the displaying
   QListView upon any changes in the sorted proxy filter.

   08209c039f
3. 
   DrahtBot commented at 8:50 pm on January 26, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	Sjors, hebasto

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. jarolrod added the label Bug on Jan 27, 2023
5. jarolrod added the label UI on Jan 27, 2023
6. 
   Sjors commented at 6:52 pm on February 1, 2023: member

   tACK 08209c039ff4ca5be4982da7a2ab7a624117ce1a
7. Sjors requested review from hebasto on Feb 1, 2023
8. 
   hebasto commented at 7:44 pm on February 1, 2023: member

   Concept ACK.
9. in src/qt/overviewpage.cpp:311 in 08209c039f

   306+void OverviewPage::LimitTransactionRows()
   307+{
   308+    if (filter && ui->listTransactions && ui->listTransactions->model() && filter.get() == ui->listTransactions->model()) {
   309+        for (int i = 0; i < filter->rowCount(); ++i) {
   310+            ui->listTransactions->setRowHidden(i, i >= NUM_ITEMS);
   311+        }
   

   ---

   ---

   

   hebasto commented at 7:52 pm on February 1, 2023:

   Could this cause any performance issues with 100K+ txs wallets?

   ---

   

   john-moffett commented at 8:21 pm on February 1, 2023:

   I loaded a wallet with 30K txs and ran a time profiler. This call took approximately 3 milliseconds:

   The vast majority of time was consumed by re-running the filter and converting to QDateTime (both already existing behavior and unrelated to this PR):

   Qualitatively, I didn’t notice any difference at all.

   I’m happy to generate a bunch more transactions overnight and report back tomorrow.

10. 
    hebasto commented at 8:12 pm on February 1, 2023: member

    Tested 08209c039ff4ca5be4982da7a2ab7a624117ce1a ob Ubuntu 22.04. It fixes #703 for me.

    Still reviewing.

11. hebasto approved
12. 
    hebasto commented at 11:19 am on February 2, 2023: member

    ACK 08209c039ff4ca5be4982da7a2ab7a624117ce1a, tested on Ubuntu 22.04.
13. hebasto added the label Needs backport (24.x) on Feb 2, 2023
14. hebasto added this to the milestone 24.1 on Feb 2, 2023
15. hebasto merged this on Feb 2, 2023
16. hebasto closed this on Feb 2, 2023

    ---

17. fanquake removed the label Needs backport (24.x) on Feb 2, 2023
18. 
    fanquake commented at 12:04 pm on February 2, 2023: member

    Added to https://github.com/bitcoin/bitcoin/pull/26878 for backporting to 24.x.
19. fanquake cross-referenced this on Feb 2, 2023 from issue [24.x] Backports by fanquake
20. fanquake referenced this in commit 863cca72ab on Feb 2, 2023
21. sidhujag referenced this in commit 6c367ae3f4 on Feb 3, 2023
22. fanquake referenced this in commit b7e242ecb3 on Feb 22, 2023
23. glozow referenced this in commit c8c85ca16e on Feb 27, 2023
24. bitcoin-core locked this on Feb 2, 2024

Contributors
john-moffett DrahtBot Sjors hebasto fanquake

Labels
Bug UI

Milestone
24.1