Page MenuHomePhabricator

[Part 4 of 5] Add a CChainState class to clarify internal interfaces
ClosedPublic

Authored by schancel on Oct 24 2018, 18:51.

Details

Summary

Create initial CChainState to hold chain state information

Backport of Core PR 10279 Commit fd4d80a

Depends on D1970
T572

Test Plan
make VERBOSE=1 check && ./test/functional/test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

schancel created this revision.Oct 24 2018, 18:51
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 24 2018, 18:51
schancel planned changes to this revision.Oct 26 2018, 22:47
Owners added a reviewer: Restricted Owners Package.Mar 31 2019, 20:07
schancel updated this revision to Diff 7886.Mar 31 2019, 20:15

Remove GlobalState errorenous comment.

schancel added inline comments.Mar 31 2019, 20:21
src/validation.cpp
3189 ↗(On Diff #7886)

Kind of unsure if some of these other functions should be rolled up into CChainState. Can be refactored in a later commit for clarity if there's a good reason to put them there. For now, only this function needs internal state of setBlockIndexCandidates

jasonbcox requested changes to this revision.Apr 1 2019, 22:58
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/validation.cpp
3208 ↗(On Diff #7886)

Rather than add global access here, UpdateFlags() should be added to CChainState.

3213 ↗(On Diff #7886)

ditto

3228 ↗(On Diff #7886)

This should also be added to CChainState. It appears to not be used outside of it's implementation.

5192 ↗(On Diff #7886)

This line isn't actually applied in the original commit. In the interest of fewer merge conflicts, lets revert this. It's fine otherwise though..

This revision now requires changes to proceed.Apr 1 2019, 22:58
jasonbcox edited the summary of this revision. (Show Details)Apr 2 2019, 20:34
jasonbcox added inline comments.Apr 2 2019, 21:47
src/validation.cpp
3188 ↗(On Diff #7886)

this function was changed from void to bool as it's return type. was that intended?

schancel updated this revision to Diff 7936.Apr 4 2019, 03:21

Move UpdateFlags items into CChainState, rebase, and address other minor nit.

deadalnix requested changes to this revision.Apr 4 2019, 15:30
deadalnix added a subscriber: deadalnix.

Overall it looks good, but there are discrepencies with the original code that need to be explained and possibly missing backports.

Also pindexFinalized should really be moved in CChainState, but that can be done later on.

src/chain.h
242 ↗(On Diff #7936)

How come this is here and not in validation.h ? Are there some backports missing?

src/validation.cpp
62 ↗(On Diff #7936)

What happened to enum DisconnectResult ?

219 ↗(On Diff #7936)

Fix comment. Also, clearly that comment doesn't match core source code, so there is something missing here as well.

2952 ↗(On Diff #7936)

dito

This revision now requires changes to proceed.Apr 4 2019, 15:30

Fix the comments and this change should be good.

src/chain.h
242 ↗(On Diff #7936)

It was moved to chain.h in this diff: D712

src/validation.cpp
62 ↗(On Diff #7936)

Moved into undo.h: D364

2952 ↗(On Diff #7936)

This comment was effectively removed in this diff: D1182 It's probably best to add it back for backporting purposes...

schancel updated this revision to Diff 7973.Apr 5 2019, 22:01

Rebase and fix comments.

jasonbcox accepted this revision.Apr 5 2019, 22:20
deadalnix accepted this revision.Apr 8 2019, 20:07
This revision is now accepted and ready to land.Apr 8 2019, 20:07
This revision was automatically updated to reflect the committed changes.