Skip to content

internal/ethapi: exclude 7702 authorities from result in eth_createAccessList #31336

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
Merged
merged 7 commits into from
Mar 25, 2025
Merged

internal/ethapi: exclude 7702 authorities from result in eth_createAccessList #31336

merged 7 commits into from
Mar 25, 2025

Conversation

sashabeton
Copy link
Contributor

@sashabeton sashabeton commented Mar 7, 2025 •
edited
Loading

closes #31335

@sashabeton sashabeton changed the title Exclude valid authorities from access list when calling eth_createAccessList core/types, internal/ethapi: exclude valid authorities from access list when calling eth_createAccessList Mar 10, 2025
@sashabeton sashabeton changed the title core/types, internal/ethapi: exclude valid authorities from access list when calling eth_createAccessList eth/tracers/logger, internal/ethapi: exclude valid authorities from access list when calling eth_createAccessList Mar 10, 2025
@sashabeton
Copy link
Contributor Author

I suppose authorization list length should be limited to prevent DoS.
However, I'm not sure about the implementation. Should we use msg.GasLimit to calculate max possible authorizations? Should we pass msg.GasLimit - len(authList) * params.CallNewAccountGas to core.ApplyMessage function?

@sashabeton sashabeton mentioned this pull request Mar 11, 2025
Closed
s1na
s1na previously approved these changes Mar 17, 2025
View reviewed changes
Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

LGTM

@sashabeton
Copy link
Contributor Author

@s1na Thank you for your review!

I would be grateful if you could also take a look at the latest commit (6e18588)
I had concerns about a potential DoS issue, so I decided to push my solution, though I'm not sure if it's correct.

If you and the other reviewers confirm that it makes sense, I'll keep it.

@sashabeton sashabeton requested a review from s1na March 17, 2025 19:28
s1na
s1na reviewed Mar 18, 2025
View reviewed changes
internal/ethapi/api.go Outdated

// Prevent redundant operations if args contain more authorizations than EVM may handle
maxAuthorizations := uint64(*args.Gas) / params.TxAuthTupleGas
if uint64(len(args.AuthorizationList)) <= maxAuthorizations {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good idea to limit the number of authorizations since they incur a heavy signature verification cost. I would instead of changing semantics rather return an error once the cap is reached.

Let's use CallNewAccountGas for the max number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly!
I just realised that CallNewAccountGas is used in state transition, so it should also be used here, thank you!

19c1f8e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about the error message and accidentally used fmt.Errorf, I apologize
c5e3ac1

s1na
s1na approved these changes Mar 19, 2025
View reviewed changes
Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

Thanks LGTM!

@sashabeton
Copy link
Contributor Author

@s1na thanks again! Is there anything else I need to do?

@s1na s1na added this to the 1.15.7 milestone Mar 25, 2025
@s1na
Copy link
Contributor

s1na commented Mar 25, 2025

@s1na thanks again! Is there anything else I need to do?

No please hold on we will merge it soon.

@fjl fjl changed the title eth/tracers/logger, internal/ethapi: exclude valid authorities from access list when calling eth_createAccessList internal/ethapi: exclude 7702 authorities from result in eth_createAccessList Mar 25, 2025
@fjl fjl merged commit c49aadc into ethereum:master Mar 25, 2025
5 of 6 checks passed
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Mar 26, 2025
@sashabeton
internal/ethapi: exclude 7702 authorities from result in eth_createAc…
52bf7fb 
…cessList (ethereum#31336)

closes ethereum#31335

---------

Co-authored-by: sashabeton <sashabeton2007@gmail.com>
@sonhv0212 sonhv0212 mentioned this pull request Apr 3, 2025
Merged
sivaratrisrinivas pushed a commit to sivaratrisrinivas/go-ethereum that referenced this pull request Apr 21, 2025
@sashabeton @sivaratrisrinivas
internal/ethapi: exclude 7702 authorities from result in eth_createAc…
ca367d1 
…cessList (ethereum#31336)

closes ethereum#31335

---------

Co-authored-by: sashabeton <sashabeton2007@gmail.com>
@colinlyguo colinlyguo mentioned this pull request Apr 29, 2025
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s1na s1na s1na approved these changes

@fjl fjl Awaiting requested review from fjl fjl is a code owner

@lightclient lightclient Awaiting requested review from lightclient lightclient is a code owner

@holiman holiman Awaiting requested review from holiman

@rjl493456442 rjl493456442 Awaiting requested review from rjl493456442

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

Successfully merging this pull request may close these issues.

Feature request: exclude valid authorities from the eth_createAccessList result (EIP-7702)
3 participants
@sashabeton @s1na @fjl