feat(sequencer)!: make empty transactions invalid #1609
Conversation
lobstergrindset
changed the title
SuperFluffy
left a comment
SuperFluffy
left a comment
There was a problem hiding this comment.
I strongly believe making non-empty transactions an invariant of the type is wrong, since downstream consumers will implicitly start to rely on this invariant:
For example, the mempool code you are working on will not actively reject empty transactions - it does not need to, empty transactions are invalid, after all!
Because we already have a concrete idea for how empty transactions might be used in the future, I would ask you to create a static check in the ActionHandler::check_stateless implementation for SignedTransaction instead.
|
@SuperFluffy I disagree with that the use case of the empty transaction is useful. Typically signing a transaction means you're going to pay for it, and I don't see that we should allow nonce-replacement of a higher fee paying transaction with an empty one |
SuperFluffy
left a comment
SuperFluffy
left a comment
There was a problem hiding this comment.
Upgrading my request for changes to an approval after offline discussion: we don't want to permit empty transactions. For the use case of nonce-replacement we would prefer to introduce a new action type somewhere down the lone.
I have a minor comment on the display formatting of the newly introduced error variant.
I would still like to get @noot's final ok before merging (i would have preferred to just dismiss my original review, but that doesn't seem to be possible at the moment).
| }, | ||
| #[error("attempted to create bundle with non bundleable `ActionGroup` type: {group}")] | ||
| NotBundleable { group: ActionGroup }, | ||
| #[error("attempted to create ActionGroup with empty list of actions")] |
There was a problem hiding this comment.
A reader would be confused what ActionGroup the error is talking about because fundamentally this is about the actions contained in a transaction not being empty:
| #[error("attempted to create ActionGroup with empty list of actions")] | |
| #[error("actions cannot be empty")] |
noot
left a comment
noot
left a comment
There was a problem hiding this comment.
looks good to me. i agree that if we implement nonce replacement, we need a way to specify fees (potentially per action, so eg a zero value transfer with a higher fee than the tx being replaced). disallowing empty txs in the meantime is good!
lobstergrindset
force-pushed
the
lilyjjo/empty_transaction_invalid
branch
from
f675269 to
b483357
Compare
* main: (34 commits) feat(proto): add bundle and optimistic block apis (#1519) feat(sequencer)!: make empty transactions invalid (#1609) chore(sequencer): simplify boolean expressions in `transaction container` (#1595) refactor(cli): merge argument parsing and command execution (#1568) feat(charts): astrotrek chart (#1513) chore(charts): genesis template to support latest changes (#1594) fix(ci): code freeze action fix (#1610) chore(sequencer)!: exclusively use Borsh encoding for stored data (ENG-768) (#1492) ci: code freeze through github actions (#1588) refactor(sequencer): use builder pattern for transaction container tests (#1592) feat(conductor)!: implement chain ID checks (#1482) chore(ci): upgrade audit-check (#1577) feat(sequencer)!: transaction categories on UnsignedTransaction (#1512) fix(charts): sequencer prometheus rules (#1598) chore(all): Migrate all instances of `#[allow]` to `#[expect]` (#1561) chore(charts,sequencer-faucet): asset precision (#1517) chore(docs): endpoints (#1543) fix(docker): use target binary build param as name of image entrypoint (#1573) fix(ci): ibc bridge test timeout (#1587) Feature: Add `graph-node` to charts. (#1489) ...
3 participants
Summary
This PR makes empty transactions (i.e. no actions) invalid.
Background
Transactions with no actions cost nothing and could be used to clog the mempool and blocks. In the future if we want a dummy transaction to aid in removing actions via nonce replacement, we could do a noop action like a zero balance transfer.
Changes
Made empty transactions invalid during the
UnsignedTransactionconstruction process.Testing
Unit test was added.
Breaking Changelist
Empty transactions are no longer valid.
Related Issues
Link any issues that are related, prefer full github links.
closes #1607