Skip to content

Reset pblocktree before deleting LevelDB file #12401

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 1 commit into from
Feb 12, 2018
Merged

Reset pblocktree before deleting LevelDB file #12401

laanwj merged 1 commit into from
Feb 12, 2018

Conversation

@Sjors
Copy link
Member

@Sjors Sjors commented Feb 9, 2018

#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
Copy link
Member Author

Sjors commented Feb 9, 2018

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

@Sjors Sjors mentioned this pull request Feb 9, 2018
Closed
@Sjors Sjors force-pushed the 2018/02/reset-pblocktree branch from dc01184 to 7bd20cf Compare February 9, 2018 19:05
sipa
sipa reviewed Feb 9, 2018
View reviewed changes
src/init.cpp Outdated
Copy link
Member

@sipa sipa Feb 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@Sjors Sjors Feb 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@maflcko maflcko added this to the 0.16.0 milestone Feb 9, 2018
@Sjors
Copy link
Member Author

Sjors commented Feb 10, 2018 •
edited
Loading

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

@Sjors
Copy link
Member Author

Sjors commented Feb 10, 2018

Also paging @practicalswift.

@practicalswift
Copy link
Contributor

practicalswift commented Feb 10, 2018

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

utACK 7bd20cf466dfa5b2cccef00d1cd05bfbb8634f0d modulo the unnecessary conditional.

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

Sjors commented Feb 10, 2018

Conditional removed.

@maflcko
Copy link
Member

maflcko commented Feb 10, 2018

utACK 8b986dc5696db1d623465cb13d036bac722762e6

@maflcko maflcko mentioned this pull request Feb 10, 2018
Closed
@practicalswift
Copy link
Contributor

practicalswift commented Feb 11, 2018

utACK 8b986dc5696db1d623465cb13d036bac722762e6

@laanwj
Copy link
Member

laanwj commented Feb 11, 2018

utACK

promag
promag reviewed Feb 11, 2018
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 8b986dc.

src/init.cpp Outdated
Copy link
Contributor

@promag promag Feb 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment here?

Copy link
Member Author

@Sjors Sjors Feb 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment.

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

maflcko commented Feb 11, 2018

utACK a8b5d20

@laanwj laanwj merged commit a8b5d20 into bitcoin:master Feb 12, 2018
laanwj added a commit that referenced this pull request Feb 12, 2018
@laanwj
Merge #12401: Reset pblocktree before deleting LevelDB file
79313d2 
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
laanwj pushed a commit that referenced this pull request Feb 12, 2018
@Sjors @laanwj
Reset pblocktree before deleting LevelDB file
d44cd7e 
Github-Pull: #12401
Rebased-From: a8b5d20
Tree-SHA512: 3a87b6113283c3588f46bb5c725ec33ac639e2f91c589b5c0eb4375e3d23bd6c18e7ba96faf70be2afea86d8e6252bf4dbcf9c9ed166ce2d49846ff947a36d2e
@Sjors Sjors deleted the 2018/02/reset-pblocktree branch February 12, 2018 10:41
HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Mar 16, 2018
@Sjors
Reset pblocktree before deleting LevelDB file
2c8f81d 
Github-Pull: bitcoin#12401
Rebased-From: a8b5d20
Tree-SHA512: 3a87b6113283c3588f46bb5c725ec33ac639e2f91c589b5c0eb4375e3d23bd6c18e7ba96faf70be2afea86d8e6252bf4dbcf9c9ed166ce2d49846ff947a36d2e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 15, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#12401: Reset pblocktree before deleting LevelDB file
5e19f32 
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
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 18, 2020
@laanwj
Merge #12401: Reset pblocktree before deleting LevelDB file
5ba4892 
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
ftrader pushed a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this pull request Aug 17, 2020
@laanwj @ftrader
Merge #12401: Reset pblocktree before deleting LevelDB file
9b09266 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sipa sipa sipa left review comments

+1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Block storage

Projects

None yet

Milestone

0.16.0

Development

Successfully merging this pull request may close these issues.

7 participants

@Sjors @practicalswift @maflcko @laanwj @sipa @promag @jonasschnelli