From ba923e32a0c03fcbb6ffe317580fd1d04669ce71 Mon Sep 17 00:00:00 2001 From: practicalswift Date: Sun, 23 Sep 2018 20:44:15 +0200 Subject: [PATCH 1/5] test: Fix broken segwit test --- test/functional/p2p_segwit.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index de12ab1ed..58bfd1c94 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -771,8 +771,8 @@ class SegWitTest(BitcoinTestFramework): # segwit-aware would also reject this for failing CLEANSTACK. test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False) - # Try to put the witness script in the script_sig, should also fail. - spend_tx.vin[0].script_sig = CScript([p2wsh_pubkey, b'a']) + # Try to put the witness script in the scriptSig, should also fail. + spend_tx.vin[0].scriptSig = CScript([p2wsh_pubkey, b'a']) spend_tx.rehash() test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False) From 1d0ce94a54be17b139a052b8e7b4605fe9ac1ecd Mon Sep 17 00:00:00 2001 From: Justin Turner Arthur Date: Sun, 23 Sep 2018 21:30:22 -0500 Subject: [PATCH 2/5] Fix for incorrect version attr set on functional test segwit block. --- test/functional/p2p_segwit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 58bfd1c94..1b3c5bd1f 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -205,7 +205,7 @@ class SegWitTest(BitcoinTestFramework): height = self.nodes[0].getblockcount() + 1 block_time = self.nodes[0].getblockheader(tip)["mediantime"] + 1 block = create_block(int(tip, 16), create_coinbase(height), block_time) - block.version = version + block.nVersion = version block.rehash() return block From 3a4449e9ad945313c6637283757de8d539cf790f Mon Sep 17 00:00:00 2001 From: Justin Turner Arthur Date: Sun, 23 Sep 2018 21:34:42 -0500 Subject: [PATCH 3/5] Strictly enforce instance attrs in critical functional test classes. Additionally, removed redundant parentheses and added PEP-8 compliant spacing around those classes. --- test/functional/test_framework/messages.py | 173 ++++++++++++++++----- test/functional/test_framework/script.py | 9 +- 2 files changed, 138 insertions(+), 44 deletions(-) diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index 0a3907cba..8e9372767 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -13,7 +13,11 @@ CBlock, CTransaction, CBlockHeader, CTxIn, CTxOut, etc....: msg_block, msg_tx, msg_headers, etc.: data structures that represent network messages -ser_*, deser_*: functions that handle serialization/deserialization.""" +ser_*, deser_*: functions that handle serialization/deserialization. + +Classes use __slots__ to ensure extraneous attributes aren't accidentally added +by tests, compromising their intended effect. +""" from codecs import encode import copy import hashlib @@ -185,7 +189,10 @@ def ToHex(obj): # Objects that map to bitcoind objects, which can be serialized/deserialized -class CAddress(): + +class CAddress: + __slots__ = ("ip", "nServices", "pchReserved", "port", "time") + def __init__(self): self.time = 0 self.nServices = 1 @@ -215,7 +222,10 @@ class CAddress(): return "CAddress(nServices=%i ip=%s port=%i)" % (self.nServices, self.ip, self.port) -class CInv(): + +class CInv: + __slots__ = ("hash", "type") + typemap = { 0: "Error", 1: "TX", @@ -244,7 +254,9 @@ class CInv(): % (self.typemap[self.type], self.hash) -class CBlockLocator(): +class CBlockLocator: + __slots__ = ("nVersion", "vHave") + def __init__(self): self.nVersion = MY_VERSION self.vHave = [] @@ -264,7 +276,9 @@ class CBlockLocator(): % (self.nVersion, repr(self.vHave)) -class COutPoint(): +class COutPoint: + __slots__ = ("hash", "n") + def __init__(self, hash=0, n=0): self.hash = hash self.n = n @@ -283,7 +297,9 @@ class COutPoint(): return "COutPoint(hash=%064x n=%i)" % (self.hash, self.n) -class CTxIn(): +class CTxIn: + __slots__ = ("nSequence", "prevout", "scriptSig") + def __init__(self, outpoint=None, scriptSig=b"", nSequence=0): if outpoint is None: self.prevout = COutPoint() @@ -311,7 +327,9 @@ class CTxIn(): self.nSequence) -class CTxOut(): +class CTxOut: + __slots__ = ("nValue", "scriptPubKey") + def __init__(self, nValue=0, scriptPubKey=b""): self.nValue = nValue self.scriptPubKey = scriptPubKey @@ -332,7 +350,9 @@ class CTxOut(): bytes_to_hex_str(self.scriptPubKey)) -class CScriptWitness(): +class CScriptWitness: + __slots__ = ("stack",) + def __init__(self): # stack is a vector of strings self.stack = [] @@ -347,7 +367,9 @@ class CScriptWitness(): return True -class CTxInWitness(): +class CTxInWitness: + __slots__ = ("scriptWitness",) + def __init__(self): self.scriptWitness = CScriptWitness() @@ -364,7 +386,9 @@ class CTxInWitness(): return self.scriptWitness.is_null() -class CTxWitness(): +class CTxWitness: + __slots__ = ("vtxinwit",) + def __init__(self): self.vtxinwit = [] @@ -392,7 +416,10 @@ class CTxWitness(): return True -class CTransaction(): +class CTransaction: + __slots__ = ("hash", "nLockTime", "nVersion", "sha256", "vin", "vout", + "wit") + def __init__(self, tx=None): if tx is None: self.nVersion = 1 @@ -496,7 +523,10 @@ class CTransaction(): % (self.nVersion, repr(self.vin), repr(self.vout), repr(self.wit), self.nLockTime) -class CBlockHeader(): +class CBlockHeader: + __slots__ = ("hash", "hashMerkleRoot", "hashPrevBlock", "nBits", "nNonce", + "nTime", "nVersion", "sha256") + def __init__(self, header=None): if header is None: self.set_null() @@ -565,6 +595,8 @@ class CBlockHeader(): class CBlock(CBlockHeader): + __slots__ = ("vtx",) + def __init__(self, header=None): super(CBlock, self).__init__(header) self.vtx = [] @@ -636,7 +668,9 @@ class CBlock(CBlockHeader): time.ctime(self.nTime), self.nBits, self.nNonce, repr(self.vtx)) -class PrefilledTransaction(): +class PrefilledTransaction: + __slots__ = ("index", "tx") + def __init__(self, index=0, tx = None): self.index = index self.tx = tx @@ -664,8 +698,12 @@ class PrefilledTransaction(): def __repr__(self): return "PrefilledTransaction(index=%d, tx=%s)" % (self.index, repr(self.tx)) + # This is what we send on the wire, in a cmpctblock message. -class P2PHeaderAndShortIDs(): +class P2PHeaderAndShortIDs: + __slots__ = ("header", "nonce", "prefilled_txn", "prefilled_txn_length", + "shortids", "shortids_length") + def __init__(self): self.header = CBlockHeader() self.nonce = 0 @@ -703,9 +741,11 @@ class P2PHeaderAndShortIDs(): def __repr__(self): return "P2PHeaderAndShortIDs(header=%s, nonce=%d, shortids_length=%d, shortids=%s, prefilled_txn_length=%d, prefilledtxn=%s" % (repr(self.header), self.nonce, self.shortids_length, repr(self.shortids), self.prefilled_txn_length, repr(self.prefilled_txn)) + # P2P version of the above that will use witness serialization (for compact # block version 2) class P2PHeaderAndShortWitnessIDs(P2PHeaderAndShortIDs): + __slots__ = () def serialize(self): return super(P2PHeaderAndShortWitnessIDs, self).serialize(with_witness=True) @@ -715,9 +755,12 @@ def calculate_shortid(k0, k1, tx_hash): expected_shortid &= 0x0000ffffffffffff return expected_shortid + # This version gets rid of the array lengths, and reinterprets the differential # encoding into indices that can be used for lookup. -class HeaderAndShortIDs(): +class HeaderAndShortIDs: + __slots__ = ("header", "nonce", "prefilled_txn", "shortids", "use_witness") + def __init__(self, p2pheaders_and_shortids = None): self.header = CBlockHeader() self.nonce = 0 @@ -778,7 +821,8 @@ class HeaderAndShortIDs(): return "HeaderAndShortIDs(header=%s, nonce=%d, shortids=%s, prefilledtxn=%s" % (repr(self.header), self.nonce, repr(self.shortids), repr(self.prefilled_txn)) -class BlockTransactionsRequest(): +class BlockTransactionsRequest: + __slots__ = ("blockhash", "indexes") def __init__(self, blockhash=0, indexes = None): self.blockhash = blockhash @@ -818,7 +862,8 @@ class BlockTransactionsRequest(): return "BlockTransactionsRequest(hash=%064x indexes=%s)" % (self.blockhash, repr(self.indexes)) -class BlockTransactions(): +class BlockTransactions: + __slots__ = ("blockhash", "transactions") def __init__(self, blockhash=0, transactions = None): self.blockhash = blockhash @@ -840,7 +885,10 @@ class BlockTransactions(): def __repr__(self): return "BlockTransactions(hash=%064x transactions=%s)" % (self.blockhash, repr(self.transactions)) -class CPartialMerkleTree(): + +class CPartialMerkleTree: + __slots__ = ("fBad", "nTransactions", "vBits", "vHash") + def __init__(self): self.nTransactions = 0 self.vHash = [] @@ -868,7 +916,10 @@ class CPartialMerkleTree(): def __repr__(self): return "CPartialMerkleTree(nTransactions=%d, vHash=%s, vBits=%s)" % (self.nTransactions, repr(self.vHash), repr(self.vBits)) -class CMerkleBlock(): + +class CMerkleBlock: + __slots__ = ("header", "txn") + def __init__(self): self.header = CBlockHeader() self.txn = CPartialMerkleTree() @@ -888,7 +939,9 @@ class CMerkleBlock(): # Objects that correspond to messages on the wire -class msg_version(): +class msg_version: + __slots__ = ("addrFrom", "addrTo", "nNonce", "nRelay", "nServices", + "nStartingHeight", "nTime", "nVersion", "strSubVer") command = b"version" def __init__(self): @@ -945,7 +998,8 @@ class msg_version(): self.strSubVer, self.nStartingHeight, self.nRelay) -class msg_verack(): +class msg_verack: + __slots__ = () command = b"verack" def __init__(self): @@ -961,7 +1015,8 @@ class msg_verack(): return "msg_verack()" -class msg_addr(): +class msg_addr: + __slots__ = ("addrs",) command = b"addr" def __init__(self): @@ -977,7 +1032,8 @@ class msg_addr(): return "msg_addr(addrs=%s)" % (repr(self.addrs)) -class msg_inv(): +class msg_inv: + __slots__ = ("inv",) command = b"inv" def __init__(self, inv=None): @@ -996,7 +1052,8 @@ class msg_inv(): return "msg_inv(inv=%s)" % (repr(self.inv)) -class msg_getdata(): +class msg_getdata: + __slots__ = ("inv",) command = b"getdata" def __init__(self, inv=None): @@ -1012,7 +1069,8 @@ class msg_getdata(): return "msg_getdata(inv=%s)" % (repr(self.inv)) -class msg_getblocks(): +class msg_getblocks: + __slots__ = ("locator", "hashstop") command = b"getblocks" def __init__(self): @@ -1035,7 +1093,8 @@ class msg_getblocks(): % (repr(self.locator), self.hashstop) -class msg_tx(): +class msg_tx: + __slots__ = ("tx",) command = b"tx" def __init__(self, tx=CTransaction()): @@ -1050,13 +1109,16 @@ class msg_tx(): def __repr__(self): return "msg_tx(tx=%s)" % (repr(self.tx)) + class msg_witness_tx(msg_tx): + __slots__ = () def serialize(self): return self.tx.serialize_with_witness() -class msg_block(): +class msg_block: + __slots__ = ("block",) command = b"block" def __init__(self, block=None): @@ -1074,9 +1136,12 @@ class msg_block(): def __repr__(self): return "msg_block(block=%s)" % (repr(self.block)) + # for cases where a user needs tighter control over what is sent over the wire # note that the user must supply the name of the command, and the data -class msg_generic(): +class msg_generic: + __slots__ = ("command", "data") + def __init__(self, command, data=None): self.command = command self.data = data @@ -1087,13 +1152,16 @@ class msg_generic(): def __repr__(self): return "msg_generic()" -class msg_witness_block(msg_block): +class msg_witness_block(msg_block): + __slots__ = () def serialize(self): r = self.block.serialize(with_witness=True) return r -class msg_getaddr(): + +class msg_getaddr: + __slots__ = () command = b"getaddr" def __init__(self): @@ -1109,7 +1177,8 @@ class msg_getaddr(): return "msg_getaddr()" -class msg_ping(): +class msg_ping: + __slots__ = ("nonce",) command = b"ping" def __init__(self, nonce=0): @@ -1127,7 +1196,8 @@ class msg_ping(): return "msg_ping(nonce=%08x)" % self.nonce -class msg_pong(): +class msg_pong: + __slots__ = ("nonce",) command = b"pong" def __init__(self, nonce=0): @@ -1145,7 +1215,8 @@ class msg_pong(): return "msg_pong(nonce=%08x)" % self.nonce -class msg_mempool(): +class msg_mempool: + __slots__ = () command = b"mempool" def __init__(self): @@ -1160,7 +1231,9 @@ class msg_mempool(): def __repr__(self): return "msg_mempool()" -class msg_sendheaders(): + +class msg_sendheaders: + __slots__ = () command = b"sendheaders" def __init__(self): @@ -1180,7 +1253,8 @@ class msg_sendheaders(): # number of entries # vector of hashes # hash_stop (hash of last desired block header, 0 to get as many as possible) -class msg_getheaders(): +class msg_getheaders: + __slots__ = ("hashstop", "locator",) command = b"getheaders" def __init__(self): @@ -1205,7 +1279,8 @@ class msg_getheaders(): # headers message has # -class msg_headers(): +class msg_headers: + __slots__ = ("headers",) command = b"headers" def __init__(self, headers=None): @@ -1225,7 +1300,8 @@ class msg_headers(): return "msg_headers(headers=%s)" % repr(self.headers) -class msg_reject(): +class msg_reject: + __slots__ = ("code", "data", "message", "reason") command = b"reject" REJECT_MALFORMED = 1 @@ -1256,7 +1332,9 @@ class msg_reject(): return "msg_reject: %s %d %s [%064x]" \ % (self.message, self.code, self.reason, self.data) -class msg_feefilter(): + +class msg_feefilter: + __slots__ = ("feerate",) command = b"feefilter" def __init__(self, feerate=0): @@ -1273,7 +1351,9 @@ class msg_feefilter(): def __repr__(self): return "msg_feefilter(feerate=%08x)" % self.feerate -class msg_sendcmpct(): + +class msg_sendcmpct: + __slots__ = ("announce", "version") command = b"sendcmpct" def __init__(self): @@ -1293,7 +1373,9 @@ class msg_sendcmpct(): def __repr__(self): return "msg_sendcmpct(announce=%s, version=%lu)" % (self.announce, self.version) -class msg_cmpctblock(): + +class msg_cmpctblock: + __slots__ = ("header_and_shortids",) command = b"cmpctblock" def __init__(self, header_and_shortids = None): @@ -1311,7 +1393,9 @@ class msg_cmpctblock(): def __repr__(self): return "msg_cmpctblock(HeaderAndShortIDs=%s)" % repr(self.header_and_shortids) -class msg_getblocktxn(): + +class msg_getblocktxn: + __slots__ = ("block_txn_request",) command = b"getblocktxn" def __init__(self): @@ -1329,7 +1413,9 @@ class msg_getblocktxn(): def __repr__(self): return "msg_getblocktxn(block_txn_request=%s)" % (repr(self.block_txn_request)) -class msg_blocktxn(): + +class msg_blocktxn: + __slots__ = ("block_transactions",) command = b"blocktxn" def __init__(self): @@ -1346,7 +1432,10 @@ class msg_blocktxn(): def __repr__(self): return "msg_blocktxn(block_transactions=%s)" % (repr(self.block_transactions)) + class msg_witness_blocktxn(msg_blocktxn): + __slots__ = () + def serialize(self): r = b"" r += self.block_transactions.serialize(with_witness=True) diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py index 375d6334f..2fe44010b 100644 --- a/test/functional/test_framework/script.py +++ b/test/functional/test_framework/script.py @@ -26,7 +26,7 @@ def hash160(s): _opcode_instances = [] class CScriptOp(int): """A single script opcode""" - __slots__ = [] + __slots__ = () @staticmethod def encode_op_pushdata(d): @@ -361,8 +361,11 @@ class CScriptTruncatedPushDataError(CScriptInvalidError): self.data = data super(CScriptTruncatedPushDataError, self).__init__(msg) + # This is used, eg, for blockchain heights in coinbase scripts (bip34) -class CScriptNum(): +class CScriptNum: + __slots__ = ("value",) + def __init__(self, d=0): self.value = d @@ -393,6 +396,8 @@ class CScript(bytes): iter(script) however does iterate by opcode. """ + __slots__ = () + @classmethod def __coerce_instance(cls, other): # Coerce other into bytes From 17b42f4122740a7d9c91f3b42f77907e9cdcf680 Mon Sep 17 00:00:00 2001 From: Justin Turner Arthur Date: Tue, 25 Sep 2018 21:10:13 -0500 Subject: [PATCH 4/5] Check for specific tx acceptance failures based on script signature --- test/functional/p2p_segwit.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 1b3c5bd1f..afbbfa899 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -769,12 +769,16 @@ class SegWitTest(BitcoinTestFramework): # will require a witness to spend a witness program regardless of # segwit activation. Note that older bitcoind's that are not # segwit-aware would also reject this for failing CLEANSTACK. - test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False) + with self.nodes[0].assert_debug_log( + expected_msgs=(spend_tx.hash, 'was not accepted: non-mandatory-script-verify-flag (Witness program was passed an empty witness)')): + test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False) # Try to put the witness script in the scriptSig, should also fail. spend_tx.vin[0].scriptSig = CScript([p2wsh_pubkey, b'a']) spend_tx.rehash() - test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False) + with self.nodes[0].assert_debug_log( + expected_msgs=('Not relaying invalid transaction {}'.format(spend_tx.hash), 'was not accepted: mandatory-script-verify-flag-failed (Script evaluated without error but finished with a false/empty top stack element)')): + test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False) # Now put the witness script in the witness, should succeed after # segwit activates. From e46023287689fc8e79b9a82fe1a827d87c769423 Mon Sep 17 00:00:00 2001 From: Justin Turner Arthur Date: Wed, 26 Sep 2018 22:13:06 -0500 Subject: [PATCH 5/5] Document fixed attribute behavior in critical test framework classes. Per @jimmysong's suggestion in bitcoin/bitcoin#14305. Also corrects module for network objects and wrappers. --- test/functional/README.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/functional/README.md b/test/functional/README.md index 6929ab599..d40052ac9 100644 --- a/test/functional/README.md +++ b/test/functional/README.md @@ -60,6 +60,11 @@ don't have test cases for. - When calling RPCs with lots of arguments, consider using named keyword arguments instead of positional arguments to make the intent of the call clear to readers. +- Many of the core test framework classes such as `CBlock` and `CTransaction` + don't allow new attributes to be added to their objects at runtime like + typical Python objects allow. This helps prevent unpredictable side effects + from typographical errors or usage of the objects outside of their intended + purpose. #### RPC and P2P definitions @@ -72,7 +77,7 @@ P2P messages. These can be found in the following source files: #### Using the P2P interface -- `mininode.py` contains all the definitions for objects that pass +- `messages.py` contains all the definitions for objects that pass over the network (`CBlock`, `CTransaction`, etc, along with the network-level wrappers for them, `msg_block`, `msg_tx`, etc).