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

[reconciler] Fix Pruning Race #217

Merged
merged 17 commits into from
Nov 3, 2020
Merged

[reconciler] Fix Pruning Race #217

merged 17 commits into from
Nov 3, 2020

Conversation

patrick-ogrady
Copy link
Contributor

@patrick-ogrady patrick-ogrady commented Oct 31, 2020 •
edited
Loading

This PR fixes a potential race condition in the optimistic balance pruning logic. This was observed in our own testing when syncing and reconciling with high concurrency.

Changes

  • Reproduce race condition with a test
  • Update backlog default to 250k (some networks have congestion spikes)
  • Fix race condition
  • Add test with multiple changes for same account at same index (different block hashes)
  • Increase reconciler safe depth
  • Test happy path pruning with 2 calls
  • add BST implementation to get fast lookup when popular *types.AccountCurrency (instead of sorting all keys in a map)

@coveralls
Copy link

coveralls commented Oct 31, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 10626

  • 136 of 149 (91.28%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 79.09%
Changes Missing Coverage Covered Lines Changed/Added Lines %
reconciler/reconciler.go 63 65 96.92%
utils/bst.go 73 84 86.9%
Files with Coverage Reduction New Missed Lines %
reconciler/reconciler.go 2 84.43%
Totals Coverage Status
Change from base Build 10464: 0.2%
Covered Lines: 7334
Relevant Lines: 9273
💛 - Coveralls

yfl92
yfl92 reviewed Nov 2, 2020
View reviewed changes
reconciler/reconciler.go Outdated Show resolved Hide resolved
yfl92
yfl92 reviewed Nov 2, 2020
View reviewed changes
reconciler/reconciler.go Outdated Show resolved Hide resolved
yfl92
yfl92 reviewed Nov 2, 2020
View reviewed changes
reconciler/reconciler.go Outdated Show resolved Hide resolved
yfl92
yfl92 reviewed Nov 2, 2020
View reviewed changes
reconciler/reconciler.go Outdated Show resolved Hide resolved
qiwu7
qiwu7 reviewed Nov 3, 2020
View reviewed changes
utils/bst.go Outdated Show resolved Hide resolved
qiwu7
qiwu7 reviewed Nov 3, 2020
View reviewed changes
reconciler/types.go Outdated Show resolved Hide resolved
qiwu7
qiwu7 previously approved these changes Nov 3, 2020
View reviewed changes
yfl92
yfl92 previously approved these changes Nov 3, 2020
View reviewed changes
@patrick-ogrady patrick-ogrady dismissed stale reviews from yfl92 and qiwu7 via 88997ae November 3, 2020 19:50
@patrick-ogrady
Copy link
Contributor Author

Resolved @qiwu7's nits! Ready for review @yflinn and @qiwu7 !

@patrick-ogrady patrick-ogrady merged commit 3a72060 into master Nov 3, 2020
@patrick-ogrady patrick-ogrady deleted the patrick/reconciler-prune-race branch November 3, 2020 22:00
This was referenced Nov 3, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yfl92 yfl92 yfl92 approved these changes

@arjun-io arjun-io Awaiting requested review from arjun-io

@qiwu7 qiwu7 Awaiting requested review from qiwu7

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@patrick-ogrady @coveralls @yfl92 @qiwu7