From: Matt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Date: Thu, 6 May 2021 21:49:24 +0000 (+0000)
Subject: Merge pull request #911 from TheBlueMatt/2021-05-fix-cltv-diff
X-Git-Tag: v0.0.98~31
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=d2955be5cf7fa96057c416a7899e7db6a0c5e622;hp=-c;p=rust-lightning

Merge pull request #911 from TheBlueMatt/2021-05-fix-cltv-diff
---

d2955be5cf7fa96057c416a7899e7db6a0c5e622
diff --combined lightning/src/chain/channelmonitor.rs
index 004c2381,a2c1abe7..62cb740e
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@@ -206,7 -206,7 +206,7 @@@ pub(crate) const CLTV_SHARED_CLAIM_BUFF
  /// HTLC-Success transaction.
  /// In other words, this is an upper bound on how many blocks we think it can take us to get a
  /// transaction confirmed (and we use it in a few more, equivalent, places).
- pub(crate) const CLTV_CLAIM_BUFFER: u32 = 6;
+ pub(crate) const CLTV_CLAIM_BUFFER: u32 = 18;
  /// Number of blocks by which point we expect our counterparty to have seen new blocks on the
  /// network and done a full update_fail_htlc/commitment_signed dance (+ we've updated all our
  /// copies of ChannelMonitors, including watchtowers). We could enforce the contract by failing
@@@ -1568,7 -1568,6 +1568,7 @@@ impl<Signer: Sign> ChannelMonitorImpl<S
  					L::Target: Logger,
  	{
  		for tx in self.get_latest_holder_commitment_txn(logger).iter() {
 +			log_info!(logger, "Broadcasting local {}", log_tx!(tx));
  			broadcaster.broadcast_transaction(tx);
  		}
  		self.pending_monitor_events.push(MonitorEvent::CommitmentTxBroadcasted(self.funding_info.0));
diff --combined lightning/src/ln/channelmanager.rs
index da775d1a,9cff2fcf..52ea21f0
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@@ -563,7 -563,7 +563,7 @@@ pub const BREAKDOWN_TIMEOUT: u16 = 6 * 
  pub(crate) const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 2 * 6 * 24 * 7;
  
  /// The minimum number of blocks between an inbound HTLC's CLTV and the corresponding outbound
- /// HTLC's CLTV. The current default represents roughly six hours of blocks at six blocks/hour.
+ /// HTLC's CLTV. The current default represents roughly seven hours of blocks at six blocks/hour.
  ///
  /// This can be increased (but not decreased) through [`ChannelConfig::cltv_expiry_delta`]
  ///
@@@ -572,13 -572,16 +572,16 @@@
  // i.e. the node we forwarded the payment on to should always have enough room to reliably time out
  // the HTLC via a full update_fail_htlc/commitment_signed dance before we hit the
  // CLTV_CLAIM_BUFFER point (we static assert that it's at least 3 blocks more).
- pub const MIN_CLTV_EXPIRY_DELTA: u16 = 6 * 6;
+ pub const MIN_CLTV_EXPIRY_DELTA: u16 = 6*7;
  pub(super) const CLTV_FAR_FAR_AWAY: u32 = 6 * 24 * 7; //TODO?
  
  /// Minimum CLTV difference between the current block height and received inbound payments.
  /// Invoices generated for payment to us must set their `min_final_cltv_expiry` field to at least
  /// this value.
- pub const MIN_FINAL_CLTV_EXPIRY: u32 = HTLC_FAIL_BACK_BUFFER;
+ // Note that we fail if exactly HTLC_FAIL_BACK_BUFFER + 1 was used, so we need to add one for
+ // any payments to succeed. Further, we don't want payments to fail if a block was found while
+ // a payment was being routed, so we add an extra block to be safe.
+ pub const MIN_FINAL_CLTV_EXPIRY: u32 = HTLC_FAIL_BACK_BUFFER + 3;
  
  // Check that our CLTV_EXPIRY is at least CLTV_CLAIM_BUFFER + ANTI_REORG_DELAY + LATENCY_GRACE_PERIOD_BLOCKS,
  // ie that if the next-hop peer fails the HTLC within
@@@ -590,7 -593,7 +593,7 @@@
  #[allow(dead_code)]
  const CHECK_CLTV_EXPIRY_SANITY: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - CLTV_CLAIM_BUFFER - ANTI_REORG_DELAY - LATENCY_GRACE_PERIOD_BLOCKS;
  
- // Check for ability of an attacker to make us fail on-chain by delaying inbound claim. See
+ // Check for ability of an attacker to make us fail on-chain by delaying an HTLC claim. See
  // ChannelMontior::would_broadcast_at_height for a description of why this is needed.
  #[deny(const_err)]
  #[allow(dead_code)]
@@@ -1743,17 -1746,13 +1746,17 @@@ impl<Signer: Sign, M: Deref, T: Deref, 
  	/// only Tor Onion addresses.
  	///
  	/// Panics if addresses is absurdly large (more than 500).
 -	pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], addresses: Vec<NetAddress>) {
 +	pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], mut addresses: Vec<NetAddress>) {
  		let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
  
  		if addresses.len() > 500 {
  			panic!("More than half the message size was taken up by public addresses!");
  		}
  
 +		// While all existing nodes handle unsorted addresses just fine, the spec requires that
 +		// addresses be sorted for future compatibility.
 +		addresses.sort_by_key(|addr| addr.get_id());
 +
  		let announcement = msgs::UnsignedNodeAnnouncement {
  			features: NodeFeatures::known(),
  			timestamp: self.last_node_announcement_serial.fetch_add(1, Ordering::AcqRel) as u32,
@@@ -2540,7 -2539,6 +2543,7 @@@
  				},
  			}
  			if let Some(tx) = funding_broadcastable {
 +				log_info!(self.logger, "Broadcasting funding transaction with txid {}", tx.txid());
  				self.tx_broadcaster.broadcast_transaction(&tx);
  			}
  			if let Some(msg) = funding_locked {
@@@ -2696,7 -2694,6 +2699,7 @@@
  				hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
  			}
  		};
 +		log_info!(self.logger, "Broadcasting funding transaction with txid {}", funding_tx.txid());
  		self.tx_broadcaster.broadcast_transaction(&funding_tx);
  		Ok(())
  	}
@@@ -2811,7 -2808,7 +2814,7 @@@
  			}
  		};
  		if let Some(broadcast_tx) = tx {
 -			log_trace!(self.logger, "Broadcast onchain {}", log_tx!(broadcast_tx));
 +			log_info!(self.logger, "Broadcasting {}", log_tx!(broadcast_tx));
  			self.tx_broadcaster.broadcast_transaction(&broadcast_tx);
  		}
  		if let Some(chan) = chan_option {