Ignore old versionbit activations to avoid 'unknown softforks' warning

[jnewbery]

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.

[jnewbery]

Alternative to #16704

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

[maflcko]

Concept ACK, but I'd prefer if the correct times were used: https://github.com/bitcoin/bips/blob/master/bip-0009/assignments.mediawiki

[maflcko]

This started in march, no?

[jnewbery]

I've updated this PR to take @ajtowns branch that changes the height at which we start looking for unknown softforks.

[maflcko]

ACK fdb3e8f

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK fdb3e8f8b2
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgH2gwAz7j3RTqo2rmSToD74d8gNagy+RG2uMcdAL+WwhEuk7u3N/KiNp7iO/Qd
zuxEIeAWbjLrUA7jKHTGqDWjrjef1+dlhAZB8pocYIrM0cluJZgYnaCIQkbZekeL
jg3xgaZ6RXeLnfKNBrNw55M/hjDn2FwF/9BxtcmrDBoUT9m1wPbrATypwvhSQlPG
LSbMfW9zl/H98Qpz6WfHc971mqNpsJgHLgJRaizrynN8sXeSYF6DWYCSUW0A4eSb
Q+ab/Ox6Z8QSh2fVUzwbFVQonNAvNgsfeIpUGZaiUi47MasORxlm0EFplvEEzQtd
ncc8Kr1LCZzWmG5bRL9yAiT08gOv8IqwKI7TsvhXTkCwKn8DMxPVBJzUlvdHg8gU
7wvwm8tMprvzlS6Q+l7Oy8FXtqtxVqm8uh+3EJCN3l+EYRfeWKbDmZv4mlvF19KD
sXZjlmvMmq1kEfzLTvoLrExNBNJ2DITBzKMLDyNHD/eBTuSL3hIkZ2X+YZmbWQ9m
dDycm2P1
=XXpf
-----END PGP SIGNATURE-----

Timestamp of file with hash cd1308070d16f4ec75ba666d53d7ee577ce57d8617bdb8f84dd8d20af9925ccf -

.

[ajtowns]

ACK fdb3e8f for what it's worth

[achow101]

ACK fdb3e8f

Code looks good to me. Also tested that the warning is longer being shown.

[sdaftuar]

Conceptually, shouldn't a minimum warning height be chain dependent? Not just mainnet vs testnet vs regtest, but also which mainnet chain you're on?

[maflcko]

@sdaftuar Why would that be? A reorg that changes the block at 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?

[sdaftuar]

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.

[ajtowns]

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

[sdaftuar]

I think I'm a -0 on this; imo this makes the code a bit harder to reason about, rather than easier to reason about, but I can also see why others disagree.

[maflcko]

This has 2-4 ACKS (unsure how to count), is consistent with the concept of burying, and is easier to review than previous attempts at fixing the 0.19 regression. I think this is good to go in as is, but I am also happy to chat about in in the IRC meeting this week

[ajtowns]

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.

[jonatack]

ACK fdb3e8f

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.

[jonatack]

Threw some LogPrintfs into WarningBitsConditionChecker as a sanity check for the values it sees.

    bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const override
    {
        LogPrintf("SegwitHeight %s\n", params.SegwitHeight);
        LogPrintf("nMinerConfirmationWindow %s\n", params.nMinerConfirmationWindow);
        LogPrintf("MinBIPWarningHeight %s\n", params.MinBIP9WarningHeight);
        LogPrintf("pindex->nHeight %s\n", pindex->nHeight);
        LogPrintf("pindex->nVersion %s\n", pindex->nVersion);
        LogPrintf("ComputeBlockVersion %s\n", ComputeBlockVersion(pindex->pprev, params));

        return pindex->nHeight >= params.MinBIP9WarningHeight &&
               ((pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) &&
               ((pindex->nVersion >> bit) & 1) != 0 &&
               ((ComputeBlockVersion(pindex->pprev, params) >> bit) & 1) == 0;
    }

2019-09-20T11:26:36Z SegwitHeight 481824
2019-09-20T11:26:36Z nMinerConfirmationWindow 2016
2019-09-20T11:26:36Z MinBIPWarningHeight 483840
2019-09-20T11:26:36Z pindex->nHeight 164836
2019-09-20T11:26:36Z pindex->nVersion 1
2019-09-20T11:26:36Z ComputeBlockVersion 536870912

2019-09-20T11:34:30Z SegwitHeight 481824
2019-09-20T11:34:30Z nMinerConfirmationWindow 2016
2019-09-20T11:34:30Z MinBIPWarningHeight 483840
2019-09-20T11:34:30Z pindex->nHeight 137541
2019-09-20T11:34:30Z pindex->nVersion 1
2019-09-20T11:34:30Z ComputeBlockVersion 536870912

2019-09-20T11:36:35Z SegwitHeight 481824
2019-09-20T11:36:35Z nMinerConfirmationWindow 2016
2019-09-20T11:36:35Z MinBIPWarningHeight 483840
2019-09-20T11:36:35Z pindex->nHeight 35101
2019-09-20T11:36:35Z pindex->nVersion 1
2019-09-20T11:36:35Z ComputeBlockVersion 536870912

[sdaftuar]

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.

[jnewbery]

could do this now and clean it up more in 0.20 -- there's other versionbits cleanups I'd like to do fwiw.

Thanks @ajtowns . Let me know when you have a PR.

[Sjors]

ACK fdb3e8f. It makes the bit 0 warning go away in mainnet and testnet QT when a new block arrives. I think the code is clear enough.

[Sjors]

I should add that testnet does still warn about bit 28 being activated; not sure what that's about.

[maflcko]

@Sjors segwit activated on testnet3 about 3 years ago. I guess you are correctly seeing the crap that testnet miners did in the last 3 years.

[practicalswift]

Reviewers of this PR might want to take a look at #17449 ("fix uninitialized variable nMinerConfirmationWindow") :)

[jnewbery]

Thanks for posting here @practicalswift . I agree that anyone who reviewed this should take a look at #17449.