Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

im-online: use EstimateNextSessionRotation to get better estimates of session progress #8242

Merged
12 commits merged into from
Mar 12, 2021

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Mar 2, 2021

Currently im-online relies on the session duration to value to calculate the block at which the session is halfway through. When using BABE the session duration is defined in slots (not blocks), as such this method can lead to a high error in the estimate if a lot of slots are skipped. This PR adds a new method estimate_current_session_progress to the EstimateNextSessionRotation trait and makes the im-online pallet use it.

polkadot companion: paritytech/polkadot#2602

@andresilva andresilva added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Mar 2, 2021
@andresilva
Copy link
Contributor Author

I still need to add tests for this but wanted to get an initial validation of the approach. Namely that it may be too expensive to calculate a rational value for this when we're only interested in doing a check progress > 50%.

frame/support/src/traits.rs Outdated Show resolved Hide resolved
frame/session/src/lib.rs Outdated Show resolved Hide resolved
frame/babe/src/lib.rs Outdated Show resolved Hide resolved
frame/babe/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

@kianenigma
Copy link
Contributor

Ahh, and, is it possible to write a test for this in im-online?

@andresilva
Copy link
Contributor Author

Updated with everything that was suggested. Please approve the companion as well :)

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Thanks!

@andresilva
Copy link
Contributor Author

bot merge force

@ghost
Copy link

ghost commented Mar 12, 2021

Trying merge.

@ghost ghost merged commit da7ca4d into master Mar 12, 2021
@ghost ghost deleted the andre/im-online-skipped-slots branch March 12, 2021 11:50
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants