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

Jump to bottom

transition cache leak during ledger catchup #2168

Merged
merged 39 commits into from Apr 19, 2019
Merged

transition cache leak during ledger catchup #2168

merged 39 commits into from Apr 19, 2019

Conversation

ghost-not-in-the-shell
Copy link
Member

@ghost-not-in-the-shell ghost-not-in-the-shell commented Apr 5, 2019 •
edited

  1. Currently, if transitions received from peers didn't pass validation, those transitions are not getting invalidated in unprocessed_transition_cache.
  2. In current implementation ledger_catchup would be blocked by transitions in cache, and the catchup would fail. This is super inefficient.
  3. The subtree of catchup_scheduler that passed to ledger_catchup is not used effectively; besides instead of passing just 1 subtree, we should pass a forest.

This PR try to fix the above problems.

@wu-s-john wu-s-john self-requested a review April 9, 2019 15:20
wu-s-john
wu-s-john suggested changes Apr 9, 2019
View reviewed changes
src/lib/cache_lib/intf.ml Outdated Show resolved Hide resolved
src/lib/cache_lib/impl.ml Outdated Show resolved Hide resolved
src/lib/cache_lib/impl.ml Outdated Show resolved Hide resolved
src/lib/ledger_catchup/ledger_catchup.ml Outdated Show resolved Hide resolved
src/lib/ledger_catchup/ledger_catchup.ml Outdated Show resolved Hide resolved
src/lib/ledger_catchup/inputs.ml Show resolved Hide resolved
src/lib/transition_handler/validator.ml Show resolved Hide resolved
@ghost-not-in-the-shell
fixing unprocessed_transition_cache leak during bootstrap
41f2d3b
@ghost-not-in-the-shell
minor fix
dfaf420
@ghost-not-in-the-shell
Merge branch 'master' of github.com:CodaProtocol/coda into fix/ledger…
d9ff9b7 
…-catchup-be-aware-of-cache
@ghost-not-in-the-shell
changed consumed to be transformed in cache_lib/impl.ml
469c49e
@ghost-not-in-the-shell
remove unused type in signature
d102e5d
@ghost-not-in-the-shell
Merge branch 'master' of github.com:CodaProtocol/coda into fix/ledger…
9a9c384 
…-catchup-be-aware-of-cache
@ghost-not-in-the-shell
Merge branch 'master' into fix/ledger-catchup-be-aware-of-cache
3254118
@ghost-not-in-the-shell
Merge branch 'master' of github.com:CodaProtocol/coda into fix/ledger…
bde7f93 
…-catchup-be-aware-of-cache
@ghost-not-in-the-shell
refactor both ledger_catchup and catchup_scheduler to eliminate d…
b41e8db 
…uplicate logic
@ghost-not-in-the-shell
Merge branch 'master' of github.com:CodaProtocol/coda into fix/ledger…
d1b3937 
…-catchup-be-aware-of-cache
@ghost-not-in-the-shell
Merge branch 'master' of github.com:CodaProtocol/coda into fix/ledger…
d5ec764 
…-catchup-be-aware-of-cache
@ghost-not-in-the-shell
refactor the catchup_scheduler to use breadcrumb_builder
1fbc64e
@ghost-not-in-the-shell
Merge branch 'fix/ledger-catchup-be-aware-of-cache' of github.com:Cod…
6ff9457 
…aProtocol/coda into fix/ledger-catchup-be-aware-of-cache
@ghost-not-in-the-shell ghost-not-in-the-shell changed the title [WIP] transition cache leak during ledger catchup transition cache leak during ledger catchup Apr 15, 2019
wu-s-john
wu-s-john suggested changes Apr 17, 2019
View reviewed changes
Copy link
Contributor

@wu-s-john wu-s-john left a comment

Choose a reason for hiding this comment

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

The PR looks good. Just have a question about test_ledger_catchup.ml Also, is it possible to dedup the tests you wrote in test_ledger_catchup?

src/lib/transition_frontier_controller_tests/test_ledger_catchup.ml Outdated Show resolved Hide resolved
@wu-s-john
Copy link
Contributor

Also, after I approve this PR, can you update the documentation for Ledger_catchup? It's slightly different after this PR update.

@nholland94
Copy link
Member

I'm trying to understand the whole final_state stuff that was added to the cache stuff. It seems like the only way it's used is just in a single test? If that's the case, is adding this really necessary? This adds a pretty good deal of complexity to the abstraction, and some of the changes made me question whether or not the cache can leak now, although I need to look closer and understand it better. Just some initial questions upon starting reviewing.

nholland94
nholland94 previously requested changes Apr 18, 2019
View reviewed changes
Copy link
Member

@nholland94 nholland94 left a comment

Choose a reason for hiding this comment

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

Looks good. Just had some minor nits, mostly for readability.

src/lib/cache_lib/impl.ml Show resolved Hide resolved
src/lib/cache_lib/impl.ml Show resolved Hide resolved
src/lib/transition_handler/validator.ml Show resolved Hide resolved
@ghost-not-in-the-shell ghost-not-in-the-shell dismissed nholland94’s stale review April 19, 2019 21:58

I already addressed those in my PR

@ghost-not-in-the-shell ghost-not-in-the-shell merged commit 1bc7fdd into master Apr 19, 2019
@ghost-not-in-the-shell ghost-not-in-the-shell deleted the fix/ledger-catchup-be-aware-of-cache branch April 19, 2019 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-s-john wu-s-john wu-s-john approved these changes

@nholland94 nholland94 nholland94 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ghost-not-in-the-shell @wu-s-john @nholland94