8000 feat(crypto): add BLS12-381 curve by melekes · Pull Request #2765 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(crypto): add BLS12-381 curve #2765

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 27 commits into from
Apr 11, 2024
Merged

feat(crypto): add BLS12-381 curve #2765

merged 27 commits into from
Apr 11, 2024

Conversation

melekes
Copy link
Contributor
@melekes melekes commented Apr 10, 2024 •
edited
Loading

curve based on blst.

The bls12381 build tag was added. Also, note that BLS requires cgo.

Co-authored by: @tac0turtle

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec

@melekes melekes requested review from a team as code owners April 10, 2024 07:09
@melekes melekes added backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x Tell Mergify to backport the PR to v1.x and removed backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x labels Apr 10, 2024
@melekes melekes mentioned this pull request Apr 10, 2024
Closed
4 tasks
cason
cason reviewed Apr 10, 2024
View reviewed changes
Copy link
Contributor
@cason cason left a comment

Choose a reason for hiding this comment

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

Left some general comments.

Not sure if the solution for having this key optional, depending on the compiling flags, is ideal.

types/block.go Outdated
// 1 byte for the flag and 14 bytes for the timestamp.
MaxCommitSigBytes int64 = 109
MaxCommitSigBytes int64 = 141
Copy link
Contributor

Choose a reason for hiding this comment

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

I am afraid the math here is wrong: 96 + 20 + 1 + 14 = 131.

Can we somehow test this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The math was wrong before, too. I don't know why exactly.

types/block.go Outdated
MaxCommitSigBytes int64 = 141

// protoEncodingOverhead represents the overhead in bytes when encoding a protocol buffer message.
protoEncodingOverhead int64 = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Why private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's probably because it's an implementation detail.

@melekes melekes marked this pull request as draft April 10, 2024 07:25
@tac0turtle
Copy link
Contributor

Cgo is more performant. Since there is no aggreagtion in comet, yet the performance overhead will be high for doing individual signatures.

melekes added a commit that referenced this pull request Apr 11, 2024
@mergify @melekes
feat(crypto): add BLS12-381 curve (backport #2765) (#2772)
354c6be 
[curve](https://github.com/cosmos/crypto/tree/marko/bls12381) based on
[blst](https://github.com/supranational/blst).

The `bls12381` build tag was added. Also, note that BLS requires `cgo`.

Co-authored by: @tac0turtle 

---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #2765 done by
[Mergify](https://mergify.com).

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
go.mod
@@ -42,11 +42,13 @@ require (
github.com/cometbft/cometbft-db v0.11.1-0.20240407063702-fa37a805b0b4
github.com/cometbft/cometbft-load-test v0.1.0
github.com/cometbft/cometbft/api v1.0.0-alpha.2
github.com/cosmos/crypto v0.0.0-20240309083813-82ed2537802e
Copy link
Contributor

Choose a reason for hiding this comment

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

No official release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope

cc @tac0turtle

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine, then

sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
sergio-mena
sergio-mena reviewed Apr 17, 2024
View reviewed changes
@sergio-mena
Copy link
Contributor

I added a few comments, some of them important. Sorry for arriving late to this review

This was referenced Apr 17, 2024
Merged
Closed
github-merge-queue bot pushed a commit that referenced this pull request Apr 18, 2024
@melekes @sergio-mena
refactor(crypto): handle errors and more in bls12381 (#2832)
d27732a 
- pin cometbft-db-testing image version
- extract const in a separate file
- return err in GenPrivKey and NewPrivateKeyFromBytes

Follow-up to #2765

---

#### PR checklist

- [x] Tests written/updated
- [ ] ~~Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)~~
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Sergio Mena <sergio@informal.systems>
mergify bot pushed a commit that referenced this pull request Apr 18, 2024
@melekes @sergio-mena @mergify
refactor(crypto): handle errors and more in bls12381 (#2832)
4cb1719 
- pin cometbft-db-testing image version
- extract const in a separate file
- return err in GenPrivKey and NewPrivateKeyFromBytes

Follow-up to #2765

---

#### PR checklist

- [x] Tests written/updated
- [ ] ~~Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)~~
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Sergio Mena <sergio@informal.systems>
(cherry picked from commit d27732a)
@mergify mergify bot mentioned this pull request Apr 18, 2024
Merged
4 tasks
melekes added a commit that referenced this pull request Apr 18, 2024
@mergify @melekes
refactor(crypto): handle errors and more in bls12381 (backport #2832)…
b86bec4 
… (#2842)

- pin cometbft-db-testing image version
- extract const in a separate file
- return err in GenPrivKey and NewPrivateKeyFromBytes

Follow-up to #2765

---

#### PR checklist

- [x] Tests written/updated
- [ ] ~~Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)~~
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #2832 done by
[Mergify](https://mergify.com).

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@cason cason mentioned this pull request May 8, 2024
Closed
4 tasks
cason
cason revi CEB7 ewed May 8, 2024
View reviewed changes
go.mod
github.com/cosmos/gogoproto v1.4.12
github.com/go-git/go-git/v5 v5.12.0
github.com/goccmack/goutil v1.2.3
github.com/gofrs/uuid v4.4.0+incompatible
github.com/google/uuid v1.6.0
github.com/minio/sha256-simd v1.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why using this version instead of the standard one?

@cason cason mentioned this pull request May 13, 2024
Closed
@jmalicevic jmalicevic mentioned this pull request Dec 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cason cason cason approved these changes

@sergio-mena sergio-mena sergio-mena left review comments

Assignees

@melekes melekes

Labels
backport-to-v1.x Tell Mergify to backport the PR to v1.x
Projects
No open projects
CometBFT
Status: Done
Milestone
2024-Q2
Development

Successfully merging this pull request may close these issues.
5 participants
@melekes @sergio-mena @itsdevbear @tac0turtle @cason
0