From: Wilmer Paulino Date: Tue, 28 Mar 2023 19:03:19 +0000 (-0700) Subject: Re-work PackageSolvingData::absolute_tx_timelock X-Git-Tag: v0.0.115~48^2~2 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=2ac09711d3e2c329d96e0bd60938ba83ff382b37;p=rust-lightning Re-work PackageSolvingData::absolute_tx_timelock Previously, this would return the earliest height the output could be confirmed, which seems to no longer be useful. The only use of the method was to determine whether we should delay a package to a future block. Instead, we choose to return the absolute locktime the transaction spending the output should have, which better corresponds to the method name and still supports the delay functionality mentioned. Doing so also allows us to expose the locktime required for HTLC transactions we need to broadcast based on our own commitments for anchor channels. --- diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 2c570f580..0ff0df59c 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -654,16 +654,17 @@ impl OnchainTxHandler .find(|locked_package| locked_package.outpoints() == req.outpoints()); if let Some(package) = timelocked_equivalent_package { log_info!(logger, "Ignoring second claim for outpoint {}:{}, we already have one which we're waiting on a timelock at {} for.", - req.outpoints()[0].txid, req.outpoints()[0].vout, package.package_timelock()); + req.outpoints()[0].txid, req.outpoints()[0].vout, package.package_locktime(cur_height)); continue; } - if req.package_timelock() > cur_height + 1 { - log_info!(logger, "Delaying claim of package until its timelock at {} (current height {}), the following outpoints are spent:", req.package_timelock(), cur_height); + let package_locktime = req.package_locktime(cur_height); + if package_locktime > cur_height + 1 { + log_info!(logger, "Delaying claim of package until its timelock at {} (current height {}), the following outpoints are spent:", package_locktime, cur_height); for outpoint in req.outpoints() { log_info!(logger, " Outpoint {}", outpoint); } - self.locktimed_packages.entry(req.package_timelock()).or_insert(Vec::new()).push(req); + self.locktimed_packages.entry(package_locktime).or_insert(Vec::new()).push(req); continue; } diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 5537a56f2..2ab374841 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -460,18 +460,23 @@ impl PackageSolvingData { _ => { panic!("API Error!"); } } } - fn absolute_tx_timelock(&self, output_conf_height: u32) -> u32 { - // Get the absolute timelock at which this output can be spent given the height at which - // this output was confirmed. We use `output_conf_height + 1` as a safe default as we can - // be confirmed in the next block and transactions with time lock `current_height + 1` - // always propagate. + fn absolute_tx_timelock(&self, current_height: u32) -> u32 { + // We use `current_height + 1` as our default locktime to discourage fee sniping and because + // transactions with it always propagate. let absolute_timelock = match self { - PackageSolvingData::RevokedOutput(_) => output_conf_height + 1, - PackageSolvingData::RevokedHTLCOutput(_) => output_conf_height + 1, - PackageSolvingData::CounterpartyOfferedHTLCOutput(_) => output_conf_height + 1, - PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => cmp::max(outp.htlc.cltv_expiry, output_conf_height + 1), - PackageSolvingData::HolderHTLCOutput(ref outp) => cmp::max(outp.cltv_expiry, output_conf_height + 1), - PackageSolvingData::HolderFundingOutput(_) => output_conf_height + 1, + PackageSolvingData::RevokedOutput(_) => current_height + 1, + PackageSolvingData::RevokedHTLCOutput(_) => current_height + 1, + PackageSolvingData::CounterpartyOfferedHTLCOutput(_) => current_height + 1, + PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => cmp::max(outp.htlc.cltv_expiry, current_height + 1), + // HTLC timeout/success transactions rely on a fixed timelock due to the counterparty's + // signature. + PackageSolvingData::HolderHTLCOutput(ref outp) => { + if outp.preimage.is_some() { + debug_assert_eq!(outp.cltv_expiry, 0); + } + outp.cltv_expiry + }, + PackageSolvingData::HolderFundingOutput(_) => current_height + 1, }; absolute_timelock } @@ -638,9 +643,36 @@ impl PackageTemplate { } amounts } - pub(crate) fn package_timelock(&self) -> u32 { - self.inputs.iter().map(|(_, outp)| outp.absolute_tx_timelock(self.height_original)) - .max().expect("There must always be at least one output to spend in a PackageTemplate") + pub(crate) fn package_locktime(&self, current_height: u32) -> u32 { + let locktime = self.inputs.iter().map(|(_, outp)| outp.absolute_tx_timelock(current_height)) + .max().expect("There must always be at least one output to spend in a PackageTemplate"); + + // If we ever try to aggregate a `HolderHTLCOutput`s with another output type, we'll likely + // end up with an incorrect transaction locktime since the counterparty has included it in + // its HTLC signature. This should never happen unless we decide to aggregate outputs across + // different channel commitments. + #[cfg(debug_assertions)] { + if self.inputs.iter().any(|(_, outp)| + if let PackageSolvingData::HolderHTLCOutput(outp) = outp { + outp.preimage.is_some() + } else { + false + } + ) { + debug_assert_eq!(locktime, 0); + }; + for timeout_htlc_expiry in self.inputs.iter().filter_map(|(_, outp)| + if let PackageSolvingData::HolderHTLCOutput(outp) = outp { + if outp.preimage.is_none() { + Some(outp.cltv_expiry) + } else { None } + } else { None } + ) { + debug_assert_eq!(locktime, timeout_htlc_expiry); + } + } + + locktime } pub(crate) fn package_weight(&self, destination_script: &Script) -> usize { let mut inputs_weight = 0;