Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABCf432e7ff62d3: [refactor] refactor FinalizeBlockAndInvalidate

Summary

Upcoming Core PR15971 requires InvalidateBlock to not hold cs_main lock. Currently the function FinalizeBlockAndInvalidate does both things, requires cs_main to be locked and has only one callsite (FinalizeBlock RPC method)

This diff extracts the RPC method exclusive functionality and refactors FinalizeBlockAndInvalidate to FinalizeBlock, requiring cs_main to be held while allowing for the upcoming diff to move the InvalidateBlock call outside the scope of the lock.

Test Plan

ninja check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

refactor_finalizedandinvalidate

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 11911
Build 23585: Default Diff Build & Tests	Link to the build
Build 23584: Build without Wallet	Link to the build
Build 23583: Build with clang-10	Link to the build
Build 23582: arc lint + arc unit

Event Timeline

majcosta created this revision.Jul 8 2020, 23:10

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 8 2020, 23:10

majcosta requested review of this revision.Jul 8 2020, 23:10

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

deadalnix requested changes to this revision.Jul 8 2020, 23:20

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/validation.h
917	This API is unsound as it may leaves the chain in an invalid state. You can however push the lock down, use a reverse lock, or some other strategy. Intuitively, I expect pushing the locking down to work best.

This revision now requires changes to proceed.Jul 8 2020, 23:20

Harbormaster completed remote builds in B11911: Diff 22095.Jul 8 2020, 23:26

encapsulated functions into CChainState class, handled locking of cs_main in public facing method

Harbormaster completed remote builds in B11973: Diff 22207.Jul 10 2020, 23:14

deadalnix requested changes to this revision.Jul 10 2020, 23:56

deadalnix added inline comments.

src/validation.cpp
2928 ↗	(On Diff #22207)	Remove
2944 ↗	(On Diff #22207)	Just flip the condition and return.
2948 ↗	(On Diff #22207)	Remove the condition.
src/validation.h
843 ↗	(On Diff #22207)	Move above. sandwishing this between finalized and unwind, which are both related, looks weird.

This revision now requires changes to proceed.Jul 10 2020, 23:56

It appears I have gotten ahead of myself and overcomplicated this diff. The previous diff changed behavior in a way that isn't needed until https://github.com/bitcoin/bitcoin/pull/15971. That one comes next and will incorporate the feedback given so far.

I've reverted it to the extent required to make it a refactor proper, and addressed the request to move CChainParams::MarkBlockFinalized closer to CChainParams::UnwindBlock where it makes more sense.

missed a spurious EXCLUSIVE_LOCKS_REQUIRED on CChainState::FinalizeBlock definition

Harbormaster completed remote builds in B11992: Diff 22241.Jul 13 2020, 21:13

now that FinalizeBlockInternal is a private member of CChainState and has its own declaration, the lock annotation can go there instead of its definition

Harbormaster completed remote builds in B11993: Diff 22242.Jul 13 2020, 21:34

Simple name change are required, but otherwise, it looks good.

src/validation.h
840 ↗	(On Diff #22242)	The name you chose does not match what the function does. This is bound to create confusion down the road.

This revision now requires changes to proceed.Jul 13 2020, 22:13

Hm, since the private function set the CChainState pointer for the finalized block, it's more fitting for it to get the "Mark" name. The public facing API should be FinalizeBlock then

majcosta planned changes to this revision.Jul 13 2020, 23:30

Harbormaster completed remote builds in B11996: Diff 22249.Jul 13 2020, 23:31

missed a few callsites, fixed a log output to point to the right place

Harbormaster completed remote builds in B11997: Diff 22250.Jul 13 2020, 23:51

deadalnix accepted this revision.Jul 14 2020, 00:28

This revision is now accepted and ready to land.Jul 14 2020, 00:28

Closed by commit rABCf432e7ff62d3: [refactor] refactor FinalizeBlockAndInvalidate (authored by majcosta). · Explain WhyJul 14 2020, 01:43

This revision was automatically updated to reflect the committed changes.

majcosta added a commit: rABCf432e7ff62d3: [refactor] refactor FinalizeBlockAndInvalidate.

PiRK mentioned this in D11207: validation: Remove global ::PreciousBlock,ResetBlockFailureFlags.Mar 17 2022, 09:29

PiRK mentioned this in rABCe671f0466064: validation: Remove global ::PreciousBlock,ResetBlockFailureFlags.Mar 17 2022, 11:11