TransactionVerifier shouldn't unwrap Results.

[ImplOfAnImpl]

Currently, TransactionVerifier is too optimistic about error handling; in particular, it assumes that DB errors cannot happen and UtxosCache creation will always succeed, calling expect on the corresponding Results. This may lead to crashes during node shutdown; in particular, both expect calls in TransactionVerifier::new_generic may fire when exiting the node-gui app.

All calls to Result::expect must be removed and errors should be propagated properly.

P.S. This may require some refactoring in mempool, which currently assumes that TransactionVerifier never fails.
Also, mempool itself can sometimes be overly optimistic, assuming for example that chainstate is always alive; this should better be fixed too (e.g. look for the line .expect("chainstate to live") in mempool/src/pool/mod.rs)

[TheQuantumPhysicist]

I've been looking into the TransactionVerifier, and I think this warrants a discussion more than action. The question is: What is the right thing to do when an irrecoverable error happens?

One side of the extreme: Let's define what irrecoverable error means, and while I admit this is a tongue-in-cheek point, I'm making this point to clarify the red line. Let's say we open the database, and the database returns an error. What are we supposed to do in this case? The bad part here is that this can easily be caused by a hardware issue. There's virtually no way to detect this.

The other extreme side: What if we expect on error that can happen for justified reasons?

Hence, the rule of thumb is, if it's a logical, invariant error or hardware error, then we expect, and crash. That's better than behaving randomly.

So, the better question for this issue is: What is it that isn't an invariant error that we should change? I'm open for that discussion. We can arrange a meeting for that.

[ImplOfAnImpl]

What is the right thing to do when an irrecoverable error happens?

IMO it depends on the location where it happens. I'd say that in places like TransactionVerifier we should propagate all errors to the caller (I mean the actual ones, produced by user actions or the environment, as opposed to invariant violations).
And then the application could:

1. Report the error to the user and exit gracefully.
2. Ignore the error and just log it with some "mild" severity. This is reasonable to do when the app is being shut down, so some subsystems may no longer be available.

Hence, the rule of thumb is, if it's a logical, invariant error or hardware error, then we expect, and crash. That's better than behaving randomly.

I'd distinguish between logical/invariant errors and hardware ones. The former ones are bugs, so it's unreasonable to try to handle them in any way other than panicking. The latter ones IMO deserve to be reported to the user at least.

So, the better question for this issue is: What is it that isn't an invariant error that we should change?

I'm not sure it's an invariant error in this case. I could be missing something though.

We can arrange a meeting for that.

Sure.