Async Ack Fixes #7735

AdityaSripal · 2020-10-29T20:16:32Z

Description

Asynchronous acks had a number of issues in it:

The queries were not implemented (PacketReceipt/Acknowledgements) or implemented incorrectly (UnreceivedPackets). These were necessary for proper relayer function
Packet receipts were not included in genesis

Other issue involved swagger files not being updated because of incorrect config

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-10-29T20:52:50Z

Codecov Report

Merging #7735 into master will decrease coverage by 0.20%.
The diff coverage is 21.78%.

@@            Coverage Diff             @@
##           master    #7735      +/-   ##
==========================================
- Coverage   54.34%   54.13%   -0.21%     
==========================================
  Files         610      610              
  Lines       38627    38848     +221     
==========================================
+ Hits        20990    21029      +39     
- Misses      15493    15671     +178     
- Partials     2144     2148       +4

AdityaSripal · 2020-10-30T00:38:07Z

client/docs/config.json

@@ -94,31 +94,31 @@
      }
    },
    {
-      "url": "./tmp-swagger-gen/ibc/channel/query.swagger.json",
+      "url": "./tmp-swagger-gen/ibc/core/channel/v1/query.swagger.json",


This misconfiguration is why proto-swagger-gen errored

AdityaSripal · 2020-10-30T00:38:40Z

proto/ibc/core/channel/v1/channel.proto

+// packet commitments, acknowledgments, and receipts
+// Caller is responsible for knowing the context necessary to interpret this
+// state as a commitment, acknowledgement, or a receipt
+message PacketState {


PacketAckCommitment was a confusing name, changed it to PacketState with docs

AdityaSripal · 2020-10-30T00:39:11Z

proto/ibc/core/channel/v1/genesis.proto

+  repeated PacketState acknowledgements = 2
+      [(gogoproto.casttype) = "PacketState", (gogoproto.nullable) = false];
+  repeated PacketState commitments    = 3 [(gogoproto.nullable) = false];
+  repeated PacketState receipts = 4 [(gogoproto.nullable) = false];


We did not include receipts in GenesisState 😱

AdityaSripal · 2020-10-30T00:40:30Z

proto/ibc/core/channel/v1/query.proto

-  // list of commitment sequences
-  repeated uint64 packet_commitment_sequences = 3;
+  // list of acknowledgment sequences
+  repeated uint64 packet_ack_sequences = 3;
 }

 // QueryUnrelayedAcksResponse is the response type for the


Personally believe QueryUnrelayedAcks should be renamed to QueryUnreceivedAcks since this should be sent to original sending chain which is waiting to receive acks

Isn't it a RPC sent to the chain writing acknowledgements? see previous discussion

I was wrong back then. Definitely this RPC is meant to go to the chain receiving acknowledgements since that is the original sender chain which still has packet commitments in it if ack hasn't been received

Renamed it along with docs, please double check the implementation to confirm my thinking here.

QueryUnreceivedAcks is being passed in a list of ack sequences and it will then check if the executing chain still has a packet commitment for that sequence. If it does, then the sequence is added to unreceived acks. Thus, the executing chain must be the original sender since it stores the packet commitments

Sorry for the flipflop!! Hopefully the last change!

colin-axner · 2020-10-30T09:37:37Z

does this fix #7651?

AdityaSripal · 2020-10-30T09:48:58Z

does this fix #7651?

Yes it does

x/ibc/core/04-channel/keeper/grpc_query.go

colin-axner

I think the discussion we had previously notes pros/cons for doing unrelayed acks vs unreceived acks. I don't have strong opinions since this is entirely a helper function for the relayer, so whatever would make it easiest. The relayer should also primarily rely on event streaming since received packets may never have a written ack

I do think the point I made last time makes a lot of sense for using unrelayed acks

How does a relayer determine which acknowledgement sequences to send? Unlike packet commitments, acknowledgements live forever so a relayer would have to already know which acknowledgements have not been acknowledged on the original chain, which is the purpose of this function.

Maybe that doesn't change anything. As long as this helps the relayer then LGTM

proto/ibc/core/channel/v1/genesis.proto

proto/ibc/core/channel/v1/query.proto

x/ibc/core/04-channel/client/cli/query.go

x/ibc/core/04-channel/client/utils/utils.go

x/ibc/core/04-channel/keeper/grpc_query_test.go

x/ibc/core/04-channel/keeper/keeper.go

x/ibc/core/04-channel/types/genesis.go

proto/ibc/core/channel/v1/channel.proto

proto/ibc/core/channel/v1/genesis.proto

cwgoes

ACK but see comments (and previous ones)

proto/ibc/core/channel/v1/query.proto

Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: colin axnér <[email protected]>

… into aditya/async-ack-fixes

jackzampolin and others added 4 commits October 29, 2020 09:30

Add function to query ack proofs

1749d45

Merge branch 'master' into jack/acks

72d23f0

fix bugs

0965e49

fix swagger

27f2187

AdityaSripal requested review from aaronc, alessio and alexanderbez as code owners October 29, 2020 20:16

fix final queries

4afa988

AdityaSripal requested a review from cwgoes October 29, 2020 20:52

AdityaSripal added x/ibc labels Oct 29, 2020

AdityaSripal requested review from colin-axner and fedekunze October 29, 2020 20:52

clevinson mentioned this pull request Oct 29, 2020

[backports]: v0.40.0-rc2 Cherry-picks #7734

Closed

45 tasks

fix merge

e7e5745

AdityaSripal commented Oct 30, 2020

View reviewed changes

AdityaSripal requested a review from jackzampolin October 30, 2020 09:54

AdityaSripal commented Oct 30, 2020

View reviewed changes

x/ibc/core/04-channel/keeper/grpc_query.go Show resolved Hide resolved

AdityaSripal added 2 commits October 30, 2020 10:21

docs and add test case

afbc7b2

Rename QueryUnrelayedAcks to more accurate QueryUnreceivedAcks

4f6da3b

colin-axner approved these changes Oct 30, 2020

View reviewed changes

fedekunze reviewed Oct 30, 2020

View reviewed changes

x/ibc/core/04-channel/types/genesis.go Show resolved Hide resolved

proto/ibc/core/channel/v1/channel.proto Outdated Show resolved Hide resolved

proto/ibc/core/channel/v1/channel.proto Outdated Show resolved Hide resolved

proto/ibc/core/channel/v1/genesis.proto Outdated Show resolved Hide resolved

Merge branch 'master' into aditya/async-ack-fixes

8e234bc

clevinson mentioned this pull request Oct 31, 2020

Stargate RC2 Tracking Issue #7768

Closed

9 tasks

cwgoes approved these changes Nov 2, 2020

View reviewed changes

proto/ibc/core/channel/v1/query.proto Outdated Show resolved Hide resolved

AdityaSripal and others added 2 commits November 2, 2020 14:25

Apply suggestions from code review

4d1f09a

Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: colin axnér <[email protected]>

address rest of reviews

0a02dd1

AdityaSripal added 4 commits November 2, 2020 14:43

Merge branch 'master' into aditya/async-ack-fixes

2ba272b

consistent spelling

f302da7

Merge branch 'aditya/async-ack-fixes' of github.com:cosmos/cosmos-sdk…

0f9a901

… into aditya/async-ack-fixes

Merge branch 'master' into aditya/async-ack-fixes

a161f83

fedekunze approved these changes Nov 2, 2020

View reviewed changes

fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label Nov 2, 2020

mergify bot merged commit e9801ed into master Nov 2, 2020

mergify bot deleted the aditya/async-ack-fixes branch November 2, 2020 15:13

AdityaSripal mentioned this pull request Nov 2, 2020

Fix IBC unreceived packets query #7651

Closed

4 tasks

colin-axner mentioned this pull request Sep 2, 2021

Allow PacketAcknowledgements to take in packet commitments for filtering cosmos/ibc-go#370

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async Ack Fixes #7735

Async Ack Fixes #7735

AdityaSripal commented Oct 29, 2020 •

edited

Loading

codecov bot commented Oct 29, 2020 •

edited

Loading

AdityaSripal Oct 30, 2020

AdityaSripal Oct 30, 2020

AdityaSripal Oct 30, 2020

AdityaSripal Oct 30, 2020

colin-axner Oct 30, 2020

AdityaSripal Oct 30, 2020

AdityaSripal Oct 30, 2020

AdityaSripal Oct 30, 2020

colin-axner commented Oct 30, 2020

AdityaSripal commented Oct 30, 2020

colin-axner left a comment

cwgoes left a comment

Async Ack Fixes #7735

Async Ack Fixes #7735

Conversation

AdityaSripal commented Oct 29, 2020 • edited Loading

Description

codecov bot commented Oct 29, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner commented Oct 30, 2020

AdityaSripal commented Oct 30, 2020

colin-axner left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

AdityaSripal commented Oct 29, 2020 •

edited

Loading

codecov bot commented Oct 29, 2020 •

edited

Loading