Page MenuHomePhabricator

Call RewindBlockIndex without cs_main held
ClosedPublic

Authored by fpelliccioni on Dec 18 2019, 17:28.

Details

Summary

This PR makes a number of improvements to the InvalidateBlock (invalidateblock RPC) and RewindBlockIndex functions, primarily around breaking up their long-term cs_main holding. In addition:

  • They're made safely interruptible (bitcoind can be shutdown, and no progress in either will be lost, though if incomplete, invalidateblock won't continue after restart and will need to be called again)
  • The validation queue is prevented from overflowing (meaning invalidateblock on a very old block will not drive bitcoind OOM) (see #14289).
  • invalidateblock won't bother to move transactions back into the mempool after 10 blocks (optimization).

Partial Backport of Bitcoin Core PR15402 (commit 880ce7d):
https://github.com/bitcoin/bitcoin/commit/880ce7d46b51835c00d77a366ec28f54a05239df

Modified version of "Call RewindBlockIndex without cs_main held"

RewindBlockIndex function was removed on D4728, but the locks were moved in`AppInitMain` function.

Test Plan
  1. Build with Clang in Debug mode:
CXX=clang++ CC=clang cmake .. -D CMAKE_CXX_FLAGS="-Werror=thread-safety-analysis" -GNinja -DCMAKE_BUILD_TYPE=Debug
ninja check
  1. Verify that the compiler has not emitted a thread-safety warning.
  2. Run the node: ./src/bitcoind -regtest
  3. Verify that text similar to "Assertion failed: lock ... not held ..." is not printed on stderr.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fpelliccioni created this revision.Dec 18 2019, 17:28
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 18 2019, 17:28
fpelliccioni planned changes to this revision.Dec 18 2019, 17:29
fpelliccioni retitled this revision from Granular invalidateblock and RewindBlockIndex to Call RewindBlockIndex without cs_main held.Dec 18 2019, 17:35
fpelliccioni edited the summary of this revision. (Show Details)
fpelliccioni edited the test plan for this revision. (Show Details)
fpelliccioni edited reviewers, added: markblundeberg; removed: Fabien.
fpelliccioni updated this revision to Diff 14969.Dec 18 2019, 17:37
fpelliccioni edited the test plan for this revision. (Show Details)

remove empty line

Heh, unfortunate title for this diff now that RewindBlockIndex is gone :-D

In theory we could follow the original PR by introducing a catch clause after the LoadChainTip, and then retake the lock immediately. I don't see any advantage since the catch clause would be identical message to the one that is already there, and these sections of init complete relatively quickly (in contrast to rewindblockindex which could take a really really long time on Core if someone were actually rewinding & erasing 2 years of blocks!).

fpelliccioni added a comment.EditedDec 19 2019, 18:02

Heh, unfortunate title for this diff now that RewindBlockIndex is gone :-D

Yes, I know, but I chose to preserve the titles of the original PR.
I clarified it in the summary.

markblundeberg accepted this revision.Dec 20 2019, 23:53

Good, though please change the description to point to the github.com/bitcoin repo, not to jasonbcox repo.

This revision is now accepted and ready to land.Dec 20 2019, 23:53
fpelliccioni edited the summary of this revision. (Show Details)Dec 24 2019, 12:36
fpelliccioni updated this revision to Diff 15446.Jan 14 2020, 15:12
fpelliccioni edited the summary of this revision. (Show Details)

rebase from master

For some reason my name appears as the Author for the commit, any idea why that happened?