diff --git a/CHANGELOG.md b/CHANGELOG.md index b8d6b99d4..5e97fad78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Changed +- Protocol: `channel_update` sent to disable channel only if we reject an HTLC. + ### Deprecated Note: You should always set `allow-deprecated-apis=false` to test for diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index 6b842ebac..56aa9e314 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -61,25 +61,6 @@ static u32 max_scids_encode_bytes = -1U; static bool suppress_gossip = false; #endif -struct local_update { - /* daemon->local_updates */ - struct list_node list; - - /* Because we're handed to a one-arg timer */ - struct daemon *daemon; - - /* Which channel this is */ - struct short_channel_id scid; - - /* Which direction we own */ - int direction; - - u16 cltv_delta; - u64 htlc_minimum_msat; - u32 fee_base_msat, fee_proportional_millionths; - bool disable; -}; - struct daemon { /* Who am I? */ struct pubkey id; @@ -111,9 +92,6 @@ struct daemon { /* To make sure our node_announcement timestamps increase */ u32 last_announce_timestamp; - - /* Unapplied local updates waiting for their timers. */ - struct list_head local_updates; }; struct peer { @@ -159,25 +137,22 @@ struct peer { struct daemon_conn *remote; }; -/* FIXME: Reorder */ -static void peer_disable_channels(struct daemon *daemon, struct node *node); -static u8 *create_channel_update(const tal_t *ctx, - struct routing_state *rstate, - const struct chan *chan, - int direction, - bool disable, - u16 cltv_expiry_delta, - u64 htlc_minimum_msat, - u32 fee_base_msat, - u32 fee_proportional_millionths); -static struct local_update *find_local_update(struct daemon *daemon, - const struct short_channel_id *scid); +static void peer_disable_channels(struct daemon *daemon, struct node *node) +{ + for (size_t i = 0; i < tal_count(node->chans); i++) { + struct chan *c = node->chans[i]; + if (pubkey_eq(&other_node(node, c)->id, &daemon->id)) + c->local_disabled = true; + } +} static void destroy_peer(struct peer *peer) { struct node *node; list_del_from(&peer->daemon->peers, &peer->list); + + /* If we have a channel with this peer, disable it. */ node = get_node(peer->daemon->rstate, &peer->id); if (node) peer_disable_channels(peer->daemon, node); @@ -930,80 +905,15 @@ static bool maybe_queue_gossip(struct peer *peer) return false; } -static void handle_get_update(struct peer *peer, const u8 *msg) -{ - struct short_channel_id scid; - struct chan *chan; - const u8 *update; - struct routing_state *rstate = peer->daemon->rstate; - - if (!fromwire_gossip_get_update(msg, &scid)) { - status_trace("peer %s sent bad gossip_get_update %s", - type_to_string(tmpctx, struct pubkey, &peer->id), - tal_hex(tmpctx, msg)); - return; - } - - chan = get_channel(rstate, &scid); - if (!chan) { - status_unusual("peer %s scid %s: unknown channel", - type_to_string(tmpctx, struct pubkey, &peer->id), - type_to_string(tmpctx, struct short_channel_id, - &scid)); - update = NULL; - } else { - struct local_update *l; - - /* We want the update that comes from our end. */ - if (pubkey_eq(&chan->nodes[0]->id, &peer->daemon->id)) - update = chan->half[0].channel_update; - else if (pubkey_eq(&chan->nodes[1]->id, &peer->daemon->id)) - update = chan->half[1].channel_update; - else { - status_unusual("peer %s scid %s: not our channel?", - type_to_string(tmpctx, struct pubkey, - &peer->id), - type_to_string(tmpctx, - struct short_channel_id, - &scid)); - update = NULL; - goto out; - } - - /* We might have a pending update which overrides: always send - * that now, since this is used to populate errors which should - * contain the latest information. */ - l = find_local_update(peer->daemon, &scid); - if (l) - update = create_channel_update(tmpctx, - rstate, - chan, l->direction, - l->disable, - l->cltv_delta, - l->htlc_minimum_msat, - l->fee_base_msat, - l->fee_proportional_millionths); - } - -out: - status_trace("peer %s schanid %s: %s update", - type_to_string(tmpctx, struct pubkey, &peer->id), - type_to_string(tmpctx, struct short_channel_id, &scid), - update ? "got" : "no"); - - msg = towire_gossip_get_update_reply(NULL, update); - daemon_conn_send(peer->remote, take(msg)); -} - -static u8 *create_channel_update(const tal_t *ctx, - struct routing_state *rstate, +static void update_local_channel(struct daemon *daemon, const struct chan *chan, int direction, bool disable, u16 cltv_expiry_delta, u64 htlc_minimum_msat, u32 fee_base_msat, - u32 fee_proportional_millionths) + u32 fee_proportional_millionths, + const char *caller) { secp256k1_ecdsa_signature dummy_sig; u8 *update, *msg; @@ -1026,8 +936,8 @@ static u8 *create_channel_update(const tal_t *ctx, if (disable) channel_flags |= ROUTING_FLAGS_DISABLED; - update = towire_channel_update(ctx, &dummy_sig, - &rstate->chain_hash, + update = towire_channel_update(tmpctx, &dummy_sig, + &daemon->rstate->chain_hash, &chan->scid, timestamp, message_flags, channel_flags, @@ -1043,201 +953,191 @@ static u8 *create_channel_update(const tal_t *ctx, } msg = wire_sync_read(tmpctx, HSM_FD); - if (!msg || !fromwire_hsm_cupdate_sig_reply(ctx, msg, &update)) { + if (!msg || !fromwire_hsm_cupdate_sig_reply(NULL, msg, &update)) { status_failed(STATUS_FAIL_HSM_IO, "Reading cupdate_sig_req: %s", strerror(errno)); } - return update; + /* We always tell peer, even if it's not public yet */ + if (!is_chan_public(chan)) { + struct peer *peer = find_peer(daemon, + &chan->nodes[!direction]->id); + if (peer) + queue_peer_msg(peer, update); + } + + msg = handle_channel_update(daemon->rstate, take(update), caller); + if (msg) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "%s: rejected local channel update %s: %s", + caller, + /* This works because handle_channel_update + * only steals onto tmpctx */ + tal_hex(tmpctx, update), + tal_hex(tmpctx, msg)); } -/* Return true if the only change would be the timestamp. */ -static bool update_redundant(const struct half_chan *hc, - bool disable, u16 cltv_delta, u64 htlc_minimum_msat, - u32 fee_base_msat, u32 fee_proportional_millionths) +static void maybe_update_local_channel(struct daemon *daemon, + struct chan *chan, int direction) { - if (!is_halfchan_defined(hc)) - return false; + const struct half_chan *hc = &chan->half[direction]; - return !(hc->channel_flags & ROUTING_FLAGS_DISABLED) == !disable - && hc->delay == cltv_delta - && hc->htlc_minimum_msat == htlc_minimum_msat - && hc->base_fee == fee_base_msat - && hc->proportional_fee == fee_proportional_millionths; + /* Don't generate a channel_update for an uninitialized channel. */ + if (!hc->channel_update) + return; + + /* Nothing to update? */ + if (!chan->local_disabled == !(hc->channel_flags & ROUTING_FLAGS_DISABLED)) + return; + + update_local_channel(daemon, chan, direction, + chan->local_disabled, + hc->delay, + hc->htlc_minimum_msat, + hc->base_fee, + hc->proportional_fee, + __func__); } -static struct local_update *find_local_update(struct daemon *daemon, - const struct short_channel_id *scid) +static bool local_direction(struct daemon *daemon, + const struct chan *chan, + int *direction) { - struct local_update *i; - - list_for_each(&daemon->local_updates, i, list) { - if (short_channel_id_eq(scid, &i->scid)) - return i; + for (*direction = 0; *direction < 2; (*direction)++) { + if (pubkey_eq(&chan->nodes[*direction]->id, &daemon->id)) + return true; } - return NULL; + return false; } -/* Frees local_update */ -static void apply_delayed_local_update(struct local_update *local_update) +static void handle_get_update(struct peer *peer, const u8 *msg) { + struct short_channel_id scid; struct chan *chan; - const struct half_chan *hc; - u8 *cupdate, *err; + const u8 *update; + struct routing_state *rstate = peer->daemon->rstate; + int direction; - /* Can theoretically happen if channel just closed. */ - chan = get_channel(local_update->daemon->rstate, &local_update->scid); - if (!chan) { - status_trace("Delayed local_channel_update for unknown %s", - type_to_string(tmpctx, struct short_channel_id, - &local_update->scid)); - tal_free(local_update); + if (!fromwire_gossip_get_update(msg, &scid)) { + status_trace("peer %s sent bad gossip_get_update %s", + type_to_string(tmpctx, struct pubkey, &peer->id), + tal_hex(tmpctx, msg)); return; } - /* Convenience variable */ - hc = &chan->half[local_update->direction]; - - /* Avoid redundant updates on public channels: on non-public channels - * we'd need to consider pending updates, so don't bother. */ - if (is_chan_public(chan) - && update_redundant(hc, - local_update->disable, - local_update->cltv_delta, - local_update->htlc_minimum_msat, - local_update->fee_base_msat, - local_update->fee_proportional_millionths)) { - status_trace("Suppressing redundant channel update for %s:(%u) %s %"PRIu64"/%u vs %u/%u", - type_to_string(tmpctx, struct short_channel_id, - &local_update->scid), - local_update->direction, - is_halfchan_defined(hc) - ? (hc->channel_flags & ROUTING_FLAGS_DISABLED ? "DISABLED" : "ACTIVE") - : "UNDEFINED", - hc->last_timestamp, - (u32)time_now().ts.tv_sec, - hc->channel_flags, - local_update->disable); - tal_free(local_update); - return; + chan = get_channel(rstate, &scid); + if (!chan) { + status_unusual("peer %s scid %s: unknown channel", + type_to_string(tmpctx, struct pubkey, &peer->id), + type_to_string(tmpctx, struct short_channel_id, + &scid)); + update = NULL; + goto out; } - cupdate = create_channel_update(tmpctx, local_update->daemon->rstate, - chan, local_update->direction, - local_update->disable, - local_update->cltv_delta, - local_update->htlc_minimum_msat, - local_update->fee_base_msat, - local_update->fee_proportional_millionths); - - err = handle_channel_update(local_update->daemon->rstate, cupdate, - "apply_delayed_local_update"); - if (err) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Rejected local channel update %s: %s", - tal_hex(tmpctx, cupdate), - tal_hex(tmpctx, err)); - - /* We always tell peer, even if it's not public yet */ - if (!is_chan_public(chan)) { - struct peer *peer = find_peer(local_update->daemon, - &chan->nodes[!local_update - ->direction]->id); - if (peer) - queue_peer_msg(peer, take(cupdate)); + /* We want the update that comes from our end. */ + if (!local_direction(peer->daemon, chan, &direction)) { + status_unusual("peer %s scid %s: not our channel?", + type_to_string(tmpctx, struct pubkey, &peer->id), + type_to_string(tmpctx, + struct short_channel_id, + &scid)); + update = NULL; + goto out; } - status_trace("Channel update for %s(%u)%s", - type_to_string(tmpctx, struct short_channel_id, - &local_update->scid), - local_update->direction, - is_chan_public(chan) ? "" : " (private)"); + /* Since we're going to send it out, make sure it's up-to-date. */ + maybe_update_local_channel(peer->daemon, chan, direction); - /* That channel_update might trigger our first channel_announcement */ - maybe_send_own_node_announce(local_update->daemon); - tal_free(local_update); -} + update = chan->half[direction].channel_update; +out: + status_trace("peer %s schanid %s: %s update", + type_to_string(tmpctx, struct pubkey, &peer->id), + type_to_string(tmpctx, struct short_channel_id, &scid), + update ? "got" : "no"); -static void destroy_local_update(struct local_update *local_update) -{ - list_del_from(&local_update->daemon->local_updates, - &local_update->list); + msg = towire_gossip_get_update_reply(NULL, update); + daemon_conn_send(peer->remote, take(msg)); } -static void queue_local_update(struct daemon *daemon, - struct local_update *local_update, - bool instant) +/* Return true if the information has changed. */ +static bool halfchan_new_info(const struct half_chan *hc, + u16 cltv_delta, u64 htlc_minimum_msat, + u32 fee_base_msat, u32 fee_proportional_millionths) { - /* Free any old unapplied update. */ - tal_free(find_local_update(daemon, &local_update->scid)); - - list_add_tail(&daemon->local_updates, &local_update->list); - tal_add_destructor(local_update, destroy_local_update); + if (!is_halfchan_defined(hc)) + return true; - if (instant) - apply_delayed_local_update(local_update); - else - /* Delay 1/4 a broadcast interval */ - new_reltimer(&daemon->timers, local_update, - time_from_msec(daemon->broadcast_interval/4), - apply_delayed_local_update, local_update); + return hc->delay != cltv_delta + || hc->htlc_minimum_msat != htlc_minimum_msat + || hc->base_fee != fee_base_msat + || hc->proportional_fee != fee_proportional_millionths; } static void handle_local_channel_update(struct peer *peer, const u8 *msg) { struct chan *chan; - struct local_update *local_update; - bool delay; - const struct pubkey *my_id = &peer->daemon->rstate->local_id; - - local_update = tal(peer->daemon, struct local_update); - local_update->daemon = peer->daemon; + struct short_channel_id scid; + bool disable; + u16 cltv_expiry_delta; + u64 htlc_minimum_msat; + u32 fee_base_msat; + u32 fee_proportional_millionths; + int direction; if (!fromwire_gossip_local_channel_update(msg, - &local_update->scid, - &local_update->disable, - &local_update->cltv_delta, - &local_update->htlc_minimum_msat, - &local_update->fee_base_msat, - &local_update->fee_proportional_millionths)) { + &scid, + &disable, + &cltv_expiry_delta, + &htlc_minimum_msat, + &fee_base_msat, + &fee_proportional_millionths)) { status_broken("peer %s bad local_channel_update %s", type_to_string(tmpctx, struct pubkey, &peer->id), tal_hex(tmpctx, msg)); - tal_free(local_update); return; } /* Can theoretically happen if channel just closed. */ - chan = get_channel(peer->daemon->rstate, &local_update->scid); + chan = get_channel(peer->daemon->rstate, &scid); if (!chan) { status_trace("peer %s local_channel_update for unknown %s", type_to_string(tmpctx, struct pubkey, &peer->id), type_to_string(tmpctx, struct short_channel_id, - &local_update->scid)); - tal_free(local_update); + &scid)); return; } - if (pubkey_eq(&chan->nodes[0]->id, my_id)) - local_update->direction = 0; - else if (pubkey_eq(&chan->nodes[1]->id, my_id)) - local_update->direction = 1; - else { + if (!local_direction(peer->daemon, chan, &direction)) { status_broken("peer %s bad local_channel_update for non-local %s", type_to_string(tmpctx, struct pubkey, &peer->id), type_to_string(tmpctx, struct short_channel_id, - &local_update->scid)); - tal_free(local_update); + &scid)); return; } - /* Don't delay the initialization update. */ - delay = !is_halfchan_defined(&chan->half[local_update->direction]); + /* We could change configuration on restart; update immediately. + * Or, if we're *enabling* an announced-disabled channel. + * Or, if it's an unannounced channel (only sending to peer). */ + if (halfchan_new_info(&chan->half[direction], + cltv_expiry_delta, htlc_minimum_msat, + fee_base_msat, fee_proportional_millionths) + || ((chan->half[direction].channel_flags & ROUTING_FLAGS_DISABLED) + && !disable) + || !is_chan_public(chan)) { + update_local_channel(peer->daemon, chan, direction, + disable, + cltv_expiry_delta, + htlc_minimum_msat, + fee_base_msat, + fee_proportional_millionths, + __func__); + } - /* channeld has reconnected, remove local disable. */ - chan->local_disabled = false; - queue_local_update(peer->daemon, local_update, delay); + /* Normal case: just toggle local_disabled, and generate broadcast in + * maybe_update_local_channel when/if someone asks about it. */ + chan->local_disabled = disable; } /** @@ -1765,30 +1665,24 @@ static struct io_plan *dev_gossip_suppress(struct io_conn *conn, } #endif /* DEVELOPER */ -static void gossip_send_keepalive_update(struct routing_state *rstate, +static void gossip_send_keepalive_update(struct daemon *daemon, const struct chan *chan, const struct half_chan *hc) { - u8 *update, *err; - - /* Generate a new update, with up to date timestamp */ - update = create_channel_update(tmpctx, rstate, chan, - hc->channel_flags & ROUTING_FLAGS_DIRECTION, - false, - hc->delay, - hc->htlc_minimum_msat, - hc->base_fee, - hc->proportional_fee); - status_trace("Sending keepalive channel_update for %s", type_to_string(tmpctx, struct short_channel_id, &chan->scid)); - err = handle_channel_update(rstate, update, "keepalive"); - if (err) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "rejected keepalive channel_update: %s", - tal_hex(tmpctx, err)); + /* As a side-effect, this will create an update which matches the + * local_disabled state */ + update_local_channel(daemon, chan, + hc->channel_flags & ROUTING_FLAGS_DIRECTION, + chan->local_disabled, + hc->delay, + hc->htlc_minimum_msat, + hc->base_fee, + hc->proportional_fee, + __func__); } static void gossip_refresh_network(struct daemon *daemon) @@ -1827,87 +1721,16 @@ static void gossip_refresh_network(struct daemon *daemon) continue; } - gossip_send_keepalive_update(daemon->rstate, n->chans[i], - hc); + gossip_send_keepalive_update(daemon, n->chans[i], hc); } } route_prune(daemon->rstate); } -static void gossip_disable_outgoing_halfchan(struct daemon *daemon, - struct chan *chan) -{ - u8 direction; - struct half_chan *hc; - u8 message_flags, channel_flags; - u32 timestamp; - struct bitcoin_blkid chain_hash; - secp256k1_ecdsa_signature sig; - struct local_update *local_update; - struct routing_state *rstate = daemon->rstate; - - direction = pubkey_eq(&chan->nodes[0]->id, &rstate->local_id)?0:1; - assert(chan); - hc = &chan->half[direction]; - - if (!is_halfchan_defined(hc)) - return; - - status_trace("Disabling channel %s/%d, active %d -> %d", - type_to_string(tmpctx, struct short_channel_id, &chan->scid), - direction, is_halfchan_enabled(hc), 0); - - local_update = tal(daemon, struct local_update); - local_update->daemon = daemon; - local_update->direction = direction; - - if (!fromwire_channel_update( - hc->channel_update, &sig, &chain_hash, - &local_update->scid, ×tamp, - &message_flags, &channel_flags, - &local_update->cltv_delta, - &local_update->htlc_minimum_msat, - &local_update->fee_base_msat, - &local_update->fee_proportional_millionths)) { - status_failed( - STATUS_FAIL_INTERNAL_ERROR, - "Unable to parse previously accepted channel_update"); - } - - local_update->disable = true; - - queue_local_update(daemon, local_update, false); -} - -/** - * Disable both directions of a local channel. - * - * Disables both directions of a local channel as a result of a close or lost - * connection. A disabling `channel_update` will be queued for the outgoing - * direction as well, but that will be a little delayed. We can't do that for - * the incoming direction, so we set local_disabled and the other endpoint - * should take care of publicly disabling it with a `channel_update`. - * - * It is important to disable the incoming edge as well since we might otherwise - * return that edge as a `contact_point` as part of an invoice. - */ -static void gossip_disable_local_channel(struct daemon *daemon, - struct chan *chan) -{ - struct routing_state *rstate = daemon->rstate; - - assert(pubkey_eq(&rstate->local_id, &chan->nodes[0]->id) || - pubkey_eq(&rstate->local_id, &chan->nodes[1]->id)); - - chan->local_disabled = true; - gossip_disable_outgoing_halfchan(daemon, chan); -} - static void gossip_disable_local_channels(struct daemon *daemon) { - struct node *local_node = - get_node(daemon->rstate, &daemon->rstate->local_id); + struct node *local_node = get_node(daemon->rstate, &daemon->id); size_t i; /* We don't have a local_node, so we don't have any channels yet @@ -1916,8 +1739,7 @@ static void gossip_disable_local_channels(struct daemon *daemon) return; for (i = 0; i < tal_count(local_node->chans); i++) - gossip_disable_local_channel(daemon, - local_node->chans[i]); + local_node->chans[i]->local_disabled = true; } /* Parse an incoming gossip init message and assign config variables @@ -1984,18 +1806,6 @@ static struct io_plan *resolve_channel_req(struct io_conn *conn, return daemon_conn_read_next(conn, &daemon->master); } -static void peer_disable_channels(struct daemon *daemon, struct node *node) -{ - struct chan *c; - size_t i; - for (i=0; i<tal_count(node->chans); i++) { - c = node->chans[i]; - if (pubkey_eq(&other_node(node, c)->id, - &daemon->rstate->local_id)) - gossip_disable_local_channel(daemon, c); - } -} - static struct io_plan *handle_txout_reply(struct io_conn *conn, struct daemon *daemon, const u8 *msg) { @@ -2099,7 +1909,7 @@ static struct io_plan *handle_local_channel_close(struct io_conn *conn, chan = get_channel(rstate, &scid); if (chan) - gossip_disable_local_channel(daemon, chan); + chan->local_disabled = true; return daemon_conn_read_next(conn, &daemon->master); } @@ -2255,7 +2065,6 @@ int main(int argc, char *argv[]) daemon = tal(NULL, struct daemon); list_head_init(&daemon->peers); - list_head_init(&daemon->local_updates); timers_init(&daemon->timers, time_mono()); daemon->broadcast_interval = 30000; daemon->last_announce_timestamp = 0; diff --git a/gossipd/routing.c b/gossipd/routing.c index 793357db4..845fe53d9 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -1101,7 +1101,7 @@ bool routing_add_channel_update(struct routing_state *rstate, return true; } -u8 *handle_channel_update(struct routing_state *rstate, const u8 *update, +u8 *handle_channel_update(struct routing_state *rstate, const u8 *update TAKES, const char *source) { u8 *serialized; diff --git a/gossipd/routing.h b/gossipd/routing.h index 349d43e9c..979fd7929 100644 --- a/gossipd/routing.h +++ b/gossipd/routing.h @@ -252,7 +252,7 @@ void handle_pending_cannouncement(struct routing_state *rstate, const u8 *txscript); /* Returns NULL if all OK, otherwise an error for the peer which sent. */ -u8 *handle_channel_update(struct routing_state *rstate, const u8 *update, +u8 *handle_channel_update(struct routing_state *rstate, const u8 *update TAKES, const char *source); /* Returns NULL if all OK, otherwise an error for the peer which sent. */ diff --git a/tests/test_pay.py b/tests/test_pay.py index d5ea50223..4567eda80 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -91,11 +91,13 @@ def test_pay_disconnect(node_factory, bitcoind): inv = l2.rpc.invoice(123000, 'test_pay_disconnect', 'description') rhash = inv['payment_hash'] + wait_for(lambda: [c['active'] for c in l1.rpc.listchannels()['channels']] == [True, True]) + # Can't use `pay` since that'd notice that we can't route, due to disabling channel_update route = l1.rpc.getroute(l2.info['id'], 123000, 1)["route"] l2.stop() - l1.daemon.wait_for_log('Disabling channel .*, active 1 -> 0') + wait_for(lambda: [c['active'] for c in l1.rpc.listchannels()['channels']] == [False, False]) # Can't pay while its offline. with pytest.raises(RpcError): @@ -146,8 +148,8 @@ def test_pay_get_error_with_update(node_factory): l2.rpc.dev_suppress_gossip() l3.stop() - # Make sure that l2 has processed the local update which disables. - l2.daemon.wait_for_log('Received channel_update for channel {}\(.*\) now DISABLED was ACTIVE \(from apply_delayed_local_update\)'.format(chanid2)) + # Make sure that l2 has seen disconnect, considers channel disabled. + wait_for(lambda: [c['active'] for c in l2.rpc.listchannels(chanid2)['channels']] == [False, False]) l1.rpc.sendpay(route, inv['payment_hash']) with pytest.raises(RpcError, match=r'WIRE_TEMPORARY_CHANNEL_FAILURE'):