[Splicing] Preserve funding_transaction for the later lifecycle of the channel#3317
[Splicing] Preserve funding_transaction for the later lifecycle of the channel#3317optout21 wants to merge 1 commit intolightningdevkit:mainfrom
funding_transaction for the later lifecycle of the channel#3317Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3317 +/- ##
==========================================
+ Coverage 89.66% 90.88% +1.22%
==========================================
Files 126 127 +1
Lines 102676 115316 +12640
Branches 102676 115316 +12640
==========================================
+ Hits 92062 104806 +12744
+ Misses 7894 7869 -25
+ Partials 2720 2641 -79
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
2e925ee to
78f428e
Compare
78f428e to
bb043a1
Compare
bb043a1 to
020ac76
Compare
jkczyz
left a comment
There was a problem hiding this comment.
Does this mean existing channels cannot be spliced?
lightning/src/ln/channel.rs
Outdated
| { | ||
| self.context.funding_transaction.take() | ||
| let res = self.context.funding_transaction_unless_rebroadcast(); | ||
| // Note: this is legacy logic, prevents (re)broadcasting twice, unclear if needed |
There was a problem hiding this comment.
Not sure what is meant by this. Why is it legacy logic?
There was a problem hiding this comment.
What I mean by legacy:
- I don't fully understand why it works this way
- I suspect it is not by intention
- but I chose to introduce an extra field just to keep the exact behavior
- changing it would break one unit tests, and who knows what else in the field
There was a problem hiding this comment.
@TheBlueMatt Any thoughts on making funding_transaction a three-state enum instead of introducing a flag and continuing to use Option? We'd probably need the flag in the serialization logic, though.
There was a problem hiding this comment.
We could do that. That said, I do think the rebroadcasting logic is really dumb right now, so this may be an opportunity to clean it up. We could/maybe should drop all the tracking of if we broadcasted a transaction and replace it with a much simpler "if its not confirmed, broadcast it" heuristic.
There was a problem hiding this comment.
Is there any downside for occasional repeated broadcast? At most some unnecessary resource usage?
There was a problem hiding this comment.
Not hugely, no. We do get users confused when broadcasts fail all the time, so we mostly try to avoid it, but there's not a huge cost. We should definitely not rebroadcast forever though.
020ac76 to
8339270
Compare
lightning/src/ln/channel.rs
Outdated
| let cur_holder_commitment_point = Some(self.context.holder_commitment_point.current_point()); | ||
| let next_holder_commitment_point = self.context.holder_commitment_point.next_point(); | ||
|
|
||
| let funding_transaction_rebroadcast_flag = Some(self.context.funding_transaction_rebroadcast_flag); |
There was a problem hiding this comment.
Instead of wrapping this, use required below.
There was a problem hiding this comment.
It's optional due to backwards compatibility.
There was a problem hiding this comment.
It's an odd TLV so older versions will simply ignore it.
lightning/src/ln/channel.rs
Outdated
| let mut next_holder_commitment_point_opt: Option<PublicKey> = None; | ||
| let mut is_manual_broadcast = None; | ||
|
|
||
| let mut funding_transaction_rebroadcast_flag: Option<bool> = None; |
There was a problem hiding this comment.
And then use (default_value, false) below instead of option.
lightning/src/ln/channel.rs
Outdated
| { | ||
| self.context.funding_transaction.take() | ||
| let res = self.context.funding_transaction_unless_rebroadcast(); | ||
| // Note: this is legacy logic, prevents (re)broadcasting twice, unclear if needed |
There was a problem hiding this comment.
@TheBlueMatt Any thoughts on making funding_transaction a three-state enum instead of introducing a flag and continuing to use Option? We'd probably need the flag in the serialization logic, though.
|
Note: Changing this behavior causes a single test case to fail: The check that fails expects 1 broadcasted transaction, while with the change it will have the transaction broadcasted 4 times. I suspect this change does not really affect the desired behavior, this checks is simply over-specific and that can be updated, and the change is acceptable. |
8339270 to
6103652
Compare
|
Alternative simpler solution in #3380 -- never clears the |
|
Supersceded by #3380. |
Fixes #3300
In splicing, the funding transaction (not just the ID) is needed during splice negotiation.
Funding transaction is kept in field
ChannelContext.funding_transaction.However, the way it is currently used is that it is cleared when the tx is broadcast, and this fact is used in some logic.
This change:
ChannelContext.funding_transactionwhen tx is broadcast, but it's preservedfunding_transaction_broadcastbool field to indicate if it has been broadcastSo behavior of not (re)broadcasting twice is preserve (though I am not sure whether this was intentional and it is needed, one test fails if this is changed (
test_completed_payment_not_retryable_on_reload)).Alternative would be to leave
funding_transactionunchanged, and introduce a newfunding_transaction_savedfield, that is set the same way, but never cleared. This way existing logic would not be affected, but the tx would be kept in two copies.