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

fix(libs/header/sync): Make ranges.Add thread-safe #1649

Merged
merged 3 commits into from
Jan 31, 2023

Conversation

tzdybal
Copy link
Member

@tzdybal tzdybal commented Jan 27, 2023

Overview

ranges.Add method is not thread safe. if you execute it in parallel you can end up with ranges out-of-order.
In syncer.newNetHead nothing prohibits parallel execution of pending.Add - syncing point is in wantSync using buffered channel.
newNetHead is called ass libp2p gossip validator, which is asynchronous - can be executed in parallel.

This change ensures that Add method is thread-safe.
Added test fails in first commit (to proof the point), and second commit fixes the issue.

Probably fixes #955. My gut feeling is that is also may fix other, more-nuanced, non-panicing sync issues.

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@tzdybal tzdybal changed the title Tzdybal/fix ranges fix: make ranges.Add thread-safe Jan 27, 2023
@tzdybal tzdybal added the kind:fix Attached to bug-fixing PRs label Jan 27, 2023
@tzdybal tzdybal force-pushed the tzdybal/fix_ranges branch from 969eda7 to cde501d Compare January 27, 2023 09:56
@Wondertan
Copy link
Member

Can you try to run the reconstruction swamp teat locally with -count 100 to see if it's really fixes it?

Good catch anyway!

@renaynay renaynay changed the title fix: make ranges.Add thread-safe fix(libs/header/sync): Make ranges.Add thread-safe Jan 27, 2023
@renaynay renaynay added the area:header Extended header label Jan 27, 2023
renaynay
renaynay previously approved these changes Jan 27, 2023
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

Looks fine to me - am running it against arabica where a lot of these issues were observed and looks good so far.

Thanks for the fix @tzdybal

@Wondertan Wondertan merged commit 6f73441 into celestiaorg:main Jan 31, 2023
renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:header Extended header kind:fix Attached to bug-fixing PRs
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

node/tests: panic during sync(happens only in tests)
3 participants