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
Branch
feature-backport-3db0cc394-880ce7d
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 8707
Build 15399: Default Diff Build & Tests
Build 15398: arc lint + arc unit

Event Timeline

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 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!).

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.

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)

rebase from master

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