/bitcoin

[qa] Rewrite BIP65/BIP66 functional tests #10695

Open
wants to merge 2 commits into
from
+213 −414

Conversation

Reviewers

@TheBlueMatt TheBlueMatt

@jnewbery jnewbery

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
4 participants
@sdaftuar @jnewbery @TheBlueMatt @fanquake
@sdaftuar
Contributor

sdaftuar commented Jun 28, 2017 • edited

After 122786d, 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 changed the title from [qa] Rewrite BIP65 functional tests to [qa] Rewrite BIP65/BIP66 functional tests Jun 28, 2017

@TheBlueMatt

TheBlueMatt reviewed Jun 28, 2017
View changes

Looks good to me at 06e3f00, 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.

test/functional/bip65-cltv-p2p.py
+ Prepends <height> CLTV DROP in the scriptSig, and sets
+ the locktime to height'''
+ tx.vin[0].nSequence = 0
+ tx.nLockTime = height
@TheBlueMatt

TheBlueMatt Jun 28, 2017

Contributor

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

sdaftuar Jun 29, 2017

Contributor

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

@sdaftuar

sdaftuar Jun 29, 2017 • edited

Contributor

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

fanquake added the Tests label Jun 29, 2017

@jnewbery

jnewbery reviewed Jun 29, 2017
View changes

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 06e3f00

test/functional/bipdersig-p2p.py
from test_framework.blocktools import create_coinbase, create_block
-from test_framework.comptool import TestInstance, TestManager
from test_framework.script import CScript
from io import BytesIO
import time
@jnewbery

jnewbery Jun 29, 2017

Member

nit: this import is unused and can be removed

test/functional/bipdersig-p2p.py
from test_framework.script import CScript
from io import BytesIO
import time
+DERSIG_HEIGHT = 1251
+
# A canonical signature consists of:
@jnewbery

jnewbery Jun 29, 2017

Member

nit: trailing whitespace

test/functional/bipdersig-p2p.py
+
+class BaseNode(NodeConnCB):
+ def __init__(self):
+ super().__init__()
@jnewbery

jnewbery Jun 29, 2017

Member

nit: trailing whitespace

test/functional/bipdersig-p2p.py
- self.tip = block.sha256
- height += 1
- yield TestInstance([[block, True]])
+ node0.send_and_ping(msg_block(block))
@jnewbery

jnewbery Jun 29, 2017

Member

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.

test/functional/bipdersig-p2p.py
- ''' Mine 1 new version block '''
- block = create_block(self.tip, create_coinbase(height), self.last_block_time + 1)
+ self.log.info("Test that transactions with non-DER signatures cannot appear in a block")
@jnewbery

jnewbery Jun 29, 2017

Member

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

sdaftuar Jun 29, 2017 • edited

Contributor

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.

test/functional/bipdersig-p2p.py
- ''' Mine 1 old version block, should be invalid '''
- block = create_block(self.tip, create_coinbase(height), self.last_block_time + 1)
- block.nVersion = 2
+ node0.send_and_ping(msg_block(block))
@jnewbery

jnewbery Jun 29, 2017

Member

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

test/functional/bip65-cltv-p2p.py
from test_framework.blocktools import create_coinbase, create_block
-from test_framework.comptool import TestInstance, TestManager
-from test_framework.script import CScript, OP_1NEGATE, OP_CHECKLOCKTIMEVERIFY, OP_DROP
+from test_framework.script import CScript, OP_1NEGATE, OP_CHECKLOCKTIMEVERIFY, OP_DROP, CScriptNum
from io import BytesIO
import time
@jnewbery

jnewbery Jun 29, 2017

Member

nit: import time not used

test/functional/bip65-cltv-p2p.py
+ signed_result = node.signrawtransaction(ToHex(tx))
+ new_tx = CTransaction()
+ f = BytesIO(hex_str_to_bytes(signed_result['hex']))
+ new_tx.deserialize(f)
@jnewbery

jnewbery Jun 29, 2017

Member

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

test/functional/bip65-cltv-p2p.py
+class BaseNode(NodeConnCB):
@jnewbery

jnewbery Jun 29, 2017

Member

Not required. Just use the base NodeConnCB below.

test/functional/bip65-cltv-p2p.py
+ super().__init__()
+ self.num_nodes = 1
+ self.extra_args = [['-promiscuousmempoolflags=1', '-whitelist=127.0.0.1']]
+ self.setup_clean_chain = True
def create_transaction(self, node, coinbase, to_address, amount):
@jnewbery

jnewbery Jun 29, 2017

Member

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

test/functional/bip65-cltv-p2p.py
@@ -58,119 +66,79 @@ def create_transaction(self, node, coinbase, to_address, amount):
tx.deserialize(f)
return tx
- def get_tests(self):
+ def run_test(self):
+ node0 = BaseNode()
@jnewbery

jnewbery Jun 29, 2017

Member

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.

test/functional/bip65-cltv-p2p.py
+ tip = self.nodes[0].getbestblockhash()
+ block_time = self.nodes[0].getblockheader(tip)['mediantime'] + 1
+ block = create_block(int(tip, 16), create_coinbase(CLTV_HEIGHT - 1), block_time)
+ block.nVersion = 3
block.vtx.append(spendtx)
block.hashMerkleRoot = block.calc_merkle_root()
block.rehash()
@jnewbery

jnewbery Jun 29, 2017

Member

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.

test/functional/bip65-cltv-p2p.py
- self.tip = block.sha256
- height += 1
- yield TestInstance([[block, True]])
+ node0.send_and_ping(msg_block(block))
@jnewbery

jnewbery Jun 29, 2017

Member

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

@sdaftuar
Contributor

sdaftuar commented Jun 29, 2017

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

sdaftuar added some commits Jun 28, 2017

@sdaftuar sdaftuar [qa] Rewrite BIP65 functional tests 
After 122786d, 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.
 d4f0d87
@sdaftuar sdaftuar [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
4ccc12a
@sdaftuar
Contributor

sdaftuar commented Jul 6, 2017

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

jnewbery commented Jul 10, 2017 • edited

Tested ACK 4ccc12a. 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment