Skip to content

feat(sequencer): have app recost transactions on fee or asset change#1411

Merged
lobstergrindset merged 7 commits intolilyjjo/mempool_balance_awarefrom
lilyjjo/app_recost_transcations
Sep 9, 2024
Merged

feat(sequencer): have app recost transactions on fee or asset change#1411
lobstergrindset merged 7 commits intolilyjjo/mempool_balance_awarefrom
lilyjjo/app_recost_transcations

Conversation

@lobstergrindset
Copy link
Copy Markdown
Contributor

@lobstergrindset lobstergrindset commented Aug 27, 2024

Summary

This PR builds on top of the mempool balance aware logic to recost the mempool when a FeeAssetChange or FeeChange action are seen in a finalized block.

There needs to be a follow up PR to ensure that the FeeChange and FeeAssetChange actions are only ran at the end of the block to ensure that transactions are invalidated mid-block due to fee changes.

Background

The mempool needs to recost transactions if the fees are changed, as the original placement of the transaction in the parked or pending structure may be out of date.

Changes

  • Added logic to flag when FeeAssetChange or FeeChange actions are included in a block.
  • The mempool will now re-cost transactions when this happens.
  • Changed the mempool to use the app's state directly instead of using getters

Testing

Unit tests for the mempool and the app logic.

Breaking changes

This doesn't have breaking changes, but the snapshot files were re-generated because I changed the public addresses for CAROL_ADDRESS, BOB_ADDRESS, and JUDY_ADDRESS in the test_utils.rs file. I needed access to their private keys to sign transactions for testing promotion flows on the app level.

Metrics

Added MEMPOOL_RECOSTED counter to provide metrics on how often fees are being changed.

@github-actions github-actions bot added the sequencer pertaining to the astria-sequencer crate label Aug 27, 2024
@lobstergrindset lobstergrindset marked this pull request as ready for review August 27, 2024 18:50
@lobstergrindset lobstergrindset requested a review from a team as a code owner August 27, 2024 18:50
@lobstergrindset lobstergrindset changed the title feat(sequencer): have app recost tranactions on fee or asset change feat(sequencer)!: have app recost tranactions on fee or asset change Aug 27, 2024
@lobstergrindset lobstergrindset requested a review from noot August 27, 2024 19:07
@lobstergrindset lobstergrindset force-pushed the lilyjjo/app_recost_transcations branch from fd00012 to cf83c96 Compare August 28, 2024 13:37
@lobstergrindset lobstergrindset changed the title feat(sequencer)!: have app recost tranactions on fee or asset change feat(sequencer): have app recost tranactions on fee or asset change Aug 28, 2024
Copy link
Copy Markdown
Contributor

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review.

@lobstergrindset lobstergrindset changed the title feat(sequencer): have app recost tranactions on fee or asset change feat(sequencer): have app recost transactions on fee or asset change Sep 6, 2024
}

pub(crate) async fn mock_state_getter() -> StateDelta<Snapshot> {
let storage = cnidarium::TempStorage::new().await.unwrap();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future we might need to return the storage instance too so it can be kept alive. Just now our tests must not be actually committing anything to the DB (since it's dropped at the end of this function). For now though, I think this is fine.

@lobstergrindset lobstergrindset force-pushed the lilyjjo/mempool_balance_aware branch from e5a1276 to 4014969 Compare September 6, 2024 17:33
@lobstergrindset lobstergrindset force-pushed the lilyjjo/mempool_balance_aware branch from 4014969 to a0d157c Compare September 6, 2024 17:34
@lobstergrindset lobstergrindset force-pushed the lilyjjo/app_recost_transcations branch from bd4d910 to 928b3cd Compare September 6, 2024 19:04
@lobstergrindset lobstergrindset force-pushed the lilyjjo/mempool_balance_aware branch from 751c709 to 588ee6b Compare September 9, 2024 14:34
@github-actions github-actions bot added conductor pertaining to the astria-conductor crate sequencer-relayer pertaining to the astria-sequencer-relayer crate composer pertaining to composer labels Sep 9, 2024
@lobstergrindset lobstergrindset force-pushed the lilyjjo/app_recost_transcations branch from 68b1b07 to 928b3cd Compare September 9, 2024 15:06
joroshiba and others added 4 commits September 9, 2024 11:07
Regular 2 week release cuts, includes breaking changes to sequencer,
bridge-withdrawer, patch updates for composer, conductor and cli.
@lobstergrindset lobstergrindset merged commit 6d4e5d0 into lilyjjo/mempool_balance_aware Sep 9, 2024
@lobstergrindset lobstergrindset deleted the lilyjjo/app_recost_transcations branch September 9, 2024 15:34
lobstergrindset added a commit that referenced this pull request Sep 11, 2024
…1411)

This PR builds on top of the mempool balance aware logic to recost the
mempool when a `FeeAssetChange` or `FeeChange` action are seen in a
finalized block.

There needs to be a follow up PR to ensure that the `FeeChange` and
`FeeAssetChange` actions are only ran at the end of the block to ensure
that transactions are invalidated mid-block due to fee changes.

The mempool needs to recost transactions if the fees are changed, as the
original placement of the transaction in the `parked` or `pending`
structure may be out of date.

- Added logic to flag when `FeeAssetChange` or `FeeChange` actions are
included in a block.
- The mempool will now re-cost transactions when this happens.
- Changed the mempool to use the app's state directly instead of using
getters

Unit tests for the mempool and the app logic.

This doesn't have breaking changes, but the snapshot files were
re-generated because I changed the public addresses for `CAROL_ADDRESS`,
`BOB_ADDRESS`, and `JUDY_ADDRESS` in the `test_utils.rs` file. I needed
access to their private keys to sign transactions for testing promotion
flows on the app level.

Added `MEMPOOL_RECOSTED` counter to provide metrics on how often fees
are being changed.

---------

Co-authored-by: Jordan Oroshiba <jordan@astria.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

composer pertaining to composer conductor pertaining to the astria-conductor crate sequencer pertaining to the astria-sequencer crate sequencer-relayer pertaining to the astria-sequencer-relayer crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants