From 63c56a4a86cd638ebd1d85c7a227b7c19f2f6765 Mon Sep 17 00:00:00 2001 From: Devrandom Date: Tue, 5 Jan 2021 14:13:05 -0800 Subject: [PATCH] Panic if signing fails in OnChainTx Signatures in OnChainTx must not fail, or we stand to lose funds --- lightning/src/ln/onchaintx.rs | 59 +++++++++------------ lightning/src/util/enforcing_trait_impls.rs | 2 +- 2 files changed, 25 insertions(+), 36 deletions(-) diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 80ad434ce..9e77ac974 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -612,17 +612,15 @@ impl OnchainTxHandler { chan_utils::get_revokeable_redeemscript(&chan_keys.revocation_key, *on_counterparty_tx_csv, &chan_keys.broadcaster_delayed_payment_key) }; - if let Ok(sig) = self.key_storage.sign_justice_transaction(&bumped_tx, i, *amount, &per_commitment_key, htlc, &self.secp_ctx) { - bumped_tx.input[i].witness.push(sig.serialize_der().to_vec()); - bumped_tx.input[i].witness[0].push(SigHashType::All as u8); - if htlc.is_some() { - bumped_tx.input[i].witness.push(chan_keys.revocation_key.clone().serialize().to_vec()); - } else { - bumped_tx.input[i].witness.push(vec!(1)); - } - bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); - } else { return None; } - //TODO: panic ? + let sig = self.key_storage.sign_justice_transaction(&bumped_tx, i, *amount, &per_commitment_key, htlc, &self.secp_ctx).expect("sign justice tx"); + bumped_tx.input[i].witness.push(sig.serialize_der().to_vec()); + bumped_tx.input[i].witness[0].push(SigHashType::All as u8); + if htlc.is_some() { + bumped_tx.input[i].witness.push(chan_keys.revocation_key.clone().serialize().to_vec()); + } else { + bumped_tx.input[i].witness.push(vec!(1)); + } + bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); log_trace!(logger, "Going to broadcast Penalty Transaction {} claiming revoked {} output {} from {} with new feerate {}...", bumped_tx.txid(), if *input_descriptor == InputDescriptors::RevokedOutput { "to_holder" } else if *input_descriptor == InputDescriptors::RevokedOfferedHTLC { "offered" } else if *input_descriptor == InputDescriptors::RevokedReceivedHTLC { "received" } else { "" }, outp.vout, outp.txid, new_feerate); } @@ -632,17 +630,16 @@ impl OnchainTxHandler { let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, &chan_keys.broadcaster_htlc_key, &chan_keys.countersignatory_htlc_key, &chan_keys.revocation_key); if !preimage.is_some() { bumped_tx.lock_time = htlc.cltv_expiry }; // Right now we don't aggregate time-locked transaction, if we do we should set lock_time before to avoid breaking hash computation - if let Ok(sig) = self.key_storage.sign_counterparty_htlc_transaction(&bumped_tx, i, &htlc.amount_msat / 1000, &per_commitment_point, htlc, &self.secp_ctx) { - bumped_tx.input[i].witness.push(sig.serialize_der().to_vec()); - bumped_tx.input[i].witness[0].push(SigHashType::All as u8); - if let &Some(preimage) = preimage { - bumped_tx.input[i].witness.push(preimage.0.to_vec()); - } else { - // Due to BIP146 (MINIMALIF) this must be a zero-length element to relay. - bumped_tx.input[i].witness.push(vec![]); - } - bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); + let sig = self.key_storage.sign_counterparty_htlc_transaction(&bumped_tx, i, &htlc.amount_msat / 1000, &per_commitment_point, htlc, &self.secp_ctx).expect("sign counterparty HTLC tx"); + bumped_tx.input[i].witness.push(sig.serialize_der().to_vec()); + bumped_tx.input[i].witness[0].push(SigHashType::All as u8); + if let &Some(preimage) = preimage { + bumped_tx.input[i].witness.push(preimage.0.to_vec()); + } else { + // Due to BIP146 (MINIMALIF) this must be a zero-length element to relay. + bumped_tx.input[i].witness.push(vec![]); } + bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); log_trace!(logger, "Going to broadcast Claim Transaction {} claiming counterparty {} htlc output {} from {} with new feerate {}...", bumped_tx.txid(), if preimage.is_some() { "offered" } else { "received" }, outp.vout, outp.txid, new_feerate); } }, @@ -952,13 +949,9 @@ impl OnchainTxHandler { // to monitor before. pub(crate) fn get_fully_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Option { if let Some(ref mut holder_commitment) = self.holder_commitment { - match self.key_storage.sign_holder_commitment_and_htlcs(holder_commitment, &self.secp_ctx) { - Ok((sig, htlc_sigs)) => { - self.holder_htlc_sigs = Some(Self::extract_holder_sigs(holder_commitment, htlc_sigs)); - Some(holder_commitment.add_holder_sig(funding_redeemscript, sig)) - }, - Err(_) => return None, - } + let (sig, htlc_sigs) = self.key_storage.sign_holder_commitment_and_htlcs(holder_commitment, &self.secp_ctx).expect("signing holder commitment"); + self.holder_htlc_sigs = Some(Self::extract_holder_sigs(holder_commitment, htlc_sigs)); + Some(holder_commitment.add_holder_sig(funding_redeemscript, sig)) } else { None } @@ -967,13 +960,9 @@ impl OnchainTxHandler { #[cfg(any(test, feature="unsafe_revoked_tx_signing"))] pub(crate) fn get_fully_signed_copy_holder_tx(&mut self, funding_redeemscript: &Script) -> Option { if let Some(ref mut holder_commitment) = self.holder_commitment { - match self.key_storage.sign_holder_commitment_and_htlcs(holder_commitment, &self.secp_ctx) { - Ok((sig, htlc_sigs)) => { - self.holder_htlc_sigs = Some(Self::extract_holder_sigs(holder_commitment, htlc_sigs)); - Some(holder_commitment.add_holder_sig(funding_redeemscript, sig)) - }, - Err(_) => return None, - } + let (sig, htlc_sigs) = self.key_storage.sign_holder_commitment_and_htlcs(holder_commitment, &self.secp_ctx).expect("sign holder commitment"); + self.holder_htlc_sigs = Some(Self::extract_holder_sigs(holder_commitment, htlc_sigs)); + Some(holder_commitment.add_holder_sig(funding_redeemscript, sig)) } else { None } diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index a340cbf42..8229f6048 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -8,7 +8,7 @@ // licenses. use ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, HolderCommitmentTransaction, CommitmentTransaction, ChannelTransactionParameters, TrustedCommitmentTransaction}; -use ln::{msgs, chan_utils}; +use ln::{chan_utils, msgs}; use chain::keysinterface::{ChannelKeys, InMemoryChannelKeys}; use std::cmp; -- 2.39.5