Following a suggestion in the comments, changed ValidateCheckInputsForAllFlags from testing all possible flag combinations to testing a random subset. Also created a new enum constant for the highest flag, so that this test doesn’t keep testing an incomplete subset in case a new flag is added.
Timing for checkinputs_test:
0Before: 6.8s
1After: 3.7s
2----------------
3Saved: 3.1s (45%)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
109- // rewrite in the future to randomly pick a set of flags to evaluate.
110- for (uint32_t test_flags=0; test_flags < (1U << 16); test_flags += 1) {
111+
112+ for (uint32_t count=0; count < 10000; count++) {
113+ // Randomly selects flag combinations
114+ uint32_t test_flags = rand() % (HIGHEST_FLAG << 1);
rand() anywhere in the code, would be preferable to use a FastRandomContext insecure_rand(true); here, also for determinism - for reproducible it is undesirable for different test runs to test a different subset of flags
111+
112+ FastRandomContext insecure_rand(true);
113+
114+ for (uint32_t count=0; count < 10000; count++) {
115+ // Randomly selects flag combinations
116+ uint32_t test_flags = insecure_rand.randrange(HIGHEST_FLAG << 1);
uint64_t to uint32_t explicit :-)
randrange return a uint64_t, but CheckInputsaccepts aunsigned int, while the flags are actually an enum which has an implementation-specific type. I guess the best is to change it here to unsigned int` (and make the conversion explicit), and maybe open a separate PR for fixing these type inconsistencies.
113@@ -114,6 +114,12 @@ enum
114 // Making OP_CODESEPARATOR and FindAndDelete fail any non-segwit scripts
115 //
116 SCRIPT_VERIFY_CONST_SCRIPTCODE = (1U << 16),
117+
118+
119+ // Constants to point to the highest flag in use. Add new flags above this line.
120+ //
121+ HIGHEST_FLAG_PLUS_ONE,
122+ HIGHEST_FLAG = HIGHEST_FLAG_PLUS_ONE - 1
SCRIPT_VERIFY_ as well. Otherwise ever single translation unit including script/interpreter.h will have a bare HIGHEST_FLAG {_PLUS_ONE} constant in its global namespace and there isn’t any reference in the name to which flags it refers to.
115- // rewrite in the future to randomly pick a set of flags to evaluate.
116- for (uint32_t test_flags=0; test_flags < (1U << 16); test_flags += 1) {
117+
118+ FastRandomContext insecure_rand(true);
119+
120+ for (uint32_t count=0; count < 10000; count++) {
int.
119+
120+ for (uint32_t count=0; count < 10000; count++) {
121 TxValidationState state;
122+
123+ // Randomly selects flag combinations
124+ unsigned int test_flags = (unsigned int) insecure_rand.randrange(HIGHEST_FLAG << 1);
uint32_t before, so I’d also that type here now rather than the more general unsigned int (in practice it won’t matter though, so feel free to ignore).
tx_validationcache_tests takes less than 2 seconds on my machine), the execution time for the validation loop would obviously increase exponentially with each script-verify-flag added.
Concept ACK Though on the current master branch enumerating through all possible script-verify-flags combinations is still feasible (running
tx_validationcache_teststakes less than 2 seconds on my machine), the execution time for the validation loop would obviously increase exponentially with each script-verify-flag added.
Hi, @theStack. Thank you for reviewing this PR. I agree with your comments and will implement them. Just a little bit busy, but hopefully will have some time for that in the next few weeks.
thanks!
113@@ -114,6 +114,12 @@ enum
114 // Making OP_CODESEPARATOR and FindAndDelete fail any non-segwit scripts
115 //
116 SCRIPT_VERIFY_CONST_SCRIPTCODE = (1U << 16),
117+
118+
113@@ -114,6 +114,12 @@ enum
114 // Making OP_CODESEPARATOR and FindAndDelete fail any non-segwit scripts
115 //
116 SCRIPT_VERIFY_CONST_SCRIPTCODE = (1U << 16),
117+
118+
119+ // Constants to point to the highest flag in use. Add new flags above this line.
120+ //
121+ SCRIPT_VERIFY_HIGHEST_FLAG_PLUS_ONE,
122+ SCRIPT_VERIFY_HIGHEST_FLAG = SCRIPT_VERIFY_HIGHEST_FLAG_PLUS_ONE - 1
SCRIPT_VERIFY_END_MARKER, and then use < SCRIPT_VERIFY_END_MARKER logic instead of this, i.e. only add one.
113- // rewrite in the future to randomly pick a set of flags to evaluate.
114- for (uint32_t test_flags=0; test_flags < (1U << 16); test_flags += 1) {
115+
116+ FastRandomContext insecure_rand(true);
117+
118+ for (int count=0; count < 10000; count++) {
Style-nit: spacing, ++ before varname:
0for (int count = 0; count < 10000; ++count) {
117+
118+ for (int count=0; count < 10000; count++) {
119 TxValidationState state;
120+
121+ // Randomly selects flag combinations
122+ uint32_t test_flags = (uint32_t) insecure_rand.randrange(SCRIPT_VERIFY_HIGHEST_FLAG << 1);
Using the marker style, to get the same you’d randrange((SCRIPT_VERIFY_END_MARKER - 1) << 1) here.
However, I’m not sure why you are doing << 1 here (1U << 17), as the original code only goes up to SCRIPT_VERIFY_END_MARKER - 1 (i.e. (1U << 16)). To retain the same, this would be randrange(SCRIPT_VERIFY_END_MARKER - 1);
1111111111111111 (1«17 -1). Because of the range being exclusive of the highest value, I pass 1<<17 = (SCRIPT_VERIFY_END_MARKER - 1) << 1
Hope that clarifies.
117+
118+ for (int count = 0; count < 10000; ++count) {
119 TxValidationState state;
120+
121+ // Randomly selects flag combinations
122+ uint32_t test_flags = (uint32_t) insecure_rand.randrange((SCRIPT_VERIFY_END_MARKER - 1) << 1);
You want to swap the - and «; right now you get 111...0, i.e.
0 uint32_t test_flags = (uint32_t) insecure_rand.randrange((SCRIPT_VERIFY_END_MARKER << 1) - 1);
Following a suggestion in the comments, changed `ValidateCheckInputsForAllFlags` from testing all possible flag combinations to testing a random subset. Also created a new enum constant for the highest flag, so that this test doesn’t keep testing an incomplete subset in case a new flag is added.
Rebased. @kallewoof would you mind ACK-ing since I addressed your comments (or otherwise make further comments)?
I think this PR has become (more) important after the new taproot flags were introduced – since none of those flags are tested by this test case in master.
tACK https://github.com/bitcoin/bitcoin/pull/13533/commits/c3e111a7daf5800026dda4455c737de0412528f1. Tested on Ubuntu 20.04 on VMWAre.
Average tests execution time: . Master branch: 1.02s . This PR branch: 0.60s
These values have remained consistent across several runs.
Thanks everyone!
I know reviewing PRs is a lot of work, and this one wasn’t a priority – so I’m really glad to see it finally merged.
I learned a lot from writing it and from your feedback!