PR 16060 removed the CSV and Segwit BIP9 softfork definitions and hard-coded (‘buried’) the activation heights. The versionbits code will warn users if an undefined softfork has been signalled in block header versions, and removing the CSV/Segwit definitions caused those warnings to be triggered.
Change the BIP 9 warning code to only check for unknown softforks after the segwit activation height.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
Adds a hardcoded height to the consensus chain parameters for
ignoring versionbit activations prior to a fixed height.
195@@ -188,6 +196,14 @@ class CTestNetParams : public CChainParams {
196 consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 1199145601; // January 1, 2008
197 consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = 1230767999; // December 31, 2008
198
199+ // CSV and Segwit versionbits parameters required to prevent soft fork warnings
200+ consensus.vDeployments[Consensus::DEPLOYMENT_CSV].bit = 0;
201+ consensus.vDeployments[Consensus::DEPLOYMENT_CSV].nStartTime = 1462060800; // May 1st, 2016
ACK fdb3e8f8b2
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK fdb3e8f8b2
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUgH2gwAz7j3RTqo2rmSToD74d8gNagy+RG2uMcdAL+WwhEuk7u3N/KiNp7iO/Qd
8zuxEIeAWbjLrUA7jKHTGqDWjrjef1+dlhAZB8pocYIrM0cluJZgYnaCIQkbZekeL
9jg3xgaZ6RXeLnfKNBrNw55M/hjDn2FwF/9BxtcmrDBoUT9m1wPbrATypwvhSQlPG
10LSbMfW9zl/H98Qpz6WfHc971mqNpsJgHLgJRaizrynN8sXeSYF6DWYCSUW0A4eSb
11Q+ab/Ox6Z8QSh2fVUzwbFVQonNAvNgsfeIpUGZaiUi47MasORxlm0EFplvEEzQtd
12ncc8Kr1LCZzWmG5bRL9yAiT08gOv8IqwKI7TsvhXTkCwKn8DMxPVBJzUlvdHg8gU
137wvwm8tMprvzlS6Q+l7Oy8FXtqtxVqm8uh+3EJCN3l+EYRfeWKbDmZv4mlvF19KD
14sXZjlmvMmq1kEfzLTvoLrExNBNJ2DITBzKMLDyNHD/eBTuSL3hIkZ2X+YZmbWQ9m
15dDycm2P1
16=XXpf
17-----END PGP SIGNATURE-----
Timestamp of file with hash cd1308070d16f4ec75ba666d53d7ee577ce57d8617bdb8f84dd8d20af9925ccf -
ACK fdb3e8f8b27e3b0b2f88c32915975c6e4c299b1e
Code looks good to me. Also tested that the warning is longer being shown.
consensus.SegwitHeight is going to split the network, so I think not displaying a warning is a minor (or non-)issue. I think the pull request could be more clear and explain that in a comment maybe?
Well, a node doing IBD and being fed an alternate/bogus chain should probably sound as many alarms as it can… But my concern here is that we’re changing utility code (in WarningBitsConditionChecker) that, at least in my mental mapping of our codebase, should be unaware of holistic reasoning around consensus issues like this.
If others disagree with my assessment, then maybe this approach is fine, but just thought I should mention that this design looks off to me.
Well, a node doing IBD and being fed an alternate/bogus chain should probably sound as many alarms as it can…
We could add a warning if our chain doesn’t contain defaultAssumeValid which would catch this? Or that it doesn’t contain the -assumevalid block if that block is non-zero (which would be easier to write tests for).
But my concern here is that we’re changing utility code (in
WarningBitsConditionChecker) that, at least in my mental mapping of our codebase, should be unaware of holistic reasoning around consensus issues like this.
WarningBitsConditionChecker already knows “holistic” information about activation though – it knows the status of all supported versionbits deployments to avoid triggering on known deployments (via the ((ComputeBlockVersion() >> bit) & 1) == 0 test which will fail if there’s a version bits deployment in STARTED or LOCKED_IN phase on that bit).
I think I’m a -0 on this; imo this makes the code a bit harder to reason about,
Only other simple approach I can see would be to move the holistic info directly into ComputeBlockVersion – maybe have it return 0x2fff_ffff (signal all the version bits) if it’s asked for a version for blocks before the threshold height? Alternatively could do this now and clean it up more in 0.20 – there’s other versionbits cleanups I’d like to do fwiw.
ACK fdb3e8f8b27e3b0b2f88c32915975c6e4c299b1e
Reviewed code, built/ran tests, verified the “Warning: unknown new rules activated (versionbit 1)” warning in validation.cpp::L2183 is no longer displayed.
The approach seems straightforward and minimal. One with better separation of concerns could be studied as a follow-up.
Threw some LogPrintfs into WarningBitsConditionChecker as a sanity check for the values it sees.
0 bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const override
1 {
2 LogPrintf("SegwitHeight %s\n", params.SegwitHeight);
3 LogPrintf("nMinerConfirmationWindow %s\n", params.nMinerConfirmationWindow);
4 LogPrintf("MinBIPWarningHeight %s\n", params.MinBIP9WarningHeight);
5 LogPrintf("pindex->nHeight %s\n", pindex->nHeight);
6 LogPrintf("pindex->nVersion %s\n", pindex->nVersion);
7 LogPrintf("ComputeBlockVersion %s\n", ComputeBlockVersion(pindex->pprev, params));
8
9 return pindex->nHeight >= params.MinBIP9WarningHeight &&
10 ((pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) &&
11 ((pindex->nVersion >> bit) & 1) != 0 &&
12 ((ComputeBlockVersion(pindex->pprev, params) >> bit) & 1) == 0;
13 }
02019-09-20T11:26:36Z SegwitHeight 481824
12019-09-20T11:26:36Z nMinerConfirmationWindow 2016
22019-09-20T11:26:36Z MinBIPWarningHeight 483840
32019-09-20T11:26:36Z pindex->nHeight 164836
42019-09-20T11:26:36Z pindex->nVersion 1
52019-09-20T11:26:36Z ComputeBlockVersion 536870912
02019-09-20T11:34:30Z SegwitHeight 481824
12019-09-20T11:34:30Z nMinerConfirmationWindow 2016
22019-09-20T11:34:30Z MinBIPWarningHeight 483840
32019-09-20T11:34:30Z pindex->nHeight 137541
42019-09-20T11:34:30Z pindex->nVersion 1
52019-09-20T11:34:30Z ComputeBlockVersion 536870912
02019-09-20T11:36:35Z SegwitHeight 481824
12019-09-20T11:36:35Z nMinerConfirmationWindow 2016
22019-09-20T11:36:35Z MinBIPWarningHeight 483840
32019-09-20T11:36:35Z pindex->nHeight 35101
42019-09-20T11:36:35Z pindex->nVersion 1
52019-09-20T11:36:35Z ComputeBlockVersion 536870912
Alternatively could do this now and clean it up more in 0.20 – there’s other versionbits cleanups I’d like to do fwiw.
Sure, that sounds fine to me.
Labels
Validation
Milestone
0.19.0