From eeefdaf7a2d150ff89ec0ce11da1bbe7b98816bb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 5 Sep 2018 14:08:13 -0400 Subject: [PATCH 1/4] Always return an Error Message in invalid sig/key errors in Channel --- src/ln/channel.rs | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/ln/channel.rs b/src/ln/channel.rs index 14ab80c90..f6e97f12f 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -338,18 +338,17 @@ const B_OUTPUT_PLUS_SPENDING_INPUT_WEIGHT: u64 = 104; // prevout: 40, nSequence: pub const MAX_FUNDING_SATOSHIS: u64 = (1 << 24); macro_rules! secp_call { - ( $res: expr, $err: expr ) => { + ( $res: expr, $err: expr, $chan_id: expr ) => { match $res { Ok(key) => key, - //TODO: make the error a parameter - Err(_) => return Err(HandleError{err: $err, action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })}) + Err(_) => return Err(HandleError {err: $err, action: Some(msgs::ErrorAction::SendErrorMessage {msg: msgs::ErrorMessage {channel_id: $chan_id, data: $err.to_string()}})}) } }; } macro_rules! secp_derived_key { - ( $res: expr ) => { - secp_call!($res, "Derived invalid key, peer is maliciously selecting parameters") + ( $res: expr, $chan_id: expr ) => { + secp_call!($res, "Derived invalid key, peer is maliciously selecting parameters", $chan_id) } } impl Channel { @@ -883,7 +882,7 @@ impl Channel { let delayed_payment_base = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.delayed_payment_base_key); let htlc_basepoint = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.htlc_base_key); - Ok(secp_derived_key!(TxCreationKeys::new(&self.secp_ctx, &per_commitment_point, &delayed_payment_base, &htlc_basepoint, &self.their_revocation_basepoint.unwrap(), &self.their_payment_basepoint.unwrap(), &self.their_htlc_basepoint.unwrap()))) + Ok(secp_derived_key!(TxCreationKeys::new(&self.secp_ctx, &per_commitment_point, &delayed_payment_base, &htlc_basepoint, &self.their_revocation_basepoint.unwrap(), &self.their_payment_basepoint.unwrap(), &self.their_htlc_basepoint.unwrap()), self.channel_id())) } #[inline] @@ -896,7 +895,7 @@ impl Channel { let revocation_basepoint = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.revocation_base_key); let htlc_basepoint = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.htlc_base_key); - Ok(secp_derived_key!(TxCreationKeys::new(&self.secp_ctx, &self.their_cur_commitment_point.unwrap(), &self.their_delayed_payment_basepoint.unwrap(), &self.their_htlc_basepoint.unwrap(), &revocation_basepoint, &payment_basepoint, &htlc_basepoint))) + Ok(secp_derived_key!(TxCreationKeys::new(&self.secp_ctx, &self.their_cur_commitment_point.unwrap(), &self.their_delayed_payment_basepoint.unwrap(), &self.their_htlc_basepoint.unwrap(), &revocation_basepoint, &payment_basepoint, &htlc_basepoint), self.channel_id())) } /// Gets the redeemscript for the funding transaction output (ie the funding transaction output @@ -960,7 +959,7 @@ impl Channel { let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &keys); - let our_htlc_key = secp_derived_key!(chan_utils::derive_private_key(&self.secp_ctx, &keys.per_commitment_point, &self.local_keys.htlc_base_key)); + let our_htlc_key = secp_derived_key!(chan_utils::derive_private_key(&self.secp_ctx, &keys.per_commitment_point, &self.local_keys.htlc_base_key), self.channel_id()); let sighash = Message::from_slice(&bip143::SighashComponents::new(&tx).sighash_all(&tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]).unwrap(); let is_local_tx = PublicKey::from_secret_key(&self.secp_ctx, &our_htlc_key) == keys.a_htlc_key; Ok((htlc_redeemscript, self.secp_ctx.sign(&sighash, &our_htlc_key), is_local_tx)) @@ -1268,7 +1267,7 @@ impl Channel { let local_sighash = Message::from_slice(&bip143::SighashComponents::new(&local_initial_commitment_tx).sighash_all(&local_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]).unwrap(); // They sign the "local" commitment transaction, allowing us to broadcast the tx if we wish. - secp_call!(self.secp_ctx.verify(&local_sighash, &sig, &self.their_funding_pubkey.unwrap()), "Invalid funding_created signature from peer"); + secp_call!(self.secp_ctx.verify(&local_sighash, &sig, &self.their_funding_pubkey.unwrap()), "Invalid funding_created signature from peer", self.channel_id()); // We sign the "remote" commitment transaction, allowing them to broadcast the tx if they wish. Ok((remote_initial_commitment_tx, self.secp_ctx.sign(&remote_sighash, &self.local_keys.funding_key))) @@ -1331,7 +1330,7 @@ impl Channel { let local_sighash = Message::from_slice(&bip143::SighashComponents::new(&local_initial_commitment_tx).sighash_all(&local_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]).unwrap(); // They sign the "local" commitment transaction, allowing us to broadcast the tx if we wish. - secp_call!(self.secp_ctx.verify(&local_sighash, &msg.signature, &self.their_funding_pubkey.unwrap()), "Invalid funding_signed signature from peer"); + secp_call!(self.secp_ctx.verify(&local_sighash, &msg.signature, &self.their_funding_pubkey.unwrap()), "Invalid funding_signed signature from peer", self.channel_id()); self.sign_commitment_transaction(&mut local_initial_commitment_tx, &msg.signature); self.channel_monitor.provide_latest_local_commitment_tx_info(local_initial_commitment_tx.clone(), local_keys, self.feerate_per_kw, Vec::new()); @@ -1515,7 +1514,7 @@ impl Channel { let mut local_commitment_tx = self.build_commitment_transaction(self.cur_local_commitment_transaction_number, &local_keys, true, false); let local_commitment_txid = local_commitment_tx.0.txid(); let local_sighash = Message::from_slice(&bip143::SighashComponents::new(&local_commitment_tx.0).sighash_all(&local_commitment_tx.0.input[0], &funding_script, self.channel_value_satoshis)[..]).unwrap(); - secp_call!(self.secp_ctx.verify(&local_sighash, &msg.signature, &self.their_funding_pubkey.unwrap()), "Invalid commitment tx signature from peer"); + secp_call!(self.secp_ctx.verify(&local_sighash, &msg.signature, &self.their_funding_pubkey.unwrap()), "Invalid commitment tx signature from peer", self.channel_id()); if msg.htlc_signatures.len() != local_commitment_tx.1.len() { return Err(HandleError{err: "Got wrong number of HTLC signatures from remote", action: None}); @@ -1530,7 +1529,7 @@ impl Channel { let mut htlc_tx = self.build_htlc_transaction(&local_commitment_txid, htlc, true, &local_keys); let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &local_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_call!(self.secp_ctx.verify(&htlc_sighash, &msg.htlc_signatures[idx], &local_keys.b_htlc_key), "Invalid HTLC tx siganture from peer"); + secp_call!(self.secp_ctx.verify(&htlc_sighash, &msg.htlc_signatures[idx], &local_keys.b_htlc_key), "Invalid HTLC tx siganture from peer", self.channel_id()); let htlc_sig = if htlc.offered { let htlc_sig = self.sign_htlc_transaction(&mut htlc_tx, &msg.htlc_signatures[idx], &None, htlc, &local_keys)?; new_local_commitment_txn.push(htlc_tx); @@ -1666,7 +1665,7 @@ impl Channel { return Err(HandleError{err: "Got revoke/ACK message when channel was not in an operational state", action: None}); } if let Some(their_prev_commitment_point) = self.their_prev_commitment_point { - if PublicKey::from_secret_key(&self.secp_ctx, &secp_call!(SecretKey::from_slice(&self.secp_ctx, &msg.per_commitment_secret), "Peer provided an invalid per_commitment_secret")) != their_prev_commitment_point { + if PublicKey::from_secret_key(&self.secp_ctx, &secp_call!(SecretKey::from_slice(&self.secp_ctx, &msg.per_commitment_secret), "Peer provided an invalid per_commitment_secret", self.channel_id())) != their_prev_commitment_point { return Err(HandleError{err: "Got a revoke commitment secret which didn't correspond to their current pubkey", action: None}); } } @@ -1893,7 +1892,7 @@ impl Channel { // limits, so check for that case by re-checking the signature here. closing_tx = self.build_closing_transaction(msg.fee_satoshis, true).0; sighash = Message::from_slice(&bip143::SighashComponents::new(&closing_tx).sighash_all(&closing_tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]).unwrap(); - secp_call!(self.secp_ctx.verify(&sighash, &msg.signature, &self.their_funding_pubkey.unwrap()), "Invalid closing tx signature from peer"); + secp_call!(self.secp_ctx.verify(&sighash, &msg.signature, &self.their_funding_pubkey.unwrap()), "Invalid closing tx signature from peer", self.channel_id()); }, }; @@ -2448,7 +2447,7 @@ impl Channel { let htlc_tx = self.build_htlc_transaction(&remote_commitment_txid, htlc, false, &remote_keys); let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &remote_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(); - let our_htlc_key = secp_derived_key!(chan_utils::derive_private_key(&self.secp_ctx, &remote_keys.per_commitment_point, &self.local_keys.htlc_base_key)); + let our_htlc_key = secp_derived_key!(chan_utils::derive_private_key(&self.secp_ctx, &remote_keys.per_commitment_point, &self.local_keys.htlc_base_key), self.channel_id()); htlc_sigs.push(self.secp_ctx.sign(&htlc_sighash, &our_htlc_key)); } From 8e4c062f1b5aacc1fb98b52537a7a98b6194c6b4 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 5 Sep 2018 14:08:38 -0400 Subject: [PATCH 2/4] Document+check commitment_signed generation success on send_htlc Because we don't have an HTLCState for update_add_htlc-generated-but-not-yet-commitment_signed to simplify the mess of HTLCState match arms, any time a Channel::send_htlc call returns Ok(Some(_)) we MUST call commitment_signed and it MUST return success (or close the channel). We mention this in the docs and panic if its not met in ChannelManager (which lets the fuzz tester check this). --- src/ln/channel.rs | 3 +++ src/ln/channelmanager.rs | 7 ++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/ln/channel.rs b/src/ln/channel.rs index f6e97f12f..e5538a059 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -2334,6 +2334,7 @@ impl Channel { /// This returns an option instead of a pure UpdateAddHTLC as we may be in a state where we are /// waiting on the remote peer to send us a revoke_and_ack during which time we cannot add new /// HTLCs on the wire or we wouldn't be able to determine what they actually ACK'ed. + /// You MUST call send_commitment prior to any other calls on this Channel pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: [u8; 32], cltv_expiry: u32, onion_routing_packet: msgs::OnionPacket) -> Result, HandleError> { if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) { return Err(HandleError{err: "Cannot send HTLC until channel is fully established and we haven't started shutting down", action: None}); @@ -2401,6 +2402,8 @@ impl Channel { } /// Creates a signed commitment transaction to send to the remote peer. + /// Always returns a Channel-failing HandleError::action if an immediately-preceding (read: the + /// last call to this Channel) send_htlc returned Ok(Some(_)) and there is an Err. pub fn send_commitment(&mut self) -> Result<(msgs::CommitmentSigned, ChannelMonitor), HandleError> { if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) { return Err(HandleError{err: "Cannot create commitment tx until channel is fully established", action: None}); diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 508997833..825669402 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -1147,7 +1147,12 @@ impl ChannelManager { if !add_htlc_msgs.is_empty() { let (commitment_msg, monitor) = match forward_chan.send_commitment() { Ok(res) => res, - Err(_e) => { + Err(e) => { + if let &Some(msgs::ErrorAction::DisconnectPeer{msg: Some(ref _err_msg)}) = &e.action { + } else if let &Some(msgs::ErrorAction::SendErrorMessage{msg: ref _err_msg}) = &e.action { + } else { + panic!("Stated return value requirements in send_commitment() were not met"); + } //TODO: Handle...this is bad! continue; }, From d1568ca7092057ffd7b84daed17cef05a50871b2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 5 Sep 2018 14:47:43 -0400 Subject: [PATCH 3/4] Drop HTLCState::LocalRemovedAwaitingCommitment This was redundant and was included because the HTLC still needed to be monitored, but that happens in ChannelMonitor, so there is no need for it in Channel itself. --- src/ln/channel.rs | 40 +++++++++++----------------------------- 1 file changed, 11 insertions(+), 29 deletions(-) diff --git a/src/ln/channel.rs b/src/ln/channel.rs index e5538a059..b2d6a70e1 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -138,18 +138,16 @@ enum HTLCState { AwaitingRemovedRemoteRevoke, /// Removed by us and a new commitment_signed was sent (if we were AwaitingRemoteRevoke when we /// created it we would have put it in the holding cell instead). When they next revoke_and_ack - /// we'll promote to LocalRemovedAwaitingCommitment if we fulfilled, otherwise we'll drop at - /// that point. + /// we'll drop it. /// Note that we have to keep an eye on the HTLC until we've received a broadcastable /// commitment transaction without it as otherwise we'll have to force-close the channel to /// claim it before the timeout (obviously doesn't apply to revoked HTLCs that we can't claim - /// anyway). + /// anyway). That said, ChannelMonitor does this for us (see + /// ChannelMonitor::would_broadcast_at_height) so we actually remove the HTLC from our own + /// local state before then, once we're sure that the next commitment_signed and + /// ChannelMonitor::provide_latest_local_commitment_tx_info will not include this HTLC. /// Implies HTLCOutput::outbound: false LocalRemoved, - /// Removed by us, sent a new commitment_signed and got a revoke_and_ack. Just waiting on an - /// updated local commitment transaction. Implies local_removed_fulfilled. - /// Implies HTLCOutput::outbound: false - LocalRemovedAwaitingCommitment, } struct HTLCOutput { //TODO: Refactor into Outbound/InboundHTLCOutput (will save memory and fewer panics) @@ -706,7 +704,6 @@ impl Channel { HTLCState::AwaitingRemoteRevokeToRemove => generated_by_local, HTLCState::AwaitingRemovedRemoteRevoke => false, HTLCState::LocalRemoved => !generated_by_local, - HTLCState::LocalRemovedAwaitingCommitment => false, }; if include { @@ -749,10 +746,6 @@ impl Channel { value_to_self_msat_offset += htlc.amount_msat as i64; } }, - HTLCState::LocalRemovedAwaitingCommitment => { - assert!(htlc.local_removed_fulfilled); - value_to_self_msat_offset += htlc.amount_msat as i64; - }, _ => {}, } } @@ -1018,7 +1011,7 @@ impl Channel { let mut pending_idx = std::usize::MAX; for (idx, htlc) in self.pending_htlcs.iter().enumerate() { if !htlc.outbound && htlc.payment_hash == payment_hash_calc && - htlc.state != HTLCState::LocalRemoved && htlc.state != HTLCState::LocalRemovedAwaitingCommitment { + htlc.state != HTLCState::LocalRemoved { if let Some(PendingHTLCStatus::Fail(_)) = htlc.pending_forward_state { } else { if pending_idx != std::usize::MAX { @@ -1143,7 +1136,7 @@ impl Channel { // we'll fail this one as soon as remote commits to it. continue; } - } else if htlc.state == HTLCState::LocalRemoved || htlc.state == HTLCState::LocalRemovedAwaitingCommitment { + } else if htlc.state == HTLCState::LocalRemoved { return Err(HandleError{err: "Unable to find a pending HTLC which matched the given payment preimage", action: None}); } else { panic!("Have an inbound HTLC when not awaiting remote revoke that had a garbage state"); @@ -1379,7 +1372,6 @@ impl Channel { HTLCState::AwaitingRemoteRevokeToRemove => { if for_remote_update_check { continue; } }, HTLCState::AwaitingRemovedRemoteRevoke => { if for_remote_update_check { continue; } }, HTLCState::LocalRemoved => {}, - HTLCState::LocalRemovedAwaitingCommitment => { if for_remote_update_check { continue; } }, } if !htlc.outbound { inbound_htlc_count += 1; @@ -1556,16 +1548,6 @@ impl Channel { need_our_commitment = true; } } - // Finally delete all the LocalRemovedAwaitingCommitment HTLCs - // We really shouldnt have two passes here, but retain gives a non-mutable ref (Rust bug) - let mut claimed_value_msat = 0; - self.pending_htlcs.retain(|htlc| { - if htlc.state == HTLCState::LocalRemovedAwaitingCommitment { - claimed_value_msat += htlc.amount_msat; - false - } else { true } - }); - self.value_to_self_msat += claimed_value_msat; self.cur_local_commitment_transaction_number -= 1; self.last_local_commitment_txn = new_local_commitment_txn; @@ -1689,7 +1671,10 @@ impl Channel { // We really shouldnt have two passes here, but retain gives a non-mutable ref (Rust bug) self.pending_htlcs.retain(|htlc| { if htlc.state == HTLCState::LocalRemoved { - if htlc.local_removed_fulfilled { true } else { false } + if htlc.local_removed_fulfilled { + value_to_self_msat_diff += htlc.amount_msat as i64; + } + false } else if htlc.state == HTLCState::AwaitingRemovedRemoteRevoke { if let Some(reason) = htlc.fail_reason.clone() { // We really want take() here, but, again, non-mut ref :( revoked_htlcs.push((htlc.payment_hash, reason)); @@ -1724,9 +1709,6 @@ impl Channel { } else if htlc.state == HTLCState::AwaitingRemoteRevokeToRemove { htlc.state = HTLCState::AwaitingRemovedRemoteRevoke; require_commitment = true; - } else if htlc.state == HTLCState::LocalRemoved { - assert!(htlc.local_removed_fulfilled); - htlc.state = HTLCState::LocalRemovedAwaitingCommitment; } } self.value_to_self_msat = (self.value_to_self_msat as i64 + value_to_self_msat_diff) as u64; From 3f5f3def639cb0d23a8f31122b994d71774c2014 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 5 Sep 2018 20:19:09 -0400 Subject: [PATCH 4/4] Add further clarification TODO in finish_force_close_channel --- src/ln/channelmanager.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 825669402..8abaf1460 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -447,6 +447,8 @@ impl ChannelManager { //TODO: We need to handle monitoring of pending offered HTLCs which just hit the chain and //may be claimed, resulting in us claiming the inbound HTLCs (and back-failing after //timeouts are hit and our claims confirm). + //TODO: In any case, we need to make sure we remove any pending htlc tracking (via + //fail_backwards or claim_funds) eventually for all HTLCs that were in the channel } /// Force closes a channel, immediately broadcasting the latest local commitment transaction to