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.

Sign up for GitHub

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

Jump to bottom

Extract finalization state storage #634

Merged
merged 14 commits into from
Feb 24, 2019
Merged

Extract finalization state storage #634

merged 14 commits into from
Feb 24, 2019

Conversation

frolosofsky
Copy link
Member

@frolosofsky frolosofsky commented Feb 18, 2019 •
edited
Loading

This PR extracts finalization states storage in finalization::StateRepository and finalization::StateProcessor components.

Also, adds commits-based finalization state processing (see StateProcessor::ProcessNewTipCandidate, StateProcessor::ProcessNewCommits) and corresponding unit tests.

Also, from now calling FinalizationState::ProcessNewTip() twice is prohibited. We process tip only once per state because we have state per every block. Generally, we shouldn't have direct calls to this function except for StateProcessor and tests.

These changes were initially present in #525. This PR is part of splitting that PR on smaller pieces.

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

@frolosofsky frolosofsky added refactoring Changes which clean up code but don't change the user-visible behavior finalization labels Feb 18, 2019
@frolosofsky frolosofsky self-assigned this Feb 18, 2019
@frolosofsky frolosofsky added this to the 0.1 milestone Feb 18, 2019
@frolosofsky frolosofsky requested review from scravy and a team February 18, 2019 15:59
@frolosofsky
Copy link
Member Author

As I added src/finalization to the linter path, I committed style changes of src/finalization/p2p* and src/finalization/vote_recorder* right in this PR.

frolosofsky
frolosofsky commented Feb 19, 2019
View reviewed changes
src/esperanza/finalizationstate.cpp
@@ -1017,12 +990,6 @@ bool FinalizationState::ProcessNewCommits(const CBlockIndex &block_index,
ProcessNewCommit(tx);
}

if (!g_storage.Restoring() && (block_index.nHeight + 2) % m_settings.epoch_length == 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

For reviewers: this piece is moved to cache.cpp.

@Gnappuraz we will completely clear finalization state from snapshot code in #525.

frolosofsky
frolosofsky commented Feb 19, 2019
View reviewed changes
src/esperanza/finalizationstate.h
@@ -220,17 +234,6 @@ class FinalizationState : public FinalizationStateData {
std::vector<Validator> GetValidators() const;
const Validator *GetValidator(const uint160 &validatorAddress) const;

static void Init(const esperanza::FinalizationParams &params,
Copy link
Member Author

Choose a reason for hiding this comment

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

For reviewers: I'm clearing finalization state from global/static functions in favor of cache component, but we still have a number of them.

For compatibility with current code base:

  • FinalizationState::GetState.

These functions depends on FinalizationState::GetState and simplifies accees to esperanza::FinalizationParams.

  • FinalizationState::ValidateDepositAmount;
  • esperanza::GetEpochLength;
  • esperanza::GetEpoch;

scravy
scravy reviewed Feb 19, 2019
View reviewed changes
test/functional/rpc_getblocksnapshot.py Outdated Show resolved Hide resolved
test/functional/rpc_getblocksnapshot.py Outdated Show resolved Hide resolved
test/functional/rpc_getblocksnapshot.py Outdated Show resolved Hide resolved
@frolosofsky frolosofsky added the wip Work in progress which is not supposed to be merged yet label Feb 19, 2019
@frolosofsky
Copy link
Member Author

I find some of the tests fail, rebased with master and working on the fix. Until then, wip.

@frolosofsky frolosofsky removed the wip Work in progress which is not supposed to be merged yet label Feb 20, 2019
scravy
scravy reviewed Feb 20, 2019
View reviewed changes
src/finalization/state_storage.cpp Outdated Show resolved Hide resolved
src/finalization/state_storage.cpp Outdated Show resolved Hide resolved
src/finalization/state_storage.cpp Outdated Show resolved Hide resolved
src/finalization/state_storage.cpp Outdated Show resolved Hide resolved
src/finalization/state_storage.cpp Outdated Show resolved Hide resolved
@frolosofsky frolosofsky mentioned this pull request Feb 21, 2019
Open
14 tasks
scravy
scravy reviewed Feb 21, 2019
View reviewed changes
src/finalization/p2p.cpp Show resolved Hide resolved
src/esperanza/finalizationstate.cpp Show resolved Hide resolved
src/finalization/p2p.h Outdated Show resolved Hide resolved
src/finalization/state_storage.cpp Outdated Show resolved Hide resolved
scravy
scravy reviewed Feb 21, 2019
View reviewed changes
test/functional/rpc_getblocksnapshot.py Outdated Show resolved Hide resolved
src/finalization/state_storage.h Outdated Show resolved Hide resolved
@frolosofsky frolosofsky mentioned this pull request Feb 21, 2019
Merged
frolosofsky added a commit that referenced this pull request Feb 21, 2019
@frolosofsky
Make rpc_getblocksnapshot.py wait for event happen (#660)
10bb823 
Rewrite the functional test in a way to wait for snapshot events, because snapshots are processed on an independent thread.

Commit fixes flakiness in the functional tests we've discovered during working on the #634.

Signed-off-by: Stanislav Frolov <[email protected]>
@frolosofsky
ref-qualifier
fbdc67b 
Signed-off-by: Stanislav Frolov <[email protected]>
Gnappuraz
Gnappuraz requested changes Feb 22, 2019
View reviewed changes
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.

Sorry, but I want to give it one last good review before giving an approval.

scravy
scravy previously requested changes Feb 22, 2019
View reviewed changes
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.

GenerateBlockIndex in the tests leaks memory.

@frolosofsky
Copy link
Member Author

@scravy @Gnappuraz, I split this component into two: StateRepository and StateProcessor. Added additional tests for the StateRepository.

Please note, StateRepository doesn't provide get/set/remove interface. The main interface functions are Find, FindOrCreate, Confrim, and Trim. It's intentionally and it follows finalization states workflow. You can check corresponding finalization/state_repository_tests.cpp and finalization/state_processor_tests.cpp.

@frolosofsky
format
c74b041 
Signed-off-by: Stanislav Frolov <[email protected]>
@scravy scravy dismissed their stale review February 22, 2019 16:15

The leak was fixed by storing it in the vector, instead of just having an unmanaged ptr

@scravy scravy requested a review from kostyantyn February 22, 2019 16:15
@thothd thothd requested a review from Gnappuraz February 22, 2019 16:23
@scravy
Copy link
Member

scravy commented Feb 22, 2019

@kostyantyn Since this pull-request changed considerably and your utACK is quite stale after a few days and a bunch of commits I re-requested a review from you. Please have another look.

Gnappuraz
Gnappuraz approved these changes Feb 22, 2019
View reviewed changes
Copy link
Member

@Gnappuraz Gnappuraz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I honestly have some reservations on how the whole repository/processor are structured, I think we can do better but for the sake of moving forward let's continue and tackle this in an other venue.

utACK c74b041

src/finalization/state_repository.cpp
if (!ReadBlockFromDisk(block, index, chainparams.GetConsensus())) {
assert(not("Failed to read block"));
}
const bool ok = proc->ProcessNewTip(*index, block);
Copy link
Member

Choose a reason for hiding this comment

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

To me it does not make much sense that the repository is calling processing logic, it should be the other way around but let's move on and we'll restructure later.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Just from the naming of things that makes more sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

This will disappear after we restore states from the disk properly. There’s a comment addressed to this issue in the header file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can elaborate on this a bit. You're commenting on a function that restores the repository from the disk. Unfortunately, we don't have a proper serialization of finalization states that leads us to restore the states by processing blocks data (ProcessNewTip). Once we implement proper serialization of states, we will get rid of this dependency as well as ResetToTip trick.

scravy
scravy approved these changes Feb 23, 2019
View reviewed changes
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.

utACK 10f1441

I very much welcome our new component to the family :-)

thothd
thothd approved these changes Feb 23, 2019
View reviewed changes
Copy link
Member

@thothd thothd left a comment

Choose a reason for hiding this comment

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

utACK 10f1441

@frolosofsky frolosofsky merged commit 95461e2 into dtr-org:master Feb 24, 2019
@frolosofsky frolosofsky deleted the finalization-cache branch February 24, 2019 07:03
@frolosofsky frolosofsky mentioned this pull request Mar 1, 2019
Merged
@kostyantyn kostyantyn mentioned this pull request Mar 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ruteri Ruteri Ruteri left review comments

@scravy scravy scravy approved these changes

@Gnappuraz Gnappuraz Gnappuraz approved these changes

@thothd thothd thothd approved these changes

@AM5800 AM5800 Awaiting requested review from AM5800

@kostyantyn kostyantyn Awaiting requested review from kostyantyn

Assignees

@frolosofsky frolosofsky

Labels
finalization refactoring Changes which clean up code but don't change the user-visible behavior
Projects
None yet
Milestone
0.1
Development

Successfully merging this pull request may close these issues.
6 participants
@frolosofsky @scravy @kostyantyn @Gnappuraz @thothd @Ruteri