test: Check that no versionbits are re-used

MarcoFalke commented at 7:57 am on April 15, 2021: member

fanquake added the label Tests on Apr 15, 2021

in src/test/versionbits_tests.cpp:452 in faac2e4100 outdated

448+            // Check that no bits are re-used (within the same chain). This is
449+            // disallowed because the transition to FAILED (on timeout) does
450+            // not take precedence over STARTED/LOCKED_IN. So all softforks on
451+            // the same bit might overlap, even when non-overlapping start-end
452+            // times are picked.
453+            BOOST_CHECK(chain_bits.insert(chainParams->GetConsensus().vDeployments[dep].bit).second);

ajtowns commented at 8:54 am on April 15, 2021:

Err, there’s already a test to “Verify that the deployment windows of different deployment using the same bit are disjoint.” wouldn’t it make more sense to do:

0+            BOOST_CHECK(VersionBitsMask(mainnetParams, static_cast<Consensus::DeploymentPos>(j)) != bitmask);
1-            if (VersionBitsMask(mainnetParams, static_cast<Consensus::DeploymentPos>(j)) == bitmask) {
2-                BOOST_CHECK(mainnetParams.vDeployments[j].nStartTime > mainnetParams.vDeployments[i].nTimeout ||
3-                        mainnetParams.vDeployments[i].nStartTime > mainnetParams.vDeployments[j].nTimeout);
4-            }

MarcoFalke commented at 10:04 am on April 15, 2021:

This is only run for the mainnetParams, but we want to run it for all chains.

I’ve removed the redundant parts from versionbits_sanity and made the other parts run for all chains.

ajtowns commented at 8:55 am on April 15, 2021: member

Concept ACK

jnewbery commented at 9:18 am on April 15, 2021: member

Concept ACK. I agree that #21691 (review) is a better way of adding this test.

in src/test/versionbits_tests.cpp:429 in fa8016dd35 outdated

425+            // Check that no bits are re-used (within the same chain). This is
426+            // disallowed because the transition to FAILED (on timeout) does
427+            // not take precedence over STARTED/LOCKED_IN. So all softforks on
428+            // the same bit might overlap, even when non-overlapping start-end
429+            // times are picked.
430+            BOOST_CHECK(chain_bits.insert(chainParams->GetConsensus().vDeployments[dep].bit).second);

jnewbery commented at 2:22 pm on April 15, 2021:

Do we have any guidance on whether we should avoid side-effects in BOOST_CHECK() expressions? This can obviously be rewritten in a way that inserts and then checks the return value on the following line, but I’m not sure if that’s preferred

MarcoFalke commented at 8:51 am on April 17, 2021:

Removed this line because I think it is redundant

in src/test/versionbits_tests.cpp:430 in fa8016dd35 outdated

426+            // disallowed because the transition to FAILED (on timeout) does
427+            // not take precedence over STARTED/LOCKED_IN. So all softforks on
428+            // the same bit might overlap, even when non-overlapping start-end
429+            // times are picked.
430+            BOOST_CHECK(chain_bits.insert(chainParams->GetConsensus().vDeployments[dep].bit).second);
431+            BOOST_CHECK(chain_masks.insert(VersionBitsMask(chainParams->GetConsensus(), dep)).second);

jnewbery commented at 2:27 pm on April 15, 2021:

Rather than adding to a std::set<uint32_t>, should we check set chain_masks to a uint32_t, and then check that there’s no overlap between the signaled bits:

 0--- a/src/test/versionbits_tests.cpp
 1+++ b/src/test/versionbits_tests.cpp
 2@@ -418,7 +418,7 @@ BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
 3     for (const auto& chain_name : {CBaseChainParams::MAIN, CBaseChainParams::TESTNET, CBaseChainParams::SIGNET, CBaseChainParams::REGTEST}) {
 4         const auto chainParams = CreateChainParams(*m_node.args, chain_name);
 5         std::set<decltype(Consensus::BIP9Deployment::bit)> chain_bits;
 6-        std::set<uint32_t> chain_masks;
 7+        uint32_t chain_masks{0};
 8         for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++i) {
 9             const auto dep = static_cast<Consensus::DeploymentPos>(i);
10             // Check that no bits are re-used (within the same chain). This is
11@@ -427,7 +427,9 @@ BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
12             // the same bit might overlap, even when non-overlapping start-end
13             // times are picked.
14             BOOST_CHECK(chain_bits.insert(chainParams->GetConsensus().vDeployments[dep].bit).second);
15-            BOOST_CHECK(chain_masks.insert(VersionBitsMask(chainParams->GetConsensus(), dep)).second);
16+            const uint32_t chain_mask = VersionBitsMask(chainParams->GetConsensus(), dep);
17+            BOOST_CHECK(!(chain_masks & chain_mask));
18+            chain_masks |= chain_mask;
19             check_computeblockversion(chainParams->GetConsensus(), dep);
20         }
21     }

MarcoFalke commented at 8:51 am on April 17, 2021:

Thanks, done that.

in src/test/versionbits_tests.cpp:288 in faac2e4100 outdated

311@@ -314,7 +312,7 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus
312     BOOST_REQUIRE(0 <= bit && bit < 32);
313     BOOST_REQUIRE(((1 << bit) & VERSIONBITS_TOP_MASK) == 0);
314     BOOST_REQUIRE(min_activation_height >= 0);
315-    BOOST_REQUIRE_EQUAL(min_activation_height % params.nMinerConfirmationWindow, 0);
316+    BOOST_REQUIRE_EQUAL(min_activation_height % params.nMinerConfirmationWindow, 0U);

ajtowns commented at 2:34 am on April 16, 2021:

Seems like there’s a few of these issues: #21701#pullrequestreview-637244766

MarcoFalke commented at 8:48 am on April 17, 2021:

The other instance is removed in the next commit

MarcoFalke added this to the milestone 22.0 on Apr 16, 2021

MarcoFalke commented at 9:14 am on April 16, 2021: member

(not for backport, obviously)

DrahtBot commented at 2:47 pm on April 16, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19438 (Introduce deploymentstatus by ajtowns)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke force-pushed on Apr 17, 2021

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

fad4167871

test: Address outstanding versionbits_test feedback

* https://github.com/bitcoin/bitcoin/pull/21377#discussion_r609585080
* https://github.com/bitcoin/bitcoin/pull/21377#discussion_r613702341

faec1e9ee1

MarcoFalke force-pushed on Apr 17, 2021

test: Run versionbits_sanity for all chains fa8eaee6a8

MarcoFalke force-pushed on Apr 17, 2021

practicalswift commented at 10:35 am on April 17, 2021: contributor

Concept ACK

in src/test/versionbits_tests.cpp:258 in fa8eaee6a8

254@@ -255,25 +255,6 @@ BOOST_AUTO_TEST_CASE(versionbits_test)
255     }
256 }
257 
258-BOOST_AUTO_TEST_CASE(versionbits_sanity)

ajtowns commented at 2:11 am on April 19, 2021:

I think it would be better to keep the “check the params make sense” checks separate from the “check ComputeBlockVersion does the right thing” checks.

MarcoFalke commented at 5:51 am on April 19, 2021:

the sanity checks are a requirement, so they need to be run as part of the same test case. Do you mean to split them up into a separate function? I think that’d be too verbose, but I am happy to take a diff if you happen to have one.

ajtowns commented at 6:19 am on April 19, 2021:

Yeah, maybe you’re right – can’t reliably test computeblockversion if some other deployment might start setting the bit in question as soon as it hits ACTIVE, even if they’re not technically overlapping. No need to hold this up over it in any event.

ajtowns commented at 2:34 am on April 19, 2021: member

ACK fa8eaee6a8531db970cc84436bf2ae8150a58642 code review only

jnewbery commented at 12:51 pm on April 19, 2021: member

Code review ACK fa8eaee6a8531db970cc84436bf2ae8150a58642

jonatack commented at 1:36 pm on April 19, 2021: member

Could pick up the suggestions by sipa at #21377 (review) and I to make these const or reference to const

0    int64_t bit = params.vDeployments[dep].bit;
1    int64_t nStartTime = params.vDeployments[dep].nStartTime;
2    int64_t nTimeout = params.vDeployments[dep].nTimeout;
3    int min_activation_height = params.vDeployments[dep].min_activation_height;

in src/test/versionbits_tests.cpp:278 in fa8eaee6a8

275+    if (nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE ||
276+        nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE)
277+    {
278+        BOOST_CHECK_EQUAL(min_activation_height, 0);
279+        return;
280+    }

jonatack commented at 1:43 pm on April 19, 2021:

0@@ -271,8 +367,7 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus
1 
2     // always/never active deployments shouldn't need to be tested further
3     if (nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE ||
4-        nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE)
5-    {
6+        nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE) {
7         BOOST_CHECK_EQUAL(min_activation_height, 0);
8         return;
9     }

MarcoFalke commented at 4:21 pm on April 19, 2021:

Personally I find it incredibly ugly to have the if-condition and the first if-branch continue with the same indentation as if they are one block. It might be good to update the clang-format to not do this.

Also this file is far from clang-format clean, so I am going to leave as is for now.

ajtowns commented at 8:55 pm on April 19, 2021:

@MarcoFalke CONCEPT ACK

jonatack commented at 2:16 pm on April 19, 2021: member

Sanity check on the values in versionbits_computeblockversion

 0     for (const auto& chain_name : {CBaseChainParams::MAIN, CBaseChainParams::TESTNET, CBaseChainParams::SIGNET, CBaseChainParams::REGTEST}) {
 1+        BOOST_TEST_MESSAGE(strprintf("chain: %s", chain_name));
 2         const auto chainParams = CreateChainParams(*m_node.args, chain_name);
 3         uint32_t chain_all_vbits{0};
 4         for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++i) {
 5@@ -426,8 +427,16 @@ BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
 6             // the same bit might overlap, even when non-overlapping start-end
 7             // times are picked.
 8             const uint32_t dep_mask{VersionBitsMask(chainParams->GetConsensus(), dep)};
 9+            BOOST_TEST_MESSAGE(strprintf("i: %d", i));
10+            BOOST_TEST_MESSAGE(strprintf("dep_mask: %d", dep_mask));
11+            BOOST_TEST_MESSAGE(strprintf("chain_all_vbits: %d", chain_all_vbits));
12             BOOST_CHECK(!(chain_all_vbits & dep_mask));
13             chain_all_vbits |= dep_mask;
14+            if (i == 0) {
15+                BOOST_TEST_MESSAGE(strprintf("updated chain_all_vbits after |= dep_mask: %d", chain_all_vbits));
16+            } else {
17+                BOOST_TEST_MESSAGE("\n");
18+            }
19             check_computeblockversion(chainParams->GetConsensus(), dep);
20         }
21     }

 0$ src/test/test_bitcoin -t versionbits_tests -l message
 1Running 2 test cases...
 2chain: main
 3i: 0
 4dep_mask: 268435456
 5chain_all_vbits: 0
 6updated chain_all_vbits after |= dep_mask: 268435456
 7i: 1
 8dep_mask: 4
 9chain_all_vbits: 268435456
10
11
12chain: test
13i: 0
14dep_mask: 268435456
15chain_all_vbits: 0
16updated chain_all_vbits after |= dep_mask: 268435456
17i: 1
18dep_mask: 4
19chain_all_vbits: 268435456
20
21
22chain: signet
23i: 0
24dep_mask: 268435456
25chain_all_vbits: 0
26updated chain_all_vbits after |= dep_mask: 268435456
27i: 1
28dep_mask: 4
29chain_all_vbits: 268435456
30
31
32chain: regtest
33i: 0
34dep_mask: 268435456
35chain_all_vbits: 0
36updated chain_all_vbits after |= dep_mask: 268435456
37i: 1
38dep_mask: 4
39chain_all_vbits: 268435456
40
41
42*** No errors detected

MarcoFalke commented at 5:15 am on April 20, 2021: member

to make these const

Left this as is for now to not invalidate reviews

MarcoFalke merged this on Apr 20, 2021

MarcoFalke closed this on Apr 20, 2021

MarcoFalke deleted the branch on Apr 20, 2021

sidhujag referenced this in commit 1525c99670 on Apr 20, 2021

gwillen referenced this in commit d273c001cc on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

test: Check that no versionbits are re-used #21691

Conflicts