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.

Sign up for GitHub

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

Jump to bottom

Revert "Commit timeout (#2057)" #2759

Merged
merged 3 commits into from
Sep 17, 2022
Merged

Revert "Commit timeout (#2057)" #2759

merged 3 commits into from
Sep 17, 2022

Conversation

czarcas7ic
Copy link
Member

@czarcas7ic czarcas7ic commented Sep 15, 2022 •
edited
Loading

This reverts commit 701b391.

Closes: #XXX

What is the purpose of the change

The PR from #2057 reduced time timeout to two seconds for all nodes, regardless of config settings. After discussion, we decided this is not desirable. I did leave the change to init.go so that any future nodes initialized will default to 2 (but can overwrite via their config file)

Testing and Verifying

This change is already covered by existing tests

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? no
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
  • How is the feature or change documented? not applicable

@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Sep 15, 2022
@czarcas7ic czarcas7ic added the A:backport/v12.x backport patches to v12.x branch label Sep 15, 2022
@czarcas7ic czarcas7ic marked this pull request as ready for review September 15, 2022 21:44
@czarcas7ic czarcas7ic requested a review from a team September 15, 2022 21:44
@faddat
Copy link
Member

faddat commented Sep 16, 2022

Interesting!

What's the thought here?

@czarcas7ic
Copy link
Member Author

czarcas7ic commented Sep 16, 2022 •
edited
Loading

Hey @faddat, sorry this was a last minute change from a discussion this morning. The change in its previous form would force all validators to run with the 2 second timeout. We instead thought it would be best to simply set the default to 2, and allow validators to change this at their desire.

@ValarDragon
Copy link
Member

ValarDragon commented Sep 16, 2022 •
edited
Loading

yeah this should have been default config only. Overriding the config at start is undesirable imo

@p0mvn p0mvn merged commit 0df6256 into main Sep 17, 2022
@p0mvn p0mvn deleted the adam/revert-timeout branch September 17, 2022 01:37
@mergify mergify bot mentioned this pull request Sep 17, 2022
Merged
mergify bot pushed a commit that referenced this pull request Sep 17, 2022
@czarcas7ic @mergify
Revert "Commit timeout (#2057)" (#2759)
cf7441e 
* Revert "Commit timeout (#2057)"

This reverts commit 701b391.

* remove changelog entry

* keep timeout in init

(cherry picked from commit 0df6256)
ValarDragon pushed a commit that referenced this pull request Sep 17, 2022
@mergify @czarcas7ic
Revert "Commit timeout (#2057)" (#2759) (#2768)
fee69d9 
* Revert "Commit timeout (#2057)"

This reverts commit 701b391.

* remove changelog entry

* keep timeout in init

(cherry picked from commit 0df6256)

Co-authored-by: Adam Tucker <adam@osmosis.team>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p0mvn p0mvn p0mvn approved these changes

@ValarDragon ValarDragon ValarDragon approved these changes

Assignees
No one assigned
Labels
A:backport/v12.x backport patches to v12.x branch C:docs Improvements or additions to documentation
Projects
Osmosis Chain Development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@czarcas7ic @faddat @ValarDragon @p0mvn