From 5101d2086c0cf2134ce9d729cfdf69b0a21d564e Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Fri, 20 Mar 2020 18:04:01 -0400 Subject: [PATCH 01/19] Remove signing local commitment transaction from ChannelMonitor Extend external signer interface to sign local commitment transactions on its behalf without seckey passing. This move will allow us to remove key access from ChannelMonitor hot memory in further work. Local commitment transaction should stay half-signed by remote until we need to broadcast for a channel force-close or a HTLC to timeout onchain. Add an unsafe test-only version of sign_local_commitment to fulfill our test_framework needs. --- lightning/src/chain/keysinterface.rs | 27 ++++++++++++++++++++- lightning/src/ln/chan_utils.rs | 22 ++++++++++++++--- lightning/src/ln/channel.rs | 4 +-- lightning/src/ln/channelmonitor.rs | 4 +-- lightning/src/util/enforcing_trait_impls.rs | 12 ++++++++- 5 files changed, 60 insertions(+), 9 deletions(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 2b90188637c..5f1a307bf27 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -24,7 +24,7 @@ use util::logger::Logger; use util::ser::{Writeable, Writer, Readable}; use ln::chan_utils; -use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys}; +use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys, LocalCommitmentTransaction}; use ln::msgs; use std::sync::Arc; @@ -215,6 +215,22 @@ pub trait ChannelKeys : Send+Clone { /// making the callee generate it via some util function we expose)! fn sign_remote_commitment(&self, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; + /// Create a signature for a local commitment transaction + /// + /// TODO: Document the things someone using this interface should enforce before signing. + /// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and + /// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method + /// making the callee generate it via some util function we expose)! + fn sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1); + + /// Create a signature for a local commitment transaction without enforcing one-time signing. + /// + /// Testing revocation logic by our test framework needs to sign multiple local commitment + /// transactions. This unsafe test-only version doesn't enforce one-time signing security + /// requirement. + #[cfg(test)] + fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1); + /// Create a signature for a (proposed) closing transaction. /// /// Note that, due to rounding, there may be one "missing" satoshi, and either party may have @@ -342,6 +358,15 @@ impl ChannelKeys for InMemoryChannelKeys { Ok((commitment_sig, htlc_sigs)) } + fn sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { + local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx); + } + + #[cfg(test)] + fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { + local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx); + } + fn sign_closing_transaction(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1) -> Result { if closing_tx.input.len() != 1 { return Err(()); } if closing_tx.input[0].witness.len() != 0 { return Err(()); } diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 561448c8fbf..0ef269c8cb8 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -516,7 +516,7 @@ pub(crate) fn sign_htlc_transaction(tx: &mut Transaction, /// We use this to track local commitment transactions and put off signing them until we are ready /// to broadcast. Eventually this will require a signer which is possibly external, but for now we /// just pass in the SecretKeys required. -pub(crate) struct LocalCommitmentTransaction { +pub struct LocalCommitmentTransaction { tx: Transaction } impl LocalCommitmentTransaction { @@ -530,7 +530,9 @@ impl LocalCommitmentTransaction { } } } - pub fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction { + /// Generate a new LocalCommitmentTransaction based on a raw commitment transaction, + /// remote signature and both parties keys + pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction { if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); } if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); } @@ -549,10 +551,13 @@ impl LocalCommitmentTransaction { Self { tx } } + /// Get the txid of the local commitment transaction contained in this + /// LocalCommitmentTransaction pub fn txid(&self) -> Sha256dHash { self.tx.txid() } + /// Check if LocalCommitmentTransaction has already been signed by us pub fn has_local_sig(&self) -> bool { if self.tx.input.len() != 1 { panic!("Commitment transactions must have input count == 1!"); } if self.tx.input[0].witness.len() == 4 { @@ -567,6 +572,15 @@ impl LocalCommitmentTransaction { } } + /// Add local signature for LocalCommitmentTransaction, do nothing if signature is already + /// present + /// + /// Funding key is your key included in the 2-2 funding_outpoint lock. Should be provided + /// by your ChannelKeys. + /// Funding redeemscript is script locking funding_outpoint. This is the mutlsig script + /// between your own funding key and your counterparty's. Currently, this is provided in + /// ChannelKeys::sign_local_commitment() calls directly. + /// Channel value is amount locked in funding_outpoint. pub fn add_local_sig(&mut self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { if self.has_local_sig() { return; } let sighash = hash_to_message!(&bip143::SighashComponents::new(&self.tx) @@ -584,7 +598,9 @@ impl LocalCommitmentTransaction { self.tx.input[0].witness.push(funding_redeemscript.as_bytes().to_vec()); } - pub fn without_valid_witness(&self) -> &Transaction { &self.tx } + /// Get raw transaction without asserting if witness is complete + pub(crate) fn without_valid_witness(&self) -> &Transaction { &self.tx } + /// Get raw transaction with panics if witness is incomplete pub fn with_valid_witness(&self) -> &Transaction { assert!(self.has_local_sig()); &self.tx diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 374d74f6c18..43aba4201f4 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4433,7 +4433,7 @@ mod tests { assert_eq!(PublicKey::from_secret_key(&secp_ctx, chan_keys.funding_key()).serialize()[..], hex::decode("023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb").unwrap()[..]); - let keys_provider = Keys { chan_keys }; + let keys_provider = Keys { chan_keys: chan_keys.clone() }; let their_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let mut config = UserConfig::default(); @@ -4490,7 +4490,7 @@ mod tests { secp_ctx.verify(&sighash, &their_signature, chan.their_funding_pubkey()).unwrap(); let mut localtx = LocalCommitmentTransaction::new_missing_local_sig(unsigned_tx.0.clone(), &their_signature, &PublicKey::from_secret_key(&secp_ctx, chan.local_keys.funding_key()), chan.their_funding_pubkey()); - localtx.add_local_sig(chan.local_keys.funding_key(), &redeemscript, chan.channel_value_satoshis, &chan.secp_ctx); + chan_keys.sign_local_commitment(&mut localtx, &redeemscript, chan.channel_value_satoshis, &chan.secp_ctx); assert_eq!(serialize(localtx.with_valid_witness())[..], hex::decode($tx_hex).unwrap()[..]); diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 2ac113b9845..8b116b9d63f 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1806,7 +1806,7 @@ impl ChannelMonitor { // tracking state and panic!()ing if we get an update after force-closure/local-tx signing. log_trace!(self, "Getting signed latest local commitment transaction!"); if let &mut Some(ref mut local_tx) = &mut self.current_local_signed_commitment_tx { - local_tx.tx.add_local_sig(&self.onchain_detection.keys.funding_key(), self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &self.secp_ctx); + self.onchain_detection.keys.sign_local_commitment(&mut local_tx.tx, self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &self.secp_ctx); } if let &Some(ref local_tx) = &self.current_local_signed_commitment_tx { let mut res = vec![local_tx.tx.with_valid_witness().clone()]; @@ -1888,7 +1888,7 @@ impl ChannelMonitor { } else { false }; if let Some(ref mut cur_local_tx) = self.current_local_signed_commitment_tx { if should_broadcast { - cur_local_tx.tx.add_local_sig(&self.onchain_detection.keys.funding_key(), self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &self.secp_ctx); + self.onchain_detection.keys.sign_local_commitment(&mut cur_local_tx.tx, self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &mut self.secp_ctx); } } if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx { diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index 26c3a07775f..bc3a1a0da15 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -1,4 +1,4 @@ -use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys}; +use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys, LocalCommitmentTransaction}; use ln::msgs; use chain::keysinterface::{ChannelKeys, InMemoryChannelKeys}; @@ -6,6 +6,7 @@ use std::cmp; use std::sync::{Mutex, Arc}; use bitcoin::blockdata::transaction::Transaction; +use bitcoin::blockdata::script::Script; use secp256k1; use secp256k1::key::{SecretKey, PublicKey}; @@ -78,6 +79,15 @@ impl ChannelKeys for EnforcingChannelKeys { Ok(self.inner.sign_remote_commitment(feerate_per_kw, commitment_tx, keys, htlcs, to_self_delay, secp_ctx).unwrap()) } + fn sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { + self.inner.sign_local_commitment(local_commitment_tx, funding_redeemscript, channel_value_satoshis, secp_ctx) + } + + #[cfg(test)] + fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { + self.inner.unsafe_sign_local_commitment(local_commitment_tx, funding_redeemscript, channel_value_satoshis, secp_ctx); + } + fn sign_closing_transaction(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1) -> Result { Ok(self.inner.sign_closing_transaction(closing_tx, secp_ctx).unwrap()) } From f60519daf21239a5fc23fd4a6af7699ea50bd7cc Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Tue, 3 Mar 2020 17:35:36 -0500 Subject: [PATCH 02/19] Remove duplicata for local commitment+HTLC txn Previously, we would regenerate this class of txn twice due to block-rescan triggered by new watching outputs registered. This commmit doesn't change behavior, it only tweaks TestBroadcaster to ensure we modify cleanly tests anticipating next commit refactor. --- lightning/src/ln/functional_test_utils.rs | 2 +- lightning/src/ln/functional_tests.rs | 54 ++++++----------------- lightning/src/util/test_utils.rs | 15 +++++++ 3 files changed, 30 insertions(+), 41 deletions(-) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 95358208e9d..2e071c2b78b 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1038,7 +1038,7 @@ pub fn fail_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: pub fn create_chanmon_cfgs(node_count: usize) -> Vec { let mut chan_mon_cfgs = Vec::new(); for _ in 0..node_count { - let tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new())}; + let tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), broadcasted_txn: Mutex::new(HashMap::new())}; let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; chan_mon_cfgs.push(TestChanMonCfg{ tx_broadcaster, fee_estimator }); } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 8c0f05723bf..62cae026911 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2335,30 +2335,13 @@ fn claim_htlc_outputs_single_tx() { } let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(node_txn.len(), 21); + assert_eq!(node_txn.len(), 9); // ChannelMonitor: justice tx revoked offered htlc, justice tx revoked received htlc, justice tx revoked to_local (3) // ChannelManager: local commmitment + local HTLC-timeout (2) - // ChannelMonitor: bumped justice tx (4), after one increase, bumps on HTLC aren't generated not being substantial anymore - // ChannelMonito r: local commitment + local HTLC-timeout (14) - - assert_eq!(node_txn[0], node_txn[5]); - assert_eq!(node_txn[0], node_txn[7]); - assert_eq!(node_txn[0], node_txn[9]); - assert_eq!(node_txn[0], node_txn[13]); - assert_eq!(node_txn[0], node_txn[15]); - assert_eq!(node_txn[0], node_txn[17]); - assert_eq!(node_txn[0], node_txn[19]); - - assert_eq!(node_txn[1], node_txn[6]); - assert_eq!(node_txn[1], node_txn[8]); - assert_eq!(node_txn[1], node_txn[10]); - assert_eq!(node_txn[1], node_txn[14]); - assert_eq!(node_txn[1], node_txn[16]); - assert_eq!(node_txn[1], node_txn[18]); - assert_eq!(node_txn[1], node_txn[20]); - - - // Check the pair local commitment and HTLC-timeout broadcast due to HTLC expiration and present 8 times (rebroadcast at every block from 200 to 206) + // ChannelMonitor: bumped justice tx, after one increase, bumps on HTLC aren't generated not being substantial anymore, bump on revoked to_local isn't generated due to more room for expiration (2) + // ChannelMonitor: local commitment + local HTLC-timeout (2) + + // Check the pair local commitment and HTLC-timeout broadcast due to HTLC expiration assert_eq!(node_txn[0].input.len(), 1); check_spends!(node_txn[0], chan_1.3); assert_eq!(node_txn[1].input.len(), 1); @@ -2438,12 +2421,10 @@ fn test_htlc_on_chain_success() { nodes[2].block_notifier.block_connected(&Block { header, txdata: vec![commitment_tx[0].clone()]}, 1); check_closed_broadcast!(nodes[2], false); check_added_monitors!(nodes[2], 1); - let node_txn = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().clone(); // ChannelManager : 3 (commitment tx, 2*htlc-success tx), ChannelMonitor : 4 (2*2 * HTLC-Success tx) - assert_eq!(node_txn.len(), 7); + let node_txn = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().clone(); // ChannelManager : 3 (commitment tx, 2*htlc-success tx), ChannelMonitor : 2 (2 * HTLC-Success tx) + assert_eq!(node_txn.len(), 5); assert_eq!(node_txn[0], node_txn[3]); assert_eq!(node_txn[1], node_txn[4]); - assert_eq!(node_txn[0], node_txn[5]); - assert_eq!(node_txn[1], node_txn[6]); assert_eq!(node_txn[2], commitment_tx[0]); check_spends!(node_txn[0], commitment_tx[0]); check_spends!(node_txn[1], commitment_tx[0]); @@ -2488,15 +2469,11 @@ fn test_htlc_on_chain_success() { macro_rules! check_tx_local_broadcast { ($node: expr, $htlc_offered: expr, $commitment_tx: expr, $chan_tx: expr) => { { let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(node_txn.len(), if $htlc_offered { 7 } else { 5 }); + assert_eq!(node_txn.len(), 5); // Node[1]: ChannelManager: 3 (commitment tx, 2*HTLC-Timeout tx), ChannelMonitor: 2 (timeout tx) - // Node[0]: ChannelManager: 3 (commtiemtn tx, 2*HTLC-Timeout tx), ChannelMonitor: 2 HTLC-timeout * 2 (block-rescan) + // Node[0]: ChannelManager: 3 (commtiemtn tx, 2*HTLC-Timeout tx), ChannelMonitor: 2 HTLC-timeout check_spends!(node_txn[0], $commitment_tx); check_spends!(node_txn[1], $commitment_tx); - if $htlc_offered { - assert_eq!(node_txn[0], node_txn[5]); - assert_eq!(node_txn[1], node_txn[6]); - } assert_ne!(node_txn[0].lock_time, 0); assert_ne!(node_txn[1].lock_time, 0); if $htlc_offered { @@ -2633,11 +2610,9 @@ fn test_htlc_on_chain_timeout() { let timeout_tx; { let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(node_txn.len(), 7); // ChannelManager : 2 (commitment tx, HTLC-Timeout tx), ChannelMonitor : (local commitment tx + HTLC-timeout) * 2 (block-rescan), timeout tx + assert_eq!(node_txn.len(), 5); // ChannelManager : 2 (commitment tx, HTLC-Timeout tx), ChannelMonitor : 2 (local commitment tx + HTLC-timeout), 1 timeout tx assert_eq!(node_txn[0], node_txn[3]); - assert_eq!(node_txn[0], node_txn[5]); assert_eq!(node_txn[1], node_txn[4]); - assert_eq!(node_txn[1], node_txn[6]); check_spends!(node_txn[2], commitment_tx[0]); assert_eq!(node_txn[2].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); @@ -4504,9 +4479,8 @@ fn test_onchain_to_onchain_claim() { check_added_monitors!(nodes[2], 1); let c_txn = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().clone(); // ChannelManager : 2 (commitment tx, HTLC-Success tx), ChannelMonitor : 1 (HTLC-Success tx) - assert_eq!(c_txn.len(), 4); + assert_eq!(c_txn.len(), 3); assert_eq!(c_txn[0], c_txn[2]); - assert_eq!(c_txn[0], c_txn[3]); assert_eq!(commitment_tx[0], c_txn[1]); check_spends!(c_txn[1], chan_2.3); check_spends!(c_txn[2], c_txn[1]); @@ -4622,11 +4596,11 @@ fn test_duplicate_payment_hash_one_failure_one_success() { _ => panic!("Unexepected event"), } let htlc_success_txn: Vec<_> = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().clone(); - assert_eq!(htlc_success_txn.len(), 7); + assert_eq!(htlc_success_txn.len(), 5); // ChannelMonitor: HTLC-Success txn (*2 due to 2-HTLC outputs), ChannelManager: local commitment tx + HTLC-Success txn (*2 due to 2-HTLC outputs) check_spends!(htlc_success_txn[2], chan_2.3); check_spends!(htlc_success_txn[3], htlc_success_txn[2]); check_spends!(htlc_success_txn[4], htlc_success_txn[2]); - assert_eq!(htlc_success_txn[0], htlc_success_txn[5]); + assert_eq!(htlc_success_txn[0], htlc_success_txn[3]); assert_eq!(htlc_success_txn[0].input.len(), 1); assert_eq!(htlc_success_txn[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); assert_eq!(htlc_success_txn[1], htlc_success_txn[4]); @@ -6819,7 +6793,7 @@ fn test_data_loss_protect() { let logger: Arc = Arc::new(test_utils::TestLogger::with_id(format!("node {}", 0))); let mut chan_monitor = <(Sha256dHash, ChannelMonitor)>::read(&mut ::std::io::Cursor::new(previous_chan_monitor_state.0), Arc::clone(&logger)).unwrap().1; let chain_monitor = Arc::new(ChainWatchInterfaceUtil::new(Network::Testnet, Arc::clone(&logger))); - tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new())}; + tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), broadcasted_txn: Mutex::new(HashMap::new())}; fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; keys_manager = test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet, Arc::clone(&logger)); monitor = test_utils::TestChannelMonitor::new(chain_monitor.clone(), &tx_broadcaster, logger.clone(), &fee_estimator); diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 13cbde4aa1f..618cef0c208 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -110,9 +110,24 @@ impl<'a> channelmonitor::ManyChannelMonitor for TestChanne pub struct TestBroadcaster { pub txn_broadcasted: Mutex>, + pub broadcasted_txn: Mutex> // Temporary field while refactoring out tx duplication } impl chaininterface::BroadcasterInterface for TestBroadcaster { fn broadcast_transaction(&self, tx: &Transaction) { + let mut already = false; + { + if let Some(counter) = self.broadcasted_txn.lock().unwrap().get_mut(&tx.txid()) { + match counter { + 0 => { *counter = 1; already = true }, // We still authorize at least 2 duplicata for a given TXID to account ChannelManager/ChannelMonitor broadcast + 1 => return, + _ => panic!() + } + } + } + if !already { + self.broadcasted_txn.lock().unwrap().insert(tx.txid(), 0); + } + print!("\nFRESH BROADCAST {}\n\n", tx.txid()); self.txn_broadcasted.lock().unwrap().push(tx.clone()); } } From 04a17b2a152396384b108e8ef284c85bd9f0b50b Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Fri, 20 Mar 2020 20:06:13 -0400 Subject: [PATCH 03/19] Cache funding_redeemscript in OnchainTxHandler As transaction generation and signature is headed to be moved inside OnchainTxHandler, cache any usefule witness element. --- lightning/src/ln/channelmonitor.rs | 4 ++-- lightning/src/ln/onchaintx.rs | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 8b116b9d63f..6855ac15a77 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1098,7 +1098,7 @@ impl ChannelMonitor { onchain_detection: onchain_detection, their_htlc_base_key: Some(their_htlc_base_key.clone()), their_delayed_payment_base_key: Some(their_delayed_payment_base_key.clone()), - funding_redeemscript: Some(funding_redeemscript), + funding_redeemscript: Some(funding_redeemscript.clone()), channel_value_satoshis: Some(channel_value_satoshis), their_cur_revocation_points: None, @@ -1121,7 +1121,7 @@ impl ChannelMonitor { onchain_events_waiting_threshold_conf: HashMap::new(), outputs_to_watch: HashMap::new(), - onchain_tx_handler: OnchainTxHandler::new(destination_script.clone(), keys, logger.clone()), + onchain_tx_handler: OnchainTxHandler::new(destination_script.clone(), keys, funding_redeemscript, logger.clone()), last_block_hash: Default::default(), secp_ctx: Secp256k1::new(), diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 40d9f18abb7..d5db6950f7a 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -141,6 +141,7 @@ macro_rules! subtract_high_prio_fee { /// do RBF bumping if possible. pub struct OnchainTxHandler { destination_script: Script, + funding_redeemscript: Script, key_storage: ChanSigner, @@ -180,6 +181,7 @@ pub struct OnchainTxHandler { impl OnchainTxHandler { pub(crate) fn write(&self, writer: &mut W) -> Result<(), ::std::io::Error> { self.destination_script.write(writer)?; + self.funding_redeemscript.write(writer)?; self.key_storage.write(writer)?; @@ -221,6 +223,7 @@ impl OnchainTxHandler { impl ReadableArgs> for OnchainTxHandler { fn read(reader: &mut R, logger: Arc) -> Result { let destination_script = Readable::read(reader)?; + let funding_redeemscript = Readable::read(reader)?; let key_storage = Readable::read(reader)?; @@ -269,6 +272,7 @@ impl ReadableArgs> for OnchainTx Ok(OnchainTxHandler { destination_script, + funding_redeemscript, key_storage, claimable_outpoints, pending_claim_requests, @@ -280,12 +284,13 @@ impl ReadableArgs> for OnchainTx } impl OnchainTxHandler { - pub(super) fn new(destination_script: Script, keys: ChanSigner, logger: Arc) -> Self { + pub(super) fn new(destination_script: Script, keys: ChanSigner, funding_redeemscript: Script, logger: Arc) -> Self { let key_storage = keys; OnchainTxHandler { destination_script, + funding_redeemscript, key_storage, pending_claim_requests: HashMap::new(), claimable_outpoints: HashMap::new(), From 73e0a0112adae5086ae6617fec2131f362086797 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Fri, 20 Mar 2020 20:26:23 -0400 Subject: [PATCH 04/19] Cache local_commitment_tx in OnchainTxHandler As transaction generation and signature is headed to be moved inside OnchainTxHandler, cache local_commitment_tx signed by remote. If access to local commitment transaction is needed, we extend Onchain TxHandler API to do so. --- lightning/src/ln/channelmonitor.rs | 3 ++- lightning/src/ln/onchaintx.rs | 17 ++++++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 6855ac15a77..70c92143af8 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1244,7 +1244,7 @@ impl ChannelMonitor { self.prev_local_signed_commitment_tx = self.current_local_signed_commitment_tx.take(); self.current_local_signed_commitment_tx = Some(LocalSignedTx { txid: commitment_tx.txid(), - tx: commitment_tx, + tx: commitment_tx.clone(), revocation_key: local_keys.revocation_key, a_htlc_key: local_keys.a_htlc_key, b_htlc_key: local_keys.b_htlc_key, @@ -1253,6 +1253,7 @@ impl ChannelMonitor { feerate_per_kw, htlc_outputs, }); + self.onchain_tx_handler.provide_latest_local_tx(commitment_tx); Ok(()) } diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index d5db6950f7a..4c935ea6548 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -15,7 +15,7 @@ use secp256k1; use ln::msgs::DecodeError; use ln::channelmonitor::{ANTI_REORG_DELAY, CLTV_SHARED_CLAIM_BUFFER, InputMaterial, ClaimRequest}; -use ln::chan_utils::HTLCType; +use ln::chan_utils::{HTLCType, LocalCommitmentTransaction}; use chain::chaininterface::{FeeEstimator, BroadcasterInterface, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT}; use chain::keysinterface::ChannelKeys; use util::logger::Logger; @@ -142,6 +142,8 @@ macro_rules! subtract_high_prio_fee { pub struct OnchainTxHandler { destination_script: Script, funding_redeemscript: Script, + local_commitment: Option, + prev_local_commitment: Option, key_storage: ChanSigner, @@ -182,6 +184,8 @@ impl OnchainTxHandler { pub(crate) fn write(&self, writer: &mut W) -> Result<(), ::std::io::Error> { self.destination_script.write(writer)?; self.funding_redeemscript.write(writer)?; + self.local_commitment.write(writer)?; + self.prev_local_commitment.write(writer)?; self.key_storage.write(writer)?; @@ -224,6 +228,8 @@ impl ReadableArgs> for OnchainTx fn read(reader: &mut R, logger: Arc) -> Result { let destination_script = Readable::read(reader)?; let funding_redeemscript = Readable::read(reader)?; + let local_commitment = Readable::read(reader)?; + let prev_local_commitment = Readable::read(reader)?; let key_storage = Readable::read(reader)?; @@ -273,6 +279,8 @@ impl ReadableArgs> for OnchainTx Ok(OnchainTxHandler { destination_script, funding_redeemscript, + local_commitment, + prev_local_commitment, key_storage, claimable_outpoints, pending_claim_requests, @@ -291,6 +299,8 @@ impl OnchainTxHandler { OnchainTxHandler { destination_script, funding_redeemscript, + local_commitment: None, + prev_local_commitment: None, key_storage, pending_claim_requests: HashMap::new(), claimable_outpoints: HashMap::new(), @@ -714,4 +724,9 @@ impl OnchainTxHandler { self.pending_claim_requests.remove(&req); } } + + pub(super) fn provide_latest_local_tx(&mut self, tx: LocalCommitmentTransaction) { + self.prev_local_commitment = self.local_commitment.take(); + self.local_commitment = Some(tx); + } } From e46e183084ed858f41aa304acd78503aea1a96ed Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Fri, 27 Mar 2020 17:53:52 -0400 Subject: [PATCH 05/19] Prevent any update of local commitment transaction once signed To prevent any unsafe state discrepancy between offchain and onchain, once local commitment transaction has been signed due to an event (either block height for HTLC-timeout or channel force-closure), don't allow any further update of local commitment transaction view to avoid delivery of revocation secret to counterparty for the aformentionned signed transaction. --- lightning/src/ln/chan_utils.rs | 11 ++++++++++- lightning/src/ln/channelmonitor.rs | 11 +++++++++-- lightning/src/ln/onchaintx.rs | 11 ++++++++++- 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 0ef269c8cb8..a5e0c1460be 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -522,9 +522,18 @@ pub struct LocalCommitmentTransaction { impl LocalCommitmentTransaction { #[cfg(test)] pub fn dummy() -> Self { + let dummy_input = TxIn { + previous_output: OutPoint { + txid: Default::default(), + vout: 0, + }, + script_sig: Default::default(), + sequence: 0, + witness: vec![vec![], vec![], vec![]] + }; Self { tx: Transaction { version: 2, - input: Vec::new(), + input: vec![dummy_input], output: Vec::new(), lock_time: 0, } } diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 70c92143af8..2fa416edb2d 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1241,10 +1241,18 @@ impl ChannelMonitor { if self.their_to_self_delay.is_none() { return Err(MonitorUpdateError("Got a local commitment tx info update before we'd set basic information about the channel")); } + // Returning a monitor error before updating tracking points means in case of using + // a concurrent watchtower implementation for same channel, if this one doesn't + // reject update as we do, you MAY have the latest local valid commitment tx onchain + // for which you want to spend outputs. We're NOT robust again this scenario right + // now but we should consider it later. + if let Err(_) = self.onchain_tx_handler.provide_latest_local_tx(commitment_tx.clone()) { + return Err(MonitorUpdateError("Local commitment signed has already been signed, no further update of LOCAL commitment transaction is allowed")); + } self.prev_local_signed_commitment_tx = self.current_local_signed_commitment_tx.take(); self.current_local_signed_commitment_tx = Some(LocalSignedTx { txid: commitment_tx.txid(), - tx: commitment_tx.clone(), + tx: commitment_tx, revocation_key: local_keys.revocation_key, a_htlc_key: local_keys.a_htlc_key, b_htlc_key: local_keys.b_htlc_key, @@ -1253,7 +1261,6 @@ impl ChannelMonitor { feerate_per_kw, htlc_outputs, }); - self.onchain_tx_handler.provide_latest_local_tx(commitment_tx); Ok(()) } diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 4c935ea6548..d0d1f29e691 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -725,8 +725,17 @@ impl OnchainTxHandler { } } - pub(super) fn provide_latest_local_tx(&mut self, tx: LocalCommitmentTransaction) { + pub(super) fn provide_latest_local_tx(&mut self, tx: LocalCommitmentTransaction) -> Result<(), ()> { + // To prevent any unsafe state discrepancy between offchain and onchain, once local + // commitment transaction has been signed due to an event (either block height for + // HTLC-timeout or channel force-closure), don't allow any further update of local + // commitment transaction view to avoid delivery of revocation secret to counterparty + // for the aformentionned signed transaction. + if let Some(ref local_commitment) = self.local_commitment { + if local_commitment.has_local_sig() { return Err(()) } + } self.prev_local_commitment = self.local_commitment.take(); self.local_commitment = Some(tx); + Ok(()) } } From 2be1f72005d407ed0183b383deeaca6b88becc31 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Tue, 3 Mar 2020 18:51:50 -0500 Subject: [PATCH 06/19] Move local commitment tx generation in OnchainTxHandler Local Commitment Transaction can't be bumped without anchor outputs so their generation is one-time for now. We move them in OnchainTxHandler for simplifying ChannelMonitor and to prepare storage of keys material behind one external signer interface. Some tests break due to change in transaction broadcast order but number of transactions broadcast should stay the same. --- lightning/src/ln/channelmonitor.rs | 21 +++- lightning/src/ln/functional_tests.rs | 44 ++++---- lightning/src/ln/onchaintx.rs | 157 ++++++++++++++++----------- 3 files changed, 132 insertions(+), 90 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 2fa416edb2d..b3cff4c2b42 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -443,6 +443,9 @@ pub(crate) enum InputMaterial { sigs: (Signature, Signature), preimage: Option, amount: u64, + }, + Funding { + channel_value: u64, } } @@ -472,6 +475,10 @@ impl Writeable for InputMaterial { sigs.1.write(writer)?; preimage.write(writer)?; writer.write_all(&byte_utils::be64_to_array(*amount))?; + }, + &InputMaterial::Funding { ref channel_value } => { + writer.write_all(&[3; 1])?; + channel_value.write(writer)?; } } Ok(()) @@ -521,6 +528,12 @@ impl Readable for InputMaterial { preimage, amount } + }, + 3 => { + let channel_value = Readable::read(reader)?; + InputMaterial::Funding { + channel_value + } } _ => return Err(DecodeError::InvalidValue), }; @@ -1894,15 +1907,11 @@ impl ChannelMonitor { let should_broadcast = if let Some(_) = self.current_local_signed_commitment_tx { self.would_broadcast_at_height(height) } else { false }; - if let Some(ref mut cur_local_tx) = self.current_local_signed_commitment_tx { - if should_broadcast { - self.onchain_detection.keys.sign_local_commitment(&mut cur_local_tx.tx, self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &mut self.secp_ctx); - } + if should_broadcast { + claimable_outpoints.push(ClaimRequest { absolute_timelock: height, aggregable: false, outpoint: BitcoinOutPoint { txid: self.onchain_detection.funding_info.as_ref().unwrap().0.txid.clone(), vout: self.onchain_detection.funding_info.as_ref().unwrap().0.index as u32 }, witness_data: InputMaterial::Funding { channel_value: self.channel_value_satoshis.unwrap() }}); } if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx { if should_broadcast { - log_trace!(self, "Broadcast onchain {}", log_tx!(cur_local_tx.tx.with_valid_witness())); - broadcaster.broadcast_transaction(&cur_local_tx.tx.with_valid_witness()); let (txs, new_outputs, _) = self.broadcast_by_local_state(&cur_local_tx); if !new_outputs.is_empty() { watch_outputs.push((cur_local_tx.txid.clone(), new_outputs)); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 62cae026911..0c6bac59cb5 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2342,24 +2342,25 @@ fn claim_htlc_outputs_single_tx() { // ChannelMonitor: local commitment + local HTLC-timeout (2) // Check the pair local commitment and HTLC-timeout broadcast due to HTLC expiration + assert_eq!(node_txn[3].input.len(), 1); + check_spends!(node_txn[3], chan_1.3); assert_eq!(node_txn[0].input.len(), 1); - check_spends!(node_txn[0], chan_1.3); - assert_eq!(node_txn[1].input.len(), 1); - let witness_script = node_txn[1].input[0].witness.last().unwrap(); + let witness_script = node_txn[0].input[0].witness.last().unwrap(); assert_eq!(witness_script.len(), OFFERED_HTLC_SCRIPT_WEIGHT); //Spending an offered htlc output - check_spends!(node_txn[1], node_txn[0]); + check_spends!(node_txn[0], node_txn[3]); - // Justice transactions are indices 2-3-4 + // Justice transactions are indices 1-2-4 + assert_eq!(node_txn[1].input.len(), 1); assert_eq!(node_txn[2].input.len(), 1); - assert_eq!(node_txn[3].input.len(), 1); assert_eq!(node_txn[4].input.len(), 1); + + check_spends!(node_txn[1], revoked_local_txn[0]); check_spends!(node_txn[2], revoked_local_txn[0]); - check_spends!(node_txn[3], revoked_local_txn[0]); check_spends!(node_txn[4], revoked_local_txn[0]); let mut witness_lens = BTreeSet::new(); + witness_lens.insert(node_txn[1].input[0].witness.last().unwrap().len()); witness_lens.insert(node_txn[2].input[0].witness.last().unwrap().len()); - witness_lens.insert(node_txn[3].input[0].witness.last().unwrap().len()); witness_lens.insert(node_txn[4].input[0].witness.last().unwrap().len()); assert_eq!(witness_lens.len(), 3); assert_eq!(*witness_lens.iter().skip(0).next().unwrap(), 77); // revoked to_local @@ -2611,18 +2612,19 @@ fn test_htlc_on_chain_timeout() { { let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); assert_eq!(node_txn.len(), 5); // ChannelManager : 2 (commitment tx, HTLC-Timeout tx), ChannelMonitor : 2 (local commitment tx + HTLC-timeout), 1 timeout tx - assert_eq!(node_txn[0], node_txn[3]); - assert_eq!(node_txn[1], node_txn[4]); - check_spends!(node_txn[2], commitment_tx[0]); - assert_eq!(node_txn[2].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); + assert_eq!(node_txn[2], node_txn[3]); + assert_eq!(node_txn[0], node_txn[4]); - check_spends!(node_txn[0], chan_2.3); - check_spends!(node_txn[1], node_txn[0]); - assert_eq!(node_txn[0].clone().input[0].witness.last().unwrap().len(), 71); - assert_eq!(node_txn[1].clone().input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); + check_spends!(node_txn[1], commitment_tx[0]); + assert_eq!(node_txn[1].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); - timeout_tx = node_txn[2].clone(); + check_spends!(node_txn[2], chan_2.3); + check_spends!(node_txn[0], node_txn[2]); + assert_eq!(node_txn[2].clone().input[0].witness.last().unwrap().len(), 71); + assert_eq!(node_txn[0].clone().input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); + + timeout_tx = node_txn[1].clone(); node_txn.clear(); } @@ -7374,11 +7376,11 @@ fn test_set_outpoints_partial_claiming() { let partial_claim_tx = { let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); assert_eq!(node_txn.len(), 3); - check_spends!(node_txn[1], node_txn[0]); - check_spends!(node_txn[2], node_txn[0]); + check_spends!(node_txn[0], node_txn[2]); + check_spends!(node_txn[1], node_txn[2]); + assert_eq!(node_txn[0].input.len(), 1); assert_eq!(node_txn[1].input.len(), 1); - assert_eq!(node_txn[2].input.len(), 1); - node_txn[1].clone() + node_txn[0].clone() }; // Broadcast partial claim on node A, should regenerate a claiming tx with HTLC dropped diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index d0d1f29e691..2270cdb7c66 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -52,7 +52,7 @@ enum OnchainEvent { pub struct ClaimTxBumpMaterial { // At every block tick, used to check if pending claiming tx is taking too // much time for confirmation and we need to bump it. - height_timer: u32, + height_timer: Option, // Tracked in case of reorg to wipe out now-superflous bump material feerate_previous: u64, // Soonest timelocks among set of outpoints claimed, used to compute @@ -64,7 +64,7 @@ pub struct ClaimTxBumpMaterial { impl Writeable for ClaimTxBumpMaterial { fn write(&self, writer: &mut W) -> Result<(), ::std::io::Error> { - writer.write_all(&byte_utils::be32_to_array(self.height_timer))?; + self.height_timer.write(writer)?; writer.write_all(&byte_utils::be64_to_array(self.feerate_previous))?; writer.write_all(&byte_utils::be32_to_array(self.soonest_timelock))?; writer.write_all(&byte_utils::be64_to_array(self.per_input_material.len() as u64))?; @@ -357,7 +357,7 @@ impl OnchainTxHandler { /// Lightning security model (i.e being able to redeem/timeout HTLC or penalize coutnerparty onchain) lays on the assumption of claim transactions getting confirmed before timelock expiration /// (CSV or CLTV following cases). In case of high-fee spikes, claim tx may stuck in the mempool, so you need to bump its feerate quickly using Replace-By-Fee or Child-Pay-For-Parent. - fn generate_claim_tx(&self, height: u32, cached_claim_datas: &ClaimTxBumpMaterial, fee_estimator: F) -> Option<(u32, u64, Transaction)> + fn generate_claim_tx(&self, height: u32, cached_claim_datas: &ClaimTxBumpMaterial, fee_estimator: F) -> Option<(Option, u64, Transaction)> where F::Target: FeeEstimator { if cached_claim_datas.per_input_material.len() == 0 { return None } // But don't prune pending claiming request yet, we may have to resurrect HTLCs @@ -422,9 +422,10 @@ impl OnchainTxHandler { // Compute new height timer to decide when we need to regenerate a new bumped version of the claim tx (if we // didn't receive confirmation of it before, or not enough reorg-safe depth on top of it). - let new_timer = Self::get_height_timer(height, cached_claim_datas.soonest_timelock); + let new_timer = Some(Self::get_height_timer(height, cached_claim_datas.soonest_timelock)); let mut inputs_witnesses_weight = 0; let mut amt = 0; + let mut dynamic_fee = true; for per_outp_material in cached_claim_datas.per_input_material.values() { match per_outp_material { &InputMaterial::Revoked { ref witness_script, ref is_htlc, ref amount, .. } => { @@ -436,71 +437,99 @@ impl OnchainTxHandler { amt += *amount; }, &InputMaterial::LocalHTLC { .. } => { return None; } - } - } - - let predicted_weight = bumped_tx.get_weight() + inputs_witnesses_weight; - let mut new_feerate; - // If old feerate is 0, first iteration of this claim, use normal fee calculation - if cached_claim_datas.feerate_previous != 0 { - if let Some((new_fee, feerate)) = RBF_bump!(amt, cached_claim_datas.feerate_previous, fee_estimator, predicted_weight as u64) { - // If new computed fee is superior at the whole claimable amount burn all in fees - if new_fee > amt { - bumped_tx.output[0].value = 0; - } else { - bumped_tx.output[0].value = amt - new_fee; + &InputMaterial::Funding { .. } => { + dynamic_fee = false; } - new_feerate = feerate; - } else { return None; } - } else { - if subtract_high_prio_fee!(self, fee_estimator, amt, predicted_weight, new_feerate) { - bumped_tx.output[0].value = amt; - } else { return None; } + } } - assert!(new_feerate != 0); - for (i, (outp, per_outp_material)) in cached_claim_datas.per_input_material.iter().enumerate() { - match per_outp_material { - &InputMaterial::Revoked { ref witness_script, ref pubkey, ref key, ref is_htlc, ref amount } => { - let sighash_parts = bip143::SighashComponents::new(&bumped_tx); - let sighash = hash_to_message!(&sighash_parts.sighash_all(&bumped_tx.input[i], &witness_script, *amount)[..]); - let sig = self.secp_ctx.sign(&sighash, &key); - bumped_tx.input[i].witness.push(sig.serialize_der().to_vec()); - bumped_tx.input[i].witness[0].push(SigHashType::All as u8); - if *is_htlc { - bumped_tx.input[i].witness.push(pubkey.unwrap().clone().serialize().to_vec()); + if dynamic_fee { + let predicted_weight = bumped_tx.get_weight() + inputs_witnesses_weight; + let mut new_feerate; + // If old feerate is 0, first iteration of this claim, use normal fee calculation + if cached_claim_datas.feerate_previous != 0 { + if let Some((new_fee, feerate)) = RBF_bump!(amt, cached_claim_datas.feerate_previous, fee_estimator, predicted_weight as u64) { + // If new computed fee is superior at the whole claimable amount burn all in fees + if new_fee > amt { + bumped_tx.output[0].value = 0; } else { - bumped_tx.input[i].witness.push(vec!(1)); + bumped_tx.output[0].value = amt - new_fee; } - bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); - log_trace!(self, "Going to broadcast Penalty Transaction {} claiming revoked {} output {} from {} with new feerate {}...", bumped_tx.txid(), if !is_htlc { "to_local" } else if HTLCType::scriptlen_to_htlctype(witness_script.len()) == Some(HTLCType::OfferedHTLC) { "offered" } else if HTLCType::scriptlen_to_htlctype(witness_script.len()) == Some(HTLCType::AcceptedHTLC) { "received" } else { "" }, outp.vout, outp.txid, new_feerate); - }, - &InputMaterial::RemoteHTLC { ref witness_script, ref key, ref preimage, ref amount, ref locktime } => { - if !preimage.is_some() { bumped_tx.lock_time = *locktime }; // Right now we don't aggregate time-locked transaction, if we do we should set lock_time before to avoid breaking hash computation - let sighash_parts = bip143::SighashComponents::new(&bumped_tx); - let sighash = hash_to_message!(&sighash_parts.sighash_all(&bumped_tx.input[i], &witness_script, *amount)[..]); - let sig = self.secp_ctx.sign(&sighash, &key); - 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.clone().0.to_vec()); - } else { - bumped_tx.input[i].witness.push(vec![]); + new_feerate = feerate; + } else { return None; } + } else { + if subtract_high_prio_fee!(self, fee_estimator, amt, predicted_weight, new_feerate) { + bumped_tx.output[0].value = amt; + } else { return None; } + } + assert!(new_feerate != 0); + + for (i, (outp, per_outp_material)) in cached_claim_datas.per_input_material.iter().enumerate() { + match per_outp_material { + &InputMaterial::Revoked { ref witness_script, ref pubkey, ref key, ref is_htlc, ref amount } => { + let sighash_parts = bip143::SighashComponents::new(&bumped_tx); + let sighash = hash_to_message!(&sighash_parts.sighash_all(&bumped_tx.input[i], &witness_script, *amount)[..]); + let sig = self.secp_ctx.sign(&sighash, &key); + bumped_tx.input[i].witness.push(sig.serialize_der().to_vec()); + bumped_tx.input[i].witness[0].push(SigHashType::All as u8); + if *is_htlc { + bumped_tx.input[i].witness.push(pubkey.unwrap().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!(self, "Going to broadcast Penalty Transaction {} claiming revoked {} output {} from {} with new feerate {}...", bumped_tx.txid(), if !is_htlc { "to_local" } else if HTLCType::scriptlen_to_htlctype(witness_script.len()) == Some(HTLCType::OfferedHTLC) { "offered" } else if HTLCType::scriptlen_to_htlctype(witness_script.len()) == Some(HTLCType::AcceptedHTLC) { "received" } else { "" }, outp.vout, outp.txid, new_feerate); + }, + &InputMaterial::RemoteHTLC { ref witness_script, ref key, ref preimage, ref amount, ref locktime } => { + if !preimage.is_some() { bumped_tx.lock_time = *locktime }; // Right now we don't aggregate time-locked transaction, if we do we should set lock_time before to avoid breaking hash computation + let sighash_parts = bip143::SighashComponents::new(&bumped_tx); + let sighash = hash_to_message!(&sighash_parts.sighash_all(&bumped_tx.input[i], &witness_script, *amount)[..]); + let sig = self.secp_ctx.sign(&sighash, &key); + 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.clone().0.to_vec()); + } else { + bumped_tx.input[i].witness.push(vec![]); + } + bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); + log_trace!(self, "Going to broadcast Claim Transaction {} claiming remote {} htlc output {} from {} with new feerate {}...", bumped_tx.txid(), if preimage.is_some() { "offered" } else { "received" }, outp.vout, outp.txid, new_feerate); + }, + _ => unreachable!() + } + } + log_trace!(self, "...with timer {}", new_timer.unwrap()); + assert!(predicted_weight >= bumped_tx.get_weight()); + return Some((new_timer, new_feerate, bumped_tx)) + } else { + for (_, (outp, per_outp_material)) in cached_claim_datas.per_input_material.iter().enumerate() { + match per_outp_material { + &InputMaterial::LocalHTLC { .. } => { + //TODO : Given that Local Commitment Transaction and HTLC-Timeout/HTLC-Success are counter-signed by peer, we can't + // RBF them. Need a Lightning specs change and package relay modification : + // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html + return None; + }, + &InputMaterial::Funding { ref channel_value } => { + if self.local_commitment.is_some() { + let mut local_commitment = self.local_commitment.clone().unwrap(); + self.key_storage.sign_local_commitment(&mut local_commitment, &self.funding_redeemscript, *channel_value, &self.secp_ctx); + let signed_tx = local_commitment.with_valid_witness().clone(); + let mut amt_outputs = 0; + for outp in signed_tx.output.iter() { + amt_outputs += outp.value; + } + let feerate = (channel_value - amt_outputs) * 1000 / signed_tx.get_weight() as u64; + // Timer set to $NEVER given we can't bump tx without anchor outputs + log_trace!(self, "Going to broadcast Local Transaction {} claiming funding output {} from {}...", signed_tx.txid(), outp.vout, outp.txid); + return Some((None, feerate, signed_tx)); + } } - bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); - log_trace!(self, "Going to broadcast Claim Transaction {} claiming remote {} htlc output {} from {} with new feerate {}...", bumped_tx.txid(), if preimage.is_some() { "offered" } else { "received" }, outp.vout, outp.txid, new_feerate); - }, - &InputMaterial::LocalHTLC { .. } => { - //TODO : Given that Local Commitment Transaction and HTLC-Timeout/HTLC-Success are counter-signed by peer, we can't - // RBF them. Need a Lightning specs change and package relay modification : - // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html - return None; + _ => unreachable!() } } } - log_trace!(self, "...with timer {}", new_timer); - assert!(predicted_weight >= bumped_tx.get_weight()); - Some((new_timer, new_feerate, bumped_tx)) + None } pub(super) fn block_connected(&mut self, txn_matched: &[&Transaction], claimable_outpoints: Vec, height: u32, broadcaster: B, fee_estimator: F) @@ -535,7 +564,7 @@ impl OnchainTxHandler { // Generate claim transactions and track them to bump if necessary at // height timer expiration (i.e in how many blocks we're going to take action). for claim in new_claims { - let mut claim_material = ClaimTxBumpMaterial { height_timer: 0, feerate_previous: 0, soonest_timelock: claim.0, per_input_material: claim.1.clone() }; + let mut claim_material = ClaimTxBumpMaterial { height_timer: None, feerate_previous: 0, soonest_timelock: claim.0, per_input_material: claim.1.clone() }; if let Some((new_timer, new_feerate, tx)) = self.generate_claim_tx(height, &claim_material, &*fee_estimator) { claim_material.height_timer = new_timer; claim_material.feerate_previous = new_feerate; @@ -653,8 +682,10 @@ impl OnchainTxHandler { // Check if any pending claim request must be rescheduled for (first_claim_txid, ref claim_data) in self.pending_claim_requests.iter() { - if claim_data.height_timer == height { - bump_candidates.insert(*first_claim_txid); + if let Some(h) = claim_data.height_timer { + if h == height { + bump_candidates.insert(*first_claim_txid); + } } } From 493ffb81e197fefe11f821f19c15a0eac8fbd3d6 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Tue, 24 Mar 2020 19:26:28 -0400 Subject: [PATCH 07/19] Cache current local commitment number in ChannelMonitor. By caching current local commitment number instead of deciphering it from local commitment tx, we may remove local commitment tx from ChannelMonitor in next commit. --- lightning/src/ln/channelmonitor.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index b3cff4c2b42..ea95f2293c6 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -780,6 +780,9 @@ pub struct ChannelMonitor { // Used just for ChannelManager to make sure it has the latest channel data during // deserialization current_remote_commitment_number: u64, + // Used just for ChannelManager to make sure it has the latest channel data during + // deserialization + current_local_commitment_number: u64, payment_preimages: HashMap, @@ -836,6 +839,7 @@ impl PartialEq for ChannelMonitor { self.remote_hash_commitment_number != other.remote_hash_commitment_number || self.prev_local_signed_commitment_tx != other.prev_local_signed_commitment_tx || self.current_remote_commitment_number != other.current_remote_commitment_number || + self.current_local_commitment_number != other.current_local_commitment_number || self.current_local_signed_commitment_tx != other.current_local_signed_commitment_tx || self.payment_preimages != other.payment_preimages || self.pending_htlcs_updated != other.pending_htlcs_updated || @@ -1008,6 +1012,12 @@ impl ChannelMonitor { writer.write_all(&byte_utils::be48_to_array(0))?; } + if for_local_storage { + writer.write_all(&byte_utils::be48_to_array(self.current_local_commitment_number))?; + } else { + writer.write_all(&byte_utils::be48_to_array(0))?; + } + writer.write_all(&byte_utils::be64_to_array(self.payment_preimages.len() as u64))?; for payment_preimage in self.payment_preimages.values() { writer.write_all(&payment_preimage.0[..])?; @@ -1126,6 +1136,7 @@ impl ChannelMonitor { prev_local_signed_commitment_tx: None, current_local_signed_commitment_tx: None, current_remote_commitment_number: 1 << 48, + current_local_commitment_number: 0xffff_ffff_ffff, payment_preimages: HashMap::new(), pending_htlcs_updated: Vec::new(), @@ -1262,6 +1273,7 @@ impl ChannelMonitor { if let Err(_) = self.onchain_tx_handler.provide_latest_local_tx(commitment_tx.clone()) { return Err(MonitorUpdateError("Local commitment signed has already been signed, no further update of LOCAL commitment transaction is allowed")); } + self.current_local_commitment_number = 0xffff_ffff_ffff - ((((commitment_tx.without_valid_witness().input[0].sequence as u64 & 0xffffff) << 3*8) | (commitment_tx.without_valid_witness().lock_time as u64 & 0xffffff)) ^ self.commitment_transaction_number_obscure_factor); self.prev_local_signed_commitment_tx = self.current_local_signed_commitment_tx.take(); self.current_local_signed_commitment_tx = Some(LocalSignedTx { txid: commitment_tx.txid(), @@ -1415,9 +1427,7 @@ impl ChannelMonitor { } pub(super) fn get_cur_local_commitment_number(&self) -> u64 { - if let &Some(ref local_tx) = &self.current_local_signed_commitment_tx { - 0xffff_ffff_ffff - ((((local_tx.tx.without_valid_witness().input[0].sequence as u64 & 0xffffff) << 3*8) | (local_tx.tx.without_valid_witness().lock_time as u64 & 0xffffff)) ^ self.commitment_transaction_number_obscure_factor) - } else { 0xffff_ffff_ffff } + self.current_local_commitment_number } /// Attempts to claim a remote commitment transaction's outputs using the revocation key and @@ -2415,6 +2425,7 @@ impl ReadableArgs> for (Sha256dH }; let current_remote_commitment_number = ::read(reader)?.0; + let current_local_commitment_number = ::read(reader)?.0; let payment_preimages_len: u64 = Readable::read(reader)?; let mut payment_preimages = HashMap::with_capacity(cmp::min(payment_preimages_len as usize, MAX_ALLOC_SIZE / 32)); @@ -2512,6 +2523,7 @@ impl ReadableArgs> for (Sha256dH prev_local_signed_commitment_tx, current_local_signed_commitment_tx, current_remote_commitment_number, + current_local_commitment_number, payment_preimages, pending_htlcs_updated, From 3cb61e979c21cd86b298202e6a716983c7349bab Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Fri, 20 Mar 2020 22:41:12 -0400 Subject: [PATCH 08/19] Access signed local commitment through OnchainTxHandler Implementing dynamic fee bumping implied to cache transaction material including its witness, to generate a bumped version if needed. ChannelMonitor is slowly rescoped to its parsing function with ongoing patchset and data duplicata are removed. If signed local commitment tx access is needed, it's done through OnchainTxHandler extended API For test framework purpose, we use the test-only method ChannelMonitor::unsafe_get_latest_local_commitment_txn to intentionally generate unsafe local commitment to exerce revocation logic. --- lightning/src/ln/channelmonitor.rs | 67 ++++++++++++++--------- lightning/src/ln/functional_test_utils.rs | 2 +- lightning/src/ln/onchaintx.rs | 18 ++++++ 3 files changed, 60 insertions(+), 27 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index ea95f2293c6..4bdc566745c 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1687,7 +1687,7 @@ impl ChannelMonitor { (claimable_outpoints, Some((htlc_txid, tx.output.clone()))) } - fn broadcast_by_local_state(&self, local_tx: &LocalSignedTx) -> (Vec, Vec, Option<(Script, SecretKey, Script)>) { + fn broadcast_by_local_state(&self, commitment_tx: &Transaction, local_tx: &LocalSignedTx) -> (Vec, Vec, Option<(Script, SecretKey, Script)>) { let mut res = Vec::with_capacity(local_tx.htlc_outputs.len()); let mut watch_outputs = Vec::with_capacity(local_tx.htlc_outputs.len()); @@ -1730,7 +1730,7 @@ impl ChannelMonitor { res.push(htlc_success_tx); } } - watch_outputs.push(local_tx.tx.without_valid_witness().output[transaction_output_index as usize].clone()); + watch_outputs.push(commitment_tx.output[transaction_output_index as usize].clone()); } else { panic!("Should have sigs for non-dust local tx outputs!") } } } @@ -1784,7 +1784,7 @@ impl ChannelMonitor { if local_tx.txid == commitment_txid { is_local_tx = true; log_trace!(self, "Got latest local commitment tx broadcast, searching for available HTLCs to claim"); - let mut res = self.broadcast_by_local_state(local_tx); + let mut res = self.broadcast_by_local_state(tx, local_tx); append_onchain_update!(res); } } @@ -1792,8 +1792,7 @@ impl ChannelMonitor { if local_tx.txid == commitment_txid { is_local_tx = true; log_trace!(self, "Got previous local commitment tx broadcast, searching for available HTLCs to claim"); - assert!(local_tx.tx.has_local_sig()); - let mut res = self.broadcast_by_local_state(local_tx); + let mut res = self.broadcast_by_local_state(tx, local_tx); append_onchain_update!(res); } } @@ -1832,22 +1831,35 @@ impl ChannelMonitor { /// out-of-band the other node operator to coordinate with him if option is available to you. /// In any-case, choice is up to the user. pub fn get_latest_local_commitment_txn(&mut self) -> Vec { - // TODO: We should likely move all of the logic in here into OnChainTxHandler and unify it - // to ensure add_local_sig is only ever called once no matter what. This likely includes - // tracking state and panic!()ing if we get an update after force-closure/local-tx signing. log_trace!(self, "Getting signed latest local commitment transaction!"); - if let &mut Some(ref mut local_tx) = &mut self.current_local_signed_commitment_tx { - self.onchain_detection.keys.sign_local_commitment(&mut local_tx.tx, self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &self.secp_ctx); + if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(self.channel_value_satoshis.unwrap()) { + let mut res = vec![commitment_tx]; + if let &Some(ref local_tx) = &self.current_local_signed_commitment_tx { + let mut htlc_txn = self.broadcast_by_local_state(res.get(0).unwrap(), local_tx).0; + res.append(&mut htlc_txn); + // We throw away the generated waiting_first_conf data as we aren't (yet) confirmed and we don't actually know what the caller wants to do. + // The data will be re-generated and tracked in check_spend_local_transaction if we get a confirmation. + } + return res } - if let &Some(ref local_tx) = &self.current_local_signed_commitment_tx { - let mut res = vec![local_tx.tx.with_valid_witness().clone()]; - res.append(&mut self.broadcast_by_local_state(local_tx).0); - // We throw away the generated waiting_first_conf data as we aren't (yet) confirmed and we don't actually know what the caller wants to do. - // The data will be re-generated and tracked in check_spend_local_transaction if we get a confirmation. - res - } else { - Vec::new() + Vec::new() + } + + /// Unsafe test-only version of get_latest_local_commitment_txn used by our test framework + /// to bypass LocalCommitmentTransaction state update lockdown after signature and generate + /// revoked commitment transaction. + #[cfg(test)] + pub fn unsafe_get_latest_local_commitment_txn(&mut self) -> Vec { + log_trace!(self, "Getting signed copy of latest local commitment transaction!"); + if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_copy_local_tx(self.channel_value_satoshis.unwrap()) { + let mut res = vec![commitment_tx]; + if let &Some(ref local_tx) = &self.current_local_signed_commitment_tx { + let mut htlc_txn = self.broadcast_by_local_state(res.get(0).unwrap(), local_tx).0; + res.append(&mut htlc_txn); + } + return res } + Vec::new() } /// Called by SimpleManyChannelMonitor::block_connected, which implements @@ -1922,13 +1934,15 @@ impl ChannelMonitor { } if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx { if should_broadcast { - let (txs, new_outputs, _) = self.broadcast_by_local_state(&cur_local_tx); - if !new_outputs.is_empty() { - watch_outputs.push((cur_local_tx.txid.clone(), new_outputs)); - } - for tx in txs { - log_trace!(self, "Broadcast onchain {}", log_tx!(tx)); - broadcaster.broadcast_transaction(&tx); + if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(self.channel_value_satoshis.unwrap()) { + let (txs, new_outputs, _) = self.broadcast_by_local_state(&commitment_tx, cur_local_tx); + if !new_outputs.is_empty() { + watch_outputs.push((cur_local_tx.txid.clone(), new_outputs)); + } + for tx in txs { + log_trace!(self, "Broadcast onchain {}", log_tx!(tx)); + broadcaster.broadcast_transaction(&tx); + } } } } @@ -2401,7 +2415,8 @@ impl ReadableArgs> for (Sha256dH LocalSignedTx { txid: tx.txid(), - tx, revocation_key, a_htlc_key, b_htlc_key, delayed_payment_key, per_commitment_point, feerate_per_kw, + tx, + revocation_key, a_htlc_key, b_htlc_key, delayed_payment_key, per_commitment_point, feerate_per_kw, htlc_outputs: htlcs } } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 2e071c2b78b..a2f9a7c3239 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -264,7 +264,7 @@ macro_rules! get_local_commitment_txn { let mut commitment_txn = None; for (funding_txo, monitor) in monitors.iter_mut() { if funding_txo.to_channel_id() == $channel_id { - commitment_txn = Some(monitor.get_latest_local_commitment_txn()); + commitment_txn = Some(monitor.unsafe_get_latest_local_commitment_txn()); break; } } diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 2270cdb7c66..68c71dd8883 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -769,4 +769,22 @@ impl OnchainTxHandler { self.local_commitment = Some(tx); Ok(()) } + + pub(super) fn get_fully_signed_local_tx(&mut self, channel_value_satoshis: u64) -> Option { + if let Some(ref mut local_commitment) = self.local_commitment { + self.key_storage.sign_local_commitment(local_commitment, &self.funding_redeemscript, channel_value_satoshis, &self.secp_ctx); + return Some(local_commitment.with_valid_witness().clone()); + } + None + } + + #[cfg(test)] + pub(super) fn get_fully_signed_copy_local_tx(&mut self, channel_value_satoshis: u64) -> Option { + if let Some(ref mut local_commitment) = self.local_commitment { + let mut local_commitment = local_commitment.clone(); + self.key_storage.unsafe_sign_local_commitment(&mut local_commitment, &self.funding_redeemscript, channel_value_satoshis, &self.secp_ctx); + return Some(local_commitment.with_valid_witness().clone()); + } + None + } } From 7e395e0265c8cad41dd23ba4d6804c158cbba99e Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Sat, 21 Mar 2020 15:48:11 -0400 Subject: [PATCH 09/19] Remove per_input_material introduce in commit 0011713 Caching of input material for HTLC transaction was introducted prevently but since then API (InputMaterial) has changed between ChannelMonitor and OnchainTxHandler --- lightning/src/ln/channelmonitor.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 4bdc566745c..fa5e359027a 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1708,9 +1708,6 @@ impl ChannelMonitor { Err(_) => continue, }; - let mut per_input_material = HashMap::with_capacity(1); - per_input_material.insert(htlc_timeout_tx.input[0].previous_output, InputMaterial::LocalHTLC { witness_script: htlc_script, sigs: (*their_sig, our_sig), preimage: None, amount: htlc.amount_msat / 1000}); - //TODO: with option_simplified_commitment track outpoint too log_trace!(self, "Outpoint {}:{} is being being claimed", htlc_timeout_tx.input[0].previous_output.vout, htlc_timeout_tx.input[0].previous_output.txid); res.push(htlc_timeout_tx); } else { @@ -1723,9 +1720,6 @@ impl ChannelMonitor { Err(_) => continue, }; - let mut per_input_material = HashMap::with_capacity(1); - per_input_material.insert(htlc_success_tx.input[0].previous_output, InputMaterial::LocalHTLC { witness_script: htlc_script, sigs: (*their_sig, our_sig), preimage: Some(*payment_preimage), amount: htlc.amount_msat / 1000}); - //TODO: with option_simplified_commitment track outpoint too log_trace!(self, "Outpoint {}:{} is being being claimed", htlc_success_tx.input[0].previous_output.vout, htlc_success_tx.input[0].previous_output.txid); res.push(htlc_success_tx); } From c2347d61b442e6ea78b48a18d04dda7171a6c6e8 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Sat, 21 Mar 2020 15:39:19 -0400 Subject: [PATCH 10/19] Remove signing htlc transaction from ChannelMonitor Extend external signer interface to sign HTLC transactions on its behalf without seckey passing. This move will allow us to remove key access access from ChannelMonitor hot memory in further work. HTLC transactions should stay half-signed by remote until we need to broadcast them for timing-out/claiming HTLCs onchain. --- lightning/src/chain/keysinterface.rs | 42 ++++++++++++++++++++- lightning/src/ln/chan_utils.rs | 41 +------------------- lightning/src/ln/channel.rs | 2 +- lightning/src/ln/channelmonitor.rs | 12 +----- lightning/src/util/enforcing_trait_impls.rs | 5 +++ 5 files changed, 51 insertions(+), 51 deletions(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 5f1a307bf27..e3e2d929447 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -2,7 +2,7 @@ //! spendable on-chain outputs which the user owns and is responsible for using just as any other //! on-chain output which is theirs. -use bitcoin::blockdata::transaction::{Transaction, OutPoint, TxOut}; +use bitcoin::blockdata::transaction::{Transaction, OutPoint, TxOut, SigHashType}; use bitcoin::blockdata::script::{Script, Builder}; use bitcoin::blockdata::opcodes; use bitcoin::network::constants::Network; @@ -25,6 +25,7 @@ use util::ser::{Writeable, Writer, Readable}; use ln::chan_utils; use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys, LocalCommitmentTransaction}; +use ln::channelmanager::PaymentPreimage; use ln::msgs; use std::sync::Arc; @@ -231,6 +232,11 @@ pub trait ChannelKeys : Send+Clone { #[cfg(test)] fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1); + /// Signs a transaction created by build_htlc_transaction. If the transaction is an + /// HTLC-Success transaction, preimage must be set! + /// TODO: should be merged with sign_local_commitment as a slice of HTLC transactions to sign + fn sign_htlc_transaction(&self, htlc_tx: &mut Transaction, their_sig: &Signature, preimage: &Option, htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey, per_commitment_point: &PublicKey, secp_ctx: &Secp256k1); + /// Create a signature for a (proposed) closing transaction. /// /// Note that, due to rounding, there may be one "missing" satoshi, and either party may have @@ -367,6 +373,40 @@ impl ChannelKeys for InMemoryChannelKeys { local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx); } + fn sign_htlc_transaction(&self, htlc_tx: &mut Transaction, their_sig: &Signature, preimage: &Option, htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey, per_commitment_point: &PublicKey, secp_ctx: &Secp256k1) { + if htlc_tx.input.len() != 1 { return; } + if htlc_tx.input[0].witness.len() != 0 { return; } + + let htlc_redeemscript = chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, a_htlc_key, b_htlc_key, revocation_key); + + if let Ok(our_htlc_key) = chan_utils::derive_private_key(secp_ctx, per_commitment_point, &self.htlc_base_key) { + let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]); + let local_tx = PublicKey::from_secret_key(&secp_ctx, &our_htlc_key) == *a_htlc_key; + let our_sig = secp_ctx.sign(&sighash, &our_htlc_key); + + htlc_tx.input[0].witness.push(Vec::new()); // First is the multisig dummy + + if local_tx { // b, then a + htlc_tx.input[0].witness.push(their_sig.serialize_der().to_vec()); + htlc_tx.input[0].witness.push(our_sig.serialize_der().to_vec()); + } else { + htlc_tx.input[0].witness.push(our_sig.serialize_der().to_vec()); + htlc_tx.input[0].witness.push(their_sig.serialize_der().to_vec()); + } + htlc_tx.input[0].witness[1].push(SigHashType::All as u8); + htlc_tx.input[0].witness[2].push(SigHashType::All as u8); + + if htlc.offered { + htlc_tx.input[0].witness.push(Vec::new()); + assert!(preimage.is_none()); + } else { + htlc_tx.input[0].witness.push(preimage.unwrap().0.to_vec()); + } + + htlc_tx.input[0].witness.push(htlc_redeemscript.as_bytes().to_vec()); + } else { return; } + } + fn sign_closing_transaction(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1) -> Result { if closing_tx.input.len() != 1 { return Err(()); } if closing_tx.input[0].witness.len() != 0 { return Err(()); } diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index a5e0c1460be..4be3b3151df 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -14,7 +14,7 @@ use bitcoin_hashes::ripemd160::Hash as Ripemd160; use bitcoin_hashes::hash160::Hash as Hash160; use bitcoin_hashes::sha256d::Hash as Sha256dHash; -use ln::channelmanager::{PaymentHash, PaymentPreimage}; +use ln::channelmanager::PaymentHash; use ln::msgs::DecodeError; use util::ser::{Readable, Writeable, Writer, WriterWriteAdaptor}; use util::byte_utils; @@ -355,7 +355,7 @@ impl_writeable!(HTLCOutputInCommitment, 1 + 8 + 4 + 32 + 5, { }); #[inline] -pub(super) fn get_htlc_redeemscript_with_explicit_keys(htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey) -> Script { +pub(crate) fn get_htlc_redeemscript_with_explicit_keys(htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey) -> Script { let payment_hash160 = Ripemd160::hash(&htlc.payment_hash.0[..]).into_inner(); if htlc.offered { Builder::new().push_opcode(opcodes::all::OP_DUP) @@ -475,43 +475,6 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s } } -/// Signs a transaction created by build_htlc_transaction. If the transaction is an -/// HTLC-Success transaction (ie htlc.offered is false), preimage must be set! -pub(crate) fn sign_htlc_transaction(tx: &mut Transaction, their_sig: &Signature, preimage: &Option, htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey, per_commitment_point: &PublicKey, htlc_base_key: &SecretKey, secp_ctx: &Secp256k1) -> Result<(Signature, Script), ()> { - if tx.input.len() != 1 { return Err(()); } - if tx.input[0].witness.len() != 0 { return Err(()); } - - let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&htlc, a_htlc_key, b_htlc_key, revocation_key); - - let our_htlc_key = derive_private_key(secp_ctx, per_commitment_point, htlc_base_key).map_err(|_| ())?; - let sighash = hash_to_message!(&bip143::SighashComponents::new(&tx).sighash_all(&tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]); - let local_tx = PublicKey::from_secret_key(&secp_ctx, &our_htlc_key) == *a_htlc_key; - let our_sig = secp_ctx.sign(&sighash, &our_htlc_key); - - tx.input[0].witness.push(Vec::new()); // First is the multisig dummy - - if local_tx { // b, then a - tx.input[0].witness.push(their_sig.serialize_der().to_vec()); - tx.input[0].witness.push(our_sig.serialize_der().to_vec()); - } else { - tx.input[0].witness.push(our_sig.serialize_der().to_vec()); - tx.input[0].witness.push(their_sig.serialize_der().to_vec()); - } - tx.input[0].witness[1].push(SigHashType::All as u8); - tx.input[0].witness[2].push(SigHashType::All as u8); - - if htlc.offered { - tx.input[0].witness.push(Vec::new()); - assert!(preimage.is_none()); - } else { - tx.input[0].witness.push(preimage.unwrap().0.to_vec()); - } - - tx.input[0].witness.push(htlc_redeemscript.as_bytes().to_vec()); - - Ok((our_sig, htlc_redeemscript)) -} - #[derive(Clone)] /// We use this to track local commitment transactions and put off signing them until we are ready /// to broadcast. Eventually this will require a signer which is possibly external, but for now we diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 43aba4201f4..81528001776 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4519,7 +4519,7 @@ mod tests { assert!(preimage.is_some()); } - chan_utils::sign_htlc_transaction(&mut htlc_tx, &remote_signature, &preimage, &htlc, &keys.a_htlc_key, &keys.b_htlc_key, &keys.revocation_key, &keys.per_commitment_point, chan.local_keys.htlc_base_key(), &chan.secp_ctx).unwrap(); + chan_keys.sign_htlc_transaction(&mut htlc_tx, &remote_signature, &preimage, &htlc, &keys.a_htlc_key, &keys.b_htlc_key, &keys.revocation_key, &keys.per_commitment_point, &chan.secp_ctx); assert_eq!(serialize(&htlc_tx)[..], hex::decode($tx_hex).unwrap()[..]); }; diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index fa5e359027a..560ace94b42 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1702,11 +1702,7 @@ impl ChannelMonitor { if htlc.offered { log_trace!(self, "Broadcasting HTLC-Timeout transaction against local commitment transactions"); let mut htlc_timeout_tx = chan_utils::build_htlc_transaction(&local_tx.txid, local_tx.feerate_per_kw, self.their_to_self_delay.unwrap(), htlc, &local_tx.delayed_payment_key, &local_tx.revocation_key); - let (our_sig, htlc_script) = match - chan_utils::sign_htlc_transaction(&mut htlc_timeout_tx, their_sig, &None, htlc, &local_tx.a_htlc_key, &local_tx.b_htlc_key, &local_tx.revocation_key, &local_tx.per_commitment_point, &self.onchain_detection.keys.htlc_base_key(), &self.secp_ctx) { - Ok(res) => res, - Err(_) => continue, - }; + self.onchain_detection.keys.sign_htlc_transaction(&mut htlc_timeout_tx, their_sig, &None, htlc, &local_tx.a_htlc_key, &local_tx.b_htlc_key, &local_tx.revocation_key, &local_tx.per_commitment_point, &self.secp_ctx); log_trace!(self, "Outpoint {}:{} is being being claimed", htlc_timeout_tx.input[0].previous_output.vout, htlc_timeout_tx.input[0].previous_output.txid); res.push(htlc_timeout_tx); @@ -1714,11 +1710,7 @@ impl ChannelMonitor { if let Some(payment_preimage) = self.payment_preimages.get(&htlc.payment_hash) { log_trace!(self, "Broadcasting HTLC-Success transaction against local commitment transactions"); let mut htlc_success_tx = chan_utils::build_htlc_transaction(&local_tx.txid, local_tx.feerate_per_kw, self.their_to_self_delay.unwrap(), htlc, &local_tx.delayed_payment_key, &local_tx.revocation_key); - let (our_sig, htlc_script) = match - chan_utils::sign_htlc_transaction(&mut htlc_success_tx, their_sig, &Some(*payment_preimage), htlc, &local_tx.a_htlc_key, &local_tx.b_htlc_key, &local_tx.revocation_key, &local_tx.per_commitment_point, &self.onchain_detection.keys.htlc_base_key(), &self.secp_ctx) { - Ok(res) => res, - Err(_) => continue, - }; + self.onchain_detection.keys.sign_htlc_transaction(&mut htlc_success_tx, their_sig, &Some(*payment_preimage), htlc, &local_tx.a_htlc_key, &local_tx.b_htlc_key, &local_tx.revocation_key, &local_tx.per_commitment_point, &self.secp_ctx); log_trace!(self, "Outpoint {}:{} is being being claimed", htlc_success_tx.input[0].previous_output.vout, htlc_success_tx.input[0].previous_output.txid); res.push(htlc_success_tx); diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index bc3a1a0da15..5e3aba43193 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -1,4 +1,5 @@ use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys, LocalCommitmentTransaction}; +use ln::channelmanager::PaymentPreimage; use ln::msgs; use chain::keysinterface::{ChannelKeys, InMemoryChannelKeys}; @@ -88,6 +89,10 @@ impl ChannelKeys for EnforcingChannelKeys { self.inner.unsafe_sign_local_commitment(local_commitment_tx, funding_redeemscript, channel_value_satoshis, secp_ctx); } + fn sign_htlc_transaction(&self, htlc_tx: &mut Transaction, their_sig: &Signature, preimage: &Option, htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey, per_commitment_point: &PublicKey, secp_ctx: &Secp256k1) { + self.inner.sign_htlc_transaction(htlc_tx, their_sig, preimage, htlc, a_htlc_key, b_htlc_key, revocation_key, per_commitment_point, secp_ctx); + } + fn sign_closing_transaction(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1) -> Result { Ok(self.inner.sign_closing_transaction(closing_tx, secp_ctx).unwrap()) } From 010fb3051c7df052df6d487d57309cd750eb0e2d Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Tue, 7 Apr 2020 18:46:14 -0400 Subject: [PATCH 11/19] Cache HTLC transaction material inside OnchainTxHandler Splitting further parsing from transaction generation, we cache transaction elements needed for local HTLC transaction inside OnchainTxHandler. Duplicated data will be removed from ChannelMonitor in future commits. --- lightning/src/ln/channelmonitor.rs | 2 +- lightning/src/ln/onchaintx.rs | 74 +++++++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 560ace94b42..b0b31193156 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1270,7 +1270,7 @@ impl ChannelMonitor { // reject update as we do, you MAY have the latest local valid commitment tx onchain // for which you want to spend outputs. We're NOT robust again this scenario right // now but we should consider it later. - if let Err(_) = self.onchain_tx_handler.provide_latest_local_tx(commitment_tx.clone()) { + if let Err(_) = self.onchain_tx_handler.provide_latest_local_tx(commitment_tx.clone(), local_keys.clone(), feerate_per_kw) { return Err(MonitorUpdateError("Local commitment signed has already been signed, no further update of LOCAL commitment transaction is allowed")); } self.current_local_commitment_number = 0xffff_ffff_ffff - ((((commitment_tx.without_valid_witness().input[0].sequence as u64 & 0xffffff) << 3*8) | (commitment_tx.without_valid_witness().lock_time as u64 & 0xffffff)) ^ self.commitment_transaction_number_obscure_factor); diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 68c71dd8883..43d3ec9a639 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -15,7 +15,8 @@ use secp256k1; use ln::msgs::DecodeError; use ln::channelmonitor::{ANTI_REORG_DELAY, CLTV_SHARED_CLAIM_BUFFER, InputMaterial, ClaimRequest}; -use ln::chan_utils::{HTLCType, LocalCommitmentTransaction}; +use ln::chan_utils; +use ln::chan_utils::{HTLCType, LocalCommitmentTransaction, TxCreationKeys}; use chain::chaininterface::{FeeEstimator, BroadcasterInterface, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT}; use chain::keysinterface::ChannelKeys; use util::logger::Logger; @@ -47,6 +48,14 @@ enum OnchainEvent { } } +/// Cache public keys and feerate used to compute any HTLC transaction. +/// We only keep state for latest 2 commitment transactions as we should +/// never have to generate HTLC txn for revoked local commitment +struct HTLCTxCache { + local_keys: TxCreationKeys, + feerate_per_kw: u64, +} + /// Higher-level cache structure needed to re-generate bumped claim txn if needed #[derive(Clone, PartialEq)] pub struct ClaimTxBumpMaterial { @@ -144,6 +153,8 @@ pub struct OnchainTxHandler { funding_redeemscript: Script, local_commitment: Option, prev_local_commitment: Option, + current_htlc_cache: Option, + prev_htlc_cache: Option, key_storage: ChanSigner, @@ -187,6 +198,27 @@ impl OnchainTxHandler { self.local_commitment.write(writer)?; self.prev_local_commitment.write(writer)?; + macro_rules! serialize_htlc_cache { + ($cache: expr) => { + $cache.local_keys.write(writer)?; + $cache.feerate_per_kw.write(writer)?; + } + } + + if let Some(ref current) = self.current_htlc_cache { + writer.write_all(&[1; 1])?; + serialize_htlc_cache!(current); + } else { + writer.write_all(&[0; 1])?; + } + + if let Some(ref prev) = self.prev_htlc_cache { + writer.write_all(&[1; 1])?; + serialize_htlc_cache!(prev); + } else { + writer.write_all(&[0; 1])?; + } + self.key_storage.write(writer)?; writer.write_all(&byte_utils::be64_to_array(self.pending_claim_requests.len() as u64))?; @@ -231,6 +263,35 @@ impl ReadableArgs> for OnchainTx let local_commitment = Readable::read(reader)?; let prev_local_commitment = Readable::read(reader)?; + macro_rules! read_htlc_cache { + () => { + { + let local_keys = Readable::read(reader)?; + let feerate_per_kw = Readable::read(reader)?; + HTLCTxCache { + local_keys, + feerate_per_kw, + } + } + } + } + + let current_htlc_cache = match ::read(reader)? { + 0 => None, + 1 => { + Some(read_htlc_cache!()) + } + _ => return Err(DecodeError::InvalidValue), + }; + + let prev_htlc_cache = match ::read(reader)? { + 0 => None, + 1 => { + Some(read_htlc_cache!()) + } + _ => return Err(DecodeError::InvalidValue), + }; + let key_storage = Readable::read(reader)?; let pending_claim_requests_len: u64 = Readable::read(reader)?; @@ -281,6 +342,8 @@ impl ReadableArgs> for OnchainTx funding_redeemscript, local_commitment, prev_local_commitment, + current_htlc_cache, + prev_htlc_cache, key_storage, claimable_outpoints, pending_claim_requests, @@ -301,6 +364,8 @@ impl OnchainTxHandler { funding_redeemscript, local_commitment: None, prev_local_commitment: None, + current_htlc_cache: None, + prev_htlc_cache: None, key_storage, pending_claim_requests: HashMap::new(), claimable_outpoints: HashMap::new(), @@ -756,7 +821,7 @@ impl OnchainTxHandler { } } - pub(super) fn provide_latest_local_tx(&mut self, tx: LocalCommitmentTransaction) -> Result<(), ()> { + pub(super) fn provide_latest_local_tx(&mut self, tx: LocalCommitmentTransaction, local_keys: chan_utils::TxCreationKeys, feerate_per_kw: u64) -> Result<(), ()> { // To prevent any unsafe state discrepancy between offchain and onchain, once local // commitment transaction has been signed due to an event (either block height for // HTLC-timeout or channel force-closure), don't allow any further update of local @@ -767,6 +832,11 @@ impl OnchainTxHandler { } self.prev_local_commitment = self.local_commitment.take(); self.local_commitment = Some(tx); + self.prev_htlc_cache = self.current_htlc_cache.take(); + self.current_htlc_cache = Some(HTLCTxCache { + local_keys, + feerate_per_kw, + }); Ok(()) } From 080afeb6ea5746ac076658c723492e3d6d257ceb Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Sat, 21 Mar 2020 18:52:00 -0400 Subject: [PATCH 12/19] Cache per-HTLC data in OnchainTxHandler::HTLCTxCache Splitting further parsing from transaction generation, we cache transaction elements needed for local HTLC transaction inside OnchainTxHandler. Duplicated data will be removed from ChannelMonitor in future commits. --- lightning/src/ln/channelmonitor.rs | 4 ++-- lightning/src/ln/onchaintx.rs | 37 +++++++++++++++++++++++++++--- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index b0b31193156..4a89f5002e1 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1270,7 +1270,7 @@ impl ChannelMonitor { // reject update as we do, you MAY have the latest local valid commitment tx onchain // for which you want to spend outputs. We're NOT robust again this scenario right // now but we should consider it later. - if let Err(_) = self.onchain_tx_handler.provide_latest_local_tx(commitment_tx.clone(), local_keys.clone(), feerate_per_kw) { + if let Err(_) = self.onchain_tx_handler.provide_latest_local_tx(commitment_tx.clone(), local_keys.clone(), feerate_per_kw, htlc_outputs.clone()) { return Err(MonitorUpdateError("Local commitment signed has already been signed, no further update of LOCAL commitment transaction is allowed")); } self.current_local_commitment_number = 0xffff_ffff_ffff - ((((commitment_tx.without_valid_witness().input[0].sequence as u64 & 0xffffff) << 3*8) | (commitment_tx.without_valid_witness().lock_time as u64 & 0xffffff)) ^ self.commitment_transaction_number_obscure_factor); @@ -1284,7 +1284,7 @@ impl ChannelMonitor { delayed_payment_key: local_keys.a_delayed_payment_key, per_commitment_point: local_keys.per_commitment_point, feerate_per_kw, - htlc_outputs, + htlc_outputs: htlc_outputs, }); Ok(()) } diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 43d3ec9a639..406d9f38bd1 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -10,13 +10,14 @@ use bitcoin::util::bip143; use bitcoin_hashes::sha256d::Hash as Sha256dHash; -use secp256k1::Secp256k1; +use secp256k1::{Secp256k1, Signature}; use secp256k1; use ln::msgs::DecodeError; use ln::channelmonitor::{ANTI_REORG_DELAY, CLTV_SHARED_CLAIM_BUFFER, InputMaterial, ClaimRequest}; +use ln::channelmanager::HTLCSource; use ln::chan_utils; -use ln::chan_utils::{HTLCType, LocalCommitmentTransaction, TxCreationKeys}; +use ln::chan_utils::{HTLCType, LocalCommitmentTransaction, TxCreationKeys, HTLCOutputInCommitment}; use chain::chaininterface::{FeeEstimator, BroadcasterInterface, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT}; use chain::keysinterface::ChannelKeys; use util::logger::Logger; @@ -54,6 +55,7 @@ enum OnchainEvent { struct HTLCTxCache { local_keys: TxCreationKeys, feerate_per_kw: u64, + per_htlc: HashMap)> } /// Higher-level cache structure needed to re-generate bumped claim txn if needed @@ -202,6 +204,16 @@ impl OnchainTxHandler { ($cache: expr) => { $cache.local_keys.write(writer)?; $cache.feerate_per_kw.write(writer)?; + writer.write_all(&byte_utils::be64_to_array($cache.per_htlc.len() as u64))?; + for (_, &(ref htlc, ref sig)) in $cache.per_htlc.iter() { + htlc.write(writer)?; + if let &Some(ref their_sig) = sig { + 1u8.write(writer)?; + writer.write_all(&their_sig.serialize_compact())?; + } else { + 0u8.write(writer)?; + } + } } } @@ -268,9 +280,21 @@ impl ReadableArgs> for OnchainTx { let local_keys = Readable::read(reader)?; let feerate_per_kw = Readable::read(reader)?; + let htlcs_count: u64 = Readable::read(reader)?; + let mut per_htlc = HashMap::with_capacity(cmp::min(htlcs_count as usize, MAX_ALLOC_SIZE / 32)); + for _ in 0..htlcs_count { + let htlc: HTLCOutputInCommitment = Readable::read(reader)?; + let sigs = match ::read(reader)? { + 0 => None, + 1 => Some(Readable::read(reader)?), + _ => return Err(DecodeError::InvalidValue), + }; + per_htlc.insert(htlc.transaction_output_index.unwrap(), (htlc, sigs)); + } HTLCTxCache { local_keys, feerate_per_kw, + per_htlc } } } @@ -821,7 +845,7 @@ impl OnchainTxHandler { } } - pub(super) fn provide_latest_local_tx(&mut self, tx: LocalCommitmentTransaction, local_keys: chan_utils::TxCreationKeys, feerate_per_kw: u64) -> Result<(), ()> { + pub(super) fn provide_latest_local_tx(&mut self, tx: LocalCommitmentTransaction, local_keys: chan_utils::TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>) -> Result<(), ()> { // To prevent any unsafe state discrepancy between offchain and onchain, once local // commitment transaction has been signed due to an event (either block height for // HTLC-timeout or channel force-closure), don't allow any further update of local @@ -833,9 +857,16 @@ impl OnchainTxHandler { self.prev_local_commitment = self.local_commitment.take(); self.local_commitment = Some(tx); self.prev_htlc_cache = self.current_htlc_cache.take(); + let mut per_htlc = HashMap::with_capacity(htlc_outputs.len()); + for htlc in htlc_outputs { + if htlc.0.transaction_output_index.is_some() { // Discard dust HTLC as we will never have to generate onchain tx for them + per_htlc.insert(htlc.0.transaction_output_index.unwrap(), (htlc.0, htlc.1)); + } + } self.current_htlc_cache = Some(HTLCTxCache { local_keys, feerate_per_kw, + per_htlc }); Ok(()) } From 6b8a5166476228f93a06fe55db40fd9715b118ad Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Mon, 23 Mar 2020 01:36:37 -0400 Subject: [PATCH 13/19] Cache csv_local inside OnchainTxHandler csv_local is csv_delay encumbering local revokable_redeemscript for to_local an htlc output on local commitment/HTLC transactions. --- lightning/src/ln/channelmonitor.rs | 2 +- lightning/src/ln/onchaintx.rs | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 4a89f5002e1..0f6cc826e5d 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1145,7 +1145,7 @@ impl ChannelMonitor { onchain_events_waiting_threshold_conf: HashMap::new(), outputs_to_watch: HashMap::new(), - onchain_tx_handler: OnchainTxHandler::new(destination_script.clone(), keys, funding_redeemscript, logger.clone()), + onchain_tx_handler: OnchainTxHandler::new(destination_script.clone(), keys, funding_redeemscript, their_to_self_delay, logger.clone()), last_block_hash: Default::default(), secp_ctx: Secp256k1::new(), diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 406d9f38bd1..fce93554902 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -157,6 +157,7 @@ pub struct OnchainTxHandler { prev_local_commitment: Option, current_htlc_cache: Option, prev_htlc_cache: Option, + local_csv: u16, key_storage: ChanSigner, @@ -230,6 +231,7 @@ impl OnchainTxHandler { } else { writer.write_all(&[0; 1])?; } + self.local_csv.write(writer)?; self.key_storage.write(writer)?; @@ -315,6 +317,7 @@ impl ReadableArgs> for OnchainTx } _ => return Err(DecodeError::InvalidValue), }; + let local_csv = Readable::read(reader)?; let key_storage = Readable::read(reader)?; @@ -368,6 +371,7 @@ impl ReadableArgs> for OnchainTx prev_local_commitment, current_htlc_cache, prev_htlc_cache, + local_csv, key_storage, claimable_outpoints, pending_claim_requests, @@ -379,7 +383,7 @@ impl ReadableArgs> for OnchainTx } impl OnchainTxHandler { - pub(super) fn new(destination_script: Script, keys: ChanSigner, funding_redeemscript: Script, logger: Arc) -> Self { + pub(super) fn new(destination_script: Script, keys: ChanSigner, funding_redeemscript: Script, local_csv: u16, logger: Arc) -> Self { let key_storage = keys; @@ -390,6 +394,7 @@ impl OnchainTxHandler { prev_local_commitment: None, current_htlc_cache: None, prev_htlc_cache: None, + local_csv, key_storage, pending_claim_requests: HashMap::new(), claimable_outpoints: HashMap::new(), From 8369541f63eead52552788198fadfa2456b2f462 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Mon, 23 Mar 2020 01:30:48 -0400 Subject: [PATCH 14/19] Add OnchainTxHandler::get_fully_signed_htlc In case of channel force-closure, access to local commitment transactions and its dependent HTLCs is needed. Instead of using broadcast_by_local_state which registers outpoint to claim and outputs to watch which are going to be discarded in this case, we simply ask OnchainTxHandler to build and sign HTLC transactions through new API. --- lightning/src/ln/channelmonitor.rs | 22 ++++++++++++++++++---- lightning/src/ln/onchaintx.rs | 21 ++++++++++++++++++++- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 0f6cc826e5d..f4f03abc3e2 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1819,10 +1819,17 @@ impl ChannelMonitor { pub fn get_latest_local_commitment_txn(&mut self) -> Vec { log_trace!(self, "Getting signed latest local commitment transaction!"); if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(self.channel_value_satoshis.unwrap()) { + let txid = commitment_tx.txid(); let mut res = vec![commitment_tx]; if let &Some(ref local_tx) = &self.current_local_signed_commitment_tx { - let mut htlc_txn = self.broadcast_by_local_state(res.get(0).unwrap(), local_tx).0; - res.append(&mut htlc_txn); + for htlc in local_tx.htlc_outputs.iter() { + if let Some(htlc_index) = htlc.0.transaction_output_index { + let preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(*preimage) } else { None }; + if let Some(htlc_tx) = self.onchain_tx_handler.get_fully_signed_htlc_tx(txid, htlc_index, preimage) { + res.push(htlc_tx); + } + } + } // We throw away the generated waiting_first_conf data as we aren't (yet) confirmed and we don't actually know what the caller wants to do. // The data will be re-generated and tracked in check_spend_local_transaction if we get a confirmation. } @@ -1838,10 +1845,17 @@ impl ChannelMonitor { pub fn unsafe_get_latest_local_commitment_txn(&mut self) -> Vec { log_trace!(self, "Getting signed copy of latest local commitment transaction!"); if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_copy_local_tx(self.channel_value_satoshis.unwrap()) { + let txid = commitment_tx.txid(); let mut res = vec![commitment_tx]; if let &Some(ref local_tx) = &self.current_local_signed_commitment_tx { - let mut htlc_txn = self.broadcast_by_local_state(res.get(0).unwrap(), local_tx).0; - res.append(&mut htlc_txn); + for htlc in local_tx.htlc_outputs.iter() { + if let Some(htlc_index) = htlc.0.transaction_output_index { + let preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(*preimage) } else { None }; + if let Some(htlc_tx) = self.onchain_tx_handler.get_fully_signed_htlc_tx(txid, htlc_index, preimage) { + res.push(htlc_tx); + } + } + } } return res } diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index fce93554902..8cb9bfb397a 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -15,7 +15,7 @@ use secp256k1; use ln::msgs::DecodeError; use ln::channelmonitor::{ANTI_REORG_DELAY, CLTV_SHARED_CLAIM_BUFFER, InputMaterial, ClaimRequest}; -use ln::channelmanager::HTLCSource; +use ln::channelmanager::{HTLCSource, PaymentPreimage}; use ln::chan_utils; use ln::chan_utils::{HTLCType, LocalCommitmentTransaction, TxCreationKeys, HTLCOutputInCommitment}; use chain::chaininterface::{FeeEstimator, BroadcasterInterface, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT}; @@ -893,4 +893,23 @@ impl OnchainTxHandler { } None } + + pub(super) fn get_fully_signed_htlc_tx(&mut self, txid: Sha256dHash, htlc_index: u32, preimage: Option) -> Option { + //TODO: store preimage in OnchainTxHandler + if let Some(ref local_commitment) = self.local_commitment { + if local_commitment.txid() == txid { + if let Some(ref htlc_cache) = self.current_htlc_cache { + if let Some(htlc) = htlc_cache.per_htlc.get(&htlc_index) { + if !htlc.0.offered && preimage.is_none() { return None; }; // If we don't have preimage for HTLC-Success, don't try to generate + let htlc_secret = if !htlc.0.offered { preimage } else { None }; // If we have a preimage for a HTLC-Timeout, don't use it that's likely a duplicate HTLC hash + let mut htlc_tx = chan_utils::build_htlc_transaction(&txid, htlc_cache.feerate_per_kw, self.local_csv, &htlc.0, &htlc_cache.local_keys.a_delayed_payment_key, &htlc_cache.local_keys.revocation_key); + self.key_storage.sign_htlc_transaction(&mut htlc_tx, htlc.1.as_ref().unwrap(), &htlc_secret, &htlc.0, &htlc_cache.local_keys.a_htlc_key, &htlc_cache.local_keys.b_htlc_key, &htlc_cache.local_keys.revocation_key, &htlc_cache.local_keys.per_commitment_point, &self.secp_ctx); + return Some(htlc_tx); + + } + } + } + } + None + } } From 1107ab06c33bd360bdee7ee64f4b690e753003f6 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Mon, 9 Mar 2020 18:15:35 -0400 Subject: [PATCH 15/19] Move HTLC tx generation in OnchainTxHandler HTLC Transaction can't be bumped without sighash changes so their gneeration is one-time for nwo. We move them in OnchainTxHandler for simplifying ChannelMonitor and to prepare storage of keys material behind one external signer interface. Some tests break due to change in transaction broadcaster order. Number of transactions may vary because of temporary anti-duplicata tweak can't dissociate between 2- broadcast from different origins (ChannelMonitor, ChannelManager) and 2-broadcast from same component. --- lightning/src/chain/keysinterface.rs | 39 +--- lightning/src/ln/chan_utils.rs | 110 ++++++++++- lightning/src/ln/channel.rs | 15 +- lightning/src/ln/channelmonitor.rs | 94 ++++------ lightning/src/ln/functional_tests.rs | 49 +++-- lightning/src/ln/onchaintx.rs | 191 +++++--------------- lightning/src/util/enforcing_trait_impls.rs | 4 +- 7 files changed, 228 insertions(+), 274 deletions(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index e3e2d929447..58bdd722609 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -2,7 +2,7 @@ //! spendable on-chain outputs which the user owns and is responsible for using just as any other //! on-chain output which is theirs. -use bitcoin::blockdata::transaction::{Transaction, OutPoint, TxOut, SigHashType}; +use bitcoin::blockdata::transaction::{Transaction, OutPoint, TxOut}; use bitcoin::blockdata::script::{Script, Builder}; use bitcoin::blockdata::opcodes; use bitcoin::network::constants::Network; @@ -235,8 +235,7 @@ pub trait ChannelKeys : Send+Clone { /// Signs a transaction created by build_htlc_transaction. If the transaction is an /// HTLC-Success transaction, preimage must be set! /// TODO: should be merged with sign_local_commitment as a slice of HTLC transactions to sign - fn sign_htlc_transaction(&self, htlc_tx: &mut Transaction, their_sig: &Signature, preimage: &Option, htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey, per_commitment_point: &PublicKey, secp_ctx: &Secp256k1); - + fn sign_htlc_transaction(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option, local_csv: u16, secp_ctx: &Secp256k1); /// Create a signature for a (proposed) closing transaction. /// /// Note that, due to rounding, there may be one "missing" satoshi, and either party may have @@ -373,38 +372,8 @@ impl ChannelKeys for InMemoryChannelKeys { local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx); } - fn sign_htlc_transaction(&self, htlc_tx: &mut Transaction, their_sig: &Signature, preimage: &Option, htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey, per_commitment_point: &PublicKey, secp_ctx: &Secp256k1) { - if htlc_tx.input.len() != 1 { return; } - if htlc_tx.input[0].witness.len() != 0 { return; } - - let htlc_redeemscript = chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, a_htlc_key, b_htlc_key, revocation_key); - - if let Ok(our_htlc_key) = chan_utils::derive_private_key(secp_ctx, per_commitment_point, &self.htlc_base_key) { - let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]); - let local_tx = PublicKey::from_secret_key(&secp_ctx, &our_htlc_key) == *a_htlc_key; - let our_sig = secp_ctx.sign(&sighash, &our_htlc_key); - - htlc_tx.input[0].witness.push(Vec::new()); // First is the multisig dummy - - if local_tx { // b, then a - htlc_tx.input[0].witness.push(their_sig.serialize_der().to_vec()); - htlc_tx.input[0].witness.push(our_sig.serialize_der().to_vec()); - } else { - htlc_tx.input[0].witness.push(our_sig.serialize_der().to_vec()); - htlc_tx.input[0].witness.push(their_sig.serialize_der().to_vec()); - } - htlc_tx.input[0].witness[1].push(SigHashType::All as u8); - htlc_tx.input[0].witness[2].push(SigHashType::All as u8); - - if htlc.offered { - htlc_tx.input[0].witness.push(Vec::new()); - assert!(preimage.is_none()); - } else { - htlc_tx.input[0].witness.push(preimage.unwrap().0.to_vec()); - } - - htlc_tx.input[0].witness.push(htlc_redeemscript.as_bytes().to_vec()); - } else { return; } + fn sign_htlc_transaction(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option, local_csv: u16, secp_ctx: &Secp256k1) { + local_commitment_tx.add_htlc_sig(&self.htlc_base_key, htlc_index, preimage, local_csv, secp_ctx); } fn sign_closing_transaction(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1) -> Result { diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 4be3b3151df..566fd7b00d8 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -14,7 +14,7 @@ use bitcoin_hashes::ripemd160::Hash as Ripemd160; use bitcoin_hashes::hash160::Hash as Hash160; use bitcoin_hashes::sha256d::Hash as Sha256dHash; -use ln::channelmanager::PaymentHash; +use ln::channelmanager::{PaymentHash, PaymentPreimage}; use ln::msgs::DecodeError; use util::ser::{Readable, Writeable, Writer, WriterWriteAdaptor}; use util::byte_utils; @@ -23,6 +23,10 @@ use secp256k1::key::{SecretKey, PublicKey}; use secp256k1::{Secp256k1, Signature}; use secp256k1; +use std::{cmp, mem}; + +const MAX_ALLOC_SIZE: usize = 64*1024; + pub(super) const HTLC_SUCCESS_TX_WEIGHT: u64 = 703; pub(super) const HTLC_TIMEOUT_TX_WEIGHT: u64 = 663; @@ -480,7 +484,11 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s /// to broadcast. Eventually this will require a signer which is possibly external, but for now we /// just pass in the SecretKeys required. pub struct LocalCommitmentTransaction { - tx: Transaction + tx: Transaction, + //TODO: modify Channel methods to integrate HTLC material at LocalCommitmentTransaction generation to drop Option here + local_keys: Option, + feerate_per_kw: Option, + per_htlc: Vec<(HTLCOutputInCommitment, Option, Option)> } impl LocalCommitmentTransaction { #[cfg(test)] @@ -499,7 +507,11 @@ impl LocalCommitmentTransaction { input: vec![dummy_input], output: Vec::new(), lock_time: 0, - } } + }, + local_keys: None, + feerate_per_kw: None, + per_htlc: Vec::new() + } } /// Generate a new LocalCommitmentTransaction based on a raw commitment transaction, @@ -520,7 +532,11 @@ impl LocalCommitmentTransaction { tx.input[0].witness.push(Vec::new()); } - Self { tx } + Self { tx, + local_keys: None, + feerate_per_kw: None, + per_htlc: Vec::new() + } } /// Get the txid of the local commitment transaction contained in this @@ -577,6 +593,67 @@ impl LocalCommitmentTransaction { assert!(self.has_local_sig()); &self.tx } + + /// Set HTLC cache to generate any local HTLC transaction spending one of htlc ouput + /// from this local commitment transaction + pub(crate) fn set_htlc_cache(&mut self, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>) { + self.local_keys = Some(local_keys); + self.feerate_per_kw = Some(feerate_per_kw); + self.per_htlc = htlc_outputs; + } + + /// Add local signature for a htlc transaction, do nothing if a cached signed transaction is + /// already present + pub fn add_htlc_sig(&mut self, htlc_base_key: &SecretKey, htlc_index: u32, preimage: Option, local_csv: u16, secp_ctx: &Secp256k1) { + if self.local_keys.is_none() || self.feerate_per_kw.is_none() { return; } + let local_keys = self.local_keys.as_ref().unwrap(); + let txid = self.txid(); + for this_htlc in self.per_htlc.iter_mut() { + if this_htlc.0.transaction_output_index.unwrap() == htlc_index { + if this_htlc.2.is_some() { return; } // we already have a cached htlc transaction at provided index + let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw.unwrap(), local_csv, &this_htlc.0, &local_keys.a_delayed_payment_key, &local_keys.revocation_key); + if !this_htlc.0.offered && preimage.is_none() { return; } // if we don't have preimage for HTLC-Success, don't try to generate + let htlc_secret = if !this_htlc.0.offered { preimage } else { None }; // if we have a preimage for HTLC-Timeout, don't use it that's likely a duplicate HTLC hash + if this_htlc.1.is_none() { return; } // we don't have any remote signature for this htlc + if htlc_tx.input.len() != 1 { return; } + if htlc_tx.input[0].witness.len() != 0 { return; } + + let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &local_keys.a_htlc_key, &local_keys.b_htlc_key, &local_keys.revocation_key); + + if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &local_keys.per_commitment_point, htlc_base_key) { + let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]); + let our_sig = secp_ctx.sign(&sighash, &our_htlc_key); + + htlc_tx.input[0].witness.push(Vec::new()); // First is the multisig dummy + + htlc_tx.input[0].witness.push(this_htlc.1.unwrap().serialize_der().to_vec()); + htlc_tx.input[0].witness.push(our_sig.serialize_der().to_vec()); + htlc_tx.input[0].witness[1].push(SigHashType::All as u8); + htlc_tx.input[0].witness[2].push(SigHashType::All as u8); + + if this_htlc.0.offered { + htlc_tx.input[0].witness.push(Vec::new()); + assert!(htlc_secret.is_none()); + } else { + htlc_tx.input[0].witness.push(htlc_secret.unwrap().0.to_vec()); + } + + htlc_tx.input[0].witness.push(htlc_redeemscript.as_bytes().to_vec()); + + this_htlc.2 = Some(htlc_tx); + } else { return; } + } + } + } + /// Expose raw htlc transaction, guarante witness is complete if non-empty + pub fn htlc_with_valid_witness(&self, htlc_index: u32) -> &Option { + for this_htlc in self.per_htlc.iter() { + if this_htlc.0.transaction_output_index.unwrap() == htlc_index { + return &this_htlc.2; + } + } + &None + } } impl PartialEq for LocalCommitmentTransaction { // We dont care whether we are signed in equality comparison @@ -592,6 +669,14 @@ impl Writeable for LocalCommitmentTransaction { _ => panic!("local tx must have been well-formed!"), } } + self.local_keys.write(writer)?; + self.feerate_per_kw.write(writer)?; + writer.write_all(&byte_utils::be64_to_array(self.per_htlc.len() as u64))?; + for &(ref htlc, ref sig, ref htlc_tx) in self.per_htlc.iter() { + htlc.write(writer)?; + sig.write(writer)?; + htlc_tx.write(writer)?; + } Ok(()) } } @@ -604,12 +689,27 @@ impl Readable for LocalCommitmentTransaction { _ => return Err(DecodeError::InvalidValue), }, }; + let local_keys = Readable::read(reader)?; + let feerate_per_kw = Readable::read(reader)?; + let htlcs_count: u64 = Readable::read(reader)?; + let mut per_htlc = Vec::with_capacity(cmp::min(htlcs_count as usize, MAX_ALLOC_SIZE / mem::size_of::<(HTLCOutputInCommitment, Option, Option)>())); + for _ in 0..htlcs_count { + let htlc: HTLCOutputInCommitment = Readable::read(reader)?; + let sigs = Readable::read(reader)?; + let htlc_tx = Readable::read(reader)?; + per_htlc.push((htlc, sigs, htlc_tx)); + } if tx.input.len() != 1 { // Ensure tx didn't hit the 0-input ambiguity case. return Err(DecodeError::InvalidValue); } - Ok(Self { tx }) + Ok(Self { + tx, + local_keys, + feerate_per_kw, + per_htlc, + }) } } diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 81528001776..47936ea61d1 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4475,6 +4475,7 @@ mod tests { let mut unsigned_tx: (Transaction, Vec); + let mut localtx; macro_rules! test_commitment { ( $their_sig_hex: expr, $our_sig_hex: expr, $tx_hex: expr) => { unsigned_tx = { @@ -4489,7 +4490,7 @@ mod tests { let sighash = Message::from_slice(&bip143::SighashComponents::new(&unsigned_tx.0).sighash_all(&unsigned_tx.0.input[0], &redeemscript, chan.channel_value_satoshis)[..]).unwrap(); secp_ctx.verify(&sighash, &their_signature, chan.their_funding_pubkey()).unwrap(); - let mut localtx = LocalCommitmentTransaction::new_missing_local_sig(unsigned_tx.0.clone(), &their_signature, &PublicKey::from_secret_key(&secp_ctx, chan.local_keys.funding_key()), chan.their_funding_pubkey()); + localtx = LocalCommitmentTransaction::new_missing_local_sig(unsigned_tx.0.clone(), &their_signature, &PublicKey::from_secret_key(&secp_ctx, chan.local_keys.funding_key()), chan.their_funding_pubkey()); chan_keys.sign_local_commitment(&mut localtx, &redeemscript, chan.channel_value_satoshis, &chan.secp_ctx); assert_eq!(serialize(localtx.with_valid_witness())[..], @@ -4498,11 +4499,11 @@ mod tests { } macro_rules! test_htlc_output { - ( $htlc_idx: expr, $their_sig_hex: expr, $our_sig_hex: expr, $tx_hex: expr ) => { + ( $htlc_idx: expr, $their_sig_hex: expr, $our_sig_hex: expr, $tx_hex: expr) => { let remote_signature = Signature::from_der(&hex::decode($their_sig_hex).unwrap()[..]).unwrap(); let ref htlc = unsigned_tx.1[$htlc_idx]; - let mut htlc_tx = chan.build_htlc_transaction(&unsigned_tx.0.txid(), &htlc, true, &keys, chan.feerate_per_kw); + let htlc_tx = chan.build_htlc_transaction(&unsigned_tx.0.txid(), &htlc, true, &keys, chan.feerate_per_kw); let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &keys); let htlc_sighash = Message::from_slice(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]).unwrap(); secp_ctx.verify(&htlc_sighash, &remote_signature, &keys.b_htlc_key).unwrap(); @@ -4519,8 +4520,12 @@ mod tests { assert!(preimage.is_some()); } - chan_keys.sign_htlc_transaction(&mut htlc_tx, &remote_signature, &preimage, &htlc, &keys.a_htlc_key, &keys.b_htlc_key, &keys.revocation_key, &keys.per_commitment_point, &chan.secp_ctx); - assert_eq!(serialize(&htlc_tx)[..], + let mut per_htlc = Vec::new(); + per_htlc.push((htlc.clone(), Some(remote_signature), None)); + localtx.set_htlc_cache(keys.clone(), chan.feerate_per_kw, per_htlc); + chan_keys.sign_htlc_transaction(&mut localtx, $htlc_idx, preimage, chan.their_to_self_delay, &chan.secp_ctx); + + assert_eq!(serialize(localtx.htlc_with_valid_witness($htlc_idx).as_ref().unwrap())[..], hex::decode($tx_hex).unwrap()[..]); }; } diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index f4f03abc3e2..d180d3bbdc0 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -409,7 +409,6 @@ impl PartialEq for OnchainDetection { struct LocalSignedTx { /// txid of the transaction in tx, just used to make comparison faster txid: Sha256dHash, - tx: LocalCommitmentTransaction, revocation_key: PublicKey, a_htlc_key: PublicKey, b_htlc_key: PublicKey, @@ -439,8 +438,6 @@ pub(crate) enum InputMaterial { locktime: u32, }, LocalHTLC { - witness_script: Script, - sigs: (Signature, Signature), preimage: Option, amount: u64, }, @@ -468,11 +465,8 @@ impl Writeable for InputMaterial { writer.write_all(&byte_utils::be64_to_array(*amount))?; writer.write_all(&byte_utils::be32_to_array(*locktime))?; }, - &InputMaterial::LocalHTLC { ref witness_script, ref sigs, ref preimage, ref amount } => { + &InputMaterial::LocalHTLC { ref preimage, ref amount } => { writer.write_all(&[2; 1])?; - witness_script.write(writer)?; - sigs.0.write(writer)?; - sigs.1.write(writer)?; preimage.write(writer)?; writer.write_all(&byte_utils::be64_to_array(*amount))?; }, @@ -517,16 +511,11 @@ impl Readable for InputMaterial { } }, 2 => { - let witness_script = Readable::read(reader)?; - let their_sig = Readable::read(reader)?; - let our_sig = Readable::read(reader)?; let preimage = Readable::read(reader)?; let amount = Readable::read(reader)?; InputMaterial::LocalHTLC { - witness_script, - sigs: (their_sig, our_sig), preimage, - amount + amount, } }, 3 => { @@ -970,7 +959,7 @@ impl ChannelMonitor { macro_rules! serialize_local_tx { ($local_tx: expr) => { - $local_tx.tx.write(writer)?; + $local_tx.txid.write(writer)?; writer.write_all(&$local_tx.revocation_key.serialize())?; writer.write_all(&$local_tx.a_htlc_key.serialize())?; writer.write_all(&$local_tx.b_htlc_key.serialize())?; @@ -1261,23 +1250,32 @@ impl ChannelMonitor { /// is important that any clones of this channel monitor (including remote clones) by kept /// up-to-date as our local commitment transaction is updated. /// Panics if set_their_to_self_delay has never been called. - pub(super) fn provide_latest_local_commitment_tx_info(&mut self, commitment_tx: LocalCommitmentTransaction, local_keys: chan_utils::TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>) -> Result<(), MonitorUpdateError> { + pub(super) fn provide_latest_local_commitment_tx_info(&mut self, mut commitment_tx: LocalCommitmentTransaction, local_keys: chan_utils::TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>) -> Result<(), MonitorUpdateError> { if self.their_to_self_delay.is_none() { return Err(MonitorUpdateError("Got a local commitment tx info update before we'd set basic information about the channel")); } + let txid = commitment_tx.txid(); + let sequence = commitment_tx.without_valid_witness().input[0].sequence as u64; + let locktime = commitment_tx.without_valid_witness().lock_time as u64; + let mut htlcs = Vec::with_capacity(htlc_outputs.len()); + for htlc in htlc_outputs.clone() { + if let Some(_) = htlc.0.transaction_output_index { + htlcs.push((htlc.0, htlc.1, None)); + } + } + commitment_tx.set_htlc_cache(local_keys.clone(), feerate_per_kw, htlcs); // Returning a monitor error before updating tracking points means in case of using // a concurrent watchtower implementation for same channel, if this one doesn't // reject update as we do, you MAY have the latest local valid commitment tx onchain // for which you want to spend outputs. We're NOT robust again this scenario right // now but we should consider it later. - if let Err(_) = self.onchain_tx_handler.provide_latest_local_tx(commitment_tx.clone(), local_keys.clone(), feerate_per_kw, htlc_outputs.clone()) { + if let Err(_) = self.onchain_tx_handler.provide_latest_local_tx(commitment_tx) { return Err(MonitorUpdateError("Local commitment signed has already been signed, no further update of LOCAL commitment transaction is allowed")); } - self.current_local_commitment_number = 0xffff_ffff_ffff - ((((commitment_tx.without_valid_witness().input[0].sequence as u64 & 0xffffff) << 3*8) | (commitment_tx.without_valid_witness().lock_time as u64 & 0xffffff)) ^ self.commitment_transaction_number_obscure_factor); + self.current_local_commitment_number = 0xffff_ffff_ffff - ((((sequence & 0xffffff) << 3*8) | (locktime as u64 & 0xffffff)) ^ self.commitment_transaction_number_obscure_factor); self.prev_local_signed_commitment_tx = self.current_local_signed_commitment_tx.take(); self.current_local_signed_commitment_tx = Some(LocalSignedTx { - txid: commitment_tx.txid(), - tx: commitment_tx, + txid, revocation_key: local_keys.revocation_key, a_htlc_key: local_keys.a_htlc_key, b_htlc_key: local_keys.b_htlc_key, @@ -1687,8 +1685,8 @@ impl ChannelMonitor { (claimable_outpoints, Some((htlc_txid, tx.output.clone()))) } - fn broadcast_by_local_state(&self, commitment_tx: &Transaction, local_tx: &LocalSignedTx) -> (Vec, Vec, Option<(Script, SecretKey, Script)>) { - let mut res = Vec::with_capacity(local_tx.htlc_outputs.len()); + fn broadcast_by_local_state(&self, commitment_tx: &Transaction, local_tx: &LocalSignedTx) -> (Vec, Vec, Option<(Script, SecretKey, Script)>) { + let mut claim_requests = Vec::with_capacity(local_tx.htlc_outputs.len()); let mut watch_outputs = Vec::with_capacity(local_tx.htlc_outputs.len()); let redeemscript = chan_utils::get_revokeable_redeemscript(&local_tx.revocation_key, self.their_to_self_delay.unwrap(), &local_tx.delayed_payment_key); @@ -1696,40 +1694,23 @@ impl ChannelMonitor { Some((redeemscript.to_v0_p2wsh(), local_delayedkey, redeemscript)) } else { None }; - for &(ref htlc, ref sigs, _) in local_tx.htlc_outputs.iter() { + for &(ref htlc, _, _) in local_tx.htlc_outputs.iter() { if let Some(transaction_output_index) = htlc.transaction_output_index { - if let &Some(ref their_sig) = sigs { - if htlc.offered { - log_trace!(self, "Broadcasting HTLC-Timeout transaction against local commitment transactions"); - let mut htlc_timeout_tx = chan_utils::build_htlc_transaction(&local_tx.txid, local_tx.feerate_per_kw, self.their_to_self_delay.unwrap(), htlc, &local_tx.delayed_payment_key, &local_tx.revocation_key); - self.onchain_detection.keys.sign_htlc_transaction(&mut htlc_timeout_tx, their_sig, &None, htlc, &local_tx.a_htlc_key, &local_tx.b_htlc_key, &local_tx.revocation_key, &local_tx.per_commitment_point, &self.secp_ctx); - - log_trace!(self, "Outpoint {}:{} is being being claimed", htlc_timeout_tx.input[0].previous_output.vout, htlc_timeout_tx.input[0].previous_output.txid); - res.push(htlc_timeout_tx); - } else { - if let Some(payment_preimage) = self.payment_preimages.get(&htlc.payment_hash) { - log_trace!(self, "Broadcasting HTLC-Success transaction against local commitment transactions"); - let mut htlc_success_tx = chan_utils::build_htlc_transaction(&local_tx.txid, local_tx.feerate_per_kw, self.their_to_self_delay.unwrap(), htlc, &local_tx.delayed_payment_key, &local_tx.revocation_key); - self.onchain_detection.keys.sign_htlc_transaction(&mut htlc_success_tx, their_sig, &Some(*payment_preimage), htlc, &local_tx.a_htlc_key, &local_tx.b_htlc_key, &local_tx.revocation_key, &local_tx.per_commitment_point, &self.secp_ctx); - - log_trace!(self, "Outpoint {}:{} is being being claimed", htlc_success_tx.input[0].previous_output.vout, htlc_success_tx.input[0].previous_output.txid); - res.push(htlc_success_tx); - } - } - watch_outputs.push(commitment_tx.output[transaction_output_index as usize].clone()); - } else { panic!("Should have sigs for non-dust local tx outputs!") } + let preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) { Some(*preimage) } else { None }; + claim_requests.push(ClaimRequest { absolute_timelock: ::std::u32::MAX, aggregable: false, outpoint: BitcoinOutPoint { txid: local_tx.txid, vout: transaction_output_index as u32 }, witness_data: InputMaterial::LocalHTLC { preimage, amount: htlc.amount_msat / 1000 }}); + watch_outputs.push(commitment_tx.output[transaction_output_index as usize].clone()); } } - (res, watch_outputs, broadcasted_local_revokable_script) + (claim_requests, watch_outputs, broadcasted_local_revokable_script) } /// Attempts to claim any claimable HTLCs in a commitment transaction which was not (yet) /// revoked using data in local_claimable_outpoints. /// Should not be used if check_spend_revoked_transaction succeeds. - fn check_spend_local_transaction(&mut self, tx: &Transaction, height: u32) -> (Vec, (Sha256dHash, Vec)) { + fn check_spend_local_transaction(&mut self, tx: &Transaction, height: u32) -> (Vec, (Sha256dHash, Vec)) { let commitment_txid = tx.txid(); - let mut local_txn = Vec::new(); + let mut claim_requests = Vec::new(); let mut watch_outputs = Vec::new(); macro_rules! wait_threshold_conf { @@ -1757,7 +1738,7 @@ impl ChannelMonitor { macro_rules! append_onchain_update { ($updates: expr) => { - local_txn.append(&mut $updates.0); + claim_requests = $updates.0; watch_outputs.append(&mut $updates.1); self.broadcasted_local_revokable_script = $updates.2; } @@ -1804,7 +1785,7 @@ impl ChannelMonitor { } } - (local_txn, (commitment_txid, watch_outputs)) + (claim_requests, (commitment_txid, watch_outputs)) } /// Used by ChannelManager deserialization to broadcast the latest local state if its copy of @@ -1898,14 +1879,11 @@ impl ChannelMonitor { watch_outputs.push(new_outputs); } if new_outpoints.is_empty() { - let (local_txn, new_outputs) = self.check_spend_local_transaction(&tx, height); - for tx in local_txn.iter() { - log_trace!(self, "Broadcast onchain {}", log_tx!(tx)); - broadcaster.broadcast_transaction(tx); - } + let (mut new_outpoints, new_outputs) = self.check_spend_local_transaction(&tx, height); if !new_outputs.1.is_empty() { watch_outputs.push(new_outputs); } + claimable_outpoints.append(&mut new_outpoints); } claimable_outpoints.append(&mut new_outpoints); } @@ -1935,14 +1913,11 @@ impl ChannelMonitor { if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx { if should_broadcast { if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(self.channel_value_satoshis.unwrap()) { - let (txs, new_outputs, _) = self.broadcast_by_local_state(&commitment_tx, cur_local_tx); + let (mut new_outpoints, new_outputs, _) = self.broadcast_by_local_state(&commitment_tx, cur_local_tx); if !new_outputs.is_empty() { watch_outputs.push((cur_local_tx.txid.clone(), new_outputs)); } - for tx in txs { - log_trace!(self, "Broadcast onchain {}", log_tx!(tx)); - broadcaster.broadcast_transaction(&tx); - } + claimable_outpoints.append(&mut new_outpoints); } } } @@ -2393,7 +2368,7 @@ impl ReadableArgs> for (Sha256dH macro_rules! read_local_tx { () => { { - let tx = ::read(reader)?; + let txid = Readable::read(reader)?; let revocation_key = Readable::read(reader)?; let a_htlc_key = Readable::read(reader)?; let b_htlc_key = Readable::read(reader)?; @@ -2414,8 +2389,7 @@ impl ReadableArgs> for (Sha256dH } LocalSignedTx { - txid: tx.txid(), - tx, + txid, revocation_key, a_htlc_key, b_htlc_key, delayed_payment_key, per_commitment_point, feerate_per_kw, htlc_outputs: htlcs } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 0c6bac59cb5..d45469194f2 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2342,25 +2342,25 @@ fn claim_htlc_outputs_single_tx() { // ChannelMonitor: local commitment + local HTLC-timeout (2) // Check the pair local commitment and HTLC-timeout broadcast due to HTLC expiration + assert_eq!(node_txn[2].input.len(), 1); + check_spends!(node_txn[2], chan_1.3); assert_eq!(node_txn[3].input.len(), 1); - check_spends!(node_txn[3], chan_1.3); - assert_eq!(node_txn[0].input.len(), 1); - let witness_script = node_txn[0].input[0].witness.last().unwrap(); + let witness_script = node_txn[3].input[0].witness.last().unwrap(); assert_eq!(witness_script.len(), OFFERED_HTLC_SCRIPT_WEIGHT); //Spending an offered htlc output - check_spends!(node_txn[0], node_txn[3]); + check_spends!(node_txn[3], node_txn[2]); // Justice transactions are indices 1-2-4 + assert_eq!(node_txn[0].input.len(), 1); assert_eq!(node_txn[1].input.len(), 1); - assert_eq!(node_txn[2].input.len(), 1); assert_eq!(node_txn[4].input.len(), 1); + check_spends!(node_txn[0], revoked_local_txn[0]); check_spends!(node_txn[1], revoked_local_txn[0]); - check_spends!(node_txn[2], revoked_local_txn[0]); check_spends!(node_txn[4], revoked_local_txn[0]); let mut witness_lens = BTreeSet::new(); + witness_lens.insert(node_txn[0].input[0].witness.last().unwrap().len()); witness_lens.insert(node_txn[1].input[0].witness.last().unwrap().len()); - witness_lens.insert(node_txn[2].input[0].witness.last().unwrap().len()); witness_lens.insert(node_txn[4].input[0].witness.last().unwrap().len()); assert_eq!(witness_lens.len(), 3); assert_eq!(*witness_lens.iter().skip(0).next().unwrap(), 77); // revoked to_local @@ -2612,19 +2612,18 @@ fn test_htlc_on_chain_timeout() { { let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); assert_eq!(node_txn.len(), 5); // ChannelManager : 2 (commitment tx, HTLC-Timeout tx), ChannelMonitor : 2 (local commitment tx + HTLC-timeout), 1 timeout tx + assert_eq!(node_txn[1], node_txn[3]); + assert_eq!(node_txn[2], node_txn[4]); - assert_eq!(node_txn[2], node_txn[3]); - assert_eq!(node_txn[0], node_txn[4]); - - check_spends!(node_txn[1], commitment_tx[0]); - assert_eq!(node_txn[1].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); + check_spends!(node_txn[0], commitment_tx[0]); + assert_eq!(node_txn[0].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); - check_spends!(node_txn[2], chan_2.3); - check_spends!(node_txn[0], node_txn[2]); - assert_eq!(node_txn[2].clone().input[0].witness.last().unwrap().len(), 71); - assert_eq!(node_txn[0].clone().input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); + check_spends!(node_txn[1], chan_2.3); + check_spends!(node_txn[2], node_txn[1]); + assert_eq!(node_txn[1].clone().input[0].witness.last().unwrap().len(), 71); + assert_eq!(node_txn[2].clone().input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); - timeout_tx = node_txn[1].clone(); + timeout_tx = node_txn[0].clone(); node_txn.clear(); } @@ -4354,8 +4353,7 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() { check_added_monitors!(nodes[0], 1); let revoked_htlc_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(revoked_htlc_txn.len(), 3); - assert_eq!(revoked_htlc_txn[0], revoked_htlc_txn[2]); + assert_eq!(revoked_htlc_txn.len(), 2); assert_eq!(revoked_htlc_txn[0].input.len(), 1); assert_eq!(revoked_htlc_txn[0].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]); @@ -4411,8 +4409,7 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() { check_added_monitors!(nodes[1], 1); let revoked_htlc_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(revoked_htlc_txn.len(), 3); - assert_eq!(revoked_htlc_txn[0], revoked_htlc_txn[2]); + assert_eq!(revoked_htlc_txn.len(), 2); assert_eq!(revoked_htlc_txn[0].input.len(), 1); assert_eq!(revoked_htlc_txn[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]); @@ -7118,7 +7115,7 @@ fn test_bump_penalty_txn_on_revoked_htlcs() { check_added_monitors!(nodes[1], 1); let revoked_htlc_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(revoked_htlc_txn.len(), 6); + assert_eq!(revoked_htlc_txn.len(), 4); if revoked_htlc_txn[0].input[0].witness.last().unwrap().len() == ACCEPTED_HTLC_SCRIPT_WEIGHT { assert_eq!(revoked_htlc_txn[0].input.len(), 1); check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]); @@ -7376,11 +7373,11 @@ fn test_set_outpoints_partial_claiming() { let partial_claim_tx = { let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); assert_eq!(node_txn.len(), 3); - check_spends!(node_txn[0], node_txn[2]); - check_spends!(node_txn[1], node_txn[2]); - assert_eq!(node_txn[0].input.len(), 1); + check_spends!(node_txn[1], node_txn[0]); + check_spends!(node_txn[2], node_txn[0]); assert_eq!(node_txn[1].input.len(), 1); - node_txn[0].clone() + assert_eq!(node_txn[2].input.len(), 1); + node_txn[1].clone() }; // Broadcast partial claim on node A, should regenerate a claiming tx with HTLC dropped diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 8cb9bfb397a..b2115c0f8ac 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -10,21 +10,20 @@ use bitcoin::util::bip143; use bitcoin_hashes::sha256d::Hash as Sha256dHash; -use secp256k1::{Secp256k1, Signature}; +use secp256k1::Secp256k1; use secp256k1; use ln::msgs::DecodeError; use ln::channelmonitor::{ANTI_REORG_DELAY, CLTV_SHARED_CLAIM_BUFFER, InputMaterial, ClaimRequest}; -use ln::channelmanager::{HTLCSource, PaymentPreimage}; -use ln::chan_utils; -use ln::chan_utils::{HTLCType, LocalCommitmentTransaction, TxCreationKeys, HTLCOutputInCommitment}; +use ln::channelmanager::PaymentPreimage; +use ln::chan_utils::{HTLCType, LocalCommitmentTransaction}; use chain::chaininterface::{FeeEstimator, BroadcasterInterface, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT}; use chain::keysinterface::ChannelKeys; use util::logger::Logger; use util::ser::{ReadableArgs, Readable, Writer, Writeable}; use util::byte_utils; -use std::collections::{HashMap, hash_map, HashSet}; +use std::collections::{HashMap, hash_map}; use std::sync::Arc; use std::cmp; use std::ops::Deref; @@ -49,15 +48,6 @@ enum OnchainEvent { } } -/// Cache public keys and feerate used to compute any HTLC transaction. -/// We only keep state for latest 2 commitment transactions as we should -/// never have to generate HTLC txn for revoked local commitment -struct HTLCTxCache { - local_keys: TxCreationKeys, - feerate_per_kw: u64, - per_htlc: HashMap)> -} - /// Higher-level cache structure needed to re-generate bumped claim txn if needed #[derive(Clone, PartialEq)] pub struct ClaimTxBumpMaterial { @@ -155,8 +145,6 @@ pub struct OnchainTxHandler { funding_redeemscript: Script, local_commitment: Option, prev_local_commitment: Option, - current_htlc_cache: Option, - prev_htlc_cache: Option, local_csv: u16, key_storage: ChanSigner, @@ -201,36 +189,6 @@ impl OnchainTxHandler { self.local_commitment.write(writer)?; self.prev_local_commitment.write(writer)?; - macro_rules! serialize_htlc_cache { - ($cache: expr) => { - $cache.local_keys.write(writer)?; - $cache.feerate_per_kw.write(writer)?; - writer.write_all(&byte_utils::be64_to_array($cache.per_htlc.len() as u64))?; - for (_, &(ref htlc, ref sig)) in $cache.per_htlc.iter() { - htlc.write(writer)?; - if let &Some(ref their_sig) = sig { - 1u8.write(writer)?; - writer.write_all(&their_sig.serialize_compact())?; - } else { - 0u8.write(writer)?; - } - } - } - } - - if let Some(ref current) = self.current_htlc_cache { - writer.write_all(&[1; 1])?; - serialize_htlc_cache!(current); - } else { - writer.write_all(&[0; 1])?; - } - - if let Some(ref prev) = self.prev_htlc_cache { - writer.write_all(&[1; 1])?; - serialize_htlc_cache!(prev); - } else { - writer.write_all(&[0; 1])?; - } self.local_csv.write(writer)?; self.key_storage.write(writer)?; @@ -274,49 +232,10 @@ impl ReadableArgs> for OnchainTx fn read(reader: &mut R, logger: Arc) -> Result { let destination_script = Readable::read(reader)?; let funding_redeemscript = Readable::read(reader)?; + let local_commitment = Readable::read(reader)?; let prev_local_commitment = Readable::read(reader)?; - macro_rules! read_htlc_cache { - () => { - { - let local_keys = Readable::read(reader)?; - let feerate_per_kw = Readable::read(reader)?; - let htlcs_count: u64 = Readable::read(reader)?; - let mut per_htlc = HashMap::with_capacity(cmp::min(htlcs_count as usize, MAX_ALLOC_SIZE / 32)); - for _ in 0..htlcs_count { - let htlc: HTLCOutputInCommitment = Readable::read(reader)?; - let sigs = match ::read(reader)? { - 0 => None, - 1 => Some(Readable::read(reader)?), - _ => return Err(DecodeError::InvalidValue), - }; - per_htlc.insert(htlc.transaction_output_index.unwrap(), (htlc, sigs)); - } - HTLCTxCache { - local_keys, - feerate_per_kw, - per_htlc - } - } - } - } - - let current_htlc_cache = match ::read(reader)? { - 0 => None, - 1 => { - Some(read_htlc_cache!()) - } - _ => return Err(DecodeError::InvalidValue), - }; - - let prev_htlc_cache = match ::read(reader)? { - 0 => None, - 1 => { - Some(read_htlc_cache!()) - } - _ => return Err(DecodeError::InvalidValue), - }; let local_csv = Readable::read(reader)?; let key_storage = Readable::read(reader)?; @@ -369,8 +288,6 @@ impl ReadableArgs> for OnchainTx funding_redeemscript, local_commitment, prev_local_commitment, - current_htlc_cache, - prev_htlc_cache, local_csv, key_storage, claimable_outpoints, @@ -392,8 +309,6 @@ impl OnchainTxHandler { funding_redeemscript, local_commitment: None, prev_local_commitment: None, - current_htlc_cache: None, - prev_htlc_cache: None, local_csv, key_storage, pending_claim_requests: HashMap::new(), @@ -451,7 +366,7 @@ impl OnchainTxHandler { /// Lightning security model (i.e being able to redeem/timeout HTLC or penalize coutnerparty onchain) lays on the assumption of claim transactions getting confirmed before timelock expiration /// (CSV or CLTV following cases). In case of high-fee spikes, claim tx may stuck in the mempool, so you need to bump its feerate quickly using Replace-By-Fee or Child-Pay-For-Parent. - fn generate_claim_tx(&self, height: u32, cached_claim_datas: &ClaimTxBumpMaterial, fee_estimator: F) -> Option<(Option, u64, Transaction)> + fn generate_claim_tx(&mut self, height: u32, cached_claim_datas: &ClaimTxBumpMaterial, fee_estimator: F) -> Option<(Option, u64, Transaction)> where F::Target: FeeEstimator { if cached_claim_datas.per_input_material.len() == 0 { return None } // But don't prune pending claiming request yet, we may have to resurrect HTLCs @@ -530,13 +445,14 @@ impl OnchainTxHandler { inputs_witnesses_weight += Self::get_witnesses_weight(if preimage.is_some() { &[InputDescriptors::OfferedHTLC] } else { &[InputDescriptors::ReceivedHTLC] }); amt += *amount; }, - &InputMaterial::LocalHTLC { .. } => { return None; } + &InputMaterial::LocalHTLC { .. } => { + dynamic_fee = false; + }, &InputMaterial::Funding { .. } => { dynamic_fee = false; } } } - if dynamic_fee { let predicted_weight = bumped_tx.get_weight() + inputs_witnesses_weight; let mut new_feerate; @@ -598,16 +514,31 @@ impl OnchainTxHandler { } else { for (_, (outp, per_outp_material)) in cached_claim_datas.per_input_material.iter().enumerate() { match per_outp_material { - &InputMaterial::LocalHTLC { .. } => { - //TODO : Given that Local Commitment Transaction and HTLC-Timeout/HTLC-Success are counter-signed by peer, we can't - // RBF them. Need a Lightning specs change and package relay modification : - // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html + &InputMaterial::LocalHTLC { ref preimage, ref amount } => { + let mut htlc_tx = None; + if let Some(ref mut local_commitment) = self.local_commitment { + if local_commitment.txid() == outp.txid { + self.key_storage.sign_htlc_transaction(local_commitment, outp.vout, *preimage, self.local_csv, &self.secp_ctx); + htlc_tx = local_commitment.htlc_with_valid_witness(outp.vout).clone(); + } + } + if let Some(ref mut prev_local_commitment) = self.prev_local_commitment { + if prev_local_commitment.txid() == outp.txid { + self.key_storage.sign_htlc_transaction(prev_local_commitment, outp.vout, *preimage, self.local_csv, &self.secp_ctx); + htlc_tx = prev_local_commitment.htlc_with_valid_witness(outp.vout).clone(); + } + } + if let Some(htlc_tx) = htlc_tx { + let feerate = (amount - htlc_tx.output[0].value) * 1000 / htlc_tx.get_weight() as u64; + // Timer set to $NEVER given we can't bump tx without anchor outputs + log_trace!(self, "Going to broadcast Local HTLC-{} claiming HTLC output {} from {}...", if preimage.is_some() { "Success" } else { "Timeout" }, outp.vout, outp.txid); + return Some((None, feerate, htlc_tx)); + } return None; }, &InputMaterial::Funding { ref channel_value } => { - if self.local_commitment.is_some() { - let mut local_commitment = self.local_commitment.clone().unwrap(); - self.key_storage.sign_local_commitment(&mut local_commitment, &self.funding_redeemscript, *channel_value, &self.secp_ctx); + if let Some(ref mut local_commitment) = self.local_commitment { + self.key_storage.sign_local_commitment(local_commitment, &self.funding_redeemscript, *channel_value, &self.secp_ctx); let signed_tx = local_commitment.with_valid_witness().clone(); let mut amt_outputs = 0; for outp in signed_tx.output.iter() { @@ -673,7 +604,7 @@ impl OnchainTxHandler { } } - let mut bump_candidates = HashSet::new(); + let mut bump_candidates = HashMap::new(); for tx in txn_matched { // Scan all input to verify is one of the outpoint spent is of interest for us let mut claimed_outputs_material = Vec::new(); @@ -730,7 +661,7 @@ impl OnchainTxHandler { } //TODO: recompute soonest_timelock to avoid wasting a bit on fees if at_least_one_drop { - bump_candidates.insert(first_claim_txid_height.0.clone()); + bump_candidates.insert(first_claim_txid_height.0.clone(), claim_material.clone()); } } break; //No need to iterate further, either tx is our or their @@ -778,27 +709,25 @@ impl OnchainTxHandler { for (first_claim_txid, ref claim_data) in self.pending_claim_requests.iter() { if let Some(h) = claim_data.height_timer { if h == height { - bump_candidates.insert(*first_claim_txid); + bump_candidates.insert(*first_claim_txid, (*claim_data).clone()); } } } // Build, bump and rebroadcast tx accordingly log_trace!(self, "Bumping {} candidates", bump_candidates.len()); - for first_claim_txid in bump_candidates.iter() { - if let Some((new_timer, new_feerate)) = { - if let Some(claim_material) = self.pending_claim_requests.get(first_claim_txid) { - if let Some((new_timer, new_feerate, bump_tx)) = self.generate_claim_tx(height, &claim_material, &*fee_estimator) { - log_trace!(self, "Broadcast onchain {}", log_tx!(bump_tx)); - broadcaster.broadcast_transaction(&bump_tx); - Some((new_timer, new_feerate)) - } else { None } - } else { unreachable!(); } - } { - if let Some(claim_material) = self.pending_claim_requests.get_mut(first_claim_txid) { - claim_material.height_timer = new_timer; - claim_material.feerate_previous = new_feerate; - } else { unreachable!(); } + let mut pending_claim_updates = Vec::with_capacity(bump_candidates.len()); + for (first_claim_txid, claim_material) in bump_candidates.iter() { + if let Some((new_timer, new_feerate, bump_tx)) = self.generate_claim_tx(height, &claim_material, &*fee_estimator) { + log_trace!(self, "Broadcast onchain {}", log_tx!(bump_tx)); + broadcaster.broadcast_transaction(&bump_tx); + pending_claim_updates.push((*first_claim_txid, new_timer, new_feerate)); + } + } + for updates in pending_claim_updates { + if let Some(claim_material) = self.pending_claim_requests.get_mut(&updates.0) { + claim_material.height_timer = updates.1; + claim_material.feerate_previous = updates.2; } } } @@ -850,7 +779,7 @@ impl OnchainTxHandler { } } - pub(super) fn provide_latest_local_tx(&mut self, tx: LocalCommitmentTransaction, local_keys: chan_utils::TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>) -> Result<(), ()> { + pub(super) fn provide_latest_local_tx(&mut self, tx: LocalCommitmentTransaction) -> Result<(), ()> { // To prevent any unsafe state discrepancy between offchain and onchain, once local // commitment transaction has been signed due to an event (either block height for // HTLC-timeout or channel force-closure), don't allow any further update of local @@ -861,18 +790,6 @@ impl OnchainTxHandler { } self.prev_local_commitment = self.local_commitment.take(); self.local_commitment = Some(tx); - self.prev_htlc_cache = self.current_htlc_cache.take(); - let mut per_htlc = HashMap::with_capacity(htlc_outputs.len()); - for htlc in htlc_outputs { - if htlc.0.transaction_output_index.is_some() { // Discard dust HTLC as we will never have to generate onchain tx for them - per_htlc.insert(htlc.0.transaction_output_index.unwrap(), (htlc.0, htlc.1)); - } - } - self.current_htlc_cache = Some(HTLCTxCache { - local_keys, - feerate_per_kw, - per_htlc - }); Ok(()) } @@ -896,18 +813,10 @@ impl OnchainTxHandler { pub(super) fn get_fully_signed_htlc_tx(&mut self, txid: Sha256dHash, htlc_index: u32, preimage: Option) -> Option { //TODO: store preimage in OnchainTxHandler - if let Some(ref local_commitment) = self.local_commitment { + if let Some(ref mut local_commitment) = self.local_commitment { if local_commitment.txid() == txid { - if let Some(ref htlc_cache) = self.current_htlc_cache { - if let Some(htlc) = htlc_cache.per_htlc.get(&htlc_index) { - if !htlc.0.offered && preimage.is_none() { return None; }; // If we don't have preimage for HTLC-Success, don't try to generate - let htlc_secret = if !htlc.0.offered { preimage } else { None }; // If we have a preimage for a HTLC-Timeout, don't use it that's likely a duplicate HTLC hash - let mut htlc_tx = chan_utils::build_htlc_transaction(&txid, htlc_cache.feerate_per_kw, self.local_csv, &htlc.0, &htlc_cache.local_keys.a_delayed_payment_key, &htlc_cache.local_keys.revocation_key); - self.key_storage.sign_htlc_transaction(&mut htlc_tx, htlc.1.as_ref().unwrap(), &htlc_secret, &htlc.0, &htlc_cache.local_keys.a_htlc_key, &htlc_cache.local_keys.b_htlc_key, &htlc_cache.local_keys.revocation_key, &htlc_cache.local_keys.per_commitment_point, &self.secp_ctx); - return Some(htlc_tx); - - } - } + self.key_storage.sign_htlc_transaction(local_commitment, htlc_index, preimage, self.local_csv, &self.secp_ctx); + return local_commitment.htlc_with_valid_witness(htlc_index).clone(); } } None diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index 5e3aba43193..c48424f9da4 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -89,8 +89,8 @@ impl ChannelKeys for EnforcingChannelKeys { self.inner.unsafe_sign_local_commitment(local_commitment_tx, funding_redeemscript, channel_value_satoshis, secp_ctx); } - fn sign_htlc_transaction(&self, htlc_tx: &mut Transaction, their_sig: &Signature, preimage: &Option, htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey, per_commitment_point: &PublicKey, secp_ctx: &Secp256k1) { - self.inner.sign_htlc_transaction(htlc_tx, their_sig, preimage, htlc, a_htlc_key, b_htlc_key, revocation_key, per_commitment_point, secp_ctx); + fn sign_htlc_transaction(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option, local_csv: u16, secp_ctx: &Secp256k1) { + self.inner.sign_htlc_transaction(local_commitment_tx, htlc_index, preimage, local_csv, secp_ctx); } fn sign_closing_transaction(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1) -> Result { From 9faf6ca85f61058f0b4eba227b8dc848f07942e5 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Mon, 9 Mar 2020 19:56:42 -0400 Subject: [PATCH 16/19] Remove temporary anti-duplicata logic --- lightning/src/ln/functional_test_utils.rs | 2 +- lightning/src/ln/functional_tests.rs | 2 +- lightning/src/util/test_utils.rs | 15 --------------- 3 files changed, 2 insertions(+), 17 deletions(-) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index a2f9a7c3239..cf0dc1832d8 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1038,7 +1038,7 @@ pub fn fail_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: pub fn create_chanmon_cfgs(node_count: usize) -> Vec { let mut chan_mon_cfgs = Vec::new(); for _ in 0..node_count { - let tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), broadcasted_txn: Mutex::new(HashMap::new())}; + let tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new())}; let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; chan_mon_cfgs.push(TestChanMonCfg{ tx_broadcaster, fee_estimator }); } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index d45469194f2..8f140310842 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -6792,7 +6792,7 @@ fn test_data_loss_protect() { let logger: Arc = Arc::new(test_utils::TestLogger::with_id(format!("node {}", 0))); let mut chan_monitor = <(Sha256dHash, ChannelMonitor)>::read(&mut ::std::io::Cursor::new(previous_chan_monitor_state.0), Arc::clone(&logger)).unwrap().1; let chain_monitor = Arc::new(ChainWatchInterfaceUtil::new(Network::Testnet, Arc::clone(&logger))); - tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), broadcasted_txn: Mutex::new(HashMap::new())}; + tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new())}; fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; keys_manager = test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet, Arc::clone(&logger)); monitor = test_utils::TestChannelMonitor::new(chain_monitor.clone(), &tx_broadcaster, logger.clone(), &fee_estimator); diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 618cef0c208..13cbde4aa1f 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -110,24 +110,9 @@ impl<'a> channelmonitor::ManyChannelMonitor for TestChanne pub struct TestBroadcaster { pub txn_broadcasted: Mutex>, - pub broadcasted_txn: Mutex> // Temporary field while refactoring out tx duplication } impl chaininterface::BroadcasterInterface for TestBroadcaster { fn broadcast_transaction(&self, tx: &Transaction) { - let mut already = false; - { - if let Some(counter) = self.broadcasted_txn.lock().unwrap().get_mut(&tx.txid()) { - match counter { - 0 => { *counter = 1; already = true }, // We still authorize at least 2 duplicata for a given TXID to account ChannelManager/ChannelMonitor broadcast - 1 => return, - _ => panic!() - } - } - } - if !already { - self.broadcasted_txn.lock().unwrap().insert(tx.txid(), 0); - } - print!("\nFRESH BROADCAST {}\n\n", tx.txid()); self.txn_broadcasted.lock().unwrap().push(tx.clone()); } } From ba880e3662d7f01c8963fcac37d0b32ad2c9086c Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Mon, 6 Apr 2020 18:32:57 -0400 Subject: [PATCH 17/19] Make acces and signature of local commitment transaction unique Local commitment transaction broadcast can be triggered by a) a Channel force-close or b) reaching some block height implying a onchain HTLC-timeout. If one of this condition is fulfilled, commitment is signed and from then any state update would be rejected. ChannelMonitor init at Channel creation need to be refactored before to make get_fully_signed_local_tx infaillible to avoid choking in the test framework. --- lightning/src/ln/onchaintx.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index b2115c0f8ac..2f08fe291d2 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -537,18 +537,15 @@ impl OnchainTxHandler { return None; }, &InputMaterial::Funding { ref channel_value } => { - if let Some(ref mut local_commitment) = self.local_commitment { - self.key_storage.sign_local_commitment(local_commitment, &self.funding_redeemscript, *channel_value, &self.secp_ctx); - let signed_tx = local_commitment.with_valid_witness().clone(); - let mut amt_outputs = 0; - for outp in signed_tx.output.iter() { - amt_outputs += outp.value; - } - let feerate = (channel_value - amt_outputs) * 1000 / signed_tx.get_weight() as u64; - // Timer set to $NEVER given we can't bump tx without anchor outputs - log_trace!(self, "Going to broadcast Local Transaction {} claiming funding output {} from {}...", signed_tx.txid(), outp.vout, outp.txid); - return Some((None, feerate, signed_tx)); + let signed_tx = self.get_fully_signed_local_tx(*channel_value).unwrap(); + let mut amt_outputs = 0; + for outp in signed_tx.output.iter() { + amt_outputs += outp.value; } + let feerate = (channel_value - amt_outputs) * 1000 / signed_tx.get_weight() as u64; + // Timer set to $NEVER given we can't bump tx without anchor outputs + log_trace!(self, "Going to broadcast Local Transaction {} claiming funding output {} from {}...", signed_tx.txid(), outp.vout, outp.txid); + return Some((None, feerate, signed_tx)); } _ => unreachable!() } @@ -793,6 +790,10 @@ impl OnchainTxHandler { Ok(()) } + //TODO: getting lastest local transactions should be infaillible and result in us "force-closing the channel", but we may + // have empty local commitment transaction if a ChannelMonitor is asked to force-close just after Channel::get_outbound_funding_created, + // before providing a initial commitment transaction. For outbound channel, init ChannelMonitor at Channel::funding_signed, there is nothing + // to monitor before. pub(super) fn get_fully_signed_local_tx(&mut self, channel_value_satoshis: u64) -> Option { if let Some(ref mut local_commitment) = self.local_commitment { self.key_storage.sign_local_commitment(local_commitment, &self.funding_redeemscript, channel_value_satoshis, &self.secp_ctx); From 851ab92ea28f4d4c4c67383c9f935d0555d2efd9 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Mon, 6 Apr 2020 18:54:45 -0400 Subject: [PATCH 18/19] Monitor should panic on receiving buggy update sequences Channel shouldn't send a ChannelForceClosed update followed by a LatestLocalCommitmentTxInfo as it would be a programming error leading to risk of money loss. Force-closing the channel will broadcast the local commitment transaction, if the revocation secret for this one is released after its broadcast, it would allow remote party to claim outputs on this transaction using the revocation path. --- lightning/src/ln/channelmonitor.rs | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index d180d3bbdc0..cba28982d48 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -794,6 +794,9 @@ pub struct ChannelMonitor { #[cfg(not(test))] onchain_tx_handler: OnchainTxHandler, + // Used to detect programming bug due to unsafe monitor update sequence { ChannelForceClosed, LatestLocalCommitmentTXInfo } + lockdown_from_offchain: bool, + // We simply modify last_block_hash in Channel's block_connected so that serialization is // consistent but hopefully the users' copy handles block_connected in a consistent way. // (we do *not*, however, update them in update_monitor to ensure any local user copies keep @@ -1053,6 +1056,8 @@ impl ChannelMonitor { } self.onchain_tx_handler.write(writer)?; + self.lockdown_from_offchain.write(writer)?; + Ok(()) } @@ -1136,6 +1141,8 @@ impl ChannelMonitor { onchain_tx_handler: OnchainTxHandler::new(destination_script.clone(), keys, funding_redeemscript, their_to_self_delay, logger.clone()), + lockdown_from_offchain: false, + last_block_hash: Default::default(), secp_ctx: Secp256k1::new(), logger, @@ -1305,8 +1312,10 @@ impl ChannelMonitor { pub(super) fn update_monitor_ooo(&mut self, mut updates: ChannelMonitorUpdate) -> Result<(), MonitorUpdateError> { for update in updates.updates.drain(..) { match update { - ChannelMonitorUpdateStep::LatestLocalCommitmentTXInfo { commitment_tx, local_keys, feerate_per_kw, htlc_outputs } => - self.provide_latest_local_commitment_tx_info(commitment_tx, local_keys, feerate_per_kw, htlc_outputs)?, + ChannelMonitorUpdateStep::LatestLocalCommitmentTXInfo { commitment_tx, local_keys, feerate_per_kw, htlc_outputs } => { + if self.lockdown_from_offchain { panic!(); } + self.provide_latest_local_commitment_tx_info(commitment_tx, local_keys, feerate_per_kw, htlc_outputs)? + }, ChannelMonitorUpdateStep::LatestRemoteCommitmentTXInfo { unsigned_commitment_tx, htlc_outputs, commitment_number, their_revocation_point } => self.provide_latest_remote_commitment_tx_info(&unsigned_commitment_tx, htlc_outputs, commitment_number, their_revocation_point), ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage } => @@ -1334,8 +1343,10 @@ impl ChannelMonitor { } for update in updates.updates.drain(..) { match update { - ChannelMonitorUpdateStep::LatestLocalCommitmentTXInfo { commitment_tx, local_keys, feerate_per_kw, htlc_outputs } => - self.provide_latest_local_commitment_tx_info(commitment_tx, local_keys, feerate_per_kw, htlc_outputs)?, + ChannelMonitorUpdateStep::LatestLocalCommitmentTXInfo { commitment_tx, local_keys, feerate_per_kw, htlc_outputs } => { + if self.lockdown_from_offchain { panic!(); } + self.provide_latest_local_commitment_tx_info(commitment_tx, local_keys, feerate_per_kw, htlc_outputs)? + }, ChannelMonitorUpdateStep::LatestRemoteCommitmentTXInfo { unsigned_commitment_tx, htlc_outputs, commitment_number, their_revocation_point } => self.provide_latest_remote_commitment_tx_info(&unsigned_commitment_tx, htlc_outputs, commitment_number, their_revocation_point), ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage } => @@ -1345,6 +1356,7 @@ impl ChannelMonitor { ChannelMonitorUpdateStep::RescueRemoteCommitmentTXInfo { their_current_per_commitment_point } => self.provide_rescue_remote_commitment_tx_info(their_current_per_commitment_point), ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast } => { + self.lockdown_from_offchain = true; if should_broadcast { self.broadcast_latest_local_commitment_txn(broadcaster); } else { @@ -2485,6 +2497,8 @@ impl ReadableArgs> for (Sha256dH } let onchain_tx_handler = ReadableArgs::read(reader, logger.clone())?; + let lockdown_from_offchain = Readable::read(reader)?; + Ok((last_block_hash.clone(), ChannelMonitor { latest_update_id, commitment_transaction_number_obscure_factor, @@ -2523,6 +2537,8 @@ impl ReadableArgs> for (Sha256dH onchain_tx_handler, + lockdown_from_offchain, + last_block_hash, secp_ctx: Secp256k1::new(), logger, From 95830edac76531fe20f9177eca532e9d73de5179 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Tue, 7 Apr 2020 21:07:55 -0400 Subject: [PATCH 19/19] Add test_update_err_monitor_lockdown This test tries the new lockdown logic in case of a signed-and-broadcast local commitment transaction while a concurrent ChannelMonitorUpdate for a next _local_ commitment is submitted from offchain. Update is rejected as expected with a ChannelMonitorUpdateErr. --- lightning/src/ln/functional_tests.rs | 63 ++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 8f140310842..1be967d939d 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -4,10 +4,12 @@ use chain::transaction::OutPoint; use chain::keysinterface::{ChannelKeys, KeysInterface, SpendableOutputDescriptor}; +use chain::chaininterface; use chain::chaininterface::{ChainListener, ChainWatchInterfaceUtil, BlockNotifier}; use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC}; use ln::channelmanager::{ChannelManager,ChannelManagerReadArgs,HTLCForwardInfo,RAACommitmentOrder, PaymentPreimage, PaymentHash, PaymentSecret, PaymentSendFailure, BREAKDOWN_TIMEOUT}; use ln::channelmonitor::{ChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ManyChannelMonitor, ANTI_REORG_DELAY}; +use ln::channelmonitor; use ln::channel::{Channel, ChannelError}; use ln::{chan_utils, onion_utils}; use ln::router::{Route, RouteHop}; @@ -7581,3 +7583,64 @@ fn test_simple_mpp() { // ...but with the right secret we should be able to claim all the way back claim_payment_along_route_with_secret(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_preimage, Some(payment_secret), 200_000); } + +#[test] +fn test_update_err_monitor_lockdown() { + // Our monitor will lock update of local commitment transaction if a broadcastion condition + // has been fulfilled (either force-close from Channel or block height requiring a HTLC- + // timeout). Trying to update monitor after lockdown should return a ChannelMonitorUpdateErr. + // + // This scenario may happen in a watchtower setup, where watchtower process a block height + // triggering a timeout while a slow-block-processing ChannelManager receives a local signed + // commitment at same time. + + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + // Create some initial channel + let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::supported(), InitFeatures::supported()); + let outpoint = OutPoint { txid: chan_1.3.txid(), index: 0 }; + + // Rebalance the network to generate htlc in the two directions + send_payment(&nodes[0], &vec!(&nodes[1])[..], 10_000_000, 10_000_000); + + // Route a HTLC from node 0 to node 1 (but don't settle) + let preimage = route_payment(&nodes[0], &vec!(&nodes[1])[..], 9_000_000).0; + + // Copy SimpleManyChannelMonitor to simulate a watchtower and update block height of node 0 until its ChannelMonitor timeout HTLC onchain + let logger = Arc::new(test_utils::TestLogger::with_id(format!("node {}", 0))); + let watchtower = { + let monitors = nodes[0].chan_monitor.simple_monitor.monitors.lock().unwrap(); + let monitor = monitors.get(&outpoint).unwrap(); + let mut w = test_utils::TestVecWriter(Vec::new()); + monitor.write_for_disk(&mut w).unwrap(); + let new_monitor = <(Sha256dHash, channelmonitor::ChannelMonitor)>::read( + &mut ::std::io::Cursor::new(&w.0), Arc::new(test_utils::TestLogger::new())).unwrap().1; + assert!(new_monitor == *monitor); + let chain_monitor = Arc::new(chaininterface::ChainWatchInterfaceUtil::new(Network::Testnet, logger.clone() as Arc)); + let watchtower = test_utils::TestChannelMonitor::new(chain_monitor, &chanmon_cfgs[0].tx_broadcaster, logger.clone(), &chanmon_cfgs[0].fee_estimator); + assert!(watchtower.add_monitor(outpoint, new_monitor).is_ok()); + watchtower + }; + let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; + watchtower.simple_monitor.block_connected(&header, 200, &vec![], &vec![]); + + // Try to update ChannelMonitor + assert!(nodes[1].node.claim_funds(preimage, &None, 9_000_000)); + check_added_monitors!(nodes[1], 1); + let updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + assert_eq!(updates.update_fulfill_htlcs.len(), 1); + nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]); + if let Some(ref mut channel) = nodes[0].node.channel_state.lock().unwrap().by_id.get_mut(&chan_1.2) { + if let Ok((_, _, _, update)) = channel.commitment_signed(&updates.commitment_signed, &node_cfgs[0].fee_estimator) { + if let Err(_) = watchtower.simple_monitor.update_monitor(outpoint, update.clone()) {} else { assert!(false); } + if let Ok(_) = nodes[0].chan_monitor.update_monitor(outpoint, update) {} else { assert!(false); } + } else { assert!(false); } + } else { assert!(false); }; + // Our local monitor is in-sync and hasn't processed yet timeout + check_added_monitors!(nodes[0], 1); + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); +}