-
Notifications
You must be signed in to change notification settings - Fork 268
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
Conversation
b99e2ca
to
43749f1
Compare
41ba8b0
to
3cd24fc
Compare
Codecov Report
@@ 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
|
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.
3cd24fc
to
66a111f
Compare
eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelData.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ErrorHandlers.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/wire/protocol/CommonCodecs.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/DualFundingHandlers.scala
Show resolved
Hide resolved
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.
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 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. |
There was a problem hiding this 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.
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ErrorHandlers.scala
Outdated
Show resolved
Hide resolved
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.
There was a problem hiding this 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 🤞.
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/DualFundingHandlers.scala
Show resolved
Hide resolved
And a few nits.
There was a problem hiding this 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! ⛵
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.