[backport#16849] Fix block index inconsistency in InvalidateBlock()
ClosedPublic
Actions

Authored by majcosta on Aug 17 2020, 19:55.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rABC67319370e360: [backport#16849] Fix block index inconsistency in InvalidateBlock()

Summary

Fix block index inconsistency in InvalidateBlock() (Suhas Daftuar)

Pull request description:

Previously, we could release `cs_main` while leaving the block index in a state
that would fail `CheckBlockIndex()`, because `setBlockIndexCandidates` was not being
fully populated before releasing `cs_main`.

Note: Because of the way our version of "InvalidateBlock" is in the codepath for Park, Invalidate and FinalizeBlock, the lock for m_cs_chainstate needs to appear in additional places.

Backport of Core PR16849

Depends on D6956

Test Plan

cmake -DCMAKE_BUILD_TYPE=Debug -DNinja
ninja check check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Branch

pr16849

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 12409
Build 24991: Build Diff	build-diff · build-without-wallet · build-clang-10 · build-clang-tidy
Build 24990: arc lint + arc unit

Event Timeline

majcosta created this revision.Aug 17 2020, 19:55

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 17 2020, 19:55

majcosta requested review of this revision.Aug 17 2020, 19:55

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

majcosta retitled this revision from Merge #16849: Fix block index inconsistency in InvalidateBlock() to [backport#16849] Fix block index inconsistency in InvalidateBlock().Aug 17 2020, 19:55

majcosta mentioned this in D7203: [refactor] move Park and InvalidateBlock to CChainState public API.

majcosta mentioned this in D6956: [thread safety] prevent double lock of cs_main in calls to CChainState::UnwindBlock.Aug 17 2020, 19:59

Harbormaster completed remote builds in B12405: Diff 22926.Aug 17 2020, 20:08

deadalnix requested changes to this revision.Aug 17 2020, 22:19

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/validation.cpp
2972 ↗	(On Diff #22926)	You are supposed to `LOCK(m_cs_chainstate);` here.
2972 ↗	(On Diff #22926)	Alternatively, you should mark the function as requiring the lock.

This revision now requires changes to proceed.Aug 17 2020, 22:19

deadalnix added inline comments.Aug 17 2020, 22:20

src/validation.cpp
2972 ↗	(On Diff #22926)	In which case, you should also assert that the lock is help and keep the commit in the original PR.

added Clang thread safety annotations and corresponding lock held/not held assertions and reworked the comments to more closely match the PR and have (a bit) less repetition

Harbormaster completed remote builds in B12409: Diff 22930.Aug 17 2020, 23:25

deadalnix accepted this revision.Aug 18 2020, 09:56

This revision is now accepted and ready to land.Aug 18 2020, 09:56

majcosta mentioned this in rABC85fc9dfe0a61: [refactor] move Park and InvalidateBlock to CChainState public API.Aug 18 2020, 12:43

Closed by commit rABC67319370e360: [backport#16849] Fix block index inconsistency in InvalidateBlock() (authored by Wladimir J. van der Laan <laanwj@protonmail.com>, committed by majcosta). · Explain WhyAug 18 2020, 13:25

This revision was automatically updated to reflect the committed changes.

majcosta mentioned this in rABC5d825d4abb76: [thread safety] prevent double lock of cs_main in calls to CChainState….

majcosta added a commit: rABC67319370e360: [backport#16849] Fix block index inconsistency in InvalidateBlock().

majcosta mentioned this in D6959: [backport#16194] refactoring: move block metadata structures into BlockManager.Sep 22 2020, 22:57