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

Improve coordinator time #1014

Open
AdityaSripal opened this issue Feb 25, 2022 · 2 comments
Open

Improve coordinator time #1014

AdityaSripal opened this issue Feb 25, 2022 · 2 comments
Labels
needs discussion Issues that need discussion before they can be worked on nice-to-have testing Testing package and unit/integration tests

Comments

@AdityaSripal
Copy link
Member

Currently the blocktime does not advance at all on NextBlock. It is only incremented by the coordinator so that it can be incremented on all testchains at the same time.

Thus, the testing suite keeps track of a global time. This is to prevent errors on UpdateClient when the update header time is ahead of the block time. However, the use of a global clock is inelegant and does not simulate the increasing time of real blockchains.

It would be ideal for chains to increment their time each block by a set amount, and then ensure the header from future bug does not occur some other way.

@AdityaSripal AdityaSripal added testing Testing package and unit/integration tests nice-to-have labels Feb 25, 2022
@colin-axner
Copy link
Contributor

With the update to ABCI++ #3707, we could replace the managed coordinator time with usage of time.Now(). We would need to still devise a way for chains to magically fast forward the clock time if they wanted to test scenarios in the future (like for gov proposals)

In the case where we use time.Now(), before committing and executing the current block, we can update the header time to be the current time. I think this would to occur regardless for globally managed time

Alternatively, instead of time.Now and a coordinator, we could simply keep the global time and allow chains to draw from it without going through a coordinator (maybe have a pointer to a time object which is given to each chain). We would likely still need to update the header before block commitment/execution

One thing I haven't determined yet is if there will be side affects between block proposal and execution where the context may reference a time which will be changed upon execution

@colin-axner
Copy link
Contributor

I think maybe the improvement here would be to turn the current time object in the coordinator into a pointer to which chains receive access. Upon executing a block, they can then increment the global time. As stated above, they will likely need to refresh the time in their header before committing/executing a block

CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this issue Nov 6, 2023
…osmos#1014)

<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.
-->

## Overview

Resolves cosmos#1005. resolves cosmos#1021 

<!-- 
Please provide an explanation of the PR, including the appropriate
context,
background, goal, and rationale. If there is an issue with this
information,
please provide a tl;dr and link the issue. 
-->

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords

---------

Co-authored-by: Matthew Sevey <[email protected]>
Co-authored-by: Ganesha Upadhyaya <[email protected]>
@crodriguezvega crodriguezvega moved this to Backlog in ibc-go Nov 6, 2023
@colin-axner colin-axner added the needs discussion Issues that need discussion before they can be worked on label Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Issues that need discussion before they can be worked on nice-to-have testing Testing package and unit/integration tests
Projects
Status: Backlog 🕐
Development

No branches or pull requests

2 participants