From fad4167871c3c9fde462e64e3ef3be937e585084 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 15 Apr 2021 10:02:32 +0200 Subject: [PATCH 1/3] test: Check that no versionbits are re-used This allows to remove check that windows for the same bit are disjoint This addresses https://github.com/bitcoin/bitcoin/pull/21377#discussion_r611492633 --- src/test/versionbits_tests.cpp | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp index 05838ec19a..c80f8cbf87 100644 --- a/src/test/versionbits_tests.cpp +++ b/src/test/versionbits_tests.cpp @@ -273,20 +273,6 @@ BOOST_AUTO_TEST_CASE(versionbits_sanity) if (mainnetParams.vDeployments[i].nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE || mainnetParams.vDeployments[i].nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE) { BOOST_CHECK_EQUAL(mainnetParams.vDeployments[i].min_activation_height, 0); } - - // Verify that the deployment windows of different deployment using the - // same bit are disjoint. - // This test may need modification at such time as a new deployment - // is proposed that reuses the bit of an activated soft fork, before the - // end time of that soft fork. (Alternatively, the end time of that - // activated soft fork could be later changed to be earlier to avoid - // overlap.) - for (int j=i+1; j<(int) Consensus::MAX_VERSION_BITS_DEPLOYMENTS; j++) { - if (VersionBitsMask(mainnetParams, static_cast(j)) == bitmask) { - BOOST_CHECK(mainnetParams.vDeployments[j].nStartTime > mainnetParams.vDeployments[i].nTimeout || - mainnetParams.vDeployments[i].nStartTime > mainnetParams.vDeployments[j].nTimeout); - } - } } } @@ -443,8 +429,18 @@ BOOST_AUTO_TEST_CASE(versionbits_computeblockversion) // ACTIVE and FAILED states in roughly the way we expect for (const auto& chain_name : {CBaseChainParams::MAIN, CBaseChainParams::TESTNET, CBaseChainParams::SIGNET, CBaseChainParams::REGTEST}) { const auto chainParams = CreateChainParams(*m_node.args, chain_name); + uint32_t chain_all_vbits{0}; for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++i) { - check_computeblockversion(chainParams->GetConsensus(), static_cast(i)); + const auto dep = static_cast(i); + // Check that no bits are re-used (within the same chain). This is + // disallowed because the transition to FAILED (on timeout) does + // not take precedence over STARTED/LOCKED_IN. So all softforks on + // the same bit might overlap, even when non-overlapping start-end + // times are picked. + const uint32_t dep_mask{VersionBitsMask(chainParams->GetConsensus(), dep)}; + BOOST_CHECK(!(chain_all_vbits & dep_mask)); + chain_all_vbits |= dep_mask; + check_computeblockversion(chainParams->GetConsensus(), dep); } } From faec1e9ee1f12612831ad5b0f0a767d87bd2d024 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 17 Apr 2021 10:27:16 +0200 Subject: [PATCH 2/3] test: Address outstanding versionbits_test feedback * https://github.com/bitcoin/bitcoin/pull/21377#discussion_r609585080 * https://github.com/bitcoin/bitcoin/pull/21377#discussion_r613702341 --- src/test/versionbits_tests.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp index c80f8cbf87..ad27c1c26e 100644 --- a/src/test/versionbits_tests.cpp +++ b/src/test/versionbits_tests.cpp @@ -81,11 +81,9 @@ class VersionBitsTester TestNeverActiveConditionChecker checker_never[CHECKERS]; // Test counter (to identify failures) - int num; + int num{1000}; public: - VersionBitsTester() : num(1000) {} - VersionBitsTester& Reset() { // Have each group of tests be counted by the 1000s part, starting at 1000 num = num - (num % 1000) + 1000; @@ -300,7 +298,7 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus BOOST_REQUIRE(0 <= bit && bit < 32); BOOST_REQUIRE(((1 << bit) & VERSIONBITS_TOP_MASK) == 0); BOOST_REQUIRE(min_activation_height >= 0); - BOOST_REQUIRE_EQUAL(min_activation_height % params.nMinerConfirmationWindow, 0); + BOOST_REQUIRE_EQUAL(min_activation_height % params.nMinerConfirmationWindow, 0U); // In the first chain, test that the bit is set by CBV until it has failed. // In the second chain, test the bit is set by CBV while STARTED and From fa8eaee6a8531db970cc84436bf2ae8150a58642 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 15 Apr 2021 12:50:09 +0200 Subject: [PATCH 3/3] test: Run versionbits_sanity for all chains --- src/test/versionbits_tests.cpp | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp index ad27c1c26e..304cd8feb0 100644 --- a/src/test/versionbits_tests.cpp +++ b/src/test/versionbits_tests.cpp @@ -255,25 +255,6 @@ BOOST_AUTO_TEST_CASE(versionbits_test) } } -BOOST_AUTO_TEST_CASE(versionbits_sanity) -{ - // Sanity checks of version bit deployments - const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::MAIN); - const Consensus::Params &mainnetParams = chainParams->GetConsensus(); - for (int i=0; i<(int) Consensus::MAX_VERSION_BITS_DEPLOYMENTS; i++) { - uint32_t bitmask = VersionBitsMask(mainnetParams, static_cast(i)); - // Make sure that no deployment tries to set an invalid bit. - BOOST_CHECK_EQUAL(bitmask & ~(uint32_t)VERSIONBITS_TOP_MASK, bitmask); - - // Check min_activation_height is on a retarget boundary - BOOST_CHECK_EQUAL(mainnetParams.vDeployments[i].min_activation_height % mainnetParams.nMinerConfirmationWindow, 0); - // Check min_activation_height is 0 for ALWAYS_ACTIVE and never active deployments - if (mainnetParams.vDeployments[i].nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE || mainnetParams.vDeployments[i].nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE) { - BOOST_CHECK_EQUAL(mainnetParams.vDeployments[i].min_activation_height, 0); - } - } -} - /** Check that ComputeBlockVersion will set the appropriate bit correctly */ static void check_computeblockversion(const Consensus::Params& params, Consensus::DeploymentPos dep) { @@ -289,17 +270,26 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus BOOST_CHECK_EQUAL(ComputeBlockVersion(nullptr, params), VERSIONBITS_TOP_BITS); // always/never active deployments shouldn't need to be tested further - if (nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE) return; - if (nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE) return; + if (nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE || + nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE) + { + BOOST_CHECK_EQUAL(min_activation_height, 0); + return; + } BOOST_REQUIRE(nStartTime < nTimeout); BOOST_REQUIRE(nStartTime >= 0); BOOST_REQUIRE(nTimeout <= std::numeric_limits::max() || nTimeout == Consensus::BIP9Deployment::NO_TIMEOUT); BOOST_REQUIRE(0 <= bit && bit < 32); + // Make sure that no deployment tries to set an invalid bit. BOOST_REQUIRE(((1 << bit) & VERSIONBITS_TOP_MASK) == 0); BOOST_REQUIRE(min_activation_height >= 0); + // Check min_activation_height is on a retarget boundary BOOST_REQUIRE_EQUAL(min_activation_height % params.nMinerConfirmationWindow, 0U); + const uint32_t bitmask{VersionBitsMask(params, dep)}; + BOOST_CHECK_EQUAL(bitmask, uint32_t{1} << bit); + // In the first chain, test that the bit is set by CBV until it has failed. // In the second chain, test the bit is set by CBV while STARTED and // LOCKED-IN, and then no longer set while ACTIVE.