Closed crodriguezvega closed 19 hours ago
Add hops to the ack error while propogating back to source so we know which chain the packet failed or timed out on [could also do counter]. Include also the deterministic error string from the error acknowledgement written by the chain where the forwarding failed
Spent some time looking at this with @chatton today! This is not so straight forward to do, we must also consider that the final destination hop may or may not be on ics20-1. In any case, it seems better to add the path forwarding info here, accepting the ack as an additional argument from the caller, here.
Using something like the following (where packet
is the forwardPacket, and not the reverse (prevPacket
):
// NewForwardErrorAcknowledgement returns a new error acknowledgement with path forwarding information.
func NewForwardErrorAcknowledgement(packet channeltypes.Packet, ack channeltypes.Acknowledgement) channeltypes.Acknowledgement {
ackErr := fmt.Sprintf("%s/%s/%s", packet.GetSourcePort(), packet.GetSourceChannel(), ack.GetError())
return channeltypes.Acknowledgement{
Response: &channeltypes.Acknowledgement_Error{
Error: ackErr,
},
}
}
Consider a path of len(4), where the final destination is ics20-1. We fail with error acknolwedgement for {code}
.
Acks would be propagated like so:
ABCI code: {code}: error handling packet: see events for details
transfer/channel-2/ABCI code: {code} ...
transfer/channel-1/transfer/channel-2/ABCI code: {code}...
Ultimately, we cannot control if the final hop has upgrade yet or not. However, we can add packet.DestPort()/packet.DestChannel()
to ALL error acknowledgements using ibc-go/v9.
For example:
-func NewErrorAcknowledgement(err error) Acknowledgement {
+func NewErrorAcknowledgement(portID, channelID string, err error) Acknowledgement {
// the ABCI code is included in the abcitypes.ResponseDeliverTx hash
// constructed in Tendermint and is therefore deterministic
_, code, _ := errorsmod.ABCIInfo(err, false) // discard non-deterministic codespace and log values
return Acknowledgement{
Response: &Acknowledgement_Error{
- Error: fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString),
+ Error: fmt.Sprintf("%s/%s ABCI code: %d: %s", portID, channelID, code, ackErrorString),
},
}
}
I think this would give us a bit more info, but no guarantees that the final destination has been upgraded.
Personally, I think other solutions sound pretty hacky. You could add some index to the error string in the acknowledgement, and parse that string on every async ack write, and increment. And then use the index to determine the failed hop when you have been propagated back to the original src and have all forwarding path info available. I would vote against this!
cc. @AdityaSripal
edit: We've decided to go with source port and channel only, as this will match the user provided info in Forwarding.Hop
. See #6731
From internal alpha audit of ICS20 v2 path forwarding:
unwindHops
where we loop over the trace elements except the first one. [@gjermundgaraba] #6702unwind
is true). [@bznein] #6706createPacketDataBytesFromVersion
both for V1 and V2. Change signature of function to also returnerror
. @chatton https://github.com/cosmos/ibc-go/pull/6704hops
tosendTransfer
. ChangeEmitTransferEvent
to accept only hops as well. [@gjermundgaraba] #6703getReceiverFromPacketData
switch around the if statements and add comment about why we do that: If the forwarding is set, then do not process the data.Receiver field and instead just shortcircuit to create receiver from forwardingInfo [@gjermundgaraba] #6709continue
[@crodriguezvega] https://github.com/cosmos/ibc-go/pull/6700SenderChainIsSource
andReceiverChainIsSource
with functionIsTracePrefixed
(like the one used in the ICS20 spec). Open a PR and see which one is easier to reason about. [@crodriguezvega]repeated AllowedForwarding allowed_forwarding
withAllowedForwarding
withrepeated Hop hops
. We choose to go with this option. Granter will have to specify the list of hops to unwind. @chatton https://github.com/cosmos/ibc-go/pull/6701slices.Contains
works to get rid of the loop [bznein] https://github.com/cosmos/ibc-go/pull/6697Thank you @chatton for leading the walkthrough and the rest of the team for the feedback.