vDeployments in consensus/params.h so that undefined behaviour is avoided if a deployment is not initialized. Also adds a check in the unit tests to alert if this is happening, since even if it doesn’t invoke undefined behaviour it’s probably a mistake.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
5@@ -6,7 +6,7 @@
6
7 #include <consensus/params.h>
8
9-const struct VBDeploymentInfo VersionBitsDeploymentInfo[Consensus::MAX_VERSION_BITS_DEPLOYMENTS] = {
10+const struct std::array<VBDeploymentInfo, Consensus::MAX_VERSION_BITS_DEPLOYMENTS> VersionBitsDeploymentInfo{{
0C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\deploymentinfo.cpp(9,109): error C2220: the following warning is treated as an error [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
1C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\deploymentinfo.cpp(9,109): warning C4099: 'std::array<VBDeploymentInfo,2>': type name first seen using 'class' now seen using 'struct' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
2C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\deploymentinfo.h(20): message : see declaration of 'std::array<VBDeploymentInfo,2>' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
-Wmismatched-tags. Anyway, fixed.
18+ * bool _f(int x, int y) { ... }
19+ *
20+ * Then writing:
21+ *
22+ * static constrexpr NamedArgument<int, struct x_arg> _x;
23+ * static constrexpr NamedArgument<int, struct y_arg> _y;
constrexpr has too many r’s.
33+{
34+ struct Arg {
35+ using Tag = TAG;
36+ const T v;
37+ Arg(T&& v) : v{std::move(v)} { }
38+ Arg(const T& v) : v{v} { }
explicit otherwise you can call as f(3, 4) instead of f(_x=3, _y=4).
60+{
61+ static constexpr size_t offset = O;
62+ const T t;
63+
64+ template<typename... I>
65+ ArrayInitElement(I... i) : t{i...} { };
ArrayInitElement(I&&... i) : t{std::forward<I>(i)...} { }. Also, no need for a semicolon.
80+ template<size_t E, size_t O, typename... I>
81+ static inline void step(Arr& a, const ArrayInitElement<O, T>& aie, const I&... i)
82+ {
83+ static_assert(E < N, "too many initializers for array");
84+ static_assert(E == O, "array initializer element is out of order");
85+ if constexpr (E < N) {
if constexpr is only here to avoid redundant warnings that std::get<N> and step<N+1> would generate.
63+static constexpr util::NamedArgument<int64_t, struct start_time_arg> _start_time;
64+static constexpr util::NamedArgument<int64_t, struct timeout_arg> _timeout;
65+static constexpr util::NamedArgument<int, struct minactheight_arg> _minactheight;
66+
67+template<size_t O>
68+static inline util::ArrayInitElement<O, Consensus::BIP9Deployment> Dep(decltype(_bit)::Arg bit, decltype(_start_time)::Arg start_time, decltype(_timeout)::Arg timeout, decltype(_minactheight)::Arg minactheight)
I don’t really like the name ‘Dep’. Giving this a more descriptive name would, imo, be beneficial
also, this may be more readable with trailing return ie
0static inline auto Dep(decltype(_bit)::Arg bit, decltype(_start_time)::Arg start_time, decltype(_timeout)::Arg timeout, decltype(_minactheight)::Arg minactheight) -> util::ArrayInitElement<O, Consensus::BIP9Deployment>
DepSettings
62+static constexpr util::NamedArgument<int, struct bit_arg> _bit;
63+static constexpr util::NamedArgument<int64_t, struct start_time_arg> _start_time;
64+static constexpr util::NamedArgument<int64_t, struct timeout_arg> _timeout;
65+static constexpr util::NamedArgument<int, struct minactheight_arg> _minactheight;
66+
67+template<size_t O>
O? I confused it for 0 when first reading
Offset; also, I renamed to Init
70+ Consensus::BIP9Deployment dep;
71+ dep.bit = bit.v;
72+ dep.nStartTime = start_time.v;
73+ dep.nTimeout = timeout.v;
74+ dep.min_activation_height = minactheight.v;
75+ return util::ArrayInitElement<O, Consensus::BIP9Deployment>{dep};
dep.bit = bit.v etc seems a bit more obvious that everythings in the right order than BIP9Deployment(bit.v, start_time.v, timeout.v, minactheight.v) does. (per godbolt, VS seems to generate some moderately weird assembly, but changing how the temporary BIP9Deployment struct is created initialised doesn’t seem to improve things)
Concept ACK
is there other ongoing work on deployment tech that is motivating this change? Or just good maitenance.
is there other ongoing work on deployment tech that is motivating this change? Or just good maitenance.
There were a bunch of clean ups that got dropped to get speedy trial done speedily; seems a good time to reconsider them now that versionbits logic isn’t actively being used, and before we update to whatever might come after speedy trial. This seemed a good one to start with since it’s caused bugs twice now.
193@@ -194,15 +194,15 @@ class VersionBitsConditionChecker : public AbstractThresholdConditionChecker {
194
195 public:
196 explicit VersionBitsConditionChecker(Consensus::DeploymentPos id_) : id(id_) {}
197- uint32_t Mask(const Consensus::Params& params) const { return ((uint32_t)1) << params.vDeployments[id].bit; }
198+ uint32_t Mask(const Consensus::Params& params) const { return ((uint32_t)1) << params.vDeployments.at(id).bit; }
int{} syntax but not int(). Learn smth new everyday
.at() change based on https://www.erisian.com.au/bitcoin-core-dev/log-2022-02-02.html#l-301 . Still in draft for #23536
Removes all the template magic; and downgrades the “you forgot to initialize a deployment” check from compile-time to the unit tests, and doesn’t convert to std::array. On the upside, this means this PR no longer conflicts with #23536.
Based on this week’s meeting, also switches to using designated initializers. Doesn’t do designated initializers because you seem to need to specify /std:c++latest instead of /std:c++17 for Visual Studio to support that.
While chainparams should explicilty set values for each possible
entry in vDeployments, in the past that has been missed resulting
in potential undefined behaviour due to accessing unitinitialized
data. Reduce the severity of future bugs of that nature by providing
benign default values. Adds a unit test to alert if the default value
is not overwritten for the real chains (NEVER_ACTIVE/NEVER_ACTIVE rather
than NEVER_ACTIVE/NO_TIMEOUT).
