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.

Sign up for GitHub

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

Jump to bottom

fix(libevm/legacy): PrecompiledStatefulContract gas and remaining gas handling #114

Merged
merged 23 commits into from
Feb 7, 2025
Merged

fix(libevm/legacy): PrecompiledStatefulContract gas and remaining gas handling #114

merged 23 commits into from
Feb 7, 2025

Conversation

qdm12
Copy link

@qdm12 qdm12 commented Feb 4, 2025 •
edited
Loading

Why this should be merged

Following refactor in #73 and a quick-fix on #104 this adds 2 fixes to PrecompiledStatefulContract

  1. Remaining gas higher than the input gas is not allowed (otherwise it would overflow as well)
  2. Remaining gas can be equal to the input gas

How this works

How this was tested

New test and vm.PrecompileEnvironment stub

@qdm12 qdm12 force-pushed the qdm12/libevm/legacy/precompile-gas branch from 67cc308 to c130fa0 Compare February 4, 2025 08:56
qdm12
qdm12 commented Feb 4, 2025
View reviewed changes
@qdm12 qdm12 force-pushed the qdm12/libevm/legacy/precompile-gas branch 2 times, most recently from 508a58d to 5d62635 Compare February 4, 2025 09:02
@qdm12 qdm12 marked this pull request as ready for review February 4, 2025 09:03
@qdm12 qdm12 force-pushed the qdm12/libevm/legacy/precompile-gas branch from 5d62635 to fe5c0bd Compare February 4, 2025 09:48
@qdm12 qdm12 force-pushed the qdm12/libevm/legacy/precompile-gas branch from fe5c0bd to 6a8fb2e Compare February 4, 2025 09:54
ARR4N
ARR4N requested changes Feb 4, 2025
View reviewed changes
qdm12 and others added 12 commits February 4, 2025 13:03
@qdm12 @ARR4N
Add context to assertions
40ea889 
Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com>
Signed-off-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@qdm12 @ARR4N
Remove bad copied comment from stubs_test.go
fabf9d9 
Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com>
Signed-off-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@qdm12 @ARR4N
Simplify check for used gas
7948b4a 
Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com>
Signed-off-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@qdm12
Embed vm.PrecompileEnvironment in stubPrecompileEnvironment
58f2cf5
@qdm12
Move stubPrecompileEnvironment to legacy_test.go
5c2be65
@qdm12
Rename testCase(s) -> test(s)
1cf6551
@qdm12
Remove input field from tests and set it to a constant
1ba366b
@qdm12
Rename test field cRet -> precompileRet
da2f151
@qdm12
Rename test field cRemainingGas -> remainingGas
c34844f
@qdm12
Rename test field cErr -> precompileErr
ef76203
@qdm12
Move env declaration in subtest closer to where it's used
a5bcae2
@qdm12
Use unexported sentinel error and require.ErrorIs
5ca19fb
@qdm12 qdm12 mentioned this pull request Feb 4, 2025
Merged
2 tasks
@qdm12 qdm12 mentioned this pull request Feb 4, 2025
Closed
3 tasks
darioush
darioush approved these changes Feb 4, 2025
View reviewed changes
Copy link

@darioush darioush left a comment

Choose a reason for hiding this comment

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

this LGTM.
I think it is best to have some form of testing that ensures changes to libevm (esp in core/vm) do not break coreth, as in the past the UTs have shown to not be sufficient to catch all cases.

ARR4N
ARR4N requested changes Feb 5, 2025
View reviewed changes
@qdm12 qdm12 requested a review from ARR4N February 5, 2025 12:36
ARR4N
ARR4N approved these changes Feb 6, 2025
View reviewed changes
Copy link
Collaborator

@ARR4N ARR4N left a comment

Choose a reason for hiding this comment

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

One necessary change before merging, please, but otherwise LGTM

qdm12 and others added 2 commits February 6, 2025 13:48
@qdm12 @ARR4N
Simplify if check using env.UseGas
58902bc 
Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com>
Signed-off-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@qdm12
Rework gas logic since it was horrendous
b32c8c3
ARR4N
ARR4N approved these changes Feb 6, 2025
View reviewed changes
Copy link
Collaborator

@ARR4N ARR4N left a comment

Choose a reason for hiding this comment

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

I like the change of gas accounting to align with reality. Suggested a field name to go with it.

@qdm12 qdm12 merged commit f906679 into main Feb 7, 2025
4 checks passed
@qdm12 qdm12 deleted the qdm12/libevm/legacy/precompile-gas branch February 7, 2025 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ARR4N ARR4N ARR4N approved these changes

@darioush darioush darioush approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qdm12 @ARR4N @darioush