Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement finalization based fork choice rule #525

Merged
merged 5 commits into from
Mar 4, 2019

Conversation

frolosofsky
Copy link
Member

@frolosofsky frolosofsky commented Feb 1, 2019

This PR introduces commits based initial full sync (UIP-21) and the new longest justified chain fork choice rule (UIP-12). These changes are interconnected and cannot be separated onto two PRs.

The good entry point to this PR is finalization/cache.h as it describes the updated workflow of finalization states.

Full sync. The entry point of the changes is net_processing.cpp/SendMessages, where initial getheaders is replaced by getcommits. Now, when a node gets connected with a peer, its initiates commits-based message exchange where we fetch commits dynasty by dynasty and restore finalization state of a remote chain.
More details are in UIP-21.

Fork choice. Added one level higher check when node decides which chain to choose. So, the first cadidate to switch is a longest justified chain, if node doesn't know such candidate, fallback to bitcoin rules. The entry point of changes is validation.cpp/FindMostWorkChain.
More details are in UIP-12

Collected various TODOs from this PR we have to address: #555.

Update

This PR has been reworked a lot, and in fact, most of the current changes are by @kostyantyn who added a bunch of functional tests and reworked the way we apply the rule.

Finalization repository and processor have been extracted in a separate PR #634 and merged.

Also, I completely removed commits full sync from this PR. One the one hand, it reduces LOC in this PR by ~1k. On the other, we don't switch to the longest justified chain which has less work than the current active chain because of bitcoin simply rejects (and don't request) such chains. It has been fixed by the commits-full-sync, so I will return such functionality as well as functional test in the upcoming PR.

One of the worst scenarios we've fixed in this PR was an undoing of the finalization during reorgs. We implemented two guards to defend against this scenario. We launched all the tests for every guard independently, and they seem to work. Finally, we enabled both of them.

The first guard is on fork-choice level,

The second one is on the finalization state processing level:

Fixes #267.

Signed-off-by: Stanislav Frolov [email protected]

@frolosofsky frolosofsky added this to the 0.1 milestone Feb 1, 2019
@frolosofsky frolosofsky self-assigned this Feb 1, 2019
@frolosofsky frolosofsky requested review from kostyantyn, AM5800, Gnappuraz and a team February 1, 2019 09:37
@frolosofsky frolosofsky changed the title Implementation of finalization fork choice and commits full sync Add implementation of finalization fork choice and commits full sync Feb 1, 2019
@frolosofsky frolosofsky added the wip Work in progress which is not supposed to be merged yet label Feb 1, 2019
Copy link
Member

@Gnappuraz Gnappuraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure cause I want to review it well before it gets merged, sorry about that.

src/esperanza/finalizationstate.cpp Outdated Show resolved Hide resolved
src/esperanza/finalizationstate.h Outdated Show resolved Hide resolved
src/esperanza/finalizationstate.h Outdated Show resolved Hide resolved
src/test/test_unite.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Gnappuraz Gnappuraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the comments below I have an other observation that is the fact that finalizationstate is becoming too big and handling too many things which makes it extremely hard to read (it took me more then 30mins to understand the whole processing and I'm quite accustomed to the class). We need a way to split it out, and keep only the casper stuff inside, everything else has to go, extracted somewhere else.

test/functional/mempool_reorg.py Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/esperanza/finalizationstate.cpp Outdated Show resolved Hide resolved
src/esperanza/finalizationstate.cpp Outdated Show resolved Hide resolved
src/esperanza/finalizationstate.cpp Outdated Show resolved Hide resolved
src/finalization/p2p.cpp Outdated Show resolved Hide resolved
src/finalization/p2p.cpp Outdated Show resolved Hide resolved
src/finalization/p2p.cpp Outdated Show resolved Hide resolved
@frolosofsky
Copy link
Member Author

Besides the comments below I have an other observation that is the fact that finalizationstate is becoming too big and handling too many things which makes it extremely hard to read (it took me more then 30mins to understand the whole processing and I'm quite accustomed to the class). We need a way to split it out, and keep only the casper stuff inside, everything else has to go, extracted somewhere else.

Fair enough. Will move cache implementation including corresponding processing of a new tip to separate file.

@Gnappuraz
Copy link
Member

Gnappuraz commented Feb 1, 2019

Fair enough. Will move cache implementation including corresponding processing of a new tip to separate file.

Thanks, this will help greatly, I really had hard time reviewing this. It has become too messy. I find largely easier to review 1.2k loc that are well organized then 900 messy ones :)

Copy link
Member

@scravy scravy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, not being so deeply engaged in the finalization implementation, it is very hard to review this. I would appreciate the use of less auto to more easily follow what is what and I would appreciate more comments on some vital things (what does FRESH or FRESH_BLOCK mean? These things are not obvious from just looking at it).

src/chain.h Outdated Show resolved Hide resolved
src/esperanza/finalizationstate.cpp Outdated Show resolved Hide resolved
src/esperanza/finalizationstate.cpp Outdated Show resolved Hide resolved
src/esperanza/finalizationstate.cpp Outdated Show resolved Hide resolved
src/esperanza/finalizationstate.cpp Outdated Show resolved Hide resolved
src/esperanza/finalizationstate.h Outdated Show resolved Hide resolved
scravy
scravy previously requested changes Feb 4, 2019
Copy link
Member

@scravy scravy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stick to code style as agreed and accepted in developer notes.

@frolosofsky frolosofsky removed the wip Work in progress which is not supposed to be merged yet label Feb 5, 2019
@scravy scravy dismissed their stale review February 5, 2019 14:29

what I complained about has been fixed

src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
@Gnappuraz Gnappuraz dismissed their stale review March 1, 2019 15:54

Releasing that I'd like to review it before merging

src/finalization/vote_recorder.cpp Show resolved Hide resolved
src/test/validation_block_tests.cpp Show resolved Hide resolved
src/chain.h Show resolved Hide resolved
src/esperanza/finalizationstate.h Show resolved Hide resolved
scravy pushed a commit that referenced this pull request Mar 3, 2019
During the review of #525 @AM5800 pointed out that we always return true from FinalizationState::ProcessNew guys. This PR fixes that.

Signed-off-by: Stanislav Frolov <[email protected]>
@frolosofsky frolosofsky force-pushed the commits-full-sync branch 2 times, most recently from 9404940 to 1911906 Compare March 4, 2019 09:02
Copy link
Member

@Gnappuraz Gnappuraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK b99f3ec

Copy link
Member

@AM5800 AM5800 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 300f334
I compiled, tested and even debugged some parts of this PR on my machine.

Overall looking at how this PR progressed I can say - great work guys!!

Kostiantyn Stepaniuk and others added 2 commits March 4, 2019 15:36
@kostyantyn kostyantyn merged commit 3e5ec2d into dtr-org:master Mar 4, 2019
@frolosofsky frolosofsky deleted the commits-full-sync branch March 4, 2019 15:09
@thothd thothd mentioned this pull request Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adjust chain reorg to PoS
7 participants