test: Reduce number of blocks generated in rpc_signrawtransaction #22542

Reduce the number of blocks generated to make sure CSV is activated in test_signing_with_cltv from 1500 to 500. This makes it consistent with the test above it, test_signing_with_csv.

Also, generating 1500 blocks in one go takes longer than the alotted 30 seconds and times out consistently on Sifive Unmatched.

I think the number is okay, but it is the comment that is wrong here? I.e. s/Make sure CSV is active/Make sure CLTV is active/

Improvement ideas for the sub-tests test_signing_with_{csv,cltv}:

• use constants CSV_ACTIVATION_HEIGHT, CLTV_HEIGHT instead of magic numbers 500, 1500
• check if CSV and CLTV have been really activated by asserting getblockchaininfo()['softforks'] after generating the blocks...

Thanks, I've been seeing this timeout as well and have been cherry-picking this commit https://github.com/jonatack/bitcoin/commit/e99f9b28abd27dbe36b178227b9441fb234fe812 as a workaround, as I wasn't sure if the number of blocks could be reduced. The code was added in a97a9298c, @achow101 thoughts?

Thanks for the suggestions @theStack @jonatack will update with that.

use constants CSV_ACTIVATION_HEIGHT, CLTV_HEIGHT instead of magic numbers 500, 1500

SGTM (but both would be 500?).

I sometimes wonder if we can do some kind of caching, pre-generate block chains with mechanisms activated, and share them among tests to save time (out of scope for this PR imo).

I think we should activate all soft forks as early as possible in regtest. I am working on this, but progress is slow: #16333

use constants CSV_ACTIVATION_HEIGHT, CLTV_HEIGHT instead of magic numbers 500, 1500

SGTM (but both would be 500?).

The constants I was referring to already exist, though they are not in a common module yet:

test/functional/feature_csv_activation.py:CSV_ACTIVATION_HEIGHT = 432
test/functional/feature_cltv.py:CLTV_HEIGHT = 1351

matching the corresponding regtest chain parameters in src/chainparams.cpp:

consensus.CSVHeight = 432; // CSV activated on regtest (Used in rpc activation tests)
consensus.BIP65Height = 1351; // BIP65 activated on regtest (Used in functional tests)

I don't think after changing from 1500 to 500 blocks the test does what it is supposed to do anymore -- CLTV is not activated then, so OP_CLTV just behaves like a NOP. To proof, checking whether CLTV was activated with the following assertion succeeds on master, but fails in this PR:

assert(self.nodes[0].getblockchaininfo()['softforks']['bip65']['active'])

I don't think after changing from 1500 to 500 blocks the test does what it is supposed to do anymore

Okay, I'll close this and disable the test locally until something like #16333.

I guess one way to make this work more reliably on systems that cannot generate the amount of blocks within the timeout would be to do multiple calls to generatetoaddress. I have no idea why generating empty blocks is so slow in the first place though.

There is also a timeout factor option, which can be used to scale all timeouts on slower systems (valgrind, VM, CI, ...)