Skip to content

Add cache of finalization states #429

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
Merged
frolosofsky merged 13 commits into from
Jan 16, 2019
Merged

Add cache of finalization states #429

frolosofsky merged 13 commits into from
Jan 16, 2019

Conversation

@frolosofsky
Copy link
Member

@frolosofsky frolosofsky commented Jan 10, 2019 •
edited
Loading

This PR prepares finalization state for commits based full sync implementation. There are some interconnected changes:

  • Use individual finalization state per block index (it's still possible to process several blocks by single state though, for keeping old tests, mostly);
  • Add cache of finalization states. It stores a small window of unfinalized states only;
  • Split ProcessNewTip on several functions: ProcessNewTip, ProcessNewCommits, ProcessNewCommit;
  • Cache also supposed to fix Starting node with -reindex does not work #421 (or a major part of it).

Implementation of finalization states restoring isn't final. Currently, it restores from blocks on the disk. It's slow for large blockchains but still fine for our tests. I'm about to implement proper finalization states serialization on every checkpoint with ability to fall back to reading blocks for any missing data. For example, let's restore finalization state of the following blockchain:

b0 - b1 - ... - b49 - b50 - b51

b49 is a checkpoint and we will have serialized finalization state for it. The algorithm is:

  1. Restore state for b49 from serialized data,
  2. Restore state for b50 and b51 from blocks.

It's only one of possible solutions. Let's discuss it the #432.

Signed-off-by: Stanislav Frolov stanislav@thirdhash.com

@frolosofsky frolosofsky added refactoring Changes which clean up code but don't change the user-visible behavior wip Work in progress which is not supposed to be merged yet finalization labels Jan 10, 2019
@frolosofsky frolosofsky self-assigned this Jan 10, 2019
@frolosofsky
Copy link
Member Author

frolosofsky commented Jan 10, 2019 •
edited
Loading

Snapshot functional tests fail as they try to fork before last finalized checkpoint. @kostyantyn and I are looking at it — a reason for WIP.

frolosofsky
frolosofsky commented Jan 10, 2019
View reviewed changes
src/esperanza/finalizationstate.cpp Outdated
Copy link
Member Author

@frolosofsky frolosofsky Jan 10, 2019

Choose a reason for hiding this comment

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

This diff looks like a mess, however, it's only code moving here. The split view can help a bit.

frolosofsky
frolosofsky commented Jan 10, 2019
View reviewed changes
src/test/esperanza/finalizationstate_tests.cpp Outdated
Copy link
Member Author

@frolosofsky frolosofsky Jan 10, 2019

Choose a reason for hiding this comment

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

I've removed this test as it checks C++ standard behavior.

frolosofsky
frolosofsky commented Jan 10, 2019
View reviewed changes
src/test/validation_block_tests.cpp Outdated
Copy link
Member Author

@frolosofsky frolosofsky Jan 10, 2019

Choose a reason for hiding this comment

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

This test can randomly create forks behind last finalization point, workaround it (fallback to default bitcoin behavior).

frolosofsky
frolosofsky commented Jan 10, 2019
View reviewed changes
test/functional/feature_block.py Outdated
Copy link
Member Author

@frolosofsky frolosofsky Jan 10, 2019

Choose a reason for hiding this comment

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

whitelist is default option for ComparisonTestFramework

Gnappuraz
Gnappuraz previously requested changes Jan 11, 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.

I really don't think that reading every time the whole blockchain from disk is a good idea, my intuition is that it will make startup time grow linearly with the blockchain and thus potentially taking a long time with disk being a big bottleneck.

src/esperanza/finalizationstate.cpp Outdated
Copy link
Member

@Gnappuraz Gnappuraz Jan 11, 2019

Choose a reason for hiding this comment

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

Here it seems to me is more a reset to Genesis than a reset to anything else.

Copy link
Member Author

@frolosofsky frolosofsky Jan 11, 2019

Choose a reason for hiding this comment

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

It resets tip's finalization to the fresh state.

Copy link
Member Author

@frolosofsky frolosofsky Jan 11, 2019

Choose a reason for hiding this comment

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

In fact, it's an incorrect and temporary solution until I implement full-sync based on commits, and this PR is a base for that. I don't want to present too large PR at once.

@frolosofsky
Copy link
Member Author

frolosofsky commented Jan 11, 2019

@Gnappuraz I understand your concern. I'm about to implement finalization states serialization on every checkpoint, but it would be a different PR, and anyway I think to fall back to reading the whole blockchain in case of serialized data isn't presented for any reason.

This implementation allows passing all the tests and works fine for regtest. I'd appreciate if you remove your reject as I don't want to mix too many changes in a single PR. I will add serialization next.

@frolosofsky frolosofsky requested review from a team, AM5800, Gnappuraz and scravy January 11, 2019 10:53
@thothd
Copy link
Member

thothd commented Jan 11, 2019

@Gnappuraz I understand your concern. I'm about to implement finalization states serialization on every checkpoint, but it would be a different PR, and anyway I think to fall back to reading the whole blockchain in case of serialized data isn't presented for any reason.

This implementation allows passing all the tests and works fine for regtest. I'd appreciate if you remove your reject as I don't want to mix too many changes in a single PR. I will add serialization next.

@frolosofsky, what is passing tests on a new code enabling us? if we know it's not the solution we wanna have, I mean it's not that master is currently red and we're looking for a quick fix.
also by looking at the PR it's not clear for me that there was supposed to be a next step of reading the serialized state from disk other than from the blocks

@frolosofsky
Copy link
Member Author

frolosofsky commented Jan 11, 2019

@Gnappuraz The primary purpose of this PR is adding finalization states cache. Once I implemented it I faced with the similar issue as described in #421: if node starts with some blocks presented on the disk, the cache is empty for that blocks, and it's impossible to accept new blocks further. So that, I implemented restoring finalization state from presented blocks.

And I'd point it one more time. After we implement proper serialization of finalization states, I think to keep the possibility to fall back to restoring finalization states from blocks on disk for case serialized data isn't presented for any reason.

Sure, I missed this description in the PR, updated.

@frolosofsky frolosofsky mentioned this pull request Jan 11, 2019
Closed
@Gnappuraz Gnappuraz dismissed their stale review January 11, 2019 13:57

Removing for the sake of moving forward and with the conditional that a proper solution for handling state serialization and recovery to/from disk

@kostyantyn kostyantyn self-assigned this Jan 15, 2019
frolosofsky and others added 9 commits January 15, 2019 14:58
@frolosofsky
Add cache of finalization states per block index older than last fina…
c303a5f 
…lized checkpoint

Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
@frolosofsky
Use TestingSetup for every finalization test
f93b3e9 
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
@frolosofsky
lock cs_main
cb7d1f4 
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
@frolosofsky
fix wallet_listsinceblock
4c13d93 
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
@frolosofsky
fix rpc_invalidateblock.py
a5adf00 
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
@frolosofsky
fix p2p_fingerprint.py
ade5e56 
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
@frolosofsky
use cblockindex as a key
151793d 
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
@frolosofsky
Fix feature_snapshot.py test
b720acf 
Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
@frolosofsky
Update feature_snapshot_creation test
d036d26 
Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
Refactoring tests
354cb6d 
Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
kostyantyn
kostyantyn reviewed Jan 15, 2019
View reviewed changes
kostyantyn
kostyantyn reviewed Jan 15, 2019
View reviewed changes
kostyantyn
kostyantyn reviewed Jan 15, 2019
View reviewed changes
kostyantyn
kostyantyn reviewed Jan 15, 2019
View reviewed changes
@frolosofsky
restoring api
2ec543d 
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
kostyantyn
kostyantyn reviewed Jan 15, 2019
View reviewed changes
scravy
scravy reviewed Jan 15, 2019
View reviewed changes
kostyantyn
kostyantyn reviewed Jan 15, 2019
View reviewed changes
@frolosofsky
nits
b3ebccb 
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
@frolosofsky frolosofsky removed the wip Work in progress which is not supposed to be merged yet label Jan 15, 2019
@frolosofsky frolosofsky requested a review from a team January 15, 2019 15:30
@frolosofsky
Copy link
Member Author

frolosofsky commented Jan 15, 2019

A lot of thanks to @kostyantyn for fixing snapshot tests.

kostyantyn
kostyantyn reviewed Jan 15, 2019
View reviewed changes
kostyantyn
kostyantyn reviewed Jan 15, 2019
View reviewed changes
@frolosofsky
renaming, renaming
60e7a70 
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
@frolosofsky frolosofsky mentioned this pull request Jan 15, 2019
Merged
@frolosofsky frolosofsky requested a review from a team January 15, 2019 16:31
kostyantyn
kostyantyn approved these changes Jan 15, 2019
View reviewed changes
Copy link
Member

@kostyantyn kostyantyn left a comment

Choose a reason for hiding this comment

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

utACK 60e7a70

Gnappuraz
Gnappuraz approved these changes Jan 16, 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.

utACK 60e7a70

@frolosofsky frolosofsky merged commit 3d9a195 into dtr-org:master Jan 16, 2019
@frolosofsky frolosofsky deleted the fin-state branch January 16, 2019 14:39
@frolosofsky frolosofsky mentioned this pull request Jan 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scravy scravy scravy left review comments

@kostyantyn kostyantyn kostyantyn approved these changes

@Gnappuraz Gnappuraz Gnappuraz approved these changes

@AM5800 AM5800 Awaiting requested review from AM5800

Assignees

@kostyantyn kostyantyn

@frolosofsky frolosofsky

Labels

finalization refactoring Changes which clean up code but don't change the user-visible behavior

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@frolosofsky @thothd @scravy @kostyantyn @Gnappuraz