feat: add transaction gas limit#214
Conversation
| let gas_used = result.gas_used(); | ||
| if gas_used > self.max_gas_per_txn { | ||
| info!("builder txn took execessive gas. not included in block"); | ||
| return Ok(()); |
There was a problem hiding this comment.
this is out builder transaction, not needed here
We should implement this logic inside execute_best_transactions
There was a problem hiding this comment.
ahh thank you for clarifying
|
Also this will need a test, looks good at this point |
akundaz
left a comment
akundaz
left a comment
There was a problem hiding this comment.
Hey, thanks for the contribution!
I just have a few suggestions:
- Let's make this cli argument optional, if it's not provided, we don't check a transaction's gas
- Keep the name consistent -- I like
max_gas_per_txn, so you should make the long arg thenbuilder.max_gas_per_txn
Otherwise I think this is good, just add a test and it should be good to merge!
|
I've added a utility function
|
varun-doshi
requested review from
avalonche,
ferranbt and
karim-agha
as code owners
crates/op-rbuilder/src/args/op.rs
Outdated
| pub chain_block_time: u64, | ||
|
|
||
| /// max gas a transaction can use | ||
| #[arg(long = "builder.max_gas_per_txn", default_value = "25000")] |
There was a problem hiding this comment.
Please remove the default value
| da_config: OpDAConfig::default(), | ||
| specific: S::default(), | ||
| sampling_ratio: 100, | ||
| max_gas_per_txn: Some(25_000), |
There was a problem hiding this comment.
The default should be None
| } | ||
|
|
||
| fn random_big_transaction(self) -> Self { | ||
| self.with_create().with_input(hex!("3593564c000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000a0000000000000000000000000000000000000000000000000000000006883d15f0000000000000000000000000000000000000000000000000000000000000004100504040000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000000000000000046000000000000000000000000000000000000000000000000000000000000004e0000000000000000000000000000000000000000000000000000000000000056000000000000000000000000000000000000000000000000000000000000003c0000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000003090b0e000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000030000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000022000000000000000000000000000000000000000000000000000000000000002a000000000000000000000000000000000000000000000000000000000000001a000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000418fa3488d7e13f0c06ac5f8485d306b5748f4f00000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000e9bacdc46b210000000000000000000000000000000000000000000000000000000da673855a3f900000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000640000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000600000000000000000000000000418fa3488d7e13f0c06ac5f8485d306b5748f4f0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000418fa3488d7e13f0c06ac5f8485d306b5748f4f000000000000000000000000000000fee13a103a10d593b9ae06b3e05f2e7e1c0000000000000000000000000000000000000000000000000009536c7089100000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000418fa3488d7e13f0c06ac5f8485d306b5748f4f000000000000000000000000090ec5e568dfab3242ac3f11cd497ec731e4a5cc0000000000000000000000000000000000000000000000000e92596fd629000000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000000000000000000000000000000090ec5e568dfab3242ac3f11cd497ec731e4a5cc00000000000000000000000000000000000000000000000000000000000000000c").into()) |
There was a problem hiding this comment.
Please explain what this transaction is in code comments
| .with_validation_node(crate::tests::ExternalNode::reth().await?) | ||
| .await?; | ||
|
|
||
| let count = rand::random_range(1..8); |
There was a problem hiding this comment.
Why are you creating a random number of transactions?
There was a problem hiding this comment.
Well I just used this and removed the outer loop...I can add that back again
There was a problem hiding this comment.
I think a test that sends a tx using an amount of gas below the threshold and one above and then checking that the tx that uses too much gas is excluded should be sufficient
| .send() | ||
| .await | ||
| .expect("Failed to send transaction"); | ||
| println!("{}", txs.len()); |
| let block = driver.build_new_block_with_current_timestamp(None).await?; | ||
| let txs = block.transactions; | ||
|
|
||
| println!("{}", txs.len()); |
| max_gas_per_txn: Some(25000), | ||
| ..Default::default() | ||
| })] | ||
| async fn chain_produces_small_tx(rbuilder: LocalInstance) -> eyre::Result<()> { |
There was a problem hiding this comment.
let's combine this test with chain_produces_big_tx_with_gas_limit
create 2 txs (big and small one)
And the check by hash that small transaction got included
| .await | ||
| .expect("Failed to send transaction"); | ||
|
|
||
| // insert txn with gas usage above limit |
There was a problem hiding this comment.
add an assertion to verify that this tx isn't in the block
| .with_validation_node(crate::tests::ExternalNode::reth().await?) | ||
| .await?; | ||
|
|
||
| // insert txn with gas usage but there is no limit |
There was a problem hiding this comment.
similarly assert that this tx is included in the block
|
@varun-doshi Sorry for slow review, we were really busy these 2 week |
|
@varun-doshi Looks great, will merge it once tests will pass! |
3 participants
📝 Summary
Adds a cli parameter named
max_gas_per_txnto signiify if a transaction uses more gas than this value, it should not be added to the block being built.Fixes #202
✅ I have completed the following steps:
make lintmake test