#3259 followups #3275
#3259 followups #3275
Conversation
5e874c3 changed `Event::DiscardFunding` to not include a dummy transaction when we were funded without a full funding tx, but in doing so started generating `DiscardFunding` events on every channel closure rather than only when there's actually still a pending funding broadcast. This restores the previous behavior to only generate the event when we should actually discard the funding tx.
TheBlueMatt
force-pushed
the
2024-08-3259-followups
branch
from
4a5739e to
3b70bd2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3275 +/- ##
==========================================
+ Coverage 89.82% 90.03% +0.20%
==========================================
Files 126 126
Lines 103024 104472 +1448
Branches 103024 104472 +1448
==========================================
+ Hits 92543 94058 +1515
+ Misses 7769 7753 -16
+ Partials 2712 2661 -51 ☔ View full report in Codecov by Sentry. |
| shutdown_res.unbroadcasted_funding_tx.map(|transaction| FundingInfo::Tx{ transaction }) | ||
| }; | ||
| if let Some(funding_info) = funding_info { | ||
| if let Some(transaction) = shutdown_res.unbroadcasted_funding_tx { |
There was a problem hiding this comment.
Would it be easy to add test coverage for the bug described in the commit message (maybe in follow-up)? Seems obvious in retrospect, ugh.
There was a problem hiding this comment.
Errr, yea, good thing I did that....the whole logic was broken.
| shutdown_res.unbroadcasted_funding_tx.map(|transaction| FundingInfo::Tx{ transaction }) | ||
| }; | ||
| if let Some(funding_info) = funding_info { | ||
| if let Some(transaction) = shutdown_res.unbroadcasted_funding_tx { |
There was a problem hiding this comment.
Pre-existing, but I think it would be useful to have some docs on unbroadcasted_funding_tx. Because I assumed it would always be None for unsafe_manual_broadcast'd channels, but that would mean we would never hit the manual broadcast clause below so that doesn't make sense..
There was a problem hiding this comment.
Ah, we added some to Channel::funding_transaction, but not the method.
There was a problem hiding this comment.
Ah okay, I see the docs now
In 5e874c3 we'd intended to not reveal the dummy funding transaction in `Event::DiscardFunding`. However, instead of looking at the channel that was just closed, the logic only looks at any other channels which were funded as a part of the same batch. Because manually-funded transactions cannot currently be done for batch funding, this was actually dead code, preventing the new changes from taking effect.
| /// Note that if [`Self::is_manual_broadcast`] is true the transaction will be a dummy | ||
| /// transaction. |
There was a problem hiding this comment.
Could we have used an enum indicating the manual case? Or does that affect the serialization logic / format?
There was a problem hiding this comment.
We could swap this, yea. We didn't bother in the initial PR cause we forgot about this issue until it was basically ready.
There was a problem hiding this comment.
Also this method is only actually used in batch funding, which never has a non-transaction, so...
| shutdown_res.unbroadcasted_funding_tx.map(|transaction| FundingInfo::Tx{ transaction }) | ||
| }; | ||
| if let Some(funding_info) = funding_info { | ||
| if let Some(transaction) = shutdown_res.unbroadcasted_funding_tx { |
There was a problem hiding this comment.
ops this is my fault, sorry that I did not thing about this!
| @@ -936,6 +936,7 @@ pub(crate) struct ShutdownResult { | |||
| pub(crate) user_channel_id: u128, | |||
| pub(crate) channel_capacity_satoshis: u64, | |||
| pub(crate) counterparty_node_id: PublicKey, | |||
| pub(crate) is_manual_broadcast: bool, | |||
There was a problem hiding this comment.
I though of doing it but I choose the monkey way of doing it by iterating over all the channels, thanks for catching it
No description provided.