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

NodeJS bindings #1

Closed
wants to merge 44 commits into from
Closed

NodeJS bindings #1

wants to merge 44 commits into from

Conversation

dgcoffman
Copy link
Owner

@dgcoffman dgcoffman commented Nov 1, 2022

This PR adds NodeJS bindings with the following interface:

  loadTrustedSetup: (filePath: string) => SetupHandle;

  freeTrustedSetup: (setupHandle: SetupHandle) => void;

  blobToKzgCommitment: (blob: Blob, setupHandle: SetupHandle) => KZGCommitment;

  computeAggregateKzgProof: (
    blobs: Blob[],
    setupHandle: SetupHandle
  ) => KZGProof;

  verifyAggregateKzgProof: (
    blobs: Blob[],
    expectedKzgCommitments: KZGCommitment[],
    kzgAggregatedProof: KZGProof,
    setupHandle: SetupHandle
  ) => boolean;

  // Not used in 4844(?), not exported and not covered with test automation. But I think it probably works.
  verifyKzgProof: (
    polynomialKzg: KZGCommitment,
    z: BLSFieldElement,
    y: BLSFieldElement,
    kzgProof: KZGProof,
    setupHandle: SetupHandle
  ) => boolean;

I also added blst as a submodule.

Most of diff is a yarn lockfile.

Test instructions: README

bindings/node.js/.gitignore Outdated Show resolved Hide resolved
bindings/node.js/.gitignore Outdated Show resolved Hide resolved
src/c_kzg_4844.c Outdated Show resolved Hide resolved
src/Makefile Outdated Show resolved Hide resolved
exports["FIELD_ELEMENTS_PER_BLOB"] = Napi::Number::New(env, FIELD_ELEMENTS_PER_BLOB);
exports["BYTES_PER_FIELD"] = Napi::Number::New(env, BYTES_PER_FIELD);

return exports;
Copy link
Owner Author

@dgcoffman dgcoffman Nov 3, 2022

Choose a reason for hiding this comment

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

This is the interface of the JavaScript module which can be imported by a NodeJS program. node-gyp turns this into kzg.node.

bindings/node.js/kzg.ts Outdated Show resolved Hide resolved
bindings/node.js/test.ts Outdated Show resolved Hide resolved
src/c_kzg_4844.c Outdated Show resolved Hide resolved
bindings/node.js/kzg.cxx Outdated Show resolved Hide resolved
src/Makefile Show resolved Hide resolved
src/c_kzg_4844.h Outdated Show resolved Hide resolved
bindings/node.js/binding.gyp Show resolved Hide resolved
Napi::ArrayBuffer buffer = Napi::ArrayBuffer::New(
env,
vector->data(),
length /* size in bytes */,
Copy link

Choose a reason for hiding this comment

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

might not matter, but should this be set to the vector capacity rather than length? I dunno of any stdlib that uses a different capacity at construction tho.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I see what you mean and I think it should be the byte length supplied to the function, not the allocation for the vector, even in the case where they differ

Comment on lines 213 to 214
auto polynomial = (Polynomial*)calloc(blobs_count, sizeof(Polynomial));
auto commitments = (KZGCommitment*)calloc(blobs_count, sizeof(KZGCommitment));
Copy link

Choose a reason for hiding this comment

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

might as well wrap these in a vector to avoid manually (and possibly forgetting to) freeing the allocation.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I tried for an hour and I don't think it's possible to make a std::vector<Blob>. https://stackoverflow.com/questions/16970624/cant-make-a-vector-of-fixed-size-arrays

Do you know how to do it? I think I want to just manage the memory manually.

Copy link
Owner Author

Choose a reason for hiding this comment

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

The error is

error: object expression of non-scalar type 'unsigned char[131072]' cannot be used in a pseudo-destructor expression

Copy link

Choose a reason for hiding this comment

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

Oh interesting that Blob is typedef'd directly as a fixed-array. Yeah that won't work.
@xrchz perhaps we could redefine these types as structs like how blst does it here. This makes it more obvious that the types are arrays and lets the C++ compiler know that they're trivially copyable.

Copy link

Choose a reason for hiding this comment

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

Sure I can do it that way instead!

Copy link

Choose a reason for hiding this comment

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

Actually I don't really understand how to use those structs. Do you?

Copy link

Choose a reason for hiding this comment

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

I tried a version of this on a new branch, struct_blob, ptal.

@dgcoffman dgcoffman requested a review from xrchz November 4, 2022 20:35
@dgcoffman
Copy link
Owner Author

Moving to dankrad#3

@dgcoffman dgcoffman closed this Nov 4, 2022
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