Fix issue #9683 "gui, wallet: random abort (segmentation fault) #15203

pull dooglus wants to merge 1 commits into bitcoin:master from dooglus:fix-startup-crash changing 1 files +1 −1

dooglus commented at 3:55 PM on January 18, 2019: contributor

Patch taken from @ryanofsky's comment #9683 (comment).

MarcoFalke wrote:

Mind to submit this patch as a pull request?

So that's what I'm doing.

I was regularly seeing crashes on startup before applying this patch and haven't seen a single crash on startup since applying it almost a month ago.
laanwj added the label GUI on Jan 18, 2019

in src/qt/transactionview.cpp:233 in ebb0758d14 outdated

 229 | @@ -229,8 +230,8 @@ void TransactionView::setModel(WalletModel *_model)
 230 |          transactionView->setAlternatingRowColors(true);
 231 |          transactionView->setSelectionBehavior(QAbstractItemView::SelectRows);
 232 |          transactionView->setSelectionMode(QAbstractItemView::ExtendedSelection);
 233 | +        transactionView->horizontalHeader()->setSortIndicator(TransactionTableModel::Date, Qt::DescendingOrder);
 234 |          transactionView->setSortingEnabled(true);
 235 | -        transactionView->sortByColumn(TransactionTableModel::Date, Qt::DescendingOrder);

ryanofsky commented at 6:50 PM on January 18, 2019:

Note to reviewers: This call is removed to avoid sorting twice in a row. setSortingEnabled already sorts the data, and sortByColumn would repeat the same sort.

hebasto commented at 8:27 PM on January 18, 2019:

Confirmed by Qt docs:

Setting the property to true with setSortingEnabled() immediately triggers a call to sortByColumn() with the current sort section and order.

ryanofsky approved
ryanofsky commented at 7:04 PM on January 18, 2019: member
utACK ebb0758d14311496e1b1f676c74d5953663e8660. Change should be ok. We never actually narrowed down exactly what was causing the abort to happen or why the change helps, but I have three theories:
1. ~~Missing setSortRole call~~ was causing TransactionTableModel::data() to return some unknown field with an inconsistent (non-transitive?) sort order that made Qt crash internally.
2. Not specifying a sort column before calling setSortingEnabled(true) was causing a similar problem.
3. Code is still buggy, and this change only masks the problem. Abort is just less likely to happen now because removed sortByColumn call means sort only happens once instead of twice in a row.
It would probably be possible to experiment more and narrow down the cause, but I think not necessary.

Maybe do keep a backup of the crashing wallet around, to help reproduce in case similar bugs happen in the future.

EDIT: setSortRole call was not actually missing: #15203 (review), so this was probably not the cause.
dooglus commented at 7:47 PM on January 18, 2019: contributor

keep a backup of the crashing wallet around

It's my main Bitcoin wallet that I've been using since 2011. I'll be keeping it around.
in src/qt/transactionview.cpp:221 in ebb0758d14 outdated
```
 217 | @@ -218,6 +218,7 @@ void TransactionView::setModel(WalletModel *_model)
 218 |      {
 219 |          transactionProxyModel = new TransactionFilterProxy(this);
 220 |          transactionProxyModel->setSourceModel(_model->getTransactionTableModel());
 221 | +        transactionProxyModel->setSortRole(Qt::EditRole);
```
hebasto commented at 7:57 PM on January 18, 2019:

This line is a duplicate of that one: https://github.com/bitcoin/bitcoin/blob/9096276e0b2d5b7e19af9a5f3c144ef108ee55e0/src/qt/transactionview.cpp#L225

ryanofsky commented at 8:58 PM on January 18, 2019:

re: #15203 (review)

This line is a duplicate of that one:

Good catch. I think it means this line is not actually doing anything and can be removed.

hebasto commented at 9:04 PM on January 18, 2019:

@ryanofsky why does the default Qt::DisplayRole does not work in this case?

ryanofsky commented at 9:25 PM on January 18, 2019:

re: #15203 (review)

@ryanofsky why does the default Qt::DisplayRole does not work in this case?

I don't know the history of this code or what best practices are in Qt, but it looks like the intention was to use EditRole to sort based on unformatted values, while letting DisplayRole return formatted values:

https://github.com/bitcoin/bitcoin/blob/63144335becb705a233b32fb8d63f37b7d62071a/src/qt/transactiontablemodel.cpp#L530-L531

laanwj commented at 3:46 PM on January 24, 2019:

but it looks like the intention was to use EditRole to sort based on unformatted values, while letting DisplayRole return formatted values:

Yep.

hebasto commented at 3:52 PM on January 24, 2019:

@ryanofsky @laanwj thanks for clarification.
hebasto changes_requested
jonasschnelli commented at 8:24 PM on January 29, 2019: contributor

utACK ebb0758d14311496e1b1f676c74d5953663e8660 Thanks for submitting.
ryanofsky commented at 8:17 PM on February 4, 2019: member

It would be nice to remove the extra setSortRole call: #15203 (review). But I think it'd be fine to merge this PR with or without that change.
dooglus commented at 12:09 AM on February 5, 2019: contributor

I can do that if you like, but the older hardware I was previously using finally quit working. I'm now on much faster CPU & storage so don't know if I can even reproduce the issue any more.

Fix issue #9683 "gui, wallet: random abort (segmentation fault) running master/HEAD".

Patch taken from @ryanofsky's comment
  https://github.com/bitcoin/bitcoin/issues/9683#issuecomment-448035913
and refined according to
  https://github.com/bitcoin/bitcoin/pull/15203#discussion_r249168229

364cff1cab

dooglus force-pushed on Feb 5, 2019
dooglus commented at 12:20 AM on February 5, 2019: contributor

I did a git reset HEAD^ and git push --force. Was that wrong? I don't see the discussion of the previous commit any more unless I click the comment link I put in the commit message.
MarcoFalke referenced this in commit 9b63c436a6 on Feb 5, 2019
MarcoFalke merged this on Feb 5, 2019
MarcoFalke closed this on Feb 5, 2019
HashUnlimited referenced this in commit 7733a8397c on Feb 6, 2019
PastaPastaPasta referenced this in commit 5f8e80e72c on Jun 27, 2021
PastaPastaPasta referenced this in commit 367c476db3 on Jun 28, 2021
PastaPastaPasta referenced this in commit 64a5d29616 on Jun 28, 2021
PastaPastaPasta referenced this in commit bc186bba30 on Jun 29, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#9683 gui, wallet: random abort (segmentation fault) running master/HEAD