Add opt-in revert protection #59

ferranbt · 2025-05-21T10:11:13Z

📝 Summary

Missing tests

💡 Motivation and Context

✅ I have completed the following steps:

Run make lint
Run make test
Added tests (if applicable)

avalonche · 2025-05-21T12:12:39Z

crates/op-rbuilder/src/tests/vanilla/revert.rs

+    }
+
+    // Test 2: Bundle reverts. It is not included in the block
+    {}


is this a TODO?

Yes, tests are not finished yet, but feel free to review the rest.

avalonche · 2025-05-21T12:15:35Z

crates/op-rbuilder/src/tx.rs

+pub trait FBPoolTransaction:
+    EthPoolTransaction + MaybeInteropTransaction + MaybeConditionalTransaction + DataAvailabilitySized
+{
+    fn set_exclude_reverting_txs(&mut self, exclude: bool);


naming is a bit confusing, can it be called revert_protected_txs?

avalonche · 2025-05-21T12:18:13Z

crates/op-rbuilder/src/payload_builder_vanilla.rs

@@ -85,8 +75,8 @@ pub struct ExecutedPayload<N: NodePrimitives> {
 #[non_exhaustive]
 pub struct CustomOpPayloadBuilder {
    builder_signer: Option<Signer>,
+    #[allow(dead_code)]


why is there dead code?

SozinM · 2025-05-21T15:29:08Z

crates/op-rbuilder/src/tx.rs

+pub trait FBPoolTransaction:
+    EthPoolTransaction + MaybeInteropTransaction + MaybeConditionalTransaction + DataAvailabilitySized
+{
+    fn set_exclude_reverting_txs(&mut self, exclude: bool);


Suggested change

fn set_exclude_reverting_txs(&mut self, exclude: bool);

fn set_reverting(&mut self, reverting: bool);

SozinM · 2025-05-21T15:29:28Z

crates/op-rbuilder/src/tx.rs

+    EthPoolTransaction + MaybeInteropTransaction + MaybeConditionalTransaction + DataAvailabilitySized
+{
+    fn set_exclude_reverting_txs(&mut self, exclude: bool);
+    fn exclude_reverting_txs(&self) -> bool;


Suggested change

fn exclude_reverting_txs(&self) -> bool;

fn reverting(&self) -> bool;

crates/op-rbuilder/src/tx.rs

SozinM · 2025-05-21T15:37:01Z

Looks almost perfect!

karim-agha · 2025-05-22T11:52:14Z

crates/op-rbuilder/src/main.rs

+                                    rollup_args.supervisor_safety_level,
+                                ),
+                        )
+                        .payload(CustomOpPayloadBuilder::new(


nit: maybe we can make CustomOpPayloadBuilder accept BuilderArgs as a constructor parameter to avoid passing too many individual arguments and changing this code as the list of arguments grows over time.

karim-agha · 2025-05-22T11:55:58Z

crates/op-rbuilder/src/payload_builder_vanilla.rs

@@ -137,10 +128,10 @@ where
            Primitives = OpPrimitives,
        >,
    >,
-    Pool: TransactionPool<Transaction: PoolTransaction<Consensus = TxTy<Node::Types>>>
+    Pool: TransactionPool<Transaction: FBPoolTransaction<Consensus = OpTransactionSigned>>


as part of my change in eliminating the need to have conditional builds I have used this pattern:

pub trait NodeBounds: FullNodeTypes< Types: NodeTypes<Payload = OpEngineTypes, ChainSpec = OpChainSpec, Primitives = OpPrimitives>, > { } impl<T> NodeBounds for T where T: FullNodeTypes< Types: NodeTypes< Payload = OpEngineTypes, ChainSpec = OpChainSpec, Primitives = OpPrimitives, >, > { } pub trait PoolBounds<Node>: TransactionPool<Transaction: OpPooledTx> + Unpin + 'static where Node: FullNodeTypes< Types: NodeTypes< Payload = OpEngineTypes, ChainSpec = OpChainSpec, Primitives = OpPrimitives, >, >, { } impl<Node, T> PoolBounds<Node> for T where T: TransactionPool<Transaction: PoolTransaction<Consensus = TxTy<Node::Types>>> + Unpin + 'static, Node: FullNodeTypes< Types: NodeTypes< Payload = OpEngineTypes, ChainSpec = OpChainSpec, Primitives = OpPrimitives, >, >, <Self as TransactionPool>::Transaction: OpPooledTx, { }

Then every time you need to implement a type that needs those bounds you can just say:

impl<Node, Pool> reth_payload::PayloadBuilder for StandardBuilder<Node, Pool> where Node: NodeBounds, Pool: PoolBounds<Node>, { // ... }

This should make the code significantly more readable and impls less chaotic.

karim-agha · 2025-05-22T11:57:21Z

crates/op-rbuilder/src/payload_builder_vanilla.rs

-            },
-        )
+        self.build_payload(args, |attrs| {
+            #[allow(clippy::unit_arg)]


why is this linter exception needed?

karim-agha · 2025-05-22T11:59:50Z

crates/op-rbuilder/src/revert_protection.rs

+impl Bundle {
+    fn conditional(&self) -> TransactionConditional {
+        TransactionConditional {
+            block_number_min: None,
+            block_number_max: self.block_number_max,
+            known_accounts: Default::default(),
+            timestamp_max: None,
+            timestamp_min: None,
+        }
+    }
+}


Maybe we could keep it close to the structure declaration in bundle.rs?

karim-agha · 2025-05-22T12:06:23Z

crates/op-rbuilder/src/tests/framework/harness.rs

+        // 'Transaction event received target="monitoring" tx_hash="<tx_hash>" kind="discarded"'
+        let builder_logs = std::fs::read_to_string(&self.builder_log_path)?;
+        let txn_log = format!(
+            "Transaction event received target=\"monitoring\" tx_hash=\"{}\" kind=\"discarded\"",


Def not within the scope of this PR, but in the future I think that we should make this more type safe and insert a #[cfg(test)] debug_probe: DebugProbe somewhere in the builder to verify those types of things more reliably. Checking logs texts can be very fragile in the long run.

karim-agha · 2025-05-22T12:10:44Z

crates/op-rbuilder/src/tests/framework/txs.rs

+
+            let tx_hash = provider
+                .client()
+                .request("eth_sendBundle", (bundle,))


Do we want to make this more explicit and make the TransactionBuilder build a Bundle type or have BundleBuilder type altogether? Not entirely sure if this preserves separation of concerns.

crates/op-rbuilder/src/tests/vanilla/revert.rs

Add revert protection endpoint

c992ec5

ferranbt requested review from SozinM and avalonche as code owners May 21, 2025 10:11

ferranbt added 5 commits May 21, 2025 11:49

more stuff

881c7f0

Merge main

fd2dc7d

Finish merge

d41ed94

A bit more stuff

956b4ec

Merge main

ff29aa3

avalonche reviewed May 21, 2025

View reviewed changes

ferranbt added 4 commits May 21, 2025 16:03

Finish tests

e76f583

Remove print

24d2aea

Fix lint

7153070

Fix test

7cf1209

SozinM reviewed May 21, 2025

View reviewed changes

Fix

92d73fd

ferranbt changed the title ~~[Draft] Add revert protection endpoint~~ Add opt-in revert protection May 21, 2025

SozinM reviewed May 21, 2025

View reviewed changes

crates/op-rbuilder/src/tx.rs Show resolved Hide resolved

SozinM reviewed May 21, 2025

View reviewed changes

crates/op-rbuilder/src/tx.rs Outdated Show resolved Hide resolved

ferranbt added 6 commits May 22, 2025 10:28

Fix lint

1623b24

Apply feedback

f50b42b

Bundle with more transactions + more safe checks

b9be9c9

Rename bundle fields

5214bb9

Rename to a more appropiate bundle

d161ffe

Rename

c62095b

ferranbt requested a review from karim-agha May 22, 2025 11:33

karim-agha approved these changes May 22, 2025

View reviewed changes

ferranbt added 5 commits May 22, 2025 14:49

Move TransactionConditional to Bundle file

2d15004

Update test comments

96e51cd

Handle unwraps

98b97be

Fix lint

0420feb

Fix lint

e1930ad

SozinM merged commit 3bc0aff into main May 23, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add opt-in revert protection #59

Add opt-in revert protection #59

Uh oh!

ferranbt commented May 21, 2025

Uh oh!

avalonche May 21, 2025

Uh oh!

ferranbt May 21, 2025 •

edited

Loading

Uh oh!

avalonche May 21, 2025

Uh oh!

avalonche May 21, 2025

Uh oh!

SozinM May 21, 2025

Uh oh!

SozinM May 21, 2025

Uh oh!

Uh oh!

Uh oh!

SozinM commented May 21, 2025

Uh oh!

karim-agha May 22, 2025

Uh oh!

karim-agha May 22, 2025

Uh oh!

karim-agha May 22, 2025

Uh oh!

karim-agha May 22, 2025

Uh oh!

karim-agha May 22, 2025

Uh oh!

karim-agha May 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	fn set_exclude_reverting_txs(&mut self, exclude: bool);
	fn set_reverting(&mut self, reverting: bool);

	fn exclude_reverting_txs(&self) -> bool;
	fn reverting(&self) -> bool;

Add opt-in revert protection #59

Add opt-in revert protection #59

Uh oh!

Conversation

ferranbt commented May 21, 2025

📝 Summary

💡 Motivation and Context

✅ I have completed the following steps:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ferranbt May 21, 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!

Uh oh!

Uh oh!

SozinM commented May 21, 2025

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!

Uh oh!

Uh oh!

Uh oh!

ferranbt May 21, 2025 •

edited

Loading