-
Notifications
You must be signed in to change notification settings - Fork 134
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
feat!: introduce epochs #1691
Merged
Merged
feat!: introduce epochs #1691
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
157efcb
docs: modify epochs ADR to capture latest design (#1668)
insumity 6c0549c
feat!: introduce epochs (#1660)
insumity ae68d99
test: Add epochs to MBT (#1676)
p-offtermatt f2764a2
added changelogs
insumity 192af11
rebase and fix compatibility test
insumity c859295
Update docs/docs/adrs/adr-014-epochs.md
insumity 800c1ae
Update docs/docs/adrs/adr-014-epochs.md
insumity b628eea
nit change in test
insumity 0a2546f
removed blocks per epoch upper limit
insumity File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
3 changes: 3 additions & 0 deletions
3
.changelog/unreleased/features/provider/1516-introduce-epochs.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
- Introduce epochs (i.e., send a VSCPacket every X blocks instead of in every | ||
block) so that we reduce the cost of relaying IBC packets needed for ICS. | ||
([\#1516](https://github.com/cosmos/interchain-security/pull/1516)) |
3 changes: 3 additions & 0 deletions
3
.changelog/unreleased/state-breaking/provider/1516-introduce-epochs.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
- Introduce epochs (i.e., send a VSCPacket every X blocks instead of in every | ||
block) so that we reduce the cost of relaying IBC packets needed for ICS. | ||
([\#1516](https://github.com/cosmos/interchain-security/pull/1516)) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasn't there a plan to get rid of the maximum/make it much larger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favor of keeping this upper limit. It just adds an additional check that in the best case will never be used and in the worst case will prevent from a potential bogus parameter.
Also, I do not believe that it is a very conservative upper limit. If the time we need for a block to be committed drops to something less than 6 seconds, then nothing bad will happen besides sending a greater number of
VSCPacket
s. However, if the time to commit blocks increases (e.g., to 1 minute) then 1200 blocks correspond to 20 hours. If we add to this a potential multi-hour upgrade delay, then we might start having "unbonding pausing" issues with Neutron that has an unbonding period of 20 days.We could get rid of this upper limit if we also looked at the BFT times when deciding when to send a
VSCPacket
, but this is not something we currently do, so I feel keeping an upper limit here is justified.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo, it seems silly to me that, if for some reason we can't think of yet governance wants the parameter greater than 1200 blocks, there would first have to be an upgrade to Gaia itself before it could be done. keep in mind that for any param changes, there will need to be a vote anyways. I would recommend just giving clear guidance on appropriate values for the param in the docs instead of having the cap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just the removed the upper limit.