Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore old versionbit activations to avoid 'unknown softforks' warning #16713

Merged

Conversation

@jnewbery
Copy link
Member

jnewbery commented Aug 24, 2019

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

This comment has been minimized.

Copy link
Member Author

jnewbery commented Aug 24, 2019

Alternative to #16704

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Aug 24, 2019

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

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot added the Validation label Aug 24, 2019
@jnewbery jnewbery force-pushed the jnewbery:2019-08-silence-softfork-warning branch from 94c75dd to 85fff24 Aug 25, 2019
@jnewbery jnewbery changed the title [consensus] Redefine CSV and segwit deployments to avoid 'unknown softforks' warning logging: Redefine CSV and segwit deployments to avoid 'unknown softforks' warning Aug 25, 2019
Adds a hardcoded height to the consensus chain parameters for
ignoring versionbit activations prior to a fixed height.
Copy link
Member

MarcoFalke left a comment

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

@@ -188,6 +196,14 @@ class CTestNetParams : public CChainParams {
consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 1199145601; // January 1, 2008
consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = 1230767999; // December 31, 2008

// CSV and Segwit versionbits parameters required to prevent soft fork warnings
consensus.vDeployments[Consensus::DEPLOYMENT_CSV].bit = 0;
consensus.vDeployments[Consensus::DEPLOYMENT_CSV].nStartTime = 1462060800; // May 1st, 2016

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 5, 2019

Member

This started in march, no?

@jnewbery jnewbery force-pushed the jnewbery:2019-08-silence-softfork-warning branch from 85fff24 to fdb3e8f Sep 5, 2019
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Sep 5, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Sep 5, 2019

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 -

@MarcoFalke MarcoFalke dismissed their stale review Sep 5, 2019

.

@MarcoFalke MarcoFalke added this to the 0.19.0 milestone Sep 5, 2019
@MarcoFalke MarcoFalke changed the title logging: Redefine CSV and segwit deployments to avoid 'unknown softforks' warning Ignore old versionbit activations to avoid 'unknown softforks' warning Sep 5, 2019
@ajtowns

This comment has been minimized.

Copy link
Contributor

ajtowns commented Sep 5, 2019

ACK fdb3e8f for what it's worth

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Sep 5, 2019

ACK fdb3e8f

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

@sdaftuar

This comment has been minimized.

Copy link
Member

sdaftuar commented Sep 10, 2019

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?

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Sep 10, 2019

@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

This comment has been minimized.

Copy link
Member

sdaftuar commented Sep 10, 2019

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

This comment has been minimized.

Copy link
Contributor

ajtowns commented Sep 11, 2019

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

This comment has been minimized.

Copy link
Member

sdaftuar commented Sep 18, 2019

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Sep 18, 2019

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

This comment has been minimized.

Copy link
Contributor

ajtowns commented Sep 20, 2019

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.

Copy link
Member

jonatack left a comment

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

This comment has been minimized.

Copy link
Member

jonatack commented Sep 20, 2019

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

This comment has been minimized.

Copy link
Member

sdaftuar commented Sep 20, 2019

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

This comment has been minimized.

Copy link
Member Author

jnewbery commented Sep 21, 2019

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

This comment has been minimized.

Copy link
Member

Sjors commented Sep 27, 2019

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.

MarcoFalke added a commit that referenced this pull request Sep 27, 2019
…tforks' warning

fdb3e8f Ignore old versionbit activations (Anthony Towns)

Pull request description:

  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.

ACKs for top commit:
  MarcoFalke:
    ACK fdb3e8f
  ajtowns:
    ACK fdb3e8f for what it's worth
  achow101:
    ACK fdb3e8f
  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.
  jonatack:
    ACK fdb3e8f

Tree-SHA512: e6fd34e8902f8c7affb28e8951803e47d542710d5f1229000746656a37ee59d754439fc33e36b7eef87544262e5aac374645db91b74cb507e73514003ca7a67f
@MarcoFalke MarcoFalke merged commit fdb3e8f into bitcoin:master Sep 27, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Sep 27, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Sep 27, 2019

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

@jnewbery jnewbery deleted the jnewbery:2019-08-silence-softfork-warning branch Sep 27, 2019
sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 28, 2019
…own softforks' warning

fdb3e8f Ignore old versionbit activations (Anthony Towns)

Pull request description:

  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.

ACKs for top commit:
  MarcoFalke:
    ACK fdb3e8f
  ajtowns:
    ACK fdb3e8f for what it's worth
  achow101:
    ACK fdb3e8f
  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.
  jonatack:
    ACK fdb3e8f

Tree-SHA512: e6fd34e8902f8c7affb28e8951803e47d542710d5f1229000746656a37ee59d754439fc33e36b7eef87544262e5aac374645db91b74cb507e73514003ca7a67f
@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 12, 2019

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

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Nov 14, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.