mirrors/core-lightning
1
0
Fork 0
mirror of https://github.com/ElementsProject/lightning.git synced 2025-03-01 17:47:30 +01:00
Code Issues Projects Releases Wiki Activity

gossipd: clean up local channel updates.

Browse source 
Make update_local_channel use a timer if it's too soon to make another
update.

1. Implement cupdate_different() which compares two updates.
2. make update_local_channel() take a single arg for timer usage.
3. Set timestamp of non-disable update back 5 minutes, so we can
   always generate a disable update if we need to.
4. Make update_local_channel() itself do the "unchanged update" suppression.
   gossipd: clean up local channel updates.
5. Keep pointer to the current timer so we override any old updates with
   a new one, to avoid a race.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This commit is contained in:
Rusty Russell 2019-09-16 20:13:59 +09:30
parent e1c431d278
commit 0bab2580fc
5 changed files with 158 additions and 135 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

270
gossipd/gossipd.c
View file

@ -510,6 +510,24 @@ static void get_cupdate_parts(const u8 *channel_update,
sizes[1] = tal_count(channel_update) - (64 + 2 + 32 + 8 + 4); sizes[1] = tal_count(channel_update) - (64 + 2 + 32 + 8 + 4);
} }
/*~ Is this channel_update different from prev (not sigs and timestamps)? */
static bool cupdate_different(struct gossip_store *gs,
const struct half_chan *hc,
const u8 *cupdate)
{
const u8 *oparts[2], *nparts[2];
size_t osizes[2], nsizes[2];
const u8 *orig;
/* Get last one we have. */
orig = gossip_store_get(tmpctx, gs, hc->bcast.index);
get_cupdate_parts(orig, oparts, osizes);
get_cupdate_parts(cupdate, nparts, nsizes);
return !memeq(oparts[0], osizes[0], nparts[0], nsizes[0])
|| !memeq(oparts[1], osizes[1], nparts[1], nsizes[1]);
}
/* Get non-signature, non-timestamp parts of (valid!) node_announcement */ /* Get non-signature, non-timestamp parts of (valid!) node_announcement */
static void get_nannounce_parts(const u8 *node_announcement, static void get_nannounce_parts(const u8 *node_announcement,
const u8 *parts[2], const u8 *parts[2],
@ -1684,39 +1702,46 @@ static void dump_gossip(struct peer *peer)
maybe_create_next_scid_reply(peer); maybe_create_next_scid_reply(peer);
} }
/*~ Our timer callbacks take a single argument, so we marshall everything
* we need into this structure: */
struct local_cupdate {
struct daemon *daemon;
struct local_chan *local_chan;
bool disable;
bool even_if_identical;
u16 cltv_expiry_delta;
struct amount_msat htlc_minimum, htlc_maximum;
u32 fee_base_msat, fee_proportional_millionths;
};
/*~ This generates a `channel_update` message for one of our channels. We do /*~ This generates a `channel_update` message for one of our channels. We do
* this here, rather than in `channeld` because we (may) need to do it * this here, rather than in `channeld` because we (may) need to do it
* ourselves anyway if channeld dies, or when we refresh it once a week. */ * ourselves anyway if channeld dies, or when we refresh it once a week,
static void update_local_channel(struct daemon *daemon, * and so we can avoid creating redundant ones. */
const struct chan *chan, static void update_local_channel(struct local_cupdate *lc /* frees! */)
int direction,
bool disable,
u16 cltv_expiry_delta,
struct amount_msat htlc_minimum,
u32 fee_base_msat,
u32 fee_proportional_millionths,
struct amount_msat htlc_maximum,
const char *caller)
{ {
struct daemon *daemon = lc->daemon;
secp256k1_ecdsa_signature dummy_sig; secp256k1_ecdsa_signature dummy_sig;
u8 *update, *msg; u8 *update, *msg;
u32 timestamp = gossip_time_now(daemon->rstate).ts.tv_sec; u32 timestamp = gossip_time_now(daemon->rstate).ts.tv_sec, next;
u8 message_flags, channel_flags; u8 message_flags, channel_flags;
struct chan *chan = lc->local_chan->chan;
struct half_chan *hc;
const int direction = lc->local_chan->direction;
/* Discard existing timer. */
lc->local_chan->channel_update_timer
= tal_free(lc->local_chan->channel_update_timer);
/* So valgrind doesn't complain */ /* So valgrind doesn't complain */
memset(&dummy_sig, 0, sizeof(dummy_sig)); memset(&dummy_sig, 0, sizeof(dummy_sig));
/* BOLT #7: /* Create an unsigned channel_update: we backdate enables, so
* * we can always send a disable in an emergency. */
* The origin node: if (!lc->disable)
*... timestamp -= daemon->gossip_min_interval;
* - MUST set `timestamp` to greater than 0, AND to greater than any
* previously-sent `channel_update` for this `short_channel_id`.
* - SHOULD base `timestamp` on a UNIX timestamp.
*/
if (is_halfchan_defined(&chan->half[direction])
&& timestamp == chan->half[direction].bcast.timestamp)
timestamp++;
/* BOLT #7: /* BOLT #7:
* *
@ -1731,7 +1756,7 @@ static void update_local_channel(struct daemon *daemon,
* | 1 | `disable` | Disable the channel. | * | 1 | `disable` | Disable the channel. |
*/ */
channel_flags = direction; channel_flags = direction;
if (disable) if (lc->disable)
channel_flags |= ROUTING_FLAGS_DISABLED; channel_flags |= ROUTING_FLAGS_DISABLED;
/* BOLT #7: /* BOLT #7:
@ -1752,11 +1777,41 @@ static void update_local_channel(struct daemon *daemon,
&chan->scid, &chan->scid,
timestamp, timestamp,
message_flags, channel_flags, message_flags, channel_flags,
cltv_expiry_delta, lc->cltv_expiry_delta,
htlc_minimum, lc->htlc_minimum,
fee_base_msat, lc->fee_base_msat,
fee_proportional_millionths, lc->fee_proportional_millionths,
htlc_maximum); lc->htlc_maximum);
hc = &chan->half[direction];
if (is_halfchan_defined(hc)) {
/* Suppress duplicates. */
if (!lc->even_if_identical
&& !cupdate_different(daemon->rstate->gs, hc, update)) {
tal_free(lc);
return;
}
/* Is it too soon to send another update? */
next = hc->bcast.timestamp + daemon->gossip_min_interval;
if (timestamp < next) {
status_debug("channel_update %s/%u: delaying %u secs",
type_to_string(tmpctx,
struct short_channel_id,
&chan->scid),
direction,
next - timestamp);
lc->local_chan->channel_update_timer
= new_reltimer(&daemon->timers, lc,
time_from_sec(next - timestamp),
update_local_channel,
lc);
/* If local chan vanishes, so does update, and timer. */
notleak(tal_steal(lc->local_chan, lc));
return;
}
}
/* Note that we treat the hsmd as synchronous. This is simple (no /* Note that we treat the hsmd as synchronous. This is simple (no
* callback hell)!, but may need to change to async if we ever want * callback hell)!, but may need to change to async if we ever want
@ -1793,50 +1848,50 @@ static void update_local_channel(struct daemon *daemon,
/* We feed it into routing.c like any other channel_update; it may /* We feed it into routing.c like any other channel_update; it may
* discard it (eg. non-public channel), but it should not complain * discard it (eg. non-public channel), but it should not complain
* about it being invalid! */ * about it being invalid! __func__ is a magic C constant which
msg = handle_channel_update(daemon->rstate, take(update), caller, NULL); * expands to this function name. */
msg = handle_channel_update(daemon->rstate, take(update), __func__,
NULL);
if (msg) if (msg)
status_failed(STATUS_FAIL_INTERNAL_ERROR, status_failed(STATUS_FAIL_INTERNAL_ERROR,
"%s: rejected local channel update %s: %s", "%s: rejected local channel update %s: %s",
caller, __func__,
/* Normally we must not touch something taken() /* Normally we must not touch something taken()
* but we're in deep trouble anyway, and * but we're in deep trouble anyway, and
* handle_channel_update only tal_steals onto * handle_channel_update only tal_steals onto
* tmpctx, so it's actually OK. */ * tmpctx, so it's actually OK. */
tal_hex(tmpctx, update), tal_hex(tmpctx, update),
tal_hex(tmpctx, msg)); tal_hex(tmpctx, msg));
tal_free(lc);
} }
/*~ We generate local channel updates lazily; most of the time we simply /*~ This is a refresh of a local channel: sends an update if one is needed. */
* toggle the `local_disabled` flag so we don't use it to route. We never static void refresh_local_channel(struct daemon *daemon,
* change anything else after startup (yet!) */ struct local_chan *local_chan,
static void maybe_update_local_channel(struct daemon *daemon, bool even_if_identical)
struct chan *chan, int direction)
{ {
const struct half_chan *hc = &chan->half[direction]; const struct half_chan *hc;
bool local_disabled; struct local_cupdate *lc;
hc = &local_chan->chan->half[local_chan->direction];
/* Don't generate a channel_update for an uninitialized channel. */ /* Don't generate a channel_update for an uninitialized channel. */
if (!is_halfchan_defined(hc)) if (!is_halfchan_defined(hc))
return; return;
/* Nothing to update? */ lc = tal(NULL, struct local_cupdate);
local_disabled = is_chan_local_disabled(daemon->rstate, chan); lc->daemon = daemon;
/*~ Note the inversions here on both sides, which is cheap conversion to lc->local_chan = local_chan;
* boolean for the RHS! */ lc->even_if_identical = even_if_identical;
if (!local_disabled == !(hc->channel_flags & ROUTING_FLAGS_DISABLED)) lc->disable = (hc->channel_flags & ROUTING_FLAGS_DISABLED)
return; || local_chan->local_disabled;
lc->cltv_expiry_delta = hc->delay;
lc->htlc_minimum = hc->htlc_minimum;
lc->htlc_maximum = hc->htlc_maximum;
lc->fee_base_msat = hc->base_fee;
lc->fee_proportional_millionths = hc->proportional_fee;
update_local_channel(daemon, chan, direction, update_local_channel(lc);
local_disabled,
hc->delay,
hc->htlc_minimum,
hc->base_fee,
hc->proportional_fee,
hc->htlc_maximum,
/* Note this magic C macro which expands to the
* function name, for debug messages */
__func__);
} }
/*~ This is when channeld asks us for a channel_update for a local channel. /*~ This is when channeld asks us for a channel_update for a local channel.
@ -1873,7 +1928,7 @@ static bool handle_get_update(struct peer *peer, const u8 *msg)
chan = local_chan->chan; chan = local_chan->chan;
/* Since we're going to send it out, make sure it's up-to-date. */ /* Since we're going to send it out, make sure it's up-to-date. */
maybe_update_local_channel(peer->daemon, chan, local_chan->direction); refresh_local_channel(peer->daemon, local_chan, false);
/* It's possible this is zero, if we've never sent a channel_update /* It's possible this is zero, if we've never sent a channel_update
* for that channel. */ * for that channel. */
@ -1893,92 +1948,49 @@ out:
return true; return true;
} }
/*~ Return true if the channel information has changed. This can only
* currently happen if the user restarts with different fee options, but we
* don't assume that. */
static bool halfchan_new_info(const struct half_chan *hc,
u16 cltv_delta, struct amount_msat htlc_minimum,
u32 fee_base_msat, u32 fee_proportional_millionths,
struct amount_msat htlc_maximum)
{
if (!is_halfchan_defined(hc))
return true;
return hc->delay != cltv_delta
|| !amount_msat_eq(hc->htlc_minimum, htlc_minimum)
|| hc->base_fee != fee_base_msat
|| hc->proportional_fee != fee_proportional_millionths
|| !amount_msat_eq(hc->htlc_maximum, htlc_maximum);
}
/*~ channeld asks us to update the local channel. */ /*~ channeld asks us to update the local channel. */
static bool handle_local_channel_update(struct peer *peer, const u8 *msg) static bool handle_local_channel_update(struct peer *peer, const u8 *msg)
{ {
struct local_chan *local_chan;
struct chan *chan;
struct short_channel_id scid; struct short_channel_id scid;
bool disable; struct local_cupdate *lc = tal(tmpctx, struct local_cupdate);
u16 cltv_expiry_delta;
struct amount_msat htlc_minimum, htlc_maximum; lc->daemon = peer->daemon;
u32 fee_base_msat; lc->even_if_identical = false;
u32 fee_proportional_millionths;
/* FIXME: We should get scid from lightningd when setting up the /* FIXME: We should get scid from lightningd when setting up the
* connection, so no per-peer daemon can mess with channels other than * connection, so no per-peer daemon can mess with channels other than
* its own! */ * its own! */
if (!fromwire_gossipd_local_channel_update(msg, if (!fromwire_gossipd_local_channel_update(msg,
&scid, &scid,
&disable, &lc->disable,
&cltv_expiry_delta, &lc->cltv_expiry_delta,
&htlc_minimum, &lc->htlc_minimum,
&fee_base_msat, &lc->fee_base_msat,
&fee_proportional_millionths, &lc->fee_proportional_millionths,
&htlc_maximum)) { &lc->htlc_maximum)) {
status_broken("peer %s bad local_channel_update %s", status_broken("peer %s bad local_channel_update %s",
type_to_string(tmpctx, struct node_id, &peer->id), type_to_string(tmpctx, struct node_id, &peer->id),
tal_hex(tmpctx, msg)); tal_hex(tmpctx, msg));
return false; return false;
} }
local_chan = local_chan_map_get(&peer->daemon->rstate->local_chan_map, lc->local_chan = local_chan_map_get(&peer->daemon->rstate->local_chan_map,
&scid); &scid);
/* Can theoretically happen if channel just closed. */ /* Can theoretically happen if channel just closed. */
if (!local_chan) { if (!lc->local_chan) {
status_debug("peer %s local_channel_update for unknown %s", status_debug("peer %s local_channel_update for unknown %s",
type_to_string(tmpctx, struct node_id, &peer->id), type_to_string(tmpctx, struct node_id, &peer->id),
type_to_string(tmpctx, struct short_channel_id, type_to_string(tmpctx, struct short_channel_id,
&scid)); &scid));
return true; return true;
} }
chan = local_chan->chan;
/* We could change configuration on restart; update immediately. /* Remove soft local_disabled flag, if they're marking it enabled. */
* Or, if we're *enabling* an announced-disabled channel. if (!lc->disable)
* Or, if it's an unannounced channel (only sending to peer). */ local_enable_chan(peer->daemon->rstate, lc->local_chan->chan);
if (halfchan_new_info(&chan->half[local_chan->direction],
cltv_expiry_delta, htlc_minimum,
fee_base_msat, fee_proportional_millionths,
htlc_maximum)
|| ((chan->half[local_chan->direction].channel_flags & ROUTING_FLAGS_DISABLED)
&& !disable)
|| !is_chan_public(chan)) {
update_local_channel(peer->daemon, chan, local_chan->direction,
disable,
cltv_expiry_delta,
htlc_minimum,
fee_base_msat,
fee_proportional_millionths,
htlc_maximum,
__func__);
}
/* Normal case: just toggle local_disabled, and generate broadcast in
* maybe_update_local_channel when/if someone asks about it. */
if (disable)
local_disable_chan(peer->daemon->rstate, chan);
else
local_enable_chan(peer->daemon->rstate, chan);
/* Apply the update they told us */
update_local_channel(tal_steal(NULL, lc));
return true; return true;
} }
@ -2284,24 +2296,16 @@ static struct io_plan *connectd_req(struct io_conn *conn,
* was added to the spec because people abandoned their channels without * was added to the spec because people abandoned their channels without
* closing them. */ * closing them. */
static void gossip_send_keepalive_update(struct daemon *daemon, static void gossip_send_keepalive_update(struct daemon *daemon,
const struct chan *chan, struct local_chan *local_chan)
const struct half_chan *hc)
{ {
status_debug("Sending keepalive channel_update for %s", status_debug("Sending keepalive channel_update for %s/%u",
type_to_string(tmpctx, struct short_channel_id, type_to_string(tmpctx, struct short_channel_id,
&chan->scid)); &local_chan->chan->scid),
local_chan->direction);
/* As a side-effect, this will create an update which matches the /* As a side-effect, this will create an update which matches the
* local_disabled state */ * local_disabled state */
update_local_channel(daemon, chan, refresh_local_channel(daemon, local_chan, true);
hc->channel_flags & ROUTING_FLAGS_DIRECTION,
is_chan_local_disabled(daemon->rstate, chan),
hc->delay,
hc->htlc_minimum,
hc->base_fee,
hc->proportional_fee,
hc->htlc_maximum,
__func__);
} }
@ -2334,7 +2338,11 @@ static void gossip_refresh_network(struct daemon *daemon)
struct chan *c; struct chan *c;
for (c = first_chan(n, &i); c; c = next_chan(n, &i)) { for (c = first_chan(n, &i); c; c = next_chan(n, &i)) {
struct half_chan *hc = half_chan_from(n, c); struct local_chan *local_chan;
struct half_chan *hc;
local_chan = is_local_chan(daemon->rstate, c);
hc = &c->half[local_chan->direction];
if (!is_halfchan_defined(hc)) { if (!is_halfchan_defined(hc)) {
/* Connection is not announced yet, so don't even /* Connection is not announced yet, so don't even
@ -2352,7 +2360,7 @@ static void gossip_refresh_network(struct daemon *daemon)
continue; continue;
} }
gossip_send_keepalive_update(daemon, c, hc); gossip_send_keepalive_update(daemon, local_chan);
} }
} }

1
gossipd/routing.c
View file

@ -457,6 +457,7 @@ static struct local_chan *new_local_chan(struct routing_state *rstate,
local_chan->chan = chan; local_chan->chan = chan;
local_chan->direction = direction; local_chan->direction = direction;
local_chan->local_disabled = false; local_chan->local_disabled = false;
local_chan->channel_update_timer = NULL;
local_chan_map_add(&rstate->local_chan_map, local_chan); local_chan_map_add(&rstate->local_chan_map, local_chan);
tal_add_destructor2(local_chan, destroy_local_chan, rstate); tal_add_destructor2(local_chan, destroy_local_chan, rstate);

3
gossipd/routing.h
View file

@ -66,6 +66,9 @@ struct local_chan {
/* We soft-disable local channels when a peer disconnects */ /* We soft-disable local channels when a peer disconnects */
bool local_disabled; bool local_disabled;
/* Timer if we're deferring an update. */
struct oneshot *channel_update_timer;
}; };
/* Use this instead of tal_free(chan)! */ /* Use this instead of tal_free(chan)! */

15
tests/test_gossip.py
View file

@ -145,7 +145,8 @@ def test_gossip_timestamp_filter(node_factory, bitcoind):
subprocess.run(['kill', '-USR1', l1.subd_pid('connectd')]) subprocess.run(['kill', '-USR1', l1.subd_pid('connectd')])
subprocess.run(['kill', '-USR1', l2.subd_pid('connectd')]) subprocess.run(['kill', '-USR1', l2.subd_pid('connectd')])
before_anything = int(time.time() - 1.0) # Updates get backdated 300 seconds.
before_anything = int(time.time() - 301.0)
# Make a public channel. # Make a public channel.
chan12 = l1.fund_channel(l2, 10**5) chan12 = l1.fund_channel(l2, 10**5)
@ -1267,13 +1268,20 @@ def setup_gossip_store_test(node_factory, bitcoind):
l2.rpc.setchannelfee(l1.info['id'], 20, 1000) l2.rpc.setchannelfee(l1.info['id'], 20, 1000)
wait_for(lambda: [c['base_fee_millisatoshi'] for c in l2.rpc.listchannels(scid12)['channels']] == [20, 20]) wait_for(lambda: [c['base_fee_millisatoshi'] for c in l2.rpc.listchannels(scid12)['channels']] == [20, 20])
# Active records in store now looks like: # Records in store now looks (something) like:
# DELETED: local-add-channel (scid23)
# DELETED: private channel_update (scid23/0)
# DELETED: private channel_update (scid23/1)
# channel_announcement (scid23) # channel_announcement (scid23)
# channel_amount # channel_amount
# DELETED: channel_update (scid23/0)
# DELETED: channel_update (scid23/1)
# node_announcement # node_announcement
# node_announcement # node_announcement
# channel_update (scid23) # channel_update (scid23)
# local_add_channel (scid12) # local_add_channel (scid12)
# DELETED: private channel_update (scid12/0)
# DELETED: private channel_update (scid12/1)
# channel_update (scid23) # channel_update (scid23)
# private_channel_update (scid12) # private_channel_update (scid12)
# private_channel_update (scid12) # private_channel_update (scid12)
@ -1294,6 +1302,7 @@ def test_gossip_store_compact_noappend(node_factory, bitcoind):
assert not l2.daemon.is_in_log('gossip_store:.*truncate') assert not l2.daemon.is_in_log('gossip_store:.*truncate')
@unittest.skipIf(not DEVELOPER, "updates are delayed without --dev-broadcast-interval")
def test_gossip_store_load_complex(node_factory, bitcoind): def test_gossip_store_load_complex(node_factory, bitcoind):
l2 = setup_gossip_store_test(node_factory, bitcoind) l2 = setup_gossip_store_test(node_factory, bitcoind)
@ -1373,7 +1382,7 @@ def test_gossip_store_compact_on_load(node_factory, bitcoind):
l2.restart() l2.restart()
wait_for(lambda: l2.daemon.is_in_log('gossip_store_compact_offline: 9 deleted, 9 copied')) wait_for(lambda: l2.daemon.is_in_log('gossip_store_compact_offline: [5-8] deleted, 9 copied'))
wait_for(lambda: l2.daemon.is_in_log(r'gossip_store: Read 1/4/2/0 cannounce/cupdate/nannounce/cdelete from store \(0 deleted\) in 1446 bytes')) wait_for(lambda: l2.daemon.is_in_log(r'gossip_store: Read 1/4/2/0 cannounce/cupdate/nannounce/cdelete from store \(0 deleted\) in 1446 bytes'))

2
tests/test_pay.py
View file

@ -1799,6 +1799,7 @@ def test_pay_direct(node_factory, bitcoind):
assert l1l2msat == l1l2msatreference assert l1l2msat == l1l2msatreference
@unittest.skipIf(not DEVELOPER, "updates are delayed without --dev-broadcast-interval")
def test_setchannelfee_usage(node_factory, bitcoind): def test_setchannelfee_usage(node_factory, bitcoind):
# TEST SETUP # TEST SETUP
# #
@ -2131,6 +2132,7 @@ def test_setchannelfee_restart(node_factory, bitcoind):
assert result['msatoshi_sent'] == 5002020 assert result['msatoshi_sent'] == 5002020
@unittest.skipIf(not DEVELOPER, "updates are delayed without --dev-broadcast-interval")
def test_setchannelfee_all(node_factory, bitcoind): def test_setchannelfee_all(node_factory, bitcoind):
# TEST SETUP # TEST SETUP
# #