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.

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

Pass in the codec to the Acknowledgement interface function Acknowledgement() #905

Closed
3 tasks
colin-axner opened this issue Feb 10, 2022 · 8 comments
Closed
3 tasks
Labels
04-channel 29-fee needs discussion Issues that need discussion before they can be worked on

Comments

@colin-axner
Copy link
Contributor

Summary

ICS29 is our first true middleware implementation. It has an acknowledgement which wraps the underlying acknowledgements. Every acknowledgement needs to implement two functions:

Success() bool
Acknowledgement() []byte

In order for us to propagate the return value of Success() from the underlying acknowledgements, we need to access the interface type (and not the marshaled bytes). This forces us to wrap the underlying acknowledgement as an Any. Marshaling/Unmarshaling Any's however requires access to a codec which has all the implementations registered against the interface.

The current API for Acknowledgement doesn't allow for this.

I propose modifying the Acknowledgement interface to pass in a codec to Acknowledgement() so that it becomes:

Acknowledgement(cdc codectypes.Codec) []byte

Would like @ethanfrey's input since he was heavily involved in designing the 04-channel Acknowledgement type/interface.


The alternative approaches are:

  • use a global cdc (I don't like this idea at all)
  • set a success boolean within the ics29 ack to represent the success return value of the underlying applications ack (this is pretty hacky)

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added the needs discussion Issues that need discussion before they can be worked on label Feb 10, 2022
@colin-axner colin-axner changed the title Pass in the codec to the Acknowledgement interface Pass in the codec to the Acknowledgement interface function Acknowledgement() Feb 10, 2022
@AdityaSripal
Copy link
Member

So it seems to me like there already is a problem with channel acks not being registered on the codec.

I actually get a failure on main if I replace this line: https://github.com/cosmos/ibc-go/blob/main/modules/core/04-channel/types/codec_test.go#L26

with: ibcmock.NewMockEmptyAcknowledgement()

which would make sense because mock acknowledgement is not registered on any codec.

Acknowledgement interface needs to be registered in channel/types with the standard acknowledgement as one of the implementations. Apps using custom implementations can register their implementations in their codec functions.

This still doesn't help the ICS29 case because as you said in order for ICS29 to marshal an acknowledgement coming from a different application it needs the global app codec.

use a global cdc (I don't like this idea at all)

This is already the codec that gets constructed in app.go and is given to every keeper. Your solution is effectively to get the global codec from the 29 keeper and give it to a function in types as a function argument.

set a success boolean within the ics29 ack to represent the success return value of the underlying applications ack (this is pretty hacky)

I actually think this approach is fine, and less ugly then changing the Acknowledgement API.

https://github.com/cosmos/ibc-go/blob/sean%2Ffix-incentifized-acknowledgement/modules/apps/29-fee/keeper/relay.go#L19

Let's simply leave the underlying ack as []byte, and fill in the success boolean in the above function since we have access to the interface here

@crodriguezvega crodriguezvega moved this to Todo in ibc-go Feb 11, 2022
@crodriguezvega crodriguezvega moved this from Todo to In progress in ibc-go Feb 11, 2022
@crodriguezvega crodriguezvega moved this from In progress to Todo in ibc-go Feb 11, 2022
@seantking seantking moved this from Todo to On hold in ibc-go Feb 11, 2022
@crodriguezvega
Copy link
Contributor

@colin-axner do you agree with the approach?

@ethanfrey
Copy link
Contributor

I don't have the brain to think on this today, but my first feeling is to be very cautious about changing this interface.
I also strongly dislike interfaces and Any in APIs.

The approach of @AdityaSripal may work. But can you please frame the issue you try to solve. If I understand properly:

"We want the middleware to commit atomically with the wrapped code. That is, if the wrapped code returns Success() == false and reverts state, we believe all middleware will also want to revert state"

Is that the correct framing of your aims? Are we sure we want that behavior? With normal tx, AnteHandlers and Decorators like Fee handler may still commit even when the underlying message fails.

I think a deeper discussion of the design goals of middleware would be good before discussing any API changes. Although there probably as such discussion already documented in the repo, that I just failed to read (just link me to it in that case)

@AdityaSripal
Copy link
Member

In the case of fee middleware, yes we want the wrapped acknowledgement to return the same Success value as the underling ack. So that we commit or don't commit, depending on the result of the base application.

It's possible for other middleware to fail even if the underlying ack succeeds. So they should in this case return Success == false even if the underlying ack success is true. This will revert all of the state changes made by middleware and application. This is possible for middleware to do, however, one of the key properties of fee middleware is that it will not block correct operation of the underlying application. Thus, we design it to simply return the value of the underlying success field and design our middleware logic accordingly

if the wrapped code returns Success() == false and reverts state, we believe all middleware will also want to revert state

This is correct. We want to make sure that if the wrapped code returns false, then all middleware must also return false. This is because the wrapped code itself does not revert its state. The handler at the end will look at the success value and decide whether to discard the cached context. So if a middleware returns Success == true when an underlying app returns false, then we can have invalid updates written to state.

This should be well documented in the spec, I don't believe it currently is.

@colin-axner
Copy link
Contributor Author

colin-axner commented Feb 17, 2022

It's possible for other middleware to fail even if the underlying ack succeeds. So they should in this case return Success == false even if the underlying ack success is true. This will revert all of the state changes made by middleware and application. This is possible for middleware to do, however, one of the key properties of fee middleware is that it will not block correct operation of the underlying application. Thus, we design it to simply return the value of the underlying success field and design our middleware logic accordingly

This makes sense to me. But it isn't obvious that middleware cannot perform any logic if the undelrying app returns Success() == false, but it is strictly required since changing the return value can lead to invalid written state or unwritten valid state

@AdityaSripal
Copy link
Member

Ahh I was incorrect earlier.

It is the case that we will always want the middleware to return the same Success value as the underlying application.

In the case, where the underlying application returns false. Any logic performed by the middleware is irrelevant, because it will all get reverted.

In the case, where the underlying application returns true. The middleware must perform a valid state transition. This may either be a valid non-trivial state transition, or it may be a no-op. If the middleware errors in its own execution, it must not change the value of Success, it may simply no-op and pass the acknowledgement upward through the stack.

In the case, where the middleware changes the Success value of the acknowledgement, this is going to cause the potential for invalid written state.

This requirement should be clearly documented in the middleware developer guide.

@ethanfrey
Copy link
Contributor

If we take a step back from the existing implementation (easy for me), I would say that the success value should apply to that level and all underlying levels, regardless of what the parents do.

This is how we organize submessages in CosmWasm and have put heavy thought into this with many devs.

Basically, if we call A, B, C. C error will revert C state. B will by default return the error one level up (in cosmwasm), or it can explicitly handle the return and choose to return success, which will commit B changes and revert C.

If A returns error, then B will also be reverted, even if it returned success. Think of this simply as nested ctx.Cache() calls.

You are at once tied to one (simple) use case and trying to develop a general formula, which makes it hard without more examples.

In my mind, a desired behavior here is fee middleware errors if there is insufficient/malformed fees. If it succeeds, it calls the next level. The wrapped handler can revert its own state by returning success = false but the fee middleware can still charge fees... just like the sdk middleware / antehandler design.

This is easy enough to do if fee middleware doesn't call the next level directly, but calls some dispatcher that handles this logic

@seantking seantking moved this from On hold to In review in ibc-go Feb 21, 2022
@crodriguezvega crodriguezvega moved this from In review to Done in ibc-go Mar 1, 2022
@crodriguezvega
Copy link
Contributor

Closed by #952

CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this issue Nov 6, 2023
Part of EPIC celestiaorg/devops#112

> Note: Before merging this the secrets `ADD_TO_PROJECT_PAT` and
`PAT_FOR_AUTO_REQUEST_REVIEW` are needed to set. See the usage and
explanation
[here](https://github.com/celestiaorg/.github/blob/v0.1.1/.github/workflows/reusable_housekeeping.yml).

Changes in the PR:

- The `.github/dependabot.yml` file is updated to add `T:dependencies`
label for dependency updates in both package ecosystems.
- A new `.github/workflows/housekeeping.yml` file is created to automate
issue and PR housekeeping tasks, such as adding issues and PRs to a
project, adding grooming labels, automatically assigning reviewers,
assigning issues and PRs to their creators, and requiring specific
labels.
- A new `.github/auto_request_review.yml` file is created to configure
the automatic assignment of reviewers based on file changes and reviewer
groups.
<details>
<summary>Reviwer rules</summary>

- `code-owners` group includes:
  - Nashqueue
  - tzdybal
  - gupadhyaya

- `rollkit` group includes:
  - Manav-Aggarwal
  - S1nus
  - tuxcanfly

- `devops` group includes:
  - smuu
  - sysrex
  - jrmanes
  - Bidon15

- `celestia` group includes the `team:celestia` team.

The file patterns and the associated reviewer groups are:

- For any file (`**`), the `code-owners` and `rollkit` groups will be
requested for review.
- For any Dockerfile (`**/*Dockerfile`), the `devops` group will be
requested for review.
- For any file within the `.github` directory and its subdirectories
(`.github/**`), the `devops` group will be requested for review.

The options section specifies that draft PRs will be ignored
(`ignore_draft: true`), PRs with "WIP" in the title will be ignored
(`ignored_keywords: - WIP`), and the number of reviewers to request is 3
(`number_of_reviewers: 3`).
</details>

Closes celestiaorg/devops#260

---------

Signed-off-by: Smuu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
04-channel 29-fee needs discussion Issues that need discussion before they can be worked on
Projects
Archived in project
Development

No branches or pull requests

5 participants