Page MenuHomePhabricator

Move erasure of non-active blocks to a separate loop in RewindBlockIndex
AbandonedPublic

Authored by fpelliccioni on Dec 9 2019, 17:23.

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 32b2696):
https://github.com/bitcoin/bitcoin/pull/15402/commits/32b2696ab4b079db736074b57bbc24deaee0b3d9

Move erasure of non-active blocks to a separate loop in RewindBlockIndex

This lets us simplify the iteration to just walking back in the chain,
rather than looping over all of mapBlockIndex.

Depends on D4653

Test Plan
ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D4669
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 8527
Build 15053: Default Diff Build & Tests
Build 15052: arc lint + arc unit

Event Timeline

fpelliccioni retitled this revision from Granular invalidateblock and RewindBlockIndex to Move erasure of non-active blocks to a separate loop in RewindBlockIndex.Dec 9 2019, 17:25
fpelliccioni edited the summary of this revision. (Show Details)
fpelliccioni edited the test plan for this revision. (Show Details)
fpelliccioni edited the summary of this revision. (Show Details)
deadalnix requested changes to this revision.Dec 11 2019, 01:15
deadalnix added inline comments.
src/validation.cpp
5001 ↗(On Diff #14696)

That loop doesn't correspond tot he loop that exist in the source material. The loop in the source material is supposed to clear segwit blocks that have been downloaded by a node using a pre segwit version.

5031 ↗(On Diff #14696)

You can probably simply remove this loop.

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

I don't think this loop should be removed. If you disconnect a block you absolutely want it to be back in setBlockIndexCandidates. The loop comment is of course completely wrong, since that loop does not modify flags.

Rather, from what I can tell, the entire existence of this function is pointless because it never rewinds a block. In fact any decent compiler doing value range analysis should be able to optimize away the entire loop since its condition is never true:

int nHeight = chainActive.Height() + 1;
...
while (chainActive.Height() >= nHeight) {

And we don't have segwit, so we don't need to rewind blocks.

If the function did ever rewind a block, then having this loop gone would immediately cause a failure in CheckBlockIndex.

(if you don't believe me, try putting assert(0) as the first line in the while-loop and observe that all tests pass).

I think we should just remove RewindBlockIndex entirely from here and from init.cpp.

I think we should just remove RewindBlockIndex entirely from here and from init.cpp.

For your consideration: D4728

deadalnix requested changes to this revision.Dec 17 2019, 23:50

As @markblundeberg pointed, this looks like a bunch of dead code. Can you look at D4728 and see what that means for this backport?

Not really requesting changes, requesting change to get this back on your queue.

This revision now requires changes to proceed.Dec 17 2019, 23:50

As @markblundeberg pointed, this looks like a bunch of dead code. Can you look at D4728 and see what that means for this backport?

Not really requesting changes, requesting change to get this back on your queue.

Yes it seems that @markblundeberg is right.
This unlocks the commits I have queued.
Thanks!