fix(x/dynamicfee): account for failed and out-of-gas tx gas in the dynamic fee window

[giunatale]

The dynamic fee base gas price is derived from State.Window, which was populated only by the per-transaction post handler (x/dynamicfee/post). That handler records gas for successfully executed messages only: its cached state write is discarded when message execution fails, and it is skipped entirely when a transaction runs out of gas (the OOG panic unwinds through BaseApp's recovery path before the post handler runs). Failed and out-of-gas transactions are nevertheless charged to the consensus block gas meter, so their gas fills real block space while remaining invisible to congestion pricing.

Fix

Make the current block's window slot authoritative by sourcing it from the consensus block gas meter in UpdateDynamicfee (EndBlock). The block gas meter already accounts for every transaction charged to the block regardless of execution outcome, and the EndBlock context carries the same meter instance each deliverTx mutated via consumeBlockGas. The read is guarded on a non-nil block gas meter, so it is inert in keeper unit tests that preset the window and active in production finalize.

With accounting now authoritative in EndBlock, the per-transaction post handler is redundant and is removed (x/dynamicfee/post deleted).

[giunatale]

Needs to be backported to the release/v0.500.x branch and a new release eventually need to be issued

[tbruyelle]

This is a nice fix, and I'm glad it allows us to get rid of the post handler.

The section Module state updates of the README contains ref to the post handler, this should be removed.

[tbruyelle]

The post handler used to return an error when the tx gas exceeds the max block gas (in state.Update()). This check no longer exists plus we are clamping the blockGas to the max. Therefore I need to be sure that this removed check was redundant: in other words there is some place in the SDK where the max block gas is checked properly.

[giunatale]

Confirmed the removed check was redundant, but let me be precise about the divergence you'd rightly worry about.

There are two separate concerns: enforcing a block's gas limit, and giving the module a bounded reference for utilization. The old state.Update error conflated them, enforcement was never really its job.

Enforcement is done outside the module: CometBFT's consensus MaxGas actually bounds the block, and the app's block gas meter backs it up: getBlockGasMeter builds a bounded NewGasMeter when MaxGas > 0, runTx refuses a tx once BlockGasMeter().IsOutOfGas(), and consumeBlockGas charges each tx into it.

The module's maxBlockGas is a different thing, and (this is your point I guess) it is authoritative for the module's own accounting, by design. The two only diverge when consensus MaxGas is 0/-1: the app then uses an infinite block gas meter (it enforces nothing), while GetMaxBlockGas falls back to DefaultMaxBlockGas. In exactly that unbounded-consensus case, the module's value is the only definition of "a full block," and the clamp is what makes it authoritative — it keeps utilization in [0,1], so the AIMD target/average/net math and the window stay bounded (no runaway fee swings, no unbounded uint64 accumulation). When consensus MaxGas > 0, module max == consensus max, so the clamp is a no-op except to absorb the block gas meter overshooting its limit on the tx whose final consumption trips it (that tx is recovered as out-of-gas, but the meter has already counted it).

So the clamp should be the correct, non-destructive way to hold the window to the module's authoritative max. The old error did a cruder version of the same bound, but destructively (it failed the tx) and, now that this runs in EndBlock, an error there would fail FinalizeBlock and halt the chain, so clamping is the only safe option here.

[julienrbrt]

lgtm!