-
Notifications
You must be signed in to change notification settings - Fork 370
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
Group channel funding transaction fields (small refactor) [splicing] #2736
Conversation
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.
Some concerns at a first glance and based on discussion in the weekly sync-up. Will defer to @wpaulino on any possible alternatives.
Thx for the comments, some reactions:
I will rework in light of the above. |
2918c4a
to
97c3969
Compare
Reworked, simplified. |
97c3969
to
c0784b6
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2736 +/- ##
==========================================
+ Coverage 88.55% 88.56% +0.01%
==========================================
Files 113 113
Lines 89330 89409 +79
Branches 89330 89409 +79
==========================================
+ Hits 79110 79189 +79
+ Misses 7849 7847 -2
- Partials 2371 2373 +2 ☔ View full report in Codecov by Sentry. |
c0784b6
to
11bbc01
Compare
you might be interested in how we did it in the VLS project - https://gitlab.com/lightning-signer/validating-lightning-signer/-/blob/2023-09-funding6-signing/vls-core/src/channel.rs?ref_type=heads#L477-481 (although you have somewhat different needs than us) |
lightning/src/ln/channel.rs
Outdated
struct TransactionConfirmation { | ||
/// The transaction, or None. | ||
transaction: Option<Transaction>, | ||
/// The hash of the block in which the transaction was included, or None. | ||
confirmed_in: Option<BlockHash>, | ||
/// The height of the block in which the transaction was included, or 0. | ||
confirmation_height: u32, | ||
} |
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.
If there are a distinct set of possibilities for how these are set and well-defined transitions, we may want to consider using an enum instead.
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.
Would make a lot of sense, but as I see currently the transitions are not so well defined... Doing this would require a much bolder change with a larger impact.
11bbc01
to
401254f
Compare
Cleaned up, adjusted description/title, marked ready. |
Do we plan to track a |
Yes, that's the idea. It is true that only one can get confirmed, but in some rare edge cases, due to reorg, a confirmed candidate can become unconfirmed, and theoretically another candidate can get confirmed. So I think it is needed to keep track of confirmation status of all candidates. |
Right, but they can't happen at the same time. We must first see the reorg before seeing the new confirmation, and by then the confirmation status should have been reset, so I don't see the benefit to tracking the confirmation status individually per transaction candidate. |
I think this small change makes sense even without considering Dual funding or Splicing. But you may be right that per-candidate confirmation tracking will not be needed. |
Closing. Now I'm less sure that this would be needed in Splicing, and outside of Splicing it brings only minor benefits. |
Groups three
Channel
fields storing funding transaction status into a separate structureTransactionConfirmation
. This is a small refactor with very minimal code logic change.Motivation:
funding_transaction
,funding_tx_confirmed_in
,funding_tx_confirmation_height
).Changes:
TransactionConfirmation
struct, with fields fortransaction
,confirmed_in
, andconfirmation_height
Note: The serialization format is untouched.
See also #2743 .