Bypass monitor sync requests when no partition key given in update_monitor_with_chain_data

[nicolft]

This proposed patch concerns chain::ChainMonitor.

Context

• In ChainMonitor, process_chain_data takes a best_height: Option<u32> parameter, which is used as a partition key in update_monitor_with_chain_data. Each channel monitor is partitioned by its channel_id mod 50, and only when best_height matches the channel_id partition does a sync request get sent to the persister.
• This is mainly intended for use with best_block_updated, which "must be called whenever a new chain tip becomes available" (doc comment).
  • Side note: best_block_updated and transactions_confirmed are Confirm trait functions
• This design distributes sync requests so that a channel (if it doesn't urgently need to sync) will sync once every 50 blocks, since process_chain_data is called each time best_block_updated is called.
• The doc comment for process_chain_data also supports this intent: "Calls which represent a new blockchain tip height should set best_height." (doc comment)

Issue

• In update_monitor_with_chain_data, where the partition logic is stored, a partition key/best_height of None will be defaulted to partition key 0, triggering unnecessary syncs for partition 0 of channel monitors.
• Each time transactions_confirmed is called, process_chain_data iterates through all channels/monitors, triggering syncs for partition 0.
• In lightning-transaction-sync, the user of ChainMonitor's Confirm trait, when common::sync_confirmed_transactions is called, each confirmed transaction will trigger a call to transactions_confirmed.

We can use some numbers and trace to be really concrete. Suppose we have 250 channels, 10 of which have pending transactions:

• Invoke lightning-transaction-sync::esplora::sync() with chain_monitor in args (esplora.rs line 85)
• → else branch, line 187: self.get_confirmed_transactions(&sync_state)
  • (stepping inside,)
  • → lines 295 - 302: Put all unique confirmed txid into confirmed_txs
  • → line 339: Return confirmed_txs, which is a 250 long Vec (1 transaction per channel)
• → line 202: sync_state.sync_confirmed_transactions(<chain_monitor>, confirmed_txs)
• → common.rs line 75: for each ctx in confirmed_txs (250), do chain_monitor.transactions_confirmed(<tx info>)
• → chainmonitor.rs line 1515: self.process_chain_data(...)
• → line 484: for each channel (250), self.update_monitor_with_chain_data(<associated monitor/channel/tx data>) (line 487)
• → line 568: Send a persist request if channel monitor has_pending_claims OR if channel falls into the partition (1/50 chance)
  • So out of these 250 inner calls, we expect 10 + 240/50 = ~15 to make a persist request
    • 10 pending; (250-10) non-pending roll the dice to see if they land in the special partition
  • 15 * (250 outer calls) = 3750
  • Notice that if just 1 more of our channels had a pending request, this would result in ~+250 more persist requests = ~4000

In the case of the application I'm working on, this means that sync is destroying buffers... which is causing problems. 😅

Solution
The problem is that syncs are being scheduled even when they shouldn't be, because a best_height of None causes partition channels 0 to sync. So the solution is to skip this syncing step conditional on the presence of best_height.

Follow-up question
We experience the NxM amplification issue with pending transactions, since these are persisted every transactions_confirmed call as well. Is there a reason why these need to be persisted on every confirmed transaction, or would it be safe to condition that persist request on best_height as well?

[ldk-reviews-bot]

I've assigned @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[ldk-claude-review-bot]

The funding.rs changes are purely rustfmt reformatting — the macro invocations are broken into multi-line format but are semantically identical. No issues there.

The core functional change in chainmonitor.rs at lines 558 and 568-569 is correct and addresses the intended bug. The previous no-op issue I flagged has been fixed.

Let me verify there aren't any other files changed in this PR that I might be missing.

The diff shows two files:

1. lightning/src/chain/chainmonitor.rs — the actual fix (2 hunks)
2. lightning/src/ln/funding.rs — formatting only (5 hunks)

Both have been reviewed. The functional fix is correct. No new bugs introduced. My prior review comments have been addressed (the no-op was fixed).

Review Summary

No new issues found beyond what was previously flagged.

The no-op bug from the prior review has been resolved — best_height.map(|height| channel_id_u32.wrapping_add(height)) now correctly returns None when best_height is None, and .is_some_and() properly skips the partition-based sync.

Cross-cutting concerns (not tied to specific lines)

• No test coverage: There is still no test verifying the partition-skip behavior when best_height is None. A regression test calling transactions_confirmed and asserting that partition-based syncs are not triggered (only pending-claims syncs) would be valuable.
• Formatting noise: The funding.rs changes are pure rustfmt reformatting, unrelated to the fix. Separating formatting into its own commit improves bisectability.

[TheBlueMatt]

oof, yea, that's not great. thanks. Trivial so gonna go ahead and land, will backport as well.