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

Use bls from chia rs #16715

Merged
merged 2 commits into from
Nov 7, 2023
Merged

Use bls from chia rs #16715

merged 2 commits into from
Nov 7, 2023

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Oct 27, 2023

This PR is best reviewed one commit at a time. There are two commits, the first one is the main change. The second one removes a hack to add type-stubs to blspy, which is no longer needed.

Purpose:

The purpose of moving the the BLS bindings from blspy (C++) to chia_rs (rust) is to move towards being able to perform all block validation in rust. The main benefit of this is to be able to use threads instead of a process pool for parallel block validation.

There's also a good chance the BLS cache could be a lot faster by being ported to rust.

Current Behavior:

We currently use BLST for BLS operations, wrapped with some additional features in C++ (bls-signatures) then given a python binding in blspy. This makes it very hard (near impossible) to reach the underlying BLST primitives from rust. And similarly impossible to build rust-native types with BLS elements as members.

New Behavior:

We still use BLST but the additional features have been ported to rust (in chia_rs) and given a python binding along with all other chia_rs functions and types, in the chia_rs wheel. This allows rust-native types to have native BLS members, and still be compatible with the python bindings.

For example, this enables signature validation to happen in rust (in threads rather than processes) and it allows the BLS cache to be implemented in rust.

One noteworthy discrepancy between blspy and chia_rs is that the PrivateKey data is not allocated in "secure memory" yet, in chia_rs. i.e. mlocked and zeroed out.

Testing Notes:

How do we know that the BLS types in chia_rs behave the same as blspy?
There's a fuzzer that calls into blspy to compare the results from chia-bls here:
https://github.com/Chia-Network/chia_rs/blob/main/chia-bls/fuzz/fuzz_targets/blspy-fidelity.rs

There's a fidelity test exercising the python bindings of both chia_rs and blspy here:
https://github.com/Chia-Network/chia_rs/blob/main/tests/test_blspy_fidelity.py

@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Oct 28, 2023
@arvidn arvidn force-pushed the use-bls-from-chia_rs branch 2 times, most recently from f0dcd4e to 0011ce6 Compare October 30, 2023 20:04
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Oct 30, 2023
@github-actions

This comment was marked as outdated.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Oct 30, 2023
@github-actions

This comment was marked as outdated.

@coveralls-official
Copy link

Pull Request Test Coverage Report for Build 6698187916

  • 137 of 140 (97.86%) changed or added relevant lines in 122 files are covered.
  • 29 unchanged lines in 9 files lost coverage.
  • Overall coverage increased (+0.07%) to 90.369%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/plotting/check_plots.py 0 1 0.0%
chia/wallet/puzzles/prefarm/spend_prefarm.py 0 1 0.0%
tests/util/benchmark_cost.py 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
tests/simulation/test_simulation.py 1 96.53%
tests/wallet/test_wallet_retry.py 1 97.18%
chia/timelord/timelord.py 2 80.36%
chia/full_node/full_node.py 3 85.93%
chia/util/streamable.py 3 98.55%
chia/wallet/wallet_node.py 3 87.35%
tests/core/util/test_streamable.py 4 99.28%
chia/full_node/full_node_api.py 5 78.02%
chia/daemon/server.py 7 86.81%
Totals Coverage Status
Change from base Build 6698129677: 0.07%
Covered Lines: 93287
Relevant Lines: 103190

💛 - Coveralls

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Nov 1, 2023

This comment was marked as outdated.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Nov 4, 2023

This comment was marked as outdated.

@arvidn arvidn marked this pull request as ready for review November 4, 2023 09:51
@arvidn arvidn requested review from altendky and a team as code owners November 4, 2023 09:51
@arvidn arvidn requested a review from emlowe November 4, 2023 11:38
Copy link
Contributor

github-actions bot commented Nov 5, 2023

File Coverage Missing Lines
chia/plotting/check_plots.py 0.0% lines 12
chia/wallet/puzzles/prefarm/spend_prefarm.py 0.0% lines 5
tests/util/benchmark_cost.py 0.0% lines 6
Total Missing Coverage
140 lines 3 lines 97%

Copy link
Contributor

@emlowe emlowe left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice work

@arvidn arvidn added ready_to_merge Submitter and reviewers think this is ready and removed coverage-diff labels Nov 7, 2023
@Starttoaster Starttoaster merged commit 95e5b97 into main Nov 7, 2023
254 of 255 checks passed
@Starttoaster Starttoaster deleted the use-bls-from-chia_rs branch November 7, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants