Remove nodejs import from trie.ts

[scorbajio]

This change fixes an import error that is coming up in webpack browser builds resulting from a nodejs-specific import being made in the trie package as a result of the readable-stream refactor done in #3231. See #3231 (comment).

[codecov]

Codecov Report

Attention: 37 lines in your changes are missing coverage. Please review.

Comparison is base (c62cce0) 86.85% compared to head (abff500) 86.85%.

Additional details and impacted files

Flag	Coverage Δ
block	88.33% <ø> (ø)
blockchain	91.61% <ø> (ø)
client	84.69% <ø> (ø)
common	98.25% <ø> (ø)
devp2p	82.12% <ø> (ø)
ethash	∅ <ø> (∅)
evm	74.33% <ø> (ø)
genesis	99.98% <ø> (ø)
rlp	∅ <ø> (∅)
statemanager	75.70% <24.48%> (-0.16%)	⬇️
trie	89.53% <100.00%> (+0.32%)	⬆️
tx	95.45% <ø> (ø)
util	89.13% <ø> (ø)
vm	80.20% <ø> (ø)
wallet	88.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[yann300]

fyi I posted a message in the previous PR: https://github.com/ethereumjs/ethereumjs-monorepo/pull/3231/files#r1487361314

[holgerd77]

@yann300 @scorbajio Ok, I'll put this back in a "WIP" state unless we have that solved. So if the main file (non type) import is also a problem, then we eventually have to roll back the whole feature (?).

TBH this completely let's my head explode, that a web standard implementation in Node.js is then breaking web compatibility. 🤯 Is there really no way around that? I wrote at some other occasion when we implemented I guess that normally all browsers would need to do is to just ignore this specific import (since things are just meant to work any in the browser without this Node.js import).

Hmm. Hmm. Hmm.

[holgerd77]

(I tried but I was personally not able to dig something out by googling on this)

[scorbajio]

@yann300 could you provide steps to reproduce the build error so we can look into possible solutions?

[holgerd77]

@yann300 could you provide steps to reproduce the build error so we can look into possible solutions?

@scorbajio Just an additional note: I am sure this can be solved on the build side, for us this is just not an option, we would cripple our browser compatibility with that.

@yann300 maybe wait with going too deep here, we might just do a follow-up release removing the new functionality again.

[yann300]

I had a look at node:stream/web and ReadableStream, that's indeed somehow weird and annoying...
You might find interesting to look at https://github.com/nodejs/readable-stream ... but I understand adding a new dep can be frustrating even if it comes from a legit org ..

[yann300]

@yann300 could you provide steps to reproduce the build error so we can look into possible solutions?

you could clone https://github.com/ethereum/remix-project and yarn && yarn build

EDIT: switch to the branch cancun before running yarn.

[holgerd77]

LGTM, thanks for adressing so quickly!

Bye bye web streams API, it was nice with you. 😑

Have added a note to #3216 so that we do not forget about the code base and might be able to re-integrate along next breaking releases.

Will still wait a couple of days to see if more stuff drops in but otherwise we would do a fixing release round. Maybe next Thursday or so.

[yann300]

@holgerd77 @scorbajio thanks for the PR! do you have guys a planned date for getting a release which include this PR?

[holgerd77]

@yann300 Hi there, yes, we plan to do releases early next week, want to bundle this with some other stuff (WASM kzg)!

[roninjin10]

Just ran into this issue after upgrading ethereumjs libraries in tevm. Thanks for the fix!

[roninjin10]

Let me know if you would like me to add a lint rule that checks for this issue

[holgerd77]

Just ran into this issue after upgrading ethereumjs libraries in tevm. Thanks for the fix!

Thanks for reporting, good to have this confirmed! 🙏 🙂

Releases on this (and some other stuff) are planned for next Tuesday (since relatively urgent).

Ah, there is also the KZG -> WASM work in there (that will actually be the main feature, so from #3294), so @acolytec3 has done a WASM port of the c-kzg library and all 4844 things (so mainly blob txs and the point evaluation precompile in the EVM)). That will likely also be relevant for Tevm? That would also be valuable if you can give feedback on the releases from this angle! 😄

@yann300 so guess for you guys this is relevant as well (?)

[holgerd77]

Let me know if you would like me to add a lint rule that checks for this issue

Yeah, that might be helpful! We are generally aware of this though, this one was a somewhat special case, mental blocker on my side, I couldn't really imagine that using a WEB streams API would break browser compatibilty. 😂

[holgerd77]

@yann300 @roninjin10 Another quick question, since I "already have you here" 🙂

We are a bit drawn back and forth how to handle KZG in EVM and VM.

Basically the thing is: post Cancun, 4844 related functionality becomes an integral part of the EVM/VM with the addition of the new 4844 point evaluation precompile. So that means that the KZG setup is basically necessary for all EVM/VM runs.

This means that we now have a somewhat solid amount of boilerplate for EVM/VM initialization:

import { createKZG } from 'kzg-wasm'
import { Common, Chain, Hardfork } from '@ethereumjs/common'
import { EVM } from '@ethereumjs/evm'
import { initKZG } from '@ethereumjs/util'

const main = async () => {
  const kzg = await createKZG()
  initKZG(kzg)
  const common = new Common({
    chain: Chain.Mainnet,
    hardfork: Hardfork.Cancun,
    customCrypto: { kzg: kzg },
  })
  const evm = new EVM({ common })
 }

We could (likely) avoid this by bundling the kzg-wasm library with the EVM/VM. For lower level libraries (like tx) we have users who have a strict no-WASM policy where we would therefore prevent this usage if we do bundle in WASM dependencies.

For higher level libraries like EVM/VM we are not so sure.

What are you guys personal opinions (respectively team based policies) on that? Do you have some problem with bundled in WASM? As a side note it might be worth to mention that the WASM file we have here is somewhat large (~500KB). So this also has the effect that this definitely adds additional boilerplate to the EVM bundle. This is a general problem though, also if the manual self-installation option is used.

Some feedback would be valuable! 🙏

[holgerd77]

Side note: we might be lucky and get some JS KZG library written in the next 1-3 months, so this might be generally a temporary situation with the boilerplate code (and this might be an argument to for now skip the deep WASM integration).

[roninjin10]

@holgerd77

all EVM/VM runs

Really? Even EVM runs that don't do any blob related things need it? Why is that?

Wasm bundle size

the WASM file we have here is somewhat large (~500KB)

Oh wow this is highly problematic. I'm definitely gonna need to have to find a workaround for dealing with that. What makes it so big and is there any potential to make it smaller?

Definitely would prefer not bundling it in on this bundle size thing alone as it would hurt my ability to work around the large bundle hit

Initialization code

  const kzg = await createKZG()
  initKZG(kzg)
  const common = new Common({
    chain: Chain.Mainnet,
    hardfork: Hardfork.Cancun,
    customCrypto: { kzg: kzg },
  })
  const evm = new EVM({ common })

I think it's fine.

From end user of tevm point of view all this gets abstracted into a client instance that has a ready method that resolves when initialization is done.

From my point of view maintaining tevm, I actually currently am following a convention of always explicitly passing in every single parameter into VM, EVM, Blockchain, etc. E.g. below is how I initialize a VM. It's already pretty verbose intentionally

const stateManager = createTevmStateManager(getStateManagerOpts())
const common = new TevmCommon({
		chain: 1,
		hardfork: options.hardfork ?? 'shanghai',
		eips: /**@type number[]*/ (options.eips ?? [1559, 4895]),
	})
		const blockchain = await createBlockchain({ common })
		const evm = createEvm({
			common,
			stateManager,
			blockchain,
			allowUnlimitedContractSize: options.allowUnlimitedContractSize ?? false,
			customPrecompiles: options.customPrecompiles ?? [],
			profiler: options.profiler ?? false,
		})
		const vm = await createVm({
			stateManager,
			evm,
			blockchain,
			common,
		})

[acolytec3]

You should be able to run the EVM just fine, but if you haven't instantiated a KZG instance and try to execute the point evaluation precompile, you'll get an error about kzg not loaded. Otherwise, shouldn't be an issue.

In terms of the size, I think it mainly comes down to the size of the trusted setup. I'm not an expert on compiling C to WASM, but I turned on all the optimizations that emscripten advised and it still was in the 500k range. If I had to guess, even if/when we have a pure TS/JS version of the KZG library, you're still going to have a big bundle size because there's no way around loading that trusted setup if you want to do KZG crypto operations (required for anything related to 4844).

[roninjin10]

You should be able to run the EVM just fine, but if you haven't instantiated a KZG instance and try to execute the point evaluation precompile, you'll get an error about kzg not loaded. Otherwise, shouldn't be an issue.

This is very nice. This means implementing some kind of lazy loading might be an option. Putting the KZG implementation on a backend server instead of browser bundle might also be an option. Will dig into it more next week.

In terms of the size, I think it mainly comes down to the size of the trusted setup. I'm not an expert on compiling C to WASM, but I turned on all the optimizations that emscripten advised and it still was in the 500k range. If I had to guess, even if/when we have a pure TS/JS version of the KZG library, you're still going to have a big bundle size because there's no way around loading that trusted setup if you want to do KZG crypto operations (required for anything related to 4844).

This makes sense. I was lowkey hoping the wasm was compiled from golang in which case there would be a lot of room for improvement.

Looking at the kzg-wasm code I'm happy to see it's initialized asyncronously. I opened this issue in the rustbn repo that is causing issues from awaiting in the top level of the module.

[roninjin10]

Looks like the kzg committment file alone is about 437kb so definitely not much headroom to make it better https://github.com/ethereum/go-ethereum/blob/00905f7dc406cfb67f64cd74113777044fb886d8/crypto/kzg4844/trusted_setup.json

[roninjin10]

With 7zip I was able to compress the 437kb file down to 210kb

7z a -mx=9 compressed-file.7z kzg.json

7-Zip [64] 17.05 : Copyright (c) 1999-2021 Igor Pavlov : 2017-08-28
p7zip Version 17.05 (locale=utf8,Utf16=on,HugeFiles=on,64 bits,10 CPUs LE)

Scanning the drive:
1 file, 447355 bytes (437 KiB)

Creating archive: compressed-file.7z

Items to compress: 1


Files read from disk: 1
Archive size: 214362 bytes (210 KiB)
Everything is Ok

[roninjin10]

btw @acolytec3 @holgerd77 is there a telegram chat or a similar channel for Ethereumjs chat? Feel free to dm me if there is but you prefer to keep it private: https://twitter.com/FUCORY?ref_src=twsrc%5Egoogle%7Ctwcamp%5Eserp%7Ctwgr%5Eauthor

[acolytec3]

You're welcome to jump on our discord for more real-time discussion - https://discord.gg/JCsNTaVw

[holgerd77]

Ok, so we have settled to not bundle WASM KZG in as a direct dependency.