Reset pblocktree before deleting LevelDB file #12401

pull Sjors wants to merge 1 commits into bitcoin:master from Sjors:2018/02/reset-pblocktree changing 1 files +3 −0

Sjors commented at 6:55 PM on February 9, 2018: member
#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 commented at 6:57 PM on February 9, 2018: member

@laanwj if I'm correct then this regression was introduced after v0.15.1 and so we might need a new rc.
Sjors cross-referenced this on Feb 9, 2018 from issue Build tx index in parallel with validation by jimpo
Sjors force-pushed on Feb 9, 2018
in src/init.cpp:1428 in 7bd20cf466 outdated
```
1424 | @@ -1425,6 +1425,9 @@ bool AppInitMain()
1425 |                  pcoinsTip.reset();
1426 |                  pcoinsdbview.reset();
1427 |                  pcoinscatcher.reset();
1428 | +                if (fReset) {
```
sipa commented at 7:26 PM on February 9, 2018:

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?

Sjors commented at 10:48 AM on February 10, 2018:

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.
MarcoFalke added this to the milestone 0.16.0 on Feb 9, 2018
jonasschnelli added the label UTXO Db and Indexes on Feb 10, 2018
jonasschnelli added the label Block storage on Feb 10, 2018
jonasschnelli removed the label UTXO Db and Indexes on Feb 10, 2018
Sjors commented at 10:50 AM on February 10, 2018: member

~Should I change pcoinsTip, pcoinsdbview and pcoinscatcher in the same way?~ It's not entirely clear to me what reset() actually does.
Sjors commented at 10:52 AM on February 10, 2018: member

Also paging @practicalswift.
practicalswift commented at 3:19 PM on February 10, 2018: contributor

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

utACK 7bd20cf466dfa5b2cccef00d1cd05bfbb8634f0d modulo the unnecessary conditional.
Sjors force-pushed on Feb 10, 2018
Sjors commented at 4:29 PM on February 10, 2018: member

Conditional removed.
MarcoFalke commented at 4:42 PM on February 10, 2018: member

utACK 8b986dc5696db1d623465cb13d036bac722762e6
MarcoFalke cross-referenced this on Feb 10, 2018 from issue QT Core wallet crash when starting with "-txindex" by user78713
MarcoFalke added the label Needs backport on Feb 10, 2018
practicalswift commented at 9:45 AM on February 11, 2018: contributor

utACK 8b986dc5696db1d623465cb13d036bac722762e6
laanwj commented at 10:35 AM on February 11, 2018: member

utACK

in src/init.cpp:1430 in 8b986dc569 outdated

1424 | @@ -1425,6 +1425,7 @@ bool AppInitMain()
1425 |                  pcoinsTip.reset();
1426 |                  pcoinsdbview.reset();
1427 |                  pcoinscatcher.reset();
1428 | +                pblocktree.reset();

promag commented at 10:44 AM on February 11, 2018:

Add comment here?

Sjors commented at 11:14 AM on February 11, 2018:

Added comment.

promag commented at 10:44 AM on February 11, 2018: member

utACK 8b986dc.
Reset pblocktree before deleting LevelDB file a8b5d20f4f
Sjors force-pushed on Feb 11, 2018
MarcoFalke commented at 9:44 PM on February 11, 2018: member

utACK a8b5d20f4f171828b2bd70ab2405c42b1e452e5b
laanwj merged this on Feb 12, 2018
laanwj closed this on Feb 12, 2018
laanwj referenced this in commit 79313d2e20 on Feb 12, 2018
laanwj referenced this in commit d44cd7ed4b on Feb 12, 2018
Sjors deleted the branch on Feb 12, 2018
MarcoFalke removed the label Needs backport on Feb 12, 2018
HashUnlimited referenced this in commit 2c8f81dc63 on Mar 16, 2018
PastaPastaPasta referenced this in commit 7a795d3f01 on Mar 14, 2020
PastaPastaPasta referenced this in commit 8b13a956bc on Mar 14, 2020
PastaPastaPasta referenced this in commit 5e19f32b6b on Mar 15, 2020
deadalnix referenced this in commit 5ba4892705 on Jun 18, 2020
ftrader referenced this in commit 9b09266ea4 on Aug 17, 2020
ckti referenced this in commit c32dc3e689 on Mar 28, 2021
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.16.0

Linked (view graph)

#11043 Use std::unique_ptr (C++11) where possible #11857 Build tx index in parallel with validation #12406 QT Core wallet crash when starting with "-txindex"