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

Decide if Zebra is at the chain tip #2722

Merged
merged 9 commits into from
Sep 6, 2021
Merged

Decide if Zebra is at the chain tip #2722

merged 9 commits into from
Sep 6, 2021

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Sep 1, 2021

Motivation

Zebra needs a way to tell when it has reached the chain tip.

Closes #2592.

Solution

This PR simply calculates the average recent sync length, and returns true if this value falls below 20. This constant (20) is based on #2592 (comment).

Reviewer Checklist

@zfnd-bot zfnd-bot bot assigned upbqdn Sep 1, 2021
@upbqdn upbqdn requested review from jvff and teor2345 September 1, 2021 20:31
@upbqdn upbqdn added the A-rust Area: Updates to Rust code label Sep 1, 2021
@upbqdn
Copy link
Member Author

upbqdn commented Sep 1, 2021

The function is_close_to_tip currently returns true even when there are too many zero values returned by RecentSyncLengths. I left it this way because I'm not sure how RecentSyncLengths really behaves when Zebra is synchronized.

Also note that is_close_to_tip can return true only if there is no value greater than 80 in the list of recent sync lengths since Average([80, 0, 0, 0]) = 20.

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 is a good minimal implementation.
We can tweak the constants later during testing, if needed.

It looks like the waits_until_close_to_tip test is failing because it assumes that a random series of sync lengths will always be close to the tip. (Which isn't true.)

And in this PR we actually check the lengths, so the test fails.

@jvff should be able to help out here.

@oxarbitrage
Copy link
Contributor

I think this looks pretty good. Should we create a specific test for it or our current tests covers it ?

@teor2345
Copy link
Contributor

teor2345 commented Sep 2, 2021

I think this looks pretty good. Should we create a specific test for it or our current tests covers it ?

I think the proptest is good, but I'd also like us to test some test vectors.

For example:

  1. totally empty sync lengths array is not near tip
  2. sync lengths array with all zeroes is near tip
  3. sync lengths array with all 500s is not near tip

Those are 3 basic tests that should keep on working, even if we tweak the algorithm or constants.

Copy link
Contributor

@jvff jvff 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! If you need any help adding the test vectors, let me know 👍

@upbqdn upbqdn requested a review from oxarbitrage September 6, 2021 20:40
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

make clippy happy.

zebrad/src/components/sync/status/tests.rs Outdated Show resolved Hide resolved
zebrad/src/components/sync/status/tests.rs Outdated Show resolved Hide resolved
zebrad/src/components/sync/status/tests.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Tests look good to me, nice job.

@oxarbitrage oxarbitrage enabled auto-merge (squash) September 6, 2021 23:01
@oxarbitrage oxarbitrage merged commit 1c4ac18 into main Sep 6, 2021
@oxarbitrage oxarbitrage deleted the mempool-activation branch September 6, 2021 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Activate Mempool storage based on a list of recent syncer obtain/extend lengths
4 participants