-
Notifications
You must be signed in to change notification settings - Fork 28
add metrics to track gas used by reverting txs #273
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
Conversation
if result.is_success() { | ||
log_txn(TxnExecutionResult::Success); | ||
num_txs_simulated_success += 1; | ||
self.metrics.successful_tx_gas_used.record(gas_used as f64); |
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.
does this not need reverted_gas_used += gas_used?
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 i'm only tracking reverted gas when the tx reverts
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.
sorry I meant successful_tx_gas_used += gas_used
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.
that would calculate the cumulative gas used by successful txs in a block. reth already tracks this in the reth_sync_execution_gas_processed_total metrics. successful_tx_gas_used is a histogram metric so it will tell us what the distribution of gas looks like used by each tx
} | ||
|
||
impl OpRBuilderMetrics { | ||
#[expect(clippy::too_many_arguments)] |
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.
can group these metrics as a struct instead of adding more args
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.
do you mean a type where each field is one of these args? i'm not sure that's a good idea it would introduce more complexity than just having the directive to ignore the lint here
📝 Summary
So we have a good idea of how much execution resources is taken up by reverting txs
💡 Motivation and Context
Unfortunately reth only tracks gas used by blocks, which isn't accurate for us since we don't include reverting txs in the block: https://github.com/paradigmxyz/reth/blob/main/crates/evm/evm/src/metrics.rs
✅ I have completed the following steps:
make lint
make test