Reorg [WIP] #164
Reorg [WIP] #164
Conversation
frolosofsky
added
the
wip
frolosofsky
requested review from
cornelius,
scravy,
kostyantyn,
AM5800,
Gnappuraz,
thothd,
Nizametdinov and
Ruteri
| BOUNTY_FRACTION_DENOMINATOR(params.m_bountyFractionDenominator), | ||
| BASE_INTEREST_FACTOR(params.m_baseInterestFactor), | ||
| BASE_PENALTY_FACTOR(params.m_basePenaltyFactor) { | ||
| const esperanza::FinalizationParams &settings) : settings(settings) { |
There was a problem hiding this comment.
This probably not related to the reorg. would you mind to make a separate PR (refactoring and the actual reorg)? It would make the review easier. Then the refactoring can be merged right away and we will focus on the actual code.
There was a problem hiding this comment.
Dunno. It's not really refactoring, just splitting FinalizationState into two classes in order to simplify copying of state objects. I guess those changes don't make sense without current reorg approach.
There was a problem hiding this comment.
But I agree diff looks weird.
There was a problem hiding this comment.
I also see that we introduced FinalizationStateData. I'd really like to see this part of the code which doesn't change the behavior in the separate PR. And since this PR is a WIP, I think more code is coming.
But if it's OK for others, then let's keep as it is, I'll adapt :)
There was a problem hiding this comment.
It is refactoring. I agree it should be done in a separate PR.
There was a problem hiding this comment.
You win :)
Just realized the real changes are in ~50 lines, good job for two weeks.
| auto sa = esperanza::FinalizationState::GetState(pa); | ||
| auto sb = esperanza::FinalizationState::GetState(pb); | ||
| if(sa->GetLastJustifiedEpoch() > sb->GetLastJustifiedEpoch()) return false; | ||
| if(sa->GetLastJustifiedEpoch() < sb->GetLastJustifiedEpoch()) return true; |
There was a problem hiding this comment.
Nit: In existing bitcoin code and also in the code we add and the style guide we adopted there's a space between if (else, switch, ...)
|
|
||
| // the current dynasy number | ||
| uint32_t m_currentDynasty; | ||
| uint32_t m_currentDynasty = 0; |
There was a problem hiding this comment.
A lot of these changes, in fact almost all of the changes, are purely refactoring and should be separated in a pull request that is doing these refactorings exclusively. It will be much easier to review this one then, the other one will probably be merged much faster, and so on.
In general that is something which is recommended across a lot of open source projects and would also happen when contributing to bitcoin.
| @@ -3624,6 +3638,26 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons | |||
|
|
|||
| NotifyHeaderTip(); | |||
|
|
|||
| assert(pindex != nullptr); | |||
| esperanza::FinalizationState::ProcessNewTip(*pindex, *pblock); | |||
There was a problem hiding this comment.
I think that once we have a new block a signal will be emitted. CValidationInterface should be implemented for this and act accordingly.
| } | ||
|
|
||
| // Check whether new block's chain has forked after current's chain last justified block | ||
| auto checkpoint = c->GetCheckpointHash(c->GetLastJustifiedEpoch()); |
There was a problem hiding this comment.
I do think that manifest typing helps readability and auto should be used sparingly.
This I think also follows from the Google C++ Style Guide which we adopted: https://google.github.io/styleguide/cppguide.html#auto
|
|
||
| uint64_t GetDepositSize(const uint256 &validatorIndex) const; | ||
|
|
||
| Vote GetRecommendedVote(const uint256 &validatorIndex) const; |
There was a problem hiding this comment.
What is moving all these function declarations down good for?
There was a problem hiding this comment.
Since I need to copy FinalizationState objects I need to either write really long copy c-tor or allow C++ to do it. I prefer the second option because it saves time, lines and possibility to lose something when somebody add new data members. FinalizationState has uncopiable member (mutex), so I factored out all the data into base Data-class which is auto copiable.
There was a problem hiding this comment.
Understood. Sounds like a very good approach. Would love to see it in it's own pull though ;-)
Rough implementation of applying finalization to chain reorganization. It's WIP and in fact just a result of my initial unite/bitcoin sources research.
Some bullets/todo: