Feat: `bls` crate with modular `backend`s #69

shreyas-londhe · 2024-12-22T13:30:12Z

This PR generalizes the use of backends in the bls crate. New backends can be added to this crate and can be used by replacing features where the crate is used.

Resolves #61

CLAassistant · 2024-12-22T13:30:17Z

All committers have signed the CLA.

shreyas-londhe · 2024-12-22T13:33:29Z

Hi @sauliusgrigaitis I have implemented the necessary traits for the bls crate and added the blst backend as default. Can you please review my changes and let me know of any changes or suggestions you have?

I would proceed to add more backends starting with bls12_381 once you have given a ✅ to my current changes.

Thank you!

povi · 2024-12-23T11:20:24Z

Suggestion:

You don't need to prefix all the trait names with Bls*.
I guess the reason for adding the prefixes was that without them trait names clash with other bls type names.
You could drop the Bls* prefix from trait names and import them like this:

use bls::{
    traits::{PublicKey as _, SignatureBytes as _},
    PublicKey, SignatureBytes,
};

We rarely need imported trait names in code. Most of the time importing traits anonymously is better.

povi · 2024-12-23T11:53:10Z

Suggestion:

All the old code that was wrapping the blst crate types was moved to blst backend, and the new traits are just the method signatures and nothing more.

When you add a new bls backend, much of the code that wraps some newly added bls crate's types will probably be pretty much copy-pasted from blst backend with minor changes.

So my suggestion is to try and move as much as possible code from blst backend to the trait methods, so it can be reused with different backends.

sauliusgrigaitis · 2024-12-23T12:04:45Z

Thanks @shreyas-londhe for PR, but I agree with @povi. I think it's best if you proceed by adding bls12_381 crate and then you simply refactor your PR so it doesn't repeat the code.

shreyas-londhe · 2024-12-24T01:31:19Z

Thanks for the suggestions @povi. I'll add the bls12_281 crate and then move the repeated code to the traits.

shreyas-londhe · 2024-12-26T04:10:29Z

Hi @povi I made the changes you requested. Can you please tell me how I can test if my changes didn't break anything? Please let me know of any more suggestions you have :)

sauliusgrigaitis · 2024-12-29T16:01:04Z

@shreyas-londhe the very first step would be to fix the build. You can run the build locally by executing the commands from the build script https://github.com/grandinetech/grandine/blob/develop/.github/workflows/ci.yml.

shreyas-londhe · 2024-12-30T07:27:10Z

@sauliusgrigaitis I'm getting clippy errors which are not from the code I modified. Can you please check that once? I even tried running the tests locally and they fail at code which I have not modified. Can you please help me out on why the CI might be failing?

povi · 2024-12-30T07:53:58Z

transition_functions/src/phase0/block_processing.rs

        process_block_header,
        |_, state, block: Phase0BeaconBlock<_>, _| unphased::process_block_header(state, &block),
        "block",
        "consensus-spec-tests/tests/mainnet/phase0/operations/block_header/*/*",
-        "consensus-spec-tests/tests/minimal/phase0/operations/block_header/*/*",
-    }
+        "consensus-spec-tests/tests/minimal/phase0/operations/block_header/*/*"


I see a TON of changes that are either a space removed, a comma removed or a semicolon added. Neither of these changes are related to this pull request.

What is more, some of these changes you made break macros, like this one. Now it produces error:

error: unexpected end of macro invocation --> transition_functions/src/phase0/block_processing.rs:494:80 | 432 | macro_rules! processing_tests { | ----------------------------- when calling this macro ... 494 | "consensus-spec-tests/tests/minimal/phase0/operations/block_header/*/*" | ^ missing tokens in macro arguments | note: while trying to match `,` --> transition_functions/src/phase0/block_processing.rs:438:34 | 438 | $minimal_glob:literal,

Why did you make these changes?

@povi Looks like these were because of running ‘cargo fmt —all’. Let me remove those.

@povi Thanks for pointing this out, I reverted all the formatting changes which are not related to this PR. How do I invoke the CI workflow on my latest commit? I cannot run the CI on my local because I have limited resources and it's taking a lot of time for me to download the submodules.

@shreyas-londhe you should be able to fork the repo and push the changes to branches in your local repo. This should trigger CI.

sauliusgrigaitis · 2024-12-30T10:07:18Z

@sauliusgrigaitis I'm getting clippy errors which are not from the code I modified. Can you please check that once? I even tried running the tests locally and they fail at code which I have not modified. Can you please help me out on why the CI might be failing?

The build on develop branch passes https://github.com/grandinetech/grandine/commits/develop/ . If you don't know which of your changes crashed the build - the easiest way for you would be to apply your changes incrementally and see which change crashed the build.

shreyas-londhe · 2025-01-02T07:14:20Z

Hi @povi @sauliusgrigaitis I fixed all the clippy issues and made sure that the CI passes. Can you please review my PR and let me know of any suggestions?

sauliusgrigaitis · 2025-01-02T08:57:12Z

We want to make this inline with https://github.com/grandinetech/rust-kzg naming. So with the same switch both our bls and rust-kzg will be able to switch to the particular backend. So the name zkcrypto needs to be used instead of bls12_381. The same is with the other backends such as arkworks.

Can you also squash to a single commit after the naming is changed?

sauliusgrigaitis · 2025-01-02T12:06:10Z

I also suggest to to enable zkcrypto backend when running tests in Gitlab CI either on Macos or on Windows target.

sauliusgrigaitis · 2025-01-02T13:26:26Z

I also suggest to to enable zkcrypto backend when running tests in Gitlab CI either on Macos or on Windows target.

I just checked and it doesnt compile with --features bls12_381. So it really makes sense to add to Gitlab CI. Simply add it here.

shreyas-londhe · 2025-01-03T13:14:47Z

@sauliusgrigaitis The code compiles when default features are off. I have kept bls-blst as the default backend and other backends can be compiled by running --no-default-features --features <bls-backend>. Please let me know if this is desired or I should change it have no default feature.

sauliusgrigaitis · 2025-01-04T10:08:30Z

@shreyas-londhe you must test your code before you ask for review. It fails b3577bc

shreyas-londhe · 2025-01-05T10:17:35Z

Sorry for the confusion @sauliusgrigaitis, as you requested I changed the feature names to match rust-kzg.

# in `grandine` root
cd bls

# builds with `blst` backend
cargo build 

# builds with `zkcrypto (bls12_281)` backend
cargo build --no-default-features --features bls-zkcrypto

sauliusgrigaitis · 2025-01-05T10:37:00Z

Sorry for the confusion @sauliusgrigaitis, as you requested I changed the feature names to match rust-kzg.
# in `grandine` root
cd bls

# builds with `blst` backend
cargo build 

# builds with `zkcrypto (bls12_281)` backend
cargo build --no-default-features --features bls-zkcrypto

The switch needs to work on the top level, not in bls crate. You need to edit .github/workflows/ci.yml and add something like this shreyas-londhe/grandine@feat/modular-bls...grandinetech:grandine:feat/modular-bls . Then push this change to your fork and make sure that the build with enabled zkcrypto passes.

shreyas-londhe · 2025-01-07T04:16:57Z

Thanks for pointing this out @sauliusgrigaitis. The latest commit on my branch passes the CI tests with both bls-blst and bls-zkcrypto features enabled here. Please let me know if you have any more suggestions.

I am also planning to write a consistency check macro for testing the newly added backends with the blst backend so the issues I faced won't be repeated when more backends are added.

povi · 2025-01-10T06:06:55Z

Looks good! There are some minor things though that I'd suggest to change, I'll write them down.

bls/Cargo.toml

bls/bls-blst/Cargo.toml

bls/bls-core/src/traits/signature.rs

bls/bls-core/src/error.rs

bls/bls-blst/src/signature.rs

bls/bls-core/Cargo.toml

bls/Cargo.toml

sauliusgrigaitis · 2025-01-10T08:05:32Z

Did you find any way to use blst backend by default without explicitly specifying it always? It's very inconvenient.

shreyas-londhe · 2025-01-13T14:52:11Z

Did you find any way to use blst backend by default without explicitly specifying it always? It's very inconvenient.

@sauliusgrigaitis I'm not sure about this one. I tried using workspace.features but they don't seem to link the features correctly. @povi Do you have any suggestions on this one?

povi · 2025-01-14T12:06:57Z

@povi Do you have any suggestions on this one?

No, sorry, I don't have

shreyas-londhe · 2025-01-14T15:20:32Z

@sauliusgrigaitis I have one solution using workspace.metadata.commands, but I don't think that is suitable in this case.

ArtiomTr · 2025-01-23T17:39:01Z

Hey @shreyas-londhe, I've faced the same issue in #88, and solved it by specifying blst as default feature in grandine crate. Try this patch:

From 10a946bb54ab25156bdbbc40a31d72933b098dfa Mon Sep 17 00:00:00 2001
From: sirse <[email protected]>
Date: Thu, 23 Jan 2025 19:36:41 +0200
Subject: [PATCH] Mark blst as default feature

---
 grandine/Cargo.toml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/grandine/Cargo.toml b/grandine/Cargo.toml
index 65f9077..d7387f3 100644
--- a/grandine/Cargo.toml
+++ b/grandine/Cargo.toml
@@ -73,6 +73,8 @@ tempfile = { workspace = true }
 test-case = { workspace = true }
 
 [features]
+default = ["bls/blst"]
+
 logger-always-write-style = []
 
 # `preset-any` and `network-any` should not be passed to Cargo.
-- 
2.43.0

povi reviewed Dec 30, 2024

View reviewed changes

shreyas-londhe force-pushed the feat/modular-bls branch 2 times, most recently from 6e2a257 to de48d2e Compare January 2, 2025 03:57

shreyas-londhe force-pushed the feat/modular-bls branch 5 times, most recently from 81f56f2 to 48c6c34 Compare January 5, 2025 12:52

povi requested changes Jan 10, 2025

View reviewed changes

sauliusgrigaitis reviewed Jan 10, 2025

View reviewed changes

bls/bls-core/Cargo.toml Outdated Show resolved Hide resolved

sauliusgrigaitis reviewed Jan 10, 2025

View reviewed changes

bls/Cargo.toml Outdated Show resolved Hide resolved

shreyas-londhe mentioned this pull request Jan 13, 2025

Verkle testnet #53

Open

shreyas-londhe force-pushed the feat/modular-bls branch from f1efdaf to 0f1efc7 Compare January 15, 2025 04:43

feat: modularised bls crate to have mulitple backend impl

fcbb8ec

shreyas-londhe force-pushed the feat/modular-bls branch from 0f1efc7 to fcbb8ec Compare January 15, 2025 04:53

sauliusgrigaitis mentioned this pull request Jan 16, 2025

Added support for multiple KZG backends #88

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: `bls` crate with modular `backend`s #69

Feat: `bls` crate with modular `backend`s #69

shreyas-londhe commented Dec 22, 2024

CLAassistant commented Dec 22, 2024 •

edited

Loading

shreyas-londhe commented Dec 22, 2024

povi commented Dec 23, 2024

povi commented Dec 23, 2024

sauliusgrigaitis commented Dec 23, 2024

shreyas-londhe commented Dec 24, 2024

shreyas-londhe commented Dec 26, 2024

sauliusgrigaitis commented Dec 29, 2024

shreyas-londhe commented Dec 30, 2024

povi Dec 30, 2024

povi Dec 30, 2024

shreyas-londhe Dec 30, 2024

shreyas-londhe Dec 30, 2024 •

edited

Loading

sauliusgrigaitis Dec 30, 2024

sauliusgrigaitis commented Dec 30, 2024

shreyas-londhe commented Jan 2, 2025

sauliusgrigaitis commented Jan 2, 2025

sauliusgrigaitis commented Jan 2, 2025

sauliusgrigaitis commented Jan 2, 2025

shreyas-londhe commented Jan 3, 2025 •

edited

Loading

sauliusgrigaitis commented Jan 4, 2025

shreyas-londhe commented Jan 5, 2025 •

edited

Loading

sauliusgrigaitis commented Jan 5, 2025

shreyas-londhe commented Jan 7, 2025

povi commented Jan 10, 2025

sauliusgrigaitis commented Jan 10, 2025

shreyas-londhe commented Jan 13, 2025 •

edited

Loading

povi commented Jan 14, 2025

shreyas-londhe commented Jan 14, 2025

ArtiomTr commented Jan 23, 2025

Feat: bls crate with modular backends #69

Are you sure you want to change the base?

Feat: bls crate with modular backends #69

Conversation

shreyas-londhe commented Dec 22, 2024

CLAassistant commented Dec 22, 2024 • edited Loading

shreyas-londhe commented Dec 22, 2024

povi commented Dec 23, 2024

povi commented Dec 23, 2024

sauliusgrigaitis commented Dec 23, 2024

shreyas-londhe commented Dec 24, 2024

shreyas-londhe commented Dec 26, 2024

sauliusgrigaitis commented Dec 29, 2024

shreyas-londhe commented Dec 30, 2024

povi Dec 30, 2024

Choose a reason for hiding this comment

povi Dec 30, 2024

Choose a reason for hiding this comment

shreyas-londhe Dec 30, 2024

Choose a reason for hiding this comment

shreyas-londhe Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

sauliusgrigaitis Dec 30, 2024

Choose a reason for hiding this comment

sauliusgrigaitis commented Dec 30, 2024

shreyas-londhe commented Jan 2, 2025

sauliusgrigaitis commented Jan 2, 2025

sauliusgrigaitis commented Jan 2, 2025

sauliusgrigaitis commented Jan 2, 2025

shreyas-londhe commented Jan 3, 2025 • edited Loading

sauliusgrigaitis commented Jan 4, 2025

shreyas-londhe commented Jan 5, 2025 • edited Loading

sauliusgrigaitis commented Jan 5, 2025

shreyas-londhe commented Jan 7, 2025

povi commented Jan 10, 2025

sauliusgrigaitis commented Jan 10, 2025

shreyas-londhe commented Jan 13, 2025 • edited Loading

povi commented Jan 14, 2025

shreyas-londhe commented Jan 14, 2025

ArtiomTr commented Jan 23, 2025

Feat: `bls` crate with modular `backend`s #69

Feat: `bls` crate with modular `backend`s #69

CLAassistant commented Dec 22, 2024 •

edited

Loading

shreyas-londhe Dec 30, 2024 •

edited

Loading

shreyas-londhe commented Jan 3, 2025 •

edited

Loading

shreyas-londhe commented Jan 5, 2025 •

edited

Loading

shreyas-londhe commented Jan 13, 2025 •

edited

Loading