Skip to content
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.

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

precompiles: Add SHA256 implementation #924

Merged
merged 7 commits into from
Jun 25, 2024
Merged

precompiles: Add SHA256 implementation #924

merged 7 commits into from
Jun 25, 2024

Conversation

chfast
Copy link
Member

@chfast chfast commented Jun 8, 2024

This adds the SHA256 implementation from Silkworm and removes the precompile stub.

axic
axic previously approved these changes Jun 8, 2024
Copy link

codecov bot commented Jun 8, 2024

Codecov Report

Attention: Patch coverage is 43.60000% with 141 lines in your changes missing coverage. Please review.

Project coverage is 94.29%. Comparing base (9be04a5) to head (b697c4f).

Current head b697c4f differs from pull request most recent head 66cb577

Please upload reports for the commit 66cb577 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #924      +/-   ##
==========================================
- Coverage   95.12%   94.29%   -0.83%     
==========================================
  Files         140      142       +2     
  Lines       15892    16102     +210     
==========================================
+ Hits        15117    15184      +67     
- Misses        775      918     +143     
Flag Coverage Δ
eof_execution_spec_tests 16.26% <8.00%> (-0.34%) ⬇️
ethereum_tests 26.96% <40.80%> (+0.05%) ⬆️
ethereum_tests_silkpre 18.87% <88.00%> (+0.52%) ⬆️
execution_spec_tests 18.00% <38.00%> (+0.17%) ⬆️
unittests 89.68% <42.00%> (-0.60%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
test/state/precompiles.cpp 100.00% <100.00%> (ø)
test/state/precompiles_stubs.cpp 100.00% <ø> (ø)
test/unittests/precompiles_sha256_test.cpp 100.00% <100.00%> (ø)
lib/evmone_precompiles/sha256.cpp 41.25% <41.25%> (ø)

... and 3 files with indirect coverage changes

@chfast chfast requested a review from battlmonstr June 8, 2024 18:05
@chfast chfast force-pushed the crypto/sha256 branch 2 times, most recently from 985dbdb to c102779 Compare June 10, 2024 08:35
@chfast chfast requested a review from axic June 10, 2024 09:38
@chfast
Copy link
Member Author

chfast commented Jun 10, 2024

I tested this additionally on AMD Zen2 CPU with sha_ni extension.

@chfast chfast force-pushed the crypto/sha256 branch 2 times, most recently from eadca78 to 23a3e62 Compare June 11, 2024 10:06
@chfast chfast requested a review from rodiazet June 11, 2024 10:07
@chfast chfast dismissed axic’s stale review June 11, 2024 11:00

Outdated, the code has changed a lot since.

@chfast chfast force-pushed the crypto/sha256 branch 6 times, most recently from 5b6bf1b to 6042829 Compare June 12, 2024 11:32
@chfast chfast force-pushed the crypto/sha256 branch 2 times, most recently from 7867022 to b697c4f Compare June 25, 2024 08:46
Copy the C implementation of SHA256 from Silkworm to evmone
without any modifications.
The original source file:
https://github.com/erigontech/silkworm/blob/capi-0.19.0/silkworm/core/crypto/sha256.c
Convert C implementation of SHA256 to C++. evmone is declared as
C++ only project. This also enabled previously missed coverage
for the sha256.cpp file.
Improve SHA256 code quality by:
- fixing/disabling some clang-tidy warnings,
- adding `BufferState` constructor,
- using `std::rotr()`,
- using C++ attributes.
Use `std::byte` instead of `uint8_t`
and drop the `use_cpu_extensions` parameter.
@chfast chfast enabled auto-merge June 25, 2024 19:49
@chfast chfast merged commit 3cab4ca into master Jun 25, 2024
24 checks passed
@chfast chfast deleted the crypto/sha256 branch June 25, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants