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

Dual funding channel confirmation #2274

Merged
merged 11 commits into from
Aug 19, 2022
Merged

Dual funding channel confirmation #2274

merged 11 commits into from
Aug 19, 2022

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented May 17, 2022

Finalize the dual-funding flow: exchange signatures and wait for the funding transaction to confirm.

If an error occurs, we need to wait for the funding tx to be double-spent before we can consider the channel closed and safely forget it.

There is some logic duplication between single-funded and dual-funded handlers...I don't know how far we should go to try to remove duplication. If we extract too much, it may make it harder to just read an event handler's code and verify that every step is there and correctly done in the right order. Also, there are small subtleties where single-funded and dual-funded differ, and there will be more as we build new protocols on top of dual funding (e.g. liquidity ads). In the long term, we will eventually remove the code for the single-funded case, as it can be achieved as just a special case of dual-funding (but we'll need all peers on the network to support dual funding before we can really remove that). Let me know what you think and where you think we should refactor.

@t-bast t-bast changed the title Dual funding confirmed Dual funding channel confirmation May 17, 2022
@t-bast t-bast force-pushed the dual-funding-confirmed branch from b99e2ca to 43749f1 Compare May 17, 2022 12:50
@t-bast t-bast mentioned this pull request May 17, 2022
@t-bast t-bast requested a review from pm47 May 18, 2022 07:54
@t-bast t-bast force-pushed the dual-funding-confirmed branch 5 times, most recently from 41ba8b0 to 3cd24fc Compare July 1, 2022 15:15
@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2022

Codecov Report

Merging #2274 (f49116a) into master (33e6fac) will decrease coverage by 0.21%.
The diff coverage is 72.34%.

@@            Coverage Diff             @@
##           master    #2274      +/-   ##
==========================================
- Coverage   84.95%   84.74%   -0.22%     
==========================================
  Files         198      199       +1     
  Lines       15255    15455     +200     
  Branches      633      639       +6     
==========================================
+ Hits        12960    13097     +137     
- Misses       2295     2358      +63     
Impacted Files Coverage Δ
...n/scala/fr/acinq/eclair/balance/CheckBalance.scala 64.89% <0.00%> (-1.42%) ⬇️
...inq/eclair/channel/fsm/SingleFundingHandlers.scala 80.00% <ø> (-4.10%) ⬇️
...main/scala/fr/acinq/eclair/db/DbEventHandler.scala 93.42% <ø> (ø)
...n/scala/fr/acinq/eclair/json/JsonSerializers.scala 95.12% <ø> (ø)
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 84.80% <41.26%> (-2.67%) ⬇️
...q/eclair/channel/fsm/ChannelOpenSingleFunded.scala 93.80% <66.66%> (+0.12%) ⬆️
...acinq/eclair/channel/fsm/DualFundingHandlers.scala 62.16% <66.66%> (+5.01%) ⬆️
...inq/eclair/channel/fsm/ChannelOpenDualFunded.scala 91.15% <75.38%> (-5.91%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 95.03% <88.88%> (+0.03%) ⬆️
...la/fr/acinq/eclair/channel/fsm/ErrorHandlers.scala 81.13% <88.88%> (+0.73%) ⬆️
... and 27 more

Once we've exchanged signatures for the funding tx, we wait for it to
confirm. If an error occurs, we need to wait for the funding tx to be
double-spent before we can consider the channel closed and safely forget it.
@t-bast t-bast force-pushed the dual-funding-confirmed branch from 3cd24fc to 66a111f Compare August 12, 2022 14:41
t-bast added 2 commits August 17, 2022 18:05
As suggested by @pm47, it's actually not that hard to handle this.

Note that we don't allow mutual closing an unconfirmed channel, but that
is also the case for single-funded channels. We can improve that in the
future if necessary.
This aligns with what is done in the single-funded case and lets us detect
channel force-close while offline.
@t-bast
Copy link
Member Author

t-bast commented Aug 18, 2022

I changed the watching behavior in bd47992 to correctly detect channel force-close while we're offline. The commit looks simple, but it's actually very subtle, please review carefully! The main subtlety is that we when we detect an alternative commitment being used, we don't claim it, we instead wait for the corresponding funding tx to be confirmed before acting. If we claimed it instantly and swapped the main commitments with this alternative one, we would be screwed if it's actually the main funding tx that ends up confirming...

I cannot add yet test cases for those alternative commitments since RBF isn't implemented, but it's on my TODO-list to add to the RBF PR.

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.

Quick feedback to make sure we are on the same page with the new approach.

t-bast added 5 commits August 18, 2022 12:36
It's likely that we'll win that race since the latest funding tx pays
more fees.
Note that when re-transmitting, we had a small bug where we didn't send
our local alias.
Instead of watching funding txs being spent.
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.

I'm pretty sure this works fine but I have still doubt about the OFFLINE/SYNCING case. Let's sleep on it 🤞.

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.

Onto the next one! ⛵

@t-bast t-bast merged commit a97e88f into master Aug 19, 2022
@t-bast t-bast deleted the dual-funding-confirmed branch August 19, 2022 09:31
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