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

Possible problem with the nil ack #1891

Closed
aeryz opened this issue May 24, 2024 · 8 comments · Fixed by #1876
Closed

Possible problem with the nil ack #1891

aeryz opened this issue May 24, 2024 · 8 comments · Fixed by #1876
Milestone

Comments

@aeryz
Copy link

aeryz commented May 24, 2024

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
Copy link
Member

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
Copy link

PoisonPhang commented May 24, 2024

// 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
Copy link

@webmaster128
Copy link
Member

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

@aeryz
Copy link
Author

aeryz commented May 24, 2024

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
Copy link

hussein-aitlahcen commented May 24, 2024

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

  • if you non initialize or assing nil to an interface, the interface is nil
  • if you assign an implementation of an interface to the interface, even if the value you cast is nil, the interface won't be nil

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
Copy link
Contributor

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
Copy link
Member

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

@webmaster128 webmaster128 transferred this issue from CosmWasm/cosmwasm May 26, 2024
@chipshort chipshort mentioned this issue Jun 10, 2024
@webmaster128 webmaster128 added this to the 0.52 milestone Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants