feat!: throttle with retries, consumer changes #1024
Merged
Conversation
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
…ttle-with-retries-consumer-changes
shaspitz
changed the base branch from
main
to
shawn/optimize-pending-packets-storage
insumity
reviewed
Aug 2, 2023
insumity
reviewed
Aug 2, 2023
insumity
reviewed
Aug 2, 2023
Co-authored-by: Marius Poke <marius.poke@posteo.de>
…//github.com/cosmos/interchain-security into shawn/throttle-with-retries-consumer-changes
Co-authored-by: Marius Poke <marius.poke@posteo.de>
…//github.com/cosmos/interchain-security into shawn/throttle-with-retries-consumer-changes
mpoke
approved these changes
Aug 9, 2023
| @@ -223,6 +236,40 @@ func (k Keeper) SendPackets(ctx sdk.Context) { | |||
| // in conjunction with the ibc module's execution of "acknowledgePacket", | |||
| // according to https://github.com/cosmos/ibc/tree/main/spec/core/ics-004-channel-and-packet-semantics#processing-acknowledgements | |||
| func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, ack channeltypes.Acknowledgement) error { | |||
| if res := ack.GetResult(); res != nil { | |||
| if len(res) != 1 { | |||
| return fmt.Errorf("acknowledgement result length must be 1, got %d", len(res)) | |||
There was a problem hiding this comment.
Is this error message logged by IBC? If not, we should log it for better debugging.
There was a problem hiding this comment.
Yea it seems to be logged here https://github.com/cosmos/ibc-go/blob/000e9d20b15b3c6939cefc0e26daa4b64c1be8fb/modules/core/keeper/msg_server.go#L680
insumity
reviewed
Aug 9, 2023
insumity
approved these changes
Aug 9, 2023
20 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Implements the consumer-side changes from https://cosmos.github.io/interchain-security/adrs/adr-008-throttle-retries. See this comment for further context on how the behavior from the ADR was implemented in code.
Note the changes should be self documented, with an addition to the ADR to explain how upgrading would look like. Essentially, these changes should be merged separately and only affect consumers, with the provider throttling remaining unchanged. Then a future PR with provider changes can fully enable "throttling v2".
Note e2e tests for new throttling functionality will be added to the PR introducing provider changes.
IMPORTANT: Note diff tests are currently commented out for this PR. We'll need a decision around what to do there.
Closes #1101
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change