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.

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

Closed
wants to merge 1 commit into from

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Nov 16, 2023

Groups three Channel fields storing funding transaction status into a separate structure TransactionConfirmation. This is a small refactor with very minimal code logic change.

Motivation:

  • In the upcoming Splicing feature, there is a need to keep info about multiple funding transactions (e.g. while the new spliced tx is being confirmed, both the new and old must be kept around). With the current structure of multiple fields this is cumbersome.
  • In Dual Funding, in case of RBF multiple funding transaction candidates have to be kept.
  • Group together the several fields that relate to the funding transaction state (funding_transaction, funding_tx_confirmed_in, funding_tx_confirmation_height).

Changes:

  • New TransactionConfirmation struct, with fields for transaction, confirmed_in, and confirmation_height
  • convenience method for computing confirmation depth, handling special cases

Note: The serialization format is untouched.
See also #2743 .

@optout21 optout21 changed the title [WIP] Refactor channel funding transaction fieldsRefactor channel funding transaction fields [WIP] Refactor channel funding transaction fields Nov 16, 2023
Copy link
Contributor

@jkczyz jkczyz left a 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.

lightning/src/ln/channel.rs Show resolved Hide resolved
lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved
@optout21
Copy link
Contributor Author

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:

  • serializaion b/w compatibility is an important point, should be done
  • ChannelFundingTransaction and ChannelFundingTransactions: The 's' here only holds an Option<> to the singular struct, with the idea that it can be easily changed to a Vec later; but on a second thought this may be a bit overengineered
  • Serialization of the additional funding tx info (if any) is also to be decided/considered.
  • An alternative is to keep one 'main'/'first' funding tx info as currently (maybe packed in one simple struct), and optionally add additional ones in a spearate Vector. This approach is more b/w compatible, not over-generalzied. Serialization can follow the same style.
  • A simple TransactionConfirmation (as suggested here) struct could be used here.
    CC: @dunxen

I will rework in light of the above.

@optout21
Copy link
Contributor Author

Reworked, simplified.

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (870a0f1) 88.55% compared to head (401254f) 88.56%.

Files Patch % Lines
lightning/src/ln/channel.rs 97.45% 0 Missing and 3 partials ⚠️

❗ 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.
📢 Have feedback on the report? Share it here.

@devrandom
Copy link
Member

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)

Comment on lines 691 to 699
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,
}
Copy link
Contributor

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.

Copy link
Contributor Author

@optout21 optout21 Nov 21, 2023

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.

@optout21 optout21 changed the title [WIP] Refactor channel funding transaction fields Refactor channel funding transaction fields Nov 21, 2023
@optout21 optout21 changed the title Refactor channel funding transaction fields Group channel funding transaction fields (small refactor) Nov 22, 2023
@optout21 optout21 changed the title Group channel funding transaction fields (small refactor) Group channel funding transaction fields (small refactor) [splicing] Nov 22, 2023
@optout21 optout21 marked this pull request as ready for review November 22, 2023 09:41
@optout21
Copy link
Contributor Author

Cleaned up, adjusted description/title, marked ready.

@wpaulino
Copy link
Contributor

Do we plan to track a TransactionConfirmation per funding transaction (this doesn't really make sense since only can transaction can be confirmed), or keep a single one and add the other funding candidates later on as part of #2743? If the latter, I don't see how this refactor helps achieve that. It does make the confirmation logic a bit nicer to follow though.

@optout21
Copy link
Contributor Author

Do we plan to track a TransactionConfirmation per funding transaction (this doesn't really make sense since only can transaction can be confirmed)

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.

@wpaulino
Copy link
Contributor

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.

@optout21
Copy link
Contributor Author

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.
I put it back to Draft for further considerations for now.

@optout21 optout21 marked this pull request as draft November 23, 2023 14:19
@optout21
Copy link
Contributor Author

optout21 commented Dec 7, 2023

Closing. Now I'm less sure that this would be needed in Splicing, and outside of Splicing it brings only minor benefits.

@optout21 optout21 closed this Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants