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

Extract tx publishing from watchers #1749

Merged
merged 12 commits into from
Apr 12, 2021
Merged

Extract tx publishing from watchers #1749

merged 12 commits into from
Apr 12, 2021

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Mar 31, 2021 •
edited
Loading

Introduce a TxPublisher actor whose responsibility is to publish channel transactions (from funding to closing).
Move logic from ZmqWatcher to this new actor, with minimal functional changes.

This is best reviewed commit-by-commit:

  • The first commit moves code from watcher to the tx-publisher (almost no functional change)
  • The second commit slightly refactors tx publishing and uses Commitments instead of TxSigningKit
  • The third commit adds a few tests around cltv and csv delays

In future PRs, we will refactor and enrich this TxPublisher.

@t-bast t-bast force-pushed the tx-publisher branch 2 times, most recently from 91ae610 to 6cc2fab Compare April 1, 2021 07:32
@t-bast
Extract tx publishing from ZmqWatcher
7050351 
Introduce a TxPublisher actor to publish channel txs.
Move logic from watcher to this new actor, with minimal functional changes.
@t-bast
Use commitments instead of signing kits
05111f8 
The TxSigningKit abstraction was introduced a bit too early.
The TxPublisher will hold all the logic so we'll start by providing the full
commitments, and we'll extract more compact objects later.

We also now publish the commit tx and its anchor tx independently.
@t-bast
Add more CLTV and CSV tests
d035b56
@t-bast
Fix channel tests
bdb68b5
@t-bast t-bast marked this pull request as ready for review April 6, 2021 12:36
@t-bast t-bast requested a review from pm47 April 6, 2021 12:37
pm47
pm47 reviewed Apr 6, 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.

I don't really like 05111f8, but I see it's meant as an intermediate step.

What do you mean by?

We also now publish the commit tx and its anchor tx independently.

eclair-core/src/main/scala/fr/acinq/eclair/channel/TxPublisher.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/TxPublisher.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/TxPublisher.scala Outdated Show resolved Hide resolved
@t-bast
Copy link
Member Author

t-bast commented Apr 6, 2021

What do you mean by?

We also now publish the commit tx and its anchor tx independently.

What we previously had was a bit ugly: we sent one PublishAsap for the commit tx which in fact could end up publishing either only the commit tx or the commit tx and its anchor child, and it wasn't very intuitive what transaction the txInfo field referred to.

Now it is cleaner, we separate these into two messages.
We first call PublishRawTx with the commit tx, and then SignAndPublishTx with the anchor tx (where we check whether we really need to publish this or not depending on the current on-chain feerate).

@t-bast t-bast mentioned this pull request Apr 9, 2021
Merged
@t-bast
Copy link
Member Author

t-bast commented Apr 12, 2021

I added the desc field to TransactionWithInputInfo in ee40547

This is something we had already discussed during #1728, I think it's worth doing (and the new HtlcDelayedTx type removes the ambiguity between the main output and 3rd-stage htlc outputs).

We still need to repeat these desc in generateTx in Helpers.scala because we want to handle TxGenerationSkipped cases. I think it's ok. I toyed with the idea of putting the desc field in companion objects of each TransactionWithInputInfo to have a static value, but it isn't very elegant and produces more code, so I kept the small string duplication instead.

Let me know what you think @pm47, I think it will make the logs much easier to analyze.

@pm47
Copy link
Member

pm47 commented Apr 12, 2021

Let me know what you think @pm47, I think it will make the logs much easier to analyze.

Most definitely. I submitted two additions on branch tx-publisher-pm. The duplication in Helpers.scala is still itching though!

@t-bast
Copy link
Member Author

t-bast commented Apr 12, 2021 •
edited
Loading

Most definitely. I submitted two additions on branch tx-publisher-pm. The duplication in Helpers.scala is still itching though!

I applied the first one, but I find the second one over-engineered to be honest ("parfois, le mieux est l'ennemi du bien !").
I'd rather keep that duplication for now.

In order to automatically to CPFP on funding txs, we will re-work part of the PublishRawTx, and we'll have an opportunity to improve it then depending on what data changes are required to make that CPFP work.

@pm47
Copy link
Member

pm47 commented Apr 12, 2021

I knew you'd say that!

@t-bast t-bast merged commit 48c0c4c into master Apr 12, 2021
@t-bast t-bast deleted the tx-publisher branch April 12, 2021 15:17
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.
2 participants
@t-bast @pm47