[RFC] Add ChannelMonitor::get_justice_txs for simplified watchtower integration by FreeOnlineUser · Pull Request #4453 · lightningdevkit/rust-lightning

FreeOnlineUser · 2026-03-01T06:57:59Z

Implements the approach @TheBlueMatt suggested in lightningdevkit/ldk-node#813 and in review of #2552: move justice tx state tracking inside ChannelMonitor so Persist implementors don't need external queues.

What this does

Adds two methods:

pub struct JusticeTransaction {
    pub tx: Transaction,
    pub revoked_commitment_txid: Txid,
    pub commitment_number: u64,
}

impl ChannelMonitor {
    pub fn get_pending_justice_txs(
        &self,
        feerate_per_kw: u64,
        destination_script: ScriptBuf,
    ) -> Vec<JusticeTransaction>;
}

impl ChannelMonitorUpdate {
    /// True when the update contains counterparty commitment data
    /// relevant to a watchtower (new commitment or revocation secret).
    pub fn updates_watchtower_state(&self) -> bool;
}

updates_watchtower_state() signals to the Persist layer when a new revocation may have landed. Persist implementations are expected to call get_pending_justice_txs(), deliver the results to a watchtower, and only then complete the monitor update. Rotation of tracked commitments happens in the normal update path; crash-safety relies on Persist delaying completion until delivery.

Covers both to_local and HTLC outputs on revoked commitments.

Changes

channelmonitor.rs: JusticeTransaction struct, cur/prev counterparty commitment tracking (TLV 43/45 for main funding, TLV 13/15 inside each FundingScope for pending splice funding), get_pending_justice_txs(), updates_watchtower_state().
test_utils.rs: simplified WatchtowerPersister (dropped JusticeTxData, unsigned_justice_tx_data queue, form_justice_data_from_commitment).
functional_tests.rs: coverage for crash-recovery and cross-validation against the persister.

399 insertions, 95 deletions across 3 files.

Splice handling

Pruning uses commitment numbers, not entry count. During a splice, multiple entries share the same commitment number (one per funding scope) and all are retained.

Backwards compatibility

New TLVs are optional. Old monitors load with empty vecs, populated on next update.
Old nodes reading monitors written by new code skip the unknown TLV fields.
Existing counterparty_commitment_txs_from_update() and sign_to_local_justice_tx() APIs unchanged.

Assisted by Joe (Claude Opus 4.7 under the hood).

ldk-reviews-bot · 2026-03-01T06:58:02Z

👋 I see @valentinewallace was un-assigned.
If you'd like another reviewer assignment, please click here.

codecov · 2026-03-01T10:09:08Z

Codecov Report

❌ Patch coverage is 95.28796% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.42%. Comparing base (3d94ac7) to head (2e36a84).

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	94.85%	7 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4453      +/-   ##
==========================================
- Coverage   86.42%   86.42%   -0.01%     
==========================================
  Files         158      158              
  Lines      109324   109446     +122     
  Branches   109324   109446     +122     
==========================================
+ Hits        94484    94586     +102     
- Misses      12300    12316      +16     
- Partials     2540     2544       +4

Flag	Coverage Δ
fuzzing-fake-hashes	`5.06% <0.00%> (-0.02%)`	⬇️
fuzzing-real-hashes	`22.74% <11.42%> (-0.04%)`	⬇️
tests	`86.15% <95.28%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

TheBlueMatt · 2026-03-02T20:04:05Z

+	///
+	/// Returns a list of [`JusticeTransaction`]s, each containing a fully signed
+	/// transaction and metadata about the revoked commitment it punishes.
+	pub fn get_justice_txs(


This API doesn't really make sense. There's no information on when I should call this or when the monitor "knows about" a revoked tx. We probably want to do soemthing like the old API where you can fetch revoked transactions in relation to a specific monitor update.

Replaced with sign_initial_justice_tx() for persist_new_channel and sign_justice_txs_from_update(update) for update_persisted_channel. Each is tied to a specific point in the persistence pipeline.

TheBlueMatt

Thanks, I think this basically looks good!

TheBlueMatt · 2026-03-09T14:47:26Z

+		let mut result = Vec::new();
+		for commitment_tx in &to_sign {
+			if let Some(jtx) =
+				self.try_sign_justice_tx(commitment_tx, feerate_per_kw, destination_script.clone())


Should we also build justice transactions for all the potential HTLC transactions? I assume we can...

Added. try_sign_justice_txs now iterates nondust_htlcs() on the revoked commitment, builds a spending tx for each HTLC output, and signs with sign_justice_revoked_htlc. Witness follows the same pattern as RevokedHTLCOutput in package.rs. Dust HTLCs are skipped when fee exceeds value.

Also added test_justice_tx_htlc_from_monitor_updates which routes a payment, captures the commitment with a pending HTLC, revokes it, and verifies the watchtower gets justice txs for both to_local and the HTLC output.

TheBlueMatt · 2026-03-09T14:48:12Z

+				funding.prev_counterparty_commitment_tx =
+					funding.cur_counterparty_commitment_tx.take();
+				funding.cur_counterparty_commitment_tx = Some(commitment_tx);
+			}


I assume we can/should do this?

Suggested change

}

} else {

debug_assert!(false);

}

Extends try_sign_justice_txs to also sweep HTLC outputs when a counterparty broadcasts a revoked commitment. For each non-dust HTLC, builds a spending transaction and signs it with sign_justice_revoked_htlc. Updates WatchtowerPersister to store multiple justice txs per revoked commitment (Vec<Transaction> instead of single Transaction). Addresses review feedback from TheBlueMatt on PR lightningdevkit#4453.

FreeOnlineUser

Updated with both changes. API is now sign_initial_justice_txs / try_sign_justice_txs (plural) since they return a Vec covering to_local + HTLCs. WatchtowerPersister stores Vec<Transaction> per revoked txid accordingly.

Extends try_sign_justice_txs to also sweep HTLC outputs when a counterparty broadcasts a revoked commitment. For each non-dust HTLC, builds a spending transaction and signs it with sign_justice_revoked_htlc. Updates WatchtowerPersister to store multiple justice txs per revoked commitment (Vec<Transaction> instead of single Transaction). Addresses review feedback from TheBlueMatt on PR lightningdevkit#4453.

ldk-claude-review-bot · 2026-03-19T08:31:25Z

+		let mut to_sign = Vec::new();
+		for funding in core::iter::once(&mut self.funding).chain(self.pending_funding.iter_mut()) {
+			let should_take =
+				funding.prev_counterparty_commitment_tx.as_ref().is_some_and(|prev| {
+					self.commitment_secrets.get_secret(prev.commitment_number()).is_some()
+				});
+			if should_take {
+				to_sign.push(funding.prev_counterparty_commitment_tx.take().unwrap());
+			}


Bug: sign_justice_txs_from_update mutates the monitor's in-memory state via .take() on prev_counterparty_commitment_tx. Since this is called inside a Persist callback, if the persist operation fails and is retried, the prev commitment has already been removed from memory — the retry will produce no justice txs.

The mutation is committed immediately (interior mutability via Mutex), but the serialized/persisted state may not reflect it if the persist fails. On a process restart, the old persisted state would have the prev, but during the same process lifetime, the data is gone.

Consider either:

Making this idempotent (don't take, just check and sign — dedup on the caller side)

Or cloning instead of taking, and only clearing after confirmed persist

ldk-claude-review-bot · 2026-03-19T08:31:29Z

+		// Store new commitments, rotating cur -> prev per funding scope.
+		for commitment_tx in new_commitment_txs {
+			let txid = commitment_tx.trust().built_transaction().txid;
+			let funding = core::iter::once(&mut self.funding)
+				.chain(self.pending_funding.iter_mut())
+				.find(|f| f.current_counterparty_commitment_txid == Some(txid));
+			if let Some(funding) = funding {
+				funding.prev_counterparty_commitment_tx =
+					funding.cur_counterparty_commitment_tx.take();
+				funding.cur_counterparty_commitment_tx = Some(commitment_tx);
+			} else {
+				debug_assert!(false);
+			}
+		}


The rotation prev = cur.take(); cur = new; happens before checking whether the old prev had a revocation secret available. If the caller skips calling sign_justice_txs_from_update for an update that only contains a revocation (no new commitment), and then calls it for the next update that does contain a new commitment, the old prev is overwritten without ever being signed.

This is safe under the assumption that the caller invokes this for every ChannelMonitorUpdate, but the doc comment doesn't state this requirement. The method should either:

Document that it MUST be called for every update (not just commitment-bearing ones), or

Check the old prev for revocation secrets before the rotation, sign it if ready, and then rotate.

ldk-claude-review-bot · 2026-03-19T08:31:33Z

+			Some(tx) => tx,
+			None => return Vec::new(),
+		};
+		self.funding.cur_counterparty_commitment_tx = Some(commitment_tx.clone());


This unconditionally overwrites cur_counterparty_commitment_tx with the initial commitment. Since provide_initial_counterparty_commitment_tx (line 3539) already sets this field, this is redundant in the normal path.

More importantly, since sign_initial_justice_txs is a pub method, a caller who mistakenly invokes it after updates have been applied would reset cur_counterparty_commitment_tx back to the initial commitment, silently corrupting the rotation state for sign_justice_txs_from_update. Consider guarding this (e.g., only set if cur_counterparty_commitment_tx.is_none()).

ldk-claude-review-bot · 2026-03-19T08:31:40Z

+					let fee = Amount::from_sat(crate::chain::chaininterface::fee_for_weight(feerate_per_kw as u32, 
+						// Base tx weight + witness weight
+						Transaction { version: Version::TWO, lock_time: LockTime::ZERO, input: input.clone(), output: vec![TxOut { script_pubkey: destination_script.clone(), value: htlc_value }] }.weight().to_wu() + weight_estimate
+					));


Nit: This constructs a full Transaction object (cloning input and destination_script) purely for weight estimation. The existing build_to_local_justice_tx does this too, but here it happens in a loop for every HTLC. Consider computing the base weight once outside the loop since it's the same for all HTLC justice txs (single input, single output to the same destination_script).

ldk-claude-review-bot · 2026-03-19T08:31:46Z

+	(13, cur_counterparty_commitment_tx, option),
+	(15, prev_counterparty_commitment_tx, option),


These fields are added to FundingScope's impl_writeable_tlv_based! (at TLV 13/15, which applies to pending_funding entries), and to the top-level write_chanmon_internal (at TLV 39/41, for the main funding scope). During deserialization, the main funding scope reads from TLV 39/41 while pending_funding entries read from their per-FundingScope TLV 13/15.

This split serialization works but is subtle and fragile — a future change that serializes the main funding as a whole FundingScope would silently double-write the data. Worth adding a comment explaining why these live in two places.

ldk-claude-review-bot · 2026-03-19T08:31:50Z

@@ -722,24 +705,19 @@ impl WatchtowerPersister {
 			.get(&channel_id)


The old justice_tx method returned the single justice tx directly. Now it returns only the first element from the vec via .first(). This works because try_sign_justice_txs pushes the to_local justice tx first, but this ordering is an implicit invariant. If the order in try_sign_justice_txs ever changes, existing callers of justice_tx would silently get the wrong transaction. Consider adding a comment documenting this assumption, or filtering by output type.

ldk-claude-review-bot · 2026-03-19T08:31:58Z

Review Summary

Critical Bug

channelmonitor.rs:3647-3654 — Security-critical: batched CommitmentSecret + new counterparty commitment in a single ChannelMonitorUpdate (confirmed to occur during revoke_and_ack with holding cell free at channel.rs:9388-9393) causes the just-revoked commitment to be permanently lost from cur/prev. The comment claiming this is safe is wrong. Justice txs for revoked commitments will be silently dropped during high-throughput forwarding.

Correctness Issues

channelmonitor.rs:4424-4425 — Comment says "for the LatestCounterpartyCommitmentTXInfo path" but this post-loop is also the only mechanism populating cur_counterparty_commitment_tx for RenegotiatedFunding (splice) scopes. Misleading for future maintainers planning to deprecate the legacy path.
test_utils.rs:804 — Vec::push accumulates duplicate justice txs across repeated watchtower-relevant updates. The old code used idempotent HashMap::insert. Should use insert instead of entry().or_insert_with(Vec::new).push().
functional_tests.rs:1102-1144 — test_justice_tx_crash_recovery doesn't test crash recovery — no serialize/deserialize round-trip. TLV 43/45 serialization is untested.

Stale Prior Comments

Many of the 21 prior inline comments reference code from an older version of the PR that no longer exists (methods sign_justice_txs_from_update, sign_initial_justice_txs, sticky guard at line 3641, debug_assert!(false), duplicate assertions, INITIAL_COMMITMENT_NUMBER import). The PR was significantly reworked — the current version uses a read-only get_pending_justice_txs instead of the previous mutable approach. These stale comments should be disregarded.

Cross-cutting Concerns

No test exercises the batched revoke_and_ack scenario — the critical bug path has zero test coverage.
No test exercises the LatestCounterpartyCommitment path — all tests use test_legacy_channel_config(), exercising only LatestCounterpartyCommitmentTXInfo.
No serialization round-trip test for TLV 43/45 or TLV 15/17.
The counterparty_commitment_txs_from_update call was promoted from debug-only to unconditional in the post-loop. If the CommitmentTransaction reconstruction via build_counterparty_commitment_tx produces an incorrect txid, the mismatch is only caught by a debug_assert_eq (line 4828), not in release builds.

TheBlueMatt · 2026-03-20T01:16:52Z

+	/// Stores new counterparty commitment(s) from the update and signs any
+	/// previously-stored commitments whose revocation secrets are now available.
+	///
+	/// Intended to be called during [`Persist::update_persisted_channel`].


This needs a lot more detail. One big concern is that we need to describe how crash-safety works. Let's say I start storing a ChannelMonitorUpdate while I'm waiting on my watchtower to persist my new transactions, but then crash before the second. I have to make sure the transactions get reloaded on restart somehow and aren't lost, even if the ChannelMonitorUpdate made it to disk (and thus wasn't replayed). Not sure if this should be handled by forcing downstream logic to persist before the monitor or if we should have some way to do a replay.

You're right, the update-already-persisted case means replay alone isn't enough.

I think the cleanest fix is: keep the prev/cur commitment data in the monitor (clone instead of .take()), serialize it, and add a get_pending_justice_txs() method that reads the stored state without mutating it. On restart, the Persist impl calls this against the loaded monitor to recover any justice txs it didn't finish sending.

The full crash-safety model would be:

Normal path: Persist impl gets justice txs from the update, sends to watchtower, returns Completed

Crash recovery: Persist impl calls get_pending_justice_txs() on the loaded monitor at startup, re-sends anything the watchtower doesn't have

The watchtower deduplicates on its end (or the Persist impl tracks what's been acked)

This makes the API idempotent and crash-safe without requiring any ordering constraints on when the monitor update is persisted. I'll rework the implementation and expand the docs.

I think that seems reasonable. My question then is do we even need sign_justice_txs_from_update? Should we instead drop the from-update API and replace it with a simple bool on a ChannelMonitorUpdate to check if there is a commitment update that may need to be written to a watchtower?

Agreed. Dropped both sign_justice_txs_from_update and sign_initial_justice_txs. The API is now just get_pending_justice_txs() plus ChannelMonitorUpdate::updates_watchtower_state() to signal when to call it. Rotation happens in the normal update path.

ldk-reviews-bot · 2026-03-21T00:01:11Z

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-03-23T00:02:02Z

🔔 2nd Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-claude-review-bot · 2026-04-03T05:02:21Z

+		// Safe to overwrite prev: CommitmentSecret and new counterparty commitments
+		// are always in separate ChannelMonitorUpdates, so the Persist callback has
+		// a chance to call get_pending_justice_txs between revocation and rotation.
+		self.funding.prev_counterparty_commitment_tx =
+			self.funding.cur_counterparty_commitment_tx.take();
+		self.funding.cur_counterparty_commitment_tx = Some(current_funding_commitment_tx.clone());


Bug (security): The comment's claimed invariant is false. revoke_and_ack processing in channel.rs (lines 9211-9216, 9239-9244) batches CommitmentSecret + LatestCounterpartyCommitment/LatestCounterpartyCommitmentTXInfo into a single ChannelMonitorUpdate whenever the holding cell is freed or a new commitment is required.

This causes missed justice transactions. Trace:

Setup: prev = C(K), cur = C(K-1). Neither has been revoked yet.

Batched update: [CommitmentSecret(K), LatestCounterpartyCommitment(C(K-2))]

CommitmentSecret(K) processed → secret for K stored

LatestCounterpartyCommitment(C(K-2)) processed → unconditional rotation: prev = C(K-1), cur = C(K-2). C(K) is lost from both slots.

Persist callback → get_pending_justice_txs(): prev = C(K-1) (no secret for K-1 yet), cur = C(K-2) (no secret) → no justice tx produced for C(K)

C(K)'s secret exists but C(K) is in neither prev nor cur. Its justice tx is permanently lost.

This happens whenever the remote's revoke_and_ack triggers maybe_free_holding_cell_htlcs() or require_commitment — a common scenario during high-throughput payment forwarding.

The same issue affects the post-loop at lines 4376-4391 (the LatestCounterpartyCommitmentTXInfo path), which also unconditionally rotates prev.

Fix suggestion: Before overwriting prev, check if it holds a commitment whose secret is now available. If so, produce justice txs for it before rotating — or buffer the outgoing prev in a temporary list that get_pending_justice_txs also checks. Alternatively, the rotation logic could be moved to the CommitmentSecret step (where the revocation actually happens) rather than the commitment step (where the new commitment arrives).

FreeOnlineUser · 2026-04-15T07:39:55Z

Addressed all feedback from the latest round:

API simplified to get_pending_justice_txs() + updates_watchtower_state() per Matt's direction
Fixed incorrect comment about batched revoke_and_ack updates (CommitmentSecret + new commitment can share a single ChannelMonitorUpdate when the holding cell is freed)
Fixed splice scope rotation bug: removed sticky guard that permanently blocked rotation after first revocation, now matches the primary funding scope with unconditional rotation
Fixed indentation inconsistency in the LatestCounterpartyCommitmentTXInfo handler
Strengthened crash recovery test assertion with cross-validation between persister and get_pending_justice_txs

Ready for re-review.

ldk-reviews-bot · 2026-04-15T07:47:15Z

✅ Added second reviewer: @valentinewallace

ldk-reviews-bot · 2026-04-18T00:00:39Z

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-20T00:02:09Z

🔔 2nd Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-22T00:02:54Z

🔔 3rd Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-25T00:00:34Z

🔔 4th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-27T00:01:39Z

🔔 5th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-29T00:02:11Z

🔔 6th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-05-02T00:00:40Z

🔔 7th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-05-04T00:01:38Z

🔔 8th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-05-06T00:02:40Z

🔔 9th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-05-09T00:00:45Z

🔔 10th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-05-11T00:02:01Z

🔔 11th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

valentinewallace

I think this is correct, although the test coverage seems a bit light. Do we have coverage claiming the HTLC justice transactions generated here?

I also have a few suggestions to get rid of the new loop when handling monitor updates, and instead only updating the stored counterparty commitment transaction in one place, provide_latest_holder_commitment_tx, see the top 4 commits here: https://github.com/valentinewallace/rust-lightning/tree/watchtower-justice-api. I also think we can DRY a good bit of the htlc justice tx generation code with what's in package.rs.

I don't want to hold this up though, so I'm okay to land this as-is if @TheBlueMatt is happy, but will probably follow-up with a cleanup or two if we do.

valentinewallace · 2026-05-11T14:43:14Z

+	/// for the same monitor state.
+	///
+	/// [`Persist`]: crate::chain::chainmonitor::Persist
+	pub fn get_pending_justice_txs(


Commit message needs updating, it references some outdated method names

valentinewallace · 2026-05-11T15:08:30Z

 			current_holder_commitment_tx: alternative_holder_commitment_tx.clone(),
 			prev_holder_commitment_tx: None,
+
+			cur_counterparty_commitment_tx: None,


It reads like we should be setting this field here? Any reason we aren't?

valentinewallace · 2026-05-11T17:30:38Z

@@ -3486,6 +3548,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		self.provide_latest_counterparty_commitment_tx(commitment_tx.trust().txid(), Vec::new(), commitment_tx.commitment_number(),
 				commitment_tx.per_commitment_point());
 		// Soon, we will only populate this field


nit: outdated comment that can be removed?

valentinewallace · 2026-05-11T17:34:05Z

 	}
+
+	/// Returns `true` if this update contains counterparty commitment data
+	/// relevant to a watchtower (a new commitment or a revocation secret).


Could use a link to get_pending_justice_txs so it's clear when/how this should be used

valentinewallace · 2026-05-11T18:07:19Z

+							&channel_parameters.channel_type_features,
+						)
+					};
+					let fee = Amount::from_sat(crate::chain::chaininterface::fee_for_weight(


nit, similar above:

Suggested change

let fee = Amount::from_sat(crate::chain::chaininterface::fee_for_weight(

let fee = Amount::from_sat(chain::chaininterface::fee_for_weight(

valentinewallace · 2026-05-11T19:10:50Z

Also needs rebase

Adds sign_initial_justice_txs(), sign_justice_txs_from_update(), and get_pending_justice_txs() to ChannelMonitor, enabling Persist implementors to obtain signed justice transactions for both to_local and HTLC outputs without maintaining external state. Storage uses cur/prev counterparty commitment fields on FundingScope, matching the existing pattern and supporting splicing. The API is crash-safe: commitment data is cloned rather than consumed, and get_pending_justice_txs() allows recovery after restart. Simplifies WatchtowerPersister in test_utils by removing manual queue and signing logic. Addresses feedback from lightningdevkit/ldk-node#813 and picks up the intent of lightningdevkit#2552.

FreeOnlineUser · 2026-05-18T01:58:13Z

Rebased onto current main. TLV IDs 13/15 in FundingScope shifted to 15/17 to make room for upstream's new contribution field at 13; outer ChannelMonitorImpl IDs (43/45) unchanged. No semantic changes.

ldk-claude-review-bot · 2026-05-18T02:03:20Z

+		// Note: CommitmentSecret and new counterparty commitments may be batched
+		// in a single ChannelMonitorUpdate (e.g. when revoke_and_ack frees the
+		// holding cell). This is safe because get_pending_justice_txs checks both
+		// cur and prev for available secrets, so the revoked commitment remains
+		// accessible in prev after rotation.
+		self.funding.prev_counterparty_commitment_tx =
+			self.funding.cur_counterparty_commitment_tx.take();
+		self.funding.cur_counterparty_commitment_tx = Some(current_funding_commitment_tx.clone());


Bug (security-critical): This comment's safety claim is false. The rotation at lines 3652-3654 unconditionally overwrites prev, causing the just-revoked commitment to be permanently lost when CommitmentSecret + LatestCounterpartyCommitment are batched in a single ChannelMonitorUpdate.

Confirmed batching occurs at channel.rs:9388-9393 (maybe_free_holding_cell_htlcs) and channel.rs:9410-9421 (require_commitment), both during revoke_and_ack processing.

Trace:

State before batch: prev = C(K), cur = C(K-1)

CommitmentSecret(K) processed → secret for K stored

LatestCounterpartyCommitment(C(K-2)) processed → rotation: prev = C(K-1), cur = C(K-2)

C(K) is gone from both slots, but its secret is available

get_pending_justice_txs: checks prev=C(K-1) (no secret for K-1), cur=C(K-2) (no secret) → empty result, justice tx for C(K) permanently lost

The comment says "the revoked commitment remains accessible in prev after rotation" — but prev is overwritten with cur (C(K-1)), not with the old prev (C(K)).

The identical bug exists in the post-loop at lines 4435-4436 (for the LatestCounterpartyCommitmentTXInfo path) and at lines 3669-3671 (pending_funding scopes).

Suggested fix: Before overwriting prev, check if it holds a commitment whose secret is now available and produce justice txs for it — or change the architecture so rotation happens at CommitmentSecret time rather than at new-commitment time. Alternatively, keep a small ring buffer of the last N commitments rather than just cur/prev.

ldk-claude-review-bot · 2026-05-18T02:03:31Z

+						.unwrap()
+						.entry(jtx.revoked_commitment_txid)
+						.or_insert_with(Vec::new)
+						.push(jtx.tx);


Bug: get_pending_justice_txs is idempotent — it returns the same results on repeated calls for the same monitor state. But Vec::push accumulates duplicates across consecutive watchtower-relevant updates that don't rotate prev.

Example: a CommitmentSecret update followed by a RenegotiatedFunding update — both trigger updates_watchtower_state(), and prev doesn't change between them, so the same justice tx(s) are pushed again.

The old code used HashMap::insert(commitment_txid, signed_justice_tx) which was an idempotent upsert. Replace push with insert to match:

Suggested change

.push(jtx.tx);

.insert(jtx.revoked_commitment_txid, vec![jtx.tx]);

(replaces rather than appends, correct since get_pending_justice_txs always returns the complete set for each revoked commitment)

ldk-claude-review-bot · 2026-05-18T02:03:41Z

+		// Populate cur/prev for the LatestCounterpartyCommitmentTXInfo path, which
+		// doesn't go through update_counterparty_commitment_data.


The comment says "for the LatestCounterpartyCommitmentTXInfo path" but this post-loop is also the only mechanism that populates cur_counterparty_commitment_tx for RenegotiatedFunding scopes (created at line 4126 with cur_counterparty_commitment_tx: None).

If this code is later removed during the planned LatestCounterpartyCommitmentTXInfo deprecation without adding equivalent logic for RenegotiatedFunding, splice funding scopes would silently lose their initial commitment tracking.

Suggested change

// Populate cur/prev for the LatestCounterpartyCommitmentTXInfo path, which

// doesn't go through update_counterparty_commitment_data.

// Populate cur/prev for paths that don't go through

// update_counterparty_commitment_data: LatestCounterpartyCommitmentTXInfo

// (legacy, to be removed) and RenegotiatedFunding (splice).

ldk-claude-review-bot · 2026-05-18T02:03:50Z

+pub fn test_justice_tx_crash_recovery() {
+	// Verify that get_pending_justice_txs returns justice txs for the most
+	// recently revoked counterparty commitment, enabling crash recovery.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let destination_script = chanmon_cfgs[1].keys_manager.get_destination_script([0; 32]).unwrap();
+	let persisters = [
+		WatchtowerPersister::new(
+			chanmon_cfgs[0].keys_manager.get_destination_script([0; 32]).unwrap(),
+		),
+		WatchtowerPersister::new(destination_script.clone()),
+	];
+	let node_cfgs = create_node_cfgs_with_persisters(2, &chanmon_cfgs, persisters.iter().collect());
+	let legacy_cfg = test_legacy_channel_config();
+	let node_chanmgrs =
+		create_node_chanmgrs(2, &node_cfgs, &[Some(legacy_cfg.clone()), Some(legacy_cfg)]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let (_, _, channel_id, _) = create_announced_chan_between_nodes(&nodes, 0, 1);
+
+	// Move to a non-initial commitment
+	send_payment(&nodes[0], &[&nodes[1]], 5_000_000);
+
+	// Capture the commitment that will be revoked
+	let revoked_local_txn = get_local_commitment_txn!(nodes[0], channel_id);
+	assert_eq!(revoked_local_txn.len(), 1);
+	let revoked_txid = revoked_local_txn[0].compute_txid();
+
+	// Revoke it
+	send_payment(&nodes[0], &[&nodes[1]], 5_000_000);
+
+	// The persister should have a justice tx for this revoked commitment
+	assert!(persisters[1].justice_tx(channel_id, &revoked_txid).is_some());
+
+	// get_pending_justice_txs should also return it, since prev still holds
+	// the revoked commitment data (cloned, not consumed by signing).
+	let pending = {
+		let monitor = get_monitor!(nodes[1], channel_id);
+		monitor.get_pending_justice_txs(FEERATE_FLOOR_SATS_PER_KW as u64, destination_script)
+	};
+	assert!(!pending.is_empty());
+	// Verify the persister and get_pending_justice_txs agree on the revoked txid
+	assert!(persisters[1].justice_tx(channel_id, &pending[0].revoked_commitment_txid).is_some());
+}


This test is named "crash_recovery" but doesn't exercise the crash recovery path — no serialize/deserialize round-trip of the monitor. It verifies that get_pending_justice_txs returns results during normal (non-crash) operation, which overlaps with test_justice_tx_idempotent.

A meaningful crash recovery test should:

Serialize the monitor after the revocation

Deserialize it into a fresh ChannelMonitor

Call get_pending_justice_txs on the deserialized monitor

Verify the justice tx is present

This would exercise the TLV 43/45 serialization round-trip and confirm prev_counterparty_commitment_tx survives persistence — the actual invariant crash recovery depends on.

FreeOnlineUser mentioned this pull request Mar 1, 2026

Watchtower support: expose channel monitor data for external watchtower implementations lightningdevkit/ldk-node#813

Open

FreeOnlineUser force-pushed the watchtower-justice-api branch 2 times, most recently from 6993e59 to d6c7903 Compare March 1, 2026 08:23

TheBlueMatt reviewed Mar 2, 2026

View reviewed changes

FreeOnlineUser force-pushed the watchtower-justice-api branch 2 times, most recently from 9af37e1 to 034c377 Compare March 2, 2026 23:38

TheBlueMatt reviewed Mar 9, 2026

View reviewed changes

FreeOnlineUser commented Mar 10, 2026

View reviewed changes

FreeOnlineUser marked this pull request as ready for review March 10, 2026 10:56

ldk-reviews-bot requested a review from joostjager March 10, 2026 10:57

FreeOnlineUser marked this pull request as draft March 10, 2026 11:01

joostjager removed their request for review March 17, 2026 10:59

FreeOnlineUser marked this pull request as ready for review March 18, 2026 13:09

ldk-reviews-bot requested a review from valentinewallace March 18, 2026 13:10

FreeOnlineUser force-pushed the watchtower-justice-api branch from 7455283 to e3976a0 Compare March 19, 2026 08:17

ldk-claude-review-bot reviewed Mar 19, 2026

View reviewed changes

Comment thread lightning/src/chain/channelmonitor.rs Outdated

ldk-claude-review-bot reviewed Mar 19, 2026

View reviewed changes

TheBlueMatt reviewed Mar 20, 2026

View reviewed changes

ldk-claude-review-bot reviewed Apr 3, 2026

View reviewed changes

FreeOnlineUser force-pushed the watchtower-justice-api branch 3 times, most recently from b8fc4f9 to 92e35d2 Compare April 3, 2026 06:06

FreeOnlineUser force-pushed the watchtower-justice-api branch 3 times, most recently from 7c2ab92 to 5cef481 Compare April 15, 2026 05:34

ldk-reviews-bot requested a review from valentinewallace April 15, 2026 07:47

valentinewallace reviewed May 11, 2026

View reviewed changes

FreeOnlineUser force-pushed the watchtower-justice-api branch from 5cef481 to 2e36a84 Compare May 18, 2026 01:49

ldk-claude-review-bot reviewed May 18, 2026

View reviewed changes

		(13, cur_counterparty_commitment_tx, option),
		(15, prev_counterparty_commitment_tx, option),

		@@ -722,24 +705,19 @@ impl WatchtowerPersister {
		.get(&channel_id)

	let fee = Amount::from_sat(crate::chain::chaininterface::fee_for_weight(
	let fee = Amount::from_sat(chain::chaininterface::fee_for_weight(

	.push(jtx.tx);
	.insert(jtx.revoked_commitment_txid, vec![jtx.tx]);

		// Populate cur/prev for the LatestCounterpartyCommitmentTXInfo path, which
		// doesn't go through update_counterparty_commitment_data.

-		// Populate cur/prev for the LatestCounterpartyCommitmentTXInfo path, which
-		// doesn't go through update_counterparty_commitment_data.
+		// Populate cur/prev for paths that don't go through
+		// update_counterparty_commitment_data: LatestCounterpartyCommitmentTXInfo
+		// (legacy, to be removed) and RenegotiatedFunding (splice).

Conversation

FreeOnlineUser commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this does

Changes

Splice handling

Backwards compatibility

Uh oh!

ldk-reviews-bot commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FreeOnlineUser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-claude-review-bot commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Summary

Critical Bug

Correctness Issues

Stale Prior Comments

Cross-cutting Concerns

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Mar 21, 2026

Uh oh!

ldk-reviews-bot commented Mar 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FreeOnlineUser commented Apr 15, 2026

Uh oh!

ldk-reviews-bot commented Apr 15, 2026

Uh oh!

ldk-reviews-bot commented Apr 18, 2026

Uh oh!

ldk-reviews-bot commented Apr 20, 2026

Uh oh!

ldk-reviews-bot commented Apr 22, 2026

Uh oh!

FreeOnlineUser commented Mar 1, 2026 •

edited

Loading

ldk-reviews-bot commented Mar 1, 2026 •

edited

Loading

codecov Bot commented Mar 1, 2026 •

edited

Loading

ldk-claude-review-bot commented Mar 19, 2026 •

edited

Loading