Return error instead of panicking when modifying a sealed app

[p0mvn]

Background

The sealed flag in baseapp is true when the app should not be modified further. It is set when baseapp.Init() or baseapp.Seal() are called.

Currently, if an attempt to modify a sealed app is made, the SDK panics. However, it might be better to return an error.

This issue is to change all baseapp modifiers to return the error instead of panicking.

Some more context is in this thread:
#11496 (comment)

Several methods are located in the options.go

Acceptance Criteria

• baseapp.Init() returns an error instead of panicking
• all baseapp modifiers in baseapp/options.go do the same
• unit tests added

[alexanderbez]

Yeah, I've read the comments from @peterbourgon. I don't necessarily think or agree that all panics are solely for programming errors as there are certain exceptions where you want to halt/exit as early as possible.

However, I do agree that we should probably consider returning an error here. But all this will do is bubble the error upstream, where we will just panic or exit anyway (e.g. from a CLI command handler).

[ValarDragon]

I think panic is the correct behavior here, theres nothing else to do with errors in initialization, and we then waste even more error checks everywhere, further adding complexity to logic that should be simple glueing of things.

[peterbourgon]

panic is not just an alternative return err for when the caller can't do anything to fix the problem. You still use errors for those situations! 😇 Error handling isn't noise, and making it explicit actually reduces overall complexity. In fact one of the fundamental conceits of the language is that the "sad path" of error handling is equally important to the "happy path" and shouldn't be obscured by the shadow control flow that panic introduces.

(The SDK gets this super duper wrong, and its systemic misuse of panic is IMO the single largest contributor to the fragility of Cosmos networks. Fixing it properly would be an impossible task, but we can at least not repeat the mistake going forward.)

[alexanderbez]

@ValarDragon is correct here though, we want to panic when the application is sealed. We can certainly return an error and bubble it, but we'd end up either panicing or exiting only one layer up, which doesn't really buy us much.

[alexanderbez]

As for resolving this issue, I'm open to returning errors and having the SDK exit upon non-nil errors in the server package.

[ValarDragon]

If the caller can't do anything to fix the problem and will just exist, and the caller can't add any more valuable context to the situation, I don't get why panic isn't preffered.

If error handling were a minimal cognitive overhead for the reader, e.g. like in rust with the ? suffix & .map_err, I don't think I'd have this complaint, but in golang code its an imo huge overhead. Strong disagree personally to this being the single biggest contributor to network fragility. Networks can just just add a panic catch in their begin block and end block if they don't want to take the cosmos safety first approach. Imported packages / utility methods elsewhere could panic as well, so this is really needed. cref osmosis-labs/osmosis#1305

[peterbourgon]

If the caller can't do anything to fix the problem and will just exist, and the caller can't add any more valuable context to the situation, I don't get why panic isn't preffered.

panic expresses an invariant violation of the program caused by fundamental programmer error. That means stuff like dividing by zero, or array-out-of-bounds errors. It explicitly is not meant to express simple logic bugs, or errors that can only be detected at runtime e.g. invalid data in a packet. Long-lived programs — like servers, i.e. anything powered by the Cosmos SDK — should actually never panic in practice. Every panic observed by a user should be understood as a showstopper bug that should be fixed immediately.

The problem is that panic subverts the idiomatic/canonical control flow. If a function panicks, you can't reason about its effect on overall program behavior. In some cases this is fine! If code encounters an invariant violation so severe that the only reasonable action is to immediately crash the process, then panicking is appropriate. But if those situations are common, then it becomes infeasible to maintain a mental model of the program. Control flow becomes subject to not just function/method calls and return statements, but arbitrary jumps dictated by panic and recover statements. For programs generally, and especially for large projects in the open-source space, this is untenable.

If error handling were a minimal cognitive overhead for the reader, e.g. like in rust with the ? suffix & .map_err, I don't think I'd have this complaint, but in golang code its an imo huge overhead.

I understand this perspective, but it's not an objective truth, and it's certainly not true for Go. In Go, error handling is explicit. That's just a fact of the language, and something that Go programmers must necessarily understand and adopt in order to be productive. if err != nil { return fmt.Errorf("annotation: %w", err) } cannot be understood as overhead, or toil, or complexity. It's just how Go programs read.

Networks can just just add a panic catch in their begin block and end block if they don't want to take the cosmos safety first approach. Imported packages / utility methods elsewhere could panic as well, so this is really needed.

A panic should be understood as an unrecoverable and process-terminating event. Libraries should never panic, and code should never treat recover as an error handling mechanism. Go does not have exceptions.

---

osmosis-labs/osmosis#1305

At the moment, any panic in BeginBlock, or EndBlock will halt the chain. This by proxy means that any panic in epoch logic will halt the chain. Only panics inside of transactions get caught.

This is correct — panic should not be treated as a recoverable event.

The usual way to report an error to a caller is to return an error . . . But what if the error is unrecoverable? Sometimes the program simply cannot continue. For this purpose, there is a built-in function panic that in effect creates a run-time error that will stop the program . . . a way to indicate that something impossible has happened, such as exiting an infinite loop . . . real library functions should avoid panic.

https://go.dev/doc/effective_go#panic

[peterbourgon]

we want to panic when the application is sealed. We can certainly return an error and bubble it, but we'd end up either panicing or exiting only one layer up, which doesn't really buy us much.

I'm claiming that this isn't true. panic isn't just another way of spelling "exit with an error" — it expresses a more fundamental and substantial problem than simply calling methods on a struct in the wrong order. That's a logic error and should terminate the program! I agree with that. But the right way to do that is to bubble up errors to func main and have it exit the program with whatever error code. It's the only place which actually has the right to terminate the process.

[ValarDragon]

The linked effective go even says panicing is fine for initialzation though?

This is only an example but real library functions should avoid panic. If the problem can be masked or worked around, it's always better to let things continue to run rather than taking down the whole program. One possible counterexample is during initialization: if the library truly cannot set itself up, it might be reasonable to panic, so to speak.

[peterbourgon]

Yes, but initialization in that context means e.g. func init, not constructors or methods on types which are expected to be called in a certain order, or at a certain phase of a program's execution.

Concretely,

if the library truly cannot set itself up, it might be reasonable to panic

Modifying a sealed app is a logic error, and a bug, but not an unrecoverable condition. The result is obvious: simply reject the modification with an error. In fact every fallible operation should return (..., error)! 😇 This straightforward rule is unfortunately violated throughout the SDK. One example I'm particularly frustrated with at the moment is the store/types.BasicKVStore. Every method in this interface should return (..., error) and yet none do!

[peterbourgon]

Please re-open this issue.

[alexanderbez]

I don't think there is a need to bubble up these errors upon app initialization 👍