Skip to content

test: Additional (refactored) BIP9 tests #21334

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 7, 2021
Merged

test: Additional (refactored) BIP9 tests #21334

merged 2 commits into from
Mar 7, 2021

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Mar 2, 2021 •
edited
Loading

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

I added some comments to the test: bae9a45#r585486781

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

@Sjors
Copy link
Member Author

Sjors commented Mar 2, 2021 •
edited
Loading

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

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

@DrahtBot DrahtBot added the Tests label Mar 2, 2021
@michaelfolkson
Copy link

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
Copy link
Member Author

Sjors commented Mar 2, 2021 •
edited
Loading

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

maflcko
maflcko reviewed Mar 2, 2021
View reviewed changes
src/test/versionbits_tests.cpp Outdated
state == ThresholdState::LOCKED_IN ? "LOCKED_IN" :
state == ThresholdState::ACTIVE ? "ACTIVE" :
state == ThresholdState::FAILED ? "FAILED" :
"");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC it's to remain compatible with C++11: bae9a45#r579334677

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

static const std::string StateName(ThresholdState state)
{
    switch (state) {
    case ThresholdState::DEFINED:   return "DEFINED";
    case ThresholdState::STARTED:   return "STARTED";
    case ThresholdState::LOCKED_IN: return "LOCKED_IN";
    case ThresholdState::ACTIVE:    return "ACTIVE";
    case ThresholdState::FAILED:    return "FAILED";
    } // no default case, so the compiler can warn about missing cases
    return "";
}

looks fine to me fwiw.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two birds, one stone...

@ajtowns
Copy link
Contributor

ajtowns commented Mar 2, 2021

Concept ACK for splitting commits out.

@luke-jr
Copy link
Member

luke-jr commented Mar 2, 2021

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 2, 2021 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot mentioned this pull request Mar 3, 2021
Closed
darosior
darosior reviewed Mar 3, 2021
View reviewed changes
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@Sjors Sjors force-pushed the 2021/03/bip9_tests branch from 6aeb212 to 0c471a5 Compare March 3, 2021 13:15
@Sjors
Copy link
Member Author

Sjors commented Mar 3, 2021 •
edited
Loading

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 0c471a5 rather than the merge commit?

@luke-jr
Copy link
Member

luke-jr commented Mar 5, 2021

@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 ajtowns mentioned this pull request Mar 6, 2021
Merged
ajtowns
ajtowns reviewed Mar 7, 2021
View reviewed changes
Copy link
Contributor

@ajtowns ajtowns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0c471a5

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.

@maflcko maflcko changed the title Additional (refactored) BIP9 tests test: Additional (refactored) BIP9 tests Mar 7, 2021
maflcko
maflcko reviewed Mar 7, 2021
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review ACK 0c471a5 🔓

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 0c471a5f306044cbd2eb230714571f05dd6aaf3c 🔓
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgISgwAyQvxPB7TAQwnAhABJgr2zwO7XYdx0ZMShKIt1rTw7HvCzqWqlY1vfun3
Ntq7sj03ee/bW2ga/EmApwzMlzObC1BTi3oD1MKnhe9KegvqOxo7uBy0RH5OMxtg
ywgM4ycrA2zLt2g95Mi1atAmas85xjXeHH3T3a4+GTXsNvVRXMpGO50Z4VACNeqQ
GXcccQitq3IXt4fvzEDouOurjUpyAnJ4LxJ4Nq5CasgEtqN5MtQC09J/h//LZ+8N
wbozI23bNmthW8eJRGIpzOnM0FvBGFprX34L08sww4Qohvpv9Ed0vuSGW0vTynFD
UiBHuiJe+v8EXhS0Hhklemg3aDOnhOFv2cPFsUQQ4A/irNBTYCj1wZ/azMsJcv4l
xBb/s5UYvG/wzjYeC5pMQq58SQDSDGI+glIUUgdmuPUZPIk7t5j+W5a1P5djoOw/
/YsJgVzcaS3kfJjKBYfNMbTzYYNWrMxq+lVTjGPx6aoKQ6z2djYX5aCfLpaH2fX1
KVNqcC3J
=paOz
-----END PGP SIGNATURE-----

Timestamp of file with hash 7eb1f34d6f69c582bcce82dc0a088637be492c5d3e060d5f19d5fac018087fee -

@maflcko maflcko merged commit 1020b04 into bitcoin:master Mar 7, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 7, 2021
Merge bitcoin#21334: test: Additional (refactored) BIP9 tests
17f4c4d 
0c471a5 tests: check never active versionbits (Anthony Towns)
3ba9283 tests: more helpful errors for failing versionbits tests (Anthony Towns)

Pull request description:

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

  I added some comments to the test: bitcoin@bae9a45#r585486781

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

ACKs for top commit:
  ajtowns:
    ACK 0c471a5
  MarcoFalke:
    review ACK 0c471a5 🔓

Tree-SHA512: 61f8d1ecaf38a6cd13db1cf71c89b8c4d2f5852ef77c5e7ecb9bd78eb216545037411641bb101cf0740c5c47845ac327954ee25b676d63779c5f148719ac5caf
@Sjors Sjors deleted the 2021/03/bip9_tests branch March 7, 2021 16:26
@ajtowns ajtowns mentioned this pull request Mar 21, 2021
Merged
maflcko pushed a commit that referenced this pull request Mar 21, 2021
Merge #21490: [0.21] Backport versionbits tests
1bad33f 
e775b0a tests: Add fuzzing harness for versionbits (Anthony Towns)
0c471a5 tests: check never active versionbits (Anthony Towns)
3ba9283 tests: more helpful errors for failing versionbits tests (Anthony Towns)

Pull request description:

  Backport of unit test (#21334) and fuzz test (#21380) changes for versionbits.

Top commit has no ACKs.

Tree-SHA512: b68b570e48e0076bb2ade3b91c59612029235d2c9e39048d548aa141fa0906343fa492e9a981065fbdbbebecbbb3dcbaf39ec69228c7581178fcca567e8201b8
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
4 more reviewers

@ajtowns ajtowns ajtowns left review comments

@luke-jr luke-jr luke-jr left review comments

@maflcko maflcko maflcko left review comments

@darosior darosior darosior left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Sjors @michaelfolkson @ajtowns @luke-jr @DrahtBot @maflcko @darosior