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.

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

feat: bundle and auto-load trusted setup #422

Merged
merged 12 commits into from
Apr 30, 2024

Conversation

matthewkeil
Copy link
Member

@matthewkeil matthewkeil commented Apr 30, 2024

Motivation

Allows for easier initialization of library for Ethereum use cases. Also bundles the trusted setup so that it does not need to be manually downloaded by consumers and stored separately. Will simplify the lodestar use case. Defaults to using the file passed by the user first. If none is passed use the one found in the dist bundle (checks here for the prod case first). If one is not found in the prod location fall back to the src location (for development purposes). Still throws error if none is sourced (invalid bundle/build).

Description of Changes

  • Bundle trusted_setup.txt with the c-kzg deps for node.js bindings
  • Update kzg.js to functions to allow for passing a trusted setup or for loading the default setup from either the bundle or the repo src
  • Export getTrustedSetupFilepath for testing purposes but do not announce via types file.
  • Unit test getTrustedSetupFilepath to ensure proper loading.

bindings/node.js/lib/kzg.js Outdated Show resolved Hide resolved
Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me. I like that users can still specify a trusted setup.

bindings/node.js/README.md Outdated Show resolved Hide resolved
bindings/node.js/lib/kzg.js Outdated Show resolved Hide resolved
bindings/node.js/lib/kzg.js Outdated Show resolved Hide resolved
@jtraglia
Copy link
Member

@matthewkeil what does "enabling exceptions" do exactly and what are the implications?

@matthewkeil
Copy link
Member Author

matthewkeil commented Apr 30, 2024

@matthewkeil what does "enabling exceptions" do exactly and what are the implications?

@jtraglia a JavaScript Value is a Reference that contains an Address to a value (little "v" actual data) that is stored either on the stack or in the heap. A large part of what the Isolate (instance of a JS virtual machine) does is manage memory and garbage collection of the values (little "v", think data, and big "V" think reference to the Address that points to the data).

These two layers of indirection allow for the data to be tracked/moved/managed/deleted etc by the runtime.

When a v8::Value is requested by a function call the Isolate does a lookup and dereferences the value (little "v") and then returns a Reference to for the value (little v) as a v8::Value (big V). It does so for ref counting which informs the garbage collection system of when a value (both big and little v) can be deleted.

The sticky wicket comes in that if exceptions are turned on, requesting a v8::Value will throw an error if the value (little v) does not exist. With exceptions off it essentially returns a std::optional which in JS world is called a v8::Maybe. The extra steps required without exceptions is a Maybe needs to be checked if it has a value (little v) associated or if it is Empty which is sort of like a nullptr. If an attempt is made to use an Empty value there will be undefined behavior or worse a segfault.

Generally this does not happen because there are several checks that occur in the workflow under the hood. However the one that is most critical is if memory is not allocated correctly for the value (little v) the system will think it is a valid v8::Value but it will be Empty which is.... not... ideal...

There are other considerations as well with regards to errors in JS propagating back into native code, however our codebase does not enter this territory so I will not elaborate.

Turning on exceptions is the preferred way of handling this and is something the docs advocate for. This was something that slipped by me when we worked on this before and I only caught it recently. If we do not want to turn on exceptions there will need to be code added to check and unwrap the Maybe values to turn them in to proper Values.

It will not affect the c-kzg code, its strictly to protect from v8 engine issues.

Here is the doc about exception handling:
https://github.com/nodejs/node-addon-api/blob/main/doc/error_handling.md

And the subsection about code compliance with exceptions turned off:
https://github.com/nodejs/node-addon-api/blob/main/doc/error_handling.md#handling-errors-with-maybe-type-and-c-exceptions-disabled

@jtraglia
Copy link
Member

@matthewkeil thanks for the explanation. I think this is a bit over my head. Could you split the exception changes into a separate PR and ask a teammate (or just someone that would know) to review it?

@matthewkeil
Copy link
Member Author

@jtraglia exceptions moved to #423

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for making those changes!

@jtraglia jtraglia merged commit 8ab57f4 into ethereum:main Apr 30, 2024
37 checks passed
@matthewkeil matthewkeil deleted the mkeil/bundle-trusted-setup branch May 7, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants