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

Correct cumulative_gas_used calculation #1559

Closed
wants to merge 1 commit into from

Conversation

MOZGIII
Copy link
Contributor

@MOZGIII MOZGIII commented Dec 17, 2024

Closes #1558.

@MOZGIII MOZGIII requested a review from sorpaas as a code owner December 17, 2024 02:32
@MOZGIII
Copy link
Contributor Author

MOZGIII commented Dec 17, 2024

Well, something is broken in the test suite it looks like...

@conr2d
Copy link
Contributor

conr2d commented Dec 17, 2024

Is the block_gas_limit value being respected? Looking through the Frontier code, it seems like there’s a check to see if a single transaction exceeds the block_gas_limit, but I don’t see any part that verifies whether the total gas consumption of the transactions in a block is less than the block_gas_limit.

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Dec 17, 2024

Yes, this is also a problem

@conr2d
Copy link
Contributor

conr2d commented Dec 17, 2024

@MOZGIII

According to the next:

let cumulative_gas_used = if let Some((_, _, receipt)) = pending.last() {
match receipt {
Receipt::Legacy(d) | Receipt::EIP2930(d) | Receipt::EIP1559(d) => {
d.used_gas.saturating_add(used_gas.effective)
}
}
} else {
used_gas.effective
};
match &transaction {
Transaction::Legacy(_) => Receipt::Legacy(ethereum::EIP658ReceiptData {
status_code,
used_gas: cumulative_gas_used,
logs_bloom,
logs,
}),
Transaction::EIP2930(_) => Receipt::EIP2930(ethereum::EIP2930ReceiptData {
status_code,
used_gas: cumulative_gas_used,
logs_bloom,
logs,
}),
Transaction::EIP1559(_) => Receipt::EIP1559(ethereum::EIP2930ReceiptData {
status_code,
used_gas: cumulative_gas_used,
logs_bloom,
logs,
}),
}

Each transaction receipt stores already cumulative_gas_used instead of gas_used. If this behavior is intended, the original code is written correctly. Unlike the return type of eth_getTransactionReceipt RPC call contain both cumulative_gas_used and gas_used, Receipt has only one gas_used field. I need to look into further what Ethereum protocol specifies.

@boundless-forest
Copy link
Collaborator

See #602

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Dec 17, 2024

Right, but then, should we rename the struct field accordingly to avoid further confusion?

@MOZGIII MOZGIII closed this Dec 17, 2024
@boundless-forest
Copy link
Collaborator

Is the block_gas_limit value being respected? Looking through the Frontier code, it seems like there’s a check to see if a single transaction exceeds the block_gas_limit, but I don’t see any part that verifies whether the total gas consumption of the transactions in a block is less than the block_gas_limit.

No check for the block_gas_limit; the block_gas_limit is a fixed value derived from the block_max_weight. In fact, we convert the transaction gas to the transaction weight and ensure all transaction weights are less than the block max weight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suspicious gas calculation at pallet_ethereum::Pallet::store_block
3 participants