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(op, txpool): add additional update routine for conditionals #14497

Merged
merged 6 commits into from
Feb 15, 2025

Conversation

fgimenez
Copy link
Member

Closes: #14196

@fgimenez fgimenez added the A-op-reth Related to Optimism and op-reth label Feb 14, 2025
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.

cool,

pedantic nit, and a suggestion
otherwise this lgtm

let Some(event) = events.next().await else { break };
if let CanonStateNotification::Commit { new } = event {
if !new.is_empty() {
let header = new.tip().clone().into_header();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I blieve there should be clone_header

if !new.is_empty() {
let header = new.tip().clone().into_header();
let mut to_remove = Vec::new();
for tx in &pool.pooled_transactions() {
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 this should be fine, even with a low blocktime to run every 1-2s

@hai-rise is support for 4337 conditionals even relevant for you? if not you can just not spawn this task

}
}
}
let _ = pool.remove_transactions(to_remove);
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 we also want some metrics for this

Comment on lines 517 to 524
let chain_events = ctx.provider().canonical_state_stream();
ctx.task_executor().spawn_critical(
"Op txpool maintenance task",
reth_optimism_txpool::maintain::maintain_transaction_pool_future(
pool,
chain_events,
),
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ideally this also has access to the enable conditional setting so we can spawn this based on that, I believe we can pass this setting down

Comment on lines +111 to +112
OpPoolBuilder::default()
.with_enable_tx_conditional(self.args.enable_tx_conditional),
Copy link
Collaborator

Choose a reason for hiding this comment

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

great that we can already propagate additional cli settings like this

@mattsse mattsse enabled auto-merge February 15, 2025 08:18
@mattsse mattsse added this pull request to the merge queue Feb 15, 2025
Merged via the queue into main with commit e4c8e47 Feb 15, 2025
43 checks passed
@mattsse mattsse deleted the fgimenez/conditional-op-update branch February 15, 2025 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-op-reth Related to Optimism and op-reth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add additional op update routine for conditionals
2 participants