Eip7594 - #20
Conversation
|
Ah ok, pretty cool, didn't see this yet, so for reference: this solves #19 |
|
Hi hi, can you also drop 2-3 sentences what brought you here? Also if you plan to complete this to be be integrated here in the library? That would be helpful for some context! |
Hello, I am Nate, and I am working on Blobkit as a project of the ETHCF. We use the kzg-wasm library in our implementation for portability and browser compatibility. The upcoming Fukasa fork on mainnet this year (and currently live on sepolia) implements EIP7594, which breaks the current logic. Since we had to make this change on our end to maintain blob functionality, I figured I would pass along the change as well. |
…fix commented out test
|
I notice that CI failed on |
|
No, Node 18 can go away! Also, did you give the new version a test in the context of our tx library here with the 4844 txs? So are these APIs aligned already that this work to pass this in like in the example with: const common = new Common({
chain: Mainnet,
hardfork: Hardfork.Cancun,
customCrypto: { kzg },
}) |
No, I haven't, I will give it a try, thanks! PS I have a couple more tests and one more function coming soon, so I switched to draft until that is fully ready. |
|
Ok, it works with that example now.
|
|
Ok, published https://www.npmjs.com/package/kzg-wasm/v/1.0.0-rc.1, can you retry? |
|
It works, I did another test tx https://sepolia.etherscan.io/tx/0xdec15e5b453f00e5a632dc7635311baf813da3ac30d5629f0f0f7521c68edea7 |
|
Ok, I'll take this in now, won't imminently release though, rather sometime between Monday and Wednesday. Will continue to check the API a bit, eventually add some additional docs, generally need some time to work myself in here! |
|
One basic question still, so the old (pre-7594) and the new post-7594 blob handling/proof generation should be working with this, right? |
|
Let me know if you have any remarks (some API docs in README). |
Yes, this compatibility is very important since not all networks support 7594 yet. The only breaking change is in the trusted setup, where we now take in 3 parameters g1_monomial, g2_monomial, and g1_lagrange, when before it was g1, n1, g2, n2. This matches the C implementation which takes in those 3. |
|
Unless you are providing a custom trusted setup, you should be able to safely upgrade to this version. |
|
Side note to anyone who finds this while debugging EIP7594 transaction's not submitting: They are serialized slightly differently from EIP4844, example, notice wrapperVersion |
|
Hey hey, we have some KZG API tests in the Util package here, so we have defined a KZG interface in Util as well to ensure API compatibility with both the JS version and the WASM version. Guess your already worked in this direction, but just to recap: so all the 4844 methods there should remain "untouched" respectively be kept/exposed in a way that these are still available without breaking changes. For the new cell-based methods from 7594 we are still somewhat flexible - nothing released yet - and we can still adjust names or parameter input. That's just as some general information, I haven't looked deeper yet. At the moment we are testing the As a concrete starter: I've opened this PR ethereumjs/ethereumjs-monorepo#4153 for Util, where I started with adjusting the Util KZG tests. The test calling into the
This |
|
Hmmmmm, did trusted setup get called in that test? Yeah, I am happy to take a look, let me see |
|
Ok, I found the issue! The only version would init with only calling loadKZG(), current expects it to look like this @holgerd77 Adding the latter line to the tests will cause them to pass, I can make a quick PR to make it match the earlier behavior |
|
Thanks a lot, I am also currently looking a bit into naming, this is a chaotic to get through and I need to recreate a bit myself: so, I think with our KZG interface we oriented along the lines of the Let me have a look what there and what's missing:
Hmm. Phew. That's several things to still settle. Can you maybe already address the things which are undisputable in your #22 PR, respectively generally have a first look here? |
|
Yeah, that should be fairly straightforward |
|
Added the aliases to the PR |
|
Very cool, thanks! Have merged and released rc.2 and updated https://github.com/ethereumjs/ethereumjs-monorepo/pull/4153/files, now tests are timing out:
Not sure, I will also take a look, but maybe you can also directly spot something. Don't want to directly increase the timeouts, since tests passed before in the timeframe and I think it's more likely that something is wrong. |
Ah, this is due to the precompute generation taking too long, set the timeout (only for beforeAll) to 30,000, and it should be solved. (Also could set precompute to 0) |
|
Precompute generation happens only once and it speeds up subsequent calculations, 8 is usually the sweet spot in production, but in testing, it slows it down |
|
Cool, ok, works https://github.com/ethereumjs/ethereumjs-monorepo/pull/4153/files, aliasing also seems ok, could fully replace all the wrapping we - also - still had internally with the new version, and tests pass! 🎉 That's some great progress, need to stop for today, will continute tomorrow a bit and add the cell methods to these simple API tests and also integrate with the tx library, and then we can hopefully release one or two days later! |
|
Hey there, I have now integrated the new Two small API things, I have started to address in #23:
Would you go along so far? I thought this would be accomplished by setting precompute to 0. However this does not change the initialization times more or less (still 15+ seconds on my machine). Does this rather point to something being wrong, or am I using precompute in a wrong way? What additionally irritates me that a) the JS library is acting instantly in base configuration and b) I thought that also this library was pretty quick on setup before the changes. Would be great if you can still address these points. Thanks! |
|
Switching the order of arguments makes sense. Yeah, the loading takes a while, I do remember it taking a while with the previous version when we were using it, I won't have the time to take an in depth look until later this week, however, here is a small preliminary. Here is the C function I added, I don't think that is the cause because it really doesn't do anything besides call the load_trusted_setup function providing the lengths of the arguments When running the setup in native C, it takes 2 seconds to load the trusted setup, so WASM setup would be expected to be 4-5 seconds. |
|
Ok, thanks for the additional look, I will then take things in with #23 and release that as the final version, also do not have the time to investigate further and I guess for now it's ok (also still possible to do further non-breaking improvements/releases down the line)! Thanks so far for all the contributions and the ongoing persistence in "making this round"! Very much appreciate that! 👍 💯 ❤️ 😄 |
| @@ -0,0 +1,18 @@ | |||
| import { fileURLToPath } from 'url'; | |||
There was a problem hiding this comment.
Ah, this is still a somewhat bigger one: so this usage of the Node.js url primitive breaks direct browser compatibility. I am not fully sure about the former setup, but I think we made the kzg.js to be able to directly import the WASM or something? Not fully sure, but we should change that. We did a lot to get all Node.js primitives out of our code base, since this makes it a lot easier to use stuff in the browser.
There was a problem hiding this comment.
Good point, I can look into this more as well, the reason I added the loaders was due to wasm path resolution issues, need to add something to make it browser compatible as well. When we were using, I think it was v0.4.0, in blobkit, I had to copy and paste the wasm file to wasm/kzg.wasm.
There was a problem hiding this comment.
Ok, I have added some easier way to test if the distribution build correctly (new npm scripts "test:src" and "test:dist" as well as some developer docs on the script/build specifics (AI supported) here #24. Hope that helps!
Regarding the path resolution issues there are some dedicated package.json scripts fixWasmDirInNode and fixWasmDirInWeb, so maybe these are related to this specific problem.
There was a problem hiding this comment.
Hey hey, do you think you could have a look into this (the url dependency thing) during the week? I know it's sometimes hard to stick for some "longer than expected time" on these kind of contributions on the side of the "normal work", but this would I think be really the last one and it would be really valuable to also have this in for the releases, then this is "round" on all sides. 🙂
There was a problem hiding this comment.
Browser support is a bit outside my expertise, but it looks like the default WASM binary loader works for the browser version, since those tests pass (The older versions always used the default loader).
There was a problem hiding this comment.
Ok, let's give it a try. Will release and run our browser tests.
There was a problem hiding this comment.
Ok, we'll see how it goes ethereumjs/ethereumjs-monorepo#4169, test-all-browser is the relevant CI run.
There was a problem hiding this comment.
Yeah, it rather costly... I think I found a solution to that issue, though. #26 whenever you get a chance.
It looks like having locateFile defined breaks the browser version; when I made it undefined, it worked. The version in that PR worked when I injected it and then ran the tests.





No description provided.