diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index ed00651814bc..0f9da5df6524 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -111,6 +111,7 @@ struct peer *new_peer(struct lightningd *ld, u64 dbid, else peer->their_features = NULL; + peer->unknown_channel_reestablishes = 0; peer->dev_ignore_htlcs = false; peer_node_id_map_add(ld->peers, peer); @@ -1898,6 +1899,9 @@ void handle_peer_connected(struct lightningd *ld, const u8 *msg) * on peer commands, and it knows to ignore if it's wrong. */ peer->connectd_counter = connectd_counter; + /* Fresh connection, fresh spam allowance. */ + peer->unknown_channel_reestablishes = 0; + /* We mark peer in "connecting" state until hooks have passed. */ assert(peer->connected == PEER_DISCONNECTED); peer->connected = PEER_CONNECTING; @@ -1987,6 +1991,11 @@ static void send_reestablish(struct peer *peer, msg))); } +/* How many channel_reestablish for channels we don't know we answer without + * hanging up. An honest peer needs one per stale channel; past that it's + * cheaper for us to make them reconnect (which connectd rate-limits). */ +#define MAX_UNKNOWN_CHANNEL_REESTABLISHES 10 + /* connectd tells us a peer has a message and we've not already attached * a subd. Normally this is a race, but it happens for real when opening * a new channel, or referring to a channel we no longer want to talk to @@ -2005,6 +2014,9 @@ void handle_peer_spoke(struct lightningd *ld, const u8 *msg) int other_fd; struct peer_fd *pfd; char *errmsg; + /* Every error we send here hangs up, except the unknown-channel + * reestablish case below. */ + bool hangup = true; if (!fromwire_connectd_peer_spoke(msg, msg, &id, &connectd_counter, &msgtype, &channel_id, &errmsg)) fatal("Connectd gave bad CONNECTD_PEER_SPOKE message %s", @@ -2145,9 +2157,30 @@ void handle_peer_spoke(struct lightningd *ld, const u8 *msg) "Channel is closed and forgotten"); goto send_error; } + /* BOLT #1: + * + * A sending node: + *... + * - when sending `error`: + * - MUST fail the channel(s) referred to by the error message. + */ + /* We don't know the channel, so there's nothing for us to + * fail, and nothing tells us to drop the connection: `error` + * exists so that we don't have to. Staying up matters here, + * because the peer needs this error to forget its own (saved, + * commitment-ready) channel: hanging up races with the error + * delivery, and then it retries on every reconnect (#8822). + * + * A hostile peer could use that to stream reestablishes for + * random channel_ids down a single connection, so we only + * tolerate a few before hanging up like we used to. */ + if (++peer->unknown_channel_reestablishes + <= MAX_UNKNOWN_CHANNEL_REESTABLISHES) + hangup = false; + break; } - /* Weird message? Log and reply with error. */ + /* Unknown channel, or a weird message? Log and reply with error. */ log_peer_unusual(ld->log, &peer->id, "Unknown channel %s for %s", fmt_channel_id(tmpctx, @@ -2159,15 +2192,16 @@ void handle_peer_spoke(struct lightningd *ld, const u8 *msg) send_error: log_peer_debug(ld->log, &peer->id, "Telling connectd to send error %s", tal_hex(tmpctx, error)); - /* Get connectd to send error and close. */ + /* Get connectd to send error, and (usually) close. */ subd_send_msg(ld->connectd, take(towire_connectd_peer_send_msg(NULL, &peer->id, peer->connectd_counter, error))); - subd_send_msg(ld->connectd, - take(towire_connectd_disconnect_peer(NULL, - &peer->id, - peer->connectd_counter))); + if (hangup) + subd_send_msg(ld->connectd, + take(towire_connectd_disconnect_peer(NULL, + &peer->id, + peer->connectd_counter))); return; tell_connectd: diff --git a/lightningd/peer_control.h b/lightningd/peer_control.h index 279a2f91d679..b430ab76b869 100644 --- a/lightningd/peer_control.h +++ b/lightningd/peer_control.h @@ -65,6 +65,11 @@ struct peer { /* If we open a channel our direction will be this */ u8 direction; + /* How many channel_reestablish for channels we don't know have we seen + * on this connection? We answer those with an error and stay + * connected, so we bound it to keep it from being a spam lever. */ + u32 unknown_channel_reestablishes; + /* Swallow incoming HTLCs (for testing) */ bool dev_ignore_htlcs; }; diff --git a/tests/test_misc.py b/tests/test_misc.py index e229a3850c43..ebd28e84d811 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -3277,17 +3277,23 @@ def test_restorefrompeer(node_factory, bitcoind): l1.start() assert l1.daemon.is_in_log('Server started with public key') - # If this happens fast enough, connect fails with "disconnected - # during connection" - try: - l1.rpc.connect(l2.info['id'], 'localhost', l2.port) - except RpcError as err: - assert "disconnected during connection" in err.error['message'] + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) l1.daemon.wait_for_log('peer_in WIRE_PEER_STORAGE_RETRIEVAL') + # We lost our db, so l2's channel_reestablish is for a channel we don't + # know: we answer with an error, but we don't hang up on them any more. + l1.daemon.wait_for_log('Unknown channel .* for WIRE_CHANNEL_REESTABLISH') + assert only_one(l1.rpc.listpeers()['peers'])['connected'] + assert l1.rpc.restorefrompeer()['stubs'][0] == _['channel_id'] + # We need to reconnect so the stub channel triggers the bogus + # channel_reestablish flow in peer_connected_hook_final. + # (l1 no longer disconnects on unknown channel_reestablish.) + l1.rpc.disconnect(l2.info['id'], force=True) + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l1.daemon.wait_for_log('Sending a bogus channel_reestablish message to make the peer unilaterally close the channel.') l1.daemon.wait_for_log('peer_out WIRE_ERROR') diff --git a/tests/test_opening.py b/tests/test_opening.py index 4954c31b87b4..27862ef5da46 100644 --- a/tests/test_opening.py +++ b/tests/test_opening.py @@ -216,6 +216,55 @@ def test_v2_open_sigs_reconnect_1(node_factory, bitcoind): l2.daemon.wait_for_log(r'to CHANNELD_NORMAL') +@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') +@pytest.mark.openchannel('v2') +def test_v2_open_reestablish_unknown_channel(node_factory, bitcoind): + """ Reconnect loop from #8822. + + l1 drops the last tx_complete on the floor *after* dualopend has decided + the commitment is ready, so l1 saves the channel in + DUALOPEND_OPEN_COMMIT_READY while l2 is still waiting and throws its + unsaved channel away. On reconnect l1 reestablishes a channel l2 has + never heard of. + + l2 answers with an error and stays connected: that's what lets l1's + dualopend actually read the error and forget the channel. When we hung + up instead, the error raced the disconnect on l1's side, and whenever it + lost that race l1 reestablished again on every reconnect. + """ + l1, l2 = node_factory.get_nodes(2, + opts=[{'disconnect': ['-WIRE_TX_COMPLETE'], + 'may_reconnect': True, + 'dev-no-reconnect': None}, + {'may_reconnect': True, + 'dev-no-reconnect': None}]) + + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + bitcoind.rpc.sendtoaddress(l1.rpc.newaddr()['p2tr'], (2**24) / 10**8 + 0.01) + bitcoind.generate_block(1) + wait_for(lambda: len(l1.rpc.listfunds()['outputs']) > 0) + + with pytest.raises(RpcError): + l1.rpc.fundchannel(l2.info['id'], 100000) + + # We saved it, they didn't. + wait_for(lambda: [c['state'] for c in l1.rpc.listpeerchannels()['channels']] + == ['DUALOPEND_OPEN_COMMIT_READY']) + wait_for(lambda: l2.rpc.listpeerchannels()['channels'] == []) + + # One reconnect has to be enough: l2 tells us it doesn't know the channel + # and we forget it. + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l2.daemon.wait_for_log('Unknown channel .* for WIRE_CHANNEL_REESTABLISH') + l1.daemon.wait_for_log('peer_in WIRE_ERROR') + wait_for(lambda: l1.rpc.listpeerchannels()['channels'] == []) + + # Neither side hung up over it: that's the whole point, an error we send + # after hanging up may never be read. + assert only_one(l1.rpc.listpeers()['peers'])['connected'] + assert only_one(l2.rpc.listpeers()['peers'])['connected'] + + @unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') @pytest.mark.openchannel('v2') def test_v2_open_sigs_out_of_order(node_factory, bitcoind):