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): address some minor issues in block sync #3104

Closed
wants to merge 1 commit into from

Conversation

bowenwang1996
Copy link
Collaborator

Address a couple issues related to block sync:

  • blocks_received is incorrectly calculated. It should not increase when we do not receive any new blocks, but currently if some heights are skipped and we accept a block that was previously orphaned, it does increase.
  • The timeout for progress is too short. This is not a problem per se. However, every time block sync is due we recompute the hashes of blocks that we need to sync. When the epoch is long and we just start block sync, this could mean going through 40k blocks every 1-2s, which is fairly suboptimal. The proper fix would be to implement caching for the requested hashes and do not recompute every time block sync is due, but given that we plan to rewrite block sync in the future, I don't see the need to change it now.

Test plan

  • test_blocks_received.

@gitpod-io
Copy link

gitpod-io bot commented Aug 7, 2020

@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #3104 into master will decrease coverage by 0.52%.
The diff coverage is 94.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3104      +/-   ##
==========================================
- Coverage   87.94%   87.42%   -0.53%     
==========================================
  Files         212      212              
  Lines       42976    41247    -1729     
==========================================
- Hits        37795    36059    -1736     
- Misses       5181     5188       +7     
Impacted Files Coverage Δ
chain/chain/tests/sync_chain.rs 89.74% <90.90%> (+1.50%) ⬆️
chain/chain/src/chain.rs 88.62% <100.00%> (-0.69%) ⬇️
chain/client/src/sync.rs 87.15% <100.00%> (+0.48%) ⬆️
runtime/runtime/src/actions.rs 79.02% <0.00%> (-5.80%) ⬇️
chain/client/src/test_utils.rs 87.45% <0.00%> (-4.07%) ⬇️
chain/client/src/types.rs 63.75% <0.00%> (-3.75%) ⬇️
runtime/runtime/src/lib.rs 87.90% <0.00%> (-3.62%) ⬇️
chain/client/src/info.rs 68.99% <0.00%> (-3.11%) ⬇️
chain/chain/src/types.rs 92.25% <0.00%> (-2.36%) ⬇️
neard/src/runtime.rs 86.34% <0.00%> (-2.18%) ⬇️
... and 22 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 9ca3117...66f6c1c. Read the comment docs.

@bowenwang1996
Copy link
Collaborator Author

superseded by #3241

@Ekleog-NEAR Ekleog-NEAR deleted the fix-block-sync-issue branch March 29, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant