Replace the fake-bitcoin-cli with a proxy that can be instrumented from tests

Makefile

@@ -38,7 +38,7 @@ ifeq ($(COMPAT),1)
 COMPAT_CFLAGS=-DCOMPAT_V052=1 -DCOMPAT_V060=1
 endif
 
-PYTEST_OPTS := -v -x
+PYTEST_OPTS := -v
 
 # This is where we add new features as bitcoin adds them.
 FEATURES :=
@@ -205,8 +205,14 @@ ifneq ($(TEST_GROUP_COUNT),)
 PYTEST_OPTS += --test-group=$(TEST_GROUP) --test-group-count=$(TEST_GROUP_COUNT)
 endif
 
+# If we run the tests in parallel we can speed testing up by a lot, however we
+# then don't exit on the first error, since that'd kill the other tester
+# processes and result in loads in loads of output. So we only tell py.test to
+# abort early if we aren't running in parallel.
 ifneq ($(PYTEST_PAR),)
 PYTEST_OPTS += -n=$(PYTEST_PAR)
+else
+PYTEST_OPTS += -x
 endif
 
 check:

contrib/Dockerfile.builder

@@ -51,4 +51,4 @@ RUN cd /tmp/ && \
     rm -rf bitcoin.tar.gz /tmp/bitcoin-$BITCOIN_VERSION
 
 RUN pip3 install --upgrade pip && \
-    python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 ephemeral-port-reserve==1.1.0 pytest-xdist==1.22.2 flaky==3.4.0
+    python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 ephemeral-port-reserve==1.1.0 pytest-xdist==1.22.2 flaky==3.4.0 CherryPy==17.3.0 Flask==1.0.2

contrib/Dockerfile.builder.i386

@@ -51,4 +51,4 @@ RUN cd /tmp/ && \
     rm -rf bitcoin.tar.gz /tmp/bitcoin-$BITCOIN_VERSION
 
 RUN pip3 install --upgrade pip && \
-    python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 ephemeral-port-reserve==1.1.0 pytest-xdist==1.22.2 flaky==3.4.0
+    python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 ephemeral-port-reserve==1.1.0 pytest-xdist==1.22.2 flaky==3.4.0 CherryPy==17.3.0 Flask==1.0.2

tests/btcproxy.py

@@ -0,0 +1,104 @@
+""" A bitcoind proxy that allows instrumentation and canned responses
+"""
+from flask import Flask, request
+from bitcoin.rpc import JSONRPCError
+from bitcoin.rpc import RawProxy as BitcoinProxy
+from cheroot.wsgi import Server
+from cheroot.wsgi import PathInfoDispatcher
+
+import decimal
+import flask
+import json
+import logging
+import os
+import threading
+
+
+class DecimalEncoder(json.JSONEncoder):
+    """By default json.dumps does not handle Decimals correctly, so we override it's handling
+    """
+    def default(self, o):
+        if isinstance(o, decimal.Decimal):
+            return "{:.8f}".format(float(o))
+        return super(DecimalEncoder, self).default(o)
+
+
+class BitcoinRpcProxy(object):
+    def __init__(self, bitcoind, rpcport=0):
+        self.app = Flask("BitcoindProxy")
+        self.app.add_url_rule("/", "API entrypoint", self.proxy, methods=['POST'])
+        self.rpcport = rpcport
+        self.mocks = {}
+        self.bitcoind = bitcoind
+        self.request_count = 0
+
+    def _handle_request(self, r):
+        conf_file = os.path.join(self.bitcoind.bitcoin_dir, 'bitcoin.conf')
+        brpc = BitcoinProxy(btc_conf_file=conf_file)
+        method = r['method']
+
+        # If we have set a mock for this method reply with that instead of
+        # forwarding the request.
+        if method in self.mocks and type(method) == dict:
+            return self.mocks[method]
+        elif method in self.mocks and callable(self.mocks[method]):
+            return self.mocks[method](r)
+
+        try:
+            reply = {
+                "result": brpc._call(r['method'], *r['params']),
+                "error": None,
+                "id": r['id']
+            }
+        except JSONRPCError as e:
+            reply = {
+                "error": e.error,
+                "id": r['id']
+            }
+        self.request_count += 1
+        return reply
+
+    def proxy(self):
+        r = json.loads(request.data.decode('ASCII'))
+
+        if isinstance(r, list):
+            reply = [self._handle_request(subreq) for subreq in r]
+        else:
+            reply = self._handle_request(r)
+
+        response = flask.Response(json.dumps(reply, cls=DecimalEncoder))
+        response.headers['Content-Type'] = 'application/json'
+        return response
+
+    def start(self):
+        d = PathInfoDispatcher({'/': self.app})
+        self.server = Server(('0.0.0.0', self.rpcport), d)
+        self.proxy_thread = threading.Thread(target=self.server.start)
+        self.proxy_thread.daemon = True
+        self.proxy_thread.start()
+
+        # Now that bitcoind is running on the real rpcport, let's tell all
+        # future callers to talk to the proxyport. We use the bind_addr as a
+        # signal that the port is bound and accepting connections.
+        while self.server.bind_addr[1] == 0:
+            pass
+        self.rpcport = self.server.bind_addr[1]
+        logging.debug("BitcoinRpcProxy proxying incoming port {} to {}".format(self.rpcport, self.bitcoind.rpcport))
+
+    def stop(self):
+        self.server.stop()
+        self.proxy_thread.join()
+        logging.debug("BitcoinRpcProxy shut down after processing {} requests".format(self.request_count))
+
+    def mock_rpc(self, method, response=None):
+        """Mock the response to a future RPC call of @method
+
+        The response can either be a dict with the full JSON-RPC response, or a
+        function that returns such a response. If the response is None the mock
+        is removed and future calls will be passed through to bitcoind again.
+
+        """
+        if response is not None:
+            self.mocks[method] = response
+        elif method in self.mocks:
+            del self.mocks[method]

tests/fixtures.py

@@ -1,5 +1,5 @@
 from concurrent import futures
-from utils import NodeFactory
+from utils import NodeFactory, BitcoinD
 
 import logging
 import os
@@ -8,7 +8,6 @@
 import shutil
 import sys
 import tempfile
-import utils
 
 
 with open('config.vars') as configfile:
@@ -69,7 +68,7 @@ def test_name(request):
 
 @pytest.fixture
 def bitcoind(directory):
-    bitcoind = utils.BitcoinD(bitcoin_dir=directory, rpcport=None)
+    bitcoind = BitcoinD(bitcoin_dir=directory)
     try:
         bitcoind.start()
     except Exception:

tests/requirements.txt

@@ -3,3 +3,5 @@ ephemeral-port-reserve==1.1.0
 pytest-forked==0.2
 pytest-xdist==1.22.2
 flaky==3.4.0
+CherryPy==17.3.0
+Flask==1.0.2

tests/test_connection.py

@@ -1088,28 +1088,35 @@ def test_fundee_forget_funding_tx_unconfirmed(node_factory, bitcoind):
     # Let blocks settle.
     time.sleep(1)
 
-    # Prevent funder from broadcasting funding tx.
-    l1.bitcoind_cmd_override('exit 1')
+    def mock_sendrawtransaction(r):
+        return {'error': 'sendrawtransaction disabled'}
+
+    # Prevent funder from broadcasting funding tx (any tx really).
+    l1.daemon.rpcproxy.mock_rpc('sendrawtransaction', mock_sendrawtransaction)
+
     # Fund the channel.
     # The process will complete, but funder will be unable
     # to broadcast and confirm funding tx.
     l1.rpc.fundchannel(l2.info['id'], 10**6)
-    # Prevent l1 from timing out bitcoin-cli.
-    l1.bitcoind_cmd_remove_override()
+
     # Generate blocks until unconfirmed.
     bitcoind.generate_block(blocks)
 
     # fundee will forget channel!
     l2.daemon.wait_for_log('Forgetting channel: It has been {} blocks'.format(blocks))
+
     # fundee will also forget and disconnect from peer.
     assert len(l2.rpc.listpeers(l1.info['id'])['peers']) == 0
 
 
 @unittest.skipIf(not DEVELOPER, "needs dev_fail")
 def test_no_fee_estimate(node_factory, bitcoind, executor):
     l1 = node_factory.get_node(start=False)
-    l1.bitcoind_cmd_override(cmd='estimatesmartfee',
-                             failscript="""echo '{ "errors": [ "Insufficient data or no feerate found" ], "blocks": 0 }'; exit 0""")
+
+    # Fail any fee estimation requests until we allow them further down
+    l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', {
+        'error': {"errors": ["Insufficient data or no feerate found"], "blocks": 0}
+    })
     l1.start()
 
     l2 = node_factory.get_node()

tests/test_misc.py

@@ -89,7 +89,12 @@ def test_bitcoin_failure(node_factory, bitcoind):
     # Make sure we're not failing it between getblockhash and getblock.
     sync_blockheight(bitcoind, [l1])
 
-    l1.bitcoind_cmd_override('exit 1')
+    def crash_bitcoincli(r):
+        return {'error': 'go away'}
+
+    # This is not a JSON-RPC response by purpose
+    l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', crash_bitcoincli)
+    l1.daemon.rpcproxy.mock_rpc('getblockhash', crash_bitcoincli)
 
     # This should cause both estimatefee and getblockhash fail
     l1.daemon.wait_for_logs(['estimatesmartfee .* exited with status 1',
@@ -100,7 +105,9 @@ def test_bitcoin_failure(node_factory, bitcoind):
                              'getblockhash .* exited with status 1'])
 
     # Restore, then it should recover and get blockheight.
-    l1.bitcoind_cmd_remove_override()
+    l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', None)
+    l1.daemon.rpcproxy.mock_rpc('getblockhash', None)
+
     bitcoind.generate_block(5)
     sync_blockheight(bitcoind, [l1])
 
@@ -570,8 +577,6 @@ def test_listconfigs(node_factory, bitcoind):
 
     configs = l1.rpc.listconfigs()
     # See utils.py
-    assert configs['bitcoin-datadir'] == bitcoind.bitcoin_dir
-    assert configs['lightning-dir'] == l1.daemon.lightning_dir
     assert configs['allow-deprecated-apis'] is False
     assert configs['network'] == 'regtest'
     assert configs['ignore-fee-limits'] is False
@@ -860,8 +865,9 @@ def test_ipv4_and_ipv6(node_factory):
 
 def test_feerates(node_factory):
     l1 = node_factory.get_node(options={'log-level': 'io'}, start=False)
-    l1.bitcoind_cmd_override(cmd='estimatesmartfee',
-                             failscript="""echo '{ "errors": [ "Insufficient data or no feerate found" ], "blocks": 0 }'; exit 0""")
+    l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', {
+        'error': {"errors": ["Insufficient data or no feerate found"], "blocks": 0}
+    })
     l1.start()
 
     # Query feerates (shouldn't give any!)
@@ -922,6 +928,8 @@ def test_logging(node_factory):
     logpath = os.path.join(l1.daemon.lightning_dir, 'logfile')
     logpath_moved = os.path.join(l1.daemon.lightning_dir, 'logfile_moved')
 
+    l1.daemon.rpcproxy.start()
+    l1.daemon.opts['bitcoin-rpcport'] = l1.daemon.rpcproxy.rpcport
     TailableProc.start(l1.daemon)
     wait_for(lambda: os.path.exists(logpath))