Page MenuHomePhabricator

[refactor] refactor FinalizeBlockAndInvalidate
ClosedPublic

Authored by majcosta on Jul 8 2020, 23:10.

Details

Reviewers
deadalnix
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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 ↗(On Diff #22095)

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

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

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

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

deadalnix requested changes to this revision.Jul 13 2020, 22:13

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

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

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