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(Validium): Fix broken integration test #1270

Conversation

SantiagoPittella
Copy link

@SantiagoPittella SantiagoPittella commented Feb 28, 2024

Related to: #107

To be correctly compatible with legacy transactions, the effective gas calculation should change and follow the specification in:
https://eips.ethereum.org/EIPS/eip-1559

What ❔

  • Updates an integration test to expect a lower or equal gas price instead of an equal gas price

Why ❔

  • A test that checks the gas price of a transaction (sent vs received in the receipt) was failing in Validium mode.

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.
  • Spellcheck has been run via zk spellcheck.
  • Linkcheck has been run via zk linkcheck.

core/lib/types/src/fee.rs Outdated Show resolved Hide resolved
@ilitteri
Copy link
Contributor

ilitteri commented Mar 12, 2024

I dove deeper into this, I think this fix could not necessarily be correct even if it makes the test pass.

For Validium we're setting compute_overhead_part to 1.0 as it is suggested here. This affects the fair_l2_gas_price computation as now we're adding gas_overhead_wei to it (if I remove that variable from the sum, the test passes).

With the above context, now focusing on the test itself, after discarding that this is exclusively a Legacy Tx issue (comment the line that overrides the type and run the test to see this), I concluded that there's an incompatibility between the gasPrice from the transaction being sent and the transaction retrieved by the Web3 API. The endpoint gets the transaction from the database implying that the transaction being sent is not exactly the same as the one stored in the database.

Do you have any idea of what could be happening here? Should we ignore this test in Validium? @popzxc @perekopskiy @StanislavBreadless

@StanislavBreadless
Copy link
Contributor

@ilitteri The reason for that is since eth_getGasPrice will return not exact gas price now, but rather a one with a multiplier (to account for the fact that the L1 gas price may rise in the near term), the gasPrice which is sent by your tx in the test is necessarily larger than the effective one (i.e. the actual one that did not have the multiplier).

My recommendation would be probably to remove the multiplier from the eth_getGasPrice estimation (at a risk having users sending txs that get temporarily stuck in the mempool in case of spikes) or, alternatively amend the test to allow gas price to be <= to the one provided in the tx. We do ignore priority fee and it is a common practice among L2s.

@ilitteri
Copy link
Contributor

@ilitteri The reason for that is since eth_getGasPrice will return not exact gas price now, but rather a one with a multiplier (to account for the fact that the L1 gas price may rise in the near term), the gasPrice which is sent by your tx in the test is necessarily larger than the effective one (i.e. the actual one that did not have the multiplier).

My recommendation would be probably to remove the multiplier from the eth_getGasPrice estimation (at a risk having users sending txs that get temporarily stuck in the mempool in case of spikes) or, alternatively amend the test to allow gas price to be <= to the one provided in the tx. We do ignore priority fee and it is a common practice among L2s.

Thanks for the explanation! We've proceeded on updating the test so the expected gasPrice is lower or equal to the tx's

@ilitteri ilitteri changed the title fix(validium): Effective gas price calculation fix(Validium): Fix broken integration test Mar 13, 2024
@ilitteri
Copy link
Contributor

@popzxc if this new change is ok, I think we're done here (the linter and unit-tests are fixed in #1312)

@ilitteri ilitteri merged commit b840979 into matter-labs:feat_validium_pubdata_abstraction Mar 14, 2024
27 of 32 checks passed
@ilitteri ilitteri deleted the fix-effective-gas-price-calculation branch March 14, 2024 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants