From 3f2efcdfa73ee703093107f908cc7eeb0aa467e8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 26 Mar 2021 18:07:24 -0400 Subject: [PATCH] Take the full funding transaction from the user on generation Instead of relying on the user to ensure the funding transaction is correct (and panicing when it is confirmed), we should check it is correct when it is generated. By taking the full funding transaciton from the user on generation, we can also handle broadcasting for them instead of doing so via an event. --- background-processor/src/lib.rs | 7 +- fuzz/src/chanmon_consistency.rs | 8 +- fuzz/src/full_stack.rs | 25 ++--- lightning/src/ln/chanmon_update_fail_tests.rs | 13 +-- lightning/src/ln/channel.rs | 50 +++++----- lightning/src/ln/channelmanager.rs | 91 +++++++++++++------ lightning/src/ln/functional_test_utils.rs | 15 ++- lightning/src/ln/functional_tests.rs | 50 ++++------ lightning/src/util/events.rs | 40 ++------ 9 files changed, 148 insertions(+), 151 deletions(-) diff --git a/background-processor/src/lib.rs b/background-processor/src/lib.rs index c3db4d55..49be55cf 100644 --- a/background-processor/src/lib.rs +++ b/background-processor/src/lib.rs @@ -189,7 +189,7 @@ mod tests { $node_a.node.handle_accept_channel(&$node_b.node.get_our_node_id(), InitFeatures::known(), &get_event_msg!($node_b, MessageSendEvent::SendAcceptChannel, $node_a.node.get_our_node_id())); let events = $node_a.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); - let (temporary_channel_id, tx, funding_output) = match events[0] { + let (temporary_channel_id, tx) = match events[0] { Event::FundingGenerationReady { ref temporary_channel_id, ref channel_value_satoshis, ref output_script, user_channel_id } => { assert_eq!(*channel_value_satoshis, $channel_value); assert_eq!(user_channel_id, 42); @@ -197,13 +197,12 @@ mod tests { let tx = Transaction { version: 1 as i32, lock_time: 0, input: Vec::new(), output: vec![TxOut { value: *channel_value_satoshis, script_pubkey: output_script.clone(), }]}; - let funding_outpoint = OutPoint { txid: tx.txid(), index: 0 }; - (*temporary_channel_id, tx, funding_outpoint) + (*temporary_channel_id, tx) }, _ => panic!("Unexpected event"), }; - $node_a.node.funding_transaction_generated(&temporary_channel_id, funding_output); + $node_a.node.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap(); $node_b.node.handle_funding_created(&$node_a.node.get_our_node_id(), &get_event_msg!($node_a, MessageSendEvent::SendFundingCreated, $node_b.node.get_our_node_id())); $node_a.node.handle_funding_signed(&$node_b.node.get_our_node_id(), &get_event_msg!($node_b, MessageSendEvent::SendFundingSigned, $node_a.node.get_our_node_id())); tx diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 7c5875ac..3feeaf46 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -397,7 +397,7 @@ pub fn do_test(data: &[u8], out: Out) { value: *channel_value_satoshis, script_pubkey: output_script.clone(), }]}; funding_output = OutPoint { txid: tx.txid(), index: 0 }; - $source.funding_transaction_generated(&temporary_channel_id, funding_output); + $source.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap(); channel_txn.push(tx); } else { panic!("Wrong event type"); } } @@ -420,12 +420,6 @@ pub fn do_test(data: &[u8], out: Out) { }; $source.handle_funding_signed(&$dest.get_our_node_id(), &funding_signed); - { - let events = $source.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - if let events::Event::FundingBroadcastSafe { .. } = events[0] { - } else { panic!("Wrong event type"); } - } funding_output } } } diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index bffb3e8e..3acf7ba5 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -51,7 +51,7 @@ use bitcoin::secp256k1::Secp256k1; use std::cell::RefCell; use std::collections::{HashMap, hash_map}; use std::cmp; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; use std::sync::atomic::{AtomicU64,AtomicUsize,Ordering}; #[inline] @@ -116,9 +116,13 @@ impl FeeEstimator for FuzzEstimator { } } -struct TestBroadcaster {} +struct TestBroadcaster { + txn_broadcasted: Mutex>, +} impl BroadcasterInterface for TestBroadcaster { - fn broadcast_transaction(&self, _tx: &Transaction) {} + fn broadcast_transaction(&self, tx: &Transaction) { + self.txn_broadcasted.lock().unwrap().push(tx.clone()); + } } #[derive(Clone)] @@ -340,7 +344,7 @@ pub fn do_test(data: &[u8], logger: &Arc) { Err(_) => return, }; - let broadcast = Arc::new(TestBroadcaster{}); + let broadcast = Arc::new(TestBroadcaster{ txn_broadcasted: Mutex::new(Vec::new()) }); let monitor = Arc::new(chainmonitor::ChainMonitor::new(None, broadcast.clone(), Arc::clone(&logger), fee_est.clone(), Arc::new(TestPersister{}))); let keys_manager = Arc::new(KeyProvider { node_secret: our_network_key.clone(), counter: AtomicU64::new(0) }); @@ -370,7 +374,6 @@ pub fn do_test(data: &[u8], logger: &Arc) { let mut payments_sent = 0; let mut pending_funding_generation: Vec<([u8; 32], u64, Script)> = Vec::new(); let mut pending_funding_signatures = HashMap::new(); - let mut pending_funding_relay = Vec::new(); loop { match get_slice!(1)[0] { @@ -513,18 +516,19 @@ pub fn do_test(data: &[u8], logger: &Arc) { continue 'outer_loop; } }; - channelmanager.funding_transaction_generated(&funding_generation.0, funding_output.clone()); + channelmanager.funding_transaction_generated(&funding_generation.0, tx.clone()).unwrap(); pending_funding_signatures.insert(funding_output, tx); } }, 11 => { - if !pending_funding_relay.is_empty() { - loss_detector.connect_block(&pending_funding_relay[..]); + let mut txn = broadcast.txn_broadcasted.lock().unwrap(); + if !txn.is_empty() { + loss_detector.connect_block(&txn[..]); for _ in 2..100 { loss_detector.connect_block(&[]); } } - for tx in pending_funding_relay.drain(..) { + for tx in txn.drain(..) { loss_detector.funding_txn.push(tx); } }, @@ -566,9 +570,6 @@ pub fn do_test(data: &[u8], logger: &Arc) { Event::FundingGenerationReady { temporary_channel_id, channel_value_satoshis, output_script, .. } => { pending_funding_generation.push((temporary_channel_id, channel_value_satoshis, output_script)); }, - Event::FundingBroadcastSafe { funding_txo, .. } => { - pending_funding_relay.push(pending_funding_signatures.remove(&funding_txo).unwrap()); - }, Event::PaymentReceived { payment_hash, payment_secret, amt } => { //TODO: enhance by fetching random amounts from fuzz input? payments_received.push((payment_hash, payment_secret, amt)); diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index a4cc5a02..dc37e3f6 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -1825,7 +1825,7 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43); - nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output); + nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_tx.clone()).unwrap(); check_added_monitors!(nodes[0], 0); *nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure)); @@ -1846,14 +1846,9 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: check_added_monitors!(nodes[0], 0); let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { - Event::FundingBroadcastSafe { ref funding_txo, user_channel_id } => { - assert_eq!(user_channel_id, 43); - assert_eq!(*funding_txo, funding_output); - }, - _ => panic!("Unexpected event"), - }; + assert_eq!(events.len(), 0); + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1); + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0)[0].txid(), funding_output.txid); if confirm_a_first { confirm_transaction(&nodes[0], &funding_tx); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index ca1887d8..174aecd3 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -409,9 +409,9 @@ pub(super) struct Channel { counterparty_forwarding_info: Option, pub(crate) channel_transaction_parameters: ChannelTransactionParameters, + funding_transaction: Option, counterparty_cur_commitment_point: Option, - counterparty_prev_commitment_point: Option, counterparty_node_id: PublicKey, @@ -603,8 +603,9 @@ impl Channel { counterparty_parameters: None, funding_outpoint: None }, - counterparty_cur_commitment_point: None, + funding_transaction: None, + counterparty_cur_commitment_point: None, counterparty_prev_commitment_point: None, counterparty_node_id, @@ -844,8 +845,9 @@ impl Channel { }), funding_outpoint: None }, - counterparty_cur_commitment_point: Some(msg.first_per_commitment_point), + funding_transaction: None, + counterparty_cur_commitment_point: Some(msg.first_per_commitment_point), counterparty_prev_commitment_point: None, counterparty_node_id, @@ -1608,7 +1610,7 @@ impl Channel { /// Handles a funding_signed message from the remote end. /// If this call is successful, broadcast the funding transaction (and not before!) - pub fn funding_signed(&mut self, msg: &msgs::FundingSigned, last_block_hash: BlockHash, logger: &L) -> Result, ChannelError> where L::Target: Logger { + pub fn funding_signed(&mut self, msg: &msgs::FundingSigned, last_block_hash: BlockHash, logger: &L) -> Result<(ChannelMonitor, Transaction), ChannelError> where L::Target: Logger { if !self.is_outbound() { return Err(ChannelError::Close("Received funding_signed for an inbound channel?".to_owned())); } @@ -1670,7 +1672,7 @@ impl Channel { self.cur_holder_commitment_transaction_number -= 1; self.cur_counterparty_commitment_transaction_number -= 1; - Ok(channel_monitor) + Ok((channel_monitor, self.funding_transaction.as_ref().cloned().unwrap())) } pub fn funding_locked(&mut self, msg: &msgs::FundingLocked) -> Result<(), ChannelError> { @@ -2771,20 +2773,21 @@ impl Channel { /// Indicates that the latest ChannelMonitor update has been committed by the client /// successfully and we should restore normal operation. Returns messages which should be sent /// to the remote side. - pub fn monitor_updating_restored(&mut self, logger: &L) -> (Option, Option, RAACommitmentOrder, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, bool, Option) where L::Target: Logger { + pub fn monitor_updating_restored(&mut self, logger: &L) -> (Option, Option, RAACommitmentOrder, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, Option, Option) where L::Target: Logger { assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, ChannelState::MonitorUpdateFailed as u32); self.channel_state &= !(ChannelState::MonitorUpdateFailed as u32); - let needs_broadcast_safe = self.channel_state & (ChannelState::FundingSent as u32) != 0 && self.is_outbound(); + let funding_broadcastable = if self.channel_state & (ChannelState::FundingSent as u32) != 0 && self.is_outbound() { + self.funding_transaction.take() + } else { None }; - // Because we will never generate a FundingBroadcastSafe event when we're in - // MonitorUpdateFailed, if we assume the user only broadcast the funding transaction when - // they received the FundingBroadcastSafe event, we can only ever hit - // monitor_pending_funding_locked when we're an inbound channel which failed to persist the - // monitor on funding_created, and we even got the funding transaction confirmed before the - // monitor was persisted. + // We will never broadcast the funding transaction when we're in MonitorUpdateFailed (and + // we assume the user never directly broadcasts the funding transaction and waits for us to + // do it). Thus, we can only ever hit monitor_pending_funding_locked when we're an inbound + // channel which failed to persist the monitor on funding_created, and we got the funding + // transaction confirmed before the monitor was persisted. let funding_locked = if self.monitor_pending_funding_locked { - assert!(!self.is_outbound(), "Funding transaction broadcast without FundingBroadcastSafe!"); + assert!(!self.is_outbound(), "Funding transaction broadcast by the local client before it should have - LDK didn't do it!"); self.monitor_pending_funding_locked = false; let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx); Some(msgs::FundingLocked { @@ -2801,7 +2804,7 @@ impl Channel { if self.channel_state & (ChannelState::PeerDisconnected as u32) != 0 { self.monitor_pending_revoke_and_ack = false; self.monitor_pending_commitment_signed = false; - return (None, None, RAACommitmentOrder::RevokeAndACKFirst, forwards, failures, needs_broadcast_safe, funding_locked); + return (None, None, RAACommitmentOrder::RevokeAndACKFirst, forwards, failures, funding_broadcastable, funding_locked); } let raa = if self.monitor_pending_revoke_and_ack { @@ -2815,11 +2818,11 @@ impl Channel { self.monitor_pending_commitment_signed = false; let order = self.resend_order.clone(); log_trace!(logger, "Restored monitor updating resulting in {}{} commitment update and {} RAA, with {} first", - if needs_broadcast_safe { "a funding broadcast safe, " } else { "" }, + if funding_broadcastable.is_some() { "a funding broadcastable, " } else { "" }, if commitment_update.is_some() { "a" } else { "no" }, if raa.is_some() { "an" } else { "no" }, match order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"}); - (raa, commitment_update, order, forwards, failures, needs_broadcast_safe, funding_locked) + (raa, commitment_update, order, forwards, failures, funding_broadcastable, funding_locked) } pub fn update_fee(&mut self, fee_estimator: &F, msg: &msgs::UpdateFee) -> Result<(), ChannelError> @@ -3734,7 +3737,7 @@ impl Channel { /// Note that channel_id changes during this call! /// Do NOT broadcast the funding transaction until after a successful funding_signed call! /// If an Err is returned, it is a ChannelError::Close. - pub fn get_outbound_funding_created(&mut self, funding_txo: OutPoint, logger: &L) -> Result where L::Target: Logger { + pub fn get_outbound_funding_created(&mut self, funding_transaction: Transaction, funding_txo: OutPoint, logger: &L) -> Result where L::Target: Logger { if !self.is_outbound() { panic!("Tried to create outbound funding_created message on an inbound channel!"); } @@ -3765,6 +3768,7 @@ impl Channel { self.channel_state = ChannelState::FundingCreated as u32; self.channel_id = funding_txo.to_channel_id(); + self.funding_transaction = Some(funding_transaction); Ok(msgs::FundingCreated { temporary_channel_id, @@ -4489,8 +4493,9 @@ impl Writeable for Channel { } self.channel_transaction_parameters.write(writer)?; - self.counterparty_cur_commitment_point.write(writer)?; + self.funding_transaction.write(writer)?; + self.counterparty_cur_commitment_point.write(writer)?; self.counterparty_prev_commitment_point.write(writer)?; self.counterparty_node_id.write(writer)?; @@ -4659,6 +4664,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel }; let channel_parameters = Readable::read(reader)?; + let funding_transaction = Readable::read(reader)?; + let counterparty_cur_commitment_point = Readable::read(reader)?; let counterparty_prev_commitment_point = Readable::read(reader)?; @@ -4731,8 +4738,9 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel counterparty_forwarding_info, channel_transaction_parameters: channel_parameters, - counterparty_cur_commitment_point, + funding_transaction, + counterparty_cur_commitment_point, counterparty_prev_commitment_point, counterparty_node_id, @@ -5000,7 +5008,7 @@ mod tests { value: 10000000, script_pubkey: output_script.clone(), }]}; let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 }; - let funding_created_msg = node_a_chan.get_outbound_funding_created(funding_outpoint, &&logger).unwrap(); + let funding_created_msg = node_a_chan.get_outbound_funding_created(tx.clone(), funding_outpoint, &&logger).unwrap(); let (funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg, last_block_hash, &&logger).unwrap(); // Node B --> Node A: funding signed diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 50f8ccbb..2f7c17b5 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -19,6 +19,7 @@ //! use bitcoin::blockdata::block::{Block, BlockHeader}; +use bitcoin::blockdata::transaction::Transaction; use bitcoin::blockdata::constants::genesis_block; use bitcoin::network::constants::Network; @@ -850,10 +851,11 @@ impl ChannelMana /// Creates a new outbound channel to the given remote node and with the given value. /// - /// user_id will be provided back as user_channel_id in FundingGenerationReady and - /// FundingBroadcastSafe events to allow tracking of which events correspond with which - /// create_channel call. Note that user_channel_id defaults to 0 for inbound channels, so you - /// may wish to avoid using 0 for user_id here. + /// user_id will be provided back as user_channel_id in FundingGenerationReady events to allow + /// tracking of which events correspond with which create_channel call. Note that the + /// user_channel_id defaults to 0 for inbound channels, so you may wish to avoid using 0 for + /// user_id here. user_id has no meaning inside of LDK, it is simply copied to events and + /// otherwise ignored. /// /// If successful, will generate a SendOpenChannel message event, so you should probably poll /// PeerManager::process_events afterwards. @@ -1525,32 +1527,75 @@ impl ChannelMana /// Call this upon creation of a funding transaction for the given channel. /// - /// Note that ALL inputs in the transaction pointed to by funding_txo MUST spend SegWit outputs - /// or your counterparty can steal your funds! + /// Returns an [`APIError::APIMisuseError`] if the funding_transaction spent non-SegWit outputs + /// or if no output was found which matches the parameters in [`Event::FundingGenerationReady`]. /// /// Panics if a funding transaction has already been provided for this channel. /// - /// May panic if the funding_txo is duplicative with some other channel (note that this should - /// be trivially prevented by using unique funding transaction keys per-channel). - pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_txo: OutPoint) { + /// May panic if the output found in the funding transaction is duplicative with some other + /// channel (note that this should be trivially prevented by using unique funding transaction + /// keys per-channel). + /// + /// Do NOT broadcast the funding transaction yourself. When we have safely received our + /// counterparty's signature the funding transaction will automatically be broadcast via the + /// [`BroadcasterInterface`] provided when this `ChannelManager` was constructed. + /// + /// Note that this includes RBF or similar transaction replacement strategies - lightning does + /// not currently support replacing a funding transaction on an existing channel. Instead, + /// create a new channel with a conflicting funding transaction. + pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_transaction: Transaction) -> Result<(), APIError> { let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier); + for inp in funding_transaction.input.iter() { + if inp.witness.is_empty() { + return Err(APIError::APIMisuseError { + err: "Funding transaction must be fully signed and spend Segwit outputs".to_owned() + }); + } + } + let (chan, msg) = { let (res, chan) = match self.channel_state.lock().unwrap().by_id.remove(temporary_channel_id) { Some(mut chan) => { - (chan.get_outbound_funding_created(funding_txo, &self.logger) + let mut output_index = None; + let expected_spk = chan.get_funding_redeemscript().to_v0_p2wsh(); + for (idx, outp) in funding_transaction.output.iter().enumerate() { + if outp.script_pubkey == expected_spk && outp.value == chan.get_value_satoshis() { + if output_index.is_some() { + return Err(APIError::APIMisuseError { + err: "Multiple outputs matched the expected script and value".to_owned() + }); + } + if idx > u16::max_value() as usize { + return Err(APIError::APIMisuseError { + err: "Transaction had more than 2^16 outputs, which is not supported".to_owned() + }); + } + output_index = Some(idx as u16); + } + } + if output_index.is_none() { + return Err(APIError::APIMisuseError { + err: "No output matched the script_pubkey and value in the FundingGenerationReady event".to_owned() + }); + } + let funding_txo = OutPoint { txid: funding_transaction.txid(), index: output_index.unwrap() }; + + (chan.get_outbound_funding_created(funding_transaction, funding_txo, &self.logger) .map_err(|e| if let ChannelError::Close(msg) = e { MsgHandleErrInternal::from_finish_shutdown(msg, chan.channel_id(), chan.force_shutdown(true), None) } else { unreachable!(); }) , chan) }, - None => return + None => { return Err(APIError::ChannelUnavailable { err: "No such channel".to_owned() }) }, }; match handle_error!(self, res, chan.get_counterparty_node_id()) { Ok(funding_msg) => { (chan, funding_msg) }, - Err(_) => { return; } + Err(_) => { return Err(APIError::ChannelUnavailable { + err: "Error deriving keys or signing initial commitment transactions - either our RNG or our counterparty's RNG is broken or the Signer refused to sign".to_owned() + }) }, } }; @@ -1567,6 +1612,7 @@ impl ChannelMana e.insert(chan); } } + Ok(()) } fn get_announcement_sigs(&self, chan: &Channel) -> Option { @@ -2359,7 +2405,7 @@ impl ChannelMana return; } - let (raa, commitment_update, order, pending_forwards, mut pending_failures, needs_broadcast_safe, funding_locked) = channel.monitor_updating_restored(&self.logger); + let (raa, commitment_update, order, pending_forwards, mut pending_failures, funding_broadcastable, funding_locked) = channel.monitor_updating_restored(&self.logger); if !pending_forwards.is_empty() { htlc_forwards.push((channel.get_short_channel_id().expect("We can't have pending forwards before funding confirmation"), funding_txo.clone(), pending_forwards)); } @@ -2391,11 +2437,8 @@ impl ChannelMana handle_cs!(); }, } - if needs_broadcast_safe { - pending_events.push(events::Event::FundingBroadcastSafe { - funding_txo: channel.get_funding_txo().unwrap(), - user_channel_id: channel.get_user_id(), - }); + if let Some(tx) = funding_broadcastable { + self.tx_broadcaster.broadcast_transaction(&tx); } if let Some(msg) = funding_locked { pending_msg_events.push(events::MessageSendEvent::SendFundingLocked { @@ -2529,7 +2572,7 @@ impl ChannelMana } fn internal_funding_signed(&self, counterparty_node_id: &PublicKey, msg: &msgs::FundingSigned) -> Result<(), MsgHandleErrInternal> { - let (funding_txo, user_id) = { + let funding_tx = { let last_block_hash = *self.last_block_hash.read().unwrap(); let mut channel_lock = self.channel_state.lock().unwrap(); let channel_state = &mut *channel_lock; @@ -2538,23 +2581,19 @@ impl ChannelMana if chan.get().get_counterparty_node_id() != *counterparty_node_id { return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id)); } - let monitor = match chan.get_mut().funding_signed(&msg, last_block_hash, &self.logger) { + let (monitor, funding_tx) = match chan.get_mut().funding_signed(&msg, last_block_hash, &self.logger) { Ok(update) => update, Err(e) => try_chan_entry!(self, Err(e), channel_state, chan), }; if let Err(e) = self.chain_monitor.watch_channel(chan.get().get_funding_txo().unwrap(), monitor) { return_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::RevokeAndACKFirst, false, false); } - (chan.get().get_funding_txo().unwrap(), chan.get().get_user_id()) + funding_tx }, hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id)) } }; - let mut pending_events = self.pending_events.lock().unwrap(); - pending_events.push(events::Event::FundingBroadcastSafe { - funding_txo, - user_channel_id: user_id, - }); + self.tx_broadcaster.broadcast_transaction(&funding_tx); Ok(()) } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index e7f61c6a..2ebdac5d 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -421,7 +421,7 @@ pub fn create_chan_between_nodes_with_value_init<'a, 'b, 'c>(node_a: &Node<'a, ' let (temporary_channel_id, tx, funding_output) = create_funding_transaction(node_a, channel_value, 42); - node_a.node.funding_transaction_generated(&temporary_channel_id, funding_output); + node_a.node.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap(); check_added_monitors!(node_a, 0); node_b.node.handle_funding_created(&node_a.node.get_our_node_id(), &get_event_msg!(node_a, MessageSendEvent::SendFundingCreated, node_b.node.get_our_node_id())); @@ -441,14 +441,11 @@ pub fn create_chan_between_nodes_with_value_init<'a, 'b, 'c>(node_a: &Node<'a, ' } let events_4 = node_a.node.get_and_clear_pending_events(); - assert_eq!(events_4.len(), 1); - match events_4[0] { - Event::FundingBroadcastSafe { ref funding_txo, user_channel_id } => { - assert_eq!(user_channel_id, 42); - assert_eq!(*funding_txo, funding_output); - }, - _ => panic!("Unexpected event"), - }; + assert_eq!(events_4.len(), 0); + + assert_eq!(node_a.tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1); + assert_eq!(node_a.tx_broadcaster.txn_broadcasted.lock().unwrap()[0], tx); + node_a.tx_broadcaster.txn_broadcasted.lock().unwrap().clear(); tx } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index c7a79e32..7c198e48 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -460,7 +460,7 @@ fn do_test_sanity_on_in_flight_opens(steps: u8) { let (temporary_channel_id, tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 42); if steps & 0x0f == 3 { return; } - nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output); + nodes[0].node.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap(); check_added_monitors!(nodes[0], 0); let funding_created = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id()); @@ -484,14 +484,7 @@ fn do_test_sanity_on_in_flight_opens(steps: u8) { } let events_4 = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events_4.len(), 1); - match events_4[0] { - Event::FundingBroadcastSafe { ref funding_txo, user_channel_id } => { - assert_eq!(user_channel_id, 42); - assert_eq!(*funding_txo, funding_output); - }, - _ => panic!("Unexpected event"), - }; + assert_eq!(events_4.len(), 0); if steps & 0x0f == 6 { return; } create_chan_between_nodes_with_value_confirm_first(&nodes[0], &nodes[1], &tx, 2); @@ -4320,7 +4313,7 @@ fn test_manager_serialize_deserialize_events() { let nodes_0_deserialized: ChannelManager; let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); - // Start creating a channel, but stop right before broadcasting the event message FundingBroadcastSafe + // Start creating a channel, but stop right before broadcasting the funding transaction let channel_value = 100000; let push_msat = 10001; let a_flags = InitFeatures::known(); @@ -4333,7 +4326,7 @@ fn test_manager_serialize_deserialize_events() { let (temporary_channel_id, tx, funding_output) = create_funding_transaction(&node_a, channel_value, 42); - node_a.node.funding_transaction_generated(&temporary_channel_id, funding_output); + node_a.node.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap(); check_added_monitors!(node_a, 0); node_b.node.handle_funding_created(&node_a.node.get_our_node_id(), &get_event_msg!(node_a, MessageSendEvent::SendFundingCreated, node_b.node.get_our_node_id())); @@ -4351,7 +4344,7 @@ fn test_manager_serialize_deserialize_events() { assert_eq!(added_monitors[0].0, funding_output); added_monitors.clear(); } - // Normally, this is where node_a would check for a FundingBroadcastSafe event, but the test de/serializes first instead + // Normally, this is where node_a would broadcast the funding transaction, but the test de/serializes first instead nodes.push(node_a); nodes.push(node_b); @@ -4395,16 +4388,11 @@ fn test_manager_serialize_deserialize_events() { assert!(nodes[0].chain_monitor.watch_channel(chan_0_monitor.get_funding_txo().0, chan_0_monitor).is_ok()); nodes[0].node = &nodes_0_deserialized; - // After deserializing, make sure the FundingBroadcastSafe event is still held by the channel manager + // After deserializing, make sure the funding_transaction is still held by the channel manager let events_4 = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events_4.len(), 1); - match events_4[0] { - Event::FundingBroadcastSafe { ref funding_txo, user_channel_id } => { - assert_eq!(user_channel_id, 42); - assert_eq!(*funding_txo, funding_output); - }, - _ => panic!("Unexpected event"), - }; + assert_eq!(events_4.len(), 0); + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1); + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap()[0].txid(), funding_output.txid); // Make sure the channel is functioning as though the de/serialization never happened assert_eq!(nodes[0].node.list_channels().len(), 1); @@ -8347,9 +8335,9 @@ fn test_pre_lockin_no_chan_closed_update() { nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &accept_chan_msg); // Move the first channel through the funding flow... - let (temporary_channel_id, _tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 42); + let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[0], 100000, 42); - nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output); + nodes[0].node.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap(); check_added_monitors!(nodes[0], 0); let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id()); @@ -8631,7 +8619,7 @@ fn test_duplicate_chan_id() { // Move the first channel through the funding flow... let (temporary_channel_id, tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 42); - nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output); + nodes[0].node.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap(); check_added_monitors!(nodes[0], 0); let mut funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id()); @@ -8680,7 +8668,7 @@ fn test_duplicate_chan_id() { let mut a_channel_lock = nodes[0].node.channel_state.lock().unwrap(); let mut as_chan = a_channel_lock.by_id.get_mut(&open_chan_2_msg.temporary_channel_id).unwrap(); let logger = test_utils::TestLogger::new(); - as_chan.get_outbound_funding_created(funding_outpoint, &&logger).unwrap() + as_chan.get_outbound_funding_created(tx.clone(), funding_outpoint, &&logger).unwrap() }; check_added_monitors!(nodes[0], 0); nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created); @@ -8714,14 +8702,9 @@ fn test_duplicate_chan_id() { } let events_4 = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events_4.len(), 1); - match events_4[0] { - Event::FundingBroadcastSafe { ref funding_txo, user_channel_id } => { - assert_eq!(user_channel_id, 42); - assert_eq!(*funding_txo, funding_output); - }, - _ => panic!("Unexpected event"), - }; + assert_eq!(events_4.len(), 0); + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1); + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap()[0].txid(), funding_output.txid); let (funding_locked, _) = create_chan_between_nodes_with_value_confirm(&nodes[0], &nodes[1], &tx); let (announcement, as_update, bs_update) = create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_locked); @@ -8758,6 +8741,7 @@ fn test_error_chans_closed() { nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id: chan_2.2, data: "ERR".to_owned() }); check_added_monitors!(nodes[0], 1); check_closed_broadcast!(nodes[0], false); + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0).len(), 1); assert_eq!(nodes[0].node.list_usable_channels().len(), 2); assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_1.2 || nodes[0].node.list_usable_channels()[1].channel_id == chan_1.2); assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_3.2 || nodes[0].node.list_usable_channels()[1].channel_id == chan_3.2); diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 118503c0..8b8cc123 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -16,7 +16,6 @@ use ln::msgs; use ln::channelmanager::{PaymentPreimage, PaymentHash, PaymentSecret}; -use chain::transaction::OutPoint; use chain::keysinterface::SpendableOutputDescriptor; use util::ser::{Writeable, Writer, MaybeReadable, Readable}; @@ -49,16 +48,6 @@ pub enum Event { /// The value passed in to ChannelManager::create_channel user_channel_id: u64, }, - /// Used to indicate that the client may now broadcast the funding transaction it created for a - /// channel. Broadcasting such a transaction prior to this event may lead to our counterparty - /// trivially stealing all funds in the funding transaction! - FundingBroadcastSafe { - /// The output, which was passed to ChannelManager::funding_transaction_generated, which is - /// now safe to broadcast. - funding_txo: OutPoint, - /// The value passed in to ChannelManager::create_channel - user_channel_id: u64, - }, /// Indicates we've received money! Just gotta dig out that payment preimage and feed it to /// ChannelManager::claim_funds to get it.... /// Note that if the preimage is not known or the amount paid is incorrect, you should call @@ -140,19 +129,14 @@ impl Writeable for Event { // We never write out FundingGenerationReady events as, upon disconnection, peers // drop any channels which have not yet exchanged funding_signed. }, - &Event::FundingBroadcastSafe { ref funding_txo, ref user_channel_id } => { - 1u8.write(writer)?; - funding_txo.write(writer)?; - user_channel_id.write(writer)?; - }, &Event::PaymentReceived { ref payment_hash, ref payment_secret, ref amt } => { - 2u8.write(writer)?; + 1u8.write(writer)?; payment_hash.write(writer)?; payment_secret.write(writer)?; amt.write(writer)?; }, &Event::PaymentSent { ref payment_preimage } => { - 3u8.write(writer)?; + 2u8.write(writer)?; payment_preimage.write(writer)?; }, &Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, @@ -161,7 +145,7 @@ impl Writeable for Event { #[cfg(test)] ref error_data, } => { - 4u8.write(writer)?; + 3u8.write(writer)?; payment_hash.write(writer)?; rejected_by_dest.write(writer)?; #[cfg(test)] @@ -170,12 +154,12 @@ impl Writeable for Event { error_data.write(writer)?; }, &Event::PendingHTLCsForwardable { time_forwardable: _ } => { - 5u8.write(writer)?; + 4u8.write(writer)?; // We don't write the time_fordwardable out at all, as we presume when the user // deserializes us at least that much time has elapsed. }, &Event::SpendableOutputs { ref outputs } => { - 6u8.write(writer)?; + 5u8.write(writer)?; (outputs.len() as u64).write(writer)?; for output in outputs.iter() { output.write(writer)?; @@ -189,19 +173,15 @@ impl MaybeReadable for Event { fn read(reader: &mut R) -> Result, msgs::DecodeError> { match Readable::read(reader)? { 0u8 => Ok(None), - 1u8 => Ok(Some(Event::FundingBroadcastSafe { - funding_txo: Readable::read(reader)?, - user_channel_id: Readable::read(reader)?, - })), - 2u8 => Ok(Some(Event::PaymentReceived { + 1u8 => Ok(Some(Event::PaymentReceived { payment_hash: Readable::read(reader)?, payment_secret: Readable::read(reader)?, amt: Readable::read(reader)?, })), - 3u8 => Ok(Some(Event::PaymentSent { + 2u8 => Ok(Some(Event::PaymentSent { payment_preimage: Readable::read(reader)?, })), - 4u8 => Ok(Some(Event::PaymentFailed { + 3u8 => Ok(Some(Event::PaymentFailed { payment_hash: Readable::read(reader)?, rejected_by_dest: Readable::read(reader)?, #[cfg(test)] @@ -209,10 +189,10 @@ impl MaybeReadable for Event { #[cfg(test)] error_data: Readable::read(reader)?, })), - 5u8 => Ok(Some(Event::PendingHTLCsForwardable { + 4u8 => Ok(Some(Event::PendingHTLCsForwardable { time_forwardable: Duration::from_secs(0) })), - 6u8 => { + 5u8 => { let outputs_len: u64 = Readable::read(reader)?; let mut outputs = Vec::new(); for _ in 0..outputs_len { -- 2.30.2