Page MenuHomePhabricator

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

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

Details

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

Event Timeline

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

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