Skip to content

[RFC] Add ChannelMonitor::get_justice_txs for simplified watchtower integration#4453

Open
FreeOnlineUser wants to merge 4 commits intolightningdevkit:mainfrom
FreeOnlineUser:watchtower-justice-api
Open

[RFC] Add ChannelMonitor::get_justice_txs for simplified watchtower integration#4453
FreeOnlineUser wants to merge 4 commits intolightningdevkit:mainfrom
FreeOnlineUser:watchtower-justice-api

Conversation

@FreeOnlineUser
Copy link

Draft PR for design feedback. 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 a single method that replaces the current 3-step dance of counterparty_commitment_txs_from_update() + manual queue + sign_to_local_justice_tx():

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

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

Internally, ChannelMonitorImpl stores recent counterparty CommitmentTransactions (populated during update application, pruned to entries within one revocation of current). get_justice_txs checks which have revocation secrets available, builds and signs the justice tx, and returns the result.

Changes

  • channelmonitor.rs: new JusticeTransaction struct, latest_counterparty_commitment_txs field, TLV 39 serialization (optional, backwards-compatible), get_justice_txs() method
  • test_utils.rs: simplified WatchtowerPersister (removed JusticeTxData, unsigned_justice_tx_data queue, form_justice_data_from_commitment)
  • 127 additions, 83 deletions. All existing tests pass.

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

  • TLV 39 is optional. Old monitors load with empty vec, populated on next update.
  • Old nodes reading monitors written by new code skip the unknown TLV field.
  • Existing counterparty_commitment_txs_from_update() and sign_to_local_justice_tx() APIs unchanged.

Design questions

Looking for input on these before moving out of draft:

  1. Signed vs unsigned return? Currently returns fully signed transactions. Returning unsigned gives callers more flexibility on fee choice at broadcast time. Preference?

  2. HTLC outputs? This only covers to_local justice. HTLC-timeout and HTLC-success outputs on revoked commitments are not included. Worth adding here, or separate follow-up?

  3. Feerate source? Caller provides feerate_per_kw. An alternative would be accepting a fee estimator. Any preference?

  4. Dust filtering? If the revokeable output is dust, revokeable_output_index() returns None and the entry is skipped silently. Right behavior, or surface this to the caller?

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Mar 1, 2026

I've assigned @valentinewallace 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.

@codecov
Copy link

codecov bot commented Mar 1, 2026

Codecov Report

❌ Patch coverage is 92.15686% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.93%. Comparing base (ec03159) to head (034c377).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/chain/channelmonitor.rs 95.12% 0 Missing and 4 partials ⚠️
lightning/src/util/test_utils.rs 80.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4453      +/-   ##
==========================================
- Coverage   85.93%   85.93%   -0.01%     
==========================================
  Files         159      159              
  Lines      104693   104736      +43     
  Branches   104693   104736      +43     
==========================================
+ Hits        89972    90001      +29     
- Misses      12213    12222       +9     
- Partials     2508     2513       +5     
Flag Coverage Δ
tests 85.93% <92.15%> (-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.

/// Contains the current and previous counterparty commitment(s). With splicing,
/// there may be multiple entries per commitment number (one per funding scope).
/// Pruned to remove entries more than one revocation old.
latest_counterparty_commitment_txs: Vec<CommitmentTransaction>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in the funding scope to support splicing. Also should probably just be cur_... and prev_... to match existing API pattern rather than a vec that is max-size 2.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to FundingScope as cur/prev fields. TLV 13/15 on FundingScope, 39/41 on the outer scope for backwards compat.

Err(_) => break,
}
}
// Use the simplified get_justice_txs API
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude loves to leave comments about how it changed the code that are totally useless - reading the code I don't care what changes claude made in the past, I care if there's something useful to know now. Probably can just drop this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, and noted for future.

///
/// 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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think this basically looks good!

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())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

funding.prev_counterparty_commitment_tx =
funding.cur_counterparty_commitment_tx.take();
funding.cur_counterparty_commitment_tx = Some(commitment_tx);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we can/should do this?

Suggested change
}
} else {
debug_assert!(false);
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

FreeOnlineUser added a commit to FreeOnlineUser/rust-lightning that referenced this pull request Mar 10, 2026
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.
Copy link
Author

@FreeOnlineUser FreeOnlineUser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@FreeOnlineUser FreeOnlineUser marked this pull request as ready for review March 10, 2026 10:56
@FreeOnlineUser FreeOnlineUser marked this pull request as draft March 10, 2026 11:01
@joostjager joostjager removed their request for review March 17, 2026 10:59
@FreeOnlineUser FreeOnlineUser marked this pull request as ready for review March 18, 2026 13:09
Adds sign_initial_justice_tx() and sign_justice_txs_from_update() to
ChannelMonitor, allowing Persist implementors to obtain signed justice
transactions without maintaining external state.

Storage uses cur/prev counterparty commitment fields on FundingScope,
matching the existing pattern and supporting splicing.

Simplifies WatchtowerPersister in test_utils by removing the manual
queue and signing logic.

Addresses feedback from lightningdevkit/ldk-node#813 and picks up
the intent of lightningdevkit#2552.
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.
Verifies that WatchtowerPersister produces justice transactions for
both to_local and in-flight HTLC outputs when a commitment with
pending HTLCs is revoked. Checks each justice tx spends a different
output and pays to the destination script.
@FreeOnlineUser FreeOnlineUser force-pushed the watchtower-justice-api branch from 7455283 to e3976a0 Compare March 19, 2026 08:17
Comment on lines +4660 to +4668
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());
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +4643 to +4656
// 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);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Document that it MUST be called for every update (not just commitment-bearing ones), or
  2. Check the old prev for revocation secrets before the rotation, sign it if ready, and then rotate.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()).

funding.cur_counterparty_commitment_tx.take();
funding.cur_counterparty_commitment_tx = Some(commitment_tx);
} else {
debug_assert!(false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug_assert!(false) only fires in debug builds. In release, the new commitment is silently dropped, meaning the watchtower will miss this commitment entirely (and any subsequent ones for this funding scope). At minimum, this should log a warning. Consider log_error! or returning an error.

Comment on lines +4743 to +4746
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
));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines +1216 to +1217
(13, cur_counterparty_commitment_tx, option),
(15, prev_counterparty_commitment_tx, option),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants