Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix htlc error causing connclose #4550

Merged
merged 2 commits into from
May 21, 2021
Merged

Fix htlc error causing connclose #4550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d10fe6d
pytest: add test for HTLC timeout on failed htlcs.
rustyrussell May 21, 2021
de3fe42
lightningd: attach HTLC timeout to htlc itself, fix gratuitous discon…
rustyrussell May 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions lightningd/channel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,6 @@ struct channel *new_unsaved_channel(struct peer *peer,
/* A zero value database id means it's not saved in the database yet */
channel->dbid = 0;
channel->error = NULL;
channel->htlc_timeout = NULL;
channel->openchannel_signed_cmd = NULL;
channel->state = DUALOPEND_OPEN_INIT;
channel->owner = NULL;
Expand Down Expand Up @@ -345,7 +344,6 @@ struct channel *new_channel(struct peer *peer, u64 dbid,
channel->dbid = dbid;
channel->unsaved_dbid = 0;
channel->error = NULL;
channel->htlc_timeout = NULL;
channel->open_attempt = NULL;
channel->openchannel_signed_cmd = NULL;
if (their_shachain)
Expand Down
3 changes: 0 additions & 3 deletions lightningd/channel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,6 @@ struct channel {
struct amount_msat msat_to_us_min;
struct amount_msat msat_to_us_max;

/* Timer we use in case they don't add an HTLC in a timely manner. */
struct oneshot *htlc_timeout;

/* Last tx they gave us. */
struct bitcoin_tx *last_tx;
enum wallet_tx_type last_tx_type;
Expand Down
1 change: 1 addition & 0 deletions lightningd/htlc_end.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ struct htlc_out *new_htlc_out(const tal_t *ctx,
hout->failmsg = NULL;
hout->failonion = NULL;
hout->preimage = NULL;
hout->timeout = NULL;

if (blinding)
hout->blinding = tal_dup(hout, struct pubkey, blinding);
Expand Down
3 changes: 3 additions & 0 deletions lightningd/htlc_end.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ struct htlc_out {

/* Blinding to send alongside, if any. */
struct pubkey *blinding;

/* Timer we use in case they don't add an HTLC in a timely manner. */
struct oneshot *timeout;
};

static inline const struct htlc_key *keyof_htlc_in(const struct htlc_in *in)
Expand Down
29 changes: 17 additions & 12 deletions lightningd/peer_htlcs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,17 +600,22 @@ static void rcvd_htlc_reply(struct subd *subd, const u8 *msg, const int *fds UNU
/* When channeld includes it in commitment, we'll make it persistent. */
}

static void htlc_offer_timeout(struct channel *channel)
static void htlc_offer_timeout(struct htlc_out *out)
{
/* Unset this in case we reconnect and start again. */
channel->htlc_timeout = NULL;
struct channel *channel = out->key.channel;

out->timeout = NULL;

/* Otherwise, timer would be removed. */
assert(out->hstate == SENT_ADD_HTLC);

/* If owner died, we should already be taken care of. */
if (!channel->owner || channel->state != CHANNELD_NORMAL)
return;

log_unusual(channel->owner->log,
"Adding HTLC too slow: killing connection");
"Adding HTLC %"PRIu64" too slow: killing connection",
out->key.id);
tal_free(channel->owner);
channel_set_billboard(channel, false,
"Adding HTLC timed out: killed connection");
Expand Down Expand Up @@ -659,12 +664,14 @@ const u8 *send_htlc_out(const tal_t *ctx,
partid, in);
tal_add_destructor(*houtp, destroy_hout_subd_died);

/* Give channel 30 seconds to commit (first) htlc. */
if (!out->htlc_timeout && !IFDEV(out->peer->ld->dev_no_htlc_timeout, 0))
out->htlc_timeout = new_reltimer(out->peer->ld->timers,
out, time_from_sec(30),
/* Give channel 30 seconds to commit this htlc. */
if (!IFDEV(out->peer->ld->dev_no_htlc_timeout, 0)) {
(*houtp)->timeout = new_reltimer(out->peer->ld->timers,
*houtp, time_from_sec(30),
htlc_offer_timeout,
out);
*houtp);
}

msg = towire_channeld_offer_htlc(out, amount, cltv, payment_hash,
onion_routing_packet, blinding);
subd_req(out->peer->ld, out->owner, take(msg), -1, 0, rcvd_htlc_reply,
Expand Down Expand Up @@ -1627,8 +1634,8 @@ static bool update_out_htlc(struct channel *channel,
/* First transition into commitment; now it outlives peer. */
if (newstate == SENT_ADD_COMMIT) {
tal_del_destructor(hout, destroy_hout_subd_died);
hout->timeout = tal_free(hout->timeout);
tal_steal(ld, hout);

} else if (newstate == RCVD_REMOVE_ACK_REVOCATION) {
remove_htlc_out(channel, hout);
}
Expand Down Expand Up @@ -1721,8 +1728,6 @@ void peer_sending_commitsig(struct channel *channel, const u8 *msg)
struct lightningd *ld = channel->peer->ld;
struct penalty_base *pbase;

channel->htlc_timeout = tal_free(channel->htlc_timeout);

if (!fromwire_channeld_sending_commitsig(msg, msg,
&commitnum,
&pbase,
Expand Down
28 changes: 27 additions & 1 deletion tests/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
scriptpubkey_addr,
EXPERIMENTAL_FEATURES
)
from pyln.testing.utils import SLOW_MACHINE, VALGRIND, EXPERIMENTAL_DUAL_FUND
from pyln.testing.utils import SLOW_MACHINE, VALGRIND, EXPERIMENTAL_DUAL_FUND, FUNDAMOUNT

import os
import pytest
Expand Down Expand Up @@ -3284,3 +3284,29 @@ def test_openchannel_init_alternate(node_factory, executor):
fut = executor.submit(l2.rpc.openchannel_init, l1.info['id'], '1000000msat', psbt2)
with pytest.raises(RpcError):
fut.result(10)


def test_htlc_failed_noclose(node_factory):
"""Test a bug where the htlc timeout would kick in even if the HTLC failed"""
l1, l2 = node_factory.line_graph(2)

payment_hash = l2.rpc.invoice(1000, "test", "test")['payment_hash']
routestep = {
'msatoshi': FUNDAMOUNT * 1000,
'id': l2.info['id'],
'delay': 5,
'channel': '1x1x1' # note: can be bogus for 1-hop direct payments
}

# This fails at channeld
l1.rpc.sendpay([routestep], payment_hash)
with pytest.raises(RpcError, match="Capacity exceeded"):
l1.rpc.waitsendpay(payment_hash)

# Send a second one, too: make sure we don't crash.
l1.rpc.sendpay([routestep], payment_hash)
with pytest.raises(RpcError, match="Capacity exceeded"):
l1.rpc.waitsendpay(payment_hash)

time.sleep(35)
assert l1.rpc.getpeer(l2.info['id'])['connected']
2 changes: 1 addition & 1 deletion tests/test_misc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1418,7 +1418,7 @@ def test_htlc_send_timeout(node_factory, bitcoind, compat):
assert not l2.daemon.is_in_log(r'{}-.*channeld.*: \[IN\] 0013'.format(l3.info['id']))
assert not l2.daemon.is_in_log(r'{}-.*channeld.*: \[OUT\] 0084'.format(l3.info['id']))
# L2 killed the channel with l3 because it was too slow.
l2.daemon.wait_for_log('{}-.*channeld-.*Adding HTLC too slow: killing connection'.format(l3.info['id']))
l2.daemon.wait_for_log('{}-.*channeld-.*Adding HTLC 0 too slow: killing connection'.format(l3.info['id']))


def test_ipv4_and_ipv6(node_factory):
Expand Down