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

maflcko · 2021-04-15T07:57:14Z

No description provided.

ajtowns

Concept ACK

src/test/versionbits_tests.cpp

jnewbery · 2021-04-15T09:18:55Z

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

src/test/versionbits_tests.cpp

maflcko · 2021-04-16T09:14:28Z

(not for backport, obviously)

DrahtBot · 2021-04-16T14:47:11Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Introduce deploymentstatus #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.

This allows to remove check that windows for the same bit are disjoint This addresses bitcoin#21377 (comment)

* bitcoin#21377 (comment) * bitcoin#21377 (comment)

practicalswift · 2021-04-17T10:35:03Z

Concept ACK

ajtowns

ACK fa8eaee code review only

ajtowns · 2021-04-19T02:11:27Z

src/test/versionbits_tests.cpp

    }
 }

-BOOST_AUTO_TEST_CASE(versionbits_sanity)


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.

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.

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.

jnewbery · 2021-04-19T12:51:18Z

Code review ACK fa8eaee

jonatack

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

    int64_t bit = params.vDeployments[dep].bit;
    int64_t nStartTime = params.vDeployments[dep].nStartTime;
    int64_t nTimeout = params.vDeployments[dep].nTimeout;
    int min_activation_height = params.vDeployments[dep].min_activation_height;

jonatack · 2021-04-19T13:43:48Z

src/test/versionbits_tests.cpp

+    {
+        BOOST_CHECK_EQUAL(min_activation_height, 0);
+        return;
+    }


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

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.

@MarcoFalke CONCEPT ACK

jonatack · 2021-04-19T14:16:58Z

Sanity check on the values in versionbits_computeblockversion

     for (const auto& chain_name : {CBaseChainParams::MAIN, CBaseChainParams::TESTNET, CBaseChainParams::SIGNET, CBaseChainParams::REGTEST}) {
+        BOOST_TEST_MESSAGE(strprintf("chain: %s", chain_name));
         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) {
@@ -426,8 +427,16 @@ BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
             // the same bit might overlap, even when non-overlapping start-end
             // times are picked.
             const uint32_t dep_mask{VersionBitsMask(chainParams->GetConsensus(), dep)};
+            BOOST_TEST_MESSAGE(strprintf("i: %d", i));
+            BOOST_TEST_MESSAGE(strprintf("dep_mask: %d", dep_mask));
+            BOOST_TEST_MESSAGE(strprintf("chain_all_vbits: %d", chain_all_vbits));
             BOOST_CHECK(!(chain_all_vbits & dep_mask));
             chain_all_vbits |= dep_mask;
+            if (i == 0) {
+                BOOST_TEST_MESSAGE(strprintf("updated chain_all_vbits after |= dep_mask: %d", chain_all_vbits));
+            } else {
+                BOOST_TEST_MESSAGE("\n");
+            }
             check_computeblockversion(chainParams->GetConsensus(), dep);
         }
     }

$ src/test/test_bitcoin -t versionbits_tests -l message
Running 2 test cases...
chain: main
i: 0
dep_mask: 268435456
chain_all_vbits: 0
updated chain_all_vbits after |= dep_mask: 268435456
i: 1
dep_mask: 4
chain_all_vbits: 268435456


chain: test
i: 0
dep_mask: 268435456
chain_all_vbits: 0
updated chain_all_vbits after |= dep_mask: 268435456
i: 1
dep_mask: 4
chain_all_vbits: 268435456


chain: signet
i: 0
dep_mask: 268435456
chain_all_vbits: 0
updated chain_all_vbits after |= dep_mask: 268435456
i: 1
dep_mask: 4
chain_all_vbits: 268435456


chain: regtest
i: 0
dep_mask: 268435456
chain_all_vbits: 0
updated chain_all_vbits after |= dep_mask: 268435456
i: 1
dep_mask: 4
chain_all_vbits: 268435456


*** No errors detected

maflcko · 2021-04-20T05:15:56Z

to make these const

Left this as is for now to not invalidate reviews

fa8eaee test: Run versionbits_sanity for all chains (MarcoFalke) faec1e9 test: Address outstanding versionbits_test feedback (MarcoFalke) fad4167 test: Check that no versionbits are re-used (MarcoFalke) Pull request description: ACKs for top commit: jnewbery: Code review ACK fa8eaee ajtowns: ACK fa8eaee code review only Tree-SHA512: e99ffcca8970921fd07fa9e04cf1ea2515a317409865d34ddfd70be0f0b0616b29d1fad58262d96a3c3418c0cf7018a6a955802a178b8f78f6ecfaa30a37d91c

maflcko mentioned this pull request Apr 15, 2021

Speedy trial support for versionbits #21377

Merged

fanquake added the Tests label Apr 15, 2021

ajtowns reviewed Apr 15, 2021

View reviewed changes

src/test/versionbits_tests.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Apr 15, 2021

View reviewed changes

src/test/versionbits_tests.cpp Outdated Show resolved Hide resolved

src/test/versionbits_tests.cpp Outdated Show resolved Hide resolved

ajtowns reviewed Apr 16, 2021

View reviewed changes

src/test/versionbits_tests.cpp Outdated Show resolved Hide resolved

maflcko added this to the 22.0 milestone Apr 16, 2021

DrahtBot mentioned this pull request Apr 16, 2021

Introduce deploymentstatus #19438

Merged

maflcko force-pushed the 2104-testVersionbits branch from fa8016d to fabcfe3 Compare April 17, 2021 08:37

MarcoFalke added 2 commits April 17, 2021 10:40

test: Check that no versionbits are re-used

fad4167

This allows to remove check that windows for the same bit are disjoint This addresses bitcoin#21377 (comment)

test: Address outstanding versionbits_test feedback

faec1e9

* bitcoin#21377 (comment) * bitcoin#21377 (comment)

maflcko force-pushed the 2104-testVersionbits branch from fabcfe3 to fabe30b Compare April 17, 2021 08:42

test: Run versionbits_sanity for all chains

fa8eaee

maflcko force-pushed the 2104-testVersionbits branch from fabe30b to fa8eaee Compare April 17, 2021 09:30

ajtowns reviewed Apr 19, 2021

View reviewed changes

jonatack reviewed Apr 19, 2021

View reviewed changes

maflcko merged commit de77cbc into bitcoin:master Apr 20, 2021

maflcko deleted the 2104-testVersionbits branch April 20, 2021 05:23

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge de77cbc into merged_master (Bitcoin PR bitcoin/bitcoin#21691)

d273c00

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

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

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

Uh oh!

Conversation

maflcko commented Apr 15, 2021

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Apr 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Apr 16, 2021

Uh oh!

DrahtBot commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Apr 17, 2021

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

ajtowns Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 19, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

ajtowns Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack commented Apr 19, 2021

Uh oh!

maflcko commented Apr 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Apr 16, 2021 •

edited

Loading