Details

Reviewers

deadalnix
markblundeberg

Group Reviewers

Restricted Project

Commits

rSTAGING9096e76f01e5: Call InvalidateBlock without cs_main held
rABC9096e76f01e5: Call InvalidateBlock without cs_main held

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

Call InvalidateBlock (and ParkBlock) without cs_main held

Depends on D4757

Test Plan

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-all

Verify that the compiler has not emitted a thread-safety warning.
Run the node: ./src/bitcoind -regtest
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-9b1ff5c

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8708
Build 15401: Default Diff Build & Tests
Build 15400: arc lint + arc unit

Event Timeline

fpelliccioni created this revision.Dec 18 2019, 18:44

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 18 2019, 18:44

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

fpelliccioni planned changes to this revision.Dec 18 2019, 18:45

fpelliccioni requested review of this revision.Dec 18 2019, 18:47

fpelliccioni retitled this revision from Granular invalidateblock and RewindBlockIndex to Call InvalidateBlock without cs_main held.

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

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

fpelliccioni added a parent revision: D4757: Call RewindBlockIndex without cs_main held.

fpelliccioni edited reviewers, added: markblundeberg; removed: Fabien.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8628: Diff 14970.Dec 18 2019, 18:57

Something seems a bit off here -- note that ABC has renamed the 'inner' InvalidateBlock to UnwindBlock in D1952 (not sure why). I think you might be putting the lock in the wrong place, and it should rather go on UnwindBlock.

markblundeberg requested changes to this revision.Dec 18 2019, 23:23

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

In D4758#115041, @markblundeberg wrote:

Something seems a bit off here -- note that ABC has renamed the 'inner' InvalidateBlock to UnwindBlock in D1952 (not sure why). I think you might be putting the lock in the wrong place, and it should rather go on UnwindBlock.

I didn't want to change the semantics of UnwindBlock because this function is used by ParkBlock.
In any case, I will create another Diff to change ParkBlock.

In D4758#115130, @fpelliccioni wrote:

In D4758#115041, @markblundeberg wrote:

Something seems a bit off here -- note that ABC has renamed the 'inner' InvalidateBlock to UnwindBlock in D1952 (not sure why). I think you might be putting the lock in the wrong place, and it should rather go on UnwindBlock.

I didn't want to change the semantics of UnwindBlock because this function is used by ParkBlock.
In any case, I will create another Diff to change ParkBlock.

Hmm, however, in the original PR there will get many many more changes made to UnwindBlock (called InvalidateBlock in Core) -- https://github.com/bitcoin/bitcoin/pull/15402/files#diff-24efdb00bfbe56b140fb006b562cc70bL2787 . The essence of the PR is that UnwindBlock will only take short-term locks during its execution. If we start off like this then it seems none of the following commits will make sense.

In D4758#115570, @markblundeberg wrote:

In D4758#115130, @fpelliccioni wrote:

In D4758#115041, @markblundeberg wrote:

Something seems a bit off here -- note that ABC has renamed the 'inner' InvalidateBlock to UnwindBlock in D1952 (not sure why). I think you might be putting the lock in the wrong place, and it should rather go on UnwindBlock.

I didn't want to change the semantics of UnwindBlock because this function is used by ParkBlock.
In any case, I will create another Diff to change ParkBlock.

Hmm, however, in the original PR there will get many many more changes made to UnwindBlock (called InvalidateBlock in Core) -- https://github.com/bitcoin/bitcoin/pull/15402/files#diff-24efdb00bfbe56b140fb006b562cc70bL2787 . The essence of the PR is that UnwindBlock will only take short-term locks during its execution. If we start off like this then it seems none of the following commits will make sense.

I think I don't get your point.
I am trying to make the same changes that the PR makes, but creating a Diff for each commit.
The final code (the code of the last Diff) should be the equal (or very similar) to PR code.

fpelliccioni added a child revision: D4802: Release cs_main during InvalidateBlock iterations.Dec 26 2019, 18:54

In D4758#115843, @fpelliccioni wrote:

I think I don't get your point.
I am trying to make the same changes that the PR makes, but creating a Diff for each commit.
The final code (the code of the last Diff) should be the equal (or very similar) to PR code.

Yeah, if that's your goal, what I mean is that the original commit inserts the lock into CChainState::InvalidateBlock, which ABC calls CChainState::UnwindBlock (aside from the name, these functions are the same thing). Instead this diff modifies a totally different function (the toplevel InvalidateBlock function) which was not touched in the original commit. So, this is simply not a faithful interpretation of the original commit.

src/validation.cpp
3071 ↗	(On Diff #14970)	change should go here

rebasing

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8708: Diff 15111.Jan 2 2020, 20:45

Se @markblundeberg 's comment. This is not a faithful translation of Core's patch.

This revision now requires changes to proceed.Jan 3 2020, 05:43

applying changes to UnwindBlock

Build Bitcoin-ABC / Diffs / Diff Testing started.

fpelliccioni edited the test plan for this revision. (Show Details)Jan 3 2020, 16:11

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

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8723: Diff 15134.Jan 3 2020, 16:15

markblundeberg accepted this revision.Jan 4 2020, 01:07

markblundeberg added inline comments.

src/rpc/blockchain.cpp
1607 ↗	(On Diff #15134)	I am wondering if this needs to be unlocked as well, however, as it does more things I don't think it should be done naively. And anyway, even if it's possible to make it more granularly locked, it's outside of the scope of this backport.

deadalnix accepted this revision.Jan 5 2020, 10:15

deadalnix added inline comments.

src/rpc/blockchain.cpp
1607 ↗	(On Diff #15134)	Making the finalization part of chainstate and the locking behavior consistent is very in line with this backport. Doesn't need to happen int he same patch, but it needs to happen.