Reset pblocktree before deleting LevelDB file by Sjors · Pull Request #12401 · bitcoin/bitcoin

Sjors · 2018-02-09T18:55:14Z

#11043 repaced:

delete pblocktree;
pblocktree = new CBlockTreeDB(nBlockTreeDBCache, false, fReset);

With:

pblocktree.reset(new CBlockTreeDB(nBlockTreeDBCache, false, fReset));

This is problematic because new CBlockTreeDB tries to delete the existing file, which will fail with LOCK: already held by process if it's still open. That's the case for QT.

When QT finds a problem with the index it will ask the user if they want to reindex. At that point it has already opened blocks/index. It then runs this while loop again with fReset = 1, resulting in the above error.

This change makes that error go away, presumably because reset() without an argument closes the file.

Sjors · 2018-02-09T18:57:28Z

@laanwj if I'm correct then this regression was introduced after v0.15.1 and so we might need a new rc.

sipa · 2018-02-09T19:26:23Z

src/init.cpp

I don't think the conditional is necessary. Also, perhaps add a comment that the reset is necessary to avoid temporarily having two CBlockTreeDB objects?

I put the conditional there because under normal circumstances pblocktree.reset() is useless. However I don't know what it does under the hood; maybe the performance overhead is negligible.

Sjors · 2018-02-10T10:50:10Z

~~Should I change pcoinsTip, pcoinsdbview and pcoinscatcher in the same way?~~ It's not entirely clear to me what reset() actually does.

Sjors · 2018-02-10T10:52:33Z

Also paging @practicalswift.

practicalswift · 2018-02-10T15:19:38Z

Thanks for the ping and thanks for finding+fixing this issue.

utACK 7bd20cf466dfa5b2cccef00d1cd05bfbb8634f0d modulo the unnecessary conditional.

Sjors · 2018-02-10T16:29:50Z

Conditional removed.

maflcko · 2018-02-10T16:42:30Z

utACK 8b986dc5696db1d623465cb13d036bac722762e6

practicalswift · 2018-02-11T09:45:31Z

utACK 8b986dc5696db1d623465cb13d036bac722762e6

laanwj · 2018-02-11T10:35:41Z

utACK

promag

utACK 8b986dc.

promag · 2018-02-11T10:44:22Z

src/init.cpp

Add comment here?

Added comment.

maflcko · 2018-02-11T21:44:44Z

utACK a8b5d20

a8b5d20 Reset pblocktree before deleting LevelDB file (Sjors Provoost) Pull request description: #11043 repaced: ``` delete pblocktree; pblocktree = new CBlockTreeDB(nBlockTreeDBCache, false, fReset); ``` With: ``` pblocktree.reset(new CBlockTreeDB(nBlockTreeDBCache, false, fReset)); ``` This is problematic because `new CBlockTreeDB` tries to delete the existing file, which will fail with `LOCK: already held by process` if it's still open. That's the case for QT. When QT finds a problem with the index it will ask the user if they want to reindex. At that point it has already opened `blocks/index`. It then runs this [while loop](https://github.com/bitcoin/bitcoin/blob/v0.16.0rc3/src/init.cpp#L1415) again with `fReset = 1`, resulting in the above error. This change makes that error go away, presumably because `reset()` without an argument closes the file. Tree-SHA512: fde8b546912f6773ac64da8476673cc270b125aa2d909212391d1a2001b35c8260a8772126b99dfd76b39faaa286feb7c43239185fe584bd4dc2bc04a64044ce

Github-Pull: #12401 Rebased-From: a8b5d20 Tree-SHA512: 3a87b6113283c3588f46bb5c725ec33ac639e2f91c589b5c0eb4375e3d23bd6c18e7ba96faf70be2afea86d8e6252bf4dbcf9c9ed166ce2d49846ff947a36d2e

Github-Pull: bitcoin#12401 Rebased-From: a8b5d20 Tree-SHA512: 3a87b6113283c3588f46bb5c725ec33ac639e2f91c589b5c0eb4375e3d23bd6c18e7ba96faf70be2afea86d8e6252bf4dbcf9c9ed166ce2d49846ff947a36d2e

a8b5d20 Reset pblocktree before deleting LevelDB file (Sjors Provoost) Pull request description: bitcoin#11043 repaced: ``` delete pblocktree; pblocktree = new CBlockTreeDB(nBlockTreeDBCache, false, fReset); ``` With: ``` pblocktree.reset(new CBlockTreeDB(nBlockTreeDBCache, false, fReset)); ``` This is problematic because `new CBlockTreeDB` tries to delete the existing file, which will fail with `LOCK: already held by process` if it's still open. That's the case for QT. When QT finds a problem with the index it will ask the user if they want to reindex. At that point it has already opened `blocks/index`. It then runs this [while loop](https://github.com/bitcoin/bitcoin/blob/v0.16.0rc3/src/init.cpp#L1415) again with `fReset = 1`, resulting in the above error. This change makes that error go away, presumably because `reset()` without an argument closes the file. Tree-SHA512: fde8b546912f6773ac64da8476673cc270b125aa2d909212391d1a2001b35c8260a8772126b99dfd76b39faaa286feb7c43239185fe584bd4dc2bc04a64044ce

Summary: a8b5d20 Reset pblocktree before deleting LevelDB file (Sjors Provoost) Pull request description: #11043 repaced: ``` delete pblocktree; pblocktree = new CBlockTreeDB(nBlockTreeDBCache, false, fReset); ``` With: ``` pblocktree.reset(new CBlockTreeDB(nBlockTreeDBCache, false, fReset)); ``` This is problematic because `new CBlockTreeDB` tries to delete the existing file, which will fail with `LOCK: already held by process` if it's still open. That's the case for QT. When QT finds a problem with the index it will ask the user if they want to reindex. At that point it has already opened `blocks/index`. It then runs this [while loop](https://github.com/bitcoin/bitcoin/blob/v0.16.0rc3/src/init.cpp#L1415) again with `fReset = 1`, resulting in the above error. This change makes that error go away, presumably because `reset()` without an argument closes the file. Tree-SHA512: fde8b546912f6773ac64da8476673cc270b125aa2d909212391d1a2001b35c8260a8772126b99dfd76b39faaa286feb7c43239185fe584bd4dc2bc04a64044ce Backport of Core [[bitcoin/bitcoin#12401 | PR12401]] Test Plan: ninja ninja check-all src/bitoin-qt Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6632

Sjors mentioned this pull request Feb 9, 2018

Build tx index in parallel with validation #11857

Closed

Sjors force-pushed the 2018/02/reset-pblocktree branch from dc01184 to 7bd20cf Compare February 9, 2018 19:05

sipa reviewed Feb 9, 2018

View reviewed changes

maflcko added this to the 0.16.0 milestone Feb 9, 2018

jonasschnelli added UTXO Db and Indexes Block storage and removed UTXO Db and Indexes labels Feb 10, 2018

Sjors force-pushed the 2018/02/reset-pblocktree branch from 7bd20cf to 8b986dc Compare February 10, 2018 16:29

maflcko mentioned this pull request Feb 10, 2018

QT Core wallet crash when starting with "-txindex" #12406

Closed

maflcko added the Needs backport label Feb 10, 2018

promag reviewed Feb 11, 2018

View reviewed changes

Reset pblocktree before deleting LevelDB file

a8b5d20

Sjors force-pushed the 2018/02/reset-pblocktree branch from 8b986dc to a8b5d20 Compare February 11, 2018 11:14

laanwj merged commit a8b5d20 into bitcoin:master Feb 12, 2018

Sjors deleted the 2018/02/reset-pblocktree branch February 12, 2018 10:41

maflcko removed the Needs backport label Feb 12, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

Sjors commented Feb 9, 2018

Uh oh!

Sjors commented Feb 9, 2018

Uh oh!

sipa Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 10, 2018

Choose a reason for hiding this comment

Uh oh!

Sjors commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Feb 10, 2018

Uh oh!

practicalswift commented Feb 10, 2018

Uh oh!

Sjors commented Feb 10, 2018

Uh oh!

maflcko commented Feb 10, 2018

Uh oh!

practicalswift commented Feb 11, 2018

Uh oh!

laanwj commented Feb 11, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Feb 11, 2018

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 11, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Sjors commented Feb 10, 2018 •

edited

Loading