Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(transaction-pool): chaining & static txs for best transactions trait #12320

Merged
merged 22 commits into from
Nov 8, 2024

Conversation

sevazhidkov
Copy link
Contributor

@sevazhidkov sevazhidkov commented Nov 5, 2024

Issue #12307.

  • Allows to combine different pools, prioritized senders and post top-of-block transactions via PayloadTransactionsChain and PayloadTransactionsFixed.
  • Introduces a test that puts a custom transaction at the end of every block.
  • Switches to use PayloadTransactions instead of BestTransactions in block composition. For a simple transition, BestPayloadTransactions wrapper is introduced.

@mattsse mattsse marked this pull request as ready for review November 7, 2024 12:57
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this looks great!

I only have one suggestion so we can merge this and improve how we track invalid senders separately.

also I'm wondering, because the PayloadTransactions are only intended for payload building and we likely want to add some evm related things to ctx, perhaps the txpool crate isn't the best location for this.

but all of the other payload crates are currently also not using anything evm, so we can keep everything where it is for now, and instead move it later once we need to evm support

fn next(
&mut self,
// In the future, `ctx` can include access to state for block building purposes.
ctx: (),
Copy link
Collaborator

Choose a reason for hiding this comment

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

cool, this will be very useful shortly

crates/optimism/payload/src/builder.rs Outdated Show resolved Hide resolved
Comment on lines 201 to 212
impl<T: PoolTransaction<Consensus: Into<TransactionSignedEcRecovered>>> PayloadTransactions
for Box<dyn crate::BestTransactions<Item = Arc<ValidPoolTransaction<T>>>>
{
fn next(&mut self, _ctx: ()) -> Option<TransactionSignedEcRecovered> {
Iterator::next(self).map(|tx| tx.to_recovered_transaction())
}

fn mark_invalid(&mut self, _sender: Address, _nonce: u64) {
// TODO: Implement this. Can't use BestTransactions::mark_invalid directly
// because it requires a sender ID.
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'd prefer not to mix these traits for now, and instead introduce a simple helper type that tracks invalid senders in a HashSet instead, this would unblock the pr entirely.

like

struct BestPayloadTransaction<I> {
  invalid: HashSet<Address>,
  best: I
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, updated, please check if that's what you had in mind.

@sevazhidkov sevazhidkov changed the title [WIP] feat(transaction-pool): chaining & static txs for best transactions trait feat(transaction-pool): chaining & static txs for best transactions trait Nov 7, 2024
@mattsse mattsse added C-enhancement New feature or request A-tx-pool Related to the transaction mempool labels Nov 8, 2024
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

awesome!

@mattsse mattsse added this pull request to the merge queue Nov 8, 2024
@mattsse mattsse removed this pull request from the merge queue due to a manual request Nov 8, 2024
@mattsse mattsse added this pull request to the merge queue Nov 8, 2024
Merged via the queue into paradigmxyz:main with commit 02d2593 Nov 8, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tx-pool Related to the transaction mempool C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants