Page MenuHomePhabricator
Differential D7578

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

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABCcc35ed039d0e: [refactor] make the finalized BlockIndex* a private member of CChainState
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

majcosta created this revision.Sep 27 2020, 19:39
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 27 2020, 19:39
majcosta requested review of this revision.Sep 27 2020, 19:39
teamcity edited the summary of this revision. (Show Details)Sep 27 2020, 19:39

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

Harbormaster completed remote builds in B12938: Diff 23827.Sep 27 2020, 19:51
majcosta added a child revision: D7580: [backport#16443 1/3] refactor: pcoinsTip -> CChainState::CoinsTip().Sep 27 2020, 20:14
majcosta edited the summary of this revision. (Show Details)Sep 27 2020, 20:54
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
majcosta updated this revision to Diff 23907.Sep 28 2020, 18:04

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

majcosta planned changes to this revision.Sep 28 2020, 18:07
majcosta marked an inline comment as done.
Harbormaster completed remote builds in B12985: Diff 23907.Sep 28 2020, 18:15
majcosta requested review of this revision.Sep 28 2020, 18:53
majcosta marked an inline comment as done.Sep 28 2020, 21:53
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 marked 4 inline comments as done.Sep 29 2020, 00:23
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 updated this revision to Diff 23934.Sep 29 2020, 00:24
majcosta marked 2 inline comments as done.

addressed comments

Harbormaster completed remote builds in B13000: Diff 23934.Sep 29 2020, 00:36
majcosta updated this revision to Diff 23936.Sep 29 2020, 00:58

finalizedBlockIndex() -> FinalizedBlockIndex()

Harbormaster completed remote builds in B13001: Diff 23936.Sep 29 2020, 01:06
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
majcosta updated this revision to Diff 23937.Sep 29 2020, 02:06

used member variable name rather than accessor within member functions

Harbormaster completed remote builds in B13002: Diff 23937.Sep 29 2020, 02:18
majcosta edited the summary of this revision. (Show Details)Sep 29 2020, 02:51
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
majcosta updated this revision to Diff 24041.Sep 29 2020, 20:36
  • 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
Harbormaster completed remote builds in B13059: Diff 24041.Sep 29 2020, 20:47
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
majcosta updated this revision to Diff 24064.Sep 30 2020, 16:46

moved GetFinalizedBlock implementation closer to same-context IsBlockFinalized

Harbormaster completed remote builds in B13074: Diff 24064.Sep 30 2020, 17:05
deadalnix accepted this revision.Sep 30 2020, 22:57
This revision is now accepted and ready to land.Sep 30 2020, 22:57
Closed by commit rABCcc35ed039d0e: [refactor] make the finalized BlockIndex* a private member of CChainState (authored by majcosta). · Explain WhyOct 1 2020, 00:23
This revision was automatically updated to reflect the committed changes.
majcosta added a commit: rABCcc35ed039d0e: [refactor] make the finalized BlockIndex* a private member of CChainState.

Revision Contents
Changeset List

PathSize
src/
avalanche/
2 lines
rpc/
3 lines
test/
34 lines
30 lines
43 lines

Diff 24064

View Options

src/avalanche/processor.cpp

Loading...
View Options

src/rpc/blockchain.cpp

Loading...
View Options

src/test/finalization_tests.cpp

Loading...
View Options

src/validation.h

Loading...
View Options

src/validation.cpp

Loading...