search

cosmos / ibc-go

Inter-Blockchain Communication Protocol (IBC) implementation in Golang.
https://ibc.cosmos.network/
MIT License
559 stars 605 forks source link

modules/apps/27-interchain-accounts/controller/keeper: Keeper.SendTx doesn't seem to respect the zero value for types.MsgSendTx.TimeoutTimestamp which signifies being disabled #2267

Closed odeke-em closed 2 years ago

odeke-em commented 2 years ago

Summary of Bug

The code in here https://github.com/cosmos/ibc-go/blob/a4be5614fb2fc92cfb430c82f18e7a9f67383c6e/modules/apps/27-interchain-accounts/controller/keeper/msg_server.go#L65 then invokes

https://github.com/cosmos/ibc-go/blob/a4be5614fb2fc92cfb430c82f18e7a9f67383c6e/modules/apps/27-interchain-accounts/controller/keeper/relay.go#L38-L40

but if we look at the definition of the types.MsgSendTx, it clearly says that a 0 value disables timeouts https://github.com/cosmos/ibc-go/blob/a4be5614fb2fc92cfb430c82f18e7a9f67383c6e/modules/apps/27-interchain-accounts/controller/types/tx.pb.go#L125-L127

Expected Behaviour

I expected that value to be honored and that ctx.BlockTime().UnixNano() wouldn't be blindly compared with what could be a zero value. If that's not the expected behavior, please update the proto definitions.

Version

a4be5614fb2fc92cfb430c82f18e7a9f67383c6e

For Admin Use

crodriguezvega commented 2 years ago

Good catch, @odeke-em!

I think we should probably do if timeoutTimestamp != 0 && uint64(ctx.BlockTime().UnixNano()) >= timeoutTimestamp, right @damiannolan?

Actually, I am wondering now... why do we need this check here at all? For ICS20 we don't have this check in SendTransfer...

colin-axner commented 2 years ago

Hi @odeke-em, the documentation is incorrect. ICS 27 packets set the timeout height to be zero, thus the timeout timestamp must be non-zero