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

Ack htlc settlement commands after writing state #1615

Merged
merged 1 commit into from
Dec 1, 2020

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Nov 23, 2020

There are two issues:

  1. because we forward commands before writing to disk in
    PendingRelayDb.safeSend in order to reduce latency, there is a race
    where the channel can process and acknowledge the command before the
    db write. As a result, the command stays in the pending relay db and
    will be cleaned up by the post-restart-htlc-cleaner at next restart.
  2. in the general case, the channel acknowledges commands before it
    writes its state to disk, which opens a window for losing the command
    if we stop eclair at that exact time.

In order to fix 2., we introduce a new acking transition method, which
will be called after storing. This method adds a delay before actually
acknowledging the commands, which should be more than enough to solve 1.

I'm not sure we need that additional delay, because now that we
acknowledge the commands after storing the state, the channel should
lose the race most of the time.

There are two issues:
1. because we forward commands *before* writing to disk in
   `PendingRelayDb.safeSend` in order to reduce latency, there is a race
   where the channel can process and acknowledge the command before the
   db write. As a result, the command stays in the pending relay db and
   will be cleaned up by the post-restart-htlc-cleaner at next restart.
2. in the general case, the channel acknowledges commands *before* it
   writes its state to disk, which opens a window for losing the command
   if we stop eclair at that exact time.

In order to fix 2., we introduce a new `acking` transition method, which
will be called after `storing`. This method adds a delay before actually
acknowledging the commands, which should be more than enough to solve 1.

I'm not sure we need that additional delay, because now that we
acknowledge the commands *after* storing the state, the channel should
lose the race most of the time.
@pm47 pm47 requested a review from t-bast November 23, 2020 18:07
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM, I think the additional delay doesn't hurt, let's keep it.

@pm47 pm47 merged commit 848b433 into master Dec 1, 2020
@pm47 pm47 deleted the ack-after-writing-state branch December 1, 2020 09:53
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