Page MenuHomePhabricator

[refactor] make the finalized BlockIndex* a private member of CChainState
ClosedPublic

Authored by majcosta on Sep 27 2020, 19:39.

Details

Summary

Upcoming Core PR16443 adds g_chainstate to validation.cpp, this caused a bit of a havoc with initializaion order of pindexFinalized (also declared in validation.cpp) so I took this opportunity to encapsulate the finalizedBlockIndex pointer into CChainState

additionally:

  • moved an assertion that cs_main lock must be held down to where the actual pointer is accessed, to guarantee that all code paths trigger that.
  • added an assert to prevent users from changing the finalizationBlockIndex pointer to nullptr
Test Plan
cmake .. -DENABLE_SANITIZERS=address
ninja all check check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Branch
encapsulate_pindexfinalized_in_cchainstate
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 13074
Build 26199: Build Difflint-circular-dependencies · build-without-wallet · build-diff · build-clang-tidy · build-clang
Build 26198: arc lint + arc unit

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.

deadalnix requested changes to this revision.Sep 27 2020, 23:03
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/validation.h
954 ↗(On Diff #23827)

Don't define state in random places. Also, this is to be the only blockindex that is in CChainState. What about others?

956 ↗(On Diff #23827)

I don't see what the function brings, especially since nullptr is a valid state.

This revision now requires changes to proceed.Sep 27 2020, 23:03

moved m_finalizedBlockIndex initalization closer to the rest of CChainState state declarations and eliminated the setter function for it

majcosta marked an inline comment as done.
deadalnix requested changes to this revision.Sep 28 2020, 22:26

Most calls to finalizedBlockIndex seems completely redundant. This is looking better.

src/validation.cpp
3333 ↗(On Diff #23907)

Why is this removed?

src/validation.h
821 ↗(On Diff #23907)

Isn't this supposed to be guarded by cs_main?

895 ↗(On Diff #23907)

Do not change comments for no reason.

956 ↗(On Diff #23907)

dito

This revision now requires changes to proceed.Sep 28 2020, 22:26
majcosta added inline comments.
src/validation.cpp
3333 ↗(On Diff #23907)

Actually I moved it to const CBlockIndex* finalizedBlock const because that is now the "lowest level" function call that accesses m_finalizedBlockIndex, so that ensures users always hit the AssertLockHeld(cs_main).

That's also why I was using finalizedBlockIndex() consistently/redundantly, make sure all codepaths led to that assertion, although that is a weaker argument now that the thread safety annotation is in place.

src/validation.h
821 ↗(On Diff #23907)

Yep, will do.

majcosta marked 2 inline comments as done.

addressed comments

finalizedBlockIndex() -> FinalizedBlockIndex()

deadalnix requested changes to this revision.Sep 29 2020, 01:48

Many calls to FinalizedBlockIndex are still redundant.

This revision now requires changes to proceed.Sep 29 2020, 01:48

used member variable name rather than accessor within member functions

majcosta edited the summary of this revision. (Show Details)
deadalnix requested changes to this revision.Sep 29 2020, 09:26
deadalnix added inline comments.
src/validation.cpp
3218 ↗(On Diff #23937)

Considering this has just one call site, some more refactoring seems to be in order.

3332 ↗(On Diff #23937)

This require locking annotations.

5019 ↗(On Diff #23937)

It is not freed in this function, so how can you possibly know it has been freed? (hint: it hasn't yet).

This revision now requires changes to proceed.Sep 29 2020, 09:26
  • made GetFinalizedBlock the only read-accessor to m_finalizedBlockIndex and added a cs_main lock required annotation
  • included IsBlockFinalized into CChainState, const qualified it.
  • corrected comment in CChainState::UnloadBlockIndex() about setting m_finalizedBlockIndex to null before it's deleted
deadalnix requested changes to this revision.Sep 30 2020, 07:05

Now this is much better. A small nit and this is good to go.

src/validation.cpp
3222 ↗(On Diff #24041)

Keep this next to IsBlockFinalized

This revision now requires changes to proceed.Sep 30 2020, 07:05

moved GetFinalizedBlock implementation closer to same-context IsBlockFinalized

This revision is now accepted and ready to land.Sep 30 2020, 22:57