test: Allow to set height in create_block #35089

eeee853 test: Allow to set height in create_block:

Can we add a simple feature_framework_unit_tests.py test here to exercise this new fallback behavior?

class TestFrameworkBlockTools(unittest.TestCase):
    def test_create_block_prefers_explicit_height(self):
        block = create_block(
            hashprev=1,
            tmpl={"height": 100, "curtime": 2},
            height=200,
        )
        assert_equal(CScriptNum.decode(block.vtx[0].vin[0].scriptSig), 200)

Nit: this is safe because genesis isn't passed here so a height number won't ever be falsy, right?

Nit: this is safe because genesis isn't passed here so a height number won't ever be falsy, right?

Correct. I am confident that it is neither possible, nor meaningful, to try to re-create the genesis block here.

feature_framework_unit_tests

I think all of this is checked by the Bitcoin Core consensus code already, so there shouldn't be a need for a unit test. If you insist, and send a full diff or a full commit that I can push as-is, I may consider it.

send a full diff or a full commit that I can push as-is, I may consider it

I actually meant just the create_block change here, which can now choose between two provided height values:

diff --git a/test/functional/test_framework/blocktools.py b/test/functional/test_framework/blocktools.py
--- a/test/functional/test_framework/blocktools.py	(revision 22ca336e09824c347e89348166d3208ac04d3693)
+++ b/test/functional/test_framework/blocktools.py	(date 1776336206046)
@@ -278,6 +278,14 @@
     return node.sendrawtransaction(tx_to_witness)
 
 class TestFrameworkBlockTools(unittest.TestCase):
+    def test_create_block_prefers_explicit_height(self):
+        block = create_block(
+            hashprev=1,
+            tmpl={"height": 100, "curtime": 2},
+            height=200,
+        )
+        assert_equal(CScriptNum.decode(block.vtx[0].vin[0].scriptSig), 200)
+
     def test_create_coinbase(self):
         height = 20
         coinbase_tx = create_coinbase(height=height)

ah nice. Pushed something similar to your diff.

eeee853 test: Allow to set height in create_block:

A positional arg was changed to a named one here, for consistency we could specify hashprev as well:

        block = create_block(hashprev=int(self.nodes[0].getbestblockhash(), 16), height=self.nodes[0].getblockcount() + 1, txlist=txs_to_add)

Hmm, I think there is no risk here, because it is followed by coinbase, which is a different type. It being followed by coinbase (a positional arg) also means a scripted-diff can not be used here. So I think I'll leave this as-is for now?

which is a different type

You trust python more than I do :p

I am happy to push a new commit with a scripted-diff (or so), or review a follow-up :)

fa37471 test: Require named arg for create_block ntime arg:

I'm fine with this as well, but the previous block_time = prev_median_time + 1 also made sense since it avoided duplicating the calculation of the prev

I just think that height and time should follow the same pattern. Every time I read this function, I spend too long thinking about this, because it mixes two patterns. No strong opinion, I just think inside this function the same patter should be used consistently.

I understand, given that + 1 is a trivial calculation, it's also fine to duplicate it

fa49662 test: Remove unused create_coinbase imports:

nit: since we're modifying these imports manually we might as well simplify them - in other cases a single create_block import is a one-liner.

I think either is fine. I think black will leave this as-is, whereas yapf prefers a single line. Not sure about ruff ... Generally, I think either both are acceptable (as-is now), or a linter should take care of this somehow, if it was important.