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

Test 🧪: add accessList gas cost tests #3134

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

pLabarta
Copy link
Contributor

@pLabarta pLabarta commented Jan 16, 2025

What does it do?

This PR adds test for 3 cases related to the accessList transaction field introduced in EIP-2930 and implemented through the evm pallet in order to confirm that the field does not present an attack vector for Moonbeam networks. In Moonbeam tho, behavior is somewhat different from the expected for the first 4 addresses and storage keys added to the access list of a transaction; costing less than expected.

Cases are the following:

  • Transactions with big access lists should not be gossiped if they exceed the block gas limit
  • After the 4th one, additional addresses should cost 2400 gas (exact value declared in the EIP)
  • After the 4th one, additional storage keys should cost 1900 gas (exact value declared in the EIP)

Is there something left for follow-up PRs?

While working on these tests it was detected that transactions using the accessList field cost more gas that those without it, even when reusing the same storage and addresses multiple times. This means the feature does not work as expected as it is meant to allow users to save some gas by pre-warming the storage they will access.

@pLabarta pLabarta added B0-silent Changes should not be mentioned in any release notes D2-notlive PR doesn't change runtime code (so can't be audited) labels Jan 16, 2025
Copy link
Contributor

github-actions bot commented Jan 16, 2025

WASM runtime size check:

Compared to target branch

Moonbase runtime: 2288 KB (no changes) ✅

Moonbeam runtime: 2256 KB (no changes) ✅

Moonriver runtime: 2256 KB (no changes) ✅

Compared to latest release (runtime-3401)

Moonbase runtime: 2288 KB (+256 KB compared to latest release) ⚠️

Moonbeam runtime: 2256 KB (+244 KB compared to latest release) ⚠️

Moonriver runtime: 2256 KB (+244 KB compared to latest release) ⚠️

Copy link
Contributor

github-actions bot commented Jan 16, 2025

Coverage Report

@@                    Coverage Diff                     @@
##           master   pablo/access-list-test      +/-   ##
==========================================================
- Coverage   74.58%                   74.47%   -0.11%     
  Files         377                      377              
- Lines       96042                    95720     -322     
==========================================================
- Hits        71632                    71283     -349     
+ Misses      24410                    24437      +27     
Files Changed Coverage
/primitives/account/src/lib.rs 62.50% (-2.34%) 🔽
/runtime/moonbase/src/weights/pallet_treasury.rs 0.00% (-13.24%) 🔽
/runtime/moonbase/tests/integration_test.rs 99.39% (-0.03%) 🔽
/runtime/moonbeam/src/lib.rs 44.41% (-0.02%) 🔽
/runtime/moonbeam/src/weights/pallet_treasury.rs 0.00% (-13.24%) 🔽
/runtime/moonbeam/tests/integration_test.rs 99.39% (-0.02%) 🔽
/runtime/moonriver/src/lib.rs 44.63% (-0.02%) 🔽
/runtime/moonriver/src/weights/pallet_treasury.rs 0.00% (-13.24%) 🔽
/runtime/moonriver/tests/integration_test.rs 99.37% (-0.02%) 🔽

Coverage generated Tue Jan 21 17:12:28 UTC 2025

@pLabarta pLabarta marked this pull request as ready for review January 17, 2025 14:02
@pLabarta pLabarta requested review from a team, stiiifff, TarekkMA and RomarQ January 17, 2025 14:11
TarekkMA
TarekkMA previously approved these changes Jan 17, 2025
Copy link
Contributor

@TarekkMA TarekkMA left a comment

Choose a reason for hiding this comment

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

LGTM, just few nits

@pLabarta pLabarta requested a review from TarekkMA January 21, 2025 19:00
pragma solidity ^0.8.0;

contract AccessListHelper {
uint256 private storedValue;
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 this should be a mapping, accessing the same value multiple times is not useful for checking the AccessList usage. The value is considered warm after the first read, and subsequent reads should be cheaper.

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 mostly for testing if warm storage was cheaper after X subsequent reads when using an access list or not (they did not prove to be cheaper than a TX without the access list field).

For the current tests, I don't this it is necessary to have a mapping unless you think we should add some other scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B0-silent Changes should not be mentioned in any release notes D2-notlive PR doesn't change runtime code (so can't be audited)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants