feat(sequencer)!: refactor authorization checks for use in mempool logic

[Lilyjjo]

Summary

We want to be able to reject transactions from the mempool which have incorrect signers for authority actions. In the old code the authentication checks were mixed in with the action execution logic. This PR refactors the authentication checks out of the ActionHandler::check_and_execute() and into a new handler AuthenticationHandler(), similar to the already existing FeeHandler.

Background

We'd like a way to fail transactions in the mempool's handle_check_tx() that will not pass authorization checks.

Changes

• Refactored authorization checks into the new AuthenticationHandler.
• Added new ABCI error code to communicate to users when they have an invalid signer for an Action in a transaction.
• Added tests for all authentications except IBC and Ics20Withdraw actions.

Testing

• Added unit tests, including one in the mempool.
• Ran locally.

Metrics

• Added mempool timing metric check_tx_duration_seconds_check_authorization.

Breaking Changelist

• Adds new ABCI error code

Changelog

Updated changelog.

[SuperFluffy]

I have pretty big reservations about this change because it breaks away from the reason we merged check & execute in the first place (namely TOCTOU).

It's also a very large conceptual change change from how we execute transactions. This change does not seem motivated by fixing issues in the execution flow, but is driven by requirements of another component.

Finally, this change moves the state reads in the mempool yet again closer to full execution.

At this point, is there even a lot of extra steps left in check_and_execute that the extra logic in the mempool?

[Lilyjjo]

Is TOCTOU a problem here? We're running the same checks for authorization in both the mempool and in the check_and_execute function. I'm not fully understanding your issue with separating out the checks to be handled in a more modular fashion.

There is a large overlap between the work that is being done during execution and work done in the mempool checks, I'm not sure how to get around that without re-introducing the TOCTOU issue.

[SuperFluffy]

Is TOCTOU a problem here? We're running the same checks for authorization in both the mempool and in the check_and_execute function. I'm not fully understanding your issue with separating out the checks to be handled in a more modular fashion.

There is a large overlap between the work that is being done during execution and work done in the mempool checks, I'm not sure how to get around that without re-introducing the TOCTOU issue.

I partially misunderstood the PR then (because I misread the diff), but that makes my reservations stronger: ActionHandler intended to be the single entry point for action execution. If this new logic is never run as part of execution, then ActionHandler is the wrong place for it.

While the mempool had a very limited subset of checks run in its early days, the fact that it starts to run more and more parts of the full execution pipeline is worrisome. How do you propose to keep the checks that are being run in both cases in sync?

[Lilyjjo]

The auth code is still ran during execution, I just refactored it to a different method on the ActionHandler to make it reachable by the mempool. The code should be kept in sync this way because it's the same method being ran to perform the checks. The mempool only needs to run a portion of the checks for entrance into the mempool and not all checks.

[Lilyjjo]

The authorization checks are still being ran during execution here @SuperFluffy

[SuperFluffy]

This is the wrong place to call it. The ActionHandler for Transaction trait delegates to its inner actions.

The idea is that checks and executions are atomic on a per-action level.

This is not atomic and on top of it surprising: why is authorization handled at the transaction but not the action level?

[Lilyjjo]

Ah, ok I see what you mean by calling it in the wrong place. I'll refactor it

[SuperFluffy]

The auth code is still ran during execution, I just refactored it to a different method on the ActionHandler to make it reachable by the mempool. The code should be kept in sync this way because it's the same method being ran to perform the checks. The mempool only needs to run a portion of the checks for entrance into the mempool and not all checks.

I commented on that code. This is a TOCTOU issue which was flagged in an audit and subsequently addressed. Instead of doing the checks atomically this patch is reverting the logic to the status quo ante.

It is incompatible with the execution design.

[Lilyjjo]

@SuperFluffy, is this refactor closer to something that is in line with our execution design? I still refactored out the authority checks but this time I wrote it as a new handler similar to the FeeHandler implementation.

It now runs the checks per-action inside of check_execute_and_pay_fees() similar to the other execution logic. The mempool check differs a little because it runs the checks without execution, but I think that should be fine since none of the sudo change actions can be bundled anyways.

[joroshiba]

This PR is stale because it has been open 45 days with no activity. Remove stale label or this PR will be
closed in 7 days.