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.

Sign up for GitHub

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

Jump to bottom

Add micro-eth-signer/kzg #3674

Merged
merged 33 commits into from
Sep 21, 2024
Merged

Add micro-eth-signer/kzg #3674

merged 33 commits into from
Sep 21, 2024
+531 −400

Conversation

acolytec3
Copy link
Contributor

@acolytec3 acolytec3 commented Sep 16, 2024 •
edited
Loading

Takes an initial pass at integrating the kzg module from micro-eth-signer, putting it in tx for now.

I've added one of the trusted setups for now so we don't have to build/import trusted-setups

Also removes loadTrustedSetup from the kzg interface. We don't use it anywhere and I'm not convinced we will ever need to load another one.

@acolytec3
Copy link
Contributor Author

image

Here are some initial benchmarks for the various kzg functions we use. Refer to [root]/packages/tx/test/kzg.bench.ts to review.

You can run with npx vitest bench test/kzg.bench.ts from the tx directory

@acolytec3 acolytec3 marked this pull request as draft September 16, 2024 20:18
@acolytec3 acolytec3 mentioned this pull request Sep 16, 2024
Open
@paulmillr
Copy link
Member

The package is now live on NPM at @paulmillr/trusted-setups

@holgerd77
Copy link
Member

https://github.com/paulmillr/trusted-setups

@acolytec3 acolytec3 added package: common PR state: needs review dependencies Pull requests that update a dependency file labels Sep 19, 2024
@acolytec3 acolytec3 marked this pull request as ready for review September 19, 2024 15:36
@codecov Codecov
Copy link

codecov bot commented Sep 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 67.94872% with 25 lines in your changes missing coverage. Please review.

Project coverage is 16.01%. Comparing base (4470cc3) to head (25e4a40).
Report is 74 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
client 0.00% <0.00%> (ø)
tx 79.45% <92.85%> (+1.68%) ⬆️
util 75.33% <56.00%> (?)
vm 61.74% <ø> (?)
wallet ?

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

holgerd77
holgerd77 reviewed Sep 20, 2024
View reviewed changes
packages/tx/package.json Outdated
@@ -57,12 +57,14 @@
"@ethereumjs/common": "^4.4.0",
"@ethereumjs/rlp": "^5.0.2",
"@ethereumjs/util": "^9.1.0",
"ethereum-cryptography": "^3.0.0"
"ethereum-cryptography": "^3.0.0",
"trusted-setups": "github:paulmillr/trusted-setups"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have been forgotten. Will push a small update.

@holgerd77
Copy link
Member

Have updated the package-lock.json file (there was a conflict) and removed the forgotten depedency.

There are some client tests failing now I also planned to fix, but I am not fully seeing through, so there is some naming/interface thing happening in the tests (e.g. getRawBlock.spec.ts, also others) and I am not fully sure how to fix.

This was referenced Sep 20, 2024
Closed
Closed
@acolytec3 acolytec3 mentioned this pull request Sep 20, 2024
Closed
jochem-brouwer
jochem-brouwer reviewed Sep 20, 2024
View reviewed changes
Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some super small nits, LGTM otherwise

packages/client/bin/cli.ts Outdated Show resolved Hide resolved
packages/evm/src/opcodes/functions.ts
@@ -762,7 +762,7 @@ export const handlers: Map<number, OpHandler> = new Map([
function (runState) {
const index = runState.stack.pop()
if (runState.env.blobVersionedHashes.length > Number(index)) {
runState.stack.push(bytesToBigInt(runState.env.blobVersionedHashes[Number(index)]))
runState.stack.push(BigInt(runState.env.blobVersionedHashes[Number(index)]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will address this at some point, bit inside the EVM we should likely not put the strings in, but directly the bigints so we can push those to stack.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, hmm ok originally they were bytes which we kept converting to bigints, which is/was also suboptimal.

packages/evm/vite.config.bundler.ts Outdated Show resolved Hide resolved
packages/util/test/kzg.bench.ts Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 😄

jochem-brouwer
jochem-brouwer approved these changes Sep 21, 2024
View reviewed changes
Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@acolytec3 acolytec3 merged commit 9bdd5ac into master Sep 21, 2024
39 checks passed
@acolytec3 acolytec3 deleted the hot-swappable-kzg branch September 21, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holgerd77 holgerd77 holgerd77 left review comments

@jochem-brouwer jochem-brouwer jochem-brouwer approved these changes

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file package: block package: client package: common package: evm package: tx package: util package: vm PR state: needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@acolytec3 @paulmillr @holgerd77 @jochem-brouwer @gabrocheleau