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(sync): enforce fork len when changing head #5244

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

schomatis
Copy link
Contributor

Similar to #5182 this PR is theoretically protocol breaking but the only case of interest is when we are working with forks close to finality threshold (which is orders of magnitude away from normal forks observed in the chain).


Most of the syncing logic seems to assume the "heaviest" tipset it receives as input is actually the head of our synced chain even if the lock is released. This is mostly innocuous and will be handled in follow-up PRs (to make this more explicit), but there are cases when we actually want to hold the lock while doing certain checks that are not related to the validity/integrity of a chain (either heaviest or not), namely how far away it is from the actual head we will be swapping it for (there may be other cases we should look into).

@schomatis schomatis added area/chain Area: Chain impact/consensus Impact: Consensus labels Dec 22, 2020
@schomatis schomatis self-assigned this Dec 22, 2020
@schomatis schomatis force-pushed the schomatis/fix/enforce-fork-len branch from d7a8445 to 378af46 Compare December 22, 2020 17:26
@schomatis schomatis requested a review from Kubuxu December 22, 2020 17:30
@schomatis schomatis marked this pull request as ready for review December 22, 2020 17:33
@schomatis
Copy link
Contributor Author

Failing flaky TestSyncBadTimestamp #4624.

@schomatis schomatis merged commit 69eb216 into master Jan 18, 2021
@schomatis schomatis deleted the schomatis/fix/enforce-fork-len branch January 18, 2021 15:34
@Kubuxu
Copy link
Contributor

Kubuxu commented Jan 18, 2021

Uff, IDK how I feel about merging this after RC is already out.
@magik6k your call.

@schomatis
Copy link
Contributor Author

I'm fine reverting but someone needs to own that this does get merged eventually please.

bibibong pushed a commit to EpiK-Protocol/go-epik that referenced this pull request Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/chain Area: Chain impact/consensus Impact: Consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants