CosmWasm / wasmd

Basic cosmos-sdk app with web assembly smart contracts
Other
366 stars 397 forks source link

Possible problem with the `nil` ack #1891

Closed aeryz closed 3 months ago

aeryz commented 4 months ago

Hello, we are using IbcReceiveResponse::without_ack to return a nil ack. But we are hitting this error. Our initial investigation shows us that nil ack returns non-nil ack with empty data. That's why we are hitting this error.

As far as I understand, this feature is meant to return a nil ack and cause WriteAcknowledgement to be deferred. Is there something that we are missing or is this a bug?

webmaster128 commented 4 months ago

Which version of wasmd and wasmvm do you use? Optional acks are a feature only supported in CosmWasm 2 chains.

In general this is a two step process:

  1. CosmWasm 2: Make acks optional. This is preparation for 2. but there can also be protocols that never ever write acks.
  2. CosmWasm 2.1: Allow writing asynchronous acks for packets received packets.
PoisonPhang commented 4 months ago
// You can edit this code!
// Click here and start typing.
package main

import "fmt"

type Ack interface {
    Value() []byte
}

type ContractAck []byte

var _ Ack = ContractAck{}

func (ack ContractAck) Value() []byte {
    return ack
}

func main() {
    var x []byte
    y := ContractAck(x)
    var z Ack
    z = y
    var b Ack
    b = nil
    var c Ack
    c = ContractAck(nil)

    if x == nil {
        fmt.Println("nil x")
    }
    if y == nil {
        fmt.Println("nil y")
    }
    if z == nil {
        fmt.Println("nil z")
    }
    if z.Value() == nil {
        fmt.Println("nil z.value")
    }
    if b == nil {
        fmt.Println("nil b")
    } else if b.Value() == nil {
        fmt.Println("nil b.value")
    }
    if c == nil {
        fmt.Println("nil c")
    } else if c.Value() == nil {
        fmt.Println("nil c.value")
    }

}
nil x
nil y
nil z.value
nil b
nil c.value
PoisonPhang commented 4 months ago

https://github.com/cosmos/ibc-go/blob/083422914ad465d2d7d829a9696b3b929c87e28f/modules/core/keeper/msg_server.go#L495-L511

https://github.com/CosmWasm/wasmd/blob/21b048d54e395ff9168e5c3037356a73797500ba/x/wasm/keeper/relay.go#L173

webmaster128 commented 4 months ago

WTF – so there is a Go type system bug in wasmd?

aeryz commented 4 months ago

We had it working by changing the ibc-go code to this:

    // Set packet acknowledgement only if the acknowledgement is not nil.
    // NOTE: IBC applications modules may call the WriteAcknowledgement asynchronously if the
    // acknowledgement is nil.
    if ack != nil && ack.Acknowledgement() != nil {
        if err := k.ChannelKeeper.WriteAcknowledgement(ctx, capability, msg.Packet, ack); err != nil {
            return nil, err
        }
    }

So it seems like although the wrapped bytes is nil, the interface is not nil

hussein-aitlahcen commented 4 months ago

So, the issue is that interfaces in Go are a bit weird:

Hence, there is an implicit newtype wrapping (extra pointer indirection) when casting, even if the value you cast is nil (so the value isn't directly used as the interface, but a new pointer is created)

chipshort commented 4 months ago

I think I already stumbled upon this while implementing WriteAcknowledgement for 2.1: https://github.com/CosmWasm/wasmd/pull/1876/files#diff-8eeb0bf4af45c3a552b61aed85469953f39a51bb5a534c0f4d52256bbfce37eb In the PR, you can see I had to make some changes in OnRecvPacket to catch this special case.

webmaster128 commented 4 months ago

As this is clearly a bug in wasmd, I am moving it over there