revive fix reported gas used #10148
Merged
revive fix reported gas used #10148
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pgherveou
force-pushed
the
pg/fix-gas-used
branch
from
a7f6b4f to
f832bda
Compare
|
/cmd prdoc --audience runtime_dev --bump patch |
pgherveou
added
the
T7-smart_contracts
…time_dev --bump patch'
pgherveou
commented
Oct 28, 2025
xermicus
approved these changes
Oct 29, 2025
|
Still some discrepancy in thr tx cost for instantiating Debugging... |
lexnv
approved these changes
Oct 29, 2025
pgherveou
commented
Oct 29, 2025
There was a problem hiding this comment.
I agree with most changes in this PR and most of the logic is more correct this way, particularly that receipts use the actual effective gas price and that (effective_gas_price * gas_used) = deducted amount.
I just have my problems with the gas estimate being (total cost) / (effective gas price) instead of (total cost) / (next fee multiplier).
I understand that the resulting gas_used as reported in the receipt should closely agree with the gas estimate but I would
- either accept that gas_used is lower (in Remix the gas estimate seems to also be 15% above the resulting actually used gas)
- don't just deduct the resulting cost (transaction fees + storage deposit) from the user but scale that by (effective gas price) / (next fee multiplier) and deduct/burn that additional amount; at least the user would be fine with that as that is consistent with Ethereum behavior
I was thinking about the last option in the last couple of days. I don't implement it that way yet in my PR but it is the most consistent option: with the first option, if a contract measures it's own gas usage through gasleft() at the beginning and end, it will see a higher usage during execution than what will be the resulting gas_used value. Not sure whether we should worry about that.
19 tasks
pgherveou
commented
Nov 3, 2025
so essentially you want to do - let fee = Pallet::<T>::convert_native_to_evm(match output.storage_deposit {
+ let fee = match output.storage_deposit {
StorageDeposit::Refund(refund) => native_fee.saturating_sub(refund),
StorageDeposit::Charge(amount) => native_fee.saturating_add(amount),
- });
+ };
+
+ let ratio = FixedU128::from_rational(
+ effective_gas_price.as_u128(),
+ Pallet::<T>::evm_base_fee().as_u128(),
+ );
+
+ let fee = Pallet::<T>::convert_native_to_evm(
+ ratio.saturating_mul_int(fee).saturating_add(1_u32.into()),
+ ); |
|
Yes, indeed and I gave my reasoning here: https://shade-verse-e97.notion.site/Gas-vs-Cost-2a08532a7ab580848878f67d756468da?pvs=74 |
|
@pgherveou could we please get this merged today? It's blocking both anvil and embedded EVM at the moment. Thanks! |
will address the latest comments and merge |
Open
3 tasks
Differential Tests ResultsSpecified Tests
Counts
FailuresThe test specifiers seen in this section have the format 'path::case_idx::compilation_mode' and they're compatible with the revive differential tests framework and can be specified to it directly in the same way that they're provided through the The failures are provided in an expandable section to ensure that the PR does not get polluted with information. Please click on the section below for more information Detailed Differential Tests Failure Information
|
|
/cmd prdoc --audience runtime_dev --bump patch --force |
…time_dev --bump patch --force'
pgherveou
commented
Nov 4, 2025
will add that in once the effective_gas_price can deviate from the base_fee, for now it can't So since we can't really test the change from the evm-test-suite, I suggest we keep it for a follow up |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
TorstenStueber
approved these changes
Nov 4, 2025
EgorPopelyaev
approved these changes
Nov 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix
gas_usedcalculation introduced in #9418 to use the actual gas instead of justref_time.With these changes we now guarantee that
tx_cost = effective_gas_price * gas.Note that since we compute gas as
fee / gas_price, this can lead to rounding errors when the chain usesSlowAdjustingFeeUpdate(i.e. the fee is not a multiple of the gas price).The changes in this PR ensure the fee still matches by burning the rounding remainder.
This PR also fixes how the actual fee is computed and introduces a new
compute_actual_feeinConfig::FeeInfo.The previous fee calculation was skipping the
extension_weightin the fee calculation.The updated tests ensure that the tx cost reported in the receipt matches the fees deducted from the user account:
https://github.com/paritytech/evm-test-suite/blob/460b2c9aa3a3019d3508bb5a34a2498ea86035ff/src/gas.test.ts?plain=1#L31-L61