Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Feb 5, 2019

Conversation

Projects
None yet
6 participants
@dooglus
Copy link
Contributor

commented Jan 18, 2019

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.

@ryanofsky
Copy link
Contributor

left a comment

utACK ebb0758. 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 (comment), so this was probably not the cause.

transactionView->setSortingEnabled(true);
transactionView->sortByColumn(TransactionTableModel::Date, Qt::DescendingOrder);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 18, 2019

Contributor

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.

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 18, 2019

Member

Confirmed by Qt docs:

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

@dooglus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

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.

@@ -218,6 +218,7 @@ void TransactionView::setModel(WalletModel *_model)
{
transactionProxyModel = new TransactionFilterProxy(this);
transactionProxyModel->setSourceModel(_model->getTransactionTableModel());
transactionProxyModel->setSortRole(Qt::EditRole);

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 18, 2019

Member

This line is a duplicate of that one:

transactionProxyModel->setSortRole(Qt::EditRole);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 18, 2019

Contributor

re: #15203 (comment)

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.

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 18, 2019

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 18, 2019

Contributor

re: #15203 (comment)

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

case Qt::EditRole:
// Edit role is used for sorting, so return the unformatted values

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 24, 2019

Member

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

Yep.

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 24, 2019

Member

@ryanofsky @laanwj thanks for clarification.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

utACK ebb0758
Thanks for submitting.

@ryanofsky
Copy link
Contributor

left a comment

It would be nice to remove the extra setSortRole call: #15203 (comment). But I think it'd be fine to merge this PR with or without that change.

@dooglus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

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) runni…
…ng master/HEAD".

Patch taken from @ryanofsky's comment
  #9683 (comment)
and refined according to
  #15203 (comment)

@dooglus dooglus force-pushed the dooglus:fix-startup-crash branch from ebb0758 to 364cff1 Feb 5, 2019

@dooglus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

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 added a commit to MarcoFalke/bitcoin that referenced this pull request Feb 5, 2019

Merge bitcoin#15203: Fix issue bitcoin#9683 "gui, wallet: random abor…
…t (segmentation fault)

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

Pull request description:

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

  [MarcoFalke wrote](bitcoin#9683 (comment)):
  > 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.

Tree-SHA512: 3bbb2291cdf03ab7e7b5b796df68d76272491e35d473a89f4550065554c092f867659a7b8d7a1a91461ae4dc9a3b13b72541eafdbd732536463e9f3cf82300c8

@MarcoFalke MarcoFalke merged commit 364cff1 into bitcoin:master Feb 5, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Feb 6, 2019

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

Patch taken from @ryanofsky's comment
  bitcoin#9683 (comment)
and refined according to
  bitcoin#15203 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.