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.

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

channeld: Fix the shutdown_sent billboard direction #4263

Merged
merged 1 commit into from
Dec 8, 2020

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Dec 6, 2020

While debugging a hanging channel with a user I noticed that they
called close on a channel, resulting in the channel showing
CHANNELD_SHUTTING_DOWN, but the billboard seemed to show the
information the wrong way around:

{
   "peers": [
      {
         "connected": true,
	 // ...
         "channels": [
            {
               "state": "CHANNELD_SHUTTING_DOWN",
	       // ...
               "status": [
                  "CHANNELD_SHUTTING_DOWN:Reconnected, and reestablished.",
                  "CHANNELD_SHUTTING_DOWN:Funding transaction locked. They need our announcement signatures. They've sent shutdown, waiting for ours"
               ],
	       // ...
            }
         ]
      }
   ]
}

Aside from the hung channel, the switch in direction of the status
seemed weird. Checking the billboard code seems to have the status
switched as well:

else if (!peer->shutdown_sent[LOCAL] && peer->shutdown_sent[REMOTE])
shutdown_status = " We've send shutdown, waiting for theirs";
else if (peer->shutdown_sent[LOCAL] && !peer->shutdown_sent[REMOTE])
shutdown_status = " They've sent shutdown, waiting for ours";

We set shutdown_sent[LOCAL] when we send the shutdown:

static void maybe_send_shutdown(struct peer *peer)
{
u8 *msg;
if (!peer->send_shutdown)
return;
/* Send a disable channel_update so others don't try to route
* over us */
send_channel_update(peer, ROUTING_FLAGS_DISABLED);
msg = towire_shutdown(NULL, &peer->channel_id, peer->final_scriptpubkey);
sync_crypto_write(peer->pps, take(msg));
peer->send_shutdown = false;
peer->shutdown_sent[LOCAL] = true;
billboard_update(peer);
}

And we set shutdown_sent[REMOTE] when we receive the shutdown:

lightning/channeld/channeld.c

Lines 1730 to 1781 in ff88308

static void handle_peer_shutdown(struct peer *peer, const u8 *shutdown)
{
struct channel_id channel_id;
u8 *scriptpubkey;
/* Disable the channel. */
send_channel_update(peer, ROUTING_FLAGS_DISABLED);
if (!fromwire_shutdown(tmpctx, shutdown, &channel_id, &scriptpubkey))
peer_failed(peer->pps,
&peer->channel_id,
"Bad shutdown %s", tal_hex(peer, shutdown));
/* BOLT #2:
*
* - if both nodes advertised the `option_upfront_shutdown_script`
* feature, and the receiving node received a non-zero-length
* `shutdown_scriptpubkey` in `open_channel` or `accept_channel`, and
* that `shutdown_scriptpubkey` is not equal to `scriptpubkey`:
* - MUST fail the connection.
*/
/* openingd only sets this if feature was negotiated at opening. */
if (tal_count(peer->remote_upfront_shutdown_script)
&& !memeq(scriptpubkey, tal_count(scriptpubkey),
peer->remote_upfront_shutdown_script,
tal_count(peer->remote_upfront_shutdown_script)))
peer_failed(peer->pps,
&peer->channel_id,
"scriptpubkey %s is not as agreed upfront (%s)",
tal_hex(peer, scriptpubkey),
tal_hex(peer, peer->remote_upfront_shutdown_script));
/* Tell master: we don't have to wait because on reconnect other end
* will re-send anyway. */
wire_sync_write(MASTER_FD,
take(towire_channeld_got_shutdown(NULL, scriptpubkey)));
peer->shutdown_sent[REMOTE] = true;
/* BOLT #2:
*
* A receiving node:
* ...
* - once there are no outstanding updates on the peer, UNLESS
* it has already sent a `shutdown`:
* - MUST reply to a `shutdown` message with a `shutdown`
*/
if (!peer->shutdown_sent[LOCAL]) {
peer->send_shutdown = true;
start_commit_timer(peer);
}
billboard_update(peer);
}

So I think the billboard code just needs to be switched around.

Changelog-Fixed: JSON-RPC: The status of the shutdown meesages being exchanged is now displayed correctly.

While debugging a hanging channel with a user I noticed that they
called `close` on a channel, resulting in the channel showing
`CHANNELD_SHUTTING_DOWN`, but the billboard seemed to show the
information the wrong way around:

```json
{
   "peers": [
      {
         "connected": true,
	 // ...
         "channels": [
            {
               "state": "CHANNELD_SHUTTING_DOWN",
	       // ...
               "status": [
                  "CHANNELD_SHUTTING_DOWN:Reconnected, and reestablished.",
                  "CHANNELD_SHUTTING_DOWN:Funding transaction locked. They need our announcement signatures. They've sent shutdown, waiting for ours"
               ],
	       // ...
            }
         ]
      }
   ]
}
```

Aside from the hung channel, the switch in direction of the status
seemed weird. Checking the billboard code seems to have the status
switched as well:

https://github.com/ElementsProject/lightning/blob/ff8830876dc2ead010dc4a6453abb7e30ec637ce/channeld/channeld.c#L223-L226

We set `shutdown_sent[LOCAL]` when we send the shutdown:

https://github.com/ElementsProject/lightning/blob/ff8830876dc2ead010dc4a6453abb7e30ec637ce/channeld/channeld.c#L823-L839

And we set `shutdown_sent[REMOTE]` when we receive the shutdown:

https://github.com/ElementsProject/lightning/blob/ff8830876dc2ead010dc4a6453abb7e30ec637ce/channeld/channeld.c#L1730-L1781

So I think the billboard code just needs to be switched around.

Changelog-Fixed: JSON-RPC: The status of the shutdown meesages being exchanged is now displayed correctly.
@cdecker cdecker added this to the v0.9.3 milestone Dec 6, 2020
@cdecker cdecker self-assigned this Dec 6, 2020
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack 9c355bc

@rustyrussell rustyrussell merged commit c656cfe into ElementsProject:master Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants