-
Notifications
You must be signed in to change notification settings - Fork 759
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
EVM: Reduce rustbn-wasm Bundle Size #3449
Comments
One alternative might be to explore the BN254 implementation in the |
Interesting, didn't know this might be an option 👀 |
Wow removing the dep entirely would be a giant win! |
@acolytec3 is this "just" - let's call it - "mathmatical intuition" that this might be a possible replacement with mcl-wasm, or did you read something about this that the one curve can be replaced by the other? Without having any clue ;-) I am just noticing that the 254 from the |
I "think" it's the same thing, despite the weird naming variance. Here's the original EIP and then here's the definition of the curve in ark-works which is the modern, canonical set of ZK crypto libraries for Rust and it looks like they're defining the same curve. I'm going to have to just put some inputs in and see if it gives the same outputs along the way. |
Interesting. I would have never considered THIS base line. 😅 |
bn254 pairings are now available in noble. The naive EVM implementation could be copy-pasted from the test file |
Cool (actually: super-cool 🤩)! I will try an integration today or tomorrow and give you some feedback! |
Closed by #3564 |
Part of #3446
While not being directly suggested in the above meta issue from @roninjin10 this issue is inspired by the write-up and particularly by again looking at the "shocking" graph included 😂 pointing out the large and unproportional share of the rustbn-wasm dependency heaping up onto the EVM bundle size.
When looking a bit into the Rust code from the underlying bn Curve library from Parity it very much seems that only a small part of this code is used to realize the precompile and there should be a potential pretty large gain (substantially > 50% reduction still seems to be conservative) by removing these code parts from the build.
While this might sound a bit frightening on first read after having a closer look I would estimate that this might not be as complicated as expected. At the same time benefits would be pretty substantial and lasting on our side (basically completely a one-time thing). So we should give this minimally a small PoC by forking the
bn
package, delete some first-round-innocent code parts and see if we manage to point the depedency inCargo.toml
to the fork and see if we can build and integrate still successfully.One preparatory work might be that this library still has the ethereum-bn128 as an intermediary library, which would (?) create the somewhat unnecessary need to fork as well, so we should consider to directly go to the
bn
lib and remove this intermediate chain part by integrating the Rust glue code over here.Looking at the Rust glue code from
ethereum-bn128
lib.rs more or less reveals the following methods and objects from thebn
library being used:On the
bn
side all this resides in the main lib.rs file. When looking at this file there seems to be very distinct code parts (fields?), these seems to be not used at all, so to name:So these seems to be likely candidates to be thrown out, including dependents in e.g. the
fields
subfolder.Special note to @acolytec3: this issue can but does not need to be tackled within the next 4 weeks or the next 24 hours. 🤣
Another note: in contrast to other size optimizations regarding tree shaking this issue is not breaking and can be theoretically tackled "stand alone".
The text was updated successfully, but these errors were encountered: