Uh oh!
There was an error while loading. Please reload this page.
Track funding tx channelmonitor - #4109
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## main #4109 +/- ##
========================================
Coverage 88.82% 88.83% ========================================
Files 180 180 Lines 137278 137511 +233 Branches 137278 137511 +233 ========================================
+ Hits 121944 122163 +219 - Misses 12522 12535 +13 - Partials 2812 2813 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// `true` when absent during upgrade so holder broadcasts aren't gated unexpectedly. | ||
| funding_seen_onchain: bool, | ||
| /// Tracks whether manual-broadcasting was requested before the funding transaction appeared on-chain. | ||
| manual_broadcast_pending: bool, |
There was a problem hiding this comment.
original comment from @TheBlueMatt#3838 (comment)
Hmm, I feel like we can just use holder_tx_signed rather than adding a new bool.
There was a problem hiding this comment.
@TheBlueMatt how do I prevent queue_latest_holder_commitment_txn_for_broadcast from being called multiple times? holder_tx_signed is never set to false, so not sure how could I prevent it
There was a problem hiding this comment.
ISTM we could call queue_latest_holder_commitment_txn_for_broadcast when we set funding_seen_onchain if holder_tx_signed is true? That should make it called only once cause we can use funding_seen_onchain indirectly to control it.
ldk-reviews-bot
commented
Sep 25, 2025
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
236e01f to
6d4f901CompareUh oh!
There was an error while loading. Please reload this page.
tnull
left a comment
There was a problem hiding this comment.
Did a first pass, changes look good so far I think.
Mind cleaning up the commit history to a) include a proper title/description for each feature commit b) maybe move the 'track funding transaction' and 'account for manual broadcast' parts into two different feature commits.
| pending_funding: Vec<FundingScope>, | ||
| /// True if this channel was configured for manual funding broadcasts. Monitors written by | ||
| /// versions prior to introducing the flag will load with `false` until a new update persists it. |
There was a problem hiding this comment.
Please be more specific on the version here and below, as it also gives us a hint when we can assume the new behavior and delete the comment/potentially old code.
| /// versions prior to introducing the flag will load with `false` until a new update persists it. | |
| /// versions prior to LDK 0.2 will load with `false` until a new update persists it. |
| (32, pending_funding, optional_vec), | ||
| (33, htlcs_resolved_to_user, option), | ||
| (34, alternative_funding_confirmed, option), | ||
| (35, is_manual_broadcast, option), |
There was a problem hiding this comment.
Those could be simplified by reading with (default_value, X).
| /// transactions that cannot be confirmed until the funding transaction is visible. | ||
| /// | ||
| /// [`Event::BumpTransaction`]: crate::events::Event::BumpTransaction | ||
| #[rustfmt::skip] |
There was a problem hiding this comment.
Mind dropping this rustfmt::skip in a separate (prefactor) commit while we're here?
| if self.is_manual_broadcast && !funding_seen_before && self.funding_seen_onchain && self.holder_tx_signed | ||
| { | ||
| self.queue_latest_holder_commitment_txn_for_broadcast( |
There was a problem hiding this comment.
I'm a bit confused - why do we queue the broadcast here immediately? Should we only do this if should_broadcast_commitment?
There was a problem hiding this comment.
good catch. this was a silly mistake. I'm working on a fix but can't find the right answer. if I do
if self.is_manual_broadcast && !funding_seen_before && self.funding_seen_onchain && self.holder_tx_signed
{
should_broadcast_commitment = true;
}
and not return immediately then it enqueues txs twice so tests fail. I'm working on it
| // the funding transaction on-chain, do not queue any transactions. | ||
| if require_funding_seen && self.is_manual_broadcast && !self.funding_seen_onchain { | ||
| log_info!(logger, "Not broadcasting holder commitment for manual-broadcast channel before funding appears on-chain"); | ||
| return; |
There was a problem hiding this comment.
I think we want to set holder_tx_signed and push the monitor event here. Basically, we should always call generate_claimable_outpoints_and_watch_outputs but we can skip actually passing them to the onchain_tx_handler, similar to what you did elsewhere.
a257c8e to
61d9c28CompareTheBlueMatt
commented
Sep 26, 2025
Feel free to squash, I think this is mostly there. Looks like CI is failing and can you write a commit message for the commit? |
martinsaposnic
commented
Sep 26, 2025
sorry, what do you mean by this? @TheBlueMatt |
TheBlueMatt
commented
Sep 26, 2025
Err, sorry, write commit messages for all the commits. Your commits currently only have titles (and many of them are too long). The subject line should be no longer than ~70 chars, followed by description of why and what was done, as well as anything that might be surprising to someone reading the commit in 5 years. See https://cbea.ms/git-commit/ for more info. |
martinsaposnic
commented
Sep 30, 2025
@TheBlueMatt@tnull what do you think about the last fixup? I'm really not sure about the solution here, and I want your ack before I squash&rebase&rewrite the commit message |
| watch_outputs.append(&mut outputs); | ||
| // Only generate claims immediately if block_confirmed | ||
| // won't also generate them to avoid duplicate registrations. | ||
| let should_broadcast = self.should_broadcast_holder_commitment_txn(logger); |
There was a problem hiding this comment.
Hmmm, yea, it does seem a bit strange to check if block_confirmedwill do something and disable an important step if it will. But then if block_confirmed changes this code will be automatically broken without touching this code. Rather, ISTM block_confirmed needs to check if we already broadcasted before broadcasting.
3db3230 to
9bd2cccCompare
TheBlueMatt
left a comment
There was a problem hiding this comment.
The new fixup looks good. Please feel free to squash fixups and clean up the git commit messages/history.
| claimable_outpoints.append(&mut new_outpoints); | ||
| watch_outputs.append(&mut new_outputs); | ||
| // Only generate claims if we haven't already done so (e.g., in transactions_confirmed). | ||
| if claimable_outpoints.is_empty() && watch_outputs.is_empty() { |
There was a problem hiding this comment.
nit maybe just the first.
| if claimable_outpoints.is_empty()&& watch_outputs.is_empty(){ | |
| if claimable_outpoints.is_empty(){ |
860ba2d to
9890bf9Comparemartinsaposnic
commented
Oct 6, 2025
ok, all squashed and history cleaned up with improved messages one last small fixup to review @TheBlueMatt |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Feel free to squash, the changes themselves LGTM.
| } | ||
| #[test] | ||
| fn test_manual_broadcast_skips_commitment_until_funding_seen() { |
There was a problem hiding this comment.
FWIW it might have been easier to write these as functional tests, which I generally personally prefer as they demonstrate the overall behavior rather than specific unit tests. It doesn't matter too much tho.
There was a problem hiding this comment.
+1, doing so would avoid a lot of the boilerplate here
9890bf9 to
400730fComparemartinsaposnic
commented
Oct 7, 2025
94ccd18 to
d88d594Compareldk-reviews-bot
commented
Oct 23, 2025
🔔 1st Reminder Hey @tnull@TheBlueMatt@wpaulino! This PR has been waiting for your review. |
2 similar comments
ldk-reviews-bot
commented
Oct 25, 2025
🔔 1st Reminder Hey @tnull@TheBlueMatt@wpaulino! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Oct 25, 2025
🔔 1st Reminder Hey @tnull@TheBlueMatt@wpaulino! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Oct 25, 2025
🔔 2nd Reminder Hey @tnull@TheBlueMatt@wpaulino! This PR has been waiting for your review. |
2 similar comments
ldk-reviews-bot
commented
Oct 27, 2025
🔔 2nd Reminder Hey @tnull@TheBlueMatt@wpaulino! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Oct 27, 2025
🔔 2nd Reminder Hey @tnull@TheBlueMatt@wpaulino! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Oct 27, 2025
🔔 3rd Reminder Hey @tnull@TheBlueMatt@wpaulino! This PR has been waiting for your review. |
tnull
left a comment
There was a problem hiding this comment.
Fixups look reasonable to me, feel free to squash from my side.
Adds `is_manual_broadcast` and `funding_seen_onchain` flags to track whether the channel uses manual funding broadcasts and whether we've seen the funding tx confirm. This enables deferring holder commitment broadcasts until after the funding tx is actually broadcast. For example, in LSPS2 with client_trusts_lsp=true, the LSP may defer broadcasting the funding tx until the client claims an HTLC, so we need to avoid broadcasting commitments that reference outputs that don't exist yet.
Marks funding_seen_onchain when we see the funding tx confirm.
Don't queue holder commitment broadcasts until funding is confirmed, unless explicitly overridden via broadcast_latest_holder_commitment_txn. Attempting to broadcast commitments before funding confirms would fail mempool validation since the funding output doesn't exist yet.
For manually-broadcast funding, we can't track claimable outputs until the funding tx is actually onchain. Otherwise we'd try to claim outputs that don't exist yet.
Sets should_broadcast_commitment=true when funding confirms. Since we skip the initial broadcast when funding_seen_onchain is false, we need to queue it once funding actually hits the chain.
Tests that holder commitment broadcasts are properly deferred until funding confirms, and that the full manual-funding flow works correctly.
5b85deb to
ea95a15Comparemartinsaposnic
commented
Oct 28, 2025
fixups squashed, this should be good now @tnull@TheBlueMatt@wpaulino |
TheBlueMatt
left a comment
There was a problem hiding this comment.
A few tweaks to the tests might be worth doing, but all in followups, I think the logic is right so would be nice to just land this and get it over the line.
| /// True if this channel was configured for manual funding broadcasts. Monitors written by | ||
| /// versions prior to LDK 0.2 load with `false` until a new update persists it. | ||
| is_manual_broadcast: bool, | ||
| /// True once we've observed either funding transaction on-chain. Older monitors prior to LDK 0.2 |
There was a problem hiding this comment.
nit: there are not two funding txn, so this is worded a bit confusingly. We could say "we've observed a funding transaction" if we're referring to dual-funding RBF or "we've observed the funding transaction" otherwise.
| /// transactions thereof. | ||
| #[rustfmt::skip] | ||
| fn filter_block<'a>(&self, txdata: &TransactionData<'a>) -> Vec<&'a Transaction> { | ||
| fn filter_block<'a>(&mut self, txdata: &TransactionData<'a>) -> Vec<&'a Transaction> { |
There was a problem hiding this comment.
nit: this hunk can be dropped now, I think.
| nodes[0].node.get_and_clear_pending_msg_events(); | ||
| nodes[1].node.get_and_clear_pending_msg_events(); | ||
| let events = nodes[0].node.get_and_clear_pending_events(); | ||
| assert_eq!(events.len(), 1); |
There was a problem hiding this comment.
Some of the test logic could be cleaned up a bit with some of our utils, but its not really a huge deal in this PR, I'll do it in a quick followup.
| let (channel_id, funding_tx, funding_outpoint) = | ||
| create_channel_manual_funding(&nodes, 0, 1, 100_000, 10_000); | ||
| let funding_msgs = |
There was a problem hiding this comment.
It would be nice to have the same test (for the HTLC timeout pipeline) that checks that the commitment tx only gets broadcasted after the funding confirms.
| // In manual-broadcast mode, if we have not yet observed the funding transaction on-chain, | ||
| // return empty vectors. | ||
| if self.is_manual_broadcast && !self.funding_seen_onchain { | ||
| return (Vec::new(), Vec::new()); |
There was a problem hiding this comment.
Would be nice to avoid the allocations above by just moving this up a bit
TheBlueMatt
commented
Oct 28, 2025
I'll do a few followups to this separately. |
Uh oh!
There was an error while loading. Please reload this page.
Minor bugfixes and test improvements to #4109
TheBlueMatt
commented
Oct 29, 2025
Backported to 0.2 in #4185. |
Closes#3591
As part of the client_trusts_lsp LSPS2 work, we decided to move the channelmonitor logic into a new PR so the other part could get merged
This comment is not yet done #3838 (comment), hence this was created as draft