Abstract EraseBlockData out of RewindBlockIndex
ClosedPublic
Actions

Authored by fpelliccioni on Dec 5 2019, 19:19.

Details

Reviewers

Fabien
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING4b92e0e79051: Abstract EraseBlockData out of RewindBlockIndex
rABC4b92e0e79051: Abstract EraseBlockData out of RewindBlockIndex

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 9d6dcc5):
https://github.com/bitcoin/bitcoin/pull/15402/commits/9d6dcc52c6cb0cdcda220fddccaabb0ffd40068d

Abstract EraseBlockData out of RewindBlockIndex

Note that the former 'else' branch in RewindBlockIndex is now
dealt with more naturally inside the EraseBlockData call (by
checking whether the parent needs to be re-added as candidate
after deleting a child).

Test Plan

ninja check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

feature-backport-3db0cc394-9d6dcc5

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8434
Build 14883: Default Diff Build & Tests
Build 14882: arc lint + arc unit

Event Timeline

fpelliccioni created this revision.Dec 5 2019, 19:19

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 5 2019, 19:19

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Need to adjust Summary

fpelliccioni edited the summary of this revision. (Show Details)Dec 5 2019, 19:27

fpelliccioni edited the test plan for this revision. (Show Details)

Build Bitcoin-ABC / Diffs / Diff Testing failed.

Harbormaster failed remote builds in B8434: Diff 14658!Dec 5 2019, 19:28

fpelliccioni requested review of this revision.Dec 5 2019, 19:29

fpelliccioni edited the summary of this revision. (Show Details)

deadalnix requested changes to this revision.Dec 6 2019, 11:41

deadalnix added inline comments.

src/validation.cpp
5034	Why would you erase the block data of every blocks that is not in the chain active? That doesn't seems to make any sense to me, and I strongly suspect this has something to do with the test suite being broken. However, I was unable to figure any of the teamcity shit out so idk what's actually broken.

This revision now requires changes to proceed.Dec 6 2019, 11:41

fpelliccioni retitled this revision from Granular invalidateblock and RewindBlockIndex to Granular invalidateblock and RewindBlockIndex (Partial).Dec 9 2019, 14:39

Solved Bitcoin-QT test

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8453: Diff 14692.Dec 9 2019, 15:37

fpelliccioni retitled this revision from Granular invalidateblock and RewindBlockIndex (Partial) to Abstract EraseBlockData out of RewindBlockIndex.Dec 9 2019, 15:59

fpelliccioni added a child revision: D4669: Move erasure of non-active blocks to a separate loop in RewindBlockIndex.Dec 9 2019, 17:26

That looks better, let me do a pass over this tomorow.

One small change, but otherwise, LGTM