crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256

theuni commented at 5:17 pm on January 4, 2024: member

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 commented at 5:17 pm on January 4, 2024: contributor

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.

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

theuni commented at 5:18 pm on January 4, 2024: member

Ping @TheCharlatan @hebasto

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

in src/Makefile.am:1026 in 2fe00dc13d outdated

1011@@ -1012,7 +1012,7 @@ libbitcoinconsensus_la_SOURCES = support/cleanse.cpp $(crypto_libbitcoin_crypto_
1012 
1013 libbitcoinconsensus_la_LDFLAGS = $(AM_LDFLAGS) -no-undefined $(RELDFLAGS)
1014 libbitcoinconsensus_la_LIBADD = $(LIBSECP256K1)
1015-libbitcoinconsensus_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(builddir)/obj -I$(srcdir)/secp256k1/include -DBUILD_BITCOIN_INTERNAL
1016+libbitcoinconsensus_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(builddir)/obj -I$(srcdir)/secp256k1/include -DBUILD_BITCOIN_INTERNAL -DDISABLE_OPTIMIZED_SHA256

hebasto commented at 5:19 pm on January 4, 2024:

Is it needed to keep -DBUILD_BITCOIN_INTERNAL now?

theuni commented at 5:20 pm on January 4, 2024:

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

theuni commented at 5:43 pm on January 4, 2024:

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

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

theuni force-pushed on Jan 4, 2024

hebasto commented at 8:06 pm on January 4, 2024: member

Concept ACK.

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

in src/crypto/sha256.cpp:582 in dcfb9ee9f6 outdated

578@@ -576,6 +579,7 @@ bool AVXEnabled()
579     return (a & 6) == 6;
580 }
581 #endif
582+#endif

hebasto commented at 8:19 pm on January 4, 2024:

nit:

0#endif // DISABLE_OPTIMIZED_SHA256

hebasto commented at 8:20 pm on January 4, 2024: member

In the master branch, the following code: https://github.com/bitcoin/bitcoin/blob/dcfb9ee9f6c8ce1f8c1e5dffdb207e55098ee9c5/src/crypto/sha256.cpp#L635-L640 compiles regardless of the BUILD_BITCOIN_INTERNAL macro. Now it is gated with the DISABLE_OPTIMIZED_SHA256 one. Is it intentional?

DrahtBot commented at 2:51 am on January 5, 2024: contributor

Guix builds (on x86_64)

File	commit 737e5884cc82dc352cef3ef26abc1cb8d3500b8b(master)	commit 15b2094767839031dbd089a8b0e1443411aa5a91(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...`

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

TheCharlatan commented at 11:27 am on January 5, 2024: contributor

Re comment #29180#pullrequestreview-1804912723

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 commented at 11:27 am on January 5, 2024: member

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 commented at 11:59 am on January 5, 2024: member

Approach ACK 9235801256a63a9ca4f8c95a005b5151afa9a5d4.

crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256

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.

4dbd0475d8

theuni force-pushed on Jan 5, 2024

theuni commented at 12:40 pm on January 5, 2024: member

Added a brief description of crypto_base.

in src/Makefile.am:549 in 86712c3135 outdated

544@@ -541,6 +545,10 @@ libbitcoin_wallet_tool_a_SOURCES = \
545 #
546 
547 # crypto #
548+
549+# crypto_base contains the unspecialized (unoptomized) versions of our

hebasto commented at 3:36 pm on January 5, 2024:

typo: unoptomized –> unoptimized

theuni commented at 5:10 pm on January 5, 2024:

😳 Whoops!

Fixed, thanks :)

hebasto approved

hebasto commented at 3:36 pm on January 5, 2024: member

ACK 86712c3135786b305f27c44dffd0808be0ee7170.

crypto: remove sha256_sse4 from the base crypto helper lib

It was unused there and a confusing outlier.

bbf218d061

theuni force-pushed on Jan 5, 2024

TheCharlatan approved

TheCharlatan commented at 5:09 pm on January 5, 2024: contributor

ACK 86712c3135786b305f27c44dffd0808be0ee7170

Guix builds (aarch64):

 0e00ac9d2dbf9d1b6a0255d7c6669ee3925696109cf4ca549dbf411da235f52bc  guix-build-86712c313578/output/aarch64-linux-gnu/SHA256SUMS.part
 1871f7e66c454ac08d4e04f0d86d23e2b34a6c3a6a3b60aabcb9485c086cdad37  guix-build-86712c313578/output/aarch64-linux-gnu/bitcoin-86712c313578-aarch64-linux-gnu-debug.tar.gz
 2f0553ca6ebaaf5eb8d00196a681a20c21d4b81cb3ec907cce01ac6a446de02e7  guix-build-86712c313578/output/aarch64-linux-gnu/bitcoin-86712c313578-aarch64-linux-gnu.tar.gz
 3476395fad048ca38b226d3c108b18262127c9c0731afb1c8048ff5e4643039f8  guix-build-86712c313578/output/arm-linux-gnueabihf/SHA256SUMS.part
 47de1c5c3685ca1d81a78d4b0a3e62b716e65341bbfdf4a0c56d154889c08ef26  guix-build-86712c313578/output/arm-linux-gnueabihf/bitcoin-86712c313578-arm-linux-gnueabihf-debug.tar.gz
 59558f4afa8c466324cfbd046731e0915fc1041aa250d05ecd7bf74a41594b928  guix-build-86712c313578/output/arm-linux-gnueabihf/bitcoin-86712c313578-arm-linux-gnueabihf.tar.gz
 6f008cb1a5ef888be0a82dcffe6d58bb8379d429448e5c4de1cef208533847b69  guix-build-86712c313578/output/arm64-apple-darwin/SHA256SUMS.part
 7acd7300c467e06ebfc3b632a61fe2f5b1ad8d35d0181d18107bc24b7a66e0947  guix-build-86712c313578/output/arm64-apple-darwin/bitcoin-86712c313578-arm64-apple-darwin-unsigned.tar.gz
 8e36e3ff8d05dd6e53b9e2ec60b42e2fa0f14933c571eda8117c93656249590c3  guix-build-86712c313578/output/arm64-apple-darwin/bitcoin-86712c313578-arm64-apple-darwin-unsigned.zip
 94a923b366e185a1eb4065c3ae6175bc7459cb15177b6cc036b48b63b94214846  guix-build-86712c313578/output/arm64-apple-darwin/bitcoin-86712c313578-arm64-apple-darwin.tar.gz
100d09788c222ca4d25fb08c504e1b8b472f15b476c22be27470e33dc963e1433b  guix-build-86712c313578/output/dist-archive/bitcoin-86712c313578.tar.gz
1163ba5f9d412197d0a5ce83ce4eb2627a7fb571199fda97ce393fb66d5537e121  guix-build-86712c313578/output/powerpc64-linux-gnu/SHA256SUMS.part
12c9a5dc8214e196d3ae211f0c30ed4ed93b19926eebc9fff35c1eee92011141a1  guix-build-86712c313578/output/powerpc64-linux-gnu/bitcoin-86712c313578-powerpc64-linux-gnu-debug.tar.gz
13681549c7fe284c4bc03257ce0030482e95da6514a4e8e4a75bb19381f3994fa6  guix-build-86712c313578/output/powerpc64-linux-gnu/bitcoin-86712c313578-powerpc64-linux-gnu.tar.gz
144b3feaaed2fd14d67e5887a195badfebf8a6593ce7f8a2342e1d2f7a1c68d721  guix-build-86712c313578/output/powerpc64le-linux-gnu/SHA256SUMS.part
157dd7e448ae90b385ad5b2da4159a6e4d26fde5cf08538cc34f40d957c78bc2bb  guix-build-86712c313578/output/powerpc64le-linux-gnu/bitcoin-86712c313578-powerpc64le-linux-gnu-debug.tar.gz
16c09d3836baca1588e991018b4ae5dcba3cfa7ecf6dfb72f202e0473dcc871e02  guix-build-86712c313578/output/powerpc64le-linux-gnu/bitcoin-86712c313578-powerpc64le-linux-gnu.tar.gz
173f807e417a77cc5be8343f1749df217b8a52ad470d9e3450d92c9e9a33a985b6  guix-build-86712c313578/output/riscv64-linux-gnu/SHA256SUMS.part
18795712a2e5360acb9934de561bb5db160087b8457a2835e81f2cdb5020a90daa  guix-build-86712c313578/output/riscv64-linux-gnu/bitcoin-86712c313578-riscv64-linux-gnu-debug.tar.gz
19f893f98a7adca6ef33acfd12f584993e92b13516f4bd3c80a815b9820b091fb7  guix-build-86712c313578/output/riscv64-linux-gnu/bitcoin-86712c313578-riscv64-linux-gnu.tar.gz
20886e2d9c45f175cd03d6fb4c2adf6c4d12a692ee3dca9008d89413e8410ba536  guix-build-86712c313578/output/x86_64-apple-darwin/SHA256SUMS.part
21c6db2e2a00823c3498857eb41845dcee1b9785e1957fab3d27e6825a8d1956a6  guix-build-86712c313578/output/x86_64-apple-darwin/bitcoin-86712c313578-x86_64-apple-darwin-unsigned.tar.gz
22a8109a1c62f92cd90f1ded5103afd09fefc69399709ca1d876894006075b8bda  guix-build-86712c313578/output/x86_64-apple-darwin/bitcoin-86712c313578-x86_64-apple-darwin-unsigned.zip
2377d38fb208ebd511af86385601c26c6feb8c33b92339bd6bcc202735502b4770  guix-build-86712c313578/output/x86_64-apple-darwin/bitcoin-86712c313578-x86_64-apple-darwin.tar.gz
24fd5cd76982bb97d75a55aa9bac2a576b9381dab09416ee5b30556198c9177ce2  guix-build-86712c313578/output/x86_64-linux-gnu/SHA256SUMS.part
25a028a47e0c75df4c392c140c5116ad1a2173db1e1b050eab195a9464e6162714  guix-build-86712c313578/output/x86_64-linux-gnu/bitcoin-86712c313578-x86_64-linux-gnu-debug.tar.gz
26c5319b1b2d9d20838e0e7c7ff6682e549fd6c5b64f0ead7a51ca8db1433f6632  guix-build-86712c313578/output/x86_64-linux-gnu/bitcoin-86712c313578-x86_64-linux-gnu.tar.gz
272810be230ef3a97a7d9b7564790a96ab1a849bec6459cc8a06b3793d875ecd29  guix-build-86712c313578/output/x86_64-w64-mingw32/SHA256SUMS.part
28b6fc5f7482500c121475c4dc34a2e599bf9391d6c0617f06dd96bfb20175768c  guix-build-86712c313578/output/x86_64-w64-mingw32/bitcoin-86712c313578-win64-debug.zip
2919469f9da899670700788e38beccd8a675a2c44e30cbc91595ac684ff69e10b8  guix-build-86712c313578/output/x86_64-w64-mingw32/bitcoin-86712c313578-win64-setup-unsigned.exe
30ee1c879b3f28628443109c9b255fd300480659044a34f3db6d5c72bbaa659061  guix-build-86712c313578/output/x86_64-w64-mingw32/bitcoin-86712c313578-win64-unsigned.tar.gz
31e5e0c780627a54f6bee3877ceae1e60cc62af9a96dd337cd0707b34e790447e2  guix-build-86712c313578/output/x86_64-w64-mingw32/bitcoin-86712c313578-win64.zip

TheCharlatan approved

TheCharlatan commented at 5:10 pm on January 5, 2024: contributor

Re-ACK bbf218d06164b7247f5e9df5ba143383022fbf74

DrahtBot requested review from hebasto on Jan 5, 2024

hebasto approved

hebasto commented at 5:11 pm on January 5, 2024: member

re-ACK bbf218d06164b7247f5e9df5ba143383022fbf74

DrahtBot requested review from hebasto on Jan 5, 2024

DrahtBot removed review request from hebasto on Jan 5, 2024

m3dwards commented at 6:31 pm on January 10, 2024: none

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.

DrahtBot added the label CI failed on Jan 15, 2024

achow101 merged this on Jan 26, 2024

achow101 closed this on Jan 26, 2024

crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 #29180

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64)