Page MenuHomePhabricator
Differential D4669

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

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 created this revision.Dec 9 2019, 17:23
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 9 2019, 17:23
Herald added a reviewer: deadalnix. · View Herald Transcript
teamcity added a comment.Dec 9 2019, 17:23

Build Bitcoin-ABC / Diffs / Diff Testing started.

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)
fpelliccioni added a parent revision: D4653: Abstract EraseBlockData out of RewindBlockIndex.
teamcity added a comment.Dec 9 2019, 17:34

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8456: Diff 14696.Dec 9 2019, 17:34
fpelliccioni added a child revision: D4683: Merge the disconnection and erasing loops in RewindBlockIndex.Dec 10 2019, 15:11
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
fpelliccioni updated this revision to Diff 14805.Dec 12 2019, 18:12

remove code section

teamcity added a comment.Dec 12 2019, 18:12

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Dec 12 2019, 18:24

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8527: Diff 14805.Dec 12 2019, 18:24
markblundeberg added a subscriber: markblundeberg.EditedDec 13 2019, 06:53

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.

markblundeberg added a comment.Dec 13 2019, 07:20

(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.

markblundeberg added a comment.Dec 14 2019, 03:56

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
fpelliccioni added a comment.Dec 18 2019, 13:50
In D4669#114633, @deadalnix wrote:

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!

fpelliccioni abandoned this revision.Dec 18 2019, 14:08

Due to D4728

Revision Contents
Changeset List

PathSize
src/
12 lines

Diff 14805

View Options

src/validation.cpp

Loading...