Replaces State() (which returned ACTIVE/STARTED/etc) with IsActiveAfter()
which just returns a bool, as this was all State was actually used
for. Drops Mask(), which was only used in tests and can be replaced with
`1<<bit`, and also drops StateSinceHeight() and Statistics(), which are
now only used internally for Info().
Rather than having the rule change period/threshold be constant for all
potential deployments on a chain, have it be specific to the deployment
itself. This both matches history (BIP 9 specified a 2016 block period
and 1916 block threshold; BIP 91 specified a 336 block period and 269
block threshold; and BIP 341 specified a 2016 block period and 1815
block threshold), and allows the code to be simplified, as only the
BIP9Deployment structure is needed, not the full Consensus::Params
structure.
For an abstract class, specifying parameters in detail serves no point;
and for the concrete implementation, changing the consensus parameters
between invocations doesn't make sense. So simplify the class by removing
the consensus params from the method arguments, and just make it a member
variable in the concrete object where needed. This also allows dropping
dummy parameters from the unit/fuzz tests.
This commit effectively moves the definition of these constants
out of the chainparamsbase to their own file.
Using the ChainType enums provides better type safety compared to
passing around strings.
The commit is part of an ongoing effort to decouple the libbitcoinkernel
library from the ArgsManager and other functionality that should not be
part of the kernel library.
f565b2836d Fixup option name in bench message (Ben Woosley)
bf209ac7a7 doc: Fix spelling errors identified by codespell in coments (Ben Woosley)
Pull request description:
From the output [here](https://cirrus-ci.com/task/5275612980969472?logs=lint#L849):
```
src/qt/test/addressbooktests.cpp:185: wilcard ==> wildcard
src/qt/test/addressbooktests.cpp:191: wilcard ==> wildcard
src/test/miniscript_tests.cpp:227: nd ==> and, 2nd
src/test/versionbits_tests.cpp:260: everytime ==> every time
src/util/time.h:89: precicion ==> precision
src/util/time.h:90: precicion ==> precision
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
~~I left the 'nd' in miniscript_tests as-is, as it's valid miniscript,
and I'm wary of whitelisting it.~~
ACKs for top commit:
dunxen:
ACK f565b28
Tree-SHA512: 501a426c5f6f9761e2c8f980d5d955611428a827321888f53e0ae9526b0fecd43f9d1fa845fc70ae2489d77be6dc0b5b371dff55c5146f4b39ed874f4a1ea917
c4c5b9ca6e 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 c4c5b9ca6e
Tree-SHA512: 22d7ff86a817d9e9e67c47301fc3b7e9d5821c13565d7706711f113dea220eea29b413a7c8d029691009159cebc85a108d77cb52418734091c196bafb2b12423
From the output here:
src/qt/test/addressbooktests.cpp:185: wilcard ==> wildcard
src/qt/test/addressbooktests.cpp:191: wilcard ==> wildcard
src/test/miniscript_tests.cpp:227: nd ==> and, 2nd
src/test/versionbits_tests.cpp:260: everytime ==> every time
src/util/time.h:89: precicion ==> precision
src/util/time.h:90: precicion ==> precision
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
https://cirrus-ci.com/task/5275612980969472?logs=lint#L849
I added 'nd' to the spelling.ignored-words.txt, as it's valid miniscript.
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).
Adds support for versionbits deployments to DeploymentEnabled,
DeploymentActiveAfter and DeploymentActiveAt. Also moves versionbitscache
from validation to deploymentstatus.
This removes the DEFINED->FAILED transition and changes the
STARTED->FAILED transition to only occur if signalling didn't pass the
threshold. This ensures that it is always possible for activation to
occur, no matter what settings are chosen, or the speed at which blocks
are found.
Previously we used deployments that would timeout prior to Bitcoin's
invention, which allowed the deployment to still be activated in unit
tests. This switches those deployments to be truly never active.
This generalises the ComputeBlockVersion test so that it can apply to
any activation parameters we might set, and checks all the parameters
set for each deployment on each chain, to simultaneously ensure that the
deployments we have configured work sensibly, and that the test code
does not suffer bitrot in the event that all interesting deployments
are buried.
The intent here is to allow checking ComputeBlockVersion behaviour with
each deployment, rather than only testdummy on mainnet. This commit does
the trivial refactoring component of that change.
2c448d6bc7 parameterize hard coded numbers referring to miner conf window (Jordan Baczuk)
Pull request description:
Replace hard coded values (eg. 2016) with `mainnetParams.nMinerConfirmationWindow` where appropriate. This parameterizes hard coded values in the unit test that refer to the `Miner Confirmation Window`, which currently is `2016`. This includes values not exactly 2016 but which were derived from it. Also changed `int` to `uint32_t` where appropriate to avoid compiler warnings. This makes one source of truth, and also helps people who might be adjusting this value in testing so the unit tests don't break.
ACKs for commit 2c448d:
Tree-SHA512: 9262e0b89c1baf7857b49fe2221b2b00f948f61317b321c4871a9182a86d6f8aadeb59d6b133e8a213cc9b31b4a417888fb1ad31caef16ccbbab1de33c4b8459
9ad6746ccd Use static_cast instead of C-style casts for non-fundamental types (practicalswift)
Pull request description:
A C-style cast is equivalent to try casting in the following order:
1. `const_cast(...)`
2. `static_cast(...)`
3. `const_cast(static_cast(...))`
4. `reinterpret_cast(...)`
5. `const_cast(reinterpret_cast(...))`
By using `static_cast<T>(...)` explicitly we avoid the possibility of an unintentional and dangerous `reinterpret_cast`. Furthermore `static_cast<T>(...)` allows for easier grepping of casts.
For a more thorough discussion, see ["ES.49: If you must use a cast, use a named cast"](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast) in the C++ Core Guidelines (Stroustrup & Sutter).
Tree-SHA512: bd6349b7ea157da93a47b8cf238932af5dff84731374ccfd69b9f732fabdad1f9b1cdfca67497040f14eaa85346391404f4c0495e22c467f26ca883cd2de4d3c
