Migrate BEEFY BLS crypto to bls12-381 curve #4931
Conversation
- Make BEEFY pair crypto to be (ECDSA,BLS12-381) instead of (ECDSA,BLS12-377)
…h bls377 and bls381
There was a problem hiding this comment.
Mostly looks good. Some considerations to discuss before approval:
- Do you think we can remove the
bls-experimentalfeature? - I see that in keystore and host functions (io) we kept bls12-377 functionalities together with the new bls12-381. Maybe we can remove bls12-377 functions as far as they are not used/required/requested by anyone yet? I'd keep both only in
primitives/corefor the moment (i.e. the lower level primitives)
| @@ -198,15 +198,15 @@ mod tests { | |||
| // from signed take a function as the aggregator | |||
| TestBlsSignedCommitmentWitness::from_signed::<_, _>(signed, |sigs| { | |||
| // we are going to aggregate the signatures here | |||
| let mut aggregatedsigs: SignatureAggregatorAssumingPoP<TinyBLS377> = | |||
| let mut aggregatedsigs: SignatureAggregatorAssumingPoP<TinyBLS381> = | |||
There was a problem hiding this comment.
This is only a test, but would be nice to encapsulate the BLS backend in primitives/core bls module.
Maybe we can construct a stand alone function exposing the aggregated signature there?
Is required only for this test?
Also this test doesn't have any assertion.
There was a problem hiding this comment.
I implemented such a thing during the Prague hackathon: w3f@53d90d3#diff-8b079a4b5fd964b13e8f4c7da10f3a924d015f1dec8f4bad2059eb3ab20ca94fR246 I asked @AlistairStewart if I should port those efforts here. He disagreed. He said that there was a plan to publish signature accumulation (either Merkle root or BLS aggregation) in BEEFY messages but that plan was scrapped. That is why you see "witness" only shows up in test. The current plan is that the aggregation should entirely happens by the relayer/prover.
Having said that, I'm happy to do whatever gets this pull request approved >:-). I could remove the test entirely or if you really insists that the aggregation code shows up in the test or in the backend, I'd think Al won't object if the approval of the pull request hanging on this, his concern was mostly time spend on something we won't use I assume.
There was a problem hiding this comment.
Was mostly a nitpick. What about the experimental feature and not exposing the bls12-377 stuff in keystore and sp-io?
There was a problem hiding this comment.
I'll remove bls12-377 from sp-io and application crypto and remove as much as needed to make it compile. I'd keep the experimental feature for now. I'd remove it in the next pull request when I implement proof of possession check, without that BLS aggregations are not secure.
davxy
added
R0-silent
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
@davxy I removed bls12-377 from io and also I had to remove the application cryptos which were using the gen functions. The rest is not user facing and compiles. I assume it is Ok to stay there, in case of some future application (bridging to Celo for example) |
|
@drskalman Looks good. Once the pipeline is fixed (I think there are some clippy issues) we can merge |
76a6d47
We are definitely going to use BLS12-381 for BEEFY and it is hard coded in JAM's spec. This PR implements missing tests for bls12-381 crypto, migrate BEEFY BLS crypto to bls12-381 and adapt the BEEFY primitive tests accordingly. --------- Co-authored-by: Davide Galassi <[email protected]>
* master: (27 commits) Bridges improved tests and nits (#5128) Fix misleading comment about RewardHandler in epm config (#3095) Introduce a workflow updating the wishlist leaderboards (#5085) membership: Restructure pallet into separate files (#4536) Fix after ring-proof api change (#5126) Bump paritytech/review-bot from 2.4.0 to 2.5.0 (#5057) Bump docker/login-action from 3.0.0 to 3.3.0 (#5109) Bump docker/build-push-action from 5.1.0 to 6.5.0 (#5108) Bump peter-evans/create-pull-request from 5.0.0 to 6.1.0 (#5093) Tx Payment: drop ED requirements for tx payments with exchangeable asset (#4488) Remove `pallet-getter` usage from pallet-transaction-payment (#4970) pallet macro: do not generate try-runtime related code when frame-support doesn't have try-runtime. (#5099) fix(chain-spec): ChainSpecBuilder with object as default genesis (#4345) Migrate BEEFY BLS crypto to bls12-381 curve (#4931) Bump clap from 4.5.9 to 4.5.10 in the known_good_semver group (#5120) Use jobserver in wasm-builder to limit concurrency of spawned cargo processes (#4946) include events for voting (#4613) [subsystem-bench] Add mocks for own assignments triggering (#5042) Remove not-audited warning (#5114) hotfix: blockchain/backend: Skip genesis leaf to unblock syncing (#5103) ...
We are definitely going to use BLS12-381 for BEEFY and it is hard coded in JAM's spec. This PR implements missing tests for bls12-381 crypto, migrate BEEFY BLS crypto to bls12-381 and adapt the BEEFY primitive tests accordingly.