Several EVM Precompile Clean-Ups & Renamings #3655
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
| import type { ExecResult } from '../types.js' | ||
| import type { PrecompileInput } from './types.js' | ||
|
|
||
| export function precompile01(opts: PrecompileInput): ExecResult { | ||
| const pName = getPrecompileName('01') |
There was a problem hiding this comment.
This constant can be moved outside this method, such that it does not get called each time if this precompile gets called (micro-optimization). (For all precompiles)
There was a problem hiding this comment.
(and totally not so "micro", makes a lot of sense)
There was a problem hiding this comment.
Ah, this does not seem to work, CI breaks on several occasions:
When looking at the code, I think this is due to circular imports (this whole precompile code is generally not so optimally organized, but I would not want to expand this into a full round refactor), so the precompiles/index.ts file is importing all the precompiles but is then itself exporting this getPrecompileName method, this does not seem to be able to be resolved.
Can we therefore please keep the non-optimized version? Will delete the respective commit again and re-push. I do not have an easy fix for this.
| import { precompile08 } from './08-ecpairing.js' | ||
| import { precompile06 } from './06-bn254-add.js' | ||
| import { precompile07 } from './07-bn254-mul.js' | ||
| import { precompile08 } from './08-bn254-pairing.js' |
There was a problem hiding this comment.
I am confused now. The EIPs https://eips.ethereum.org/EIPS/eip-196 https://eips.ethereum.org/EIPS/eip-197 mention that these are "alt_bn128"? It seems that bn254 is an alias for this? 🤔
There was a problem hiding this comment.
Yes, these are the same things. It was called bn128 when it had 128-bit security, but then researchers decreased security bits. So it's now bn254, because of 254-bit field
There was a problem hiding this comment.
Yes, I followed Pauls advice here, also googling a bit and stumbling upon articles like https://hackmd.io/@jpw/bn254 for some confirmation that this is the "official" respectively "cryptographically correct" name now.
This naming confusion is a thing though, to add on this: there is even the name "bn256" (!!) around, this seems to be wrong and there is some explanation what happened to these two bits I forgot again. 😂 Geth is using this (wrong) name though, I just discovered yesterday (which should not bother us).
But let's stop here. This is really not something where we can clean up the mess. So: bn254 seems very much to be the best choice here.
holgerd77
force-pushed
the
several-evm-precompile-clean-ups
branch
from
559b7e5
to
88ccf9c
Compare
|
Ok, addressed, ready for re-review! 🙂 |
holgerd77
force-pushed
the
several-evm-precompile-clean-ups
branch
from
88ccf9c
to
23af1ac
Compare
This PR does the following:
ecbased names likeecAddtoo generic, not consistent with bls12)precompiles/index.tsprecompileEntriesas a source of truth (this comes along with renamings as well as inconsistency fixes, e.g. Identity -> IDENTITY, some hex address additions, KZG -> KZG_POINT_EVALUATION)getPrecompileName()a bit more flexible and comfortable, by left padding eventually left out 0s as well as return '' (instead of undefined) if no name available (simplifies typing, alternative would have been to just throw but decided that this might be a bit overblown for this type of method)ec*Gasparam names have also be renamed, tobn254*GasBls12381*Gasparam names have been renamed tobls12381*Gas(all our param names start with lower-case)P.S.: Ignore the in-between hex reordering. I was confused. 🙂 🙃