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

Create a SyncStatus helper type #2685

Merged
merged 4 commits into from
Aug 30, 2021

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Aug 27, 2021

Motivation

For some mempool tasks and to decide if new block commit advertisements should be sent, it is necessary to know if the synchronizer has finished and is at (or close to) the chain tip.

Solution

A helper type to determine if the synchronizer is close to the chain tip or not was created, and it has methods to check if it has likely reached the chain tip and to wait until it reaches the chain tip.

Review

@teor2345 was already following the development of this as part of the mempool crawler PR.

Reviewer Checklist

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

Follow Up Work

teor2345
teor2345 previously approved these changes Aug 27, 2021
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This looks good!

I think @upbqdn might be waiting to fill in is_close_to_tip for #2592, so let's try to get it merged within the next day or two.

All I think it needs is some temporary #[allow(dead_code)] to resolve the clippy errors.
(Feel free to admin merge or get anyone to approve after that's done.)

jvff added 3 commits August 28, 2021 14:34
Keeps track if the synchronizer is close to the chain tip or not.
Change the constructor API so that it returns a higher level construct.
Test if waiting for the chain tip to be reached correctly finishes when
the chain tip is reached. This is done by sending recent sync lengths to
the `SyncStatus` instance, and checking that every time a separate
`SyncStatus` instance determines it has reached the tip the original
instance wakes up.
@jvff
Copy link
Contributor Author

jvff commented Aug 28, 2021

I added a proptest to check if the SyncStatus was working properly when waiting for the tip to be reached. The test became a bit complicated, because there are two concurrent tasks working together, but I'm hoping it should help capture any regressions in the future.

I limited the number of proptest cases so that it runs in around 10 seconds locally. We could reduce this number further or reduce the EVENT_TIMEOUT if we want to speed it up.

@jvff jvff marked this pull request as ready for review August 28, 2021 14:38
The code added isn't used yet, so we'll add a temporary waiver until
another PR is merged to use them.
Comment on lines +17 to +20
/// The maximum time to wait for an event to be received.
///
/// If an event is not received in this time, it is considered that it will never be received.
const EVENT_TIMEOUT: Duration = Duration::from_millis(5);
Copy link
Contributor

Choose a reason for hiding this comment

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

5ms might be too short for heavily loaded VMs, but let's see how it goes.

Copy link
Contributor

@teor2345 teor2345 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, let's see how these tests go in CI.

@teor2345 teor2345 merged commit 83a2e30 into ZcashFoundation:main Aug 30, 2021
@jvff jvff deleted the add-sync-status-helper branch August 30, 2021 10:03
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.

2 participants