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 syncer download order and add sync tests #3168

Merged
merged 20 commits into from
Jan 11, 2022
Merged

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Dec 7, 2021

Motivation

We want the syncer to download blocks in chain order, as documented.

We want to make sure that the syncer correctly drops blocks that are a long way ahead of the state tip.
This is a follow-up test for PR #3167 for ticket #1389.

Solution

ChainSync bug fixes:

  • Download synced blocks in chain order, not arbitrary HashSet order
    • This adds a direct dependency on indexmap (multiple Zebra dependencies already use indexmap)
  • Increase the syncer lookahead height limit, because in-order syncs are more efficient
  • Spawn the syncer into its own task - closes Spawn the syncer as an async task #3114

Extra Tests:

  • Add ChainSync tests for obtain_tips, extend_tips, and wrong block hashes
  • Add block too high tests for obtain_tips and extend_tips
  • Add ChainSync tests for duplicate FindBlocks response hashes

Make some futures std::marker::Send or std::marker::Sync, so they can be spawned:

  • Refactor so that RetryLimit::Future is std::marker::Sync
  • Make the ChainSync::sync future std::marker::Send by spawning tips futures

Improve test harnesses:

  • Improve MockService failure messages
  • Add closure-based responses to the MockService API
  • Move MockChainTip to zebra-chain
  • Add a MockChainTipSender type alias
  • Support MockChainTip in ChainSync and its downloader

Review

I'm not sure who is around to review this PR, maybe @upbqdn?

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

@teor2345 teor2345 added C-enhancement Category: This is an improvement P-Medium labels Dec 7, 2021
@teor2345 teor2345 self-assigned this Dec 7, 2021
@teor2345 teor2345 changed the base branch from main to drop-far-future-blocks December 9, 2021 04:39
@teor2345 teor2345 force-pushed the drop-far-future-blocks branch 3 times, most recently from 95a329e to 03e8299 Compare December 16, 2021 04:47
@teor2345 teor2345 force-pushed the drop-far-future-blocks branch from 03e8299 to 8d2a425 Compare December 17, 2021 00:44
Base automatically changed from drop-far-future-blocks to main December 17, 2021 16:31
@teor2345 teor2345 force-pushed the sync-height-limit-test branch from f4635ee to fb2053e Compare December 31, 2021 08:06
@teor2345 teor2345 added A-dependencies Area: Dependency file updates A-diagnostics Area: Diagnosing issues or monitoring performance A-network Area: Network protocol updates or fixes C-testing Category: These are tests and removed C-enhancement Category: This is an improvement labels Dec 31, 2021
@teor2345 teor2345 force-pushed the sync-height-limit-test branch 2 times, most recently from 367b5af to 20a3e47 Compare December 31, 2021 08:12
@codecov
Copy link

codecov bot commented Dec 31, 2021

Codecov Report

Merging #3168 (508b3ea) into main (4c310c0) will increase coverage by 0.81%.
The diff coverage is 59.63%.

@@            Coverage Diff             @@
##             main    #3168      +/-   ##
==========================================
+ Coverage   77.30%   78.11%   +0.81%     
==========================================
  Files         265      266       +1     
  Lines       31415    31455      +40     
==========================================
+ Hits        24284    24572     +288     
+ Misses       7131     6883     -248     

@teor2345 teor2345 changed the title WIP: Test syncer block height lookahead limits Fix syncer download order and add sync tests Jan 3, 2022
@teor2345 teor2345 added C-bug Category: This is a bug I-slow Problems with performance or responsiveness labels Jan 3, 2022
@teor2345 teor2345 force-pushed the sync-height-limit-test branch 2 times, most recently from c08fbe7 to 53f2e20 Compare January 3, 2022 03:25
@teor2345 teor2345 marked this pull request as ready for review January 3, 2022 03:25
@teor2345 teor2345 force-pushed the sync-height-limit-test branch from 53f2e20 to 592dda5 Compare January 3, 2022 07:57
@teor2345 teor2345 enabled auto-merge (squash) January 3, 2022 07:57
@teor2345 teor2345 force-pushed the sync-height-limit-test branch from 113e717 to 19e7caa Compare January 4, 2022 00:25
@teor2345 teor2345 requested review from upbqdn and removed request for dconnolly and oxarbitrage January 5, 2022 21:37
@teor2345
Copy link
Contributor Author

teor2345 commented Jan 6, 2022

+ Coverage   77.18%   77.90%   +0.72%     

This is a really nice test coverage increase.

@mpguerra mpguerra requested a review from jvff January 11, 2022 08:52
Copy link
Collaborator

@conradoplg conradoplg 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 tests are great.

There are two muts that I don't understand why they're needed, but it's not a big deal. I'm approving this and we can take a look later if needed.

@@ -740,7 +782,7 @@ where
}
}

fn update_metrics(&self) {
fn update_metrics(&mut self) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member

Choose a reason for hiding this comment

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

If I understand it correctly, those muts make the compiler aware that update_metrics has the sole access to self. This is needed in a multi-threaded environment, but I can't remember why. I had a discussion about this with Teor.

@@ -395,7 +408,7 @@ where
}

/// Get the number of currently in-flight download tasks.
pub fn in_flight(&self) -> usize {
pub fn in_flight(&mut self) -> usize {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

@teor2345 teor2345 merged commit d076b99 into main Jan 11, 2022
@teor2345 teor2345 deleted the sync-height-limit-test branch January 11, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates A-diagnostics Area: Diagnosing issues or monitoring performance A-network Area: Network protocol updates or fixes C-bug Category: This is a bug C-testing Category: These are tests I-slow Problems with performance or responsiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spawn the syncer as an async task
4 participants