Skip to content

Make ChannelSigner solely responsible for validating commitment sigs #3878

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make ChannelSigner solely responsible for validating commitment sigs #3878

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3773a7
Make `ChannelSigner` solely responsible for validating commitment sigs
tankyleo Jun 21, 2025
4005d59
Remove format skips in channel holder commitment validation functions
tankyleo Jun 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 50 additions & 100 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@ use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::hashes::sha256d::Hash as Sha256d;
use bitcoin::hashes::Hash;

use bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE;
use bitcoin::secp256k1::{ecdsa::Signature, Secp256k1};
use bitcoin::secp256k1::{PublicKey, SecretKey};
use bitcoin::{secp256k1, sighash};
use bitcoin::secp256k1::{
self, constants::PUBLIC_KEY_SIZE, ecdsa::Signature, PublicKey, Secp256k1, SecretKey,
};

use crate::chain::chaininterface::{
fee_for_weight, ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator,
Expand Down Expand Up @@ -2512,29 +2511,6 @@ where

fn received_msg(&self) -> &'static str;

#[rustfmt::skip]
fn check_counterparty_commitment_signature<L: Deref>(
&self, sig: &Signature, holder_commitment_point: &HolderCommitmentPoint, logger: &L
) -> Result<CommitmentTransaction, ChannelError> where L::Target: Logger {
let funding_script = self.funding().get_funding_redeemscript();

let commitment_data = self.context().build_commitment_transaction(self.funding(),
holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(),
true, false, logger);
let initial_commitment_tx = commitment_data.tx;
let trusted_tx = initial_commitment_tx.trust();
let initial_commitment_bitcoin_tx = trusted_tx.built_transaction();
let sighash = initial_commitment_bitcoin_tx.get_sighash_all(&funding_script, self.funding().get_value_satoshis());
// They sign the holder commitment transaction...
log_trace!(logger, "Checking {} tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} for channel {}.",
self.received_msg(), log_bytes!(sig.serialize_compact()[..]), log_bytes!(self.funding().counterparty_funding_pubkey().serialize()),
encode::serialize_hex(&initial_commitment_bitcoin_tx.transaction), log_bytes!(sighash[..]),
encode::serialize_hex(&funding_script), &self.context().channel_id());
secp_check!(self.context().secp_ctx.verify_ecdsa(&sighash, sig, self.funding().counterparty_funding_pubkey()), format!("Invalid {} signature from peer", self.received_msg()));

Ok(initial_commitment_tx)
}

#[rustfmt::skip]
fn initial_commitment_signed<L: Deref>(
&mut self, channel_id: ChannelId, counterparty_signature: Signature, holder_commitment_point: &mut HolderCommitmentPoint,
Expand All @@ -2543,32 +2519,11 @@ where
where
L::Target: Logger
{
let initial_commitment_tx = match self.check_counterparty_commitment_signature(&counterparty_signature, holder_commitment_point, logger) {
Ok(res) => res,
Err(ChannelError::Close(e)) => {
// TODO(dual_funding): Update for V2 established channels.
if !self.funding().is_outbound() {
self.funding_mut().channel_transaction_parameters.funding_outpoint = None;
}
return Err(ChannelError::Close(e));
},
Err(e) => {
// The only error we know how to handle is ChannelError::Close, so we fall over here
// to make sure we don't continue with an inconsistent state.
panic!("unexpected error type from check_counterparty_commitment_signature {:?}", e);
}
};
let context = self.context();
let commitment_data = context.build_commitment_transaction(self.funding(),
context.cur_counterparty_commitment_transaction_number,
&context.counterparty_cur_commitment_point.unwrap(), false, false, logger);
let counterparty_initial_commitment_tx = commitment_data.tx;
let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust();
let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction();

log_trace!(logger, "Initial counterparty tx for channel {} is: txid {} tx {}",
&context.channel_id(), counterparty_initial_bitcoin_tx.txid, encode::serialize_hex(&counterparty_initial_bitcoin_tx.transaction));

let initial_commitment_tx = context.build_commitment_transaction(self.funding(),
holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(),
true, false, logger).tx;
let holder_commitment_tx = HolderCommitmentTransaction::new(
initial_commitment_tx,
counterparty_signature,
Expand All @@ -2577,10 +2532,29 @@ where
&self.funding().counterparty_funding_pubkey()
);

if context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, Vec::new()).is_err() {
log_trace!(logger, "Validating {} commitment in channel {}", self.received_msg(), context.channel_id());
if context.holder_signer.as_ref().validate_holder_commitment(
&self.funding().channel_transaction_parameters,
&holder_commitment_tx,
Vec::new(),
&context.secp_ctx,
).is_err() {
if !self.funding().is_outbound() {
self.funding_mut().channel_transaction_parameters.funding_outpoint = None;
}
return Err(ChannelError::close("Failed to validate our commitment".to_owned()));
}

let commitment_data = context.build_commitment_transaction(self.funding(),
context.cur_counterparty_commitment_transaction_number,
&context.counterparty_cur_commitment_point.unwrap(), false, false, logger);
let counterparty_initial_commitment_tx = commitment_data.tx;
let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust();
let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction();

log_trace!(logger, "Initial counterparty tx for channel {} is: txid {} tx {}",
&context.channel_id(), counterparty_initial_bitcoin_tx.txid, encode::serialize_hex(&counterparty_initial_bitcoin_tx.transaction));

// Now that we're past error-generating stuff, update our local state:

let is_v2_established = self.is_v2_established();
Expand Down Expand Up @@ -4207,25 +4181,30 @@ where
where
L::Target: Logger,
{
let funding_script = funding.get_funding_redeemscript();

let commitment_data = self.build_commitment_transaction(funding,
holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(),
true, false, logger);
let commitment_txid = {
let trusted_tx = commitment_data.tx.trust();
let bitcoin_tx = trusted_tx.built_transaction();
let sighash = bitcoin_tx.get_sighash_all(&funding_script, funding.get_value_satoshis());

log_trace!(logger, "Checking commitment tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} in channel {}",
log_bytes!(msg.signature.serialize_compact()[..]),
log_bytes!(funding.counterparty_funding_pubkey().serialize()), encode::serialize_hex(&bitcoin_tx.transaction),
log_bytes!(sighash[..]), encode::serialize_hex(&funding_script), &self.channel_id());
if let Err(_) = self.secp_ctx.verify_ecdsa(&sighash, &msg.signature, &funding.counterparty_funding_pubkey()) {
return Err(ChannelError::close("Invalid commitment tx signature from peer".to_owned()));
}
bitcoin_tx.txid
};

if msg.htlc_signatures.len() != commitment_data.tx.nondust_htlcs().len() {
return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.tx.nondust_htlcs().len())));
}

let holder_commitment_tx = HolderCommitmentTransaction::new(
commitment_data.tx,
msg.signature,
msg.htlc_signatures.clone(),
&funding.get_holder_pubkeys().funding_pubkey,
funding.counterparty_funding_pubkey()
);

log_trace!(logger, "Validating commitment_signed commitment in channel {}", self.channel_id());
self.holder_signer.as_ref().validate_holder_commitment(
&funding.channel_transaction_parameters,
&holder_commitment_tx,
commitment_data.outbound_htlc_preimages,
&self.secp_ctx,
)
.map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?;

// If our counterparty updated the channel fee in this commitment transaction, check that
// they can actually afford the new fee now.
Expand Down Expand Up @@ -4257,28 +4236,10 @@ where
}
}

if msg.htlc_signatures.len() != commitment_data.tx.nondust_htlcs().len() {
return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.tx.nondust_htlcs().len())));
}

let holder_keys = commitment_data.tx.trust().keys();
let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.tx.nondust_htlcs().len());
let mut dust_htlcs = Vec::with_capacity(commitment_data.htlcs_included.len() - commitment_data.tx.nondust_htlcs().len());
for (idx, (htlc, mut source_opt)) in commitment_data.htlcs_included.into_iter().enumerate() {
let mut nondust_htlc_sources = Vec::with_capacity(holder_commitment_tx.nondust_htlcs().len());
let mut dust_htlcs = Vec::with_capacity(commitment_data.htlcs_included.len() - holder_commitment_tx.nondust_htlcs().len());
for (htlc, mut source_opt) in commitment_data.htlcs_included.into_iter() {
if let Some(_) = htlc.transaction_output_index {
let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.tx.feerate_per_kw(),
funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, funding.get_channel_type(),
&holder_keys.broadcaster_delayed_payment_key, &holder_keys.revocation_key);

let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, funding.get_channel_type(), &holder_keys);
let htlc_sighashtype = if funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() { EcdsaSighashType::SinglePlusAnyoneCanPay } else { EcdsaSighashType::All };
let htlc_sighash = hash_to_message!(&sighash::SighashCache::new(&htlc_tx).p2wsh_signature_hash(0, &htlc_redeemscript, htlc.to_bitcoin_amount(), htlc_sighashtype).unwrap()[..]);
log_trace!(logger, "Checking HTLC tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} in channel {}.",
log_bytes!(msg.htlc_signatures[idx].serialize_compact()[..]), log_bytes!(holder_keys.countersignatory_htlc_key.to_public_key().serialize()),
encode::serialize_hex(&htlc_tx), log_bytes!(htlc_sighash[..]), encode::serialize_hex(&htlc_redeemscript), &self.channel_id());
if let Err(_) = self.secp_ctx.verify_ecdsa(&htlc_sighash, &msg.htlc_signatures[idx], &holder_keys.countersignatory_htlc_key.to_public_key()) {
return Err(ChannelError::close("Invalid HTLC tx signature from peer".to_owned()));
}
if htlc.offered {
if let Some(source) = source_opt.take() {
nondust_htlc_sources.push(source.clone());
Expand All @@ -4292,17 +4253,6 @@ where
debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere");
}

let holder_commitment_tx = HolderCommitmentTransaction::new(
commitment_data.tx,
msg.signature,
msg.htlc_signatures.clone(),
&funding.get_holder_pubkeys().funding_pubkey,
funding.counterparty_funding_pubkey()
);

self.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_data.outbound_htlc_preimages)
.map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?;

Ok(LatestHolderCommitmentTXInfo {
commitment_tx: holder_commitment_tx,
htlc_outputs: dust_htlcs,
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8892,7 +8892,7 @@ pub fn test_duplicate_funding_err_in_funding() {
funding_created_msg.funding_output_index += 10;
nodes[1].node.handle_funding_created(node_c_id, &funding_created_msg);
get_err_msg(&nodes[1], &node_c_id);
let err = "Invalid funding_created signature from peer".to_owned();
let err = "Failed to validate our commitment".to_owned();
Copy link
Collaborator

@TheBlueMatt TheBlueMatt Jun 23, 2025

Choose a reason for hiding this comment

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

Ha, the old error was more descriptive :)

let reason = ClosureReason::ProcessingError { err };
let expected_closing = ExpectedCloseEvent::from_id_reason(real_channel_id, false, reason);
check_closed_events(&nodes[1], &[expected_closing]);
Expand Down
68 changes: 64 additions & 4 deletions lightning/src/sign/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,8 +763,9 @@ pub trait ChannelSigner {
/// closed. If you wish to make this operation asynchronous, you should instead return `Ok(())`
/// and pause future signing operations until this validation completes.
fn validate_holder_commitment(
&self, holder_tx: &HolderCommitmentTransaction,
outbound_htlc_preimages: Vec<PaymentPreimage>,
&self, channel_parameters: &ChannelTransactionParameters,
holder_tx: &HolderCommitmentTransaction, outbound_htlc_preimages: Vec<PaymentPreimage>,
secp_ctx: &Secp256k1<secp256k1::All>,
) -> Result<(), ()>;
Comment on lines 765 to 769
Copy link
Contributor Author

@tankyleo tankyleo Jun 21, 2025

Choose a reason for hiding this comment

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

Do we add a logger here ? This would add a generic on the ChannelSigner trait as far as I see.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Jun 23, 2025

Choose a reason for hiding this comment

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

Logging is great, but we shouldn't add it to the trait, rather store a logger in the InMemorySigner directly.


/// Validate the counterparty's revocation.
Expand Down Expand Up @@ -1362,9 +1363,68 @@ impl ChannelSigner for InMemorySigner {
}

fn validate_holder_commitment(
&self, _holder_tx: &HolderCommitmentTransaction,
_outbound_htlc_preimages: Vec<PaymentPreimage>,
&self, channel_parameters: &ChannelTransactionParameters,
holder_tx: &HolderCommitmentTransaction, _outbound_htlc_preimages: Vec<PaymentPreimage>,
secp_ctx: &secp256k1::Secp256k1<secp256k1::All>,
) -> Result<(), ()> {
let counterparty_funding_pubkey =
&channel_parameters.counterparty_pubkeys().as_ref().unwrap().funding_pubkey;
let funding_script = channel_parameters.make_funding_redeemscript();
let channel_value_satoshis = channel_parameters.channel_value_satoshis;
let trusted_tx = holder_tx.trust();
let bitcoin_tx = trusted_tx.built_transaction();
let sighash = bitcoin_tx.get_sighash_all(&funding_script, channel_value_satoshis);

secp_ctx
.verify_ecdsa(&sighash, &holder_tx.counterparty_sig, counterparty_funding_pubkey)
.map_err(|_| ())?;

let holder_keys = trusted_tx.keys();

if holder_tx.nondust_htlcs().len() != holder_tx.counterparty_htlc_sigs.len() {
return Err(());
}
for (htlc, sig) in
holder_tx.nondust_htlcs().iter().zip(holder_tx.counterparty_htlc_sigs.iter())
{
let htlc_tx = chan_utils::build_htlc_transaction(
&bitcoin_tx.txid,
holder_tx.feerate_per_kw(),
channel_parameters.as_holder_broadcastable().contest_delay(),
&htlc,
&channel_parameters.channel_type_features,
&holder_keys.broadcaster_delayed_payment_key,
&holder_keys.revocation_key,
);
let htlc_redeemscript = chan_utils::get_htlc_redeemscript(
&htlc,
&channel_parameters.channel_type_features,
&holder_keys,
);
let htlc_sighashtype =
if channel_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() {
EcdsaSighashType::SinglePlusAnyoneCanPay
} else {
EcdsaSighashType::All
};
let htlc_sighash = hash_to_message!(
&sighash::SighashCache::new(&htlc_tx)
.p2wsh_signature_hash(
0,
&htlc_redeemscript,
htlc.to_bitcoin_amount(),
htlc_sighashtype
)
Comment on lines +1390 to +1417
Copy link
Collaborator

@TheBlueMatt TheBlueMatt Jun 23, 2025

Choose a reason for hiding this comment

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

For code moves, please move the code as-is first (with only indentation changes) and then run rustfmt in a separate commit. That way git show --color-moved --color-moved-ws=ignore-space-change highlights it as a clean move.

.unwrap()[..]
);
secp_ctx
.verify_ecdsa(
&htlc_sighash,
&sig,
&holder_keys.countersignatory_htlc_key.to_public_key(),
)
.map_err(|_| ())?;
}
Ok(())
}

Expand Down
4 changes: 3 additions & 1 deletion lightning/src/util/dyn_signer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,10 @@ delegate!(DynSigner, ChannelSigner,
) -> Result<PublicKey, ()>,
fn release_commitment_secret(, idx: u64) -> Result<[u8; 32], ()>,
fn validate_holder_commitment(,
channel_parameters: &ChannelTransactionParameters,
holder_tx: &HolderCommitmentTransaction,
preimages: Vec<PaymentPreimage>
preimages: Vec<PaymentPreimage>,
secp_ctx: &Secp256k1<All>
) -> Result<(), ()>,
fn pubkeys(,
splice_parent_funding_txid: Option<Txid>, secp_ctx: &Secp256k1<secp256k1::All>
Expand Down
12 changes: 9 additions & 3 deletions lightning/src/util/test_channel_signer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,9 @@ impl ChannelSigner for TestChannelSigner {
}

fn validate_holder_commitment(
&self, holder_tx: &HolderCommitmentTransaction,
_outbound_htlc_preimages: Vec<PaymentPreimage>,
&self, channel_parameters: &ChannelTransactionParameters,
holder_tx: &HolderCommitmentTransaction, outbound_htlc_preimages: Vec<PaymentPreimage>,
secp_ctx: &Secp256k1<secp256k1::All>,
) -> Result<(), ()> {
let mut state = self.state.lock().unwrap();
let idx = holder_tx.commitment_number();
Expand All @@ -205,7 +206,12 @@ impl ChannelSigner for TestChannelSigner {
);
}
state.last_holder_commitment = idx;
Ok(())
self.inner.validate_holder_commitment(
channel_parameters,
holder_tx,
outbound_htlc_preimages,
secp_ctx,
)
}

fn validate_counterparty_revocation(&self, idx: u64, _secret: &SecretKey) -> Result<(), ()> {
Expand Down