Merge pull request #3136 from patricklodder/1.14.7-enforce-experimental

distinguish experimental features and enforce explicit configuration of these

.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 }}

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

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
+])

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 <arm_neon.h>])
@@ -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

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.

src/Makefile.am

@@ -144,6 +144,7 @@ BITCOIN_CORE_H = \
   support/allocators/zeroafterfree.h \
   support/cleanse.h \
   support/events.h \
+  support/experimental.h \
   support/lockedpool.h \
   sync.h \
   threadsafety.h \

src/crypto/scrypt-sse2.cpp

@@ -28,12 +28,16 @@
  */
 
 #include "crypto/scrypt.h"
+#include "support/experimental.h"
 #include <stdlib.h>
 #include <stdint.h>
 #include <string.h>
 
 #include <emmintrin.h>
 
+// 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;

src/crypto/sha1.cpp

@@ -6,6 +6,7 @@
 #include "crypto/sha1.h"
 
 #include "crypto/common.h"
+#include "support/experimental.h"
 
 #include <string.h>
 
@@ -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)

src/crypto/sha256.cpp

@@ -6,6 +6,7 @@
 #include "crypto/sha256.h"
 
 #include "crypto/common.h"
+#include "support/experimental.h"
 
 #include <string.h>
 
@@ -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)

src/crypto/sha512.cpp

@@ -6,6 +6,7 @@
 #include "crypto/sha512.h"
 
 #include "crypto/common.h"
+#include "support/experimental.h"
 
 #include <string.h>
 
@@ -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;

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