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(op-precompiles): Check subset of l1 precompiles in op #2204

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

emhane
Copy link
Contributor

@emhane emhane commented Mar 13, 2025

Ref #2189

Checks that subset of l1 precompiles are part of op precompiles wrt hardfork

Copy link

codspeed-hq bot commented Mar 13, 2025

CodSpeed Performance Report

Merging #2204 will degrade performances by 2.07%

Comparing emhane:precompile-subsets (2643c9f) with main (8ed1b16)

Summary

❌ 1 regressions
✅ 7 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
precompile bench | ecrecover precompile 197.1 µs 201.2 µs -2.07%

@emhane emhane force-pushed the precompile-subsets branch from ae1af23 to 528a1bf Compare March 13, 2025 15:25
@emhane
Copy link
Contributor Author

emhane commented Mar 13, 2025

@rakita could you pls explain how tests pass locally without default features, but not in ci?

@rakita
Copy link
Member

rakita commented Mar 13, 2025

Need to check to be sure, but bls does not have a no_std variant so one precompile will be missing.

@rakita
Copy link
Member

rakita commented Mar 14, 2025

Need to check to be sure, but bls does not have a no_std variant so one precompile will be missing.

Lets wait for #2210, it should make that test pass.

blst are not supported for no_std but for kona they accelare all precompiles so they are fine without it.

@rakita
Copy link
Member

rakita commented Mar 19, 2025

#2249 is done so if we merge this, diff would be same. if there is no bls feature and this precompile is called by default it will throw Fatal error.

@emhane
Copy link
Contributor Author

emhane commented Mar 19, 2025

found the issue here, when blst is not enabled, we still add all the addresses to Prague with bls12_381_precompiles_not_supported, but not to isthmus

@emhane
Copy link
Contributor Author

emhane commented Mar 19, 2025

odd , this still works locally, but in ci the output is different

@emhane
Copy link
Contributor Author

emhane commented Mar 19, 2025

ah it's the precompile function pointers that are different in ci for Prague and isthmus sets...no easy way around that unfortunately. equality of function pointers is needed to see that granite overwrites the bn128 pair. will simplify the difference check to just check for address, to make it pass in ci. that granite overwrites bn128 pair is checked in another test already.

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.

2 participants