Page MenuHomePhabricator

Call InvalidateBlock without cs_main held
ClosedPublic

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

Details

Reviewers
deadalnix
markblundeberg
Group Reviewers
Restricted Project
Commits
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
  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-all
  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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fpelliccioni created this revision.Dec 18 2019, 18:44
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 18 2019, 18:44
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 edited reviewers, added: markblundeberg; removed: Fabien.

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

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.

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.

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.

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

deadalnix requested changes to this revision.Fri, Jan 3, 05:43

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

This revision now requires changes to proceed.Fri, Jan 3, 05:43
fpelliccioni updated this revision to Diff 15134.Fri, Jan 3, 16:10

applying changes to UnwindBlock

fpelliccioni edited the test plan for this revision. (Show Details)Fri, Jan 3, 16:11
fpelliccioni edited the summary of this revision. (Show Details)
markblundeberg accepted this revision.Sat, Jan 4, 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.Sun, Jan 5, 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.

This revision is now accepted and ready to land.Sun, Jan 5, 10:15
fpelliccioni added inline comments.Fri, Jan 10, 18:34
src/rpc/blockchain.cpp
1607 ↗(On Diff #15134)

Done in D4803

fpelliccioni updated this revision to Diff 15448.Tue, Jan 14, 15:20

rebase from master

This revision was automatically updated to reflect the committed changes.