Make IBC ante-handler update CheckTX state

[ValarDragon]

Summary

IBC ante-handler for preventing duplicate relays does not update CheckTX State. Because of this, you can still get duplicate packets relayed in the same block. (But thankfully, the existing work fixes it across blocks!) We can see evidence of this issue on multiple chains, here is an example @larry0x showed me on Osmosis: relay one, relay two. This is only found when they are in the same block.

To fix this we need to make the IBC ante-handler update the CheckTX State.

Proposal

Look at how we update CheckTX state for sequence numbers of transactions: https://github.com/cosmos/cosmos-sdk/blob/2646b474c7beb0c93d4fafd395ef345f41afc251/x/auth/ante/sigverify.go#L308-L344

After we've determined the tx is Check-TX valid (e.g. SigVerify, and can pay fee), we update the CheckTx state for that account, incrementing the sequence number.

Similarly in the IBC Check-TX Ante-handler, we need to update the packet acknowledgements to show as already being in, in order to avoid this duplicate relay. So in this logic https://github.com/cosmos/ibc-go/blob/main/modules/core/ante/ante.go#L23, if we determine there is a component that is not redundant, we need to also add that to state, so a subsequent run for another tx will see it as duplicate.

Not entirely sure if this should be architected by having a segment in state thats 'checkTX only' and not written to during DeliverTx execution, so you don't have to interfere with existing logic.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[AdityaSripal]

Thanks for the detailed writeup as always @ValarDragon !

Just some things to note for future readers, CheckTx is not called during block execution. It is called on txs in the mempool upon inclusion and after each block commit.

The crucial thing to note is that in the SDK, we maintain a checkState that persists updates made by checkTx's between block commits so that we can check transactions not just against the latest head state but also against the cumulative updates from checkTxs since latest block commit. This is what allows the SDK to reject txs that are valid wrt to latest commit, but not valid wrt to each other (in this case latter tx gets rejected).

We should also note that only the antehandler runs in CheckTx, and thus only state updates that occur during antehandler will update the checkState.

---

I have a few concerns with your suggestion.

The analogy to IncrementSequence is not accurate.

The reason IncrementSequence works well is because the account sequence will increment so long as the antehandler passes (even if the rest of the msg handling fails). So we can increment the sequence safely at the end of the antehandler and reject all future txs with the same sequence because even if that tx fails later on DeliverTx, the sequence will still get incremented.

With the IBC case, we will need to "mock" the tx being handled successfully in order to reject a subsequent tx of the same type. This raises a potential issue.

Suppose there are 2 recvPacket txs in the mempool, the first will fail during msg handling and the second will be successful.

Here, the first tx will be mocked as successful and thus the second tx will be kicked out of the mempool. The first tx will later fail during block execution and the second tx will have to be resubmitted.

The problem then arises since, it's possible based on the chosen Mempool config that the node has chosen KeepInvalidTxsInCache=true (idk the status of this param on mainnets). In this case, a tx that fails CheckTx can never be resubmitted.

This shouldn't be too bad so long as there's at least one node that gets the tx's in a different order (or gets them in between block commits). Or the above config is not common in the network.

However, there are enough caveats here to make me nervous. There's certainly a corner case that might require manual intervention to get the message executed and this corner case would be a nightmare to discover and debug

---

The above issues make me a bit nervous about implementing this suggestion, but it's possible that the above concerns are irrelevant.

In that case, there is a custom "mock" update to checkState for each msg type.

RecvPacket on UNORDERED channel => write packet receipt
RecvPacket on ORDERED channel => increment recv sequence
AcknowledgePacket and TimeoutPacket => delete packet commitment

All of the above changes would only be made to checkState and so we must only do these updated if ctx.IsCheckTx()==true

---

However, given both my concerns for mocking successful msg handling and the fact that there's a custom update for the different msg types; I wonder if it wouldn't be simpler to just execute the entire message in these cases during AnteHandler.

This would make CheckTx more expensive for nodes for IBC txs, but at least then we could be sure that whatever txs we do leave in the mempool are definitely going to succeed.

[ValarDragon]

I'm supportive of simulating the entire tx in the mempool. This is a trade-off ~every other blockchain ecosystem takes as far as I can tell.

In terms of computational load overheads, while it does increase it slightly, I think this should be bounded by peer scoring at the p2p / mempool layer. (Ala tendermint/tendermint#7918) Please correct me if I'm wrong with the assumption that we can detect to a large extent, whether the IBC relay could ever have been valid. (E.g. valid validator signatures, so no forgery there. All you could do was attempt a valid relay for something old, but packet acks should save us then!)

So would like to add the caveat that the ante handler ordering should imo be:

• Checks tx signature
• The quick (no signature verification) acknowledgement check as it does now first
• The deep 'run-update-in-check-TX' method. (Eventually once linked Tendermint issue lands, mark cases as never being possible to be valid, and thus eliminate mempool dossing concerns)

I think this is a great idea!

[ValarDragon]

(Your definitely right, the original suggestion of just incrementing those packets without verifying the full IBC data was definitely bad. If I'm being honest, I just didn't think about verifying the IBC data or out-of-gas problems)