From 064ad486e3fdd4718e9d3303dc51b97c31035c66 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Jun 2021 13:21:12 +0930 Subject: [PATCH] close: check that destination is going to be accepted. Prior to this, sending a v1 address (or, in fact, any random crap!) would cause the unsupporting node to unilaterally close. Signed-off-by: Rusty Russell --- doc/lightning-close.7 | 7 ++++-- doc/lightning-close.7.md | 5 ++++- lightningd/channel_control.c | 2 +- lightningd/peer_control.c | 20 +++++++++++++++++ lightningd/test/run-invoice-select-inchan.c | 4 ++++ tests/test_closing.py | 24 +++++++++++++++++++++ wallet/db_postgres_sqlgen.c | 2 +- wallet/db_sqlite3_sqlgen.c | 2 +- wallet/statements_gettextgen.po | 6 +++--- wallet/test/run-wallet.c | 4 ++++ 10 files changed, 67 insertions(+), 9 deletions(-) diff --git a/doc/lightning-close.7 b/doc/lightning-close.7 index c6313e417..5255b73a8 100644 --- a/doc/lightning-close.7 +++ b/doc/lightning-close.7 @@ -27,7 +27,10 @@ The default is 2 days (172800 seconds)\. The \fIdestination\fR can be of any Bitcoin accepted type, including bech32\. -If it isn't specified, the default is a c-lightning wallet address\. +If it isn't specified, the default is a c-lightning wallet address\. If +the peer hasn't offered the \fBoption_shutdown_anysegwit\fR feature, then +taproot addresses (or other v1+ segwit) are not allowed\. Tell your +friends to upgrade! The \fIfee_negotiation_step\fR parameter controls how closing fee @@ -120,4 +123,4 @@ ZmnSCPxj \fI is mainly responsible\. Main web site: \fIhttps://github.com/ElementsProject/lightning\fR -\" SHA256STAMP:507a9ca707e244eef65c5e16daa5a4d7ba8f59e93e988d252f7e854ae9f44781 +\" SHA256STAMP:17f5bb362d8501b04314756c4134e3d5d20f8729dd55f5f3cfa0b5e111b104a1 diff --git a/doc/lightning-close.7.md b/doc/lightning-close.7.md index 306598ca7..067ec8df1 100644 --- a/doc/lightning-close.7.md +++ b/doc/lightning-close.7.md @@ -26,7 +26,10 @@ indefinitely until the peer is online and can negotiate a mutual close. The default is 2 days (172800 seconds). The *destination* can be of any Bitcoin accepted type, including bech32. -If it isn't specified, the default is a c-lightning wallet address. +If it isn't specified, the default is a c-lightning wallet address. If +the peer hasn't offered the `option_shutdown_anysegwit` feature, then +taproot addresses (or other v1+ segwit) are not allowed. Tell your +friends to upgrade! The *fee_negotiation_step* parameter controls how closing fee negotiation is performed assuming the peer proposes a fee that is diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index e8c87401a..a322fe57e 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -244,7 +244,7 @@ static void peer_got_shutdown(struct channel *channel, const u8 *msg) channel_fail_permanent(channel, REASON_PROTOCOL, "Bad shutdown scriptpubkey %s", - tal_hex(channel, scriptpubkey)); + tal_hex(tmpctx, scriptpubkey)); return; } diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 9d74abd01..47957c1ab 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -1655,6 +1656,7 @@ static struct command_result *json_close(struct command *cmd, const char *fee_negotiation_step_str; struct bitcoin_outpoint *wrong_funding; char* end; + bool anysegwit; if (!param(cmd, buffer, params, p_req("id", param_tok, &idtok), @@ -1731,6 +1733,24 @@ static struct command_result *json_close(struct command *cmd, } else close_script_set = false; + /* Don't send a scriptpubkey peer won't accept */ + anysegwit = feature_negotiated(cmd->ld->our_features, + channel->peer->their_features, + OPT_SHUTDOWN_ANYSEGWIT); + if (!valid_shutdown_scriptpubkey(channel->shutdown_scriptpubkey[LOCAL], + anysegwit)) { + /* Explicit check for future segwits. */ + if (!anysegwit && + valid_shutdown_scriptpubkey(channel->shutdown_scriptpubkey + [LOCAL], true)) { + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Peer does not allow v1+ shutdown addresses"); + } + + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Invalid close destination"); + } + if (fee_negotiation_step_str == NULL) { channel->closing_fee_negotiation_step = 50; channel->closing_fee_negotiation_step_unit = diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index 8709fb588..8a4fd854e 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -670,6 +670,10 @@ u8 *towire_warningfmt(const tal_t *ctx UNNEEDED, const struct channel_id *channel UNNEEDED, const char *fmt UNNEEDED, ...) { fprintf(stderr, "towire_warningfmt called!\n"); abort(); } +/* Generated stub for valid_shutdown_scriptpubkey */ +bool valid_shutdown_scriptpubkey(const u8 *scriptpubkey UNNEEDED, + bool anysegwit UNNEEDED) +{ fprintf(stderr, "valid_shutdown_scriptpubkey called!\n"); abort(); } /* Generated stub for version */ const char *version(void) { fprintf(stderr, "version called!\n"); abort(); } diff --git a/tests/test_closing.py b/tests/test_closing.py index eae705d94..55d05d447 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -2772,3 +2772,27 @@ def test_segwit_anyshutdown(node_factory, bitcoind, executor): l1.rpc.close(l2.info['id'], destination=addr) bitcoind.generate_block(1, wait_for_mempool=1) wait_for(lambda: all([c['state'] == 'ONCHAIN' for c in only_one(l1.rpc.listpeers()['peers'])['channels']])) + + +@pytest.mark.developer("needs to manipulate features") +@unittest.skipIf(TEST_NETWORK == 'liquid-regtest', "Uses regtest addresses") +def test_anysegwit_close_needs_feature(node_factory, bitcoind): + """Rather than have peer reject our shutdown, we should refuse to shutdown toa v1+ address if they don't support it""" + # L2 says "no option_shutdown_anysegwit" + l1, l2 = node_factory.line_graph(2, opts=[{'may_reconnect': True}, + {'may_reconnect': True, + 'dev-force-features': -27}]) + + with pytest.raises(RpcError, match=r'Peer does not allow v1\+ shutdown addresses'): + l1.rpc.close(l2.info['id'], destination='bcrt1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7k0ylj56') + + # From TFM: "Tell your friends to upgrade!" + l2.stop() + del l2.daemon.opts['dev-force-features'] + l2.start() + + # Now it will work! + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l1.rpc.close(l2.info['id'], destination='bcrt1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7k0ylj56') + wait_for(lambda: only_one(only_one(l1.rpc.listpeers()['peers'])['channels'])['state'] == 'CLOSINGD_COMPLETE') + bitcoind.generate_block(1, wait_for_mempool=1) diff --git a/wallet/db_postgres_sqlgen.c b/wallet/db_postgres_sqlgen.c index 8a3b70c57..e85d5929d 100644 --- a/wallet/db_postgres_sqlgen.c +++ b/wallet/db_postgres_sqlgen.c @@ -1924,4 +1924,4 @@ struct db_query db_postgres_queries[] = { #endif /* LIGHTNINGD_WALLET_GEN_DB_POSTGRES */ -// SHA256STAMP:1379bcdee314439910fc6b238f7ec986536543c53933883ffd1b750dfc34f9b9 +// SHA256STAMP:dbbcb7d784e7b3d6c7b27c2ff976dcc39335fdc26fbf095b65116488007799f7 diff --git a/wallet/db_sqlite3_sqlgen.c b/wallet/db_sqlite3_sqlgen.c index 98ed0bf0c..5b6eb8fa0 100644 --- a/wallet/db_sqlite3_sqlgen.c +++ b/wallet/db_sqlite3_sqlgen.c @@ -1924,4 +1924,4 @@ struct db_query db_sqlite3_queries[] = { #endif /* LIGHTNINGD_WALLET_GEN_DB_SQLITE3 */ -// SHA256STAMP:1379bcdee314439910fc6b238f7ec986536543c53933883ffd1b750dfc34f9b9 +// SHA256STAMP:dbbcb7d784e7b3d6c7b27c2ff976dcc39335fdc26fbf095b65116488007799f7 diff --git a/wallet/statements_gettextgen.po b/wallet/statements_gettextgen.po index 27896c8b6..030daf206 100644 --- a/wallet/statements_gettextgen.po +++ b/wallet/statements_gettextgen.po @@ -1262,11 +1262,11 @@ msgstr "" msgid "not a valid SQL statement" msgstr "" -#: wallet/test/run-wallet.c:1451 +#: wallet/test/run-wallet.c:1455 msgid "SELECT COUNT(1) FROM channel_funding_inflights WHERE channel_id = ?;" msgstr "" -#: wallet/test/run-wallet.c:1649 +#: wallet/test/run-wallet.c:1653 msgid "INSERT INTO channels (id) VALUES (1);" msgstr "" -# SHA256STAMP:51de0bd1efd4b12ec550d3faf934a32f745018a46e61b50cc242c2d5bae09470 +# SHA256STAMP:e3c8d5cac8615668f0c9f37ebf6edff3b18833bafdf9643c2203b2a4ab654b7c diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index ede16ddd6..c95aca7cb 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -841,6 +841,10 @@ u8 *towire_warningfmt(const tal_t *ctx UNNEEDED, const struct channel_id *channel UNNEEDED, const char *fmt UNNEEDED, ...) { fprintf(stderr, "towire_warningfmt called!\n"); abort(); } +/* Generated stub for valid_shutdown_scriptpubkey */ +bool valid_shutdown_scriptpubkey(const u8 *scriptpubkey UNNEEDED, + bool anysegwit UNNEEDED) +{ fprintf(stderr, "valid_shutdown_scriptpubkey called!\n"); abort(); } /* Generated stub for watch_txid */ struct txwatch *watch_txid(const tal_t *ctx UNNEEDED, struct chain_topology *topo UNNEEDED,