test: Moved the CScriptNum asserts into the unit test in script.py

gillichu commented at 5:07 pm on May 27, 2020: none

The CScriptNum test (#14816) is a roundtrip test of the test framework. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See #18576.

This PR:

Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py.
Extends the script.py CScriptNum test to trial larger numbers.

MarcoFalke commented at 5:18 pm on May 27, 2020: member

Concept ACK

It would be good to motivate the change. Maybe something like this?

The scriptnum test (https://github.com/bitcoin/bitcoin/pull/14816) is a roundtrip test of the test framwork. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See https://github.com/bitcoin/bitcoin/pull/18576

gillichu renamed this:
~~Moved the CScriptNum asserts into the unit test in script.py~~
[WIP]. Moved the CScriptNum asserts into the unit test in script.py
on May 27, 2020

gillichu renamed this:
~~[WIP]. Moved the CScriptNum asserts into the unit test in script.py~~
[WIP] Moved the CScriptNum asserts into the unit test in script.py
on May 27, 2020

in test/functional/test_framework/script.py:763 in 4b38a1cd1b outdated

758+        # sequence numbers must not be max for nLockTime to have effect
759+        coinbase_tx.vin[0].nSequence = 2 ** 32 - 2
760+        coinbase_tx.rehash()
761+
762+        # round-trip the encoded bip34 block height commitment
763+        self.assert_equal(CScriptNum.decode(coinbase_tx.vin[0].scriptSig), next_height)

MarcoFalke commented at 5:21 pm on May 27, 2020:

I think for a unit test it could make sense to simply hard-code the value of coinbase_tx.vin[0].scriptSig instead of computing it every single time by mining 200 blocks.

gillichu marked this as a draft on May 27, 2020

in test/functional/mining_basic.py:94 in 0107629e29 outdated

87-        # sequence numbers must not be max for nLockTime to have effect
88-        coinbase_tx.vin[0].nSequence = 2 ** 32 - 2
89-        coinbase_tx.rehash()
90-
91-        # round-trip the encoded bip34 block height commitment
92-        assert_equal(CScriptNum.decode(coinbase_tx.vin[0].scriptSig), next_height)

MarcoFalke commented at 5:50 pm on May 27, 2020:

I think this one was actually testing the miner, so should be kept in this file.

jnewbery commented at 4:12 pm on June 4, 2020:

I don’t understand. How is this testing the miner? It’s asserting on the return value of create_coinbase(), which is purely a python helper function.

MarcoFalke commented at 4:23 pm on June 4, 2020:

Correct. I missed that this is not the coinbase from the previous generatetoaddress. It seems the test was moved, so this should be correct now in master.

DrahtBot added the label Tests on May 27, 2020

in test/functional/mining_basic.py:98 in fba67c6672 outdated

92-        # round-trip the encoded bip34 block height commitment
93-        assert_equal(CScriptNum.decode(coinbase_tx.vin[0].scriptSig), next_height)
94-        # round-trip negative and multi-byte CScriptNums to catch python regression
95-        assert_equal(CScriptNum.decode(CScriptNum.encode(CScriptNum(1500))), 1500)
96-        assert_equal(CScriptNum.decode(CScriptNum.encode(CScriptNum(-1500))), -1500)
97-        assert_equal(CScriptNum.decode(CScriptNum.encode(CScriptNum(-1))), -1)

glozow commented at 6:32 pm on May 27, 2020:

I think you should only really be touching these 4 lines in mining_basic.py. The first assertion should stay like Marco said, and you’ll need to keep the import statement in order for it to work.

in test/functional/test_framework/script.py:735 in fba67c6672 outdated

730@@ -731,3 +731,11 @@ def test_bn2vch(self):
731         self.assertEqual(bn2vch(0xFFFFFFFF), bytes([0xFF, 0xFF, 0xFF, 0xFF, 0x00]))
732         self.assertEqual(bn2vch(123456789), bytes([0x15, 0xCD, 0x5B, 0x07]))
733         self.assertEqual(bn2vch(-54321), bytes([0x31, 0xD4, 0x80]))
734+
735+    def test_decode(self):

glozow commented at 6:50 pm on May 27, 2020:

nit: maybe name this test_cscriptnum_encoding or something else more specific? Since there are other encode/decode functions in this file, i.e. for CScriptOp.

in test/functional/test_framework/script.py:736 in fba67c6672 outdated

730@@ -731,3 +731,11 @@ def test_bn2vch(self):
731         self.assertEqual(bn2vch(0xFFFFFFFF), bytes([0xFF, 0xFF, 0xFF, 0xFF, 0x00]))
732         self.assertEqual(bn2vch(123456789), bytes([0x15, 0xCD, 0x5B, 0x07]))
733         self.assertEqual(bn2vch(-54321), bytes([0x31, 0xD4, 0x80]))
734+
735+    def test_decode(self):
736+        # round-trip the encoded bip34 block height commitment

glozow commented at 6:54 pm on May 27, 2020:

Does this comment apply here?

in test/functional/test_framework/script.py:738 in fba67c6672 outdated

736+        # round-trip the encoded bip34 block height commitment
737+        self.assertEqual(CScriptNum.decode(b'\x02\xcb\x00'), 203)
738+        # round-trip negative and multi-byte CScriptNums to catch python regression
739+        self.assertEqual(CScriptNum.decode(CScriptNum.encode(CScriptNum(1500))), 1500)
740+        self.assertEqual(CScriptNum.decode(CScriptNum.encode(CScriptNum(-1500))), -1500)
741+        self.assertEqual(CScriptNum.decode(CScriptNum.encode(CScriptNum(-1))), -1)

glozow commented at 6:57 pm on May 27, 2020:

Maybe out of scope for this PR, but would it be helpful to add more tests (e.g. big numbers up to 4 bytes)?

laanwj commented at 3:05 pm on May 28, 2020:

Yes—more tests would be welcome here IMO, Maybe the list of test numbers in scriptnum_tests.cpp also comes in useful here.

glozow commented at 7:02 pm on May 27, 2020: member

Concept ACK, I suggest trying to undo some of your changes in mining_basic.py so that you aren’t touching any extra lines. You could squash your commits to try to make it easier to see overall changes (you’ll need to do this anyway).

There’s also some description/naming conventions you could take a look at in the contributing doc as well. For example, adding [test] to the commit message and PR title.

practicalswift commented at 7:06 pm on May 27, 2020: contributor

Concept ACK: more testing is better!

Welcome as a contributor @gillichu :)

DrahtBot commented at 7:47 pm on May 27, 2020: member

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

Conflicts

No conflicts as of last run.

gillichu force-pushed on May 27, 2020

gillichu renamed this:
~~[WIP] Moved the CScriptNum asserts into the unit test in script.py~~
[WIP] [test] Moved the CScriptNum asserts into the unit test in script.py
on May 27, 2020

gillichu force-pushed on May 27, 2020

gillichu renamed this:
~~[WIP] [test] Moved the CScriptNum asserts into the unit test in script.py~~
test: Moved the CScriptNum asserts into the unit test in script.py
on May 28, 2020

gillichu marked this as ready for review on May 28, 2020

in test/functional/mining_basic.py:89 in 9d5dd8752a outdated

81@@ -83,20 +82,12 @@ def assert_submitblock(block, result_str_1, result_str_2=None):
82         self.log.info("getblocktemplate: Test capability advertised")
83         assert 'proposal' in tmpl['capabilities']
84         assert 'coinbasetxn' not in tmpl
85-
86         next_height = int(tmpl["height"])
87         coinbase_tx = create_coinbase(height=next_height)
88-        # sequence numbers must not be max for nLockTime to have effect

MarcoFalke commented at 11:34 am on May 28, 2020:

Any reason to remove this comment and the empty lines?

MarcoFalke approved

MarcoFalke commented at 11:34 am on May 28, 2020: member

Almost ACK 9d5dd8752a1bc89f7fcfc1a9b6e0d042b9c20616

laanwj commented at 2:59 pm on May 28, 2020: member

Thanks for you contribution. Looks good to me.

A small nit: please change the git commit message to a short title (up to say, 70 characters), then an empty line, then the rest of the description. This because tools (such as git pretty=oneline) will otherwise put it all on one line instead of displaying just the title:

09d5dd8752… (pull/19082/head) [test] Migrates the CScriptNum decode tests into a unit test, moving some changes made in [#14816](/bitcoin-bitcoin/14816/). This was made possible by the integration of test_framework unit testing in [#18576](/bitcoin-bitcoin/18576/).

gillichu force-pushed on May 28, 2020

in test/functional/test_framework/script.py:739 in 401487d24e outdated

730@@ -731,3 +731,12 @@ def test_bn2vch(self):
731         self.assertEqual(bn2vch(0xFFFFFFFF), bytes([0xFF, 0xFF, 0xFF, 0xFF, 0x00]))
732         self.assertEqual(bn2vch(123456789), bytes([0x15, 0xCD, 0x5B, 0x07]))
733         self.assertEqual(bn2vch(-54321), bytes([0x31, 0xD4, 0x80]))
734+    def test_cscriptnum_encoding(self):
735+        # round-trip negative and multi-byte CScriptNums to catch python regression
736+        self.assertEqual(CScriptNum.decode(CScriptNum.encode(CScriptNum(1500))), 1500)
737+        self.assertEqual(CScriptNum.decode(CScriptNum.encode(CScriptNum(-1500))), -1500)
738+        self.assertEqual(CScriptNum.decode(CScriptNum.encode(CScriptNum(-1))), -1)
739+    def test_cscriptnum_encoding_large(self):

glozow commented at 6:43 pm on May 30, 2020:

Question: I’m not too familiar with what “catch python regression” means 😕 so I’m kind of confused about the difference betweentest_cscriptnum_encoding and test_cscriptnum_encoding_large. Could they be combined in the values array?

MarcoFalke commented at 10:59 pm on May 30, 2020:

“python regression” is an odd way to say “regression in the scriptnum encoding (which is written in python and not c++)”

MarcoFalke commented at 11:00 pm on May 30, 2020:

So you are probably right and they can be combined in one list.

jnewbery commented at 4:49 pm on May 31, 2020:

I think you can remove " to catch python regression" from the comment

gillichu force-pushed on May 31, 2020

in test/functional/mining_basic.py:94 in ef4cce175b outdated

91@@ -92,11 +92,6 @@ def assert_submitblock(block, result_str_1, result_str_2=None):
92 
93         # round-trip the encoded bip34 block height commitment
94         assert_equal(CScriptNum.decode(coinbase_tx.vin[0].scriptSig), next_height)

jnewbery commented at 4:54 pm on May 31, 2020:

This seems to be a test of the create_coinbase() function in blocktools.py. It might be worth moving it to a unit test there.

in test/functional/test_framework/script.py:735 in ef4cce175b outdated

730@@ -731,3 +731,8 @@ def test_bn2vch(self):
731         self.assertEqual(bn2vch(0xFFFFFFFF), bytes([0xFF, 0xFF, 0xFF, 0xFF, 0x00]))
732         self.assertEqual(bn2vch(123456789), bytes([0x15, 0xCD, 0x5B, 0x07]))
733         self.assertEqual(bn2vch(-54321), bytes([0x31, 0xD4, 0x80]))
734+    def test_cscriptnum_encoding(self):

jnewbery commented at 4:55 pm on May 31, 2020:

Please add a blank line between function definitions.

jnewbery commented at 4:56 pm on May 31, 2020: member

Welcome to the project @gillichu, and thanks for your contribution ! I have a couple of small suggestions inline.

jnewbery commented at 5:00 pm on May 31, 2020: member

A couple more suggestions for the commit log and PR description:

use the imperative mood (s/Moved the CScript/Move the CScript/ and s/Migrates the CScript/Migrate the CScript/).
wrap the message body width at 72 or 80 characters.

See https://chris.beams.io/posts/git-commit/ for more tips on writing great commit logs.

gillichu force-pushed on May 31, 2020

in test/functional/test_framework/blocktools.py:229 in 58c4a30b0d outdated

224+    def test_createcoinbase(self):
225+        next_height = 20
226+        coinbase_tx = create_coinbase(height=next_height)
227+        # round-trip the encoded bip34 block height commitment
228+        assert_equal(CScriptNum.decode(coinbase_tx.vin[0].scriptSig), next_height)
229+

gillichu commented at 5:48 pm on May 31, 2020:

Added this newline failing the Python linter, will update with next batch of changes.

glozow commented at 6:14 pm on May 31, 2020:

Btw, if you install flake8, you can run the python linter with test/lint/lint-python.sh. It has saved me a lot of force pushes 😂

in test/functional/test_framework/blocktools.py:223 in 58c4a30b0d outdated

218@@ -217,3 +219,11 @@ def send_to_witness(use_p2wsh, node, utxo, pubkey, encode_p2sh, amount, sign=Tru
219             tx_to_witness = ToHex(tx)
220 
221     return node.sendrawtransaction(tx_to_witness)
222+
223+class TestFrameworkScript(unittest.TestCase):

glozow commented at 6:20 pm on May 31, 2020:

I think you might have copied this from script.py? A better name might be TestFrameworkBlocktools

in test/functional/test_framework/blocktools.py:224 in 58c4a30b0d outdated

218@@ -217,3 +219,11 @@ def send_to_witness(use_p2wsh, node, utxo, pubkey, encode_p2sh, amount, sign=Tru
219             tx_to_witness = ToHex(tx)
220 
221     return node.sendrawtransaction(tx_to_witness)
222+
223+class TestFrameworkScript(unittest.TestCase):
224+    def test_createcoinbase(self):

glozow commented at 6:21 pm on May 31, 2020:

nit: I would keep the underscore in between, i.e. test_create_coinbase, and maybe just call it height since it’s not in a sequence of blocks.

in test/functional/mining_basic.py:91 in 58c4a30b0d outdated

90@@ -91,12 +91,6 @@ def assert_submitblock(block, result_str_1, result_str_2=None):
91         coinbase_tx.rehash()
92 
93         # round-trip the encoded bip34 block height commitment

glozow commented at 6:23 pm on May 31, 2020:

delete this comment?

jnewbery commented at 4:13 pm on June 4, 2020:

This hasn’t been deleted.

gillichu commented at 4:25 pm on June 4, 2020:

Sorry about that :( I’ll watch my changes more carefully next time, and look for a chance to take this out in the future.

glozow commented at 6:27 pm on May 31, 2020: member

Almost there

jnewbery deleted a comment on Jun 1, 2020

in test/functional/test_runner.py:71 in 58c4a30b0d outdated

67@@ -68,6 +68,7 @@
68 
69 TEST_FRAMEWORK_MODULES = [
70     "script",
71+    "blocktools",

jnewbery commented at 2:00 am on June 1, 2020:

nit: sort please :)

gillichu force-pushed on Jun 1, 2020

DrahtBot added the label Needs rebase on Jun 1, 2020

gillichu force-pushed on Jun 2, 2020

DrahtBot removed the label Needs rebase on Jun 2, 2020

[test] CScriptNum Decode Check as Unit Tests

Migrates the CScriptNum decode tests into a unit test, and moved some
changes made in #14816. Made possible by the integration of
test_framework unit testing in #18576. Further extends the original
test with larger ints, similar to the scriptnum_tests.cpp file. Adds
test to blocktools.py testing fn create_coinbase() with CScriptNum
decode.

7daffc6a90

gillichu force-pushed on Jun 3, 2020

gillichu commented at 8:31 pm on June 3, 2020: none

rebased the branch, addressed review comments and updated tests. thanks for the review @laanwj @gzhao408 @jnewbery comments have been addressed!

jnewbery commented at 3:03 am on June 4, 2020: member

Thanks @gillichu. I think the PR title, commit summary and commit details could be made a bit clearer:

switch to indicative mood (eg “test: Moved the CScriptNum asserts into the unit test in script.py” -> “Add CScriptNum and create_coinbase unit tests”, “Migrates the CScriptNum decode tests” -> “Migrate the CScriptNum decode tests”, etc)
no need to include the complete history ("… changes made in #14816", “… integration of test_framework unit testing in #18576.”).

laanwj commented at 3:26 pm on June 4, 2020: member

ACK 7daffc6a90a797ce7c365a893a31a31b0206985c Looks good to me now! Please keep the additional suggestions in mind for a next PR, but I don’t think we should keep iterating on the commit message too much.

laanwj merged this on Jun 4, 2020

laanwj closed this on Jun 4, 2020

sidhujag referenced this in commit f5a1bed93b on Jun 4, 2020

fanquake referenced this in commit 45a6811d36 on Jun 11, 2020

sidhujag referenced this in commit 2bd52568a8 on Jun 11, 2020

Fabcien referenced this in commit 2a3ba49ebb on Mar 1, 2021

DrahtBot locked this on Aug 16, 2022

test: Moved the CScriptNum asserts into the unit test in script.py #19082

Conflicts