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.

Sign up for GitHub

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

Jump to bottom

Implement the interactive-tx protocol #2273

Merged
merged 12 commits into from
Aug 12, 2022
Merged

Implement the interactive-tx protocol #2273

merged 12 commits into from
Aug 12, 2022

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented May 17, 2022 •
edited
Loading

After exchanging open_channel2 and accept_channel2, we start building the funding transaction with the interactive-tx protocol.

We stop once we've generated our signatures for the funding transaction, at which point we should store the channel in the DB (which will be done in future commits).

@t-bast t-bast force-pushed the dual-funding-created branch from b2cf365 to ab70d6f Compare May 17, 2022 09:20
@t-bast t-bast mentioned this pull request May 17, 2022
Merged
@t-bast t-bast requested a review from pm47 May 18, 2022 07:54
@t-bast t-bast force-pushed the dual-funding-created branch from ab70d6f to 8056fa9 Compare May 19, 2022 10:04
@t-bast t-bast force-pushed the dual-funding-created branch 4 times, most recently from f63bef6 to 7b43477 Compare July 1, 2022 14:50
@t-bast
Implement the interactive-tx protocol
ba5a730 
After exchanging `open_channel2` and `accept_channel2`, we start building
the funding transaction.

We stop once we've generated our signatures for the funding transaction,
at which point we should store the channel in the DB (which will be done in
future commits).
@t-bast t-bast force-pushed the dual-funding-created branch from 7b43477 to ba5a730 Compare August 9, 2022 15:18
pm47
pm47 reviewed Aug 10, 2022
View reviewed changes
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.

First pass.

t-bast added 5 commits August 11, 2022 09:59
@t-bast
Always wait a few blocks before confirmation
4b71d40 
Otherwise there is a risk of losing state in reorgs.
@t-bast
Add UnusableInput type
83e51e4 
For inputs that can't be used in the interactive-tx construction.
@t-bast
Fix comments from @pm47's first pass
e354a68
@t-bast
Rename previousAttempts and prevent infinite loop
6e0c15d 
We explicitly handle the case where bitcoind incorrectly reuses unusable
inputs that we locked, otherwise we could go into an infinite funding loop.
@t-bast
Log number of inputs / outputs
180c23f
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2022 •
edited
Loading

Codecov Report

Merging #2273 (22a21a2) into master (0310bf5) will increase coverage by 0.00%.
The diff coverage is 85.74%.

@@           Coverage Diff            @@
##           master    #2273    +/-   ##
========================================
  Coverage   84.84%   84.85%            
========================================
  Files         195      198     +3     
  Lines       14850    15255   +405     
  Branches      609      633    +24     
========================================
+ Hits        12600    12945   +345     
- Misses       2250     2310    +60
Impacted Files Coverage Δ
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 87.47% <ø> (-0.12%) ⬇️
...inq/eclair/channel/fsm/SingleFundingHandlers.scala 84.09% <42.85%> (ø)
...acinq/eclair/channel/fsm/DualFundingHandlers.scala 57.14% <57.14%> (ø)
...q/eclair/wire/protocol/LightningMessageTypes.scala 94.11% <62.50%> (-3.67%) ⬇️
...fr/acinq/eclair/channel/InteractiveTxBuilder.scala 85.54% <85.54%> (ø)
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 94.81% <90.90%> (-0.36%) ⬇️
...inq/eclair/channel/fsm/ChannelOpenDualFunded.scala 97.05% <94.82%> (+5.67%) ⬆️
...ala/fr/acinq/eclair/blockchain/OnChainWallet.scala 100.00% <100.00%> (ø)
...ir/blockchain/bitcoind/rpc/BitcoinCoreClient.scala 96.36% <100.00%> (+0.06%) ⬆️
...in/scala/fr/acinq/eclair/channel/ChannelData.scala 100.00% <100.00%> (ø)
... and 17 more

pm47
pm47 reviewed Aug 11, 2022
View reviewed changes
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.

The send/receive ping-pong is really neat. Yay simple protocols!

t-bast added 5 commits August 12, 2022 11:40
@t-bast
Fix most comments from @pm47's second pass
b3ab1d3 
Except:

- use stash instead of message timer delays
- investigate non-initiator fees tweaking

Those two deserve their own commit.
@t-bast
Discount fees for non-initiator
370822c 
When we have change output, it's easy to discount the fees for the common
fields.
@t-bast
fixup! Discount fees for non-initiator
ffe5708
@t-bast
Use stash instead of delayed messages
13a3d64
@t-bast
Handle unexpected bitcoind funded tx
d5c640b 
Explicitly handle cases where bitcoind returns an invalid funded transaction.
pm47
pm47 reviewed Aug 12, 2022
View reviewed changes
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.

InteractiveTxBuilderSpec is really nice, just one nit and we are good to go.

@t-bast t-bast merged commit 33e6fac into master Aug 12, 2022
@t-bast t-bast deleted the dual-funding-created branch August 12, 2022 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sstone sstone sstone left review comments

@pm47 pm47 pm47 approved these changes

Assignees
No one assigned
Labels
Optech Make Me Famous!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@t-bast @codecov-commenter @sstone @pm47