Merge pull request #1025 from TheBlueMatt/2021-07-detect-htlcs-on-local-commitment
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Tue, 10 Aug 2021 15:10:45 +0000 (15:10 +0000)
committerGitHub <noreply@github.com>
Tue, 10 Aug 2021 15:10:45 +0000 (15:10 +0000)
1  2 
lightning/src/chain/channelmonitor.rs
lightning/src/ln/mod.rs

index 6c11703d3158fc86567a49b2c81d771a1b8517b0,b5f6bce228a3886ee17d337ddcac2f103ba32cd7..3c75ec9e99ecd68baa51be9d1f60eb8e39becb2f
@@@ -438,9 -438,6 +438,9 @@@ pub(crate) enum ChannelMonitorUpdateSte
                /// think we've fallen behind!
                should_broadcast: bool,
        },
 +      ShutdownScript {
 +              scriptpubkey: Script,
 +      },
  }
  
  impl_writeable_tlv_based_enum!(ChannelMonitorUpdateStep,
        (4, ChannelForceClosed) => {
                (0, should_broadcast, required),
        },
 +      (5, ShutdownScript) => {
 +              (0, scriptpubkey, required),
 +      },
  ;);
  
  /// A ChannelMonitor handles chain events (blocks connected and disconnected) and generates
@@@ -499,7 -493,7 +499,7 @@@ pub(crate) struct ChannelMonitorImpl<Si
        destination_script: Script,
        broadcasted_holder_revokable_script: Option<(Script, PublicKey, PublicKey)>,
        counterparty_payment_script: Script,
 -      shutdown_script: Script,
 +      shutdown_script: Option<Script>,
  
        channel_keys_id: [u8; 32],
        holder_revocation_basepoint: PublicKey,
        on_holder_tx_csv: u16,
  
        commitment_secrets: CounterpartyCommitmentSecrets,
+       /// The set of outpoints in each counterparty commitment transaction. We always need at least
+       /// the payment hash from `HTLCOutputInCommitment` to claim even a revoked commitment
+       /// transaction broadcast as we need to be able to construct the witness script in all cases.
        counterparty_claimable_outpoints: HashMap<Txid, Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>,
        /// We cannot identify HTLC-Success or HTLC-Timeout transactions by themselves on the chain.
        /// Nor can we figure out their commitment numbers without the commitment transaction they are
@@@ -675,10 -672,7 +678,10 @@@ impl<Signer: Sign> Writeable for Channe
                }
  
                self.counterparty_payment_script.write(writer)?;
 -              self.shutdown_script.write(writer)?;
 +              match &self.shutdown_script {
 +                      Some(script) => script.write(writer)?,
 +                      None => Script::new().write(writer)?,
 +              }
  
                self.channel_keys_id.write(writer)?;
                self.holder_revocation_basepoint.write(writer)?;
  }
  
  impl<Signer: Sign> ChannelMonitor<Signer> {
 -      pub(crate) fn new(secp_ctx: Secp256k1<secp256k1::All>, keys: Signer, shutdown_pubkey: &PublicKey,
 +      pub(crate) fn new(secp_ctx: Secp256k1<secp256k1::All>, keys: Signer, shutdown_script: Option<Script>,
                          on_counterparty_tx_csv: u16, destination_script: &Script, funding_info: (OutPoint, Script),
                          channel_parameters: &ChannelTransactionParameters,
                          funding_redeemscript: Script, channel_value_satoshis: u64,
                          best_block: BestBlock) -> ChannelMonitor<Signer> {
  
                assert!(commitment_transaction_number_obscure_factor <= (1 << 48));
 -              let our_channel_close_key_hash = WPubkeyHash::hash(&shutdown_pubkey.serialize());
 -              let shutdown_script = Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&our_channel_close_key_hash[..]).into_script();
                let payment_key_hash = WPubkeyHash::hash(&keys.pubkeys().payment_point.serialize());
                let counterparty_payment_script = Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&payment_key_hash[..]).into_script();
  
        }
  }
  
+ /// Compares a broadcasted commitment transaction's HTLCs with those in the latest state,
+ /// failing any HTLCs which didn't make it into the broadcasted commitment transaction back
+ /// after ANTI_REORG_DELAY blocks.
+ ///
+ /// We always compare against the set of HTLCs in counterparty commitment transactions, as those
+ /// are the commitment transactions which are generated by us. The off-chain state machine in
+ /// `Channel` will automatically resolve any HTLCs which were never included in a commitment
+ /// transaction when it detects channel closure, but it is up to us to ensure any HTLCs which were
+ /// included in a remote commitment transaction are failed back if they are not present in the
+ /// broadcasted commitment transaction.
+ ///
+ /// Specifically, the removal process for HTLCs in `Channel` is always based on the counterparty
+ /// sending a `revoke_and_ack`, which causes us to clear `prev_counterparty_commitment_txid`. Thus,
+ /// as long as we examine both the current counterparty commitment transaction and, if it hasn't
+ /// been revoked yet, the previous one, we we will never "forget" to resolve an HTLC.
+ macro_rules! fail_unbroadcast_htlcs {
+       ($self: expr, $commitment_tx_type: expr, $commitment_tx_conf_height: expr, $confirmed_htlcs_list: expr, $logger: expr) => { {
+               macro_rules! check_htlc_fails {
+                       ($txid: expr, $commitment_tx: expr) => {
+                               if let Some(ref latest_outpoints) = $self.counterparty_claimable_outpoints.get($txid) {
+                                       for &(ref htlc, ref source_option) in latest_outpoints.iter() {
+                                               if let &Some(ref source) = source_option {
+                                                       // Check if the HTLC is present in the commitment transaction that was
+                                                       // broadcast, but not if it was below the dust limit, which we should
+                                                       // fail backwards immediately as there is no way for us to learn the
+                                                       // payment_preimage.
+                                                       // Note that if the dust limit were allowed to change between
+                                                       // commitment transactions we'd want to be check whether *any*
+                                                       // broadcastable commitment transaction has the HTLC in it, but it
+                                                       // cannot currently change after channel initialization, so we don't
+                                                       // need to here.
+                                                       let confirmed_htlcs_iter: &mut Iterator<Item = (&HTLCOutputInCommitment, Option<&HTLCSource>)> = &mut $confirmed_htlcs_list;
+                                                       let mut matched_htlc = false;
+                                                       for (ref broadcast_htlc, ref broadcast_source) in confirmed_htlcs_iter {
+                                                               if broadcast_htlc.transaction_output_index.is_some() && Some(&**source) == *broadcast_source {
+                                                                       matched_htlc = true;
+                                                                       break;
+                                                               }
+                                                       }
+                                                       if matched_htlc { continue; }
+                                                       $self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
+                                                               if entry.height != $commitment_tx_conf_height { return true; }
+                                                               match entry.event {
+                                                                       OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
+                                                                               *update_source != **source
+                                                                       },
+                                                                       _ => true,
+                                                               }
+                                                       });
+                                                       let entry = OnchainEventEntry {
+                                                               txid: *$txid,
+                                                               height: $commitment_tx_conf_height,
+                                                               event: OnchainEvent::HTLCUpdate {
+                                                                       source: (**source).clone(),
+                                                                       payment_hash: htlc.payment_hash.clone(),
+                                                                       onchain_value_satoshis: Some(htlc.amount_msat / 1000),
+                                                               },
+                                                       };
+                                                       log_trace!($logger, "Failing HTLC with payment_hash {} from {} counterparty commitment tx due to broadcast of {} commitment transaction, waiting for confirmation (at height {})",
+                                                               log_bytes!(htlc.payment_hash.0), $commitment_tx, $commitment_tx_type, entry.confirmation_threshold());
+                                                       $self.onchain_events_awaiting_threshold_conf.push(entry);
+                                               }
+                                       }
+                               }
+                       }
+               }
+               if let Some(ref txid) = $self.current_counterparty_commitment_txid {
+                       check_htlc_fails!(txid, "current");
+               }
+               if let Some(ref txid) = $self.prev_counterparty_commitment_txid {
+                       check_htlc_fails!(txid, "previous");
+               }
+       } }
+ }
  impl<Signer: Sign> ChannelMonitorImpl<Signer> {
        /// Inserts a revocation secret into this channel monitor. Prunes old preimages if neither
        /// needed by holder commitment transactions HTCLs nor by counterparty ones. Unless we haven't already seen
                                                // shouldn't print the scary warning above.
                                                log_info!(logger, "Channel off-chain state closed after we broadcasted our latest commitment transaction.");
                                        }
 -                              }
 +                              },
 +                              ChannelMonitorUpdateStep::ShutdownScript { scriptpubkey } => {
 +                                      log_trace!(logger, "Updating ChannelMonitor with shutdown script");
 +                                      if let Some(shutdown_script) = self.shutdown_script.replace(scriptpubkey.clone()) {
 +                                              panic!("Attempted to replace shutdown script {} with {}", shutdown_script, scriptpubkey);
 +                                      }
 +                              },
                        }
                }
                self.latest_update_id = updates.update_id;
                                }
                                self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);
  
-                               macro_rules! check_htlc_fails {
-                                       ($txid: expr, $commitment_tx: expr) => {
-                                               if let Some(ref outpoints) = self.counterparty_claimable_outpoints.get($txid) {
-                                                       for &(ref htlc, ref source_option) in outpoints.iter() {
-                                                               if let &Some(ref source) = source_option {
-                                                                       self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
-                                                                               if entry.height != height { return true; }
-                                                                               match entry.event {
-                                                                                       OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
-                                                                                               *update_source != **source
-                                                                                       },
-                                                                                       _ => true,
-                                                                               }
-                                                                       });
-                                                                       let entry = OnchainEventEntry {
-                                                                               txid: *$txid,
-                                                                               height,
-                                                                               event: OnchainEvent::HTLCUpdate {
-                                                                                       source: (**source).clone(),
-                                                                                       payment_hash: htlc.payment_hash.clone(),
-                                                                                       onchain_value_satoshis: Some(htlc.amount_msat / 1000),
-                                                                               },
-                                                                       };
-                                                                       log_info!(logger, "Failing HTLC with payment_hash {} from {} counterparty commitment tx due to broadcast of revoked counterparty commitment transaction, waiting for confirmation (at height {})", log_bytes!(htlc.payment_hash.0), $commitment_tx, entry.confirmation_threshold());
-                                                                       self.onchain_events_awaiting_threshold_conf.push(entry);
-                                                               }
-                                                       }
-                                               }
-                                       }
-                               }
-                               if let Some(ref txid) = self.current_counterparty_commitment_txid {
-                                       check_htlc_fails!(txid, "current");
-                               }
-                               if let Some(ref txid) = self.prev_counterparty_commitment_txid {
-                                       check_htlc_fails!(txid, "counterparty");
-                               }
-                               // No need to check holder commitment txn, symmetric HTLCSource must be present as per-htlc data on counterparty commitment tx
+                               fail_unbroadcast_htlcs!(self, "revoked counterparty", height, [].iter().map(|a| *a), logger);
                        }
                } else if let Some(per_commitment_data) = per_commitment_option {
                        // While this isn't useful yet, there is a potential race where if a counterparty
                        self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);
  
                        log_info!(logger, "Got broadcast of non-revoked counterparty commitment transaction {}", commitment_txid);
-                       macro_rules! check_htlc_fails {
-                               ($txid: expr, $commitment_tx: expr, $id: tt) => {
-                                       if let Some(ref latest_outpoints) = self.counterparty_claimable_outpoints.get($txid) {
-                                               $id: for &(ref htlc, ref source_option) in latest_outpoints.iter() {
-                                                       if let &Some(ref source) = source_option {
-                                                               // Check if the HTLC is present in the commitment transaction that was
-                                                               // broadcast, but not if it was below the dust limit, which we should
-                                                               // fail backwards immediately as there is no way for us to learn the
-                                                               // payment_preimage.
-                                                               // Note that if the dust limit were allowed to change between
-                                                               // commitment transactions we'd want to be check whether *any*
-                                                               // broadcastable commitment transaction has the HTLC in it, but it
-                                                               // cannot currently change after channel initialization, so we don't
-                                                               // need to here.
-                                                               for &(ref broadcast_htlc, ref broadcast_source) in per_commitment_data.iter() {
-                                                                       if broadcast_htlc.transaction_output_index.is_some() && Some(source) == broadcast_source.as_ref() {
-                                                                               continue $id;
-                                                                       }
-                                                               }
-                                                               log_trace!(logger, "Failing HTLC with payment_hash {} from {} counterparty commitment tx due to broadcast of counterparty commitment transaction", log_bytes!(htlc.payment_hash.0), $commitment_tx);
-                                                               self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
-                                                                       if entry.height != height { return true; }
-                                                                       match entry.event {
-                                                                               OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
-                                                                                       *update_source != **source
-                                                                               },
-                                                                               _ => true,
-                                                                       }
-                                                               });
-                                                               self.onchain_events_awaiting_threshold_conf.push(OnchainEventEntry {
-                                                                       txid: *$txid,
-                                                                       height,
-                                                                       event: OnchainEvent::HTLCUpdate {
-                                                                               source: (**source).clone(),
-                                                                               payment_hash: htlc.payment_hash.clone(),
-                                                                               onchain_value_satoshis: Some(htlc.amount_msat / 1000),
-                                                                       },
-                                                               });
-                                                       }
-                                               }
-                                       }
-                               }
-                       }
-                       if let Some(ref txid) = self.current_counterparty_commitment_txid {
-                               check_htlc_fails!(txid, "current", 'current_loop);
-                       }
-                       if let Some(ref txid) = self.prev_counterparty_commitment_txid {
-                               check_htlc_fails!(txid, "previous", 'prev_loop);
-                       }
+                       fail_unbroadcast_htlcs!(self, "counterparty", height, per_commitment_data.iter().map(|(a, b)| (a, b.as_ref().map(|b| b.as_ref()))), logger);
  
                        let htlc_claim_reqs = self.get_counterparty_htlc_output_claim_reqs(commitment_number, commitment_txid, Some(tx));
                        for req in htlc_claim_reqs {
                        let res = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, height);
                        let mut to_watch = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, tx);
                        append_onchain_update!(res, to_watch);
+                       fail_unbroadcast_htlcs!(self, "latest holder", height, self.current_holder_commitment_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
                } else if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
                        if holder_tx.txid == commitment_txid {
                                is_holder_tx = true;
                                let res = self.get_broadcasted_holder_claims(holder_tx, height);
                                let mut to_watch = self.get_broadcasted_holder_watch_outputs(holder_tx, tx);
                                append_onchain_update!(res, to_watch);
-                       }
-               }
-               macro_rules! fail_dust_htlcs_after_threshold_conf {
-                       ($holder_tx: expr, $commitment_tx: expr) => {
-                               for &(ref htlc, _, ref source) in &$holder_tx.htlc_outputs {
-                                       if htlc.transaction_output_index.is_none() {
-                                               if let &Some(ref source) = source {
-                                                       self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
-                                                               if entry.height != height { return true; }
-                                                               match entry.event {
-                                                                       OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
-                                                                               update_source != source
-                                                                       },
-                                                                       _ => true,
-                                                               }
-                                                       });
-                                                       let entry = OnchainEventEntry {
-                                                               txid: commitment_txid,
-                                                               height,
-                                                               event: OnchainEvent::HTLCUpdate {
-                                                                       source: source.clone(), payment_hash: htlc.payment_hash,
-                                                                       onchain_value_satoshis: Some(htlc.amount_msat / 1000)
-                                                               },
-                                                       };
-                                                       log_trace!(logger, "Failing HTLC with payment_hash {} from {} holder commitment tx due to broadcast of transaction, waiting confirmation (at height{})",
-                                                               log_bytes!(htlc.payment_hash.0), $commitment_tx, entry.confirmation_threshold());
-                                                       self.onchain_events_awaiting_threshold_conf.push(entry);
-                                               }
-                                       }
-                               }
+                               fail_unbroadcast_htlcs!(self, "previous holder", height, holder_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
                        }
                }
  
                if is_holder_tx {
-                       fail_dust_htlcs_after_threshold_conf!(self.current_holder_commitment_tx, "latest");
-                       if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
-                               fail_dust_htlcs_after_threshold_conf!(holder_tx, "previous");
-                       }
                }
  
                (claim_requests, (commitment_txid, watch_outputs))
                                }));
                                break;
                        }
 -                      if outp.script_pubkey == self.shutdown_script {
 +                      if self.shutdown_script.as_ref() == Some(&outp.script_pubkey) {
                                spendable_output = Some(SpendableOutputDescriptor::StaticOutput {
                                        outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
                                        output: outp.clone(),
@@@ -2635,10 -2582,7 +2595,10 @@@ impl<'a, Signer: Sign, K: KeysInterface
                        _ => return Err(DecodeError::InvalidValue),
                };
                let counterparty_payment_script = Readable::read(reader)?;
 -              let shutdown_script = Readable::read(reader)?;
 +              let shutdown_script = {
 +                      let script = <Script as Readable>::read(reader)?;
 +                      if script.is_empty() { None } else { Some(script) }
 +              };
  
                let channel_keys_id = Readable::read(reader)?;
                let holder_revocation_basepoint = Readable::read(reader)?;
@@@ -2870,7 -2814,6 +2830,7 @@@ mod tests 
        use ln::{PaymentPreimage, PaymentHash};
        use ln::chan_utils;
        use ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, ChannelTransactionParameters, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters};
 +      use ln::script::ShutdownScript;
        use util::test_utils::{TestLogger, TestBroadcaster, TestFeeEstimator};
        use bitcoin::secp256k1::key::{SecretKey,PublicKey};
        use bitcoin::secp256k1::Secp256k1;
                };
                // Prune with one old state and a holder commitment tx holding a few overlaps with the
                // old state.
 +              let shutdown_pubkey = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let best_block = BestBlock::from_genesis(Network::Testnet);
                let monitor = ChannelMonitor::new(Secp256k1::new(), keys,
 -                                                &PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()), 0, &Script::new(),
 +                                                Some(ShutdownScript::new_p2wpkh_from_pubkey(shutdown_pubkey).into_inner()), 0, &Script::new(),
                                                  (OutPoint { txid: Txid::from_slice(&[43; 32]).unwrap(), index: 0 }, Script::new()),
                                                  &channel_parameters,
                                                  Script::new(), 46, 0,
diff --combined lightning/src/ln/mod.rs
index 5f928eea072bb85bc3323defe036499e17079e35,f1ba914cabd81caeebce00f427fb0cd755504fca..bebd763f660b83569ac7e1c1eff93770d7696b12
@@@ -27,7 -27,6 +27,7 @@@ pub mod msgs
  pub mod peer_handler;
  pub mod chan_utils;
  pub mod features;
 +pub mod script;
  
  #[cfg(feature = "fuzztarget")]
  pub mod peer_channel_encryptor;
@@@ -53,6 -52,9 +53,9 @@@ mod reorg_tests
  #[cfg(test)]
  #[allow(unused_mut)]
  mod onion_route_tests;
+ #[cfg(test)]
+ #[allow(unused_mut)]
+ mod monitor_tests;
  
  pub use self::peer_channel_encryptor::LN_MAX_MSG_LEN;