Skip to content

test: Default to running -checkblockindex in test_framework (TheBlueMatt) #13912

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
Closed
wants to merge 1 commit into from
Closed

test: Default to running -checkblockindex in test_framework (TheBlueMatt) #13912

wants to merge 1 commit into from

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Aug 8, 2018

Salvaged from #12138, and adjusted to current test framework conventions.

Block index inconsistencies are one of the most critical types of
bugs we could see. Making sure we get good coverage of
CheckBlockIndex() is pretty important (and there is minimal, if any
performance difference in a default run).

Test suite on master takes 928-988 seconds on my machine, with this change it takes 901-1045 seconds (n=2, and not a perfect experimental setup).

@TheBlueMatt @Sjors
Default to running -checkblockindex in test_framework
9f47bde 
Block index inconsistencies are one of the most critical types of
bugs we could see. Making sure we get good coverage of
CheckBlockIndex() is pretty important (and there is minimal, if any
performance difference in a default run).
@Sjors
Copy link
Member Author

Sjors commented Aug 8, 2018

cc @TheBlueMatt

@maflcko
Copy link
Member

maflcko commented Aug 8, 2018

utACK 9f47bde

Note that we enable tx pool consistency checks for regtest by default and regtest is the network used by the test framework. So an alternative could be to enable it by default for regtest. Though, the current patch is already a clear improvement.

For reference the help text of the bool arg:

  -checkblockindex
       Do a full consistency check for mapBlockIndex, setBlockIndexCandidates,
       chainActive and mapBlocksUnlinked occasionally. (default: 0)

@maflcko maflcko added the Tests label Aug 8, 2018
@maflcko maflcko changed the title Default to running -checkblockindex in test_framework test: Default to running -checkblockindex in test_framework Aug 8, 2018
@maflcko maflcko changed the title test: Default to running -checkblockindex in test_framework test: Default to running -checkblockindex in test_framework (TheBlueMatt) Aug 8, 2018
@domob1812
Copy link
Contributor

I agree with @MarcoFalke - ideally, these checks should be enabled for regtest net automatically, similar to the mempool checks. That seems much cleaner to me than enabling them using command-line arguments (because those can be forgotten / acidentally removed more easily).

Concept ACK to running the checks regularly on regtest.

@maflcko maflcko mentioned this pull request Aug 8, 2018
Merged
@Sjors Sjors mentioned this pull request Aug 10, 2018
Closed
@Empact
Copy link
Contributor

Empact commented Aug 11, 2018 •
edited
Loading

As noted by @MarcoFalke in #13913, checkblockindex is active by default on regtest.

bitcoin/src/chainparams.cpp

Line 328 in d6faea4

fDefaultConsistencyChecks = true;

maflcko pushed a commit that referenced this pull request Aug 11, 2018
Merge #13913: qa: Remove redundant checkmempool/checkblockindex extra…
a08533c 
…_args

fa31ca0 qa: Remove redundant checkmempool/checkblockindex extra_args (MarcoFalke)

Pull request description:

  They are already enabled by default for regtest:

  https://github.com/bitcoin/bitcoin/blob/df9f71274645a917e2578c52a1c59745bce8112d/src/init.cpp#L1002-L1007

  Closes  #13912. CC #12138

Tree-SHA512: b11a3e8cc4715569f917ab89132f8c8dcae64aebcd7a34182675f86cf7f6e207e3187b7ea01a56c92c8c3af76122b6b995e84f533e134676863f8953dc1f0574
@Sjors Sjors deleted the 2018/08/test-checkblockindex branch August 11, 2018 11:34
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
@PastaPastaPasta
Merge bitcoin#13913: qa: Remove redundant checkmempool/checkblockinde…
81bf95e 
…x extra_args

fa31ca0 qa: Remove redundant checkmempool/checkblockindex extra_args (MarcoFalke)

Pull request description:

  They are already enabled by default for regtest:

  https://github.com/bitcoin/bitcoin/blob/df9f71274645a917e2578c52a1c59745bce8112d/src/init.cpp#L1002-L1007

  Closes  bitcoin#13912. CC bitcoin#12138

Tree-SHA512: b11a3e8cc4715569f917ab89132f8c8dcae64aebcd7a34182675f86cf7f6e207e3187b7ea01a56c92c8c3af76122b6b995e84f533e134676863f8953dc1f0574
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
@PastaPastaPasta
Merge bitcoin#13913: qa: Remove redundant checkmempool/checkblockinde…
2341749 
…x extra_args

fa31ca0 qa: Remove redundant checkmempool/checkblockindex extra_args (MarcoFalke)

Pull request description:

  They are already enabled by default for regtest:

  https://github.com/bitcoin/bitcoin/blob/df9f71274645a917e2578c52a1c59745bce8112d/src/init.cpp#L1002-L1007

  Closes  bitcoin#13912. CC bitcoin#12138

Tree-SHA512: b11a3e8cc4715569f917ab89132f8c8dcae64aebcd7a34182675f86cf7f6e207e3187b7ea01a56c92c8c3af76122b6b995e84f533e134676863f8953dc1f0574
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@PastaPastaPasta
Merge bitcoin#13913: qa: Remove redundant checkmempool/checkblockinde…
5edf469 
…x extra_args

fa31ca0 qa: Remove redundant checkmempool/checkblockindex extra_args (MarcoFalke)

Pull request description:

  They are already enabled by default for regtest:

  https://github.com/bitcoin/bitcoin/blob/df9f71274645a917e2578c52a1c59745bce8112d/src/init.cpp#L1002-L1007

  Closes  bitcoin#13912. CC bitcoin#12138

Tree-SHA512: b11a3e8cc4715569f917ab89132f8c8dcae64aebcd7a34182675f86cf7f6e207e3187b7ea01a56c92c8c3af76122b6b995e84f533e134676863f8953dc1f0574
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@PastaPastaPasta
Merge bitcoin#13913: qa: Remove redundant checkmempool/checkblockinde…
d42488e 
…x extra_args

fa31ca0 qa: Remove redundant checkmempool/checkblockindex extra_args (MarcoFalke)

Pull request description:

  They are already enabled by default for regtest:

  https://github.com/bitcoin/bitcoin/blob/df9f71274645a917e2578c52a1c59745bce8112d/src/init.cpp#L1002-L1007

  Closes  bitcoin#13912. CC bitcoin#12138

Tree-SHA512: b11a3e8cc4715569f917ab89132f8c8dcae64aebcd7a34182675f86cf7f6e207e3187b7ea01a56c92c8c3af76122b6b995e84f533e134676863f8953dc1f0574
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@PastaPastaPasta
Merge bitcoin#13913: qa: Remove redundant checkmempool/checkblockinde…
fa57557 
…x extra_args

fa31ca0 qa: Remove redundant checkmempool/checkblockindex extra_args (MarcoFalke)

Pull request description:

  They are already enabled by default for regtest:

  https://github.com/bitcoin/bitcoin/blob/df9f71274645a917e2578c52a1c59745bce8112d/src/init.cpp#L1002-L1007

  Closes  bitcoin#13912. CC bitcoin#12138

Tree-SHA512: b11a3e8cc4715569f917ab89132f8c8dcae64aebcd7a34182675f86cf7f6e207e3187b7ea01a56c92c8c3af76122b6b995e84f533e134676863f8953dc1f0574
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@PastaPastaPasta
Merge bitcoin#13913: qa: Remove redundant checkmempool/checkblockinde…
f33c2c8 
…x extra_args

fa31ca0 qa: Remove redundant checkmempool/checkblockindex extra_args (MarcoFalke)

Pull request description:

  They are already enabled by default for regtest:

  https://github.com/bitcoin/bitcoin/blob/df9f71274645a917e2578c52a1c59745bce8112d/src/init.cpp#L1002-L1007

  Closes  bitcoin#13912. CC bitcoin#12138

Tree-SHA512: b11a3e8cc4715569f917ab89132f8c8dcae64aebcd7a34182675f86cf7f6e207e3187b7ea01a56c92c8c3af76122b6b995e84f533e134676863f8953dc1f0574
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@PastaPastaPasta
Merge bitcoin#13913: qa: Remove redundant checkmempool/checkblockinde…
b04090d 
…x extra_args

fa31ca0 qa: Remove redundant checkmempool/checkblockindex extra_args (MarcoFalke)

Pull request description:

  They are already enabled by default for regtest:

  https://github.com/bitcoin/bitcoin/blob/df9f71274645a917e2578c52a1c59745bce8112d/src/init.cpp#L1002-L1007

  Closes  bitcoin#13912. CC bitcoin#12138

Tree-SHA512: b11a3e8cc4715569f917ab89132f8c8dcae64aebcd7a34182675f86cf7f6e207e3187b7ea01a56c92c8c3af76122b6b995e84f533e134676863f8953dc1f0574
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Sjors @maflcko @domob1812 @Empact @TheBlueMatt