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

theStack commented at 11:11 PM on July 25, 2021: member

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#pullrequestreview-714297804).

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 #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

test: check that CSV/CLTV are active in rpc_signrawtransaction

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()`.

e3237b1cd0

DrahtBot added the label Tests on Jul 26, 2021

laanwj commented at 5:37 AM on July 26, 2021: member

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

rajarshimaitra commented at 6:23 AM on July 26, 2021: contributor

tACK https://github.com/bitcoin/bitcoin/pull/22550/commits/4f620605604fd5a1161bf01b60a2cf095044afd0

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.

in test/functional/test_framework/util.py:571 in 4f62060560 outdated

 567 | +    current_height = node.getblockcount()
 568 | +    while current_height < target_height:
 569 | +        nblocks = max(200, target_height - current_height)
 570 | +        node.generatetoaddress(nblocks=nblocks, address=address)
 571 | +        current_height = node.getblockcount()
 572 | +

MarcoFalke commented at 8:06 AM on July 26, 2021:

Could write shorter and add an assert?

        node.generate(nblocks)
        current_height = node.getblockcount()
    assert_equal(target_height, node.getblockcount())

laanwj commented at 8:25 AM on July 26, 2021:

Couldn't we just as well remove the getblockcount within the loop, then assert afterward? e.g. is there any harm in asserting that nothing else is going to generate blocks behind our back?

theStack commented at 1:32 PM on July 26, 2021:

@MarcoFalke: Thanks, done. @laanwj: If I understood you correctly, we'd have current_height += nblocks in the loop and call the getblockcount RPC only twice -- once before the loop to initialize current_height, and another time after the loop for the assertion, right? That sounds reasonable to me, will try that out later.

in test/functional/rpc_signrawtransaction.py:8 in 4f62060560 outdated

   3 | @@ -4,6 +4,8 @@
   4 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 |  """Test transaction signing using the signrawtransaction* RPCs."""
   6 |  
   7 | +from feature_cltv import CLTV_HEIGHT
   8 | +from feature_csv_activation import CSV_ACTIVATION_HEIGHT

laanwj commented at 8:30 AM on July 26, 2021:

No strong opinion but I think moving these constants to blocktools would make some sense; as COINBASE_MATURITY is already there, and all three tests using them already import from there.

theStack commented at 1:26 PM on July 26, 2021:

Thanks, that seems like a good place to put them. Done.

theStack force-pushed on Jul 26, 2021

theStack commented at 1:28 PM on July 26, 2021: member

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

test: introduce `generate_to_height` helper, use in rpc_signrawtransaction

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

746f203f19

test: use constants for CSV/CLTV activation heights in rpc_signrawtransaction 12f094ec21

theStack force-pushed on Jul 26, 2021

theStack commented at 10:23 PM on July 26, 2021: member

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 commented at 4:42 AM on July 27, 2021: contributor

reACK https://github.com/bitcoin/bitcoin/pull/22550/commits/12f094ec215aacf30e4e380c0399f80d4e45c345

in test/functional/test_framework/util.py:569 in 12f094ec21

 564 | +       To prevent timeouts, only up to 200 blocks are generated per RPC call.
 565 | +       Can be used to activate certain soft-forks (e.g. CSV, CLTV)."""
 566 | +    current_height = node.getblockcount()
 567 | +    while current_height < target_height:
 568 | +        nblocks = min(200, target_height - current_height)
 569 | +        current_height += len(node.generate(nblocks))

laanwj commented at 12:02 PM on July 27, 2021:

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 commented at 12:02 PM on July 27, 2021: member

Code review and tested ACK 12f094ec215aacf30e4e380c0399f80d4e45c345

DrahtBot commented at 12:46 PM on July 27, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22567 by MarcoFalke
#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.

MarcoFalke merged this on Jul 28, 2021

MarcoFalke closed this on Jul 28, 2021

in test/functional/test_framework/blocktools.py:60 in 12f094ec21

  54 | @@ -55,6 +55,10 @@
  55 |  # Coinbase transaction outputs can only be spent after this number of new blocks (network rule)
  56 |  COINBASE_MATURITY = 100
  57 |  
  58 | +# Soft-fork activation heights
  59 | +CLTV_HEIGHT = 1351
  60 | +CSV_ACTIVATION_HEIGHT = 432

jonatack commented at 12:57 PM on July 28, 2021:

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

jonatack commented at 12:58 PM on July 28, 2021: member

Posthumous ACK 12f094ec215aacf30e4e380c0399f80d4e45c345

sidhujag referenced this in commit d4ceafb5e7 on Jul 28, 2021

theStack deleted the branch on Jul 31, 2021

DrahtBot locked this on Aug 16, 2022