Comments after alpha audit of path forwarding

[crodriguezvega]

From internal alpha audit of ICS20 v2 path forwarding:

• Add a comment in unwindHops where we loop over the trace elements except the first one. [@gjermundgaraba] chore: comment hop slicing #6702
• Move all forwarding validation together to the same helper function if possible (including the check for empty source port and channel if unwind is true). [@bznein] (chore) Refactor code around forwarding validation #6706
• Run packet data validation after it is constructed in createPacketDataBytesFromVersion both for V1 and V2. Change signature of function to also return error . @chatton Add packet data validation back #6704
• Pass only hops to sendTransfer. Change EmitTransferEvent to accept only hops as well. [@gjermundgaraba] chore: pass only hops to sendTransfer and events #6703
• getReceiverFromPacketData 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] feat(transfer): allow non-cosmos-sdk AccAddress in final receiver for forwarded packets #6709
• Switch to if-else blocks in OnRecvPacket instead of using continue [@crodriguezvega] core: replace continue with if/else #6700
• Maybe replace SenderChainIsSource and ReceiverChainIsSource with function IsTracePrefixed (like the one used in the ICS20 spec). Open a PR and see which one is easier to reason about. [@crodriguezvega]
• 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. @damiannolan (chore: propagate ics20 forwarding path in error acknowledgements #6731)
• Authz:
  • Allow explicit list of allowed hops. Do not allow unwind boolean
  • Use repeated AllowedForwarding allowed_forwarding with AllowedForwarding with repeated Hop hops . We choose to go with this option. Granter will have to specify the list of hops to unwind. @chatton Remove unwind field in authz #6701
• Replace deep equal in authz with something else. Check if slices.Contains works to get rid of the loop [bznein] (chore) replace reflect.DeepEqual with slices.Equal #6697

Thank you @chatton for leading the walkthrough and the rest of the team for the feedback.

[damiannolan]

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:

• RecvPacket failure (hop3): ABCI code: {code}: error handling packet: see events for details
• Handle ack(hop2), append path info and writeAck: transfer/channel-2/ABCI code: {code} ...
• Handle ack(hop1), append path info and writeAck: transfer/channel-1/transfer/channel-2/ABCI code: {code}...
• Handle ack(hop0) - original sender. Note, we cannot modify the acknowledgement on the chain that it was not written on and therefore the first outbound port/channel ID from A->B would not be included in the emitted event.

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