Add native implementation of RIPEMD160 #846
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #846 +/- ##
==========================================
- Coverage 98.13% 98.13% -0.01%
==========================================
Files 123 125 +2
Lines 12992 13095 +103
==========================================
+ Hits 12750 12851 +101
- Misses 242 244 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
085ef67 to
f0b686f
Compare
bf3bd05 to
5187061
Compare
| { | ||
| namespace | ||
| { | ||
| // TODO(C++23): Use std::byteswap. |
There was a problem hiding this comment.
I've used __cplusplus instead of TODO(C++23) and it is possible to use intrinsics if you know sizeof(T).
There was a problem hiding this comment.
You can even use a feature macro: https://en.cppreference.com/w/cpp/feature_test#cpp_lib_byteswap.
Here I was playing a bit with portable C++. I'd slightly uncomfortable to enable C++23 implementation if we don't test C++23 builds.
There was a problem hiding this comment.
I prefer to not keep TODOs in code if possible due to https://cwe.mitre.org/data/definitions/546
If you want to avoid people trying C++23, it is better to disable it in the global makelist, and in the error message explain how to disable it if they want to still use C++23 at their own risk (maybe with an extra flag).
Using __cplusplus feels simpler to my taste, because once we update our baseline, which is likely to happen, it will be easier to clean up the code.
There was a problem hiding this comment.
The TODO(C++23) comments mean: after C++23 is enabled you can come here and simplify/improve the code. I actually did this for C++20 and this is not much work but can discover still missing features in compilers.
Maybe we can use different tag than TODO, but it won't solve the CWE issue.
| }; | ||
|
|
||
| /// Converts native representation to/from little-endian. | ||
| inline auto to_le(std::integral auto x) noexcept |
There was a problem hiding this comment.
similar methods exist in intx::le namespace in case you are able to use intx.hpp here, see: https://github.com/erigontech/silkworm/blob/master/silkworm/core/common/endian.hpp
There was a problem hiding this comment.
On one hand I plan to improve intx's API for this. On the other hand, I wanted to stick to C++ std only for this file.
| // tbl[static_cast<size_t>(PrecompileId::ecrecover)].execute = silkpre_ecrecover_execute; | ||
| tbl[static_cast<size_t>(PrecompileId::sha256)].execute = silkpre_sha256_execute; | ||
| tbl[static_cast<size_t>(PrecompileId::ripemd160)].execute = silkpre_ripemd160_execute; | ||
| // tbl[static_cast<size_t>(PrecompileId::ripemd160)].execute = silkpre_ripemd160_execute; |
There was a problem hiding this comment.
These override evmone's implementations of precompiles with silkpre. This does not make sense any more for RIPEMD160 by default as evmone just got the native implementation which is faster.
There was a problem hiding this comment.
maybe remove it completely instead of commenting?
There was a problem hiding this comment.
This section gives the visualization what has been replaced so far. The goal is get rid of silkpre dependency.
5187061 to
f82140a
Compare
Add C++ implementation of the RIPEMD160 and use it for the precompile
0x03.