crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 #29180

theuni · 2024-01-04T17:17:42Z

Replace it with a more explicit DISABLE_OPTIMIZED_SHA256 and clean up some.

The macro was originally used by libbitcoinconsensus which opts out of optimized sha256 for the sake of simplicity.

Also remove the BUILD_BITCOIN_INTERNAL define from libbitcoinkernel for now as it does not export an api. When it does we can pick a less confusing define to control its exports.

Removing the define should have the effect of enabling sha256 optimizations for the kernel.

DrahtBot · 2024-01-04T17:17:45Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28893 (Fix SSE4.1-related issues by hebasto)
#28526 (Add 1-way SSE4 SHA256 implementation using intrinsics for MSVC builds by hebasto)
#24773 (Enable HW-accelerated implementations of SHA256 for MSVC builds by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

theuni · 2024-01-04T17:18:57Z

Ping @TheCharlatan @hebasto

I went around in circles several times on this. I think it accomplishes what we need, but please sanity check me :)

hebasto · 2024-01-04T17:19:14Z

src/Makefile.am

@@ -1012,7 +1012,7 @@ libbitcoinconsensus_la_SOURCES = support/cleanse.cpp $(crypto_libbitcoin_crypto_

 libbitcoinconsensus_la_LDFLAGS = $(AM_LDFLAGS) -no-undefined $(RELDFLAGS)
 libbitcoinconsensus_la_LIBADD = $(LIBSECP256K1)
-libbitcoinconsensus_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(builddir)/obj -I$(srcdir)/secp256k1/include -DBUILD_BITCOIN_INTERNAL
+libbitcoinconsensus_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(builddir)/obj -I$(srcdir)/secp256k1/include -DBUILD_BITCOIN_INTERNAL -DDISABLE_OPTIMIZED_SHA256


Is it needed to keep -DBUILD_BITCOIN_INTERNAL now?

Yes, this macro is now exclusively used to control exports.

Updated for @fanquake's request: it's now exclusively used to control libbitcoinconsensus exports. libbitcoinkernel will get a new define when the time comes.

hebasto · 2024-01-04T20:06:00Z

Concept ACK.

It makes the libbitcoinkernel code equivalent for both internal and external usage.

hebasto

In the master branch, the following code:

bitcoin/src/crypto/sha256.cpp

Lines 635 to 640 in dcfb9ee

    
               if (have_sse4) { 
        
           #if defined(__x86_64__) || defined(__amd64__) 
        
                   Transform = sha256_sse4::Transform; 
        
                   TransformD64 = TransformD64Wrapper<sha256_sse4::Transform>; 
        
                   ret = "sse4(1way)"; 
        
           #endif

compiles regardless of the BUILD_BITCOIN_INTERNAL macro. Now it is gated with the DISABLE_OPTIMIZED_SHA256 one. Is it intentional?

src/crypto/sha256.cpp

DrahtBot · 2024-01-05T02:51:19Z

Guix builds (on x86_64)

File	commit `737e588` (master)	commit `15b2094` (master and this pull)
SHA256SUMS.part	`a05449ccd1eedf9d...`	`290e73441a4a2464...`
*-aarch64-linux-gnu-debug.tar.gz	`566e2e720e510496...`	`c5126acb30e5b7b9...`
*-aarch64-linux-gnu.tar.gz	`ac229ee212444de0...`	`90d488f0d3c142de...`
*-arm-linux-gnueabihf-debug.tar.gz	`102d6be068c3d6db...`	`2ce9547c69e7111d...`
*-arm-linux-gnueabihf.tar.gz	`d9dbd398b7ac417a...`	`340b890fbab3d982...`
*-arm64-apple-darwin-unsigned.tar.gz	`0e35063d34b37755...`	`9d498e301260de90...`
*-arm64-apple-darwin-unsigned.zip	`75cc4be9a95235df...`	`b907e11c60a38aa4...`
*-arm64-apple-darwin.tar.gz	`661ebadfcd81951b...`	`3407d7456856adbb...`
*-powerpc64-linux-gnu-debug.tar.gz	`4e30d97f09a8ad90...`	`722233445121474f...`
*-powerpc64-linux-gnu.tar.gz	`2fa917b453e3b98e...`	`61c0954dda76898f...`
*-powerpc64le-linux-gnu-debug.tar.gz	`57964282dce22f4d...`	`4baa0e715b2db5e8...`
*-powerpc64le-linux-gnu.tar.gz	`ceb6f7cf6acdff9f...`	`d401224f54174c09...`
*-riscv64-linux-gnu-debug.tar.gz	`436eb35b0015a18e...`	`97c440a19e783274...`
*-riscv64-linux-gnu.tar.gz	`d3022d5be39d5c59...`	`cd9bde04c30c6fb6...`
*-x86_64-apple-darwin-unsigned.tar.gz	`c5a222621368a649...`	`ee3b80f1850dca71...`
*-x86_64-apple-darwin-unsigned.zip	`3148c2407851a49f...`	`ba7b9d7b39c2fbe8...`
*-x86_64-apple-darwin.tar.gz	`0bd0c25b23aa186d...`	`1ba89ac24252dffc...`
*-x86_64-linux-gnu-debug.tar.gz	`bba534832b7dd68f...`	`0994c6c7913e957d...`
*-x86_64-linux-gnu.tar.gz	`e8b23d8149cb6e00...`	`d19a82fe81f7612d...`
*.tar.gz	`3f2dc263df892f77...`	`0e1dd1e8c541322c...`
guix_build.log	`5b6f96ccdf1553af...`	`2c1e7ebc9ec58362...`
guix_build.log.diff		`6ae8f28660e7c10f...`

TheCharlatan · 2024-01-05T11:27:22Z

Re comment #29180 (review)

Now it is gated with the DISABLE_OPTIMIZED_SHA256 one. Is it intentional?

That code seems to be unused currently if BUILD_BITCOIN_INTERNAL is defined, so this seems like an improvement to me.

theuni · 2024-01-05T11:27:54Z

compiles regardless of the BUILD_BITCOIN_INTERNAL macro. Now it is gated with the DISABLE_OPTIMIZED_SHA256 one. Is it intentional?

Thanks for pointing this out. Yes it was intentional as I believe this is currently wonky in master.

Note that libbitcoinconsensus is the only direct user of LIBBITCOIN_CRYPTO_BASE. And because it never calls SHA256AutoDetect it will never actually use sha256_sse4::Transform. It's therefore useless to include sse4 in LIBBITCOIN_CRYPTO_BASE.

I'll push another commit to clean that up. It was a bit of an outlier before because it didn't require any special CXXFLAGS like the others do (-msse4.1, -mavx, etc). But we can just treat it as though it's one of the others.

As a bonus, LIBBITCOIN_CRYPTO_BASE will now have a very clear meaning: 100% unspecialized implementations with no runtime opt-ins. I believe that's much more clear than the single outlier we have today.

hebasto

Approach ACK 9235801.

Replace it with a more explicit DISABLE_OPTIMIZED_SHA256 and clean up some. The macro was originally used by libbitcoinconsensus which opts out of optimized sha256 for the sake of simplicity. Also remove the BUILD_BITCOIN_INTERNAL define from libbitcoinkernel for now as it does not export an api. When it does we can pick a less confusing define to control its exports. Removing the define should have the effect of enabling sha256 optimizations for the kernel.

theuni · 2024-01-05T12:40:05Z

Added a brief description of crypto_base.

hebasto

ACK 86712c3.

src/Makefile.am

It was unused there and a confusing outlier.

TheCharlatan

ACK 86712c3

Guix builds (aarch64):

e00ac9d2dbf9d1b6a0255d7c6669ee3925696109cf4ca549dbf411da235f52bc  guix-build-86712c313578/output/aarch64-linux-gnu/SHA256SUMS.part
871f7e66c454ac08d4e04f0d86d23e2b34a6c3a6a3b60aabcb9485c086cdad37  guix-build-86712c313578/output/aarch64-linux-gnu/bitcoin-86712c313578-aarch64-linux-gnu-debug.tar.gz
f0553ca6ebaaf5eb8d00196a681a20c21d4b81cb3ec907cce01ac6a446de02e7  guix-build-86712c313578/output/aarch64-linux-gnu/bitcoin-86712c313578-aarch64-linux-gnu.tar.gz
476395fad048ca38b226d3c108b18262127c9c0731afb1c8048ff5e4643039f8  guix-build-86712c313578/output/arm-linux-gnueabihf/SHA256SUMS.part
7de1c5c3685ca1d81a78d4b0a3e62b716e65341bbfdf4a0c56d154889c08ef26  guix-build-86712c313578/output/arm-linux-gnueabihf/bitcoin-86712c313578-arm-linux-gnueabihf-debug.tar.gz
9558f4afa8c466324cfbd046731e0915fc1041aa250d05ecd7bf74a41594b928  guix-build-86712c313578/output/arm-linux-gnueabihf/bitcoin-86712c313578-arm-linux-gnueabihf.tar.gz
f008cb1a5ef888be0a82dcffe6d58bb8379d429448e5c4de1cef208533847b69  guix-build-86712c313578/output/arm64-apple-darwin/SHA256SUMS.part
acd7300c467e06ebfc3b632a61fe2f5b1ad8d35d0181d18107bc24b7a66e0947  guix-build-86712c313578/output/arm64-apple-darwin/bitcoin-86712c313578-arm64-apple-darwin-unsigned.tar.gz
e36e3ff8d05dd6e53b9e2ec60b42e2fa0f14933c571eda8117c93656249590c3  guix-build-86712c313578/output/arm64-apple-darwin/bitcoin-86712c313578-arm64-apple-darwin-unsigned.zip
4a923b366e185a1eb4065c3ae6175bc7459cb15177b6cc036b48b63b94214846  guix-build-86712c313578/output/arm64-apple-darwin/bitcoin-86712c313578-arm64-apple-darwin.tar.gz
0d09788c222ca4d25fb08c504e1b8b472f15b476c22be27470e33dc963e1433b  guix-build-86712c313578/output/dist-archive/bitcoin-86712c313578.tar.gz
63ba5f9d412197d0a5ce83ce4eb2627a7fb571199fda97ce393fb66d5537e121  guix-build-86712c313578/output/powerpc64-linux-gnu/SHA256SUMS.part
c9a5dc8214e196d3ae211f0c30ed4ed93b19926eebc9fff35c1eee92011141a1  guix-build-86712c313578/output/powerpc64-linux-gnu/bitcoin-86712c313578-powerpc64-linux-gnu-debug.tar.gz
681549c7fe284c4bc03257ce0030482e95da6514a4e8e4a75bb19381f3994fa6  guix-build-86712c313578/output/powerpc64-linux-gnu/bitcoin-86712c313578-powerpc64-linux-gnu.tar.gz
4b3feaaed2fd14d67e5887a195badfebf8a6593ce7f8a2342e1d2f7a1c68d721  guix-build-86712c313578/output/powerpc64le-linux-gnu/SHA256SUMS.part
7dd7e448ae90b385ad5b2da4159a6e4d26fde5cf08538cc34f40d957c78bc2bb  guix-build-86712c313578/output/powerpc64le-linux-gnu/bitcoin-86712c313578-powerpc64le-linux-gnu-debug.tar.gz
c09d3836baca1588e991018b4ae5dcba3cfa7ecf6dfb72f202e0473dcc871e02  guix-build-86712c313578/output/powerpc64le-linux-gnu/bitcoin-86712c313578-powerpc64le-linux-gnu.tar.gz
3f807e417a77cc5be8343f1749df217b8a52ad470d9e3450d92c9e9a33a985b6  guix-build-86712c313578/output/riscv64-linux-gnu/SHA256SUMS.part
795712a2e5360acb9934de561bb5db160087b8457a2835e81f2cdb5020a90daa  guix-build-86712c313578/output/riscv64-linux-gnu/bitcoin-86712c313578-riscv64-linux-gnu-debug.tar.gz
f893f98a7adca6ef33acfd12f584993e92b13516f4bd3c80a815b9820b091fb7  guix-build-86712c313578/output/riscv64-linux-gnu/bitcoin-86712c313578-riscv64-linux-gnu.tar.gz
886e2d9c45f175cd03d6fb4c2adf6c4d12a692ee3dca9008d89413e8410ba536  guix-build-86712c313578/output/x86_64-apple-darwin/SHA256SUMS.part
c6db2e2a00823c3498857eb41845dcee1b9785e1957fab3d27e6825a8d1956a6  guix-build-86712c313578/output/x86_64-apple-darwin/bitcoin-86712c313578-x86_64-apple-darwin-unsigned.tar.gz
a8109a1c62f92cd90f1ded5103afd09fefc69399709ca1d876894006075b8bda  guix-build-86712c313578/output/x86_64-apple-darwin/bitcoin-86712c313578-x86_64-apple-darwin-unsigned.zip
77d38fb208ebd511af86385601c26c6feb8c33b92339bd6bcc202735502b4770  guix-build-86712c313578/output/x86_64-apple-darwin/bitcoin-86712c313578-x86_64-apple-darwin.tar.gz
fd5cd76982bb97d75a55aa9bac2a576b9381dab09416ee5b30556198c9177ce2  guix-build-86712c313578/output/x86_64-linux-gnu/SHA256SUMS.part
a028a47e0c75df4c392c140c5116ad1a2173db1e1b050eab195a9464e6162714  guix-build-86712c313578/output/x86_64-linux-gnu/bitcoin-86712c313578-x86_64-linux-gnu-debug.tar.gz
c5319b1b2d9d20838e0e7c7ff6682e549fd6c5b64f0ead7a51ca8db1433f6632  guix-build-86712c313578/output/x86_64-linux-gnu/bitcoin-86712c313578-x86_64-linux-gnu.tar.gz
2810be230ef3a97a7d9b7564790a96ab1a849bec6459cc8a06b3793d875ecd29  guix-build-86712c313578/output/x86_64-w64-mingw32/SHA256SUMS.part
b6fc5f7482500c121475c4dc34a2e599bf9391d6c0617f06dd96bfb20175768c  guix-build-86712c313578/output/x86_64-w64-mingw32/bitcoin-86712c313578-win64-debug.zip
19469f9da899670700788e38beccd8a675a2c44e30cbc91595ac684ff69e10b8  guix-build-86712c313578/output/x86_64-w64-mingw32/bitcoin-86712c313578-win64-setup-unsigned.exe
ee1c879b3f28628443109c9b255fd300480659044a34f3db6d5c72bbaa659061  guix-build-86712c313578/output/x86_64-w64-mingw32/bitcoin-86712c313578-win64-unsigned.tar.gz
e5e0c780627a54f6bee3877ceae1e60cc62af9a96dd337cd0707b34e790447e2  guix-build-86712c313578/output/x86_64-w64-mingw32/bitcoin-86712c313578-win64.zip

TheCharlatan

Re-ACK bbf218d

hebasto

re-ACK bbf218d

m3dwards · 2024-01-10T18:31:20Z

I've reviewed the changes and they appear correct to me. libbitcoinconsensus will continue to not get SSE4 ASM (nor any other optimisations) and libbitcoinkernel (external?) will now get all of them (if available and enabled by ./configure).

I think DISABLE_OPTIMIZED_SHA256 is much clearer macro name. I find the USE_ASM symbol / macro and asm feature in autoconf confusing.

If I've read it correctly, the ASM feature in autoconf enables not only the assembly optimisations but also starts a check to see if the compiler supports various other C++ intrinsics. A USE_ASM symbol is then set (as well as various other optimisation symbols) for use in src/Makefile.am. Inside Makefike.am the symbol USE_ASM is just used to enable the SSE4 ASM file. Finally the sha256.cpp file uses USE_ASM to gate intrinsics based optimisations as well as the asm optimisation.

Is there any mileage in extending this to do the following? :

Change the asm feature in configure.ac to optimized_sha256 with default of enabled
Change the symbol set in configure.ac and used in src/Makefile from USE_ASM to DISABLE_OPTIMIZED_SHA256 to match this macro.
Remove gates on USE_ASM from sha256.cpp as I think the current gates on DISABLE_OPTIMIZED_SHA256 and checking it's not MSVC compiler which I think defined(__x86_64__) || defined(__amd64__) || defined(__i386__) achieves.

Apologies if this is a bit out of scope for this PR. I'm also new to this code so may not have read things correctly.

25dc87e libconsensus: deprecate (Cory Fields) Pull request description: This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib). Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway. See for example the discussions: hebasto#41 #29123 And here: #29180 Where it is pointed out that the libbitcoinconsensus functions are slower than those the internal bitcoind equivalents due to the missing sha2 implementations. Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v27. Any remaining use-cases could be handled in the future by libbitcoinkernel. If there are any users currently using libbitcoinconsensus, please chime in with your use-case! Edit: Corrected final release to be v27. ACKs for top commit: TheCharlatan: ACK 25dc87e fanquake: ACK 25dc87e - this library has very little, if any impactful real world usage. It has been entirely broken (on various platforms) for long periods of its existence, where nobody even noticed. Pruning this out to save porting, and starting anew with the kernel, is the sane thing to do. Tree-SHA512: baff2b3c4f76f520c96021035f751fdcb51bedf00e767660249e92a7bc7c5c176786bcf2c4cfe2d2351c200f932b39eb886bcfb22fbec824a41617590d6a1638

DrahtBot added the Utils/log/libs label Jan 4, 2024

hebasto reviewed Jan 4, 2024

View reviewed changes

maflcko added the DrahtBot Guix build requested label Jan 4, 2024

theuni force-pushed the kernel-sha2-optims branch from 2fe00dc to dcfb9ee Compare January 4, 2024 17:38

hebasto reviewed Jan 4, 2024

View reviewed changes

src/crypto/sha256.cpp Outdated Show resolved Hide resolved

This was referenced Jan 4, 2024

Fix SSE4.1-related issues #28893

Open

Add 1-way SSE4 SHA256 implementation using intrinsics for MSVC builds #28526

Closed

Enable HW-accelerated implementations of SHA256 for MSVC builds #24773

Closed

DrahtBot removed the DrahtBot Guix build requested label Jan 5, 2024

hebasto reviewed Jan 5, 2024

View reviewed changes

theuni force-pushed the kernel-sha2-optims branch from 9235801 to 86712c3 Compare January 5, 2024 12:38

hebasto approved these changes Jan 5, 2024

View reviewed changes

src/Makefile.am Outdated Show resolved Hide resolved

crypto: remove sha256_sse4 from the base crypto helper lib

bbf218d

It was unused there and a confusing outlier.

theuni force-pushed the kernel-sha2-optims branch from 86712c3 to bbf218d Compare January 5, 2024 17:09

TheCharlatan approved these changes Jan 5, 2024

View reviewed changes

DrahtBot requested a review from hebasto January 5, 2024 17:10

hebasto approved these changes Jan 5, 2024

View reviewed changes

DrahtBot requested a review from hebasto January 5, 2024 17:11

theuni mentioned this pull request Jan 5, 2024

RFC: Deprecate libconsensus #29189

Merged

DrahtBot removed the request for review from hebasto January 5, 2024 17:22

DrahtBot added the CI failed label Jan 15, 2024

achow101 merged commit 5fbcc8f into bitcoin:master Jan 26, 2024
15 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 #29180

crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 #29180

theuni commented Jan 4, 2024 •

edited by fanquake

DrahtBot commented Jan 4, 2024 •

edited

theuni commented Jan 4, 2024

hebasto Jan 4, 2024

theuni Jan 4, 2024

theuni Jan 4, 2024

hebasto commented Jan 4, 2024

hebasto left a comment

DrahtBot commented Jan 5, 2024

TheCharlatan commented Jan 5, 2024

theuni commented Jan 5, 2024 •

edited

hebasto left a comment

theuni commented Jan 5, 2024

hebasto left a comment

TheCharlatan left a comment

TheCharlatan left a comment

hebasto left a comment •

edited

m3dwards commented Jan 10, 2024 •

edited

	if (have_sse4) {
	#if defined(__x86_64__) \|\| defined(__amd64__)
	Transform = sha256_sse4::Transform;
	TransformD64 = TransformD64Wrapper<sha256_sse4::Transform>;
	ret = "sse4(1way)";
	#endif

crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 #29180

crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 #29180

Conversation

theuni commented Jan 4, 2024 • edited by fanquake

DrahtBot commented Jan 4, 2024 • edited

Code Coverage

Reviews

Conflicts

theuni commented Jan 4, 2024

hebasto Jan 4, 2024

Choose a reason for hiding this comment

theuni Jan 4, 2024

Choose a reason for hiding this comment

theuni Jan 4, 2024

Choose a reason for hiding this comment

hebasto commented Jan 4, 2024

hebasto left a comment

Choose a reason for hiding this comment

DrahtBot commented Jan 5, 2024

Guix builds (on x86_64)

TheCharlatan commented Jan 5, 2024

theuni commented Jan 5, 2024 • edited

hebasto left a comment

Choose a reason for hiding this comment

theuni commented Jan 5, 2024

hebasto left a comment

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

hebasto left a comment • edited

Choose a reason for hiding this comment

m3dwards commented Jan 10, 2024 • edited

theuni commented Jan 4, 2024 •

edited by fanquake

DrahtBot commented Jan 4, 2024 •

edited

theuni commented Jan 5, 2024 •

edited

hebasto left a comment •

edited

m3dwards commented Jan 10, 2024 •

edited