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

Rework TxPublisher #1844

Merged
merged 6 commits into from
Jun 22, 2021
Merged

Rework TxPublisher #1844

merged 6 commits into from
Jun 22, 2021

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Jun 18, 2021 •
edited
Loading

This PR is an alternative approach to #1827

It contains more architectural changes (and more code to review) but is a cleaner design and will make automatic RBF much simpler to add.

The high-level architecture is the following:

+---------+
| Channel |---+
+---------+   |             +--------------------+
              | PublishTx   |    TxPublisher     |
              +------------>| - stores txs and   |---+                      +-----------------+
                            |   deadlines        |   | create child actor   |    TxPublish    |
                            | - create child     |   | ask it to publish    | - preconditions |
                            |   actors that fund |   | at a given feerate   | - (funding)     |
                            |   and publish txs  |   +--------------------->| - (signing)     |
                            +--------------------+                          | - publishing    |
                                      ^                                     | - waiting       |
                                      |                                     | - stopping      |
                                      |                                     +-----------------+
                                      |                                              |
                                      +----------------------------------------------+
                                        result:
                                         - transaction confirmed
                                         - transaction rejected

There is one TxPublisher per channel. The channel tells it to publish transactions (and in the future will give it deadline instructions). The TxPublisher creates child actors (RawTxPublisher / ReplaceableTxPublisher) for each transaction publishing attempts and waits for them to report the result and schedule a retry if necessary.

A RawTxPublisher simply publishes a complete transaction without modifying it.
A ReplaceableTxPublisher will add inputs to match the desired feerate and publish the resulting transaction.
Internally, they use a TxTimeLockMonitor that checks CLTV and CSV delays, and a MempoolTxMonitor that regularly checks if its transaction confirms or is rejected from the mempool.

NB: it's worth turning on debug logs for the fr.acinq.eclair.channel.publish package to troubleshoot potential issues with this new publishing mechanism.

@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2021 •
edited
Loading

Codecov Report

Merging #1844 (2ca2811) into master (afb1b41) will decrease coverage by 0.13%.
The diff coverage is 88.27%.

@@            Coverage Diff             @@
##           master    #1844      +/-   ##
==========================================
- Coverage   89.09%   88.95%   -0.14%     
==========================================
  Files         146      150       +4     
  Lines       10994    11219     +225     
  Branches      444      472      +28     
==========================================
+ Hits         9795     9980     +185     
- Misses       1199     1239      +40
Impacted Files Coverage Δ
...n/scala/fr/acinq/eclair/channel/ChannelTypes.scala 97.14% <ø> (ø)
...cinq/eclair/channel/publish/MempoolTxMonitor.scala 80.51% <80.51%> (ø)
.../acinq/eclair/channel/publish/RawTxPublisher.scala 82.50% <82.50%> (ø)
...clair/channel/publish/ReplaceableTxPublisher.scala 85.79% <85.79%> (ø)
...nq/eclair/channel/publish/TxTimeLocksMonitor.scala 95.23% <95.23%> (ø)
.../fr/acinq/eclair/channel/publish/TxPublisher.scala 98.41% <98.41%> (ø)
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 92.77% <100.00%> (+0.04%) ⬆️
...eclair/blockchain/bitcoind/BitcoinCoreWallet.scala 89.70% <100.00%> (-0.71%) ⬇️
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala 96.87% <100.00%> (ø)
...lockchain/bitcoind/rpc/ExtendedBitcoinClient.scala 93.93% <100.00%> (-2.49%) ⬇️
... and 11 more

@t-bast t-bast marked this pull request as ready for review June 18, 2021 15:34
@t-bast t-bast requested a review from pm47 June 21, 2021 07:08
pm47
pm47 reviewed Jun 21, 2021
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.

Part of what makes this PR interesting is that the seemingly simple task of funding and publishing transactions is much -much- more complicated than it appears. That's why I believe this architecture is right even if it looks over-engineered at first glance (especially the *Monitor actors).

The implementation of some of the actors is pretty complex (looking at ReplaceableTxPublisher), but I don't see a way around that, and we still manage to have a well defined, simple interface with those actors.

I'm going to do a review-by-testing now.

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/TxPublisher.scala Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/TxPublisher.scala Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/TxPublisher.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/TxPublisher.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/RawTxPublisher.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/MempoolTxMonitor.scala Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/MempoolTxMonitor.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/MempoolTxMonitor.scala Outdated Show resolved Hide resolved
@t-bast
Rework TxPublisher
a8af19b 
Splt the TxPublisher in many smaller actors with clear responsibilities.
Each tx publishing attempt is its own actor and watches the tx until it
either confirms or becomes evicted, and reports the result to its parent.

The TxPublisher (one per channel) orchestrates publishing attempts and
will in the future decide to RBF txs based on deadline information.
@t-bast
Fix discrepancy in sealed trait style
43c4482
@t-bast
Clean up attempt pattern matching
df62f7a
@t-bast
Rename log context classes
fb1dd6f
@t-bast
PublishRawTx only needs parent txid
eb4ddfc
@t-bast
Fix InputStatus
2ca2811 
SpentConfirmed and SpentUnconfirmed should be mutually exclusive.
@t-bast t-bast mentioned this pull request Jun 22, 2021
Closed
@pm47
Copy link
Member

pm47 commented Jun 22, 2021

🎉

@t-bast t-bast merged commit d43d06f into master Jun 22, 2021
@t-bast t-bast deleted the rework-tx-publisher branch June 22, 2021 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pm47 pm47 pm47 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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