pyln: Add safe fallback results for hooks

contrib/pyln-client/pyln/client/plugin.py

@@ -97,6 +97,26 @@ def _write_result(self, result):
         self.plugin._write_locked(result)
 
 
+# If a hook call fails we need to coerce it into something the main daemon can
+# handle. Returning an error is not an option since we explicitly do not allow
+# those as a response to the calls, otherwise the only option we have is to
+# crash the main daemon. The goal of these is to present a safe fallback
+# should the hook call fail unexpectedly.
+hook_fallbacks = {
+    'htlc_accepted': {
+        'result': 'fail',
+        'failure_message': '2002'  # Fail with a temporary node failure
+    },
+    'peer_connected': {'result': 'continue'},
+    # commitment_revocation cannot recover from failing, let lightningd crash
+    # db_write cannot recover from failing, let lightningd crash
+    'invoice_payment': {'result': 'continue'},
+    'openchannel': {'result': 'reject'},
+    'rpc_command': {'result': 'continue'},
+    'custommsg': {'result': 'continue'},
+}
+
+
 class Plugin(object):
     """Controls interactions with lightningd, and bundles functionality.
 
@@ -453,7 +473,18 @@ def _dispatch_request(self, request):
                 # return a result or raise an exception.
                 request.set_result(result)
         except Exception as e:
-            request.set_exception(e)
+            if name in hook_fallbacks:
+                response = hook_fallbacks[name]
+                self.log((
+                    "Hook handler for {name} failed with an exception. "
+                    "Returning safe fallback response {response} to avoid "
+                    "crashing the main daemon. Please contact the plugin "
+                    "author!"
+                ).format(name=name, response=response), level="error")
+
+                request.set_result(response)
+            else:
+                request.set_exception(e)
             self.log(traceback.format_exc())
 
     def _dispatch_notification(self, request):

tests/plugins/htlc_accepted-crash.py

@@ -0,0 +1,14 @@
+#!/usr/bin/env python3
+from pyln.client import Plugin
+
+
+plugin = Plugin()
+
+
+@plugin.hook('htlc_accepted')
+def on_htlc_accepted(plugin, **kwargs):
+    plugin.log("Crashing on purpose...")
+    raise ValueError()
+
+
+plugin.run()

tests/test_plugin.py

@@ -1741,3 +1741,32 @@ def get_logfile_match(logpath, regex):
 def test_dev_builtin_plugins_unimportant(node_factory):
     n = node_factory.get_node(options={"dev-builtin-plugins-unimportant": None})
     n.rpc.plugin_stop(plugin="pay")
+
+
+def test_htlc_accepted_hook_crash(node_factory, executor):
+    """Test that we do not hang incoming HTLCs if the hook plugin crashes.
+
+    Reproduces #3748.
+    """
+    plugin = os.path.join(os.getcwd(), 'tests/plugins/htlc_accepted-crash.py')
+    l1 = node_factory.get_node()
+    l2 = node_factory.get_node(
+        options={'plugin': plugin},
+        allow_broken_log=True
+    )
+    l1.connect(l2)
+    l1.fund_channel(l2, 10**6)
+
+    i = l2.rpc.invoice(500, "crashpls", "crashpls")['bolt11']
+
+    # This should still succeed
+
+    f = executor.submit(l1.rpc.pay, i)
+
+    l2.daemon.wait_for_log(r'Crashing on purpose...')
+    l2.daemon.wait_for_log(
+        r'Hook handler for htlc_accepted failed with an exception.'
+    )
+
+    with pytest.raises(RpcError, match=r'failed: WIRE_TEMPORARY_NODE_FAILURE'):
+        f.result(10)