Add defaults to vDeployments to avoid uninitialized variables #24032
Conversation
|
Something like this was previously proposed in #21401. Deployments have been left uninitialised in the past, see #20157 and also taproot activation in elements so this seems worth |
fb00b70 to
02bda4e
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
02bda4e to
6eaf3af
Compare
There was a problem hiding this comment.
Went over this with @amitiuttarwar and noticed some things.
src/util/syntaxhelp.h
Outdated
| const T t; | ||
|
|
||
| template<typename... I> | ||
| ArrayInitElement(I... i) : t{i...} { }; |
There was a problem hiding this comment.
I think this potentially copies its arguments unnecessarily and would be better as ArrayInitElement(I&&... i) : t{std::forward<I>(i)...} { }. Also, no need for a semicolon.
src/util/syntaxhelp.h
Outdated
| { | ||
| static_assert(E < N, "too many initializers for array"); | ||
| static_assert(E == O, "array initializer element is out of order"); | ||
| if constexpr (E < N) { |
There was a problem hiding this comment.
FYI: this if constexpr is only here to avoid redundant warnings that std::get<N> and step<N+1> would generate.
6eaf3af to
08aa93b
Compare
|
Concept ACK 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. |
08aa93b to
8bb515a
Compare
|
Please see PastaPastaPasta@768a84f to constexpr all the things |
8bb515a to
22886c1
Compare
22886c1 to
3d3724e
Compare
|
Rebased. Removed that |
3d3724e to
e679415
Compare
|
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
|
e679415 to
16cd0f4
Compare
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).
16cd0f4 to
c4c5b9c
Compare
|
Code review ACK c4c5b9c |
…zed variables c4c5b9c consensus/params: set default values for BIP9Deployment (Anthony Towns) Pull request description: Adds default values for `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. ACKs for top commit: laanwj: Code review ACK c4c5b9c Tree-SHA512: 22d7ff86a817d9e9e67c47301fc3b7e9d5821c13565d7706711f113dea220eea29b413a7c8d029691009159cebc85a108d77cb52418734091c196bafb2b12423
Adds default values for
vDeploymentsinconsensus/params.hso 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.