Add Timeout type

[colin-axner]

Summary

Apply suggested spec change. There is no rush, so maybe it makes sense to add a Timeout type directly to main and then adjust 04-channel-upgrades to use it?

---

For Admin Use

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

[colin-axner]

Expanding on this. While reviewing #3333, I proposed adding a helper function to bundle the timeout logic into a single function, see #3333 (comment). Since we are adding a timeout type which bundles the height and timestamp of a timeout, it seemed useful to me to add a function which handles timeout checks for both types.

04-channel handlers are primarily interested in if the timeout has been reached or not, aside from pretty error messages, it does not care which if the timeout height or timestamp was reached.

The original proposed API included a boolean and a string to provide information about the timeout relative to the provided timestamp/height:

func (t Timeout) Status(height clienttypes.Height, timestamp uint64) (bool, string)

The intention was that the bool returns if the timeout has passed and the string provides relevant information that can be used in the returned error.

This was then modified to be:

func (t Timeout) HasPassed(ctx sdk.Context) (bool, error) {

I raised the concern (see #3535 (comment)) about using the error as a secondary return value since an error occurs based on the calling context, not whether the timeout has been reached or not.

Further concerns were raised (see #3535 (comment)) about returning relevant information in the secondary return. A proposal was made to use Before()/After() functions to align with the golang time package:

func (t Timeout) AfterHeight(height clienttypes.Height) bool 
func (t Timeout) AfterTimestamp(timestamp uint64) bool 

This was then implemented in #3535

Upon reviewing the changes, I had differing opinions on proposed direction. Given all the back and forth design discussion on the pr's and offline, we decided to move the discussion to an issue so we could come to consensus on the design introduced. As stated in the first comment of this issue, the timeout type API is a convenience to improve code readability and safety, so it should not need to block implementation of channel upgradability. It's design can be agreed upon and merged to main and then updated on channel upgradability to be used. Channel upgradability only needs the type for verifiability which does not require additional API's being discussed here.

---

The current timeout handling may look like such:

    // check if packet timeouted by comparing it with the latest height of the chain
    selfHeight := clienttypes.GetSelfHeight(ctx)
    timeoutHeight := packet.GetTimeoutHeight()
    if !timeoutHeight.IsZero() && selfHeight.GTE(timeoutHeight) {
        return sdkerrors.Wrapf(
            types.ErrPacketTimeout,
            "block height >= packet timeout height (%s >= %s)", selfHeight, timeoutHeight,
        )
    }   

    // check if packet timeouted by comparing it with the latest timestamp of the chain
    if packet.GetTimeoutTimestamp() != 0 && uint64(ctx.BlockTime().UnixNano()) >= packet.GetTimeoutTimestamp() {
        return sdkerrors.Wrapf(
            types.ErrPacketTimeout,
            "block timestamp >= packet timeout timestamp (%s >= %s)", ctx.BlockTime(), time.Unix(0, int64(packet.GetTimeoutTimestamp())),
        )
    }   

The intention of my original comment was to accomplish two tasks:

• reduce individual logic for timeout handling (checked several times in the codebase using different structures, variable names)
• replace the exact timeout checking logic with a function name which indicates if the timeout has occurred or not

04-channel handlers are only interested in knowing if the timeout has occurred or not. For example, sending a packet wants to ensure a timeout has not occurred yet, the same is true for receiving packet. Timing out a packet wants to ensure the timeout has occurred.

My concern with using the AfterHeight and AfterTimestamp API is that I'm not sure it improves readability over the existing code. Here is how it might be used:

if !timeout.IsZeroHeight() && timeout.AfterHeight(selfHeight) {
    return err
}

if !timeout.IsZeroTimestamp() && timeout.AfterTimestamp(selfTimestamp) {
    return err
}

This reads as: "if the timeout height is not zero and the timeout is after the current height". The problem I see is that understanding if the timeout has occurred requires reversing the logic, a timeout has occurred if the current height is greater than or equal to the timeout (you think of timeouts in relation to a current time, not a current time in relation to a timeout). That is, it's easier to read when you use the After(), Before() functions on the current height and timestamp: currentHeight.Before(timeout). The difficulty with this is that we would have to add additional functions for the current timestamp/height (timestamp is currently a uint64).

After discussion with @damiannolan. we landed on another alternative approach:

func (t Timeout) DeadlineExceeded(height clienttypes.Height, timestamp uint64) bool 

paired with a couple other helper functions for forumulating the error message:

func (t Timeout) ErrTimeoutNotReached(height clienttypes.Height, timestamp uint64) error 
func (t Timeout) ErrDeadlineExceeded(height clienttypes.Height, timestamp uint64

The example usage of this may look like:

if !timeout.DeadlineExceeded(selfHeight, selfTimestamp) {
    return timeout.ErrTimeoutNotReached(selfHeight, selfTimestamp)
}

In this proposal, DeadlineExceeded may look like:

func (t Timeout) DeadlineExceeded(height clienttypes.Height, timestamp uint64) bool {
    timestampDeadlineExceeded := !t.IsZeroTimestamp() && timestamp >= t.Timestamp
    heightDeadlineExceeded := !t.IsZeroHeight() && height.GTE(t.Height)
    
    return heightDeadlineExceeded || timestampDeadlineExceeded
}

and ErrTimeoutNotReached might look like:

func (t Timeout) ErrTimeoutNotReached(height clienttypes.Height, timestamp uint64) error {
    return sdkerrors.Wrapf(types.ErrTimeoutNotReached, "current timestamp: %s, timeout timestamp %s, current height: %s, timeout height %s", timestamp, timeout.Timestamp, height, timeout.Height)
}

It could still be desirable to modify an errors return value if a timeout height or timestamp was not set (it has the zero value). The example ErrTimeoutNotReached is less explicit than the original error message, but it should be possible for the reader to quickly see which value is higher (the current height or the timestamp height). There could also be a solution with enums as @DimitrisJim suggests #3535 (comment).

---

Additional timeout API's that seem to have rough consensus so far:

// IsValid validates the Timeout. It ensures that either height or timestamp is set.
func (t Timeout) IsValid() bool {
	return !t.IsZeroHeight() || !t.IsZeroTimestamp()
}

// IsZeroHeight returns true if Timeout height is zero, otherwise false.
func (t Timeout) IsZeroHeight() bool {
	return t.Height.IsZero()
}

// IsZeroTimestamp returns true if Timeout timestamp is zero, otherwise false.
func (t Timeout) IsZeroTimestamp() bool {
	return t.Timestamp == 0
}

Are there any other modifications or alternative approaches folks think would be better?

[damiannolan]

That is, it's easier to read when you use the After(), Before() functions on the current height and timestamp: currentHeight.Before(timeout).

Definitely agree with this and inverting the logic does require a bit more cognitive effort to parse when reading the code.

I am pretty happy with the solution we discussed async - I think the main problem previously was returning error information to the client (i.e. having to provide an additional return arg to propagate it to the caller). I think providing all timeout info in the error return and allowing the client to determine the cause (height or timestamp) is the right direction.

Appreciate the write up and capturing our discussion @colin-axner ❤️

[DimitrisJim]

Are there any other modifications or alternative approaches folks think would be better?

Some initial bike-shedding, would Elapsed be a better name instead of DeadlineExceeded? Its a common term when it comes to Timeouts and folks might find it easier to read.

The intention was that the bool returns if the timeout has passed and the string provides relevant information that can be used in the returned error.

This is basically why I suggested an enum, the idea being that the different information (timestamp or height elapsing) is relevant and we can encode the three different return states using only the enum. DeadlineExceeded could then return that enum on which we'll switch. Additional perk being that enums also serve as additional documentation. Disadvantage is the additional verbosity which goes against one of the initial goals.

Experience should probably make the decision on if we should really care about what part of the timeout was the one that expired, seems like it isn't information we greatly care about. A boolean answer might be all we need really.

Other than that, +1.

[colin-axner]

Thanks for the followup :)

Some initial bike-shedding, would Elapsed be a better name instead of DeadlineExceeded?

Both sound like good names to me. I guess it'd look like:

if !timeout.Elapsed(selfHeight, selfTimestamp) {
    return timeout.ErrTimeoutNotReached(selfHeight, selfTimestamp)
}

vs

if !timeout.DeadlineExceeded(selfHeight, selfTimestamp) {
    return timeout.ErrTimeoutNotReached(selfHeight, selfTimestamp)
}

Does anyone have a preference?

This is basically why I suggested an enum, the idea being that the different information (timestamp or height elapsing) is relevant and we can encode the three different return states using only the enum. DeadlineExceeded could then return that enum on which we'll switch. Additional perk being that enums also serve as additional documentation. Disadvantage is the additional verbosity which goes against one of the initial goals.

I see. So this solution is basically (with better names):

const (
    TimeoutNotReached = iota
    TimeoutHeightDeadlineExceeded
    TimeoutTimestampDeadlineExceeded
    TimeoutHeightAndTimestampDeadlineExceeded
)
    

)


if state := timeout.DeadlineExceeded(selfHeight, selfTimestamp); state == TimeoutNotReached {
    return timeout.ErrTimeoutNotReached(selfHeight, selfTimestamp)
}

// or

if state := timeout.DeadlineExceeded(selfHeight, selfTimestamp); state != TimeoutNotReached {
    return timeout.ErrTimeoutDeadlineExceeded(selfHeight, selfTimestamp, state)
}

Experience should probably make the decision on if we should really care about what part of the timeout was the one that expired, seems like it isn't information we greatly care about. A boolean answer might be all we need really.

I think my experience is that most packets only specify one timeout, height or timestamp. I think I have a preference for the non-enum approach. What do others think?

[DimitrisJim]

I think my experience is that most packets only specify one timeout, height or timestamp. I think I have a preference for the non-enum approach.

that sounds good to me. Dropped the enum approach to just have it documented here as considered.

[damiannolan]

Agree, I think I'd prefer to go with the boolean only approach. In terms of naming, I think both Elapsed and DeadlineExceeded work fine, I could take either :)

[chatton]

I like the suggestions made here, and thanks for the discussion everyone! I'll throw my hat in for a boolean only timeout.Elapsed function.

I will propose one small addition

In order to provide more information as to whether the timeout was due to height, or timestamp, we could introduce two different errors.

ErrTimeoutTimestampNotReached
ErrTimeoutHeightNotReached

and the timeout.ErrTimeoutNotReached function would return the correct one to give a more granular error message.

if !timeout.Elapsed(selfHeight, selfTimestamp) {
    return timeout.ErrTimeoutNotReached(selfHeight, selfTimestamp)
}

[crodriguezvega]

Closed by #5404