test: Additional (refactored) BIP9 tests

Sjors commented at 11:47 am on March 2, 2021: member

Extracted from #19573 to make review easier. I also reviewed it myself.

I added some comments to the test: https://github.com/bitcoin/bitcoin/pull/19573/commits/bae9a452191a7a83478f7d508a54f4a04d385505#r585486781

I also moved some TestState changes from the second to the first commit, to reduce the latter diff.

Sjors commented at 11:52 am on March 2, 2021: member

cc @ajtowns @luke-jr @TheBlueMatt and those who ACK’d the full PR: @benthecarman, @michaelfolkson, @AnthonyRonning, @lucasmoten. Try:

0PREV=bd715ff8 N=2 && git range-diff `git merge-base --all HEAD $PREV`...$PREV HEAD~$N...HEAD --color-moved=dimmed-zebra

DrahtBot added the label Tests on Mar 2, 2021

michaelfolkson commented at 12:00 pm on March 2, 2021: contributor

Cool. Thanks for opening this Sjors. I’d rather not unpick all the work that has gone into BIP 8 and consensus that has built up around it over the past months. Is there a path forward you could get behind where we do actually refer to BIP 8 rather than BIP 9 even if you are opening pull requests assuming lockinontimeout=false?

Sjors commented at 12:19 pm on March 2, 2021: member

@michaelfolkson these first commits refer to existing BIP9 code, hence the title.

I agree it makes sense to refer to BIP 8 even with LOT=false code, but that’s not yet applicable here.

in src/test/versionbits_tests.cpp:23 in 6aeb212c3b outdated

18+    return (state == ThresholdState::DEFINED ?     "DEFINED" :
19+            state == ThresholdState::STARTED ?     "STARTED" :
20+            state == ThresholdState::LOCKED_IN ?   "LOCKED_IN" :
21+            state == ThresholdState::ACTIVE ?      "ACTIVE" :
22+            state == ThresholdState::FAILED ?      "FAILED" :
23+            "");

MarcoFalke commented at 12:20 pm on March 2, 2021:

Any reason to disable the compile time checks that come with a switch-case?

Sjors commented at 12:22 pm on March 2, 2021:

IIUC it’s to remain compatible with C++11: https://github.com/bitcoin/bitcoin/pull/19573/commits/bae9a452191a7a83478f7d508a54f4a04d385505#r579334677

MarcoFalke commented at 12:29 pm on March 2, 2021:

I am pretty sure that switch exists way before C++11

ajtowns commented at 12:48 pm on March 2, 2021:

It can’t be a constexpr function with a switch. I’m not sure what the reason for it being constexpr was, though; I may have been trying something with templates at one point?

Sjors commented at 1:06 pm on March 2, 2021:

I can change it to use switch and not constexpr. Is there a ./configure flag to force c++11?

luke-jr commented at 9:23 pm on March 2, 2021:

@Sjors Automatic if you develop on top of 831675c8dccfa6525ffe751da3cc60709c380953 ;)

ajtowns commented at 10:59 am on March 3, 2021:

 0static const std::string StateName(ThresholdState state)
 1{
 2    switch (state) {
 3    case ThresholdState::DEFINED:   return "DEFINED";
 4    case ThresholdState::STARTED:   return "STARTED";
 5    case ThresholdState::LOCKED_IN: return "LOCKED_IN";
 6    case ThresholdState::ACTIVE:    return "ACTIVE";
 7    case ThresholdState::FAILED:    return "FAILED";
 8    } // no default case, so the compiler can warn about missing cases
 9    return "";
10}

looks fine to me fwiw.

Sjors commented at 12:55 pm on March 3, 2021:

Two birds, one stone…

ajtowns commented at 12:49 pm on March 2, 2021: member

Concept ACK for splitting commits out.

luke-jr commented at 5:59 pm on March 2, 2021: member

Can you keep this based on 831675c8dccfa6525ffe751da3cc60709c380953 please? (So it’s a clean merge to both 0.21 and master)

DrahtBot commented at 9:37 pm on March 2, 2021: member

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

Conflicts

No conflicts as of last run.

darosior commented at 1:05 pm on March 3, 2021: member

Concept ACK

tests: more helpful errors for failing versionbits tests

Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>

3ba9283a47

tests: check never active versionbits 0c471a5f30

Sjors force-pushed on Mar 3, 2021

Sjors commented at 1:15 pm on March 3, 2021: member

It’s now based on based on 831675c, using the switch statement suggested above, and using height + 1 again. @luke-jr so if I do a followup on this commit, I should base it on 0c471a5f306044cbd2eb230714571f05dd6aaf3c rather than the merge commit?

luke-jr commented at 0:04 am on March 5, 2021: member

@Sjors Due to conflicts with the remaining BIP 8 code, followups may need to be based on the merge commit - but in ideal circumstances, yes, it would be best to have kept it mergable in both branches where practical.

ajtowns commented at 8:54 am on March 7, 2021: member

ACK 0c471a5f306044cbd2eb230714571f05dd6aaf3c

This is a test only change that’s useful for both bip9 and bip8, so it would be good to get this merged quickly I think.

MarcoFalke renamed this:
~~Additional (refactored) BIP9 tests~~
test: Additional (refactored) BIP9 tests
on Mar 7, 2021

MarcoFalke commented at 11:47 am on March 7, 2021: member

review ACK 0c471a5f306044cbd2eb230714571f05dd6aaf3c 🔓

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 0c471a5f306044cbd2eb230714571f05dd6aaf3c 🔓
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgISgwAyQvxPB7TAQwnAhABJgr2zwO7XYdx0ZMShKIt1rTw7HvCzqWqlY1vfun3
 8Ntq7sj03ee/bW2ga/EmApwzMlzObC1BTi3oD1MKnhe9KegvqOxo7uBy0RH5OMxtg
 9ywgM4ycrA2zLt2g95Mi1atAmas85xjXeHH3T3a4+GTXsNvVRXMpGO50Z4VACNeqQ
10GXcccQitq3IXt4fvzEDouOurjUpyAnJ4LxJ4Nq5CasgEtqN5MtQC09J/h//LZ+8N
11wbozI23bNmthW8eJRGIpzOnM0FvBGFprX34L08sww4Qohvpv9Ed0vuSGW0vTynFD
12UiBHuiJe+v8EXhS0Hhklemg3aDOnhOFv2cPFsUQQ4A/irNBTYCj1wZ/azMsJcv4l
13xBb/s5UYvG/wzjYeC5pMQq58SQDSDGI+glIUUgdmuPUZPIk7t5j+W5a1P5djoOw/
14/YsJgVzcaS3kfJjKBYfNMbTzYYNWrMxq+lVTjGPx6aoKQ6z2djYX5aCfLpaH2fX1
15KVNqcC3J
16=paOz
17-----END PGP SIGNATURE-----

Timestamp of file with hash 7eb1f34d6f69c582bcce82dc0a088637be492c5d3e060d5f19d5fac018087fee -

MarcoFalke merged this on Mar 7, 2021

MarcoFalke closed this on Mar 7, 2021

sidhujag referenced this in commit 17f4c4d8cc on Mar 7, 2021

Sjors deleted the branch on Mar 7, 2021

MarcoFalke referenced this in commit 1bad33f952 on Mar 21, 2021

DrahtBot locked this on Aug 16, 2022

test: Additional (refactored) BIP9 tests #21334

Conflicts