fix(sequencer)!: fix TOCTOU issues by merging check and execution

[SuperFluffy]

Summary

Introduces ActionHandler::check_and_execute, replacing ActionHandler::check_stateful and ActionHandler::execute.

Background

Zelic found that separating execution into check_stateful and execute lead to time-of-check-vs-time-of-use risks: while check_stateful for two actions A and B might each pass, the execution of action A might change the state such that a subsequent execution of action B would now fail - or worse, lead to invalid state.

This patch follows Penumbra (see issues linked at the bottom) in merging them into one atomic operation (atomic in the sense that <Action>::check_and_execute are run sequentially).

There is also a ActionHandler::check_historic trait method, which however is currently not used. It is left for future work to go through the individual checks and move them to check_historic, where applicable (for example, checking address prefixes as long as changing these is not possible after chain init).

Changes

• change ActionHandler trait to merge check_stateful and execute into check_and_execute.
• inject a transaction's signer into the ephemeral object store, setting before and after a transaction is executed. Necessary because this follows the cnidarium_component::ActionHandler convention, but also allows simplifying
• remove the notion of bech32m addresses from many state writes and reads: the prefix only matters at the boundary, not inside the system

Testing

All tests were updated and pass.

NOTE: a useful test would be to craft a problematic transaction that would be rejected with the newest change. However, crafting and executing such a transaction so that it is rejected by the current sequencer but leads to incorrect is left to a follow-up.

Breaking Changelist

While no snapshots guarding against breaking app state were triggered, this is still a breaking change: if supplied with a problematic payload, a sequencer node without this patch will reach a different (and invalid) state compared a node with the present patch.

Related Issues

Original Penumbra issues and fix:
penumbra-zone/penumbra#3960
penumbra-zone/penumbra#3960

Closes #1318

[Fraser999]

I don't see any blocking issues with the changes in terms of moving the checks and executions around. I have a few nitpick comments, but I'd be really keen to see the new transaction::State[Read|Write]Ext removed.

[Fraser999]

I strongly think we should follow the previous style in ActionHandler::execute where the from address is passed directly into the method.

It would mean that we couldn't implement ActionHandler on SignedTransaction, but that's far outweighed by the benefit of not using the StateExt's ephemeral store for passing around the address.

My biggest concern is that we're calling get_current_source().expect() in many places now, and while I think they're all currently safe to unwrap, I guess that might not always be the case. By just passing the address, we avoid the risk of panicking altogether.

There's also a performance hit: every time we read from the ephemeral cache, at least one lock is acquired, and more if the cache entry is nested down in a lower layer of StateDelta.

Finally, there's also the cost in terms of code complexity - it seems simpler to read and follow the code where the from address is just passed in.

[SuperFluffy]

Discussed offline: leaving this in to not change the entire fix. We will rethink and rework the construction of this trait, keeping in mind cache and mempool access.

[Fraser999]

After discussion, approving pending resolution of the comments in follow-up discussions or PR.