From 6c9d9ee9a296ab16d069323e569477eab78fadf6 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 16 Mar 2021 14:14:36 +1030 Subject: [PATCH] connect: return address we actually connected to. Otherwise, we might find an address other than the one given and the user might think that address worked. Fixes: #4185 Signed-off-by: Rusty Russell Changelog-Added: JSON-RPC: `connect` returns `address` it actually connected to --- doc/lightning-connect.7 | 6 +++--- doc/lightning-connect.7.md | 4 ++-- lightningd/connect_control.c | 13 +++++++++---- lightningd/connect_control.h | 3 ++- lightningd/peer_control.c | 2 +- lightningd/test/run-invoice-select-inchan.c | 3 ++- tests/test_connection.py | 7 ++++++- tests/test_gossip.py | 9 ++++++++- tests/test_misc.py | 3 ++- wallet/db_postgres_sqlgen.c | 2 +- wallet/db_sqlite3_sqlgen.c | 2 +- wallet/statements_gettextgen.po | 6 +++--- wallet/test/run-wallet.c | 3 ++- 13 files changed, 42 insertions(+), 21 deletions(-) diff --git a/doc/lightning-connect.7 b/doc/lightning-connect.7 index 23d9eed9c..9cc0ae3b0 100644 --- a/doc/lightning-connect.7 +++ b/doc/lightning-connect.7 @@ -22,7 +22,7 @@ be of the form \fIid@host\fR or \fIid@host:port\fR\. In this case, the \fIhost\f If not specified, the \fIport\fR defaults to 9735\. -If \fIhost\fR is not specified, the connection will be attempted to an IP +If \fIhost\fR is not specified (or doesn't work), the connection will be attempted to an IP belonging to \fIid\fR obtained through gossip with other already connected peers\. This can fail if your C-lightning node is a fresh install that has not @@ -43,7 +43,7 @@ another node\. Once the peer is connected a channel can be opened with .SH RETURN VALUE On success the peer \fIid\fR is returned, as well as a hexidecimal \fIfeatures\fR -bitmap\. +bitmap and an \fIaddress\fR object as per \fBlightning-listnodes\fR(7)\. .SH ERRORS @@ -94,4 +94,4 @@ Felix \fI is the original author of this manpage\. Main web site: \fIhttps://github.com/ElementsProject/lightning\fR -\" SHA256STAMP:5f99f87ec4d94984b36da81b3f3b6fd21100b58baea493f5d0c3f50ffdcbf301 +\" SHA256STAMP:336c35e791dc6439115e25a2f58ffb9dc68989c96d51b6c39cb3de6d40328ae5 diff --git a/doc/lightning-connect.7.md b/doc/lightning-connect.7.md index 1f9071bda..89476195c 100644 --- a/doc/lightning-connect.7.md +++ b/doc/lightning-connect.7.md @@ -20,7 +20,7 @@ be of the form *id@host* or *id@host:port*. In this case, the *host* and If not specified, the *port* defaults to 9735. -If *host* is not specified, the connection will be attempted to an IP +If *host* is not specified (or doesn't work), the connection will be attempted to an IP belonging to *id* obtained through gossip with other already connected peers. This can fail if your C-lightning node is a fresh install that has not @@ -40,7 +40,7 @@ RETURN VALUE ------------ On success the peer *id* is returned, as well as a hexidecimal *features* -bitmap. +bitmap and an *address* object as per lightning-listnodes(7). ERRORS ------ diff --git a/lightningd/connect_control.c b/lightningd/connect_control.c index 98f6878f6..fd6a5cf6d 100644 --- a/lightningd/connect_control.c +++ b/lightningd/connect_control.c @@ -68,11 +68,13 @@ static struct connect *find_connect(struct lightningd *ld, } static struct command_result *connect_cmd_succeed(struct command *cmd, - const struct peer *peer) + const struct peer *peer, + const struct wireaddr_internal *addr) { struct json_stream *response = json_stream_success(cmd); json_add_node_id(response, "id", &peer->id); json_add_hex_talarr(response, "features", peer->their_features); + json_add_address_internal(response, "address", addr); return command_success(cmd, response); } @@ -143,7 +145,9 @@ static struct command_result *json_connect(struct command *cmd, if (peer->uncommitted_channel || (channel && channel->connected)) { - return connect_cmd_succeed(cmd, peer); + log_debug(cmd->ld->log, "Already connected via %s", + type_to_string(tmpctx, struct wireaddr_internal, &peer->addr)); + return connect_cmd_succeed(cmd, peer, &peer->addr); } } @@ -260,14 +264,15 @@ static void connect_failed(struct lightningd *ld, const u8 *msg) delay_then_reconnect(channel, seconds_to_delay, addrhint); } -void connect_succeeded(struct lightningd *ld, const struct peer *peer) +void connect_succeeded(struct lightningd *ld, const struct peer *peer, + const struct wireaddr_internal *addr) { struct connect *c; /* We can have multiple connect commands: fail them all */ while ((c = find_connect(ld, &peer->id)) != NULL) { /* They delete themselves from list */ - connect_cmd_succeed(c->cmd, peer); + connect_cmd_succeed(c->cmd, peer, addr); } } diff --git a/lightningd/connect_control.h b/lightningd/connect_control.h index 64c5adaa8..0248eb4d9 100644 --- a/lightningd/connect_control.h +++ b/lightningd/connect_control.h @@ -12,7 +12,8 @@ void connectd_activate(struct lightningd *ld); void delay_then_reconnect(struct channel *channel, u32 seconds_delay, const struct wireaddr_internal *addrhint TAKES); -void connect_succeeded(struct lightningd *ld, const struct peer *peer); +void connect_succeeded(struct lightningd *ld, const struct peer *peer, + const struct wireaddr_internal *addr); void gossip_connect_result(struct lightningd *ld, const u8 *msg); #endif /* LIGHTNING_LIGHTNINGD_CONNECT_CONTROL_H */ diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index e0c79e0e4..14159b68b 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -1194,7 +1194,7 @@ void peer_connected(struct lightningd *ld, const u8 *msg, peer_update_features(peer, their_features); /* Complete any outstanding connect commands. */ - connect_succeeded(ld, peer); + connect_succeeded(ld, peer, &hook_payload->addr); /* Can't be opening, since we wouldn't have sent peer_disconnected. */ assert(!peer->uncommitted_channel); diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index c3ab8f5f9..07b296929 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -142,7 +142,8 @@ struct command_result *command_success(struct command *cmd UNNEEDED, { fprintf(stderr, "command_success called!\n"); abort(); } /* Generated stub for connect_succeeded */ -void connect_succeeded(struct lightningd *ld UNNEEDED, const struct peer *peer UNNEEDED) +void connect_succeeded(struct lightningd *ld UNNEEDED, const struct peer *peer UNNEEDED, + const struct wireaddr_internal *addr UNNEEDED) { fprintf(stderr, "connect_succeeded called!\n"); abort(); } /* Generated stub for delay_then_reconnect */ void delay_then_reconnect(struct channel *channel UNNEEDED, u32 seconds_delay UNNEEDED, diff --git a/tests/test_connection.py b/tests/test_connection.py index 045b0bf77..b7323c032 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -33,9 +33,12 @@ def test_connect(node_factory): # Reconnect should be a noop ret = l1.rpc.connect(l2.info['id'], 'localhost', port=l2.port) assert ret['id'] == l2.info['id'] + assert ret['address'] == {'type': 'ipv4', 'address': '127.0.0.1', 'port': l2.port} ret = l2.rpc.connect(l1.info['id'], host='localhost', port=l1.port) assert ret['id'] == l1.info['id'] + # FIXME: This gives a bogus address (since they connected to us): better to give none! + assert 'address' in ret # Should still only have one peer! assert len(l1.rpc.listpeers()) == 1 @@ -62,6 +65,7 @@ def test_connect_standard_addr(node_factory): # node@host ret = l1.rpc.connect("{}@{}".format(l2.info['id'], 'localhost'), port=l2.port) assert ret['id'] == l2.info['id'] + assert ret['address'] == {'type': 'ipv4', 'address': '127.0.0.1', 'port': l2.port} # node@host:port ret = l1.rpc.connect("{}@localhost:{}".format(l3.info['id'], l3.port)) @@ -127,7 +131,8 @@ def test_connection_moved(node_factory, executor): l1.daemon.wait_for_log('connection from') # Provide correct connection details - l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + ret = l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + assert ret['address'] == {'type': 'ipv4', 'address': '127.0.0.1', 'port': l2.port} # If we failed to update the connection, this call will error fut_hang.result(TIMEOUT) diff --git a/tests/test_gossip.py b/tests/test_gossip.py index a7165d457..fd20ccaf1 100644 --- a/tests/test_gossip.py +++ b/tests/test_gossip.py @@ -243,7 +243,14 @@ def test_connect_by_gossip(node_factory, bitcoind): l1.daemon.wait_for_logs(['Received node_announcement for node {}'.format(l3.info['id'])]) # Have l1 connect to l3 without explicit host and port. - l1.rpc.connect(l3.info['id']) + ret = l1.rpc.connect(l3.info['id']) + assert ret['address'] == {'type': 'ipv4', 'address': '127.0.0.1', 'port': l3.port} + + # Now give it *wrong* port (after we make sure l2 isn't listening), it should fall back. + l1.rpc.disconnect(l3.info['id']) + l2.stop() + ret = l1.rpc.connect(l3.info['id'], 'localhost', l2.port) + assert ret['address'] == {'type': 'ipv4', 'address': '127.0.0.1', 'port': l3.port} @unittest.skipIf(not DEVELOPER, "DEVELOPER=1 needed to speed up gossip propagation, would be too long otherwise") diff --git a/tests/test_misc.py b/tests/test_misc.py index dba3c0a30..a6c4fb358 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -784,7 +784,8 @@ def test_address(node_factory): l1.start() l2 = node_factory.get_node() - l2.rpc.connect(l1.info['id'], l1.daemon.opts['bind-addr']) + ret = l2.rpc.connect(l1.info['id'], l1.daemon.opts['bind-addr']) + assert ret['address'] == {'type': 'local socket', 'socket': l1.daemon.opts['bind-addr']} # 'addr' with local socket works too. l1.stop() diff --git a/wallet/db_postgres_sqlgen.c b/wallet/db_postgres_sqlgen.c index 35845f36f..7a330511f 100644 --- a/wallet/db_postgres_sqlgen.c +++ b/wallet/db_postgres_sqlgen.c @@ -1888,4 +1888,4 @@ struct db_query db_postgres_queries[] = { #endif /* LIGHTNINGD_WALLET_GEN_DB_POSTGRES */ -// SHA256STAMP:fa582dba4c41760ea1760e8c98a53ca4a450ab8236d68bc8749eda0efb8c59af +// SHA256STAMP:8b9d33d20380d74e6f143f65bb05ab95a05403cba5b5d6afe19edc91c0a185cb diff --git a/wallet/db_sqlite3_sqlgen.c b/wallet/db_sqlite3_sqlgen.c index d486f6316..befd4b1a7 100644 --- a/wallet/db_sqlite3_sqlgen.c +++ b/wallet/db_sqlite3_sqlgen.c @@ -1888,4 +1888,4 @@ struct db_query db_sqlite3_queries[] = { #endif /* LIGHTNINGD_WALLET_GEN_DB_SQLITE3 */ -// SHA256STAMP:fa582dba4c41760ea1760e8c98a53ca4a450ab8236d68bc8749eda0efb8c59af +// SHA256STAMP:8b9d33d20380d74e6f143f65bb05ab95a05403cba5b5d6afe19edc91c0a185cb diff --git a/wallet/statements_gettextgen.po b/wallet/statements_gettextgen.po index 5fa433368..4fbd889fd 100644 --- a/wallet/statements_gettextgen.po +++ b/wallet/statements_gettextgen.po @@ -1238,11 +1238,11 @@ msgstr "" msgid "not a valid SQL statement" msgstr "" -#: wallet/test/run-wallet.c:1441 +#: wallet/test/run-wallet.c:1442 msgid "SELECT COUNT(1) FROM channel_funding_inflights WHERE channel_id = ?;" msgstr "" -#: wallet/test/run-wallet.c:1639 +#: wallet/test/run-wallet.c:1640 msgid "INSERT INTO channels (id) VALUES (1);" msgstr "" -# SHA256STAMP:249962d1ad354071c65dcbdbabf051fde833e7c9b36f78b2db093d3209e419d5 +# SHA256STAMP:708d261517851b29a6f7a8102b1d7388b0aef6fb95714cb17efab3079e44d700 diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 960d43c95..12996e014 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -89,7 +89,8 @@ struct command_result *command_success(struct command *cmd UNNEEDED, { fprintf(stderr, "command_success called!\n"); abort(); } /* Generated stub for connect_succeeded */ -void connect_succeeded(struct lightningd *ld UNNEEDED, const struct peer *peer UNNEEDED) +void connect_succeeded(struct lightningd *ld UNNEEDED, const struct peer *peer UNNEEDED, + const struct wireaddr_internal *addr UNNEEDED) { fprintf(stderr, "connect_succeeded called!\n"); abort(); } /* Generated stub for create_onionreply */ struct onionreply *create_onionreply(const tal_t *ctx UNNEEDED,