test: improve `test_signing_with_{csv,cltv}` subtests (speed, prevent timeout) #22550

theStack · 2021-07-25T23:11:00Z

This PR primarily aims to solve the current RPC timeout problem for test rpc_signrawtransaction.py, as described in #22542. In the course of that the test is also improved in other ways (see #22542 (review)).

Reviewers guideline:

In test_signing_with_cltv(), a comment is fixed -- it wrongly referred to CSV, it should be CLTV.
As preparation, assertions are added that ensure that CSV and CLTV have been really activated after generating blocks by checking the 'softforks' output of the getblockchaininfo() RPC. Right now in master, one could remove (or decrease, like in test: Reduce number of blocks generated in rpc_signrawtransaction #22542) the generate calls and the test would still pass, when it shouldn't.
A helper generate_to_height() is introduced which improves the previous way of reaching a block height in two ways:
- instead of blindly generating TH blocks to reach target block height >= TH, the current block height CH is taken into account, and only (TH - CH) are generated in total
- to avoid potential RPC timeouts, the block generation is split up into multiple generatetoaddress RPC calls (as suggested by laanwj); here chunks of 200 blocks have been chosen
The helper is used in the affected sub-tests, which should both speed-up the test (from ~18s to ~12s on my machine) and avoid potential timeouts
Finally, the activation constants for CSV and CLTV are used instead of using magic numbers 500 and 1500

Open questions:

Any good naming alternatives for generate_to_height()? Not really happy with the name, happy to hear suggestions
Where to put the CSV and CLTV activation height constants in the test_framewor folder? I guess importing constants from other tests isn't really the desired way to go

Without this check, the tests would also pass if the CSV and CLTV activation heights are not reached yet (e.g. if the .generate() calls before are removed), as the operations OP_CSV and OP_CLTV simply behave as NOPs. Also fixes a comment in the sub-test `test_signing_with_cltv()`.

laanwj · 2021-07-26T05:37:59Z

Concept and tested ACK. All tests pass now. Fixes #22542.

rajarshimaitra

tACK 4f62060

Code change looks clear.

generate_to_height seems like a reasonable name to me. Maybe another one can be generate_blocks_upto.

Maybe a better place to have the activation height constants is in test_frameowrk/blocktools.py? It seems that the module already contains some constants regarding block height, so could be a natural place for them.

test/functional/test_framework/util.py

test/functional/rpc_signrawtransaction.py

theStack · 2021-07-26T13:28:53Z

Force-pushed with the following changes:

fixed a logical flaw in the generate_to_height: instead of calling generate with at most 200 blocks, it was called with a minimum of 200 blocks (changed max(...) to min(...) now)
using the test-framework generate helper instead of generatetoaddress RPC, also add an assert after, as suggested by MarcoFalke
put the CSV/CLTV soft-fork activation height constants into blocktools.py as suggested by rajarshimaitra and laanwj

…ction This will speed up the test a bit and avoid potential .generate() RPC timeouts (in sub-test `test_signing_with_cltv()`) on slower machines.

…nsaction

theStack · 2021-07-26T22:23:00Z

Force-pushed again, taking laanwj's suggestion of not calling the getblockcount() RPC in the generation loop into account -- I don't think this approach has any drawbacks. The block height is now only fetched twice: once at the start of the function to initialize the current_height variable, and another time at the end for the assertion that checks that we really reached the desired block height.

rajarshimaitra

reACK 12f094e

laanwj · 2021-07-27T12:02:23Z

test/functional/test_framework/util.py

+    current_height = node.getblockcount()
+    while current_height < target_height:
+        nblocks = min(200, target_height - current_height)
+        current_height += len(node.generate(nblocks))


Clever idea. An additional sanity assertion could be that length(result) == nblocks, as it could get in an infinite loop if generate is broken and returns nothing.

laanwj · 2021-07-27T12:02:56Z

Code review and tested ACK 12f094e

DrahtBot · 2021-07-27T12:46:42Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567 by MarcoFalke
test: Set regtest.BIP65Height = 111 to speed up tests #21862 by MarcoFalke

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.

jonatack

Posthumous ACK 12f094e

jonatack · 2021-07-28T12:57:31Z

test/functional/test_framework/blocktools.py


+# Soft-fork activation heights
+CLTV_HEIGHT = 1351
+CSV_ACTIVATION_HEIGHT = 432


It may have been nice to mention the related BIP numbers (65 OP_CHECKLOCKTIMEVERIFY, 112 CHECKSEQUENCEVERIFY) here.

…tests (speed, prevent timeout) 12f094e test: use constants for CSV/CLTV activation heights in rpc_signrawtransaction (Sebastian Falbesoner) 746f203 test: introduce `generate_to_height` helper, use in rpc_signrawtransaction (Sebastian Falbesoner) e3237b1 test: check that CSV/CLTV are active in rpc_signrawtransaction (Sebastian Falbesoner) Pull request description: This PR primarily aims to solve the current RPC timeout problem for test rpc_signrawtransaction.py, as described in bitcoin#22542. In the course of that the test is also improved in other ways (see bitcoin#22542 (review)). Reviewers guideline: * In `test_signing_with_cltv()`, a comment is fixed -- it wrongly referred to CSV, it should be CLTV. * As preparation, assertions are added that ensure that CSV and CLTV have been really activated after generating blocks by checking the 'softforks' output of the getblockchaininfo() RPC. Right now in master, one could remove (or decrease, like in bitcoin#22542) the generate calls and the test would still pass, when it shouldn't. * A helper `generate_to_height()` is introduced which improves the previous way of reaching a block height in two ways: - instead of blindly generating TH blocks to reach target block height >= TH, the current block height CH is taken into account, and only (TH - CH) are generated in total - to avoid potential RPC timeouts, the block generation is split up into multiple generatetoaddress RPC calls ([as suggested by laanwj](bitcoin#22542 (comment))); here chunks of 200 blocks have been chosen * The helper is used in the affected sub-tests, which should both speed-up the test (from ~18s to ~12s on my machine) and avoid potential timeouts * Finally, the activation constants for CSV and CLTV are used instead of using magic numbers 500 and 1500 Open questions: * Any good naming alternatives for `generate_to_height()`? Not really happy with the name, happy to hear suggestions * Where to put the CSV and CLTV activation height constants in the test_framewor folder? I guess importing constants from other tests isn't really the desired way to go ACKs for top commit: laanwj: Code review and tested ACK 12f094e rajarshimaitra: reACK bitcoin@12f094e Tree-SHA512: 14509f6d3e5a5a05d6a30a3145bb82cd96a29d9d8a589abf1944a8bf34291cde78ce711195f52e9426588dc822b3618ec9b455e057360021ae46152bb7613516

DrahtBot added the Tests label Jul 26, 2021

rajarshimaitra reviewed Jul 26, 2021

View reviewed changes

maflcko reviewed Jul 26, 2021

View reviewed changes

test/functional/test_framework/util.py Outdated Show resolved Hide resolved

laanwj reviewed Jul 26, 2021

View reviewed changes

test/functional/rpc_signrawtransaction.py Outdated Show resolved Hide resolved

theStack force-pushed the 202107-test-improve_test_signing_with_csv_cltv_subtests branch from 4f62060 to 125e83f Compare July 26, 2021 13:19

theStack added 2 commits July 27, 2021 00:14

test: introduce generate_to_height helper, use in rpc_signrawtransa…

746f203

…ction This will speed up the test a bit and avoid potential .generate() RPC timeouts (in sub-test `test_signing_with_cltv()`) on slower machines.

test: use constants for CSV/CLTV activation heights in rpc_signrawtra…

12f094e

…nsaction

theStack force-pushed the 202107-test-improve_test_signing_with_csv_cltv_subtests branch from 125e83f to 12f094e Compare July 26, 2021 22:16

rajarshimaitra reviewed Jul 27, 2021

View reviewed changes

laanwj reviewed Jul 27, 2021

View reviewed changes

This was referenced Jul 27, 2021

test: Set regtest.BIP65Height = 111 to speed up tests #21862

Merged

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567

Closed

maflcko merged commit 548ca1d into bitcoin:master Jul 28, 2021

jonatack reviewed Jul 28, 2021

View reviewed changes

theStack deleted the 202107-test-improve_test_signing_with_csv_cltv_subtests branch July 31, 2021 20:03

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: improve `test_signing_with_{csv,cltv}` subtests (speed, prevent timeout) #22550

test: improve `test_signing_with_{csv,cltv}` subtests (speed, prevent timeout) #22550

Uh oh!

theStack commented Jul 25, 2021

Uh oh!

laanwj commented Jul 26, 2021 •

edited

Loading

Uh oh!

rajarshimaitra left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

theStack commented Jul 26, 2021

Uh oh!

theStack commented Jul 26, 2021

Uh oh!

rajarshimaitra left a comment

Uh oh!

laanwj Jul 27, 2021

Uh oh!

laanwj commented Jul 27, 2021

Uh oh!

DrahtBot commented Jul 27, 2021 •

edited

Loading

Uh oh!

jonatack left a comment

Uh oh!

jonatack Jul 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

test: improve test_signing_with_{csv,cltv} subtests (speed, prevent timeout) #22550

test: improve test_signing_with_{csv,cltv} subtests (speed, prevent timeout) #22550

Uh oh!

Conversation

theStack commented Jul 25, 2021

Uh oh!

laanwj commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajarshimaitra left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

theStack commented Jul 26, 2021

Uh oh!

theStack commented Jul 26, 2021

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jul 27, 2021

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

test: improve `test_signing_with_{csv,cltv}` subtests (speed, prevent timeout) #22550

test: improve `test_signing_with_{csv,cltv}` subtests (speed, prevent timeout) #22550

laanwj commented Jul 26, 2021 •

edited

Loading

rajarshimaitra left a comment •

edited

Loading

DrahtBot commented Jul 27, 2021 •

edited

Loading