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)

[robert-zaremba]

I would still love to see a sequential approach rather than recursive one.

I wouldn't call it recursion: a middleware doesn't call itself. It wait's for other downstream middleware to complete and continue it's execution.

[robert-zaremba]

Alternatively, my preferred solution would be to revert all middleware changes, do branching in baseapp like before

Many projects were asking and updating their code for the new middleware design. This will be a pain to do the revert and redo the work.

[alexanderbez]

I wouldn't call it recursion: a middleware doesn't call itself. It wait's for other downstream middleware to complete and continue it's execution.

It is 100% recursion...you're literally calling next in each handler. Keeping a mental model of what was happening, when and where is nearly impossible. It was also really poorly designed IMO, not to mention implemented incorrectly as mentioned initially (state not being reverted). With the proposed fix by @AmauryM it made it even more confusing.

[amaury1093]

To be fair, antehandlers in this case would also be recursive by your definition.

I think what made middlewares more confusing than antehandlers was that each middleware could be ante-runMsgs and post-runMsgs at the same time, and tracking that in a big pile of 15 middlewares is hard.

[alexanderbez]

Yes, technically that's true, but AnteHandlers were always ordered and executed sequentially (AnteDecorator) and didn't have this pre/post nonsense that makes it nearly impossible to reason about, not to mention that the baseapp called it in a single place and so state commitment and reversion was very easy to reason about.

[robert-zaremba]

Exactly, anthehandlers recuse but the good feature is that they user tail recursion, so correct - it's easy to reason.

Personally I like the pre / post approach - it makes it powerful. However I agree it can create bugs if people don't think about it.
Maybe good a very good documentation and a diagram about all core middlewares + important NOTE for about the middlewares would be a good interim solution? I will think about some alternatives.

[robert-zaremba]

I think @aaronc design is the right one, but it will require adding custom types for correctly resolving dependencies and it will be harder for newcomers.

[aaronc]

I think there are some cases where the middleware wrapping makes sense, but when you have to isolate state changes, it breaks down. A directed graph (i.e. A -> B -> C) is much easier to reason about and you can draw boxes around the parts which are in the same transaction. Otherwise with the middleware approach you end up with (pre A -> pre B -> pre C -> post C -> post B -> post A) and if you have transactions which only wrap say part of the post workflow, then you need to do weird things which is what we ended up with.

[alexanderbez]

I think there are some cases where the middleware wrapping makes sense, but when you have to isolate state changes, it breaks down. A directed graph (i.e. A -> B -> C) is much easier to reason about and you can draw boxes around the parts which are in the same transaction. Otherwise with the middleware approach you end up with (pre A -> pre B -> pre C -> post C -> post B -> post A) and if you have transactions which only wrap say part of the post workflow, then you need to do weird things which is what we ended up with.

Yes, exactly!

[robert-zaremba]

I don't think reverting to anthe handlers is a good solution. Ante handler implementation is more complex and it is "dirty" (involves tangled code which breaks the modularity - see the simulate flag)

Moreover, Antehandlers don't solve problem 1:

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

Antehanders must be correctly ordered as well.

I would claim that it doesn't solve the problem 2 either:

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

Ante handlers use the defer + recover statements to escape the linear flow with "tail recursion" (example: ContextDecorator with GasMeter - sic! 3 responsibilities in a single handler: context, gas meter and simulations). So you don't have a direct graph with Antehandlers either.

With Middlewares, at least this is more clean.

[robert-zaremba]

I wrote more analysis here: https://hackmd.io/UY-Xh1gBQTmK95WXjJ_anw with motivation AGAINST Antehandlers. It also provides another way how we can implement Middleware with example implementation

[alexanderbez]

@robert-zaremba you have dead links in there so I have no idea what Antehandlers vs 0.46-beta2 Middlewares is about or how it's "Better".

As for your alternative, I'm still pretty opposed to it. I really do not like the notion of "pre" and "post" logic being executed in the same context. It's too confusing IMO. While you may argue that it's cleaner and more extensible, I think that's pretty subjective as I can't see how that's the case.

[ValarDragon]

FWIW, I'm not really a fan of the next() api at all. I'd prefer they have a clear ordering, in the way that the ChainAnteHandlers currently enforces.

I feel like theres other ways to handling the gas metering panicing, e.g. making a single "gas handling wrapper" around the rest of the ante handler. Such wrapping activities should be very explicitly specified, rather than via post-handling methods imo. The linearization of events should always remain clear.

[alexanderbez]

Yes, I totally agree @ValarDragon

[aaronc]

Another aspect to the current decision to revert is that we feel that if we're releasing something new, it should be something we want to stick with for a while. That clearly isn't the current state of things and we don't think we'll have a better API ready in a short period of time. So reverting to the previous thing we had working seems better than releasing an API we can't stand behind.

[robert-zaremba]

@alexanderbez sorry, the links were missing a tag, I've updated them

@ValarDragon almost all Antehandlers are written using the decorator style - so with the next parameter.

[robert-zaremba]

@ValarDragon where the linearization of the handlers/middlewares is not clear?

[ValarDragon]

I view that v0.46.0 failed to revert fees upon signature failure as linearization of state reversion logic being very unclear

Also the next api design feels like a hack to simplify some panic recovery, that makes everything pay in complexity. I can't fearlessly add a new ante handler, it could very well just skip every ante handler after it. It should have a worst case of just rejecting a tx, not causing a tx with invalid sig to be accepted.

[amaury1093]

In v0.46.0-rc1 we reverted to antehandlers. I'll keep this issue open for future work on how to best refactor antehandlers.

[tac0turtle]

closing since we reverted the middleware approach