-
Notifications
You must be signed in to change notification settings - Fork 586
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
Add Timeout type #3483
Comments
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 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:
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 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 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, 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 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 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? |
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 ❤️ |
Some initial bike-shedding, would
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. 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. |
Thanks for the followup :)
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?
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)
}
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? |
that sounds good to me. Dropped the enum approach to just have it documented here as considered. |
Agree, I think I'd prefer to go with the boolean only approach. In terms of naming, I think both |
I like the suggestions made here, and thanks for the discussion everyone! I'll throw my hat in for a boolean only 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.
and the if !timeout.Elapsed(selfHeight, selfTimestamp) {
return timeout.ErrTimeoutNotReached(selfHeight, selfTimestamp)
} |
Closed by #5404 |
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
The text was updated successfully, but these errors were encountered: