POC of interop filtering during block building #15348

SozinM · 2025-03-28T08:56:07Z

emhane

as long as txns aren't marked invalid, this should work, since transactions are cloned when passed to BestPayloadTransactions

reth/crates/transaction-pool/src/pool/parked.rs

Lines 272 to 286 in dd76b42

    
               /// Returns all transactions that satisfy the given basefee. 
        
               /// 
        
               /// Note: this does _not_ remove the transactions 
        
               #[allow(dead_code)] 
        
               pub(crate) fn satisfy_base_fee_transactions( 
        
                   &self, 
        
                   basefee: u64, 
        
               ) -> Vec<Arc<ValidPoolTransaction<T>>> { 
        
                   let ids = self.satisfy_base_fee_ids(basefee); 
        
                   let mut txs = Vec::with_capacity(ids.len()); 
        
                   for id in ids { 
        
                       txs.push(self.get(&id).expect("transaction exists").transaction.clone().into()); 
        
                   } 
        
                   txs 
        
               }

so they stay in the pool for interop maintenance #15295

emhane · 2025-03-28T09:35:50Z

crates/optimism/payload/src/builder.rs

+            // Check that cross chain tx is validated by supervisor for this block
+            if let Some(interop) = interop {
+                if !interop.is_valid(self.config.attributes.timestamp()) {
+                    best_txs.mark_invalid(tx.signer(), tx.nonce());


this line needs to be removed! otherwise the txns have no chance of ever becoming valid

Okay, makes sense, because we will remove them on the next block update anyway

But i think mark_invalid would work too, because we validate txs upon receiving for 1h, so we won't have a case when transaction that wasn't validated ends up in pending pool

right, so we can guarantee this mark_invalid only effects the virtualisation BestTransactions over the pool, i.e. no message is sent to tx pool to change the status of the tx to invalid or similar

the mark_invalid only affects the BestTransactions iterator, see:

#15348 (comment)

this does not interact with the pool

so we need this here

emhane · 2025-03-28T09:43:20Z

crates/optimism/payload/src/builder.rs

+            let interop = tx.interop();
            let tx = tx.into_consensus();
+            // Check that cross chain tx is validated by supervisor for this block
+            if let Some(interop) = interop {


Suggested change

if let Some(interop) = interop {

if let Some(interop) = tx.interop() {

nitpick, redundant bind which doesn't make code more simple to read

The problem is that once i do let tx = tx.into_consensus(); i don't have access to interop
And to make invalid it need to do let tx = tx.into_consensus(); to get the signer :)

Fixed be removing mark_invalid

ah, makes sense, my bad. we can keep mark_invalid, see prev comment.

Anyway presence/absence of mark_invalid won't do anything as we have proper maintenance in place

mattsse

yeah this is easier,

I still want the other feature but this isn't strictly required for sequencer support, and we likely need some additional features for the context, like evm access

mattsse · 2025-03-28T10:30:17Z

crates/optimism/payload/src/builder.rs

+            // maintenance job
+            if let Some(interop) = tx.interop() {
+                if !interop.is_valid(self.config.attributes.timestamp()) {
+                    continue


we need to mark this one as invalid, so that the iter clears the sender's txs that we know won't be valid:

best_txs.mark_invalid(tx.signer(), tx.nonce());

we also should only do this after all the less expensive checks after the 4844 sanity check on L646

mattsse

lgtm

@nkysg for vis because this introduces another call to the interop fn

mattsse · 2025-03-28T11:35:51Z

crates/optimism/payload/src/builder.rs

-                    continue
-                }
-            }
+            let interop = tx.interop();


ah I overlooked the that we need to convert the tx, so we don't actually safe anything by moving the check down below

this is still fine

Yes 😁
We save 1 instruction :)

mattsse

checking what's wrong with the tests

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

…rrors into interop variants (paradigmxyz#15323) Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

mattsse · 2025-03-28T12:02:43Z

fucked up the merge

taking over

mattsse · 2025-03-28T12:06:28Z

crates/optimism/txpool/src/validator.rs

-        transaction.set_interop(TransactionInterop {
-            timeout: self.block_timestamp() + TRANSACTION_VALIDITY_WINDOW_SECS,
-        });


this was the bug that now materialized after we checked it in the sequencer

hm, but we should set interop of the validated tx

still don't understand why the logic is faulty

this always set the interop info even for non interop txs

github-project-automation bot added this to Reth Tracker Mar 28, 2025

github-project-automation bot moved this to Todo in Reth Tracker Mar 28, 2025

POC of interop filtering during block building

263aaaa

SozinM force-pushed the msozin/op-reth-builder-interop-workaround branch from 23d8df4 to 263aaaa Compare March 28, 2025 09:09

SozinM mentioned this pull request Mar 28, 2025

feat(txpool): New OpPayloadTransactions impl for interop #15316

Closed

emhane reviewed Mar 28, 2025

View reviewed changes

Review

a22be33

emhane approved these changes Mar 28, 2025

View reviewed changes

github-project-automation bot moved this from Todo to In Progress in Reth Tracker Mar 28, 2025

mattsse requested changes Mar 28, 2025

View reviewed changes

mattsse marked this pull request as ready for review March 28, 2025 10:33

mattsse requested review from Rjected and fgimenez as code owners March 28, 2025 10:33

Solar Mithril added 3 commits March 28, 2025 18:23

Review

7d112f9

remove error to not break PR

2c28e81

remove error to not break PR

08931f2

mattsse approved these changes Mar 28, 2025

View reviewed changes

mattsse reviewed Mar 28, 2025

View reviewed changes

Solar Mithril and others added 6 commits March 28, 2025 18:51

lint fix

13975e2

chore: replace reth-provider with reth-storage-api (paradigmxyz#15344)

42dacb8

chore: add warning unused deps (paradigmxyz#15340)

f59d28e

feat(txpool): monitor pending interop txs (paradigmxyz#15313)

4beccb0

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

Fixes paradigmxyz#15321 feat(txpool): properly parse supervisor RPC e…

7fcf42e

…rrors into interop variants (paradigmxyz#15323) Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

fix: interop set

254e8f8

mattsse requested a review from gakonst as a code owner March 28, 2025 12:01

mattsse added 2 commits March 28, 2025 13:04

Merge branch 'main' into msozin/op-reth-builder-interop-workaround

a705429

fix: interop set

b9a1b75

mattsse approved these changes Mar 28, 2025

View reviewed changes

mattsse added this pull request to the merge queue Mar 28, 2025

Merged via the queue into paradigmxyz:main with commit d1ed669 Mar 28, 2025
42 checks passed

github-project-automation bot moved this from In Progress to Done in Reth Tracker Mar 28, 2025

SozinM deleted the msozin/op-reth-builder-interop-workaround branch March 28, 2025 12:50

	/// Returns all transactions that satisfy the given basefee.
	///
	/// Note: this does _not_ remove the transactions
	#[allow(dead_code)]
	pub(crate) fn satisfy_base_fee_transactions(
	&self,
	basefee: u64,
	) -> Vec<Arc<ValidPoolTransaction<T>>> {
	let ids = self.satisfy_base_fee_ids(basefee);
	let mut txs = Vec::with_capacity(ids.len());
	for id in ids {
	txs.push(self.get(&id).expect("transaction exists").transaction.clone().into());
	}
	txs
	}

	if let Some(interop) = interop {
	if let Some(interop) = tx.interop() {

POC of interop filtering during block building #15348

POC of interop filtering during block building #15348

Uh oh!

Conversation

SozinM commented Mar 28, 2025 • edited by emhane Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emhane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsse Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

mattsse commented Mar 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SozinM Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SozinM commented Mar 28, 2025 •

edited by emhane

Loading

mattsse Mar 28, 2025 •

edited

Loading

SozinM Mar 28, 2025 •

edited

Loading