Epic for Bindings Rebuild

[matthewkeil]

Rebuilding blst-ts

Adding this issue as a tracker for rebuilding the bindings using N-API. There is not much code that is shared between the new and old versions so the ease the review process I am going to break stuff up into pieces. I am thinking about using mkeil/rebuild as the trunk branch and will merge all the pieces below into it. I think once everything is merged there may be a a PR to organize a bit and remove the stuff related to swig before merging mkeil/rebuild to master.

Review/Merge Hierarchy

Most of these PR's cascade. I was attempting to do the base Classes and then keep all the functions independent but there were a few changes in verifyMultipleAggregateSignatures that should cascade to avoid difficult merge conflicts for the verify suite. Fixed the branch in PR#97 and merged the aggregates into the verify branch to test fastAggregateVerify. Any remaining PR's will follow from #97 to avoid similar confusion.

89 -> #90 -> #91 -> #94 -> this is where there is a branch.

branch 1) #94 -> #92 -> #93 branch 2) #94 -> #95 -> #96

branches get merged together in) #97

97 -> #98 -> #99 -> #100->#101

All unit tests and all specs are passing. The bulk of the code is substantially complete. Just the PR review changes and ancillary code (CI/CD etc) needs to be completed.

The implementation for this code in Lodestar can be found here:

• https://github.com/ChainSafe/lodestar/pull/5493

Reviewer Notes

There is a lot of code in here and the first few pieces are mostly boilerplate to make the rest work. It may be easier to take a peek at #92 to see how everything comes together and gets implemented. Then peek at #91 and #94 to get an overview of the pki classes. Then circle back and start reviewing #90.

To view the entire lot at once, mkeil/fix-agg-spec has all of the pieces and is 100% passing specs and unit tests.

Base

• [X] Folder structure and config files

  • 89

Classes

• [ ] Rebuild Addon, BlstBase, BlstAsyncWorker, Uint8ArrayArg, Uint8ArrayArgArray

  • 90

• [ ] Rebuild SecretKey, PublicKey and Signature

  • 91

• [ ] Add PublicKeyArg, PublicKeyArgArray, SignatureArg and SignatureArgArray

  • 94

• [ ] Spec test for SecretKey.sign

  • 99

aggregatePublicKey and aggregateSignatures

• [ ] Rebuild aggregation functions with unit tests

  • 92

• [ ] Aggregation Spec tests

  • 93

• [ ] Pass x40 spec test

  • 100

verifyMultipleAggregateSignatures

• [ ] Rebuild verifyMultipleAggregateSignatures function with unit tests

  • 95

verify and aggregateVerify

• [ ] Rebuild function with unit and spec tests

  • 96

fastAggregateVerify

• [ ] fastAggregateVerify unit and Spec tests (also covers merge of all function PR's)

  • 98

Cross-Compatability

• [ ] Debug for linux builds

  • 101

• [ ] Debug for windows builds

Things to double check

• [ ] HandleScope is applied correctly everywhere
• [x] Passing by const & everywhere applicable

Wishlist

• [ ] Add validate param to Signature.deserialize
• [ ] Add validate param to PublicKey.deserialize
• [ ] If single key or sig is passed to aggregate just pass through function and don't aggregate

Testing

• [ ] Add valgrind leak check test
• [X] Add spec tests
• [x] Add benchmark tests
• [x] Performance comparison tests to old bindings

DevOps

• [x] Update workflows for push/merge

  • 101

• [x] Create workflow to node-pre-gyp or prebuild for popular platforms
• [x] Update install script to reference pre-built bindings

Integration/E2E Testing

• [x] Publish alpha set of updated bindings (or create Dockerfile using updated code)
• [ ] Determine telemetry that should be collected and compared for runtime testing
• [ ] Flamegraph for CPU tuning
• [ ] Heapdumps for memory tuning

Audit

• [ ] Assess if external review/audit is prudent

Documentation

• [x] Add docs before PR process for team
  • https://github.com/ChainSafe/blst-ts/pull/86
• [x] Update README.md
• [ ] Update CONTRIBUTING.md
• [ ] PR/Issue Node for HandleScope
  • https://github.com/nodejs/node/issues/47504

[matthewkeil]

@philknows @dapplion I have a question about the process above. There is little overlap from old to new code now that everything is hand built instead of SWIG. I am intending to put the new code in with the old code to test for regression and for performance validation. We have some of that stuff already on the POC branches but figured it should be formalized during the merge.

There is one DX question as both new and old code cannot sit at the root of the repo together and still build correctly. The old bindings do not have a binding.gyp at the root of the repo and the new ones require it. Having it there breaks the old bindings build and causes issues with the new build.

The rebuild-bindings branch did a wholesale rebuild, to avoid this, and did not preserve the original code. I do not think that was a good decision in retrospect. I am in process of moving all the code using this epic to track the process, and any remaining bits that come up during the process. I started writing the first PR is what sparked this question.

I can either put all the new code in a folder like new-bindings so there will be no git interaction with master after merging. Then build everything inside of there as it will be in the root after the epic merges. That way all of the original code will work as expected and the new code will too. Once merged there will be a PR or two to remove the old code and move the new code to its final resting place. Then we can update the CI/CD workflows.

The other option is to move all the old code into a deprecated folder and update the legacy scripts and whatnot to work from the new home. Then I will copy the new code in to its final resting place. After the new bindings get merged to main we can add a finalization PR that removes the deprecated code. This seems like more work I think so I already started working on the one above but will be easy enough to switch gears early

[matthewkeil]

@wemeetagain @g11tech @nazarhussain @tuyennhv @nflaig was curious if you guys had any thoughts about the process

[wemeetagain]

i think this can be closed since 1.0 has been released :)