Skip to content

Commit

Permalink
feat(builder): reject ops if condition not met after failure
Browse files Browse the repository at this point in the history
  • Loading branch information
dancoombs committed Jun 18, 2024
1 parent e9527c5 commit 17d37a7
Show file tree
Hide file tree
Showing 10 changed files with 209 additions and 10 deletions.
94 changes: 90 additions & 4 deletions crates/builder/src/bundle_proposer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ use rundler_utils::{emit::WithEntryPoint, math};
use tokio::{sync::broadcast, try_join};
use tracing::{error, info, warn};

use crate::emit::{BuilderEvent, OpRejectionReason, SkipReason};
use crate::emit::{BuilderEvent, ConditionNotMetReason, OpRejectionReason, SkipReason};

/// Extra buffer percent to add on the bundle transaction gas estimate to be sure it will be enough
const BUNDLE_TRANSACTION_GAS_OVERHEAD_PERCENT: u64 = 5;
Expand Down Expand Up @@ -101,7 +101,7 @@ pub(crate) trait BundleProposer: Send + Sync + 'static {
/// If `min_fees` is `Some`, the proposer will ensure the bundle has
/// at least `min_fees`.
async fn make_bundle(
&self,
&mut self,
min_fees: Option<GasFees>,
is_replacement: bool,
) -> anyhow::Result<Bundle<Self::UO>>;
Expand All @@ -111,6 +111,9 @@ pub(crate) trait BundleProposer: Send + Sync + 'static {
/// If `min_fees` is `Some`, the proposer will ensure the gas fees returned are at least `min_fees`.
async fn estimate_gas_fees(&self, min_fees: Option<GasFees>)
-> anyhow::Result<(GasFees, U256)>;

/// Notifies the proposer that a condition was not met during the last bundle proposal
fn notify_condition_not_met(&mut self);
}

#[derive(Debug)]
Expand All @@ -123,6 +126,7 @@ pub(crate) struct BundleProposerImpl<UO, S, E, P, M> {
settings: Settings,
fee_estimator: FeeEstimator<P>,
event_sender: broadcast::Sender<WithEntryPoint<BuilderEvent>>,
condition_not_met_notified: bool,
_uo_type: PhantomData<UO>,
}

Expand Down Expand Up @@ -155,8 +159,12 @@ where
self.fee_estimator.required_bundle_fees(required_fees).await
}

fn notify_condition_not_met(&mut self) {
self.condition_not_met_notified = true;
}

async fn make_bundle(
&self,
&mut self,
required_fees: Option<GasFees>,
is_replacement: bool,
) -> anyhow::Result<Bundle<UO>> {
Expand Down Expand Up @@ -238,6 +246,16 @@ where
gas_estimate
);

// If recently notified that a bundle condition was not met, check each of
// the conditions again to ensure if they are met, rejecting OPs if they are not.
if self.condition_not_met_notified {
self.condition_not_met_notified = false;
self.check_conditions_met(&mut context).await?;
if context.is_empty() {
break;
}
}

let mut expected_storage = ExpectedStorage::default();
for op in context.iter_ops_with_simulations() {
expected_storage.merge(&op.simulation.expected_storage)?;
Expand Down Expand Up @@ -295,6 +313,7 @@ where
),
settings,
event_sender,
condition_not_met_notified: false,
_uo_type: PhantomData,
}
}
Expand Down Expand Up @@ -549,6 +568,73 @@ where
context
}

async fn check_conditions_met(&self, context: &mut ProposalContext<UO>) -> anyhow::Result<()> {
let futs = context
.iter_ops_with_simulations()
.enumerate()
.map(|(i, op)| async move {
self.check_op_conditions_met(&op.simulation.expected_storage)
.await
.map(|reason| (i, reason))
})
.collect::<Vec<_>>();

let to_reject = future::join_all(futs).await.into_iter().flatten();

for (index, reason) in to_reject {
self.emit(BuilderEvent::rejected_op(
self.builder_index,
self.op_hash(&context.get_op_at(index)?.op),
OpRejectionReason::ConditionNotMet(reason),
));
self.reject_index(context, index).await;
}

Ok(())
}

async fn check_op_conditions_met(
&self,
expected_storage: &ExpectedStorage,
) -> Option<ConditionNotMetReason> {
let futs = expected_storage
.0
.iter()
.map(|(address, slots)| async move {
let storage = match self
.provider
.batch_get_storage_at(*address, slots.keys().copied().collect())
.await
{
Ok(storage) => storage,
Err(e) => {
error!("Error getting storage for address {address:?} failing open: {e:?}");
return None;
}
};

for ((slot, expected), actual) in slots.iter().zip(storage) {
if *expected != actual {
return Some(ConditionNotMetReason {
address: *address,
slot: *slot,
expected: *expected,
actual,
});
}
}
None
});

let results = future::join_all(futs).await;
for result in results {
if result.is_some() {
return result;
}
}
None
}

async fn reject_index(&self, context: &mut ProposalContext<UO>, i: usize) {
let changed_aggregator = context.reject_index(i);
self.compute_aggregator_signatures(context, &changed_aggregator)
Expand Down Expand Up @@ -2154,7 +2240,7 @@ mod tests {
.expect_aggregate_signatures()
.returning(move |address, _| Ok(signatures_by_aggregator[&address]().unwrap()));
let (event_sender, _) = broadcast::channel(16);
let proposer = BundleProposerImpl::new(
let mut proposer = BundleProposerImpl::new(
0,
pool_client,
simulator,
Expand Down
27 changes: 24 additions & 3 deletions crates/builder/src/bundle_sender.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ enum SendBundleAttemptResult {
NoOperations,
// Replacement Underpriced
ReplacementUnderpriced,
// Condition not met
ConditionNotMet,
// Nonce too low
NonceTooLow,
}
Expand Down Expand Up @@ -255,6 +257,11 @@ where
info!("Replacement transaction underpriced, entering cancellation loop");
state.update(InnerState::Cancelling(inner.to_cancelling()));
}
Ok(SendBundleAttemptResult::ConditionNotMet) => {
info!("Condition not met, notifying proposer and starting new bundle attempt");
self.proposer.notify_condition_not_met();
state.reset();
}
Err(error) => {
error!("Bundle send error {error:?}");
self.metrics.increment_bundle_txns_failed();
Expand Down Expand Up @@ -487,14 +494,20 @@ where
Ok(SendBundleAttemptResult::Success)
}
Err(TransactionTrackerError::NonceTooLow) => {
warn!("Replacement transaction underpriced");
self.metrics.increment_bundle_txn_nonce_too_low();
warn!("Bundle attempt nonce too low");
Ok(SendBundleAttemptResult::NonceTooLow)
}
Err(TransactionTrackerError::ReplacementUnderpriced) => {
self.metrics.increment_bundle_txn_replacement_underpriced();
warn!("Replacement transaction underpriced");
warn!("Bundle attempt replacement transaction underpriced");
Ok(SendBundleAttemptResult::ReplacementUnderpriced)
}
Err(TransactionTrackerError::ConditionNotMet) => {
self.metrics.increment_bundle_txn_condition_not_met();
warn!("Bundle attempt condition not met");
Ok(SendBundleAttemptResult::ConditionNotMet)
}
Err(e) => {
error!("Failed to send bundle with unexpected error: {e:?}");
Err(e.into())
Expand All @@ -505,7 +518,7 @@ where
/// Builds a bundle and returns some metadata and the transaction to send
/// it, or `None` if there are no valid operations available.
async fn get_bundle_tx(
&self,
&mut self,
nonce: U256,
required_fees: Option<GasFees>,
is_replacement: bool,
Expand Down Expand Up @@ -964,6 +977,14 @@ impl BuilderMetrics {
metrics::counter!("builder_bundle_replacement_underpriced", "entry_point" => self.entry_point.to_string(), "builder_index" => self.builder_index.to_string()).increment(1);
}

fn increment_bundle_txn_nonce_too_low(&self) {
metrics::counter!("builder_bundle_nonce_too_low", "entry_point" => self.entry_point.to_string(), "builder_index" => self.builder_index.to_string()).increment(1);
}

fn increment_bundle_txn_condition_not_met(&self) {
metrics::counter!("builder_bundle_condition_not_met", "entry_point" => self.entry_point.to_string(), "builder_index" => self.builder_index.to_string()).increment(1);
}

fn increment_cancellation_txns_sent(&self) {
metrics::counter!("builder_cancellation_txns_sent", "entry_point" => self.entry_point.to_string(), "builder_index" => self.builder_index.to_string()).increment(1);
}
Expand Down
11 changes: 11 additions & 0 deletions crates/builder/src/emit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,17 @@ pub enum OpRejectionReason {
FailedRevalidation { error: SimulationError },
/// Operation reverted during bundle formation simulation with message
FailedInBundle { message: Arc<String> },
/// Operation's storage slot condition was not met
ConditionNotMet(ConditionNotMetReason),
}

/// Reason for a condition not being met
#[derive(Clone, Debug)]
pub struct ConditionNotMetReason {
pub address: Address,
pub slot: H256,
pub expected: H256,
pub actual: H256,
}

impl Display for BuilderEvent {
Expand Down
11 changes: 11 additions & 0 deletions crates/builder/src/sender/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ pub(crate) enum TxSenderError {
/// Nonce too low
#[error("nonce too low")]
NonceTooLow,
/// Conditional value not met
#[error("storage slot value condition not met")]
ConditionNotMet,
/// Soft cancellation failed
#[error("soft cancel failed")]
SoftCancelFailed,
Expand Down Expand Up @@ -292,6 +295,14 @@ impl From<ProviderError> for TxSenderError {
return TxSenderError::ReplacementUnderpriced;
} else if e.message.contains("nonce too low") {
return TxSenderError::NonceTooLow;
// Arbitrum conditional sender error message
// TODO push them to use a specific error code and to return the specific slot that is not met.
} else if e
.message
.to_lowercase()
.contains("storage slot value condition not met")
{
return TxSenderError::ConditionNotMet;
}
}
TxSenderError::Other(value.into())
Expand Down
3 changes: 3 additions & 0 deletions crates/builder/src/transaction_tracker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ pub(crate) enum TransactionTrackerError {
NonceTooLow,
#[error("replacement transaction underpriced")]
ReplacementUnderpriced,
#[error("storage slot value condition not met")]
ConditionNotMet,
/// All other errors
#[error(transparent)]
Other(#[from] anyhow::Error),
Expand Down Expand Up @@ -403,6 +405,7 @@ impl From<TxSenderError> for TransactionTrackerError {
TxSenderError::ReplacementUnderpriced => {
TransactionTrackerError::ReplacementUnderpriced
}
TxSenderError::ConditionNotMet => TransactionTrackerError::ConditionNotMet,
TxSenderError::SoftCancelFailed => {
TransactionTrackerError::Other(anyhow::anyhow!("soft cancel failed"))
}
Expand Down
46 changes: 44 additions & 2 deletions crates/provider/src/ethers/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,9 @@ use ethers::{
},
};
use reqwest::Url;
use rundler_types::contracts::utils::get_gas_used::{
GasUsedResult, GetGasUsed, GETGASUSED_DEPLOYED_BYTECODE,
use rundler_types::contracts::utils::{
get_gas_used::{GasUsedResult, GetGasUsed, GETGASUSED_DEPLOYED_BYTECODE},
storage_loader::STORAGELOADER_DEPLOYED_BYTECODE,
};
use serde::{de::DeserializeOwned, Serialize};

Expand Down Expand Up @@ -207,6 +208,47 @@ impl<C: JsonRpcClient + 'static> Provider for EthersProvider<C> {
.await
.context("should get gas used")?)
}

async fn batch_get_storage_at(
&self,
address: Address,
slots: Vec<H256>,
) -> ProviderResult<Vec<H256>> {
let mut state_overrides = spoof::State::default();
state_overrides
.account(address)
.code(STORAGELOADER_DEPLOYED_BYTECODE.clone());

let expected_ret_size = slots.len() * 32;
let slot_data = slots
.into_iter()
.flat_map(|slot| slot.to_fixed_bytes())
.collect::<Vec<_>>();

let tx: TypedTransaction = Eip1559TransactionRequest {
to: Some(address.into()),
data: Some(slot_data.into()),
..Default::default()
}
.into();

let result_bytes = self
.call_raw(&tx)
.state(&state_overrides)
.await
.context("should call storage loader")?;

if result_bytes.len() != expected_ret_size {
return Err(anyhow::anyhow!(
"expected {} bytes, got {}",
expected_ret_size,
result_bytes.len()
)
.into());
}

Ok(result_bytes.chunks(32).map(H256::from_slice).collect())
}
}

impl From<EthersProviderError> for ProviderError {
Expand Down
7 changes: 7 additions & 0 deletions crates/provider/src/traits/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,4 +137,11 @@ pub trait Provider: Send + Sync + Debug + 'static {
data: Bytes,
state_overrides: spoof::State,
) -> ProviderResult<GasUsedResult>;

/// Get the storage values at a given address and slots
async fn batch_get_storage_at(
&self,
address: Address,
slots: Vec<H256>,
) -> ProviderResult<Vec<H256>>;
}
1 change: 1 addition & 0 deletions crates/types/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ fn generate_utils_bindings() -> Result<(), Box<dyn error::Error>> {
MultiAbigen::from_abigens([
abigen_of("utils", "GetCodeHashes")?,
abigen_of("utils", "GetGasUsed")?,
abigen_of("utils", "StorageLoader")?,
])
.build()?
.write_to_module("src/contracts/utils", false)?;
Expand Down
2 changes: 1 addition & 1 deletion crates/types/contracts/foundry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ out = 'out'
libs = ['lib']
test = 'test'
cache_path = 'cache'
solc_version = '0.8.23'
solc_version = '0.8.26'

remappings = [
'forge-std/=lib/forge-std/src',
Expand Down
17 changes: 17 additions & 0 deletions crates/types/contracts/src/utils/StorageLoader.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.25;

contract StorageLoader {
fallback() external payable {
assembly {
let cursor := 0

for {} lt(cursor, calldatasize()) {cursor := add(cursor, 0x20)} {
let slot := calldataload(cursor)
mstore(cursor, sload(slot))
}

return(0, cursor)
}
}
}

0 comments on commit 17d37a7

Please sign in to comment.