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

ledger: create catchpoint tracker #3085

Merged
merged 27 commits into from
Nov 4, 2021

Conversation

tsachiherman
Copy link
Contributor

Summary

Extract catchpoint logic out of the accounts update tracker.

Test Plan

Refactor existing unit tests.

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2021

Codecov Report

Merging #3085 (848dd28) into master (9b39c66) will decrease coverage by 0.15%.
The diff coverage is 52.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3085      +/-   ##
==========================================
- Coverage   47.34%   47.18%   -0.16%     
==========================================
  Files         364      365       +1     
  Lines       58296    58361      +65     
==========================================
- Hits        27599    27537      -62     
- Misses      27515    27631     +116     
- Partials     3182     3193      +11     
Impacted Files Coverage Δ
data/pools/transactionPool.go 43.20% <0.00%> (-0.33%) ⬇️
ledger/voters.go 52.30% <0.00%> (ø)
ledger/catchpointtracker.go 41.54% <41.54%> (ø)
ledger/acctupdates.go 64.82% <71.42%> (+0.47%) ⬆️
ledger/tracker.go 78.82% <81.11%> (+2.63%) ⬆️
ledger/ledger.go 63.12% <84.61%> (-0.14%) ⬇️
ledger/bulletin.go 93.54% <100.00%> (+0.21%) ⬆️
ledger/metrics.go 100.00% <100.00%> (ø)
ledger/notifier.go 88.57% <100.00%> (+0.33%) ⬆️
ledger/txtail.go 87.50% <100.00%> (+2.99%) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b39c66...848dd28. Read the comment docs.

@tsachiherman tsachiherman requested a review from cce October 18, 2021 21:12
@tsachiherman tsachiherman marked this pull request as ready for review October 18, 2021 21:12
@tsachiherman tsachiherman requested a review from cce October 20, 2021 16:12
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

I like the separation of the logic.
In the same time I'm a bit concerned with mixing commit data (offset, base) with context data into deferredCommitContext. I propose to keep them separate: let trackers contribute into commit data by modifying (offset, base) as needed but only setting/getting data from the context in order to have kind of "these data are set by au and used by ct"

@tsachiherman
Copy link
Contributor Author

I like the separation of the logic.
In the same time I'm a bit concerned with mixing commit data (offset, base) with context data into deferredCommitContext. I propose to keep them separate: let trackers contribute into commit data by modifying (offset, base) as needed but only setting/getting data from the context in order to have kind of "these data are set by au and used by ct"

I've broken a subset of deferredCommitContext into deferredCommitRange, so that each tracker could individually, during produceCommittingTask to create a offset/base. Once all the trackers are doing that, this data structure is available as part of the deferredCommitContext, allowing all the trackers to review it.

Copy link
Contributor

@algorandskiy algorandskiy 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. The only thing worries me is deferredCommitRange is modified by all trackers and there are no restrictions on offset/base change. Maybe some kind "protocol" on modifications is needed otherwise there is a chance some tracker can set the offset way too off for some other tracker. It looks like the range deferredCommitRange wants to be a type with some encapsulated behavior with max/min offset values for this round.

@tsachiherman
Copy link
Contributor Author

Looks good. The only thing worries me is deferredCommitRange is modified by all trackers and there are no restrictions on offset/base change. Maybe some kind "protocol" on modifications is needed otherwise there is a chance some tracker can set the offset way too off for some other tracker. It looks like the range deferredCommitRange wants to be a type with some encapsulated behavior with max/min offset values for this round.

I didn't want to be too restrictive on what can be done with the deferredCommitRange, as it feels like over engineering. Generally, we want only to narrow down the range as we move along, but given that we have only two parties that really care about that, I couldn't really justify that..

@tsachiherman tsachiherman merged commit f72764b into algorand:master Nov 4, 2021
@tsachiherman tsachiherman deleted the tsachi/catchpointtracker branch November 4, 2021 19:39
tsachiherman added a commit that referenced this pull request Nov 16, 2021
## Summary

When implementing the catchpoint tracker, I missed the re-initilization location for some of the local variables.
This would generate incorrect catchpoint labels after a node completes a fast-catchup.

#3085

## Test Plan

- [x] Add unit tests
- [x] Perform manual testing
@egieseke egieseke mentioned this pull request Nov 23, 2021
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.

4 participants