Thanks for this @itsdevbear. We will shortly be running large scale tests with (and without) this PR. If we see visible performance gain, we'll adopt this.

Will be CPU architecture dependant, ensure test environment is setup correctly

Thanks @itsdevbear.

@sergio-mena do you know if we have a mixed environment in our tests ? Do we test on Intel and ARM ?

Looks like the benefits are dependent on message size according to the repo. Might be also interesting to see if this could be tested with different input sizes.

One concern that I have though is that this library seems to be a bit stale, last commit was February 2023. Maybe it's stable enough. But since we're replacing it everywhere where a Sha256 is needed, changing it on hash, merkletree, keys, etc,

It is critical that this library is vetted. I understand this is used in production in many projects, but the concern is if there's a vulnerability how fast it can be addressed as opposed to using the official one that would be patched possibly faster.

We are in the process of having an expert in golang crypto libraries vet this library change.

I can also hide behind an optional build flag if desired.

@andynog yeah intel and arm, but notably will see the greatest improvement on cpus with AVX-512 support

This being said, we want to optimize some places in the code to use the AVX-512 scheduler directly. But that's a bigger change

This package was added by #2765. Not sure why they decided to use this precise implementation.

Overall the code now is kind of confusing with the use of:

• crypto/sha256: standard lib
• minio/sha256-simd: possibly improved lib
• crypto/tmhash: our own lib, which is a wrapper to crypto/sha256

We might consider simplifying this setup.

Personally, I like having everywhere in the codebase important from tmhash where tmhash is an alias it whatever library we like. This alias could be changed via build flags for instance.

many to one to few relationship

Free performance boost, used in hundreds of production projects.

Can you add a Go test/benchmark so we can evaluate this boost?

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:

• Open an issue (Remove dependency from minio/sha256-simd library #3071 thanks @cason !) to move all imports of minio/sha256-simd (BLS-related code) to crypto/sha256
• Wait for docs(ADR): #113 Modular transaction hashing #2241 to be completed so that users can do what is suggested in this issue on app-side code

Use this issue to move all imports of minio/sha256-simd (BLS-related code) to crypto/sha256

Opened #3071.

We currently don't have performance data that shows an obvious gain in the production setups where CometBFT is used

IDK how much realworldish gain there is, but:

Support for Intel SHA Extensions

hehe this would make it a lot faster on my homebrew routers which use the J3455 cpu

Support for AVX512

That's basically "every validator"

Agree with @andynog's concern about maintenance though, that is an easy way to get burned. PR into crypto/sha256? The performance gains are pretty undeniable, and while the SHA extensions are (bizarrely) on just one intel cpu + AMD ryzen, a lot of validators use Ryzen.

IDK how much realworldish gain there is, but:

That's the culprit of this PR 😅

https://twitter.com/phoronix/status/1790016231614849318

What scenario would allow us to see real world results here?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Closing as per #2909 (comment).