Page MenuHomePhabricator

Release cs_main during RewindBlockIndex operation
AbandonedPublic

Authored by fpelliccioni on Dec 10 2019, 18:10.

Details

Reviewers
Fabien
deadalnix
Group Reviewers
Restricted Project
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 436f7d7):
https://github.com/bitcoin/bitcoin/pull/15402/commits/436f7d735f1c37e77d42ff59d4cbb1bd76d5fcfb

Depends on D4683

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-436f7d7
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 8477
Build 14965: Default Diff Build & Tests
Build 14964: arc lint + arc unit

Event Timeline

fpelliccioni retitled this revision from Granular invalidateblock and RewindBlockIndex to Release cs_main during RewindBlockIndex operation.Dec 10 2019, 18:12
fpelliccioni edited the summary of this revision. (Show Details)
fpelliccioni edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Dec 11 2019, 01:26
deadalnix added inline comments.
src/validation.cpp
4991

There is a comment that is missing here, indicating that you are likely missing other backports.

5013

This is completely wrong. The original code switched to height at some point, so there is likely a missing dependency here, but, in addition, the loop isn't even doing what the loop does in the original code - and is likely not needed at all.

This revision now requires changes to proceed.Dec 11 2019, 01:26