Use ErrorReceipt as Error type

[colin-axner]

Summary

As demonstated in #3418, use the ErrorReceipt as an Error type, so all our API's which return an error can return an ErrorReceipt. Once the msg server receives the ErrorReceipt it can call abortUpgradeHandshake which does app callbacks and writes the error receipt

I like this flow because it makes it very clear to the msg server when the handshake should be aborted and when it succeeded, otherwise a nil error could be returned by channelKeeper.ChanUpgradeTry even though the handshake should not continue

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[colin-axner]

Here's how I imagine things:

1. chanUpgradeTry enters startFlushUpgradeHandshake subprotcol and hits error receipts situation
2. The code returns an error receipt:

if err := func(); err != nil {
    return types.NewErrorReceipt(err)
}

3. The msg server checks for an error receipt:

if ok := err.(types.ErrorReceipt); ok {
    abortUpgradeHandsahke(err)
}

4. restoreChannel emits/logs the error

    ctx.Log(errorReceipt.FullError())
    emitErrorReceiptEvent()
    
    // restore channel ensure non-determinstic info is discarded from Message field
    errorReceipt = types.NewErrorReceipt(channel.Sequence, err)
    k.SetErrorReceipt(errorReceipt)

One part I'm unsure about, in the initial return, I think it is nice for it to look like a normal error type (just with a ErrorReceipt prefix), but I would like to do logs/emit events later in the restore channel function. So creating an API which creates an error receipt type that has full non-determinstic error info that can then be converted into the verifiable error receipt type

This is just my ideal handling, open to other ideas

[chatton]

Should the actual emission of events / setting error receipt happen in the restore function? I was thinking something like

// msgServer
err := ChanUpgradeTry(...)
if isErrReceipt(err) {
   abortUpgradeHandshake(err)
   return ... 
}

func abortUpgradeHandshake(err) {
   log(err)
   restoreChannel()
   emitErrorReceiptEvent(err)
}

I'm not sure the setting of an error receipt should be coupled with the act of restoring, I think logically it makes sense that we restore, and then write an error receipt as two concrete actions. (which may end up in a function like abortUpgradeHandshake )

Maybe I am misinterpreting your pseudo code.

[chatton]

I think we can wait until either the #3671 and/or #3628 is merged before we continue with this. We can update those functions to return the error receipt type and handle the abort/event emission

[chatton]

Looking a little more into it, if we want to be able to maintain both the original error message (for logs & events) while also being able to deterministically determine the message for the ErrorReceipt that we write in state, I think we can actually introduce a new error type. This type will implement error and can be checked in order to determine if a restore is necessary, and we don't need to worry about adding any additional fields or behaviour to the ErrorReceipt type.

// UpgradeError defines an error that occurs during an upgrade.
type UpgradeError struct {
	// underlyingError is the underlying error that caused the upgrade to fail.
	// this error should not be written to state.
	underlyingError error
	// upgradeSequence is the sequence number of the upgrade that failed.
	upgradeSequence uint64
}

func (u *UpgradeError) Error() string {
	return u.underlyingError.Error()
}

// GetErrorReceipt returns an error receipt with the code from the underlying error type stripped.
func (u *UpgradeError) GetErrorReceipt() ErrorReceipt {
	_, code, _ := errorsmod.ABCIInfo(u.underlyingError, false) // discard non-determinstic codespace and log values
	return ErrorReceipt{
		Sequence: u.upgradeSequence,
		Message:  fmt.Sprintf("ABCI code: %d: %s", code, restoreErrorString),
	}
}

// NewUpgradeError returns a new UpgradeError instance.
func NewUpgradeError(upgradeSequence uint64, err error) UpgradeError {
	return UpgradeError{
		underlyingError: err,
		upgradeSequence: upgradeSequence,
	}
}

WDYT @colin-axner ?

[colin-axner]

I love it! @damiannolan recommended removing the NewErrorReceipt function as well, which I think would be good to avoid confusion. Excellent suggestion @chatton ❤️