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

Abort HTLC tx publisher if remote commit confirms #2080

Merged
merged 1 commit into from
Dec 2, 2021

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Dec 1, 2021

If the remote commit confirms before our local commit, there is no reason to try to publish our HTLC transactions, we will instead directly claim the htlc outputs from the remote commit.

We previously checked timelocks before checking preconditions, which in this case means we would be waiting for a confirmation on our local commit forever. We now check preconditions before timelocks, and added a precondition that verifies that the remote commit isn't confirmed before publishing our HTLC txs.

It wasn't a big issue, we just had a watch that would linger forever and a few idle actors (until the channel closes and the channel actors stops, which stops all its publisher children), but we can easily avoid it without making the code more complex.

If the remote commit confirms before our local commit, there is no reason
to try to publish our HTLC transactions, we will instead directly claim
the htlc outputs from the remote commit.

We previously checked timelocks before checking preconditions, which in
this case means we would be waiting for a confirmation on our local commit
forever. We now check preconditions before timelocks, and added a
precondition that verifies that the remote commit isn't confirmed before
publishing our HTLC txs.
@t-bast t-bast requested a review from pm47 December 1, 2021 15:32
@codecov-commenter
Copy link

Codecov Report

Merging #2080 (905f01e) into master (9792c72) will decrease coverage by 0.01%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master    #2080      +/-   ##
==========================================
- Coverage   87.58%   87.56%   -0.02%     
==========================================
  Files         166      166              
  Lines       12717    12725       +8     
  Branches      536      556      +20     
==========================================
+ Hits        11138    11143       +5     
- Misses       1579     1582       +3     
Impacted Files Coverage Δ
...clair/channel/publish/ReplaceableTxPublisher.scala 86.81% <78.57%> (+0.60%) ⬆️
...main/scala/fr/acinq/eclair/router/Validation.scala 90.76% <0.00%> (-1.54%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 87.78% <0.00%> (-0.08%) ⬇️
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 97.56% <0.00%> (+1.62%) ⬆️

Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

LGTM

@t-bast t-bast merged commit 4ad502c into master Dec 2, 2021
@t-bast t-bast deleted the htlc-tx-publisher-remote-commit branch December 2, 2021 13:10
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.

3 participants