diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c index 8b554218c63f..0d96a9b8e15a 100644 --- a/lightningd/opening_control.c +++ b/lightningd/opening_control.c @@ -765,6 +765,8 @@ struct openchannel_hook_payload { u16 max_accepted_htlcs; u8 channel_flags; u8 *shutdown_scriptpubkey; + const u8 *our_upfront_shutdown_script; + char *errmsg; }; static void @@ -803,13 +805,12 @@ static void openchannel_payload_remove_openingd(struct subd *openingd, payload->openingd = NULL; } -static void openchannel_hook_cb(struct openchannel_hook_payload *payload STEALS, - const char *buffer, - const jsmntok_t *toks) +static void +openchannel_hook_final(struct openchannel_hook_payload *payload STEALS) { struct subd *openingd = payload->openingd; - const u8 *our_upfront_shutdown_script; - const char *errmsg = NULL; + const u8 *our_upfront_shutdown_script = payload->our_upfront_shutdown_script; + const char *errmsg = payload->errmsg; /* We want to free this, whatever happens. */ tal_steal(tmpctx, payload); @@ -820,65 +821,87 @@ static void openchannel_hook_cb(struct openchannel_hook_payload *payload STEALS, tal_del_destructor2(openingd, openchannel_payload_remove_openingd, payload); - /* If we had a hook, check what it says */ - if (buffer) { - const jsmntok_t *t = json_get_member(buffer, toks, "result"); - if (!t) - fatal("Plugin returned an invalid response to the" - " openchannel hook: %.*s", - toks[0].end - toks[0].start, - buffer + toks[0].start); - - if (json_tok_streq(buffer, t, "reject")) { - t = json_get_member(buffer, toks, "error_message"); - if (t) - errmsg = json_strdup(tmpctx, buffer, t); - else - errmsg = ""; - log_debug(openingd->ld->log, - "openchannel_hook_cb says '%s'", - errmsg); - our_upfront_shutdown_script = NULL; - } else if (!json_tok_streq(buffer, t, "continue")) - fatal("Plugin returned an invalid result for the " - "openchannel hook: %.*s", - t->end - t->start, buffer + t->start); - - /* Check for a 'close_to' address passed back */ - if (!errmsg) { - t = json_get_member(buffer, toks, "close_to"); - if (t) { - switch (json_to_address_scriptpubkey(tmpctx, chainparams, - buffer, t, - &our_upfront_shutdown_script)) { - case ADDRESS_PARSE_UNRECOGNIZED: - fatal("Plugin returned an invalid response to the" - " openchannel.close_to hook: %.*s", - t->end - t->start, buffer + t->start); - case ADDRESS_PARSE_WRONG_NETWORK: - fatal("Plugin returned invalid response to the" - " openchannel.close_to hook: address %s is" - " not on network %s", - tal_hex(NULL, our_upfront_shutdown_script), - chainparams->network_name); - case ADDRESS_PARSE_SUCCESS: - errmsg = NULL; - } - } else - our_upfront_shutdown_script = NULL; - } - } else - our_upfront_shutdown_script = NULL; - subd_send_msg(openingd, take(towire_openingd_got_offer_reply(NULL, errmsg, our_upfront_shutdown_script))); } -REGISTER_SINGLE_PLUGIN_HOOK(openchannel, - openchannel_hook_cb, - openchannel_hook_serialize, - struct openchannel_hook_payload *); +static bool +openchannel_hook_deserialize(struct openchannel_hook_payload *payload, + const char *buffer, + const jsmntok_t *toks) +{ + struct subd *openingd = payload->openingd; + + /* already rejected by prior plugin hook in the chain */ + if (payload->errmsg != NULL) + return true; + + if (!toks || !buffer) + return true; + + const jsmntok_t *t_result = json_get_member(buffer, toks, "result"); + const jsmntok_t *t_errmsg = json_get_member(buffer, toks, "error_message"); + const jsmntok_t *t_closeto = json_get_member(buffer, toks, "close_to"); + + if (!t_result) + fatal("Plugin returned an invalid response to the" + " openchannel hook: %.*s", + toks[0].end - toks[0].start, buffer + toks[0].start); + + /* reject */ + if (json_tok_streq(buffer, t_result, "reject")) { + payload->errmsg = ""; + if (t_errmsg) + payload->errmsg = json_strdup(payload, buffer, t_errmsg); + log_debug(openingd->ld->log, + "openchannel_hook rejects and says '%s'", + payload->errmsg); + if (t_closeto) + fatal("Plugin rejected openchannel but also set close_to"); + openchannel_hook_final(payload); + return false; + } else if (!json_tok_streq(buffer, t_result, "continue")) { + fatal("Plugin returned an invalid result for the " + "openchannel hook: %.*s", + t_result->end - t_result->start, buffer + t_result->start); + } + + /* Check for a valid 'close_to' address passed back */ + if (t_closeto) { + /* First plugin can set close_to. Log others. */ + if (payload->our_upfront_shutdown_script != NULL) { + log_unusual(openingd->ld->log, + "openchannel_hook close_to address was" + " already set by other plugin. Ignoring!"); + return true; + } + switch (json_to_address_scriptpubkey(tmpctx, chainparams, + buffer, t_closeto, + &payload->our_upfront_shutdown_script)) { + case ADDRESS_PARSE_UNRECOGNIZED: + fatal("Plugin returned an invalid response to" + " the openchannel.close_to hook: %.*s", + t_closeto->end - t_closeto->start, + buffer + t_closeto->start); + case ADDRESS_PARSE_WRONG_NETWORK: + fatal("Plugin returned invalid response to the" + " openchannel.close_to hook: address %s is" + " not on network %s", + tal_hex(NULL, payload->our_upfront_shutdown_script), + chainparams->network_name); + case ADDRESS_PARSE_SUCCESS: + break; + } + } + return true; +} + +REGISTER_PLUGIN_HOOK(openchannel, + openchannel_hook_deserialize, + openchannel_hook_final, + openchannel_hook_serialize, + struct openchannel_hook_payload *); static void opening_got_offer(struct subd *openingd, const u8 *msg, @@ -896,6 +919,8 @@ static void opening_got_offer(struct subd *openingd, payload = tal(openingd, struct openchannel_hook_payload); payload->openingd = openingd; + payload->our_upfront_shutdown_script = NULL; + payload->errmsg = NULL; if (!fromwire_openingd_got_offer(payload, msg, &payload->funding_satoshis, &payload->push_msat, diff --git a/tests/plugins/accepter_close_to.py b/tests/plugins/accepter_close_to.py deleted file mode 100755 index 3711154d84f8..000000000000 --- a/tests/plugins/accepter_close_to.py +++ /dev/null @@ -1,35 +0,0 @@ -#!/usr/bin/env python3 -"""Simple plugin to test the openchannel_hook's - 'close_to' address functionality. - - If the funding amount is: - - a multiple of 11: we send back a valid address (regtest) - - a multiple of 7: we send back an empty address - - a multiple of 5: we send back an address for the wrong chain (mainnet) - - otherwise: we don't include the close_to -""" - -from pyln.client import Plugin, Millisatoshi - -plugin = Plugin() - - -@plugin.hook('openchannel') -def on_openchannel(openchannel, plugin, **kwargs): - # - a multiple of 11: we send back a valid address (regtest) - if Millisatoshi(openchannel['funding_satoshis']).to_satoshi() % 11 == 0: - return {'result': 'continue', 'close_to': 'bcrt1q7gtnxmlaly9vklvmfj06amfdef3rtnrdazdsvw'} - - # - a multiple of 7: we send back an empty address - if Millisatoshi(openchannel['funding_satoshis']).to_satoshi() % 7 == 0: - return {'result': 'continue', 'close_to': ''} - - # - a multiple of 5: we send back an address for the wrong chain (mainnet) - if Millisatoshi(openchannel['funding_satoshis']).to_satoshi() % 5 == 0: - return {'result': 'continue', 'close_to': 'bc1qlq8srqnz64wgklmqvurv7qnr4rvtq2u96hhfg2'} - - # - otherwise: we don't include the close_to - return {'result': 'continue'} - - -plugin.run() diff --git a/tests/plugins/openchannel_hook_accept.py b/tests/plugins/openchannel_hook_accept.py new file mode 100755 index 000000000000..afc922dc77c5 --- /dev/null +++ b/tests/plugins/openchannel_hook_accept.py @@ -0,0 +1,19 @@ +#!/usr/bin/env python3 +"""Plugin to test openchannel_hook + +Will simply accept any channel. Useful fot testing chained hook. +""" + +from pyln.client import Plugin + +plugin = Plugin() + + +@plugin.hook('openchannel') +def on_openchannel(openchannel, plugin, **kwargs): + msg = "accept on principle" + plugin.log(msg) + return {'result': 'continue'} + + +plugin.run() diff --git a/tests/plugins/openchannel_hook_accepter.py b/tests/plugins/openchannel_hook_accepter.py new file mode 100755 index 000000000000..5e7dec4ed857 --- /dev/null +++ b/tests/plugins/openchannel_hook_accepter.py @@ -0,0 +1,51 @@ +#!/usr/bin/env python3 +"""Simple plugin to test the openchannel_hook's + 'close_to' address functionality. + + If the funding amount is: + - 100005sat: we reject correctly w/o close_to + - 100004sat: we reject invalid by setting a close_to + - 100003sat: we send back a valid address (regtest) + - 100002sat: we send back an empty address + - 100001sat: we send back an address for the wrong chain (mainnet) + - otherwise: we don't include the close_to +""" + +from pyln.client import Plugin, Millisatoshi + +plugin = Plugin() + + +@plugin.hook('openchannel') +def on_openchannel(openchannel, plugin, **kwargs): + # - 100005sat: we reject correctly w/o close_to + if Millisatoshi(openchannel['funding_satoshis']).to_satoshi() == 100005: + msg = "reject for a reason" + plugin.log(msg) + return {'result': 'reject', 'error_message': msg} + + # - 100004sat: we reject invalid by setting a close_to + if Millisatoshi(openchannel['funding_satoshis']).to_satoshi() == 100004: + msg = "I am a broken plugin" + plugin.log(msg) + return {'result': 'reject', 'error_message': msg, + 'close_to': "bcrt1q7gtnxmlaly9vklvmfj06amfdef3rtnrdazdsvw"} + + # - 100003sat: we send back a valid address (regtest) + if Millisatoshi(openchannel['funding_satoshis']).to_satoshi() == 100003: + return {'result': 'continue', 'close_to': 'bcrt1q7gtnxmlaly9vklvmfj06amfdef3rtnrdazdsvw'} + + # - 100002sat: we send back an empty address + if Millisatoshi(openchannel['funding_satoshis']).to_satoshi() == 100002: + return {'result': 'continue', 'close_to': ''} + + # - 100001sat: we send back an address for the wrong chain (mainnet) + if Millisatoshi(openchannel['funding_satoshis']).to_satoshi() == 100001: + return {'result': 'continue', 'close_to': 'bc1qlq8srqnz64wgklmqvurv7qnr4rvtq2u96hhfg2'} + + # - otherwise: accept and don't include the close_to + plugin.log("accept by design") + return {'result': 'continue'} + + +plugin.run() diff --git a/tests/plugins/openchannel_hook_reject.py b/tests/plugins/openchannel_hook_reject.py new file mode 100755 index 000000000000..c89655dde2f3 --- /dev/null +++ b/tests/plugins/openchannel_hook_reject.py @@ -0,0 +1,20 @@ +#!/usr/bin/env python3 +"""Plugin to test openchannel_hook + +Will simply reject any channel with message "reject on principle". +Useful fot testing chained hook. +""" + +from pyln.client import Plugin + +plugin = Plugin() + + +@plugin.hook('openchannel') +def on_openchannel(openchannel, plugin, **kwargs): + msg = "reject on principle" + plugin.log(msg) + return {'result': 'reject', 'error_message': msg} + + +plugin.run() diff --git a/tests/test_connection.py b/tests/test_connection.py index d6b35aa2958b..f7ab73ffba1d 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -1050,13 +1050,13 @@ def test_funding_cancel_race(node_factory, bitcoind, executor): def test_funding_close_upfront(node_factory, bitcoind): l1 = node_factory.get_node() - opts = {'plugin': os.path.join(os.getcwd(), 'tests/plugins/accepter_close_to.py')} + opts = {'plugin': os.path.join(os.getcwd(), 'tests/plugins/openchannel_hook_accepter.py')} l2 = node_factory.get_node(options=opts) # The 'accepter_close_to' plugin uses the channel funding amount to determine # whether or not to include a 'close_to' address - amt_normal = 100007 # continues without returning a close_to - amt_addr = 100001 # returns valid regtest address + amt_normal = 100000 # continues without returning a close_to + amt_addr = 100003 # returns valid regtest address remote_valid_addr = 'bcrt1q7gtnxmlaly9vklvmfj06amfdef3rtnrdazdsvw' diff --git a/tests/test_plugin.py b/tests/test_plugin.py index b2772d5da29f..518f70311ba3 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -538,13 +538,7 @@ def test_openchannel_hook(node_factory, bitcoind): """ opts = [{}, {'plugin': os.path.join(os.getcwd(), 'tests/plugins/reject_odd_funding_amounts.py')}] l1, l2 = node_factory.line_graph(2, fundchannel=False, opts=opts) - - # Get some funds. - addr = l1.rpc.newaddr()['bech32'] - txid = bitcoind.rpc.sendtoaddress(addr, 10) - numfunds = len(l1.rpc.listfunds()['outputs']) - bitcoind.generate_block(1, txid) - wait_for(lambda: len(l1.rpc.listfunds()['outputs']) > numfunds) + l1.fundwallet(10**6) # Even amount: works. l1.rpc.fundchannel(l2.info['id'], 100000) @@ -574,6 +568,70 @@ def test_openchannel_hook(node_factory, bitcoind): l1.rpc.fundchannel(l2.info['id'], 100001) +def test_openchannel_hook_error_handling(node_factory, bitcoind): + """ l2 uses a plugin that should fatal() crash the node. + + This is because the plugin rejects a channel while + also setting a close_to address which isn't allowed. + """ + opts = {'plugin': os.path.join(os.getcwd(), 'tests/plugins/openchannel_hook_accepter.py')} + # openchannel_reject_but_set_close_to.py')} + l1 = node_factory.get_node() + l2 = node_factory.get_node(options=opts, + expect_fail=True, + may_fail=True, + allow_broken_log=True) + l1.connect(l2) + l1.fundwallet(10**6) + + # next fundchannel should fail fatal() for l2 + with pytest.raises(RpcError, match=r'Owning subdaemon openingd died'): + l1.rpc.fundchannel(l2.info['id'], 100004) + assert l2.daemon.is_in_log("Plugin rejected openchannel but also set close_to") + + +def test_openchannel_hook_chaining(node_factory, bitcoind): + """ l2 uses a set of plugin that all use the openchannel_hook. + + We test that chaining works by using multiple plugins in a way + that we check for the first plugin that rejects prevents from evaluating + further plugin responses down the chain. + + """ + opts = [{}, {'plugin': [ + os.path.join(os.path.dirname(__file__), '..', 'tests/plugins/openchannel_hook_accept.py'), + os.path.join(os.path.dirname(__file__), '..', 'tests/plugins/openchannel_hook_accepter.py'), + os.path.join(os.path.dirname(__file__), '..', 'tests/plugins/openchannel_hook_reject.py') + ]}] + l1, l2 = node_factory.line_graph(2, fundchannel=False, opts=opts) + l1.fundwallet(10**6) + + hook_msg = "openchannel_hook rejects and says '" + # 100005sat fundchannel should fail fatal() for l2 + # because hook_accepter.py rejects on that amount 'for a reason' + with pytest.raises(RpcError, match=r'They sent error channel'): + l1.rpc.fundchannel(l2.info['id'], 100005) + + # Note: hook chain order is currently undefined, because hooks are registerd + # as a result of the getmanifest call, which may take some random time. + # We need to workaround that fact, so test can be stable + correct_order = l2.daemon.is_in_log(hook_msg + "reject for a reason") + if correct_order: + assert l2.daemon.wait_for_log(hook_msg + "reject for a reason") + # the other plugin must not be called + assert not l2.daemon.is_in_log("reject on principle") + else: + assert l2.daemon.wait_for_log(hook_msg + "reject on principle") + # the other plugin must not be called + assert not l2.daemon.is_in_log("reject for a reason") + + # 100000sat is good for hook_accepter, so it should fail 'on principle' + # at third hook openchannel_reject.py + with pytest.raises(RpcError, match=r'They sent error channel'): + l1.rpc.fundchannel(l2.info['id'], 100000) + assert l2.daemon.wait_for_log(hook_msg + "reject on principle") + + @unittest.skipIf(not DEVELOPER, "without DEVELOPER=1, gossip v slow") def test_htlc_accepted_hook_fail(node_factory): """Send payments from l1 to l2, but l2 just declines everything.