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

Update ICS 2, 3, 7 for hybrid delay period #552

Merged
merged 2 commits into from
Apr 6, 2021

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Mar 25, 2021

Closes #539

Split delay period into both a minimum time period which must have passed and a minimum number of blocks which must have passed in order to allow packets, acknowledgements, etc. to be processed.

Note that this breaks the connection handshake.

@cwgoes cwgoes requested a review from milosevic as a code owner March 25, 2021 12:06
@cwgoes cwgoes changed the title Update for hybrid delay period Update ICS 2, 3, 7 for hybrid delay period Mar 25, 2021
@cwgoes cwgoes added tao Transport, authentication, & ordering layer. from-review Feedback / alterations from specification review. labels Mar 25, 2021
@cwgoes cwgoes requested a review from AdityaSripal March 25, 2021 12:11
@milosevic
Copy link
Contributor

@cwgoes In general looks good. If I am not wrong, we need to remember also processedHeight in ics07, checkValidityAndUpdateState, the similar way we set processed time (set("clients/{identifier}/processedHeight/{header.height}", currentHeight()) and then use it in verify functions.

@cwgoes
Copy link
Contributor Author

cwgoes commented Apr 5, 2021

@cwgoes In general looks good. If I am not wrong, we need to remember also processedHeight in ics07, checkValidityAndUpdateState, the similar way we set processed time (set("clients/{identifier}/processedHeight/{header.height}", currentHeight()) and then use it in verify functions.

Ah yes, of course, I had the wrong height! Fixed.

Copy link
Contributor

@milosevic milosevic left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Chris!

Copy link
Member

@AdityaSripal AdityaSripal 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 correct to me.

I guess I'm still uncertain of why this change is needed.

@cwgoes cwgoes merged commit 0eba039 into master Apr 6, 2021
@cwgoes cwgoes deleted the cwgoes/hybrid-delay-period branch April 6, 2021 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from-review Feedback / alterations from specification review. tao Transport, authentication, & ordering layer.
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

Hybrid delayPeriod
3 participants