From 5446a74f084fdbd8c924a35bb741291db79877a3 Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Fri, 14 Oct 2022 12:58:36 +0200 Subject: [PATCH 1/4] build: explicitly enable experimental functions Introduces a configure flag --enable-experimental that controls at configure time whether or not experimental features can be enabled. This serves as a circuit breaker to both make sure that CI jobs are configured properly, and ensures manual compilations are intentionally configuring experimental / non-production code. Additionally, experimental features get listed in the summary after configuration completes if enabled. Further work can insert compile time checks with static_asserts against the ALLOW_DOGECOIN_EXPERIMENTAL macro. --- build-aux/m4/dogecoin_experimental.m4 | 11 +++++++++++ configure.ac | 23 +++++++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 build-aux/m4/dogecoin_experimental.m4 diff --git a/build-aux/m4/dogecoin_experimental.m4 b/build-aux/m4/dogecoin_experimental.m4 new file mode 100644 index 000000000..f301d03a9 --- /dev/null +++ b/build-aux/m4/dogecoin_experimental.m4 @@ -0,0 +1,11 @@ +dnl Copyright (c) 2022 The Dogecoin Core developers +dnl Distributed under the MIT software license, see the accompanying +dnl file COPYING or http://www.opensource.org/licenses/mit-license.php. + +dnl Helper function to make experimental flag checking easy +dnl experimental functions simply call this macro inside their checks +AC_DEFUN([DOGECOIN_REQUIRE_EXPERIMENTAL],[ + if test x$allow_experimental != xyes; then + AC_MSG_ERROR([Experimental features need to be enabled explicitly. Use --enable-experimental.]) + fi +]) diff --git a/configure.ac b/configure.ac index 3a0f925a7..5426db0ee 100644 --- a/configure.ac +++ b/configure.ac @@ -177,6 +177,12 @@ AC_ARG_ENABLE([zmq], [use_zmq=$enableval], [use_zmq=yes]) +AC_ARG_ENABLE([experimental], + [AS_HELP_STRING([--enable-experimental], + [Allow experimental features to be configured (default is no)])], + [allow_experimental=$enableval], + [allow_experimental=no]) + AC_ARG_ENABLE([scrypt-sse2], [AS_HELP_STRING([--enable-scrypt-sse2], [Build with scrypt sse2 implementation (default is no)])], @@ -731,12 +737,19 @@ BOOST_LIBS="$BOOST_LDFLAGS $BOOST_SYSTEM_LIB $BOOST_FILESYSTEM_LIB $BOOST_PROGRA fi +# Configure experimental for compile-time asserts +if test x$allow_experimental = xyes; then + AC_DEFINE(ALLOW_DOGECOIN_EXPERIMENTAL, 1, [Define this symbol if experimental features are allowed]) +fi + # Configure Scrypt SSE2 if test x$use_scrypt_sse2 = xyes; then + DOGECOIN_REQUIRE_EXPERIMENTAL AC_DEFINE(USE_SSE2, 1, [Define this symbol if SSE2 works]) fi if test x$armv8_crypto = xyes; then + DOGECOIN_REQUIRE_EXPERIMENTAL AC_MSG_CHECKING([whether to build with armv8 crypto]) AC_MSG_RESULT(yes) AC_DEFINE(USE_ARMV8, 1, [Define this symbol if armv8 crypto works]) @@ -744,6 +757,7 @@ if test x$armv8_crypto = xyes; then fi if test x$armv82_crypto = xyes; then + DOGECOIN_REQUIRE_EXPERIMENTAL AC_CHECK_DECLS([vsha512su0q_u64], [AC_DEFINE(USE_ARMV82, 1, [Define this symbol if armv8.2 crypto works]) CXXFLAGS="$CXXFLAGS -march=armv8.2-a+crypto+sha3"], AC_MSG_ERROR(sha512 missing), [#include ]) @@ -824,6 +838,7 @@ else fi if test x$intel_avx2 = xyes; then + DOGECOIN_REQUIRE_EXPERIMENTAL case $host in x86_64-*-linux*) AC_CHECK_LIB([IPSec_MB],[sha1_one_block_avx2],LIBS=-lIPSec_MB, AC_MSG_ERROR(IPSec_MB missing)) @@ -1188,6 +1203,14 @@ echo " with upnp = $use_upnp" echo " debug enabled = $enable_debug" echo " werror = $enable_werror" echo +echo " experimental = $allow_experimental" +if test x$allow_experimental = xyes; then + echo " SSE2 Scrypt = $use_scrypt_sse2" + echo " AVX2 crypto = $intel_avx2" + echo " ARMv8 crypto = $armv8_crypto" + echo " ARMv82 crypto = $armv82_crypto" +fi +echo echo " target os = $TARGET_OS" echo " build os = $BUILD_OS" echo From 2c46336ce6854074c02955e386c7ae62544f80f7 Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Fri, 14 Oct 2022 13:29:01 +0200 Subject: [PATCH 2/4] ci: configure experimental jobs explicitly --- .github/workflows/ci.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a95aad5e5..2d86789e3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -78,7 +78,7 @@ jobs: check-security: true check-symbols: false dep-opts: "NO_QT=1" - config-opts: "--with-armv8-crypto --enable-zmq --enable-glibc-back-compat LDFLAGS=-static-libstdc++" + config-opts: "--enable-experimental --with-armv8-crypto --enable-zmq --enable-glibc-back-compat LDFLAGS=-static-libstdc++" goal: install - name: aarch64-linux-sha512-experimental host: aarch64-linux-gnu @@ -90,7 +90,7 @@ jobs: check-security: true check-symbols: false dep-opts: "NO_QT=1" - config-opts: "--with-armv82-crypto --enable-zmq --enable-glibc-back-compat LDFLAGS=-static-libstdc++" + config-opts: "--enable-experimental --with-armv82-crypto --enable-zmq --enable-glibc-back-compat LDFLAGS=-static-libstdc++" goal: install - name: aarch64-linux host: aarch64-linux-gnu @@ -182,7 +182,7 @@ jobs: check-security: true check-symbols: false dep-opts: "AVX2=1 MINGW=1" - config-opts: "--enable-scrypt-sse2 --with-intel-avx2 --with-gui=qt5" + config-opts: "--enable-experimental --enable-scrypt-sse2 --with-intel-avx2 --with-gui=qt5" goal: install - name: x86_64-macos host: x86_64-apple-darwin11 @@ -206,7 +206,7 @@ jobs: qa/pull-tester/install-deps.sh qa/pull-tester/rpc-tests.py --coverage dep-opts: "AVX2=1" - config-opts: "--enable-scrypt-sse2 --with-intel-avx2 --with-gui=qt5 --enable-zmq --enable-glibc-back-compat --enable-reduce-exports" + config-opts: "--enable-experimental --enable-scrypt-sse2 --with-intel-avx2 --with-gui=qt5 --enable-zmq --enable-glibc-back-compat --enable-reduce-exports" goal: install runs-on: ${{ matrix.os }} From 330cea911f1020972113c8c3ad6a138282cd6b17 Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Sat, 15 Oct 2022 10:56:55 +0200 Subject: [PATCH 3/4] enforce explicit experimental configuration through annotation Introduces a new support header that exposes the macro EXPERIMENTAL_FEATURE that (a) allows us to clearly mark blocks of code as experimental (or entire files) and (b) enforces that none of the annotated code gets compiled if --enable-experimental was not configured through a static assertion. Existing experimental features are annotated: - AVX2 using the intel-ipsec-mb dependency for SHA1/256/512 - ARMv8 intrinsics for SHA1/256 - ARMv82 intrinsics for SHA512 - SSE2 for scrypt --- src/Makefile.am | 1 + src/crypto/scrypt-sse2.cpp | 4 ++++ src/crypto/sha1.cpp | 5 +++++ src/crypto/sha256.cpp | 5 +++++ src/crypto/sha512.cpp | 4 ++++ src/support/experimental.h | 22 ++++++++++++++++++++++ 6 files changed, 41 insertions(+) create mode 100644 src/support/experimental.h diff --git a/src/Makefile.am b/src/Makefile.am index 1574df227..1809a8626 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -143,6 +143,7 @@ BITCOIN_CORE_H = \ support/allocators/zeroafterfree.h \ support/cleanse.h \ support/events.h \ + support/experimental.h \ support/lockedpool.h \ sync.h \ threadsafety.h \ diff --git a/src/crypto/scrypt-sse2.cpp b/src/crypto/scrypt-sse2.cpp index 3a951ba85..7241abe9e 100644 --- a/src/crypto/scrypt-sse2.cpp +++ b/src/crypto/scrypt-sse2.cpp @@ -28,12 +28,16 @@ */ #include "crypto/scrypt.h" +#include "support/experimental.h" #include #include #include #include +// this entire functionality is experimental +EXPERIMENTAL_FEATURE + static inline void xor_salsa8_sse2(__m128i B[4], const __m128i Bx[4]) { __m128i X0, X1, X2, X3; diff --git a/src/crypto/sha1.cpp b/src/crypto/sha1.cpp index fcd510e40..d928d5814 100644 --- a/src/crypto/sha1.cpp +++ b/src/crypto/sha1.cpp @@ -6,6 +6,7 @@ #include "crypto/sha1.h" #include "crypto/common.h" +#include "support/experimental.h" #include @@ -71,6 +72,9 @@ const uint32_t k4 = 0xCA62C1D6ul; void Transform(uint32_t* s, const unsigned char* chunk) { #if defined(USE_ARMV8) || defined(USE_ARMV82) + // this entire block is experimental + EXPERIMENTAL_FEATURE + uint32x4_t ABCD, ABCD_SAVED; uint32x4_t TMP0, TMP1; uint32x4_t MSG0, MSG1, MSG2, MSG3; @@ -241,6 +245,7 @@ void Transform(uint32_t* s, const unsigned char* chunk) #elif USE_AVX2 // Perform SHA1 one block (Intel AVX2) + EXPERIMENTAL_FEATURE sha1_one_block_avx2(chunk, s); #else // Perform SHA one block (legacy) diff --git a/src/crypto/sha256.cpp b/src/crypto/sha256.cpp index bd61c3858..f943da940 100644 --- a/src/crypto/sha256.cpp +++ b/src/crypto/sha256.cpp @@ -6,6 +6,7 @@ #include "crypto/sha256.h" #include "crypto/common.h" +#include "support/experimental.h" #include @@ -91,6 +92,9 @@ void inline Initialize(uint32_t* s) void Transform(uint32_t* s, const unsigned char* chunk) { #if defined(USE_ARMV8) || defined(USE_ARMV82) + // entire block is experimental + EXPERIMENTAL_FEATURE + uint32x4_t STATE0, STATE1, ABEF_SAVE, CDGH_SAVE; uint32x4_t MSG0, MSG1, MSG2, MSG3; uint32x4_t TMP0, TMP1, TMP2; @@ -247,6 +251,7 @@ void Transform(uint32_t* s, const unsigned char* chunk) #elif USE_AVX2 // Perform SHA256 one block (Intel AVX2) + EXPERIMENTAL_FEATURE sha256_one_block_avx2(chunk, s); #else // Perform SHA256 one block (legacy) diff --git a/src/crypto/sha512.cpp b/src/crypto/sha512.cpp index c911ed312..91842e0e6 100644 --- a/src/crypto/sha512.cpp +++ b/src/crypto/sha512.cpp @@ -6,6 +6,7 @@ #include "crypto/sha512.h" #include "crypto/common.h" +#include "support/experimental.h" #include @@ -100,6 +101,8 @@ void inline Round(uint64_t a, uint64_t b, uint64_t c, uint64_t& d, uint64_t e, u #ifdef USE_ARMV82 +EXPERIMENTAL_FEATURE + /* ---------------------------------------------------------------------- * Hardware-accelerated implementation of SHA-512 using Arm NEON. */ @@ -311,6 +314,7 @@ void Transform(uint64_t* s, const unsigned char* chunk) { #ifdef USE_AVX2 // Perform SHA512 one block (Intel AVX2) + EXPERIMENTAL_FEATURE sha512_one_block_avx2(chunk, s); #elif USE_ARMV82 sha512_neon_core core; diff --git a/src/support/experimental.h b/src/support/experimental.h new file mode 100644 index 000000000..b7201691a --- /dev/null +++ b/src/support/experimental.h @@ -0,0 +1,22 @@ +// Copyright (c) 2022 The Dogecoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef DOGECOIN_SUPPORT_EXPERIMENTAL_H +#define DOGECOIN_SUPPORT_EXPERIMENTAL_H + +// include config for experimental flag +#if defined(HAVE_CONFIG_H) +#include "config/bitcoin-config.h" +#endif //HAVE_CONFIG_H + +#if defined(ALLOW_DOGECOIN_EXPERIMENTAL) +#define EXPERIMENTAL_FEATURES_ALLOWED 1 +#else +#define EXPERIMENTAL_FEATURES_ALLOWED 0 +#endif // ALLOW_DOGECOIN_EXPERIMENTAL + +#define EXPERIMENTAL_FEATURE static_assert(EXPERIMENTAL_FEATURES_ALLOWED == 1, \ + "Experimental features need to be explicitly enabled during configuration."); + +#endif //BITCOIN_SUPPORT_EXPERIMENTAL_H From f23050b1f1d39ba837bfb0292f28734b1f0759c8 Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Tue, 18 Oct 2022 15:26:49 +0200 Subject: [PATCH 4/4] doc: experimental features introduces a new document doc/experimental.md and adapts the contribution guidelines to this new type of feature --- CONTRIBUTING.md | 14 ++++++++++++ doc/experiments.md | 57 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 doc/experiments.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index db9278df1..c07b93858 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -111,6 +111,19 @@ releases will automatically benefit from these. When refactoring Dogecoin-specific code, please keep refactoring requests short, low complexity and easy to verify. +### Experimental features and optimizations + +In some cases where a pull request introduces a new feature or optimization, +reviewers or maintainers can request the feature to be introduced as an +experimental-only feature, meaning that the feature will not be released in +provided binaries but will be available for self-compilation for those who +wish to test it. Experimental features are still expected to be complete and +the process to be followed for all contribution guidelines as outlined in this +document. + +For more information, see the +[experimental feature documentation](./doc/experiments.md) + ## "Decision Making" Process @@ -144,6 +157,7 @@ approval and merge: - Consensus rule changes (through softfork or otherwise) - Policy changes +- Maturing experimental features into production While each case will be different, one should be prepared to expend more time and effort than for other kinds of patches because of increased peer review diff --git a/doc/experiments.md b/doc/experiments.md new file mode 100644 index 000000000..780b9aa32 --- /dev/null +++ b/doc/experiments.md @@ -0,0 +1,57 @@ +Experimental features +---------------------- + +Features can be marked as experimental when the functionality is desired but +further analysis, testing or follow-up work is needed to make sure that the +feature is fully ready for rollout to every node in the network. This can +specifically help the introduction of performance updates or other lower-level +improvements where positive and/or negative effects may take a longer time to +test. The PR benefits from being merged because this makes it easier for +testers to pick and choose sets of features to experiment with in their custom +built Dogecoin Core deployments. + +## Enabling experimental features + +Experiments can be enabled by passing `--enable-experimental` AND the desired +experimental feature to `./configure`: + +### Current experiments + +| Feature | Configure flag | Description +| :---------- | :--------------------- | :----- +| Scrypt SSE2 | `--enable-scrypt-sse2` | SSE2 asm for Scrypt functions +| SHA ARMv8 | `--with-armv8-crypto` | SHA1/256 intrinsics for ARMv8-crypto capable processors +| SHA ARMv82 | `--with-armv82-crypto` | SHA512 intrinsics for ARMv8.2-crypto capable processors +| SHA AVX2 | `--with-intel-avx2` | SHA1/256/512 intrinsics using Intel AVX2 extensions, depends on intel-ipsec-mb + +## Requirements + +1. An experimental feature shall be controlled by a configuration flag inside + `configure.ac` that explicitly enables the feature. +2. The feature shall by default be disabled. +3. The feature shall call the `DOGECOIN_REQUIRE_EXPERIMENTAL` macro in + `configure.ac`, to enforce that `--enable-experimental` was passed. +4. All code blocks related to the feature shall be guarded by a preprocessor + check on the configuration flag, to prevent leaking code into releases. +5. Inside each experimental code block, a call shall be made to the + `DOGECOIN_REQUIRE_EXPERIMENTAL` macro from `support/experimental.h`, to + clearly mark the code as experimental and ease review and troubleshooting. +6. Only when an experimental feature matures into production shall the above + requirements be voided. + +## Lifecycle + +Experimental features move through different stages. By default, each experiment +is "maturing", which means no decision is made whether the feature will be +included in a release, and the feature does not have to be ready for inclusion +at that time. This gives interested parties time to test, suggest changes and +form an opinion about the benefits of the feature without this blocking release. + +Once there is consensus that a feature is beneficial, it needs to be prepared +for generic inclusion. This means that while it retains experimental status, +the feature is tested to not break any builds on known platforms, after which +it can be matured into production code in a separate PR. + +It is also possible that an experiment is abandoned instead of included in a +release, for example when it lacks a maintainer or when another implementation +is proposed for the same functionality that is more desirable.