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

[Build] Add chiabls subtree #2419

Merged

Conversation

Fuzzbawls
Copy link
Collaborator

This introduces a new upcoming library dependency as a git subtree with support for native autotools (and CMake) builds.

#2405 had also introduced this library dependency, but only as part of the depends system, which introduced numerous issues, not least of which was essentially breaking any non-depends based builds. After some discussion, it was decided that the git subtree approach (as used with leveldb, crc32, univalue, and secp256k1) would be more ideal as it would allow for both depends and non-depends based builds.

Achieving this requires us to maintain our own fork of https://github.com/Chia-Network/bls-signatures (ours is at https://github.com/PIVX-Project/bls-signatures) with a native autotools based build system added to it (PIVX-Project/bls-signatures#1) that has been integrated into PIVX Core here.

Note: This PR simply introduces the new library and connects it to our autotools and CMake build systems, but does not introduce new code that actually uses the new library.

Also Note: This effectively bumps our minimum required version of CMake (for CMake builds only) to 3.14.

@random-zebra
Copy link

Huge concept ACK here.
Tested both gitian and local (non-deterministic) build.
Awesome job 🥃

Just one minor nit: let's not track relic_conf.h.in.
I added a temporary commit for this in 52b35cc, but it would be better to directly push the change the library, and update the subtree here.

@Fuzzbawls
Copy link
Collaborator Author

regarding the relic_conf.h.in file: it would go against the git subtree rules to exclude it's changes from being tracked here, or even in our forked bls-signatures repository, as doing so would add a conflict whenever we wanted/needed to update the relic version.

I wish git had a way to "un-track"/ignore an already tracked file, but the only solutions i've found only apply locally and cannot be applied as a general rule by default to anyone who clones the repo.

@Fuzzbawls Fuzzbawls force-pushed the 2021_build-chiabls-subtree branch 2 times, most recently from 1109a6c to 73d32aa Compare June 24, 2021 10:22
@Fuzzbawls
Copy link
Collaborator Author

Rebased to resolve conflicts after #2338 being merged.

This would be a good candidate to prioritize for merging before other conflicting PRs now, as rebasing subtree additions is not an automated process, and any dependent PRs (#2420 and #2421) will also need to do a more complicated rebase process until this is merged

@Fuzzbawls
Copy link
Collaborator Author

Also, for informational purposes, the relic_conf.h.in file can be forcibly ignored locally by issuing the following command:

git update-index --assume-unchanged src/chiabls/contrib/relic/include/relic_conf.h.in

This will ensure that any changes to the file are not considered when doing things like git add -A, and it will not show up in the list of modified files when doing git status or git diff.

I think this is an acceptable workaround that preserves git subtree rules and doesn't add an un-necessary layer of complication (being that the only other "proper" way of achieving this whilst having clean subtrees, would to also maintain our own fork of the Relic repository, which just seems overkill for such a minor issue). If need be, we can add the above command to the developer documentation.

random-zebra
random-zebra previously approved these changes Jun 28, 2021
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK 73d32aa

@random-zebra random-zebra requested a review from furszy June 29, 2021 09:03
@furszy
Copy link

furszy commented Jun 30, 2021

Concept ACK, starting here and with all of DIP6 lecture, going to start with PIVX-Project/bls-signatures#1 and will back to this one. Then keep reading and reading.. lot of stuff.

@furszy
Copy link

furszy commented Jul 7, 2021

I'm still moving forward through the DIP6 lecture (which is quite nice) and the PRs that were created in the https://github.com/PIVX-Project/bls-signatures repository (will review them all before giving my final approval here so we are fully sync).

Dropping few comments from the initial review:

  • Compilation wise: all good. make and cmake, both completed properly ✅.
  • The subtree hash matches correctly ✅.
  • The BLS signatures sources aren't going to be affected by our custom libsodium requirement [WIP][Build] Libsodium upgrade & sanity checks #2358.
    The repository only uses libsodium for secure memory allocation (and can even be disabled). There is no problem with our consensus requirements for the latest libsodium Ed25519 pubkey and signature validation ✅.
  • Need to add the new subtree into the into the developer-notes, "Subtrees" section.

Side from that, first glance, looking good. As said above, will finish the bls repo PRs review first and come back here, nice job ☕ .

@furszy
Copy link

furszy commented Jul 18, 2021

just finished the review of the bls-sigs repo.

Worked some time there and found few things on the way:

Better to merge them, and update this subtree with them. So the new Aggregated SSS scheme and DKG protocol test case can be run here as well and we don't utilize, by mistake, the operator with the infinite recursion.

random-zebra added a commit that referenced this pull request Jul 21, 2021
855aa70 Backport: git-subtree-check.sh updated to latest upstream (furszy)

Pull request description:

  Pulled latest `git-subtree-check.sh` changes from upstream. So it's easier to verify the new subtree that is being included in #2419.

ACKs for top commit:
  random-zebra:
    ACK 855aa70 and merging...

Tree-SHA512: 0e5c963b5899f42c729495b2c8fbd9cae6c9444a0fc726c2091240edac0c2e48dd01e814cb21b3c809409158d35aeec34d42766ae65ea2a62484ac61ca4652cd
@random-zebra
Copy link

I think that this can be rebased on master, and the subtree updated to PIVX-Project/bls-signatures@676ea45, so we can merge it and move forward.

git-subtree-dir: src/chiabls
git-subtree-split: 676ea45e8034f59ac49a2b53e09880bbcc0767e5
Plug in the native autotools based build system to provide the
libchiabls.la libtool library.
This is a subtree import, linters should ignore any violations
@Fuzzbawls
Copy link
Collaborator Author

rebased on master and the subtree commit was updated. lets get this merged sooner rather than later as subtree updates are much more straight forward and simple than subtree adds (ie, if we need to update the subtree commit later then it is a simple process compared to rebasing a subtree add).

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Tested local deployment once more, ACK c775ab3

@furszy furszy requested a review from random-zebra July 28, 2021 02:41
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

utACK 5b289eb and merging...

@random-zebra random-zebra merged commit 1e4fa87 into PIVX-Project:master Jul 28, 2021
random-zebra added a commit that referenced this pull request Sep 17, 2021
… and unit tests

0d126ee [QA] Test encryption/decryption of sk shares in dkg unit test (random-zebra)
db69885 [QA] Refactor bls_ies_tests using CBLSIESEncryptedObject (random-zebra)
0453474 Test: Add bls ies basic encryption and decryption test coverage. (furszy)
aa51751 [Refactoring] Replace std::random_shuffle with Shuffle (random-zebra)
9f4ac39 [QA] Add unit test for DKG using wrapper/worker (random-zebra)
0253ea7 [QA] Add test for BLS sign/verify message and sethexstr (random-zebra)
c4c6efe [Refactor] Initialize BLS allocator in BasicTestingSetup (random-zebra)
104c4b0 Bail out early from secure deallocation (Alexander Block)
f5c0870 Add ECDSA benchmarks (Alexander Block)
5b16d89 [BUG] Initialize Random for bench tests (random-zebra)
1c3f2cb [Trivial] Remove unused variables in bls bench test (random-zebra)
9502597 Add BLS and DKG benchmarks (Alexander Block)
7bb9f02 [Trivial] bls_worker: Fix members initialization order in Aggregators (random-zebra)
225c8a8 Add highly parallelized worker/helper for BLS/DKG calculations (Alexander Block)
df56dea Add simple helpers/wrappers for BLS+AES based integrated encryption schemes (IES) (Alexander Block)
6f1d0e2 Implement wrappers around Chia BLS libs (random-zebra)
a0b04c1 Add pooled_secure_allocator and mt_pooled_secure_allocator (Alexander Block)
dbb0305 [Trivial] Rename BCLog::BENCH/RAND to BCLog::BENCHMARK/RANDOM (random-zebra)
d9ffaa6 Add helper to rename all threads of a ctpl::thread_pool (Alexander Block)
9f9a3d7 Add ctpl header only library (random-zebra)

Pull request description:

  Based on top of:
  - [x] #2419

  This reworks #2405, adapting the commits to the new build system (which includes the library already at `v1.0.1`, as git subtree).

ACKs for top commit:
  furszy:
    one step closer, ACK 0d126ee
  Fuzzbawls:
    ACK 0d126ee

Tree-SHA512: 9a07bf32790326444c7e035bd9b24351715d3906fb6e2b9e51b46950ed1aa82cf2cb72fba1b13611d771c339936433ff4c5a914e1ca2bb03965b83c371c8817f
furszy added a commit that referenced this pull request Oct 8, 2021
d3843cd [Refactor] No need to get the publickey of the active masternode (random-zebra)
483f509 [Refactor] De-duplicate operator BLS key parsing in rpc (random-zebra)
855c094 [Build] Fix CMake builds with chiabls/relic includes (random-zebra)
77ec208 [Tests] Fix TierTwo functional tests with BLS operator key (random-zebra)
dd46f72 [TierTwo] Migration of DMN operator key to BLS (random-zebra)
1dfdcc1 [RPC] Implement generateblskeypair() RPC command (random-zebra)
619c0f8 scripted-diff: Rename keyIDOperator to pubKeyOperator (random-zebra)
b0a7fa5 [QA] Add tests for fbv messages signed with BLS keys (random-zebra)
dd0fb01 [TierTwo] Add functions to sign/verify messages with BLS (random-zebra)

Pull request description:

  This builds on top of:

  - [x] #2363
  - [x] #2419
  - [x] #2420

  As per title, introduce BLS keys/signatures for messages signed by the masternode operator (which is a requirement for [DIP 6](https://github.com/dashpay/dips/blob/master/dip-0006.md), and subsequent upgrades).

  - a new RPC command `generateblskeypair` returns a new publickey/secretkey BLS keypair.
  - `CDeterministicMNState::keyIDOperator` is now a `CBLSLazyPublicKey` (instead of a `CKeyID`), and it's renamed `pubKeyOperator`.
  - `-mnoperatorprivatekey` flag takes a BLS secret key in hex format.
  - `CActiveMasternodeInfo` stores a `CBLSPublicKey`/`CBLSSecretKey`
  - the BLS signature byte vector is serialized for legacy messages (mnw, fbv) in the compatibility code.

ACKs for top commit:
  furszy:
    great, ACK d3843cd
  Fuzzbawls:
    ACK d3843cd

Tree-SHA512: 1caf49615283d10b356caa049b57091dc70805c0bf2c11645d560c81223ce0d9936a46096cc4871c5ec25164518ebdf259d0f23a8c5bca39f2e436cc9e954519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants