8000 Remove dependency from `minio/sha256-simd` library · Issue #3071 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove dependency from minio/sha256-simd library #3071

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
Closed
cason opened this issue May 13, 2024 · 2 comments · Fixed by #3074
Closed

Remove dependency from minio/sha256-simd library #3071

cason opened this issue May 13, 2024 · 2 comments · Fixed by #3074
Assignees
@melekes
Labels
crypto Cryptography-related hygiene Any work relating to code legibility/hygiene to make it easier to read

Comments

@cason
Copy link
Contributor
cason commented May 13, 2024 •
edited
Loading

The dependency to this library was introduced by #2765.

It is present on main and v1.x branches (which is still unreleased).

We should at the moment stick to the standard crypto/sha256 library, with the same purpose.

The adoption of minio/sha256-simd was proposed in #2909, but at the moment we are not planning to switch to this non-standard library, as summarized in the following comment:

We discussed about this in our internal meeting today. These are the conclusions:

  1. We currently don't have performance data that shows an obvious gain in the production setups where CometBFT is used
  2. We could gather such performance data ourselves as a due-diligence activity, but we don't have cycles ATM
  3. There is an ongoing longer term solution, described in docs(ADR): #113 Modular transaction hashing #2241, whereby an app can have CometBFT use custom hashing logic.
    • This longer term solution will render our efforts in point 2 unnecessary

Therefore, we will:

Originally posted by @sergio-mena in #2909 (comment)
@cason cason added crypto Cryptography-related hygiene Any work relating to code legibility/hygiene to make it easier to read labels May 13, 2024
@cason cason added this to CometBFT May 13, 2024
@github-project-automation github-project-automation bot moved this to Todo in CometBFT May 13, 2024
@cason cason mentioned this issue May 13, 2024
Closed
4 tasks
@cason
Copy link
Contributor Author
cason commented May 13, 2024

Ideally we should adopt crypto/tmhash, our own lib, which is currently a wrapper to crypto/sha256. And in a second moment, allow for multiple implementations of this libraries, via build option of something similar.

#2909 (comment)

@andynog
Copy link
Contributor
andynog commented May 13, 2024

I agree that we should revert the additional library added in the BLS key, as we don't have enough benchmarking tests to justify the need for the minio library to make BLS signatures feasible.

Since BLS is optional to build with a flag, I believe it would be appropriate to encapsulate the logic under tmhash and work towards a solution that allows tmhash to be modular and utilize different crypto hashing libraries.

@melekes melekes self-assigned this May 14, 2024
melekes added a commit that referenced this issue May 14, 2024
@melekes
deps: remove minio/sha256-simd
204 8000 2bae 
Closes #3071
@melekes melekes mentioned this issue May 14, 2024
Merged
4 tasks
github-merge-queue bot pushed a commit that referenced this issue May 14, 2024
@melekes
chore(deps): remove minio/sha256-simd (#3074)
2e56388 
Closes #3071

<!--

Please add a reference to the issue that this PR addresses and indicate
which
files are most critical to review. If it fully addresses a particular
issue,
please include "Closes #XXX" (where "XXX" is the issue number).

If this PR is non-trivial/large/complex, please ensure that you have
either
created an issue that the team's had a chance to respond to, or had some
discussion with the team prior to submitting substantial pull requests.
The team
can be reached via GitHub Discussions or the Cosmos Network Discord
server in
the #cometbft channel. GitHub Discussions is preferred over Discord as
it
allows us to keep track of conversations topically.
https://github.com/cometbft/cometbft/discussions

If the work in this PR is not aligned with the team's current
priorities, please
be advised that it may take some time before it is merged - especially
if it has
not yet been discussed with the team.

See the project board for the team's current priorities:
https://github.com/orgs/cometbft/projects/1

-->

---

#### PR checklist

- [ ] ~~Tests written/updated~~
- [ ] ~~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
@github-project-automation github-project-automation bot moved this from Todo to Done in CometBFT May 14, 2024
mergify bot pushed a commit that referenced this issue May 14, 2024
@melekes @mergify
chore(deps): remove minio/sha256-simd (#3074)
90f2cd7 
Closes #3071

<!--

Please add a reference to the issue that this PR addresses and indicate
which
files are most critical to review. If it fully addresses a particular
issue,
please include "Closes #XXX" (where "XXX" is the issue number).

If this PR is non-trivial/large/complex, please ensure that you have
either
created an issue that the team's had a chance to respond to, or had some
discussion with the team prior to submitting substantial pull requests.
The team
can be reached via GitHub Discussions or the Cosmos Network Discord
server in
the #cometbft channel. GitHub Discussions is preferred over Discord as
it
allows us to keep track of conversations topically.
https://github.com/cometbft/cometbft/discussions

If the work in this PR is not aligned with the team's current
priorities, please
be advised that it may take some time before it is merged - especially
if it has
not yet been discussed with the team.

See the project board for the team's current priorities:
https://github.com/orgs/cometbft/projects/1

-->

---

#### PR checklist

- [ ] ~~Tests written/updated~~
- [ ] ~~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

(cherry picked from commit 2e56388)
@mergify mergify bot mentioned this issue May 14, 2024
Merged
4 tasks
melekes added a commit that referenced this issue May 14, 2024
@mergify @melekes
chore(deps): remove minio/sha256-simd (backport #3074) (#3076)
4d5ed69 
Closes #3071



---

#### PR checklist

- [ ] ~~Tests written/updated~~
- [ ] ~~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 #3074 done by
[Mergify](https://mergify.com).

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@melekes melekes

Labels
crypto Cryptography-related hygiene Any work relating to code legibility/hygiene to make it easier to read
Projects
No open projects
CometBFT
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore(deps): remove minio/sha256-simd cometbft/cometbft
3 participants
@cason @melekes @andynog
0