-
Notifications
You must be signed in to change notification settings - Fork 208
feat: sponsored bundles (#25) #675
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -35,6 +35,14 @@ use revm::{ | |||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| use std::{collections::HashMap, sync::Arc}; | ||||||||||||||||||||||||||||||
| use thiserror::Error; | ||||||||||||||||||||||||||||||
| use tracing::trace; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /// Maximum amount that can be sponsored in a single transaction (2 ETH) | ||||||||||||||||||||||||||||||
| const MAX_SPONSOR_AMOUNT: U256 = U256::from_limbs([2_000_000_000_000_000_000, 0, 0, 0]); // 2 * 10^18 wei = 2 ETH | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /// Safety buffer for sponsorship transactions to account for gas price changes, | ||||||||||||||||||||||||||||||
| /// timing differences, and EVM precision issues (0.001 ETH) | ||||||||||||||||||||||||||||||
| const SPONSORSHIP_SAFETY_BUFFER: U256 = U256::from_limbs([1_000_000_000_000_000, 0, 0, 0]); // 0.001 ETH | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| const SPONSORSHIP_SAFETY_BUFFER: U256 = U256::from_limbs([1_000_000_000_000_000, 0, 0, 0]); // 0.001 ETH | |
| const SPONSORSHIP_SAFETY_BUFFER: U256 = U256::from(10).pow(U256::from(15)); // 0.001 ETH |
Copilot
AI
Aug 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using max_fee_per_gas() for fund calculation may be incorrect. For EIP-1559 transactions, the actual gas price depends on the base fee and priority fee. Consider using the effective gas price calculation that accounts for min(max_fee_per_gas, base_fee + max_priority_fee_per_gas).
| let effective_gas_price = tx_inner.max_fee_per_gas(); | |
| // Calculate effective gas price according to EIP-1559 rules | |
| let effective_gas_price = match tx_inner.tx_type() { | |
| // EIP-1559 transaction (type 2) | |
| Some(2) => { | |
| let max_fee_per_gas = tx_inner.max_fee_per_gas(); | |
| let max_priority_fee_per_gas = tx_inner.max_priority_fee_per_gas(); | |
| let base_fee = self.ctx.block_env.basefee; | |
| let base_fee_plus_priority = base_fee.saturating_add(max_priority_fee_per_gas); | |
| std::cmp::min(max_fee_per_gas, base_fee_plus_priority) | |
| } | |
| // Legacy or EIP-2930 transaction | |
| _ => tx_inner.gas_price(), | |
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read it correctly all these exits from the sponsorship branch like continue here are related to loop over txs in the bundle which means that this bundle tx will simply be skipped? If so then its not correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No thats not correct. We are prchecking if a tx needs sponsorship prior to simulation. So if it needs sponsorship, and qualifies for sponsorship (ie below our MAX_SPONSOR_AMOUNT and assuming builder balance can support it), it will then attempt to add a sponsorship tx prior to the transaction that would otherwise fail. When it hits continue here, we just skip trying to add a sponsorship tx, and importantly roll back the state. So we're just skipping the shortfall loop, and that loop only attempts to add a sponsorship tx, it doesn't affect the tx that requires the sponsorship.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See next comment, yes this is a bug introduced from extracting code from our fork to this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this branch is not needed, its enough to just use else version of this branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming you mean the if statement on line 800... when coinbase_payment is set to true in the config, the coin bases signing capability will be removed (builder_signer will be None). The coinbase is needed to sign the prepayment transaction for the sponsorship.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized I misread this one.. You're referring to the builder nonce code branch. This is handling the case when we sponsor multiple transactions within a single bundle. So the flow should be:
- Builder's onchain nonce is 100
- First sponsored tx: uses nonce 100, updates nonces_updated to (builder, 100)
- Second sponsored tx: finds (builder, 100) in nonces_updated, uses 101
- Third sponsored tx: finds (builder, 101) in nonces_updated, uses 102
Copilot
AI
Aug 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gas limit value 21_000 is a magic number. Consider defining it as a named constant like SIMPLE_TRANSFER_GAS_LIMIT to make its purpose clear and improve maintainability.
| 21_000, | |
| SIMPLE_TRANSFER_GAS_LIMIT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you check nonce here, code above should give you correct nonce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just defensive programming to ensure if there are any race conditions or alterations introduced accidentally in the future, we catch them. Our fork has a lot more trace logging I added at various points that would actually log this if it ever happened. If it did, we'd miss the slot and also cause demotions of optimistic relay submissions, so it's a critical error to catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all other logs in the builder use style like this
trace!(?tx_hash, success = ..., "Sponsor tx execution result")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if all of these can be simplified dramatically. Currently it adds a new branch for committing sponsored txs while it should be possible to do it in the same way so things like coinbase profit and refundable profit are handled by the same code that its handled now. Otherwise its a bit hard to verify that its correct.
For example, (see comment about continue above) I think this approach can lead to part of the bundle being committed while we should revert that bundle instead.
In other words, better approach would be so the sponsorhip path hits let result = self.commit_tx(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there should be no issues with part of the bundle being committed, see the above response. If a sponsorship tx added prior to an underfunded tx doesn't cause a successful tx (along with profit), then it rolls back to the point before the sponsorship attempt and proceeds normally, meaning the underfunded transaction will fail, causing the bundle to fail unless its in reverting/dropping hashes.
So I just realized I am looking at the code on our fork, not on this fork. This code is a fork from develop branch and I copied things over to this without double checking or testing it. Our fork has diverted significantly and the branching is different. So yes, I think as is this will cause a problem.
The main goal with this approach was to prevent simulation overhead. If we wait until LackOfFundForGasLimit, then we'll have to create a sponsorship transactions before the tx, then retry the tx, and this can happen multiple times in a bundle. There are a lot of LackOfFundForGasLimit simulation errors and the vast majority of them are not profitable to sponsor, so when we tested it, it did significantly increase simulation load with almost no benefit.
We also try not to make too many architecturally significant changes, that way future merges are easier for us. Because of that, we tend to prefer more code duplication rather than more cross cutting changes, as long as it doesn't affect the feature or the performance. So if you guys feel like there's a better long term architecture here, that makes sense, and we'd probably merge that in and help support it moving forward.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,8 +7,8 @@ use std::collections::{HashMap, HashSet}; | |||||||
|
|
||||||||
| use crate::{ | ||||||||
| building::{ | ||||||||
| testing::bundle_tests::setup::NonceValue, BuiltBlockTrace, BundleErr, ExecutionResult, | ||||||||
| OrderErr, TransactionErr, | ||||||||
| testing::bundle_tests::setup::NonceValue, BuiltBlockTrace, BundleErr, ExecutionError, | ||||||||
| ExecutionResult, OrderErr, TransactionErr, | ||||||||
| }, | ||||||||
| primitives::{Bundle, BundleRefund, Order, OrderId, Refund, RefundConfig, TxRevertBehavior}, | ||||||||
| utils::{constants::BASE_TX_GAS, int_percentage}, | ||||||||
|
|
@@ -966,3 +966,70 @@ fn test_original_order_id() -> eyre::Result<()> { | |||||||
|
|
||||||||
| Ok(()) | ||||||||
| } | ||||||||
|
|
||||||||
| #[test] | ||||||||
| fn test_sponsored_transaction() -> eyre::Result<()> { | ||||||||
| let target_block = 11; | ||||||||
| let mut test_setup = TestSetup::gen_test_setup(BlockArgs::default().number(target_block))?; | ||||||||
|
|
||||||||
| test_setup.begin_bundle_order(target_block); | ||||||||
|
|
||||||||
| let sender1 = NamedAddr::User(1); | ||||||||
|
|
||||||||
| let large_value_to_coinbase = 999_999_999_999_999_999_u64; // Just under 1 ETH but gas will push it over | ||||||||
|
||||||||
| let large_value_to_coinbase = 999_999_999_999_999_999_u64; // Just under 1 ETH but gas will push it over | |
| // Just under 1 ETH but gas will push it over | |
| let large_value_to_coinbase = U256::from(10).pow(U256::from(18)) - U256::from(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number should be replaced with a more maintainable constant. Consider using
U256::from(2) * U256::from(10).pow(U256::from(18))or a similar expression that makes the calculation clearer.