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: remove contradictory gas limit check #11958

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mooori
Copy link
Contributor

@mooori mooori commented Aug 16, 2024

validate_chunk_with_chunk_extra_and_receipts_root compares prev_chunk_extra.gas_limit() and chunk_header.gas_limit() twice:

  1. In the first comparison they must be equal.
  2. A few lines down they might differ within the bounds of GAS_LIMIT_ADJUSTMENT_FACTOR.

If I'm not missing anything here, one of these checks should be removed. Since there is a GAS_LIMIT_ADJUSTMENT_FACTOR, I assume gas limit changes should be possible, so this PR removes the first check. Or has anything changed that requires gas limits of prev_chunk_extra and chunk_header to be equal now?

Copy link

codecov bot commented Aug 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.62%. Comparing base (566e926) to head (02fa4d2).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11958      +/-   ##
==========================================
- Coverage   71.62%   71.62%   -0.01%     
==========================================
  Files         810      810              
  Lines      163751   163747       -4     
  Branches   163751   163747       -4     
==========================================
- Hits       117288   117283       -5     
- Misses      41387    41389       +2     
+ Partials     5076     5075       -1     
Flag Coverage Δ
backward-compatibility 0.23% <ø> (+<0.01%) ⬆️
db-migration 0.23% <ø> (+<0.01%) ⬆️
genesis-check 1.33% <ø> (+<0.01%) ⬆️
integration-tests 38.53% <ø> (-0.01%) ⬇️
linux 71.39% <ø> (+0.01%) ⬆️
linux-nightly 71.19% <ø> (-0.02%) ⬇️
macos 53.14% <ø> (-1.20%) ⬇️
pytests 1.60% <ø> (+<0.01%) ⬆️
sanity-checks 1.40% <ø> (+<0.01%) ⬆️
unittests 65.69% <ø> (-0.01%) ⬇️
upgradability 0.28% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mooori mooori marked this pull request as ready for review August 16, 2024 10:48
@mooori mooori requested a review from a team as a code owner August 16, 2024 10:48
@mooori mooori requested a review from wacban August 16, 2024 10:48
Copy link
Contributor

@wacban wacban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that one of the checks is redundant.

I'm almost certain that the gas limit in prev_extra should be equal to the header gas limit. As a rule the chunk header contains info from the prev extra copied byte by byte.

I am not aware of any feature to adjust the gas limit. I know we have adjustments for gas price built in so perhaps there was a mismatch there? Can you do some digging and see if the gas limit can actually be changed and if the gas price adjustment is correctly verified?

@mooori
Copy link
Contributor Author

mooori commented Aug 16, 2024

My understanding is that gas_limit is passed through many layers, but so far it's never modified. Which would explain why these checks haven't caused any problems. For #11460 we're working on enabling gas limit adjustments in tests/benchmarks and #11863 is the first step towards that.

I'm almost certain that the gas limit in prev_extra should be equal to the header gas limit. As a rule the chunk header contains info from the prev extra copied byte by byte.

That's interesting...and leaves me confused as to why GAS_LIMIT_ADJUSTMENT_FACTOR exists? Because the only place where it is used is in the checks in this file.

Can you do some digging and see if the gas limit can actually be changed and if the gas price adjustment is correctly verified?

I've been able to change the gas limit. Doing it here triggers the following check to fail.

prev_chunk_extra.gas_used() != chunk_header.prev_gas_used()

When commenting out this check, nodes seem to run fine with gas limits that are adjusted repeatedly at runtime.

I'm taking the hint you gave in #11863 and will try to see if there is a place where the gas limit can be changed at runtime without triggering this test to fail.

@Longarithm
Copy link
Member

LGTM due to #11863 (comment) but tagging @bowenwang1996 just in case.

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.

3 participants