Use global success value in acknowledgment #7621
Closed
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
…lite (#6982) * feat(lite): counterparty client logic (#6307) * imp: added counterparty client store * imp: added provide counterparty to proto * imp: ran 'make proto-all' * imp: added logic to counterparty client * imp: fix proto * imp: fix proto * imp: ran 'make proto-all' * feat: finished counterparty client logic * change counterparty to include custom prefix * fix imports * import fixes, review suggestions * rm lite comment * applying review suggestions * add creator tests * addressing aditya review * Update proto/ibc/core/client/v1/client.proto Co-authored-by: DimitrisJim <[email protected]> * Update modules/core/02-client/types/msgs.go Co-authored-by: DimitrisJim <[email protected]> * Update modules/core/keeper/msg_server.go Co-authored-by: colin axnér <[email protected]> * addressing jim review * refactor(proto): use counterparty type in MsgProvideCounterparty. Validate Counterparty type. * refactor(keys): move Counterparty key to 02-client keys.go * feat(core): delete creator after registering counterparty. * chore(core): make GetCreator return a boolean if not found. * tests(02-client): add tests for counterparty validation. * tests(02-client): add tests for msg_server provide counterparty handler. * nit(core): remove stale key for counterparty in host. * Update modules/core/02-client/keeper/keeper_test.go --------- Co-authored-by: srdtrk <[email protected]> Co-authored-by: Aditya Sripal <[email protected]> Co-authored-by: Stefano Angieri <[email protected]> Co-authored-by: DimitrisJim <[email protected]> Co-authored-by: colin axnér <[email protected]>
…ient verify_* functions. (#7006)
* chore: split out packet handling rpcs * add keeper, expected interfaces, merkle tweaks. * add verify functions of client keeper. * self review
* add versions to packet and separate commitment function * use IBC Version to switch hashing * fix build and tests, found bug in switch logic * add documentation * improve code docstrings * address jim review * rename eureka to v2
* feat(tests): add helper functions, keeper test suite. * wire up packet server in app --------- Co-authored-by: Aditya Sripal <[email protected]>
* send packet eureka * test progress * add tests * lint * nit * lint moar * refactor tests
* feat(core/eureka): add recv handler. * review: address feedback, self review. * tests(core/packet-server): add tests for recv. * chore: make lint-fix. * chore: address review nits.
* timeout eureka implementation * test progress * continued progress with tests * tests * cleanup and docs * use sentinel channel in sendPacket events * address review * test review fixes * lint
* feat(core/eureka): add writeack, ack handler. * tests(core/packet-server): add tests for write acknowledgement. * chore: add packet protocol version checks to both. * fix: add check for packet receipt being present. * tests(core/packet-server): add tests for ack. * tests: address review, add FreezeClient helper to endpoint.
* chore: return app version in handlers * add expected keeper interface for packet handler functions. * add switch in msg_server to dispatch based on protocol version. * guard TimeoutExecuted with version check for time being. * rename interface. * inline timeoutExecuted * slipped WriteAck. * use msg-server entrypoints for packet flow in testing. * use endpoint.SendPacket in recv test.
…eout height (#7109) * fix condition in commit packet, added test for zero timeout height, change some error messages and add some more comments * error for verify functions
* fix: add validation of protocol version and app version to packet validatebasic * Update modules/core/04-channel/types/packet_test.go * Update modules/core/04-channel/types/packet.go Co-authored-by: Carlos Rodriguez <[email protected]> --------- Co-authored-by: Carlos Rodriguez <[email protected]>
…counterparty (#7160) * refactor: regenerate merkle path as non-nullable + add tests to build merkle path * fix: avoid mutating prefix provided * fix test build --------- Co-authored-by: Aditya Sripal <[email protected]>
* chore: add godoc * Apply suggestions from code review --------- Co-authored-by: Aditya <[email protected]>
* add tests for MsgProvideCounterparty ValidateBasic * address review comments
* add submodule for logger * add submodule in keys.go
* test: TestAcknowledgePacketV2 * fix up recv tests * add timeout v2 tests * imp: use expErr instead of expPass --------- Co-authored-by: Aditya Sripal <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]>
* merkle validation funcs * add merkle prefix validation and testing * address comments --------- Co-authored-by: Carlos Rodriguez <[email protected]>
…7198) * add helper to retrieve handler and module based on protocol version * address self review comments * returning values instead of binding vars * chore: clean up case for v1. --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: DimitrisJim <[email protected]>
…tifier (#7120) * chore: add grpc query for counterParty * chpre: made changes to the proto * chore: updated goDoc * chore: added creator to the respone * fix: fixes in the grpc_query * fixes * improve implementation and add tests * trying to fix weird linter error * fix build * chore: make counterparty field of response a non pointer. * chore: simplify logic, remove uneeded ifs * chore(tests): use expError pattern. --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: DimitrisJim <[email protected]>
* wip * wip * chore: added description * fix: fixed lint issues * address my self-review * lint * accept hex-encoded strings for merkle path prefix * fix build error --------- Co-authored-by: Carlos Rodriguez <[email protected]>
* Use client and counterparty terminology in packet server rather than channel * add counterparty not found error * add ErrCounterpartyNotFound * improve comment --------- Co-authored-by: Carlos Rodriguez <[email protected]>
…ion 2 (#7209) * chore: require app version be non-empty when protocol version is version 2. * chore(tests): use constructor instead of direct struct initialization. * chore(tests): update usages in msg_server, packet_server tests to use non-empty app version.
* add back client registerCounterparty * change channel to id in packet * fix tests * split messaging key to prefix and clientId * change eureka packet handlers to no longer rely on channel * naming suggestions * fix test file * start to fix tests * fix v2/keeper tests * fix tests * DELETE channel from v2 * rm commented code * move nextSeqSend to a better place * remove resolveV2Identifiers for now * lint * lint * review suggestions * Update cli.go
* compare seconds instead of nanoseconds * remove unnecessary helper func * improve naming
* imp: simplified BuildMerklePath * imp: review item * test: improved test structure
* reuse transfer keeper in v2 * remove keeper from transfer module v2 * moved some comments around and disabled forwarding * code review fixes * Update modules/apps/transfer/keeper/relay_test.go --------- Co-authored-by: Aditya <[email protected]>
* simplify writeack api * fix tests
* imp: updated channel v2 events * fix: removed event
* distribute build on native OS for wasm builds * fix build-wasm-simd-image-from-tag.yml * add buildx * Add install go for macos runner * setup docker before login * use linux arm instead of macos * fix image name * reference issue in TODO
* enforce strict portID check in eureka module * make base denom check * fix comments * lint * lint moar * one last time --------- Co-authored-by: Gjermund Garaba <[email protected]>
* imp: abi decoding * imp: removed abigen --------- Co-authored-by: Gjermund Garaba <[email protected]>
AdityaSripal
commented
Feb 5, 2025
| // if any of the application callbacks return a failure, the entire packet receive is considered a failure | ||
| // break the acknowledgement loop but keep a single non-nil acknowledgement in the acknowledgement slice | ||
| // so that the error acknowledgment is not equivalent to the empty acknowledgement | ||
| if res.Status == types.PacketStatus_Failure { |
There was a problem hiding this comment.
I moved this down so that specific app acknowledgement is filled in even in failure with single packet payload
| @@ -173,10 +185,24 @@ func (k *Keeper) Acknowledgement(ctx context.Context, msg *types.MsgAcknowledgem | |||
| return nil, errorsmod.Wrap(err, "acknowledge packet verification failed") | |||
| } | |||
|
|
|||
| // acknowledgement only needs to be filled in if the receive was successful | |||
| // thus we only validate if receive is successful | |||
| if msg.Acknowledgement.RecvSuccess { | |||
There was a problem hiding this comment.
One annoying thing here is I prevent an empty ack in RecvPacket or WriteAck since this could be a developer error
But on AcknowledgePacket, I allow relayers to just pass in empty ack to signal a failed receive since the alternative is to enforce a sentinel non-empty ack that will not be inspected at all by handler
There was a problem hiding this comment.
Open to feedback on better ways to structure
| @@ -6,6 +6,8 @@ import ( | |||
| sdk "github.com/cosmos/cosmos-sdk/types" | |||
| ) | |||
|
|
|||
| var FailedAcknowledgementCommitment = sha256.Sum256([]byte{byte(2), byte(0)}) | |||
There was a problem hiding this comment.
I've left this as a var for now but can change to a string const if preferred.
Need feedback from @srdtrk if this constant err ack hash is acceptable
There was a problem hiding this comment.
This is acceptable. maybe it would be better to hash a human readable string like sha256.Sum256([]byte("UNIVERSAL_ERROR_ACK")) but this is also good.
| _, err := sdk.AccAddressFromBech32(msg.Signer) | ||
| if err != nil { | ||
| return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) | ||
| } | ||
|
|
||
| // NOTE: Acknowledgement is allowed to be empty if the receive failed |
There was a problem hiding this comment.
Note: This annoying point that I do allow empty acks being sent in by relayer as a valid message now for error acks
There was a problem hiding this comment.
Why do you allow this? Isn't it emitted in the event? This doesn't make much sense to me since you'd need the proof for it anyway but I'm fine with it.
|
srdtrk
reviewed
Feb 6, 2025
| @@ -6,6 +6,8 @@ import ( | |||
| sdk "github.com/cosmos/cosmos-sdk/types" | |||
| ) | |||
|
|
|||
| var FailedAcknowledgementCommitment = sha256.Sum256([]byte{byte(2), byte(0)}) | |||
There was a problem hiding this comment.
This is acceptable. maybe it would be better to hash a human readable string like sha256.Sum256([]byte("UNIVERSAL_ERROR_ACK")) but this is also good.
Comment on lines
62
to
72
| buf = append([]byte{byte(2)}, buf...) | ||
| // prepend the preimage with the IBC Version and the recv success | ||
| // to ensure there is no possibility of hash collisions | ||
| buf = append([]byte{byte(2), byte(1)}, buf...) |
There was a problem hiding this comment.
This is a consensus breaking change right? i.e. we need to update this in solidity as well. Would it be possible to avoid this by picking a different FailedAcknowledgementCommitment?
| _, err := sdk.AccAddressFromBech32(msg.Signer) | ||
| if err != nil { | ||
| return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) | ||
| } | ||
|
|
||
| // NOTE: Acknowledgement is allowed to be empty if the receive failed |
There was a problem hiding this comment.
Why do you allow this? Isn't it emitted in the event? This doesn't make much sense to me since you'd need the proof for it anyway but I'm fine with it.
Comment on lines
40
to
44
| // Acknowledgement contains a list of all ack results associated with a single packet. | ||
| message Acknowledgement { | ||
| repeated bytes app_acknowledgements = 1; | ||
| bool recv_success = 1; | ||
| repeated bytes app_acknowledgements = 2; | ||
| } |
There was a problem hiding this comment.
This is not submitted by the relayer right? This is an internal ack
testing/mock/v2/ibc_module.go
Outdated
Comment on lines
53
to
55
| } else { | ||
| return MockFailRecvPacketResult | ||
| } |
There was a problem hiding this comment.
Linter issue, you should't need to use else after an if statement if the if block ends with a return.
|
Superceded by #7924 |
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
closes: #7645
closes: #7472
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.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.