Fix regression in TransactionTableModel #8

pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:200619-crash changing 4 files +12 −3
  1. hebasto commented at 2:16 PM on June 19, 2020: member

    Since https://github.com/bitcoin/bitcoin/pull/17993 a crash is possible on exit.

    Steps to reproduce:

    • precondition: the old chain
    • start bitcoin-qt
    • wait until sync
    • on main window: Menu -> File -> Quit
    • crash

    This PR is based on ryanofsky's suggestion.

    Fixes #7.

  2. MarcoFalke renamed this:
    qt: Fix regression in TransactionTableModel
    Fix regression in TransactionTableModel
    on Jun 19, 2020
  3. in src/qt/transactiontablemodel.cpp:667 in cd3b6d0666 outdated 
     663 | @@ -664,7 +664,7 @@ QVariant TransactionTableModel::headerData(int section, Qt::Orientation orientat
 664 |  QModelIndex TransactionTableModel::index(int row, int column, const QModelIndex &parent) const
 665 |  {
 666 |      Q_UNUSED(parent);
 667 | -    TransactionRecord* data = priv->index(walletModel->wallet(), walletModel->clientModel().getBestBlockHash(), row);
 668 | +    TransactionRecord* data = priv->index(walletModel->wallet(), walletModel->getLastBlockProcessed(), row);
    ryanofsky commented at 2:53 PM on June 19, 2020:

    In commit "qt: Fix regression in TransactionTableModel" (cd3b6d0666fe0e6191596e2aa56980ab60841f2d)

    Maybe also add null check on line 195 above:

    https://github.com/bitcoin-core/gui/blob/cd3b6d0666fe0e6191596e2aa56980ab60841f2d/src/qt/transactiontablemodel.cpp#L195

    if (!cur_block_hash.IsNull() && rec->statusUpdateNeeded(cur_block_hash) && wallet.tryGetTxStatus(rec->hash, wtx, numBlocks, block_time)) {

    To avoid spurious updateStatus calls during shutdown

    hebasto commented at 4:00 PM on June 19, 2020:

    Updated.

    promag commented at 9:43 AM on June 21, 2020:

    9735b0750bc1e510a5ca6764f9d3cdca5cc7f760

    nit, assert block hash is not null in TransactionRecord::statusUpdateNeeded.

    hebasto commented at 6:45 PM on June 21, 2020:

    @promag

    nit, assert block hash is not null in TransactionRecord::statusUpdateNeeded.

    Does this suggestion implies that status.needsUpdate should be the first operand of OR in TransactionRecord::statusUpdateNeeded()?

    promag commented at 8:18 PM on June 22, 2020:

    No, I don't mean to change that.

    hebasto commented at 8:47 PM on June 22, 2020:

    Updated.

  4. ryanofsky approved
  5. ryanofsky commented at 2:54 PM on June 19, 2020: contributor

    Code review ACK cd3b6d0666fe0e6191596e2aa56980ab60841f2d. Thanks for catching & fixing!

  6. hebasto commented at 3:59 PM on June 19, 2020: member

    Updated cd3b6d0666fe0e6191596e2aa56980ab60841f2d -> 9735b0750bc1e510a5ca6764f9d3cdca5cc7f760 (pr8.01 -> pr8.02, diff):

  7. vasild commented at 6:26 PM on June 19, 2020: contributor

    ACK 9735b075

    Maybe both commits should be squashed because the second fixes an issue from the first.

  8. in src/qt/walletmodel.h:159 in cd3b6d0666 outdated 
     154 | @@ -155,6 +155,9 @@ class WalletModel : public QObject
 155 |      AddressTableModel* getAddressTableModel() const { return addressTableModel; }
 156 |  
 157 |      void refresh(bool pk_hash_only = false);
 158 | +
 159 | +    uint256 getLastBlockProcessed();
    promag commented at 9:38 AM on June 21, 2020:

    cd3b6d0666fe0e6191596e2aa56980ab60841f2d

    Can be const.

    hebasto commented at 6:46 PM on June 21, 2020:

    Can be const.

    Correct.

    hebasto commented at 8:46 PM on June 22, 2020:

    Updated.

  9. promag commented at 9:45 AM on June 21, 2020: contributor

    Tested ACK 9735b0750bc1e510a5ca6764f9d3cdca5cc7f760.

    Just a couple of minor suggestions.

  10. ryanofsky approved
  11. ryanofsky commented at 7:30 PM on June 22, 2020: contributor

    Code review ACK 9735b0750bc1e510a5ca6764f9d3cdca5cc7f760. Only change since last review is avoiding updates for null blocks on shutdown

    This PR probably has enough acks to be merged alreayd, though I agree with other reviewers suggestions, like squashing the second commit to make reason for that change more clear.

  12. promag commented at 7:52 PM on June 22, 2020: contributor

    I've a slight preference to "fix" before merging, suggestions so far are trivial.

  13. hebasto commented at 8:07 PM on June 22, 2020: member

    @promag I will happy to fix, just awaiting a response for #8 (review) :)

  14. hebasto force-pushed on Jun 22, 2020
  15. in src/qt/transactionrecord.cpp:237 in f8e385a8f8 outdated 
     233 | @@ -234,7 +234,7 @@ void TransactionRecord::updateStatus(const interfaces::WalletTxStatus& wtx, cons
 234 |  
 235 |  bool TransactionRecord::statusUpdateNeeded(const uint256& block_hash) const
 236 |  {
 237 | -    return status.m_cur_block_hash != block_hash || status.needsUpdate;
 238 | +    return (!block_hash.IsNull() && status.m_cur_block_hash != block_hash) || status.needsUpdate;
    promag commented at 8:39 PM on June 22, 2020:

    I meant

    assert(!block_hash.IsNull());
return status.m_cur_block_hash != block_hash || status.needsUpdate;
    hebasto commented at 8:48 PM on June 22, 2020:

    Updated.

  16. qt: Fix regression in TransactionTableModel
    Since #17993 a crash is possible on exit.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
    d906aaa117
  17. hebasto force-pushed on Jun 22, 2020
  18. hebasto commented at 8:46 PM on June 22, 2020: member

    Updated 9735b0750bc1e510a5ca6764f9d3cdca5cc7f760 -> d906aaa117e337fc70575beecc0d6da314f57385 (pr8.02 -> pr8.04, diff):

    • addressed all comments
    • squashed
  19. promag commented at 8:50 PM on June 22, 2020: contributor

    Code review ACK d906aaa117e337fc70575beecc0d6da314f57385.

    Long term I think WalletModel::setClientModel should go away and maybe have a reference to client model instead. On quit all wallet models should be deleted before client model.

  20. ryanofsky approved
  21. ryanofsky commented at 9:03 PM on June 22, 2020: contributor

    Code review ACK d906aaa117e337fc70575beecc0d6da314f57385. Only changes are squashing, adding assert and adding const

  22. vasild approved
  23. vasild commented at 7:33 AM on June 23, 2020: contributor

    ACK d906aaa1

  24. hebasto commented at 3:55 PM on June 25, 2020: member

    @MarcoFalke Mind looking into this PR?

  25. MarcoFalke merged this on Jun 26, 2020
  26. MarcoFalke closed this on Jun 26, 2020
  27. hebasto deleted the branch on Jun 26, 2020
  28. laanwj referenced this in commit 924a4ff7eb on Oct 29, 2020
  29. jonasschnelli referenced this in commit c33662a0ea on Dec 2, 2020
  30. apoelstra referenced this in commit e9e690258d on Dec 3, 2020
  31. Fabcien referenced this in commit 1efc1fd377 on Feb 23, 2021
  32. gwillen referenced this in commit 3f85455775 on Mar 19, 2021
  33. MarcoFalke referenced this in commit 590e49ccf2 on Apr 4, 2021
  34. MarcoFalke referenced this in commit bce09da122 on Apr 28, 2021
  35. fanquake referenced this in commit fa00bb2c5c on Apr 29, 2021
  36. MarcoFalke referenced this in commit eb9a1fe037 on May 7, 2021
  37. laanwj referenced this in commit ee9befe8b4 on May 12, 2021
  38. MarcoFalke referenced this in commit c857148636 on May 15, 2021
  39. bitcoin-core locked this on Feb 15, 2022
Contributors
hebastoryanofskypromagvasild
Linked (view graph)
#7 Crash on exit#247 bitcoin-qt stays at "Done loading" for a long time#545 SIGABRT on macOS after Dock menu --> Quit#668 Got stuck in "loading wallets", then segfault on closing#689 Segmentation fault when closing while loading wallets#785 New crash in v26.0#796 ci_native_asan: UndefinedBehaviorSanitizer: null-pointer-use qt/test/wallettests.cpp:424:25 in#880 SegFault in QSortFilterProxyModelPrivate::build_source_to_proxy_mapping
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 21:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me