[qa] Rewrite BIP65/BIP66 functional tests #10695

sdaftuar commented at 7:47 PM on June 28, 2017: member

After 122786d0e0170c73536360b705af711e1338adbf, BIP65 and BIP66 activate at particular fixed heights (without regard to version numbers of blocks below those heights). Rewrite the functional tests to take this into account, and remove two tests that weren't really testing anything.

Moves the rewritten functional tests out of the extended test suite, so that they run in travis regularly.

Note: I discovered that the ComparisonTestFramework (which the original versions of these p2p tests were written is, has a bug that caused them to not catch obvious errors, eg if you just comment out setting the script flags for these softforks in ConnectBlock, the versions of these tests in master do not fail(!) -- will separately PR a fix for the comparison test framework).

sdaftuar force-pushed on Jun 28, 2017

sdaftuar renamed this:
~~[qa] Rewrite BIP65 functional tests~~
[qa] Rewrite BIP65/BIP66 functional tests
on Jun 28, 2017

in test/functional/bip65-cltv-p2p.py:41 in 1cf60bb125 outdated

  25 | @@ -33,19 +26,34 @@ def cltv_invalidate(tx):
  26 |      tx.vin[0].scriptSig = CScript([OP_1NEGATE, OP_CHECKLOCKTIMEVERIFY, OP_DROP] +
  27 |                                    list(CScript(tx.vin[0].scriptSig)))
  28 |  
  29 | +def cltv_validate(node, tx, height):
  30 | +    '''Modify the signature in vin 0 of the tx to pass CLTV
  31 | +    Prepends <height> CLTV DROP in the scriptSig, and sets
  32 | +    the locktime to height'''
  33 | +    tx.vin[0].nSequence = 0
  34 | +    tx.nLockTime = height

TheBlueMatt commented at 10:13 PM on June 28, 2017:

For my own sanity it'd be nice if cltv_invalidate looked much more like cltv_validate, which would make it more obvious that the invalid tx is clearly invalid due to the CLTV and not some other reason, and while you're at it would be nice to test other ways CLTV can be invalid (both tx doesnt meet sequence requirements and tx doesnt meet locktime requirements).

sdaftuar commented at 5:27 PM on June 29, 2017:

Agree that this would be nice, but left this as a todo for now.

sdaftuar commented at 5:33 PM on June 29, 2017:

I did add a test that shows that the transaction is accepted to the node's mempool, which is using -promiscuousmempoolflags so that it's not enforcing the soft-fork rule. So hopefully that helps assuage the concern that the transaction might be invalid for non-soft-fork related reasons.

(Also, this helps sneak in a test that is useful post-#10192, by verifying that a block is invalid even if we may have cached validation success using a different set of flags.)

TheBlueMatt commented at 11:11 PM on June 28, 2017: member

Looks good to me at 06e3f006e499e3238905b92cbca563c69e170410, my test framework/python chops dont make me want to ACK, but I did test a few cases that the test actually failed if tweaked.

fanquake added the label Tests on Jun 29, 2017

in test/functional/bipdersig-p2p.py:98 in 06e3f006e4 outdated

 156 |          block.solve()
 157 | -        self.last_block_time += 1
 158 | -        self.tip = block.sha256
 159 | -        height += 1
 160 | -        yield TestInstance([[block, True]])
 161 | +        node0.send_and_ping(msg_block(block))

jnewbery commented at 9:02 AM on June 29, 2017:

The node-under-test sends a reject message at this point:

2017-06-29 08:56:00.945000 TestFramework.mininode (DEBUG): Received message from 127.0.0.1:11900: msg_reject: b'block' 17 b'bad-version(0x00000002)' [7bb0412e9813a1d13cd14ff2277a906e101d183f93b0136c848bdb94ffd1d6b7]

It'd be nice to add an assert that mininode received that reject.

I know that there's talk of removing reject messages from bitcoind, but while they're still there, they're useful for asserting that a block/tx was rejected for the correct reason. If there was some error in the code above (eg if we'd called create_block() with bad parameters or it'd created a bad block for some reason), the block would have been rejected, the test would pass, but we wouldn't actually be testing BIP66 activation.

in test/functional/bipdersig-p2p.py:128 in 06e3f006e4 outdated

 192 | -        yield TestInstance([[block, False]])
 193 |  
 194 | -        ''' Mine 1 old version block, should be invalid '''
 195 | -        block = create_block(self.tip, create_coinbase(height), self.last_block_time + 1)
 196 | -        block.nVersion = 2
 197 | +        node0.send_and_ping(msg_block(block))

jnewbery commented at 9:05 AM on June 29, 2017:

Again, it'd be good to assert that we received a reject, this time with cause 'block-validation-failed'

in test/functional/bipdersig-p2p.py:16 in 06e3f006e4 outdated

  24 | +from test_framework.mininode import *
  25 |  from test_framework.blocktools import create_coinbase, create_block
  26 | -from test_framework.comptool import TestInstance, TestManager
  27 |  from test_framework.script import CScript
  28 |  from io import BytesIO
  29 |  import time

jnewbery commented at 9:13 AM on June 29, 2017:

nit: this import is unused and can be removed

in test/functional/bipdersig-p2p.py:20 in 06e3f006e4 outdated

  28 |  from io import BytesIO
  29 |  import time
  30 |  
  31 | +DERSIG_HEIGHT = 1251
  32 | +
  33 |  # A canonical signature consists of:

jnewbery commented at 9:16 AM on June 29, 2017:

nit: trailing whitespace

in test/functional/bipdersig-p2p.py:39 in 06e3f006e4 outdated

  31 | @@ -40,18 +32,18 @@ def unDERify(tx):
  32 |          else:
  33 |              newscript.append(i)
  34 |      tx.vin[0].scriptSig = CScript(newscript)
  35 | +
  36 | +class BaseNode(NodeConnCB):
  37 | +    def __init__(self):
  38 | +        super().__init__()
  39 |

jnewbery commented at 9:16 AM on June 29, 2017:

nit: trailing whitespace

in test/functional/bipdersig-p2p.py:108 in 06e3f006e4 outdated

 161 | +        node0.send_and_ping(msg_block(block))
 162 | +        assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip)
 163 |  
 164 | -        ''' Mine 1 new version block '''
 165 | -        block = create_block(self.tip, create_coinbase(height), self.last_block_time + 1)
 166 | +        self.log.info("Test that transactions with non-DER signatures cannot appear in a block")

jnewbery commented at 9:18 AM on June 29, 2017:

Are transactions with non-DER signatures added to the mempool or relayed after BIP66 activation? If not, is it possible to add a sub-test here to test that?

sdaftuar commented at 5:31 PM on June 29, 2017:

Default policy is to enforce DERSIG and CLTV on all transactions entering the mempool. I suppose it might be nice to add a test that the policy is enforcing these rules, but in this test I set custom script flags so that I could show that a transaction is valid under script flags that don't include the soft fork, but invalid under the soft fork.

It might be nice to add a second node to these tests that uses default policy so that we can test that default policy is enforcing these script flags all the time, but I would prefer to save that for a future PR.

in test/functional/bip65-cltv-p2p.py:17 in 06e3f006e4 outdated

  26 |  from test_framework.blocktools import create_coinbase, create_block
  27 | -from test_framework.comptool import TestInstance, TestManager
  28 | -from test_framework.script import CScript, OP_1NEGATE, OP_CHECKLOCKTIMEVERIFY, OP_DROP
  29 | +from test_framework.script import CScript, OP_1NEGATE, OP_CHECKLOCKTIMEVERIFY, OP_DROP, CScriptNum
  30 |  from io import BytesIO
  31 |  import time

jnewbery commented at 9:29 AM on June 29, 2017:

nit: import time not used

in test/functional/bip65-cltv-p2p.py:40 in 06e3f006e4 outdated

  35 | +
  36 | +    # Need to re-sign, since nSequence and nLockTime changed
  37 | +    signed_result = node.signrawtransaction(ToHex(tx))
  38 | +    new_tx = CTransaction()
  39 | +    f = BytesIO(hex_str_to_bytes(signed_result['hex']))
  40 | +    new_tx.deserialize(f)

jnewbery commented at 9:32 AM on June 29, 2017:

nit: perhaps combine this with the line above, rather than have a throwaway variable.

in test/functional/bip65-cltv-p2p.py:46 in 06e3f006e4 outdated

  42 | -class BIP65Test(ComparisonTestFramework):
  43 | +    new_tx.vin[0].scriptSig = CScript([CScriptNum(height), OP_CHECKLOCKTIMEVERIFY, OP_DROP] +
  44 | +                                  list(CScript(new_tx.vin[0].scriptSig)))
  45 | +    return new_tx
  46 |  
  47 | +class BaseNode(NodeConnCB):

jnewbery commented at 9:34 AM on June 29, 2017:

Not required. Just use the base NodeConnCB below.

in test/functional/bip65-cltv-p2p.py:58 in 06e3f006e4 outdated

  61 | +        super().__init__()
  62 | +        self.num_nodes = 1
  63 | +        self.extra_args = [['-promiscuousmempoolflags=1', '-whitelist=127.0.0.1']]
  64 | +        self.setup_clean_chain = True
  65 |  
  66 |      def create_transaction(self, node, coinbase, to_address, amount):

jnewbery commented at 9:36 AM on June 29, 2017:

This doesn't need to be a method in BIP65Test. Consider moving it above the class definition to be a module-level function (alongside cltv_invalidate() and cltv_validate())

in test/functional/bip65-cltv-p2p.py:70 in 06e3f006e4 outdated

  65 | @@ -58,119 +66,79 @@ def create_transaction(self, node, coinbase, to_address, amount):
  66 |          tx.deserialize(f)
  67 |          return tx
  68 |  
  69 | -    def get_tests(self):
  70 | +    def run_test(self):
  71 | +        node0 = BaseNode()

jnewbery commented at 9:38 AM on June 29, 2017:

self.nodes[0] is used throughout this test. Consider adding an alias:

node = self.nodes[0]

and then using node throughout the test.

I think that looks slightly neater for tests that only use one node, but that's just personal preference.

in test/functional/bip65-cltv-p2p.py:97 in 06e3f006e4 outdated

 130 | +        block_time = self.nodes[0].getblockheader(tip)['mediantime'] + 1
 131 | +        block = create_block(int(tip, 16), create_coinbase(CLTV_HEIGHT - 1), block_time)
 132 | +        block.nVersion = 3
 133 |          block.vtx.append(spendtx)
 134 |          block.hashMerkleRoot = block.calc_merkle_root()
 135 |          block.rehash()

jnewbery commented at 9:48 AM on June 29, 2017:

block.solve() calls block.rehash(), so there's no need for all of these explicit calls to block.rehash(). Up to you if you want to remove them all.

in test/functional/bip65-cltv-p2p.py:109 in 06e3f006e4 outdated

 167 |          block.solve()
 168 | -        self.last_block_time += 1
 169 | -        self.tip = block.sha256
 170 | -        height += 1
 171 | -        yield TestInstance([[block, True]])
 172 | +        node0.send_and_ping(msg_block(block))

jnewbery commented at 9:50 AM on June 29, 2017:

I've added a comment to bipdersig-p2p.py about asserting on reject messages. Same comment applies here

jnewbery commented at 9:55 AM on June 29, 2017: member

Very nice cleanup. I love removing redundant tests!

A few nits below. Nothing super important, although I would like to see asserts on reject messages, to ensure that blocks are being rejected for the correct reason.

It'd be nice to rename these (and other tests) to remove the -p2p suffix, since that now doesn't mean much. That could be done in a future PR if people agree.

Consider using a linter like flake8 (together with the vim syntastic plugin) for catching obvious style nits like unused imports and trailing whitespace.

Even without the nit fixes, this is a definite improvement over the current tests. Tested ACK 06e3f006e499e3238905b92cbca563c69e170410

sdaftuar force-pushed on Jun 29, 2017

sdaftuar commented at 5:31 PM on June 29, 2017: member

@jnewbery Thanks for the review; I addressed all your nits except the one I responded to.

[qa] Rewrite BIP65 functional tests

After 122786d0e0170c73536360b705af711e1338adbf, BIP65 activates at
a particular height (without regard to version numbers of blocks
below that height).  Rewrite the BIP65 functional tests to take
this into account, and add a test case that exercises
OP_CHECKLOCKTIMEVERIFY in a block where the soft-fork is active.

Also moves the bip65 functional test out of the extended test suite.

d4f0d87b6f

sdaftuar force-pushed on Jun 30, 2017

[qa] Rewrite BIP66 functional tests

Rewrite the BIP66 functional tests to reflect height-based activation,
and move it out of the extended test suite.

Remove the unnecessary bipdersig.py test

4ccc12a54a

sdaftuar force-pushed on Jul 6, 2017

sdaftuar commented at 2:15 PM on July 6, 2017: member

Bumped travis after spurious zapwallettxs error popped up (appeared to be an instance of #10678)

jnewbery commented at 1:48 PM on July 10, 2017: member

Tested ACK https://github.com/bitcoin/bitcoin/pull/10695/commits/4ccc12a54a217892bd4ccfd94c46052a11cdb3fb. ~It'd be nice to add asserting on reject messages, but that can wait for a future PR.~

Update: I'm an idiot. You've already added testing reject messages.

ACK

MarcoFalke commented at 8:57 PM on August 8, 2017: member

utACK 4ccc12a54a217892bd4ccfd94c46052a11cdb3fb

MarcoFalke merged this on Aug 8, 2017

MarcoFalke closed this on Aug 8, 2017

MarcoFalke referenced this in commit 929fd7276c on Aug 8, 2017

PastaPastaPasta referenced this in commit 4fe8c74dc1 on Sep 8, 2019

PastaPastaPasta referenced this in commit 4ce64908e5 on Sep 8, 2019

PastaPastaPasta referenced this in commit 6fc9285412 on Sep 8, 2019

barrystyle referenced this in commit f55e67f696 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021