fix: fill state candidates

Author: prestwich

crates/bundle/src/send/driver.rs

@@ -40,22 +40,6 @@ where
     pub bundle_orders: AggregateOrders,
 }
 
-impl<Db, Insp> From<signet_evm::EvmNeedsTx<Db, Insp>> for DriverOutput<Db, Insp>
-where
-    Db: Database,
-    Insp: Inspector<Ctx<Db>>,
-{
-    fn from(evm: signet_evm::EvmNeedsTx<Db, Insp>) -> Self {
-        Self {
-            host_evm: Some(evm),
-            total_gas_used: 0,
-            beneficiary_balance_increase: U256::ZERO,
-            bundle_fills: AggregateFills::default(),
-            bundle_orders: AggregateOrders::default(),
-        }
-    }
-}
-
 impl<Db, Insp> DriverOutput<Db, Insp>
 where
     Db: Database,
@@ -141,9 +125,8 @@ where
     /// The bundle to apply.
     bundle: &'a SignetEthBundle,
 
-    /// Reference to the fill state to check against. When we modify it, we'll
-    /// clone and take a running total.
-    fill_state: Cow<'b, AggregateFills>,
+    /// Reference to the fill state to check against.
+    pub fill_state: Cow<'b, AggregateFills>,
 
     /// Execution deadline for this bundle. This limits the total WALLCLOCK
     /// time spent simulating the bundle.
@@ -178,7 +161,18 @@ where
         deadline: std::time::Instant,
         fill_state: Cow<'b, AggregateFills>,
     ) -> Self {
-        Self { bundle, fill_state, deadline, output: host_evm.into() }
+        Self {
+            bundle,
+            fill_state,
+            deadline,
+            output: DriverOutput {
+                host_evm: Some(host_evm),
+                total_gas_used: 0,
+                beneficiary_balance_increase: U256::ZERO,
+                bundle_fills: AggregateFills::default(),
+                bundle_orders: AggregateOrders::default(),
+            },
+        }
     }
 
     /// Get a reference to the bundle.
@@ -258,7 +252,6 @@ where
         // We simply run all host transactions first, accumulating their state
         // changes into the host_evm's state. If any reverts, we error out the
         // simulation.
-        let mut host_bundle_fills = AggregateFills::default();
         for tx in host_txs.into_iter() {
             self.output.host_evm = Some(trevm_try!(
                 self.output
@@ -275,13 +268,14 @@ where
                             EVMError::Custom("host transaction reverted".to_string())
                         );
 
+                        // The host fills go in the bundle fills.
                         let host_fills = htrevm
                             .inner_mut_unchecked()
                             .inspector
                             .as_mut_detector()
                             .take_aggregates()
                             .0;
-                        host_bundle_fills.absorb(&host_fills);
+                        self.output.bundle_fills.absorb(&host_fills);
 
                         Ok(htrevm.accept_state())
                     })
@@ -292,8 +286,6 @@ where
                 trevm
             ));
         }
-        // Absorb the host fills into our running total.
-        self.fill_state.to_mut().absorb(&host_bundle_fills);
 
         // -- ROLLUP PORTION --
         for tx in txs.into_iter() {
@@ -324,13 +316,18 @@ where
                 let (tx_fills, tx_orders) =
                     t.inner_mut_unchecked().inspector.as_mut_detector().take_aggregates();
 
+                // These clones are inefficient. We can optimize later if
+                // needed.
+                let mut candidate_fills = self.output.bundle_fills.clone();
+                let mut candidate_orders = self.output.bundle_orders.clone();
+
+                // The candidate is the updated
+                candidate_fills.absorb(&tx_fills);
+                candidate_orders.absorb(&tx_orders);
+
                 // Then we check that the fills are sufficient against the
                 // provided fill state. This does nothing on error.
-                if self
-                    .fill_state
-                    .to_mut()
-                    .checked_remove_ru_tx_events(&tx_fills, &tx_orders)
-                    .is_err()
+                if self.fill_state.check_ru_tx_events(&candidate_fills, &candidate_orders).is_err()
                 {
                     if self.bundle.reverting_tx_hashes().contains(tx_hash) {
                         debug!("transaction marked as revertible, reverting");
@@ -342,9 +339,9 @@ where
                     }
                 }
 
-                // now we absorb the fills and orders into our running totals.
-                self.output.absorb(&tx_fills, &tx_orders);
-                self.output.use_gas(gas_used);
+                // Now we accept the fills and order candidates
+                self.output.bundle_fills = candidate_fills;
+                self.output.bundle_orders = candidate_orders;
             } else {
                 // EVM Execution did not succeed.
                 // If not success, we are in a revert or halt. If the tx is
@@ -354,11 +351,11 @@ where
                     debug!("transaction reverted, not marked as revertible");
                     return Err(t.errored(BundleError::BundleReverted.into()));
                 }
-                self.output.use_gas(gas_used);
             }
 
             // If we did not shortcut return/continue, we accept the state
             // changes from this transaction.
+            self.output.use_gas(gas_used);
             trevm = t.accept_state()
         }
 

crates/sim/src/env/shared.rs

@@ -100,7 +100,7 @@ where
         Arc::get_mut(&mut self.inner)
             .expect("sims dropped already")
             .rollup_mut()
-            .accept_aggregates(&outcome.fills, &outcome.orders)
+            .accept_aggregates(&outcome.bundle_fills, &outcome.bundle_orders)
             .expect("checked during simulation");
 
         Some(SimulatedItem { gas_used: outcome.gas_used, score: outcome.score, item })

crates/sim/src/env/sim_env.rs

@@ -136,10 +136,10 @@ where
                 // insufficient, we error out. Otherwise we'll return them as
                 // part of the `SimOutcomeWithCache`, to allow _later_ stages to
                 // process them (e.g., to update the fill state).
-                let (fills, orders) =
+                let (bundle_fills, bundle_orders) =
                     trevm.inner_mut_unchecked().inspector.as_mut_detector().take_aggregates();
 
-                self.rollup.fill_state().check_ru_tx_events(&fills, &orders)?;
+                self.rollup.fill_state().check_ru_tx_events(&bundle_fills, &bundle_orders)?;
 
                 // We will later commit these to the trevm DB when the
                 // SimOutcome is accepted.
@@ -169,8 +169,8 @@ where
                     rollup_cache: cache,
                     host_cache: Default::default(),
                     gas_used,
-                    fills,
-                    orders,
+                    bundle_fills,
+                    bundle_orders,
                 })
             }
             Err(e) => Err(SignetEthBundleError::from(e.into_error())),
@@ -206,7 +206,12 @@ where
         let score = driver.beneficiary_balance_increase();
         let outputs = driver.into_outputs();
 
-        let rollup_cache = trevm.into_db().into_cache();
+        // This is redundant with the driver, however, we double check here.
+        // If perf is hit too much we can remove.
+        self.rollup
+            .fill_state()
+            .check_ru_tx_events(&outputs.bundle_fills, &outputs.bundle_orders)?;
+
         let host_cache = outputs.host_evm.map(|evm| evm.into_db().into_cache()).unwrap_or_default();
         trace!(
             gas_used = outputs.total_gas_used,
@@ -217,11 +222,11 @@ where
         Ok(SimOutcomeWithCache {
             cache_rank,
             score: score.to(),
-            rollup_cache,
+            rollup_cache: trevm.into_db().into_cache(),
             host_cache,
             gas_used: outputs.total_gas_used,
-            fills: outputs.bundle_fills,
-            orders: outputs.bundle_orders,
+            bundle_fills: outputs.bundle_fills,
+            bundle_orders: outputs.bundle_orders,
         })
     }
 
@@ -254,13 +259,13 @@ where
         let _og = outer.enter();
 
         // to be used in the scope
-        let this_ref = &self;
+        let this_ref = self.clone();
 
-        std::thread::scope(move |scope| {
+        std::thread::scope(|scope| {
             // Spawn a thread per bundle to simulate.
             for (cache_rank, item) in active_sim.into_iter() {
                 let c = candidates.clone();
-
+                let this_ref = this_ref.clone();
                 scope.spawn(move || {
                     let identifier = item.identifier();
                     let _ig = trace_span!(parent: outer_ref, "sim_task", %identifier).entered();
@@ -288,7 +293,6 @@ where
             // Drop the TX so that the channel is closed when all threads
             // are done.
             drop(candidates);
-
             // Wait for each thread to finish. Find the best outcome.
             while let Some(candidate) = candidates_rx.blocking_recv() {
                 // Update the best score and send it to the channel.

crates/sim/src/outcome.rs

@@ -1,9 +1,8 @@
+use crate::SimItem;
 use alloy::primitives::U256;
 use signet_types::{AggregateFills, AggregateOrders};
 use trevm::revm::database::Cache;
 
-use crate::SimItem;
-
 /// A simulation outcome that includes the score, gas used, and a cache of
 /// state changes.
 #[derive(Debug, Clone)]
@@ -27,10 +26,10 @@ pub struct SimOutcomeWithCache {
     pub host_cache: Cache,
 
     /// The aggregate fills after simulation.
-    pub fills: AggregateFills,
+    pub bundle_fills: AggregateFills,
 
     /// The aggregate orders after simulation.
-    pub orders: AggregateOrders,
+    pub bundle_orders: AggregateOrders,
 }
 
 /// An item after simulation, containing the score and gas used.