From df5053d3969689036582ffa46bf3da61b7238bf0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 7 Feb 2020 20:08:31 -0500 Subject: [PATCH] Use ChannelMonitorUpdates in commitment signing fns in Channel This is a rather big step towards using the new ChannelMonitorUpdate flow, using it in the various commitment signing and commitment update message processing functions in Channel. Becase they all often call each other, they all have to be updated as a group, resulting in the somewhat large diff in this commit. In order to keep the update_ids strictly increasing by one for ease of use on the user end, we have to play some games with the latest_monitor_update_id field, though its generally still pretty readable, and the pattern of "get an update_id at the start, and use the one we got at the start when returning, irrespective of what other calls into the Channel during that time did" is relatively straightforward. --- lightning/src/ln/chanmon_update_fail_tests.rs | 6 +- lightning/src/ln/channel.rs | 214 ++++++++++++------ lightning/src/ln/channelmanager.rs | 52 +++-- lightning/src/ln/channelmonitor.rs | 89 ++++++++ 4 files changed, 278 insertions(+), 83 deletions(-) diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 995855427..1437d91bf 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -201,6 +201,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) { } nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed); + check_added_monitors!(nodes[0], 1); assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1); } @@ -791,6 +792,7 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) { send_event = SendEvent::from_event(nodes[2].node.get_and_clear_pending_msg_events().remove(0)); nodes[1].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &send_event.msgs[0]); nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &send_event.commitment_msg); + check_added_monitors!(nodes[1], 1); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); @@ -1177,6 +1179,7 @@ fn claim_while_disconnected_monitor_update_fail() { let as_updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &as_updates.update_add_htlcs[0]); nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_updates.commitment_signed); + check_added_monitors!(nodes[1], 1); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1); // Note that nodes[1] not updating monitor here is OK - it wont take action on the new HTLC @@ -1374,6 +1377,7 @@ fn first_message_on_recv_ordering() { // the appropriate HTLC acceptance). nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &payment_event.commitment_msg); + check_added_monitors!(nodes[1], 1); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1); @@ -1655,7 +1659,7 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails: assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); if fail_on_generate && !restore_between_fails { nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented funding_signed from allowing funding broadcast".to_string(), 1); - check_added_monitors!(nodes[0], 0); + check_added_monitors!(nodes[0], 1); } else { nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1); check_added_monitors!(nodes[0], 1); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index dece3649e..5b222cb10 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1144,7 +1144,7 @@ impl Channel { /// Per HTLC, only one get_update_fail_htlc or get_update_fulfill_htlc call may be made. /// In such cases we debug_assert!(false) and return an IgnoreError. Thus, will always return /// Ok(_) if debug assertions are turned on and preconditions are met. - fn get_update_fulfill_htlc(&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage) -> Result<(Option, Option>), ChannelError> { + fn get_update_fulfill_htlc(&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage) -> Result<(Option, Option), ChannelError> { // Either ChannelFunded got set (which means it won't be unset) or there is no way any // caller thought we could have something claimed (cause we wouldn't have accepted in an // incoming HTLC anyway). If we got to ShutdownComplete, callers aren't allowed to call us, @@ -1190,7 +1190,14 @@ impl Channel { // // We have to put the payment_preimage in the channel_monitor right away here to ensure we // can claim it even if the channel hits the chain before we see their next commitment. - self.channel_monitor.provide_payment_preimage(&payment_hash_calc, &payment_preimage_arg); + self.latest_monitor_update_id += 1; + let monitor_update = ChannelMonitorUpdate { + update_id: self.latest_monitor_update_id, + updates: vec![ChannelMonitorUpdateStep::PaymentPreimage { + payment_preimage: payment_preimage_arg.clone(), + }], + }; + self.channel_monitor.update_monitor_ooo(monitor_update.clone()).unwrap(); if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 { for pending_update in self.holding_cell_htlc_updates.iter() { @@ -1205,7 +1212,7 @@ impl Channel { log_warn!(self, "Have preimage and want to fulfill HTLC with pending failure against channel {}", log_bytes!(self.channel_id())); // TODO: We may actually be able to switch to a fulfill here, though its // rare enough it may not be worth the complexity burden. - return Ok((None, Some(self.channel_monitor.clone()))); + return Ok((None, Some(monitor_update))); } }, _ => {} @@ -1215,7 +1222,7 @@ impl Channel { self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::ClaimHTLC { payment_preimage: payment_preimage_arg, htlc_id: htlc_id_arg, }); - return Ok((None, Some(self.channel_monitor.clone()))); + return Ok((None, Some(monitor_update))); } { @@ -1223,7 +1230,7 @@ impl Channel { if let InboundHTLCState::Committed = htlc.state { } else { debug_assert!(false, "Have an inbound HTLC we tried to claim before it was fully committed to"); - return Ok((None, Some(self.channel_monitor.clone()))); + return Ok((None, Some(monitor_update))); } log_trace!(self, "Upgrading HTLC {} to LocalRemoved with a Fulfill!", log_bytes!(htlc.payment_hash.0)); htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(payment_preimage_arg.clone())); @@ -1233,16 +1240,24 @@ impl Channel { channel_id: self.channel_id(), htlc_id: htlc_id_arg, payment_preimage: payment_preimage_arg, - }), Some(self.channel_monitor.clone()))) + }), Some(monitor_update))) } - pub fn get_update_fulfill_htlc_and_commit(&mut self, htlc_id: u64, payment_preimage: PaymentPreimage) -> Result<(Option<(msgs::UpdateFulfillHTLC, msgs::CommitmentSigned)>, Option>), ChannelError> { + pub fn get_update_fulfill_htlc_and_commit(&mut self, htlc_id: u64, payment_preimage: PaymentPreimage) -> Result<(Option<(msgs::UpdateFulfillHTLC, msgs::CommitmentSigned)>, Option), ChannelError> { match self.get_update_fulfill_htlc(htlc_id, payment_preimage)? { - (Some(update_fulfill_htlc), _) => { + (Some(update_fulfill_htlc), Some(mut monitor_update)) => { + let (commitment, mut additional_update) = self.send_commitment_no_status_check()?; + // send_commitment_no_status_check may bump latest_monitor_id but we want them to be + // strictly increasing by one, so decrement it here. + self.latest_monitor_update_id = monitor_update.update_id; + monitor_update.updates.append(&mut additional_update.updates); + Ok((Some((update_fulfill_htlc, commitment)), Some(monitor_update))) + }, + (Some(update_fulfill_htlc), None) => { let (commitment, monitor_update) = self.send_commitment_no_status_check()?; Ok((Some((update_fulfill_htlc, commitment)), Some(monitor_update))) }, - (None, Some(channel_monitor)) => Ok((None, Some(channel_monitor))), + (None, Some(monitor_update)) => Ok((None, Some(monitor_update))), (None, None) => Ok((None, None)) } } @@ -1501,12 +1516,12 @@ 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) -> Result> { + pub fn funding_signed(&mut self, msg: &msgs::FundingSigned) -> Result, ChannelError)> { if !self.channel_outbound { - return Err(ChannelError::Close("Received funding_signed for an inbound channel?")); + return Err((None, ChannelError::Close("Received funding_signed for an inbound channel?"))); } if self.channel_state & !(ChannelState::MonitorUpdateFailed as u32) != ChannelState::FundingCreated as u32 { - return Err(ChannelError::Close("Received funding_signed in strange state!")); + return Err((None, ChannelError::Close("Received funding_signed in strange state!"))); } if self.commitment_secrets.get_min_seen_secret() != (1 << 48) || self.cur_remote_commitment_transaction_number != INITIAL_COMMITMENT_NUMBER - 1 || @@ -1516,14 +1531,16 @@ impl Channel { let funding_script = self.get_funding_redeemscript(); - let local_keys = self.build_local_transaction_keys(self.cur_local_commitment_transaction_number)?; + let local_keys = self.build_local_transaction_keys(self.cur_local_commitment_transaction_number).map_err(|e| (None, e))?; let local_initial_commitment_tx = self.build_commitment_transaction(self.cur_local_commitment_transaction_number, &local_keys, true, false, self.feerate_per_kw).0; let local_sighash = hash_to_message!(&bip143::SighashComponents::new(&local_initial_commitment_tx).sighash_all(&local_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]); let their_funding_pubkey = &self.their_pubkeys.as_ref().unwrap().funding_pubkey; // They sign the "local" commitment transaction, allowing us to broadcast the tx if we wish. - secp_check!(self.secp_ctx.verify(&local_sighash, &msg.signature, their_funding_pubkey), "Invalid funding_signed signature from peer"); + if let Err(_) = self.secp_ctx.verify(&local_sighash, &msg.signature, their_funding_pubkey) { + return Err((None, ChannelError::Close("Invalid funding_signed signature from peer"))); + } self.latest_monitor_update_id += 1; let monitor_update = ChannelMonitorUpdate { @@ -1533,14 +1550,15 @@ impl Channel { local_keys, feerate_per_kw: self.feerate_per_kw, htlc_outputs: Vec::new(), }] }; - self.channel_monitor.update_monitor(monitor_update.clone()).unwrap(); + self.channel_monitor.update_monitor_ooo(monitor_update.clone()).unwrap(); self.channel_state = ChannelState::FundingSent as u32 | (self.channel_state & (ChannelState::MonitorUpdateFailed as u32)); self.cur_local_commitment_transaction_number -= 1; if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 { Ok(monitor_update) } else { - Err(ChannelError::Ignore("Previous monitor update failure prevented funding_signed from allowing funding broadcast")) + Err((Some(monitor_update), + ChannelError::Ignore("Previous monitor update failure prevented funding_signed from allowing funding broadcast"))) } } @@ -1750,20 +1768,20 @@ impl Channel { Ok(()) } - pub fn commitment_signed(&mut self, msg: &msgs::CommitmentSigned, fee_estimator: &FeeEstimator) -> Result<(msgs::RevokeAndACK, Option, Option, ChannelMonitor), ChannelError> { + pub fn commitment_signed(&mut self, msg: &msgs::CommitmentSigned, fee_estimator: &FeeEstimator) -> Result<(msgs::RevokeAndACK, Option, Option, ChannelMonitorUpdate), (Option, ChannelError)> { if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) { - return Err(ChannelError::Close("Got commitment signed message when channel was not in an operational state")); + return Err((None, ChannelError::Close("Got commitment signed message when channel was not in an operational state"))); } if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 { - return Err(ChannelError::Close("Peer sent commitment_signed when we needed a channel_reestablish")); + return Err((None, ChannelError::Close("Peer sent commitment_signed when we needed a channel_reestablish"))); } if self.channel_state & BOTH_SIDES_SHUTDOWN_MASK == BOTH_SIDES_SHUTDOWN_MASK && self.last_sent_closing_fee.is_some() { - return Err(ChannelError::Close("Peer sent commitment_signed after we'd started exchanging closing_signeds")); + return Err((None, ChannelError::Close("Peer sent commitment_signed after we'd started exchanging closing_signeds"))); } let funding_script = self.get_funding_redeemscript(); - let local_keys = self.build_local_transaction_keys(self.cur_local_commitment_transaction_number)?; + let local_keys = self.build_local_transaction_keys(self.cur_local_commitment_transaction_number).map_err(|e| (None, e))?; let mut update_fee = false; let feerate_per_kw = if !self.channel_outbound && self.pending_update_fee.is_some() { @@ -1781,7 +1799,9 @@ impl Channel { let local_commitment_txid = local_commitment_tx.0.txid(); let local_sighash = hash_to_message!(&bip143::SighashComponents::new(&local_commitment_tx.0).sighash_all(&local_commitment_tx.0.input[0], &funding_script, self.channel_value_satoshis)[..]); log_trace!(self, "Checking commitment tx signature {} by key {} against tx {} with redeemscript {}", log_bytes!(msg.signature.serialize_compact()[..]), log_bytes!(self.their_funding_pubkey().serialize()), encode::serialize_hex(&local_commitment_tx.0), encode::serialize_hex(&funding_script)); - secp_check!(self.secp_ctx.verify(&local_sighash, &msg.signature, &self.their_funding_pubkey()), "Invalid commitment tx signature from peer"); + if let Err(_) = self.secp_ctx.verify(&local_sighash, &msg.signature, &self.their_funding_pubkey()) { + return Err((None, ChannelError::Close("Invalid commitment tx signature from peer"))); + } //If channel fee was updated by funder confirm funder can afford the new fee rate when applied to the current local commitment transaction if update_fee { @@ -1789,12 +1809,12 @@ impl Channel { let total_fee: u64 = feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000; if self.channel_value_satoshis - self.value_to_self_msat / 1000 < total_fee + self.their_channel_reserve_satoshis { - return Err(ChannelError::Close("Funding remote cannot afford proposed new fee")); + return Err((None, ChannelError::Close("Funding remote cannot afford proposed new fee"))); } } if msg.htlc_signatures.len() != local_commitment_tx.1 { - return Err(ChannelError::Close("Got wrong number of HTLC signatures from remote")); + return Err((None, ChannelError::Close("Got wrong number of HTLC signatures from remote"))); } let mut htlcs_and_sigs = Vec::with_capacity(local_commitment_tx.2.len()); @@ -1804,7 +1824,9 @@ impl Channel { let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &local_keys); log_trace!(self, "Checking HTLC tx signature {} by key {} against tx {} with redeemscript {}", log_bytes!(msg.htlc_signatures[idx].serialize_compact()[..]), log_bytes!(local_keys.b_htlc_key.serialize()), encode::serialize_hex(&htlc_tx), encode::serialize_hex(&htlc_redeemscript)); let htlc_sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]); - secp_check!(self.secp_ctx.verify(&htlc_sighash, &msg.htlc_signatures[idx], &local_keys.b_htlc_key), "Invalid HTLC tx signature from peer"); + if let Err(_) = self.secp_ctx.verify(&htlc_sighash, &msg.htlc_signatures[idx], &local_keys.b_htlc_key) { + return Err((None, ChannelError::Close("Invalid HTLC tx signature from peer"))); + } htlcs_and_sigs.push((htlc, Some(msg.htlc_signatures[idx]), source)); } else { htlcs_and_sigs.push((htlc, None, source)); @@ -1831,9 +1853,15 @@ impl Channel { let their_funding_pubkey = self.their_pubkeys.as_ref().unwrap().funding_pubkey; - self.channel_monitor.provide_latest_local_commitment_tx_info( - LocalCommitmentTransaction::new_missing_local_sig(local_commitment_tx.0, &msg.signature, &PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()), &their_funding_pubkey), - local_keys, self.feerate_per_kw, htlcs_and_sigs).unwrap(); + self.latest_monitor_update_id += 1; + let mut monitor_update = ChannelMonitorUpdate { + update_id: self.latest_monitor_update_id, + updates: vec![ChannelMonitorUpdateStep::LatestLocalCommitmentTXInfo { + commitment_tx: LocalCommitmentTransaction::new_missing_local_sig(local_commitment_tx.0, &msg.signature, &PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()), &their_funding_pubkey), + local_keys, feerate_per_kw: self.feerate_per_kw, htlc_outputs: htlcs_and_sigs + }] + }; + self.channel_monitor.update_monitor_ooo(monitor_update.clone()).unwrap(); for htlc in self.pending_inbound_htlcs.iter_mut() { let new_forward = if let &InboundHTLCState::RemoteAnnounced(ref forward_info) = &htlc.state { @@ -1866,26 +1894,31 @@ impl Channel { // If we were going to send a commitment_signed after the RAA, go ahead and do all // the corresponding HTLC status updates so that get_last_commitment_update // includes the right HTLCs. - // Note that this generates a monitor update that we ignore! This is OK since we - // won't actually send the commitment_signed that generated the update to the other - // side until the latest monitor has been pulled from us and stored. self.monitor_pending_commitment_signed = true; - self.send_commitment_no_status_check()?; + let (_, mut additional_update) = self.send_commitment_no_status_check().map_err(|e| (None, e))?; + // send_commitment_no_status_check may bump latest_monitor_id but we want them to be + // strictly increasing by one, so decrement it here. + self.latest_monitor_update_id = monitor_update.update_id; + monitor_update.updates.append(&mut additional_update.updates); } // TODO: Call maybe_propose_first_closing_signed on restoration (or call it here and // re-send the message on restoration) - return Err(ChannelError::Ignore("Previous monitor update failure prevented generation of RAA")); + return Err((Some(monitor_update), ChannelError::Ignore("Previous monitor update failure prevented generation of RAA"))); } - let (our_commitment_signed, monitor_update, closing_signed) = if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 { + let (our_commitment_signed, closing_signed) = if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 { // If we're AwaitingRemoteRevoke we can't send a new commitment here, but that's ok - // we'll send one right away when we get the revoke_and_ack when we // free_holding_cell_htlcs(). - let (msg, monitor) = self.send_commitment_no_status_check()?; - (Some(msg), monitor, None) + let (msg, mut additional_update) = self.send_commitment_no_status_check().map_err(|e| (None, e))?; + // send_commitment_no_status_check may bump latest_monitor_id but we want them to be + // strictly increasing by one, so decrement it here. + self.latest_monitor_update_id = monitor_update.update_id; + monitor_update.updates.append(&mut additional_update.updates); + (Some(msg), None) } else if !need_our_commitment { - (None, self.channel_monitor.clone(), self.maybe_propose_first_closing_signed(fee_estimator)) - } else { (None, self.channel_monitor.clone(), None) }; + (None, self.maybe_propose_first_closing_signed(fee_estimator)) + } else { (None, None) }; Ok((msgs::RevokeAndACK { channel_id: self.channel_id, @@ -1896,11 +1929,16 @@ impl Channel { /// Used to fulfill holding_cell_htlcs when we get a remote ack (or implicitly get it by them /// fulfilling or failing the last pending HTLC) - fn free_holding_cell_htlcs(&mut self) -> Result)>, ChannelError> { + fn free_holding_cell_htlcs(&mut self) -> Result, ChannelError> { assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, 0); if self.holding_cell_htlc_updates.len() != 0 || self.holding_cell_update_fee.is_some() { log_trace!(self, "Freeing holding cell with {} HTLC updates{}", self.holding_cell_htlc_updates.len(), if self.holding_cell_update_fee.is_some() { " and a fee update" } else { "" }); + let mut monitor_update = ChannelMonitorUpdate { + update_id: self.latest_monitor_update_id + 1, // We don't increment this yet! + updates: Vec::new(), + }; + let mut htlc_updates = Vec::new(); mem::swap(&mut htlc_updates, &mut self.holding_cell_htlc_updates); let mut update_add_htlcs = Vec::with_capacity(htlc_updates.len()); @@ -1935,7 +1973,12 @@ impl Channel { }, &HTLCUpdateAwaitingACK::ClaimHTLC { ref payment_preimage, htlc_id, .. } => { match self.get_update_fulfill_htlc(htlc_id, *payment_preimage) { - Ok(update_fulfill_msg_option) => update_fulfill_htlcs.push(update_fulfill_msg_option.0.unwrap()), + Ok((update_fulfill_msg_option, additional_monitor_update_opt)) => { + update_fulfill_htlcs.push(update_fulfill_msg_option.unwrap()); + if let Some(mut additional_monitor_update) = additional_monitor_update_opt { + monitor_update.updates.append(&mut additional_monitor_update.updates); + } + }, Err(e) => { if let ChannelError::Ignore(_) = e {} else { @@ -1985,7 +2028,13 @@ impl Channel { } else { None }; - let (commitment_signed, monitor_update) = self.send_commitment_no_status_check()?; + + let (commitment_signed, mut additional_update) = self.send_commitment_no_status_check()?; + // send_commitment_no_status_check and get_update_fulfill_htlc may bump latest_monitor_id + // but we want them to be strictly increasing by one, so reset it here. + self.latest_monitor_update_id = monitor_update.update_id; + monitor_update.updates.append(&mut additional_update.updates); + Ok(Some((msgs::CommitmentUpdate { update_add_htlcs, update_fulfill_htlcs, @@ -2007,7 +2056,7 @@ impl Channel { /// waiting on this revoke_and_ack. The generation of this new commitment_signed may also fail, /// generating an appropriate error *after* the channel state has been updated based on the /// revoke_and_ack message. - pub fn revoke_and_ack(&mut self, msg: &msgs::RevokeAndACK, fee_estimator: &FeeEstimator) -> Result<(Option, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, Option, ChannelMonitor), ChannelError> { + pub fn revoke_and_ack(&mut self, msg: &msgs::RevokeAndACK, fee_estimator: &FeeEstimator) -> Result<(Option, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, Option, ChannelMonitorUpdate), ChannelError> { if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) { return Err(ChannelError::Close("Got revoke/ACK message when channel was not in an operational state")); } @@ -2023,10 +2072,6 @@ impl Channel { return Err(ChannelError::Close("Got a revoke commitment secret which didn't correspond to their current pubkey")); } } - self.commitment_secrets.provide_secret(self.cur_remote_commitment_transaction_number + 1, msg.per_commitment_secret) - .map_err(|_| ChannelError::Close("Previous secrets did not match new one"))?; - self.channel_monitor.provide_secret(self.cur_remote_commitment_transaction_number + 1, msg.per_commitment_secret) - .unwrap(); if self.channel_state & ChannelState::AwaitingRemoteRevoke as u32 == 0 { // Our counterparty seems to have burned their coins to us (by revoking a state when we @@ -2039,6 +2084,18 @@ impl Channel { return Err(ChannelError::Close("Received an unexpected revoke_and_ack")); } + self.commitment_secrets.provide_secret(self.cur_remote_commitment_transaction_number + 1, msg.per_commitment_secret) + .map_err(|_| ChannelError::Close("Previous secrets did not match new one"))?; + self.latest_monitor_update_id += 1; + let mut monitor_update = ChannelMonitorUpdate { + update_id: self.latest_monitor_update_id, + updates: vec![ChannelMonitorUpdateStep::CommitmentSecret { + idx: self.cur_remote_commitment_transaction_number + 1, + secret: msg.per_commitment_secret, + }], + }; + self.channel_monitor.update_monitor_ooo(monitor_update.clone()).unwrap(); + // Update state now that we've passed all the can-fail calls... // (note that we may still fail to generate the new commitment_signed message, but that's // OK, we step the channel here and *then* if the new generation fails we can fail the @@ -2164,28 +2221,44 @@ impl Channel { // When the monitor updating is restored we'll call get_last_commitment_update(), // which does not update state, but we're definitely now awaiting a remote revoke // before we can step forward any more, so set it here. - self.send_commitment_no_status_check()?; + let (_, mut additional_update) = self.send_commitment_no_status_check()?; + // send_commitment_no_status_check may bump latest_monitor_id but we want them to be + // strictly increasing by one, so decrement it here. + self.latest_monitor_update_id = monitor_update.update_id; + monitor_update.updates.append(&mut additional_update.updates); } self.monitor_pending_forwards.append(&mut to_forward_infos); self.monitor_pending_failures.append(&mut revoked_htlcs); - return Ok((None, Vec::new(), Vec::new(), None, self.channel_monitor.clone())); + return Ok((None, Vec::new(), Vec::new(), None, monitor_update)) } match self.free_holding_cell_htlcs()? { - Some(mut commitment_update) => { - commitment_update.0.update_fail_htlcs.reserve(update_fail_htlcs.len()); + Some((mut commitment_update, mut additional_update)) => { + commitment_update.update_fail_htlcs.reserve(update_fail_htlcs.len()); for fail_msg in update_fail_htlcs.drain(..) { - commitment_update.0.update_fail_htlcs.push(fail_msg); + commitment_update.update_fail_htlcs.push(fail_msg); } - commitment_update.0.update_fail_malformed_htlcs.reserve(update_fail_malformed_htlcs.len()); + commitment_update.update_fail_malformed_htlcs.reserve(update_fail_malformed_htlcs.len()); for fail_msg in update_fail_malformed_htlcs.drain(..) { - commitment_update.0.update_fail_malformed_htlcs.push(fail_msg); + commitment_update.update_fail_malformed_htlcs.push(fail_msg); } - Ok((Some(commitment_update.0), to_forward_infos, revoked_htlcs, None, commitment_update.1)) + + // free_holding_cell_htlcs may bump latest_monitor_id multiple times but we want them to be + // strictly increasing by one, so decrement it here. + self.latest_monitor_update_id = monitor_update.update_id; + monitor_update.updates.append(&mut additional_update.updates); + + Ok((Some(commitment_update), to_forward_infos, revoked_htlcs, None, monitor_update)) }, None => { if require_commitment { - let (commitment_signed, monitor_update) = self.send_commitment_no_status_check()?; + let (commitment_signed, mut additional_update) = self.send_commitment_no_status_check()?; + + // send_commitment_no_status_check may bump latest_monitor_id but we want them to be + // strictly increasing by one, so decrement it here. + self.latest_monitor_update_id = monitor_update.update_id; + monitor_update.updates.append(&mut additional_update.updates); + Ok((Some(msgs::CommitmentUpdate { update_add_htlcs: Vec::new(), update_fulfill_htlcs: Vec::new(), @@ -2195,7 +2268,7 @@ impl Channel { commitment_signed }), to_forward_infos, revoked_htlcs, None, monitor_update)) } else { - Ok((None, to_forward_infos, revoked_htlcs, self.maybe_propose_first_closing_signed(fee_estimator), self.channel_monitor.clone())) + Ok((None, to_forward_infos, revoked_htlcs, self.maybe_propose_first_closing_signed(fee_estimator), monitor_update)) } } } @@ -2230,7 +2303,7 @@ impl Channel { }) } - pub fn send_update_fee_and_commit(&mut self, feerate_per_kw: u64) -> Result)>, ChannelError> { + pub fn send_update_fee_and_commit(&mut self, feerate_per_kw: u64) -> Result, ChannelError> { match self.send_update_fee(feerate_per_kw) { Some(update_fee) => { let (commitment_signed, monitor_update) = self.send_commitment_no_status_check()?; @@ -2463,7 +2536,7 @@ impl Channel { /// May panic if some calls other than message-handling calls (which will all Err immediately) /// have been called between remove_uncommitted_htlcs_and_mark_paused and this call. - pub fn channel_reestablish(&mut self, msg: &msgs::ChannelReestablish) -> Result<(Option, Option, Option, Option>, RAACommitmentOrder, Option), ChannelError> { + pub fn channel_reestablish(&mut self, msg: &msgs::ChannelReestablish) -> Result<(Option, Option, Option, Option, RAACommitmentOrder, Option), ChannelError> { if self.channel_state & (ChannelState::PeerDisconnected as u32) == 0 { // While BOLT 2 doesn't indicate explicitly we should error this channel here, it // almost certainly indicates we are going to end up out-of-sync in some way, so we @@ -2568,7 +2641,7 @@ impl Channel { match self.free_holding_cell_htlcs() { Err(ChannelError::Close(msg)) => return Err(ChannelError::Close(msg)), Err(ChannelError::Ignore(_)) | Err(ChannelError::CloseDelayBroadcast { .. }) => panic!("Got non-channel-failing result from free_holding_cell_htlcs"), - Ok(Some((commitment_update, channel_monitor))) => return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(channel_monitor), self.resend_order.clone(), shutdown_msg)), + Ok(Some((commitment_update, monitor_update))) => return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(monitor_update), self.resend_order.clone(), shutdown_msg)), Ok(None) => return Ok((resend_funding_locked, required_revoke, None, None, self.resend_order.clone(), shutdown_msg)), } } else { @@ -3428,7 +3501,7 @@ impl Channel { /// Always returns a ChannelError::Close if an immediately-preceding (read: the /// last call to this Channel) send_htlc returned Ok(Some(_)) and there is an Err. /// May panic if called except immediately after a successful, Ok(Some(_))-returning send_htlc. - pub fn send_commitment(&mut self) -> Result<(msgs::CommitmentSigned, ChannelMonitor), ChannelError> { + pub fn send_commitment(&mut self) -> Result<(msgs::CommitmentSigned, ChannelMonitorUpdate), ChannelError> { if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) { panic!("Cannot create commitment tx until channel is fully established"); } @@ -3460,7 +3533,7 @@ impl Channel { self.send_commitment_no_status_check() } /// Only fails in case of bad keys - fn send_commitment_no_status_check(&mut self) -> Result<(msgs::CommitmentSigned, ChannelMonitor), ChannelError> { + fn send_commitment_no_status_check(&mut self) -> Result<(msgs::CommitmentSigned, ChannelMonitorUpdate), ChannelError> { // We can upgrade the status of some HTLCs that are waiting on a commitment, even if we // fail to generate this, we still are at least at a position where upgrading their status // is acceptable. @@ -3484,15 +3557,26 @@ impl Channel { let (res, remote_commitment_tx, htlcs) = match self.send_commitment_no_state_update() { Ok((res, (remote_commitment_tx, mut htlcs))) => { // Update state now that we've passed all the can-fail calls... - let htlcs_no_ref = htlcs.drain(..).map(|(htlc, htlc_source)| (htlc, htlc_source.map(|source_ref| Box::new(source_ref.clone())))).collect(); + let htlcs_no_ref: Vec<(HTLCOutputInCommitment, Option>)> = + htlcs.drain(..).map(|(htlc, htlc_source)| (htlc, htlc_source.map(|source_ref| Box::new(source_ref.clone())))).collect(); (res, remote_commitment_tx, htlcs_no_ref) }, Err(e) => return Err(e), }; - self.channel_monitor.provide_latest_remote_commitment_tx_info(&remote_commitment_tx, htlcs, self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap()); + self.latest_monitor_update_id += 1; + let monitor_update = ChannelMonitorUpdate { + update_id: self.latest_monitor_update_id, + updates: vec![ChannelMonitorUpdateStep::LatestRemoteCommitmentTXInfo { + unsigned_commitment_tx: remote_commitment_tx.clone(), + htlc_outputs: htlcs.clone(), + commitment_number: self.cur_remote_commitment_transaction_number, + their_revocation_point: self.their_cur_commitment_point.unwrap() + }] + }; + self.channel_monitor.update_monitor_ooo(monitor_update.clone()).unwrap(); self.channel_state |= ChannelState::AwaitingRemoteRevoke as u32; - Ok((res, self.channel_monitor.clone())) + Ok((res, monitor_update)) } /// Only fails in case of bad keys. Used for channel_reestablish commitment_signed generation @@ -3545,7 +3629,7 @@ impl Channel { /// to send to the remote peer in one go. /// Shorthand for calling send_htlc() followed by send_commitment(), see docs on those for /// more info. - pub fn send_htlc_and_commit(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result)>, ChannelError> { + pub fn send_htlc_and_commit(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result, ChannelError> { match self.send_htlc(amount_msat, payment_hash, cltv_expiry, source, onion_routing_packet)? { Some(update_add_htlc) => { let (commitment_signed, monitor_update) = self.send_commitment_no_status_check()?; diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 311cf2e49..b489305c9 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1203,8 +1203,8 @@ impl ChannelManager { - if let Err(e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) { + Some((update_add, commitment_signed, monitor_update)) => { + if let Err(e) = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update) { maybe_break_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::CommitmentFirst, false, true); // Note that MonitorUpdateFailed here indicates (per function docs) // that we will resent the commitment update once we unfree monitor @@ -1438,7 +1438,7 @@ impl ChannelManager res, Err(e) => { // We surely failed send_commitment due to bad keys, in that case @@ -1464,7 +1464,7 @@ impl ChannelManager ChannelManager { - if let Some(chan_monitor) = monitor_option { - if let Err(e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) { + if let Some(monitor_update) = monitor_option { + if let Err(e) = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update) { if was_frozen_for_monitor { assert!(msgs.is_none()); } else { @@ -2105,7 +2105,16 @@ impl ChannelManager try_chan_entry!(self, Err(e), channel_state, chan), + Err((Some(monitor_update), e)) => { + assert!(chan.get().is_awaiting_monitor_update()); + let _ = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update); + try_chan_entry!(self, Err(e), channel_state, chan); + unreachable!(); + }, + Ok(update) => update, + }; if let Err(e) = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update) { return_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::RevokeAndACKFirst, false, false); } @@ -2363,9 +2372,18 @@ impl ChannelManager try_chan_entry!(self, Err(e), channel_state, chan), + Err((Some(update), e)) => { + assert!(chan.get().is_awaiting_monitor_update()); + let _ = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), update); + try_chan_entry!(self, Err(e), channel_state, chan); + unreachable!(); + }, + Ok(res) => res + }; + if let Err(e) = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update) { return_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::RevokeAndACKFirst, true, commitment_signed.is_some()); //TODO: Rebroadcast closing_signed if present on monitor update restoration } @@ -2440,9 +2458,9 @@ impl ChannelManager ChannelManager ChannelManager, Option)>, }, + LatestRemoteCommitmentTXInfo { + unsigned_commitment_tx: Transaction, // TODO: We should actually only need the txid here + htlc_outputs: Vec<(HTLCOutputInCommitment, Option>)>, + commitment_number: u64, + their_revocation_point: PublicKey, + }, + PaymentPreimage { + payment_preimage: PaymentPreimage, + }, + CommitmentSecret { + idx: u64, + secret: [u8; 32], + }, } impl Writeable for ChannelMonitorUpdateStep { @@ -677,6 +690,32 @@ impl Writeable for ChannelMonitorUpdateStep { source.write(w)?; } } + &ChannelMonitorUpdateStep::LatestRemoteCommitmentTXInfo { ref unsigned_commitment_tx, ref htlc_outputs, ref commitment_number, ref their_revocation_point } => { + 1u8.write(w)?; + unsigned_commitment_tx.write(w)?; + commitment_number.write(w)?; + their_revocation_point.write(w)?; + (htlc_outputs.len() as u64).write(w)?; + for &(ref output, ref source) in htlc_outputs.iter() { + output.write(w)?; + match source { + &None => 0u8.write(w)?, + &Some(ref s) => { + 1u8.write(w)?; + s.write(w)?; + }, + } + } + }, + &ChannelMonitorUpdateStep::PaymentPreimage { ref payment_preimage } => { + 2u8.write(w)?; + payment_preimage.write(w)?; + }, + &ChannelMonitorUpdateStep::CommitmentSecret { ref idx, ref secret } => { + 3u8.write(w)?; + idx.write(w)?; + secret.write(w)?; + }, } Ok(()) } @@ -699,6 +738,32 @@ impl Readable for ChannelMonitorUpdateStep { }, }) }, + 1u8 => { + Ok(ChannelMonitorUpdateStep::LatestRemoteCommitmentTXInfo { + unsigned_commitment_tx: Readable::read(r)?, + commitment_number: Readable::read(r)?, + their_revocation_point: Readable::read(r)?, + htlc_outputs: { + let len: u64 = Readable::read(r)?; + let mut res = Vec::new(); + for _ in 0..len { + res.push((Readable::read(r)?, as Readable>::read(r)?.map(|o| Box::new(o)))); + } + res + }, + }) + }, + 2u8 => { + Ok(ChannelMonitorUpdateStep::PaymentPreimage { + payment_preimage: Readable::read(r)?, + }) + }, + 3u8 => { + Ok(ChannelMonitorUpdateStep::CommitmentSecret { + idx: Readable::read(r)?, + secret: Readable::read(r)?, + }) + }, _ => Err(DecodeError::InvalidValue), } } @@ -1372,6 +1437,24 @@ impl ChannelMonitor { self.payment_preimages.insert(payment_hash.clone(), payment_preimage.clone()); } + /// Used in Channel to cheat wrt the update_ids since it plays games, will be removed soon! + 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::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 } => + self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()), &payment_preimage), + ChannelMonitorUpdateStep::CommitmentSecret { idx, secret } => + self.provide_secret(idx, secret)? + } + } + self.latest_update_id = updates.update_id; + Ok(()) + } + /// Updates a ChannelMonitor on the basis of some new information provided by the Channel /// itself. /// @@ -1384,6 +1467,12 @@ impl ChannelMonitor { 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::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 } => + self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()), &payment_preimage), + ChannelMonitorUpdateStep::CommitmentSecret { idx, secret } => + self.provide_secret(idx, secret)? } } self.latest_update_id = updates.update_id; -- 2.39.5