Skip to content

Commit

Permalink
channeld: give some tolerance for empty commitments.
Browse files Browse the repository at this point in the history
The spec says not to send a commitment_signed without any changes, but LND
does this.  To understand why, you have to understand how LND works.  I
haven't read the code, but I'm pretty sure it works like this:

1. lnd slows down to do garbage collection, because it's in Go.
2. When an alert timer goes off, noticing it's not making process, it
   sends a twitter message to @Roasbeef.
3. @Roasbeef sshs into the user's machine and binary patches lnd to send
   a commitment_signed message.
4. Unfortunately he works so fast that various laws of causality are broken,
   meaning sometimes the commitment_signed is sent before any of thes
   other things happen.

I'm fairly sure that this will stop as @Roasbeef ages, or lnd introduces
some kind of causality enforcement fix.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
  • Loading branch information
rustyrussell committed Apr 16, 2019
1 parent 77236ca commit ef5fdf8
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 4 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ changes.
- `--bind-addr=<path>` fixed for nodes using local sockets (eg. testing).
- Unannounced local channels were forgotten for routing on restart until reconnection occurred.
- lightning-cli: arguments containing `"` now succeed, rather than causing JSON errors.
- protocol: handle lnd sending more messages before `reestablish`; don't fail channel.
- protocol: handle lnd sending more messages before `reestablish`; don't fail channel, and handle older lnd's spurious empty commitments.

### Security

Expand Down
14 changes: 11 additions & 3 deletions channeld/channeld.c
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ struct peer {

/* Additional confirmations need for local lockin. */
u32 depth_togo;

/* Empty commitments. Spec violation, but a minor one. */
u64 last_empty_commitment;
};

static u8 *create_channel_announcement(const tal_t *ctx, struct peer *peer);
Expand Down Expand Up @@ -1344,9 +1347,13 @@ static void handle_peer_commit_sig(struct peer *peer, const u8 *msg)
* - MUST NOT send a `commitment_signed` message that does not
* include any updates.
*/
peer_failed(&peer->cs,
&peer->channel_id,
"commit_sig with no changes");
status_trace("Oh hi LND! Empty commitment at #%"PRIu64,
peer->next_index[LOCAL]);
if (peer->last_empty_commitment == peer->next_index[LOCAL] - 1)
peer_failed(&peer->cs,
&peer->channel_id,
"commit_sig with no changes (again!)");
peer->last_empty_commitment = peer->next_index[LOCAL];
}

/* We were supposed to check this was affordable as we go. */
Expand Down Expand Up @@ -2967,6 +2974,7 @@ int main(int argc, char *argv[])
peer->last_update_timestamp = 0;
/* We actually received it in the previous daemon, but near enough */
peer->last_recv = time_now();
peer->last_empty_commitment = 0;

/* We send these to HSM to get real signatures; don't have valgrind
* complain. */
Expand Down

1 comment on commit ef5fdf8

@Roasbeef
Copy link

@Roasbeef Roasbeef commented on ef5fdf8 Apr 16, 2019 via email

Choose a reason for hiding this comment

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

Please sign in to comment.