Suspicious gas calculation at pallet_ethereum::Pallet::store_block

[MOZGIII]

Found this code while doing the code review on frontier.

frontier/frame/ethereum/src/lib.rs

Lines 443 to 455 in db4411c

	let mut cumulative_gas_used = U256::zero();
	for (transaction, status, receipt) in Pending::<T>::get() {
	transactions.push(transaction);
	statuses.push(status);
	receipts.push(receipt.clone());
	let (logs, used_gas) = match receipt {
	Receipt::Legacy(d) | Receipt::EIP2930(d) | Receipt::EIP1559(d) => {
	(d.logs.clone(), d.used_gas)
	}
	};
	cumulative_gas_used = used_gas;
	Self::logs_bloom(logs, &mut logs_bloom);
	}

Why cumulative_gas_used = used_gas?

I'd think it should be something like cumulative_gas_used += used_gas?

Unless used_gas already includes the gas of all previous txs (which doesn't seem to be the case) - this is a bug?

[conr2d]

This clearly looks like a bug. Looking at this commit 278be3e, it seems there was a mistake while modifying the calculation of the gas_used amount, which was previously obtained by addition.

@@ -412,11 +412,18 @@ impl<T: Config> Pallet<T> {
                let mut statuses = Vec::new();
                let mut receipts = Vec::new();
                let mut logs_bloom = Bloom::default();
+               let mut cumulative_gas_used = U256::zero();
                for (transaction, status, receipt) in Pending::<T>::get() {
                        transactions.push(transaction);
                        statuses.push(status);
                        receipts.push(receipt.clone());
-                       Self::logs_bloom(receipt.logs.clone(), &mut logs_bloom);
+                       let (logs, used_gas) = match receipt {
+                               Receipt::Legacy(d) | Receipt::EIP2930(d) | Receipt::EIP1559(d) => {
+                                       (d.logs.clone(), d.used_gas)
+                               }
+                       };
+                       cumulative_gas_used = used_gas;
+                       Self::logs_bloom(logs, &mut logs_bloom);
                }

                let ommers = Vec::<ethereum::Header>::new();
@@ -431,10 +438,7 @@ impl<T: Config> Pallet<T> {
                        difficulty: U256::zero(),
                        number: block_number,
                        gas_limit: T::BlockGasLimit::get(),
-                       gas_used: receipts
-                               .clone()
-                               .into_iter()
-                               .fold(U256::zero(), |acc, r| acc + r.used_gas),
+                       gas_used: cumulative_gas_used,
                        timestamp: UniqueSaturatedInto::<u64>::unique_saturated_into(
                                pallet_timestamp::Pallet::<T>::get(),
                        ),

[MOZGIII]

Let's fix it then!

[conr2d]

Sorry for providing incorrect information earlier. The Ethereum Yellow Paper only specifies that the transaction receipt contains the cumulative gas used value.

In Geth:

type Receipt struct {
	// Consensus fields: These fields are defined by the Yellow Paper
	Type              uint8  `json:"type,omitempty"`
	PostState         []byte `json:"root"`
	Status            uint64 `json:"status"`
	CumulativeGasUsed uint64 `json:"cumulativeGasUsed" gencodec:"required"`
	Bloom             Bloom  `json:"logsBloom"         gencodec:"required"`
	Logs              []*Log `json:"logs"              gencodec:"required"`

	// Implementation fields: These fields are added by geth when processing a transaction.
	/* ... */
	GasUsed           uint64         `json:"gasUsed" gencodec:"required"`
	/* ... */
}

In Reth:

pub struct Receipt {
    pub tx_type: TxType,
    pub success: bool,
    pub cumulative_gas_used: u64,
    pub logs: Vec<Log>,
}

The receipt includes only the cumulative gas used value. When making an RPC call, the gasUsed value is calculated as the difference between the cumulative gas used of the current transaction and that of the previous transaction.

[MOZGIII]

I see! This sure looks suspicious, so, how about we change the naming of the variables?

[boundless-forest]

Any renaming ideas? I think we should rename used_gas to cumulative_gas_used at this link: https://github.com/rust-ethereum/ethereum/blob/master/src/receipt.rs#L21C6-L21C14. The current field name is misleading.

[MOZGIII]

Yes, my thinking as well...

[boundless-forest]

Yes, my thinking as well...

Are you willing to raise a pr for this?

[MOZGIII]

Not at this time. I'm busy trying to figure out why our chain exhibits some odd behavior around CREATE opcode... Maybe once I'm done with that...