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 crash when trying to forget a channel with HTLCs #987

Closed
wants to merge 3 commits into from
Closed

Fix crash when trying to forget a channel with HTLCs #987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fdfd7cb
peer: Extract htlc_{in,out} detection into separate functions
cdecker Feb 12, 2018
d4b9096
json-rpc: Allow forgetting channels that have HTLCs associated
cdecker Feb 12, 2018
abb50ea
pytest: Extend forget-channel test to include HTLCs
cdecker Feb 12, 2018
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
50 changes: 41 additions & 9 deletions lightningd/peer_control.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,29 +91,47 @@ static void peer_set_owner(struct peer *peer, struct subd *owner)
subd_release_peer(old_owner, peer);
}

static void destroy_peer(struct peer *peer)
static struct htlc_out *peer_first_htlc_out(struct peer *peer)
{
/* Must not have any HTLCs! */
struct htlc_out_map_iter outi;
struct htlc_out *hout;
struct htlc_in_map_iter ini;
struct htlc_in *hin;

for (hout = htlc_out_map_first(&peer->ld->htlcs_out, &outi);
hout;
hout = htlc_out_map_next(&peer->ld->htlcs_out, &outi)) {
if (hout->key.peer != peer)
continue;
fatal("Freeing peer %s has hout %s",
peer_state_name(peer->state),
htlc_state_name(hout->hstate));
return hout;
}
return NULL;
}

static struct htlc_in *peer_first_htlc_in(struct peer *peer)
{
struct htlc_in_map_iter ini;
struct htlc_in *hin;
for (hin = htlc_in_map_first(&peer->ld->htlcs_in, &ini);
hin;
hin = htlc_in_map_next(&peer->ld->htlcs_in, &ini)) {
if (hin->key.peer != peer)
continue;
return hin;
}
return NULL;
}

static void destroy_peer(struct peer *peer)
{
/* Must not have any HTLCs! */
struct htlc_out *hout = peer_first_htlc_out(peer);
struct htlc_in *hin = peer_first_htlc_in(peer);

if (hout) {
fatal("Freeing peer %s has hout %s",
peer_state_name(peer->state),
htlc_state_name(hout->hstate));
}

if (hin) {
fatal("Freeing peer %s has hin %s",
peer_state_name(peer->state),
htlc_state_name(hin->hstate));
Expand Down Expand Up @@ -2961,13 +2979,17 @@ static void process_dev_forget_channel(struct bitcoind *bitcoind UNUSED,
"channel");
return;
}

response = new_json_result(forget->cmd);
json_object_start(response, NULL);
json_add_bool(response, "forced", forget->force);
json_add_bool(response, "funding_unspent", txout != NULL);
json_add_txid(response, "funding_txid", forget->peer->funding_txid);
json_object_end(response);

/* Free HTLCs so we don't panic when freeing the peer */
free_htlcs(forget->peer->ld, forget->peer);

if (peer_persists(forget->peer))
wallet_channel_delete(forget->cmd->ld->wallet, forget->peer->channel->id);
free_peer(forget->peer, "dev-forget-channel called");
Expand All @@ -2994,8 +3016,18 @@ static void json_dev_forget_channel(struct command *cmd, const char *buffer,
json_tok_bool(buffer, forcetok, &forget->force);

forget->peer = peer_from_json(cmd->ld, buffer, nodeidtok);
if (!forget->peer)
if (!forget->peer) {
command_fail(cmd, "Could not find channel with that peer");
return;
}

if (!forget->force && (peer_first_htlc_in(forget->peer) ||
peer_first_htlc_out(forget->peer))) {
command_fail(cmd, "Channel has HTLCs attached, use {force} if "
"you are sure about what you're doing.");
return;
}

if (!forget->peer->funding_txid) {
process_dev_forget_channel(cmd->ld->topology->bitcoind, NULL, forget);
} else {
Expand Down
24 changes: 23 additions & 1 deletion tests/test_lightningd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3453,8 +3453,10 @@ def test_cli(self):

@unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1")
def test_forget_channel(self):
l1 = self.node_factory.get_node()
disconnects = ['@WIRE_REVOKE_AND_ACK']
l1 = self.node_factory.get_node(options=['--dev-no-reconnect'])
l2 = self.node_factory.get_node()
l3 = self.node_factory.get_node(disconnect=disconnects)
self.give_funds(l1, 10**6)
l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port'])
l1.rpc.fundchannel(l2.info['id'], 10**5)
Expand All @@ -3473,6 +3475,26 @@ def test_forget_channel(self):
l1.restart()
assert len(l1.rpc.listpeers()['peers']) == 0

# Now try again but have an HTLC
l1.rpc.connect(l3.info['id'], 'localhost', l3.info['port'])
l1.rpc.fundchannel(l3.info['id'], 10**5)
l1.bitcoin.rpc.generate(2)
time.sleep(5)
l3.daemon.wait_for_log(r'State changed from CHANNELD_AWAITING_LOCKIN to CHANNELD_NORMAL')
self.executor.submit(l1.rpc.pay, l3.rpc.invoice(10**6, "test", "test")['bolt11'])
l3.daemon.wait_for_log(r'peer_in WIRE_COMMITMENT_SIGNED')

# Cause the unilateral close and let l1 notice
l3.rpc.dev_fail(l1.info['id'])
l1.bitcoin.rpc.generate(1)

# L1 should refuse to forget if not forced
self.assertRaises(ValueError, l1.rpc.dev_forget_channel, l3.info['id'])

# Forcing should work
l1.rpc.dev_forget_channel(l3.info['id'], True)
assert len(l1.rpc.listpeers()['peers']) == 0


if __name__ == '__main__':
unittest.main(verbosity=2)