11 files changed, 299 insertions, 22 deletions
diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py
index 26bc6a73df..20ab0fdd1d 100755
--- a/qa/pull-tester/rpc-tests.py
+++ b/qa/pull-tester/rpc-tests.py
@@ -154,6 +154,7 @@ testScripts = [
     'bumpfee.py',
     'rpcnamedargs.py',
     'listsinceblock.py',
+    'p2p-leaktests.py',
 ]
 if ENABLE_ZMQ:
     testScripts.append('zmq_test.py')
@@ -168,6 +169,7 @@ testScriptsExt = [
     # vv Tests less than 2m vv
     'bip68-sequence.py',
     'getblocktemplate_longpoll.py',
+    'p2p-timeouts.py',
     # vv Tests less than 60s vv
     'bip9-softforks.py',
     'p2p-feefilter.py',
diff --git a/qa/rpc-tests/forknotify.py b/qa/rpc-tests/forknotify.py
index a1901aedab..8b1403083f 100755
--- a/qa/rpc-tests/forknotify.py
+++ b/qa/rpc-tests/forknotify.py
@@ -22,7 +22,7 @@ class ForkNotifyTest(BitcoinTestFramework):
     def setup_network(self):
         self.nodes = []
         self.alert_filename = os.path.join(self.options.tmpdir, "alert.txt")
-        with open(self.alert_filename, 'w', encoding='utf8') as f:
+        with open(self.alert_filename, 'w', encoding='utf8'):
             pass  # Just open then close to create zero-length file
         self.nodes.append(start_node(0, self.options.tmpdir,
                             ["-blockversion=2", "-alertnotify=echo %s >> \"" + self.alert_filename + "\""]))
diff --git a/qa/rpc-tests/p2p-leaktests.py b/qa/rpc-tests/p2p-leaktests.py
new file mode 100755
index 0000000000..41ca84d779
--- /dev/null
+++ b/qa/rpc-tests/p2p-leaktests.py
@@ -0,0 +1,145 @@
+#!/usr/bin/env python3
+# Copyright (c) 2017 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+from test_framework.mininode import *
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import *
+
+'''
+Test for message sending before handshake completion
+
+A node should never send anything other than VERSION/VERACK/REJECT until it's
+received a VERACK.
+
+This test connects to a node and sends it a few messages, trying to intice it
+into sending us something it shouldn't.
+'''
+
+banscore = 10
+
+class CLazyNode(NodeConnCB):
+    def __init__(self):
+        self.connection = None
+        self.unexpected_msg = False
+        self.connected = False
+        super().__init__()
+
+    def add_connection(self, conn):
+        self.connection = conn
+
+    def send_message(self, message):
+        self.connection.send_message(message)
+
+    def bad_message(self, message):
+        self.unexpected_msg = True
+        print("should not have received message: %s" % message.command)
+
+    def on_open(self, conn):
+        self.connected = True
+
+    def on_version(self, conn, message): self.bad_message(message)
+    def on_verack(self, conn, message): self.bad_message(message)
+    def on_reject(self, conn, message): self.bad_message(message)
+    def on_inv(self, conn, message): self.bad_message(message)
+    def on_addr(self, conn, message): self.bad_message(message)
+    def on_alert(self, conn, message): self.bad_message(message)
+    def on_getdata(self, conn, message): self.bad_message(message)
+    def on_getblocks(self, conn, message): self.bad_message(message)
+    def on_tx(self, conn, message): self.bad_message(message)
+    def on_block(self, conn, message): self.bad_message(message)
+    def on_getaddr(self, conn, message): self.bad_message(message)
+    def on_headers(self, conn, message): self.bad_message(message)
+    def on_getheaders(self, conn, message): self.bad_message(message)
+    def on_ping(self, conn, message): self.bad_message(message)
+    def on_mempool(self, conn): self.bad_message(message)
+    def on_pong(self, conn, message): self.bad_message(message)
+    def on_feefilter(self, conn, message): self.bad_message(message)
+    def on_sendheaders(self, conn, message): self.bad_message(message)
+    def on_sendcmpct(self, conn, message): self.bad_message(message)
+    def on_cmpctblock(self, conn, message): self.bad_message(message)
+    def on_getblocktxn(self, conn, message): self.bad_message(message)
+    def on_blocktxn(self, conn, message): self.bad_message(message)
+
+# Node that never sends a version. We'll use this to send a bunch of messages
+# anyway, and eventually get disconnected.
+class CNodeNoVersionBan(CLazyNode):
+    def __init__(self):
+        super().__init__()
+
+    # send a bunch of veracks without sending a message. This should get us disconnected.
+    # NOTE: implementation-specific check here. Remove if bitcoind ban behavior changes
+    def on_open(self, conn):
+        super().on_open(conn)
+        for i in range(banscore):
+            self.send_message(msg_verack())
+
+    def on_reject(self, conn, message): pass
+
+# Node that never sends a version. This one just sits idle and hopes to receive
+# any message (it shouldn't!)
+class CNodeNoVersionIdle(CLazyNode):
+    def __init__(self):
+        super().__init__()
+
+# Node that sends a version but not a verack.
+class CNodeNoVerackIdle(CLazyNode):
+    def __init__(self):
+        self.version_received = False
+        super().__init__()
+
+    def on_reject(self, conn, message): pass
+    def on_verack(self, conn, message): pass
+    # When version is received, don't reply with a verack. Instead, see if the
+    # node will give us a message that it shouldn't. This is not an exhaustive
+    # list!
+    def on_version(self, conn, message):
+        self.version_received = True
+        conn.send_message(msg_ping())
+        conn.send_message(msg_getaddr())
+
+class P2PLeakTest(BitcoinTestFramework):
+    def __init__(self):
+        super().__init__()
+        self.num_nodes = 1
+    def setup_network(self):
+        extra_args = [['-debug', '-banscore='+str(banscore)]
+                      for i in range(self.num_nodes)]
+        self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, extra_args)
+
+    def run_test(self):
+        no_version_bannode = CNodeNoVersionBan()
+        no_version_idlenode = CNodeNoVersionIdle()
+        no_verack_idlenode = CNodeNoVerackIdle()
+
+        connections = []
+        connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], no_version_bannode, send_version=False))
+        connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], no_version_idlenode, send_version=False))
+        connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], no_verack_idlenode))
+        no_version_bannode.add_connection(connections[0])
+        no_version_idlenode.add_connection(connections[1])
+        no_verack_idlenode.add_connection(connections[2])
+
+        NetworkThread().start()  # Start up network handling in another thread
+
+        assert(wait_until(lambda: no_version_bannode.connected and no_version_idlenode.connected and no_verack_idlenode.version_received, timeout=10))
+
+        # Mine a block and make sure that it's not sent to the connected nodes
+        self.nodes[0].generate(1)
+
+        #Give the node enough time to possibly leak out a message
+        time.sleep(5)
+
+        #This node should have been banned
+        assert(no_version_bannode.connection.state == "closed")
+
+        [conn.disconnect_node() for conn in connections]
+
+        # Make sure no unexpected messages came in
+        assert(no_version_bannode.unexpected_msg == False)
+        assert(no_version_idlenode.unexpected_msg == False)
+        assert(no_verack_idlenode.unexpected_msg == False)
+
+if __name__ == '__main__':
+    P2PLeakTest().main()
diff --git a/qa/rpc-tests/p2p-segwit.py b/qa/rpc-tests/p2p-segwit.py
index a7858ad3d8..2f339bb54f 100755
--- a/qa/rpc-tests/p2p-segwit.py
+++ b/qa/rpc-tests/p2p-segwit.py
@@ -951,7 +951,6 @@ class SegWitTest(BitcoinTestFramework):
         tx.rehash()
 
         tx_hash = tx.sha256
-        tx_value = tx.vout[0].nValue
 
         # Verify that unnecessary witnesses are rejected.
         self.test_node.announce_tx_and_wait_for_getdata(tx)
diff --git a/qa/rpc-tests/p2p-timeouts.py b/qa/rpc-tests/p2p-timeouts.py
new file mode 100755
index 0000000000..f1b190587d
--- /dev/null
+++ b/qa/rpc-tests/p2p-timeouts.py
@@ -0,0 +1,103 @@
+#!/usr/bin/env python3
+# Copyright (c) 2016 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+""" TimeoutsTest -- test various net timeouts (only in extended tests)
+
+- Create three bitcoind nodes:
+
+    no_verack_node - we never send a verack in response to their version
+    no_version_node - we never send a version (only a ping)
+    no_send_node - we never send any P2P message.
+
+- Start all three nodes
+- Wait 1 second
+- Assert that we're connected
+- Send a ping to no_verack_node and no_version_node
+- Wait 30 seconds
+- Assert that we're still connected
+- Send a ping to no_verack_node and no_version_node
+- Wait 31 seconds
+- Assert that we're no longer connected (timeout to receive version/verack is 60 seconds)
+"""
+
+from time import sleep
+
+from test_framework.mininode import *
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import *
+
+class TestNode(SingleNodeConnCB):
+    def __init__(self):
+        SingleNodeConnCB.__init__(self)
+        self.connected = False
+        self.received_version = False
+
+    def on_open(self, conn):
+        self.connected = True
+
+    def on_close(self, conn):
+        self.connected = False
+
+    def on_version(self, conn, message):
+        # Don't send a verack in response
+        self.received_version = True
+
+class TimeoutsTest(BitcoinTestFramework):
+    def __init__(self):
+        super().__init__()
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+
+    def setup_network(self):
+        self.nodes = []
+
+        # Start up node0 to be a version 1, pre-segwit node.
+        self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, 
+                [["-debug", "-logtimemicros=1"]])
+
+    def run_test(self):
+        # Setup the p2p connections and start up the network thread.
+        self.no_verack_node = TestNode() # never send verack
+        self.no_version_node = TestNode() # never send version (just ping)
+        self.no_send_node = TestNode() # never send anything
+
+        connections = []
+        connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], self.no_verack_node))
+        connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], self.no_version_node, send_version=False))
+        connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], self.no_send_node, send_version=False))
+        self.no_verack_node.add_connection(connections[0])
+        self.no_version_node.add_connection(connections[1])
+        self.no_send_node.add_connection(connections[2])
+
+        NetworkThread().start()  # Start up network handling in another thread
+
+        sleep(1)
+
+        assert(self.no_verack_node.connected)
+        assert(self.no_version_node.connected)
+        assert(self.no_send_node.connected)
+
+        ping_msg = msg_ping()
+        connections[0].send_message(ping_msg)
+        connections[1].send_message(ping_msg)
+
+        sleep(30)
+
+        assert(self.no_verack_node.received_version)
+
+        assert(self.no_verack_node.connected)
+        assert(self.no_version_node.connected)
+        assert(self.no_send_node.connected)
+
+        connections[0].send_message(ping_msg)
+        connections[1].send_message(ping_msg)
+
+        sleep(31)
+
+        assert(not self.no_verack_node.connected)
+        assert(not self.no_version_node.connected)
+        assert(not self.no_send_node.connected)
+
+if __name__ == '__main__':
+    TimeoutsTest().main()
diff --git a/qa/rpc-tests/receivedby.py b/qa/rpc-tests/receivedby.py
index 4f17b661cb..1b90b23330 100755
--- a/qa/rpc-tests/receivedby.py
+++ b/qa/rpc-tests/receivedby.py
@@ -14,7 +14,6 @@ def get_sub_array_from_array(object_array, to_match):
         Finds and returns a sub array from an array of arrays.
         to_match should be a unique idetifier of a sub array
     '''
-    num_matched = 0
     for item in object_array:
         all_match = True
         for key,value in to_match.items():
@@ -104,7 +103,7 @@ class ReceivedByTest(BitcoinTestFramework):
         received_by_account_json = get_sub_array_from_array(self.nodes[1].listreceivedbyaccount(),{"account":account})
         if len(received_by_account_json) == 0:
             raise AssertionError("No accounts found in node")
-        balance_by_account = rec_by_accountArr = self.nodes[1].getreceivedbyaccount(account)
+        balance_by_account = self.nodes[1].getreceivedbyaccount(account)
 
         txid = self.nodes[0].sendtoaddress(addr, 0.1)
         self.sync_all()
diff --git a/qa/rpc-tests/replace-by-fee.py b/qa/rpc-tests/replace-by-fee.py
index 8aba06c60c..2b29dfdd2b 100755
--- a/qa/rpc-tests/replace-by-fee.py
+++ b/qa/rpc-tests/replace-by-fee.py
@@ -393,7 +393,6 @@ class ReplaceByFeeTest(BitcoinTestFramework):
         utxo = make_utxo(self.nodes[0], initial_nValue)
         fee = int(0.0001*COIN)
         split_value = int((initial_nValue-fee)/(MAX_REPLACEMENT_LIMIT+1))
-        actual_fee = initial_nValue - split_value*(MAX_REPLACEMENT_LIMIT+1)
 
         outputs = []
         for i in range(MAX_REPLACEMENT_LIMIT+1):
diff --git a/qa/rpc-tests/rpcnamedargs.py b/qa/rpc-tests/rpcnamedargs.py
index 0484204668..da2d8f040f 100755
--- a/qa/rpc-tests/rpcnamedargs.py
+++ b/qa/rpc-tests/rpcnamedargs.py
@@ -37,7 +37,7 @@ class NamedArgumentTest(BitcoinTestFramework):
         h = node.help(command='getinfo')
         assert(h.startswith('getinfo\n'))
 
-        assert_raises_jsonrpc(-8, node.help, random='getinfo')
+        assert_raises_jsonrpc(-8, 'Unknown named parameter', node.help, random='getinfo')
 
         h = node.getblockhash(height=0)
         node.getblock(blockhash=h)
diff --git a/qa/rpc-tests/test_framework/mininode.py b/qa/rpc-tests/test_framework/mininode.py
index 91daa4ab1f..5b563c58ae 100755
--- a/qa/rpc-tests/test_framework/mininode.py
+++ b/qa/rpc-tests/test_framework/mininode.py
@@ -1540,6 +1540,7 @@ class NodeConnCB(object):
         if conn.ver_send > BIP0031_VERSION:
             conn.send_message(msg_pong(message.nonce))
     def on_reject(self, conn, message): pass
+    def on_open(self, conn): pass
     def on_close(self, conn): pass
     def on_mempool(self, conn): pass
     def on_pong(self, conn, message): pass
@@ -1614,7 +1615,7 @@ class NodeConn(asyncore.dispatcher):
         "regtest": b"\xfa\xbf\xb5\xda",   # regtest
     }
 
-    def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE_NETWORK):
+    def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE_NETWORK, send_version=True):
         asyncore.dispatcher.__init__(self, map=mininode_socket_map)
         self.log = logging.getLogger("NodeConn(%s:%d)" % (dstaddr, dstport))
         self.dstaddr = dstaddr
@@ -1631,14 +1632,16 @@ class NodeConn(asyncore.dispatcher):
         self.disconnect = False
         self.nServices = 0
 
-        # stuff version msg into sendbuf
-        vt = msg_version()
-        vt.nServices = services
-        vt.addrTo.ip = self.dstaddr
-        vt.addrTo.port = self.dstport
-        vt.addrFrom.ip = "0.0.0.0"
-        vt.addrFrom.port = 0
-        self.send_message(vt, True)
+        if send_version:
+            # stuff version msg into sendbuf
+            vt = msg_version()
+            vt.nServices = services
+            vt.addrTo.ip = self.dstaddr
+            vt.addrTo.port = self.dstport
+            vt.addrFrom.ip = "0.0.0.0"
+            vt.addrFrom.port = 0
+            self.send_message(vt, True)
+
         print('MiniNode: Connecting to Bitcoin Node IP # ' + dstaddr + ':' \
             + str(dstport))
 
@@ -1652,8 +1655,10 @@ class NodeConn(asyncore.dispatcher):
         self.log.debug(msg)
 
     def handle_connect(self):
-        self.show_debug_msg("MiniNode: Connected & Listening: \n")
-        self.state = "connected"
+        if self.state != "connected":
+            self.show_debug_msg("MiniNode: Connected & Listening: \n")
+            self.state = "connected"
+            self.cb.on_open(self)
 
     def handle_close(self):
         self.show_debug_msg("MiniNode: Closing Connection to %s:%d... "
@@ -1681,11 +1686,20 @@ class NodeConn(asyncore.dispatcher):
 
     def writable(self):
         with mininode_lock:
+            pre_connection = self.state == "connecting"
             length = len(self.sendbuf)
-        return (length > 0)
+        return (length > 0 or pre_connection)
 
     def handle_write(self):
         with mininode_lock:
+            # asyncore does not expose socket connection, only the first read/write
+            # event, thus we must check connection manually here to know when we
+            # actually connect
+            if self.state == "connecting":
+                self.handle_connect()
+            if not self.writable():
+                return
+
             try:
                 sent = self.send(self.sendbuf)
             except:
diff --git a/qa/rpc-tests/test_framework/util.py b/qa/rpc-tests/test_framework/util.py
index aca82c8b6f..dc8555c44c 100644
--- a/qa/rpc-tests/test_framework/util.py
+++ b/qa/rpc-tests/test_framework/util.py
@@ -550,13 +550,30 @@ def assert_raises_message(exc, message, fun, *args, **kwds):
     else:
         raise AssertionError("No exception raised")
 
-def assert_raises_jsonrpc(code, fun, *args, **kwds):
-    '''Check for specific JSONRPC exception code'''
+def assert_raises_jsonrpc(code, message, fun, *args, **kwds):
+    """Run an RPC and verify that a specific JSONRPC exception code and message is raised.
+
+    Calls function `fun` with arguments `args` and `kwds`. Catches a JSONRPCException
+    and verifies that the error code and message are as expected. Throws AssertionError if
+    no JSONRPCException was returned or if the error code/message are not as expected.
+
+    Args:
+        code (int), optional: the error code returned by the RPC call (defined
+            in src/rpc/protocol.h). Set to None if checking the error code is not required.
+        message (string), optional: [a substring of] the error string returned by the
+            RPC call. Set to None if checking the error string is not required
+        fun (function): the function to call. This should be the name of an RPC.
+        args*: positional arguments for the function.
+        kwds**: named arguments for the function.
+    """
     try:
         fun(*args, **kwds)
     except JSONRPCException as e:
-        if e.error["code"] != code:
+        # JSONRPCException was thrown as expected. Check the code and message values are correct.
+        if (code is not None) and (code != e.error["code"]):
             raise AssertionError("Unexpected JSONRPC error code %i" % e.error["code"])
+        if (message is not None) and (message not in e.error['message']):
+            raise AssertionError("Expected substring not found:"+e.error['message'])
     except Exception as e:
         raise AssertionError("Unexpected exception raised: "+type(e).__name__)
     else:
diff --git a/qa/rpc-tests/wallet.py b/qa/rpc-tests/wallet.py
index f325ecb4a3..c82e0adffa 100755
--- a/qa/rpc-tests/wallet.py
+++ b/qa/rpc-tests/wallet.py
@@ -359,7 +359,6 @@ class WalletTest (BitcoinTestFramework):
         rawtx = self.nodes[0].createrawtransaction([{"txid":singletxid, "vout":0}], {chain_addrs[0]:node0_balance/2-Decimal('0.01'), chain_addrs[1]:node0_balance/2-Decimal('0.01')})
         signedtx = self.nodes[0].signrawtransaction(rawtx)
         singletxid = self.nodes[0].sendrawtransaction(signedtx["hex"])
-        txids = [singletxid, singletxid]
         self.nodes[0].generate(1)
 
         # Make a long chain of unconfirmed payments without hitting mempool limit