Skip to content
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

fix(l1): fix the transactions spammer count diff with our node #1650

Merged
merged 23 commits into from
Jan 8, 2025
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
3e3c19e
Merge branch 'check-transactions-hanged-from-the-spammer' into check-…
rodrigo-o Dec 10, 2024
1e220c8
Merge branch 'main' into check-transaction-spammer-count-diff
rodrigo-o Dec 10, 2024
26f6b98
Merge branch 'main' into check-transaction-spammer-count-diff
rodrigo-o Dec 30, 2024
880de3f
Merge branch 'main' into check-transaction-spammer-count-diff
rodrigo-o Jan 2, 2025
119aee2
Merge branch 'main' into check-transaction-spammer-count-diff
rodrigo-o Jan 2, 2025
dc96722
Merge branch 'main' into check-transaction-spammer-count-diff
rodrigo-o Jan 6, 2025
cdad54b
Initial maxPriorityFeePerGas implementation based on the gasPrice one
rodrigo-o Jan 6, 2025
d8b8569
Test the check of the 240 tx addition (80 acc x 3 txs) per block with…
rodrigo-o Jan 6, 2025
532b2d0
Merge branch 'main' into check-transaction-spammer-count-diff
rodrigo-o Jan 6, 2025
921314e
Merge branch 'main' into check-transaction-spammer-count-diff
rodrigo-o Jan 7, 2025
c18ed39
Removed unnedded diffs due to testing
rodrigo-o Jan 7, 2025
3c52243
Initial enhanced implementation of max_priority_fee basing also gas_p…
rodrigo-o Jan 7, 2025
d160d20
Fixing some lint issues and added a comment regarding gas_price results
rodrigo-o Jan 7, 2025
d8e9c86
Fix blob github to add tx params
rodrigo-o Jan 7, 2025
12a8d2e
Merge branch 'main' into check-transaction-spammer-count-diff
rodrigo-o Jan 7, 2025
199e6fe
Format
rodrigo-o Jan 7, 2025
9b078dd
Fixed the relation between max_priority_fee_per_gas and max_fee_per_g…
rodrigo-o Jan 8, 2025
9314caf
Removed old comment
rodrigo-o Jan 8, 2025
4f2c49a
Merge branch 'main' into check-transaction-spammer-count-diff
rodrigo-o Jan 8, 2025
bfe8faf
Simplify and share test utilities across fee related tests
rodrigo-o Jan 8, 2025
3a16239
Merge branch 'main' into check-transaction-spammer-count-diff
rodrigo-o Jan 8, 2025
b6ecbca
Point to main for the el-stability-check and fix a typo in a title
rodrigo-o Jan 8, 2025
b59ea69
Merge branch 'main' into check-transaction-spammer-count-diff
rodrigo-o Jan 8, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions .github/config/assertoor/el-stability-check.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,6 @@ tasks:
title: "Check if CL clients are synced"
- name: check_execution_sync_status
title: "Check if EL clients are synced"
- name: check_consensus_block_proposals
title: "Check the tx spammer is working as expected for block proposal with >= 50 transactions"
config:
minTransactionCount: 50
configVars:
clientPattern: "1-ethrex-lighthouse"

- name: run_task_matrix
title: "Check block proposals from all client pairs"
Expand All @@ -42,6 +36,13 @@ tasks:
configVars:
validatorNamePattern: "validatorPairName"

- name: check_consensus_block_proposals
title: "Check the tx spammer is working as expected for block proposal with >= 50 transactions"
timeout: 3m
config:
minTransactionCount: 240
validatorNamePattern: "[\\d]-ethrex-lighthouse"

- name: run_tasks_concurrent
title: "Check chain stability (reorgs and forks)"
timeout: 7m
Expand Down
5 changes: 4 additions & 1 deletion .github/config/assertoor/network_params_blob.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,7 @@ assertoor_params:
run_block_proposal_check: false
tests:
- https://raw.githubusercontent.com/ethpandaops/assertoor/refs/heads/master/playbooks/stable/blob-transactions-test.yaml
- https://raw.githubusercontent.com/lambdaclass/ethrex/refs/heads/main/.github/config/assertoor/el-stability-check.yaml
- https://raw.githubusercontent.com/lambdaclass/ethrex/refs/heads/check-transaction-spammer-count-diff/.github/config/assertoor/el-stability-check.yaml
rodrigo-o marked this conversation as resolved.
Show resolved Hide resolved

tx_spammer_params:
tx_spammer_extra_args: ["--txcount=3", "--accounts=80"]
79 changes: 79 additions & 0 deletions crates/networking/rpc/eth/fee_calculator.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
use ethrex_storage::Store;
use tracing::error;

use crate::utils::RpcErr;

// TODO: Maybe these constants should be some kind of config.
// How many transactions to take as a price sample from a block.
const TXS_SAMPLE_SIZE: usize = 3;
// How many blocks we'll go back to calculate the estimate.
const BLOCK_RANGE_LOWER_BOUND_DEC: u64 = 20;

// The following comment is taken from the implementation of gas_price and is still valid, the logic was just moved here.

// Disclaimer:
// This estimation is somewhat based on how currently go-ethereum does it.
// Reference: https://github.com/ethereum/go-ethereum/blob/368e16f39d6c7e5cce72a92ec289adbfbaed4854/eth/gasprice/gasprice.go#L153
// Although it will (probably) not yield the same result.
// The idea here is to:
// - Take the last 20 blocks (100% arbitrary, this could be more or less blocks)
// - For each block, take the 3 txs with the lowest gas price (100% arbitrary)
// - Join every fetched tx into a single vec and sort it.
// - Return the one in the middle (what is also known as the 'median sample')
// The intuition here is that we're sampling already accepted transactions,
// fetched from recent blocks, so they should be real, representative values.
// This specific implementation probably is not the best way to do this
// but it works for now for a simple estimation, in the future
// we can look into more sophisticated estimation methods, if needed.
/// Estimate Gas Price based on already accepted transactions,
/// as per the spec, this will be returned in wei.
pub fn estimate_gas_tip(storage: &Store) -> Result<Option<u64>, RpcErr> {
let latest_block_number = storage.get_latest_block_number()?;
let block_range_lower_bound = latest_block_number.saturating_sub(BLOCK_RANGE_LOWER_BOUND_DEC);
// These are the blocks we'll use to estimate the price.
let block_range = block_range_lower_bound..=latest_block_number;
if block_range.is_empty() {
error!(
"Calculated block range from block {} \
up to block {} for gas price estimation is empty",
block_range_lower_bound, latest_block_number
);
return Err(RpcErr::Internal("Error calculating gas price".to_string()));
}
let mut results = vec![];
// TODO: Estimating gas price involves querying multiple blocks
// and doing some calculations with each of them, let's consider
// caching this result, also we can have a specific DB method
// that returns a block range to not query them one-by-one.
for block_num in block_range {
let Some(block_body) = storage.get_block_body(block_num)? else {
error!("Block body for block number {block_num} is missing but is below the latest known block!");
return Err(RpcErr::Internal(
"Error calculating gas price: missing data".to_string(),
));
};

let base_fee = storage
.get_block_header(latest_block_number)
.ok()
.flatten()
.and_then(|header| header.base_fee_per_gas);

// Previously we took the gas_price, now we take the effective_gas_tip and add the base_fee in the RPC
// call if needed.
let mut gas_tip_samples = block_body
.transactions
.into_iter()
.filter_map(|tx| tx.effective_gas_tip(base_fee))
.collect::<Vec<u64>>();

gas_tip_samples.sort();
results.extend(gas_tip_samples.into_iter().take(TXS_SAMPLE_SIZE));
}
results.sort();

match results.get(results.len() / 2) {
None => Ok(None),
Some(gas) => Ok(Some(*gas)),
}
}
95 changes: 24 additions & 71 deletions crates/networking/rpc/eth/gas_price.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use std::cmp::max;

use crate::eth::fee_calculator::estimate_gas_tip;
use ethrex_blockchain::constants::MIN_GAS_LIMIT;
use tracing::error;

use crate::utils::RpcErr;
use crate::{RpcApiContext, RpcHandler};
Expand All @@ -13,84 +15,35 @@ use serde_json::Value;
#[derive(Debug, Clone)]
pub struct GasPrice;

// TODO: Maybe these constants should be some kind of config.
// How many transactions to take as a price sample from a block.
const TXS_SAMPLE_SIZE: usize = 3;
// How many blocks we'll go back to calculate the estimate.
const BLOCK_RANGE_LOWER_BOUND_DEC: u64 = 20;

impl RpcHandler for GasPrice {
fn parse(_: &Option<Vec<Value>>) -> Result<Self, RpcErr> {
Ok(GasPrice {})
}

// Disclaimer:
// This estimation is somewhat based on how currently go-ethereum does it.
// Reference: https://github.com/ethereum/go-ethereum/blob/368e16f39d6c7e5cce72a92ec289adbfbaed4854/eth/gasprice/gasprice.go#L153
// Although it will (probably) not yield the same result.
// The idea here is to:
// - Take the last 20 blocks (100% arbitrary, this could be more or less blocks)
// - For each block, take the 3 txs with the lowest gas price (100% arbitrary)
// - Join every fetched tx into a single vec and sort it.
// - Return the one in the middle (what is also known as the 'median sample')
// The intuition here is that we're sampling already accepted transactions,
// fetched from recent blocks, so they should be real, representative values.
// This specific implementation probably is not the best way to do this
// but it works for now for a simple estimation, in the future
// we can look into more sophisticated estimation methods, if needed.
/// Estimate Gas Price based on already accepted transactions,
/// as per the spec, this will be returned in wei.
fn handle(&self, context: RpcApiContext) -> Result<Value, RpcErr> {
let latest_block_number = context.storage.get_latest_block_number()?;
let block_range_lower_bound =
latest_block_number.saturating_sub(BLOCK_RANGE_LOWER_BOUND_DEC);
// These are the blocks we'll use to estimate the price.
let block_range = block_range_lower_bound..=latest_block_number;
if block_range.is_empty() {
error!(
"Calculated block range from block {} \
up to block {} for gas price estimation is empty",
block_range_lower_bound, latest_block_number
);
return Err(RpcErr::Internal("Error calculating gas price".to_string()));
}
let mut results = vec![];
// TODO: Estimating gas price involves querying multiple blocks
// and doing some calculations with each of them, let's consider
// caching this result, also we can have a specific DB method
// that returns a block range to not query them one-by-one.
for block_num in block_range {
let Some(block_body) = context.storage.get_block_body(block_num)? else {
error!("Block body for block number {block_num} is missing but is below the latest known block!");
return Err(RpcErr::Internal(
"Error calculating gas price: missing data".to_string(),
));
};
let mut gas_price_samples = block_body
.transactions
.into_iter()
.map(|tx| tx.gas_price())
.collect::<Vec<u64>>();
gas_price_samples.sort();
results.extend(gas_price_samples.into_iter().take(TXS_SAMPLE_SIZE));
}
results.sort();

let sample_gas = match results.get(results.len() / 2) {
Some(gas) => *gas,
None => {
// If we don't have enough samples, we'll return the base fee or the min gas limit as a default.
context
.storage
.get_block_header(latest_block_number)
.ok()
.flatten()
.and_then(|header| header.base_fee_per_gas)
.unwrap_or(MIN_GAS_LIMIT)
}
let estimated_gas_tip = estimate_gas_tip(&context.storage)?;

let base_fee = context
.storage
.get_block_header(latest_block_number)
.ok()
.flatten()
.and_then(|header| header.base_fee_per_gas);

// To complete the gas price, we need to add the base fee to the estimated gas.
// If we don't have the estimated gas, we'll use the base fee as the gas price.
// If we don't have the base fee, we'll use the minimum gas limit.
let gas_price = match (estimated_gas_tip, base_fee) {
(Some(gas_tip), Some(base_fee)) => gas_tip + base_fee,
(None, Some(base_fee)) => base_fee,
// TODO: We might want to return null in this cases?
(Some(gas_tip), None) => max(gas_tip, MIN_GAS_LIMIT),
rodrigo-o marked this conversation as resolved.
Show resolved Hide resolved
(None, None) => MIN_GAS_LIMIT,
};

let gas_as_hex = format!("0x{:x}", sample_gas);
let gas_as_hex = format!("0x{:x}", gas_price);
Ok(serde_json::Value::String(gas_as_hex))
}
}
Expand Down Expand Up @@ -184,8 +137,8 @@ mod tests {
Transaction::EIP1559Transaction(EIP1559Transaction {
chain_id: 1,
nonce,
max_fee_per_gas: nonce * BASE_PRICE_IN_WEI,
max_priority_fee_per_gas: (nonce * (10_u64.pow(9))).pow(2),
max_fee_per_gas: nonce * BASE_PRICE_IN_WEI * 2,
max_priority_fee_per_gas: nonce * BASE_PRICE_IN_WEI,
gas_limit: 10000,
to: TxKind::Create,
value: 100.into(),
Expand Down
34 changes: 34 additions & 0 deletions crates/networking/rpc/eth/max_priority_fee.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
use crate::eth::fee_calculator::estimate_gas_tip;

use crate::utils::RpcErr;
use crate::{RpcApiContext, RpcHandler};
use serde_json::Value;

// TODO: This does not need a struct,
// but I'm leaving it like this for consistency
// with the other RPC endpoints.
// The handle function could simply be
// a function called 'estimate'.
#[derive(Debug, Clone)]
pub struct MaxPriorityFee;

impl RpcHandler for MaxPriorityFee {
fn parse(_: &Option<Vec<Value>>) -> Result<Self, RpcErr> {
Ok(MaxPriorityFee {})
}

fn handle(&self, context: RpcApiContext) -> Result<Value, RpcErr> {
let estimated_gas_tip = estimate_gas_tip(&context.storage)?;

let gas_tip = match estimated_gas_tip {
Some(gas_tip) => gas_tip,
None => return Ok(serde_json::Value::Null),
};

let gas_as_hex = format!("0x{:x}", gas_tip);
Ok(serde_json::Value::String(gas_as_hex))
}
}

#[cfg(test)]
mod tests {}
5 changes: 4 additions & 1 deletion crates/networking/rpc/eth/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ pub(crate) mod block;
pub(crate) mod client;
pub(crate) mod fee_market;
pub(crate) mod filter;
pub(crate) mod gas_price;
pub(crate) mod logs;
pub(crate) mod transaction;

mod fee_calculator;
pub(crate) mod gas_price;
pub(crate) mod max_priority_fee;
1 change: 1 addition & 0 deletions crates/networking/rpc/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ pub fn map_eth_requests(req: &RpcRequest, context: RpcApiContext) -> Result<Valu
"eth_sendRawTransaction" => SendRawTransactionRequest::call(req, context),
"eth_getProof" => GetProofRequest::call(req, context),
"eth_gasPrice" => GasPrice::call(req, context),
"eth_maxPriorityFeePerGas" => eth::max_priority_fee::MaxPriorityFee::call(req, context),
unknown_eth_method => Err(RpcErr::MethodNotFound(unknown_eth_method.to_owned())),
}
}
Expand Down
5 changes: 5 additions & 0 deletions test_data/network_params.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,8 @@ additional_services:
- dora
- el_forkmon
- tx_spammer

tx_spammer_params:
# A list of optional extra params that will be passed to the TX Spammer container for modifying its behaviour
tx_spammer_extra_args: ["--txcount=3", "--accounts=80"]
# Some tested seeds: 0x5a8e7b08fef94497, 0x6619e189b8a8b911, 0x52a0d7198393262e, use it as an extra argument for the tx_spammer, i.e: "--seed=0x5a8e7b08fef94497"
Loading