Make middlewares less confusing

[amaury1093]

Summary

Reconsider the middleware "decorator" approach as described in ADR-045 in favor of a less confusing model.

Problem Definition

Multiple feedback concur that middlewares are hard to reason about (cf Bez, Amaury, Yihuang).

Some problems:

1. Middlewares have dependencies between them, but those dependencies are not clearly defined.

For example, SigVerificationMiddleware always needs to be run after SetPubKeyMiddleware, but the only place where this dependency is defined is in the godoc comment. Or GasTxMiddleware: any other middlewares that access the GasMeter needs to run after it. Again, this is only enforced by go comments.

If any chain puts the middlewares in wrong order, then there are security risks.

2. It's hard to track pre- and post-runMsg logic for the whole middleware stack.

If middlewareA is below middlewareB in the stack, it's slightly unclear which which one's pre/post-hook runs after the other's pre/post-hook. But this gets impossible to track with 15+ middlewares. For example, store branching and when to write back to store is hard to get right with the current composition design: #11942.

In ConsumeBlockGasMiddleware, we also use a defer func(). Good luck tracking that ordering with other post-hook logic.

Middleware dependencies

Here's a quick overview of the dependencies between middlewares. \t⬑ means "depends on" i.e. "needs to run after", and items on the same level of indentation can be run parallely (have no dependencies).

Store Branching (all middlewares are in a store branch, store writes only happens if the whole branch succeeds)
⬑ Antehandler Branch (what corresponds to antehandlers in the old baseapp)
    ⬑ RunMsgs Branch (what corresponds to runMsgs in the old baseapp)

Where:
Antehandler Branch:
    ⬑TxDecoder (all middlewares need a decoded tx)
        ⬑ Gas (all middlewares that track gas need this one)
            ⬑ Recovery (not 100% required so high in the stack, but good to have, in case any middleware panics. needs Gas)
                ⬑ ExtensionOptions
                ⬑ ValidateBasic
                ⬑ TxTimeoutHeight
                ⬑ ValidateMemo
                ⬑ ConsumeTxSizeGas (needs gas)
                ⬑ IncrementSequence (needs gas)
                ⬑ IndexEvents (all middlewares that emit events needs this one)
                    ⬑ SetPubKey (needs event emitting)
                        ⬑ ValidateSigCount (needs pubkey)
                        ⬑ SigGasConsume (needs pubkey)
                        ⬑ SigVerification (needs pubkey)
                    ⬑ DeductFeeMiddleware (needs events)

And:
RunMsgs Branch:
    ⬑ RunMsgs
        ⬑ Tips
            ⬑ (Future: fee refund)
                ⬑ ConsumeBlockGas (add tx gas to the block GasMeter, needs to be run after ALL tx gas consumption, also runs on failed txs)

Proposals

In any of the 2 proposals below, we should consider removing the possibility of having both pre and post logic for middlewares. A middleware should either be a pre-middleware, or a post-middleware, not both at the same time.

Proposal 1: Build a dependency graph between middlewares

App developers specify relative dependencies of middlewares.

Then, an algorithm will propose a graph that satisfies all the defined dependencies. Finally, the SDK will figure out the sequential ordering of middlewares by traversing the dependency graph, in a way that this sequential order satisfies all the initial dependencies.

Example:

// Syntax:
// <middleware>: [<array of dependencies>]
SigVerification: [SetPubKey, IndexEvents, Gas]
SetPubKey: [IndexEvents]
IndexEvents: [TxDecoder]
Gas: [TxDecoder]

In this case the SDK will know that any of the 2 following orderings are valid:

• TxDecoder, Gas, IndexEvents, SetPubKey, SigVerification
• TxDecoder, IndexEvents, Gas, SetPubKey, SigVerification

Done in v0.46 Proposal 2: Fallback to something similar to the old baseapp

We could also define 2 middleware stacks:

• one before runMsgs, exactly like that old antehandlers
• one after runMsgs (for tips, and in the future for e.g. fee refunds)

This would make the code easier to reason about.

Further Notes

If we go with the dependency graph solution, we could use the same graph design for BeginBlockers, EndBlockers, InitChain etc (as proposed initially by @ValarDragon): instead of middleware dependencies, it would be module dependencies.

[alexanderbez]

I like the proposal, but personally, I would still love to see a sequential approach rather than recursive one. Its so much easier to reason about when you know the ordering and execution stack of the middleware. Does proposal (1) still use a recursive approach?

I'll also post this here as it might be useful. Osmosis did something similar with respect to EndBlock and BeginBlock: https://github.com/osmosis-labs/osmosis/blob/main/app/modules.go#L178. They essentially define a global ordering and then define more custom local relational ordering. Maybe we can do something similar?

[amaury1093]

Does proposal (1) still use a recursive approach?

Not necessarily. Actual proposal 1 is similar to what you described in Osmosis: you define relative ordering of middlewares (see Example), and the SDK figures out the global ordering.

[aaronc]

I think having a topological sort of a dependency graph is going to be the more correct approach for begin/end blocker, init chain, and ante/middleware ordering. Otherwise, you need to drop the idea of independent components altogether.

I agree we should drop the wrapping behavior and just have things execute one after the other (or even in parallel in the future) based on dependency order. If there is need for before/after behavior that can be dealt with using two handlers where after depends on before.

I'm wondering what the best way is to specify order. One thought is to do something similar to what's happening with dependency injection where handlers specify the types of things that must be created/happen using go types. This would be instead of passing context.Context which is now being used as an untyped bag of variables. For example:

func ExtractPubKeys(tx Tx) PubKeyInfo

func VerifySignatures(pubkeys PubKeyInfo) SignatureValidationResult { ... }
 
func DeductFees(sigs SignatureValidationResult)

func IncrementSeqs(sigs SignatureValidationResult)

Then in dependency graph we would know that this order is required based on the type of information needed:

ExtractPubKeys -> VerifySignatures -> DeductFees | IncrementSeqs

This may be a case where putting too much in context.Context actually makes an incorrect ordering more likely.

I don't think we would actually need to do as much store branching with this approach - deduct fees and increment seqs should commit on their own if signatures were validated. IMHO they don't actually need a transaction wrapping from the outside but instead should be able to initiate a tx and commit to the store based on they already know. I can't think of a scenario where something else happening should prevent them from committing or even if say deduct fees fails but increment seqs doesn't that we should rollback both. I'm actually a bit confused by why in the current ordering we verify sigs after running deduct fees and then roll that back based on sigs instead of waiting to do it until after verifying sigs.

[alexanderbez]

Great, do you think proposal (1) is inline with that? I'd like to use something clean and simple.

[amaury1093]

@aaronc Your reflect.Type-based proposal seems elegant, but for me it also seems like magic.

We could also give a name (string) to each middleware, and define proposal (1)'s dependencies using strings, arrays and maps.

[aaronc]

@aaronc Your reflect.Type-based proposal seems elegant, but for me it also seems like magic.

We could also give a name (string) to each middleware, and define proposal (1)'s dependencies using strings, arrays and maps.

I'm concerned that strings are hard to coordinate around and don't force good specification. Actually passing dependencies is more explicit even if it's "magic"

[aaronc]

Either way I think we should probably postpone a dependency graph solution till after v0.46.

In the meantime, should we change to what we're doing in 0.46? Maybe moving to a simple ante and post handler approach where each of those has store branching would be an improvement, even if it's a bit of a step backwards?

[alexanderbez]

In the meantime, should we change to what we're doing in 0.46?

Yes, absolutely. The flow and model is just way too confusing...too much recursion. This is what I recommended to @AmauryM -- release v0.46 with the caveat that middleware is subject to API breakage. So I recommend we refactor it while releasing 0.46, but only a medium term slightly better refactor, while we work on a more permanent solution later.

[amaury1093]

In the meantime, should we change to what we're doing in 0.46? Maybe moving to a simple ante and post handler approach where each of those has store branching would be an improvement, even if it's a bit of a step backwards?

That could work. It solves problem 2 in OP, but not 1. We can think of middleware dependencies later.

Alternatively, my preferred solution would be to revert all middleware changes, do branching in baseapp like before, and add a new posthandler in baseapp for tips. It's an even bigger step backwards, but at the end of the day it's less API-breaking changes, and probably safer too (less code changes compared to v0.45).

[ValarDragon]

I'm very in favor of:

• Separate AnteHandler and PostHandlers
• Dependency graph solutions (I quite like it in osmosis =) ), I'm happy to upstream the constituent packages to the SDK if we want to ease testing out API's with them. (We can put it in an internal package, so we'd have no concerns around API promises)