Skip to content
290 changes: 169 additions & 121 deletions lightning/src/events/bump_transaction/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ use crate::util::logger::Logger;
use bitcoin::amount::Amount;
use bitcoin::consensus::Encodable;
use bitcoin::constants::WITNESS_SCALE_FACTOR;
use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::hashes::{Hash, HashEngine};
use bitcoin::locktime::absolute::LockTime;
use bitcoin::secp256k1;
use bitcoin::secp256k1::ecdsa::Signature;
Expand Down Expand Up @@ -847,24 +849,13 @@ where
/// Handles a [`BumpTransactionEvent::HTLCResolution`] event variant by producing a
/// fully-signed, fee-bumped HTLC transaction that is broadcast to the network.
async fn handle_htlc_resolution(
&self, claim_id: ClaimId, target_feerate_sat_per_1000_weight: u32,
htlc_descriptors: &[HTLCDescriptor], tx_lock_time: LockTime,
&self, target_feerate_sat_per_1000_weight: u32, htlc_descriptors: &[HTLCDescriptor],
tx_lock_time: LockTime,
) -> Result<(), ()> {
let channel_type = &htlc_descriptors[0]
.channel_derivation_parameters
.transaction_parameters
.channel_type_features;
let mut htlc_tx = Transaction {
version: if channel_type.supports_anchor_zero_fee_commitments() {
Version::non_standard(3)
} else {
Version::TWO
},
lock_time: tx_lock_time,
input: vec![],
output: vec![],
};
let mut must_spend = Vec::with_capacity(htlc_descriptors.len());
let (htlc_success_witness_weight, htlc_timeout_witness_weight) =
if channel_type.supports_anchor_zero_fee_commitments() {
(
Expand All @@ -879,123 +870,181 @@ where
} else {
panic!("channel type should be either zero-fee HTLCs, or zero-fee commitments");
};
for htlc_descriptor in htlc_descriptors {
let htlc_input = htlc_descriptor.unsigned_tx_input();
must_spend.push(Input {
outpoint: htlc_input.previous_output.clone(),
previous_utxo: htlc_descriptor.previous_utxo(&self.secp),
satisfaction_weight: EMPTY_SCRIPT_SIG_WEIGHT
+ if htlc_descriptor.preimage.is_some() {
htlc_success_witness_weight
} else {
htlc_timeout_witness_weight
},
});
htlc_tx.input.push(htlc_input);
let htlc_output = htlc_descriptor.tx_output(&self.secp);
htlc_tx.output.push(htlc_output);
}

log_debug!(
self.logger,
"Performing coin selection for HTLC transaction targeting {} sat/kW",
target_feerate_sat_per_1000_weight
);
let max_weight = if channel_type.supports_anchor_zero_fee_commitments() {
// Cap the size of transactions claiming `HolderHTLCOutput` in 0FC channels.
// Otherwise, we could hit the max 10_000vB size limit on V3 transactions
// (BIP 431 rule 4).
chan_utils::TRUC_MAX_WEIGHT
} else {
u64::MAX
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well cap this to the max weight we can relay?

};
let mut broadcasted_htlcs = 0;
let mut batch_size = htlc_descriptors.len() - broadcasted_htlcs;

while broadcasted_htlcs < htlc_descriptors.len() {
let htlcs = &htlc_descriptors[broadcasted_htlcs..broadcasted_htlcs + batch_size];
// Generate a new claim_id to map a user-provided utxo to this
// particular set of HTLCs via `select_confirmed_utxos`.
//
// This matches the scheme used in `onchaintx.rs`, so for non-0fc-channels,
// this should match the `ClaimId` of the claim generated in `onchaintx.rs`.
let mut engine = Sha256::engine();
for htlc in htlcs {
engine.input(&htlc.commitment_txid.to_byte_array());
engine.input(&htlc.htlc.transaction_output_index.unwrap().to_be_bytes());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: pull out into ClaimID::from_prev_outs to de-duplicate?

let utxo_id = ClaimId(Sha256::from_engine(engine).to_byte_array());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we want to reuse the same ID until we move on to the next batch?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, was just wondering this - should we use the one from the event at least until we split the batch, which would work in the common case. Might want to sort the outputs so that we use them in a deterministic order (I imagine they are in the event, but...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed below, don't quiet yet sort the HTLCs, need to look into whether they are already deterministic in the event.

log_info!(
self.logger,
"Batch transaction assigned to UTXO id {} contains {} HTLCs: {}",
log_bytes!(utxo_id.0),
htlcs.len(),
log_iter!(htlcs.iter().map(|d| d.outpoint()))
);
let mut htlc_tx = Transaction {
version: if channel_type.supports_anchor_zero_fee_commitments() {
Version::non_standard(3)
} else {
Version::TWO
},
lock_time: tx_lock_time,
input: vec![],
output: vec![],
};
let mut must_spend = Vec::with_capacity(htlcs.len());
for htlc_descriptor in htlcs {
let htlc_input = htlc_descriptor.unsigned_tx_input();
must_spend.push(Input {
outpoint: htlc_input.previous_output.clone(),
previous_utxo: htlc_descriptor.previous_utxo(&self.secp),
satisfaction_weight: EMPTY_SCRIPT_SIG_WEIGHT
+ if htlc_descriptor.preimage.is_some() {
htlc_success_witness_weight
} else {
htlc_timeout_witness_weight
},
});
htlc_tx.input.push(htlc_input);
let htlc_output = htlc_descriptor.tx_output(&self.secp);
htlc_tx.output.push(htlc_output);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to break out of this loop early if we get close to the max_weight? Would mean generating the claim ID during this loop rather than separately but that's trivial. Would avoid an extra coin-selection pass in a few cases, but its not critical.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed below

}

#[cfg(debug_assertions)]
let must_spend_satisfaction_weight =
must_spend.iter().map(|input| input.satisfaction_weight).sum::<u64>();
#[cfg(debug_assertions)]
let must_spend_amount =
must_spend.iter().map(|input| input.previous_utxo.value.to_sat()).sum::<u64>();
log_debug!(
self.logger,
"Performing coin selection for HTLC transaction targeting {} sat/kW",
target_feerate_sat_per_1000_weight
);

let coin_selection: CoinSelection = self
.utxo_source
.select_confirmed_utxos(
claim_id,
must_spend,
&htlc_tx.output,
target_feerate_sat_per_1000_weight,
)
.await?;

#[cfg(debug_assertions)]
let input_satisfaction_weight: u64 =
coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum();
#[cfg(debug_assertions)]
let total_satisfaction_weight = must_spend_satisfaction_weight + input_satisfaction_weight;
#[cfg(debug_assertions)]
let input_value: u64 =
coin_selection.confirmed_utxos.iter().map(|utxo| utxo.output.value.to_sat()).sum();
#[cfg(debug_assertions)]
let total_input_amount = must_spend_amount + input_value;

self.process_coin_selection(&mut htlc_tx, &coin_selection);

// construct psbt
let mut htlc_psbt = Psbt::from_unsigned_tx(htlc_tx).unwrap();
// add witness_utxo to htlc inputs
for (i, htlc_descriptor) in htlc_descriptors.iter().enumerate() {
debug_assert_eq!(
htlc_psbt.unsigned_tx.input[i].previous_output,
htlc_descriptor.outpoint()
let must_spend_satisfaction_weight =
must_spend.iter().map(|input| input.satisfaction_weight).sum::<u64>();
#[cfg(debug_assertions)]
let must_spend_amount =
must_spend.iter().map(|input| input.previous_utxo.value.to_sat()).sum::<u64>();

let coin_selection: CoinSelection = self
.utxo_source
.select_confirmed_utxos(
utxo_id,
must_spend,
&htlc_tx.output,
target_feerate_sat_per_1000_weight,
)
.await?;

let input_satisfaction_weight: u64 =
coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum();
let total_satisfaction_weight =
must_spend_satisfaction_weight + input_satisfaction_weight;

#[cfg(debug_assertions)]
let input_value: u64 =
coin_selection.confirmed_utxos.iter().map(|utxo| utxo.output.value.to_sat()).sum();
#[cfg(debug_assertions)]
let total_input_amount = must_spend_amount + input_value;

self.process_coin_selection(&mut htlc_tx, &coin_selection);

let unsigned_tx_weight =
htlc_tx.weight().to_wu() - (htlc_tx.input.len() as u64 * EMPTY_SCRIPT_SIG_WEIGHT);
let expected_signed_tx_weight = unsigned_tx_weight + total_satisfaction_weight;
if expected_signed_tx_weight >= max_weight {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be >? We're allowed to be on exactly 10,000 vbytes, just no larger.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this, but went for the more conservative approach here.

let extra_weight = expected_signed_tx_weight - max_weight;
let htlcs_to_remove = (extra_weight / chan_utils::htlc_timeout_tx_weight(channel_type)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not critical, but I think these weights include transaction overhead (version etc) so we'll overcount by 40 wu per htlc?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, we should just use the weight of the HTLC input-output pair

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks ! yes this likely resolves the weird + 2 I've got below

// If we remove extra_weight / timeout_weight + 1 we sometimes still land above max_weight
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this wouldn't be an issue if we just round up the integer division?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likely it's because htlc_timeout_tx_weight is the weight of the full-transaction, but we want to only count the bytes from the single input-output pair

+ 2) as usize;
batch_size = batch_size.checked_sub(htlcs_to_remove).ok_or(())?;
continue;
}
broadcasted_htlcs += batch_size;
batch_size = htlc_descriptors.len() - broadcasted_htlcs;

// construct psbt
let mut htlc_psbt = Psbt::from_unsigned_tx(htlc_tx).unwrap();
// add witness_utxo to htlc inputs
for (i, htlc_descriptor) in htlcs.iter().enumerate() {
debug_assert_eq!(
htlc_psbt.unsigned_tx.input[i].previous_output,
htlc_descriptor.outpoint()
);
htlc_psbt.inputs[i].witness_utxo = Some(htlc_descriptor.previous_utxo(&self.secp));
}

// add witness_utxo to remaining inputs
for (idx, utxo) in coin_selection.confirmed_utxos.into_iter().enumerate() {
// offset to skip the htlc inputs
let index = idx + htlcs.len();
debug_assert_eq!(htlc_psbt.unsigned_tx.input[index].previous_output, utxo.outpoint);
if utxo.output.script_pubkey.is_witness_program() {
htlc_psbt.inputs[index].witness_utxo = Some(utxo.output);
}
}

log_debug!(
self.logger,
"Signing HTLC transaction {}",
htlc_psbt.unsigned_tx.compute_txid()
);
htlc_psbt.inputs[i].witness_utxo = Some(htlc_descriptor.previous_utxo(&self.secp));
}
// add witness_utxo to remaining inputs
for (idx, utxo) in coin_selection.confirmed_utxos.into_iter().enumerate() {
// offset to skip the htlc inputs
let index = idx + htlc_descriptors.len();
debug_assert_eq!(htlc_psbt.unsigned_tx.input[index].previous_output, utxo.outpoint);
if utxo.output.script_pubkey.is_witness_program() {
htlc_psbt.inputs[index].witness_utxo = Some(utxo.output);
let mut htlc_tx = self.utxo_source.sign_psbt(htlc_psbt).await?;

let mut signers = BTreeMap::new();
for (idx, htlc_descriptor) in htlcs.iter().enumerate() {
let keys_id = htlc_descriptor.channel_derivation_parameters.keys_id;
let signer = signers
.entry(keys_id)
.or_insert_with(|| self.signer_provider.derive_channel_signer(keys_id));
let htlc_sig = signer.sign_holder_htlc_transaction(
&htlc_tx,
idx,
htlc_descriptor,
&self.secp,
)?;
let witness_script = htlc_descriptor.witness_script(&self.secp);
htlc_tx.input[idx].witness =
htlc_descriptor.tx_input_witness(&htlc_sig, &witness_script);
}
}

#[cfg(debug_assertions)]
let unsigned_tx_weight = htlc_psbt.unsigned_tx.weight().to_wu()
- (htlc_psbt.unsigned_tx.input.len() as u64 * EMPTY_SCRIPT_SIG_WEIGHT);
#[cfg(debug_assertions)]
{
let signed_tx_weight = htlc_tx.weight().to_wu();
// Our estimate should be within a 2% error margin of the actual weight and we should
// never underestimate.
assert!(expected_signed_tx_weight >= signed_tx_weight);
assert!(expected_signed_tx_weight * 98 / 100 <= signed_tx_weight);

log_debug!(
self.logger,
"Signing HTLC transaction {}",
htlc_psbt.unsigned_tx.compute_txid()
);
htlc_tx = self.utxo_source.sign_psbt(htlc_psbt).await?;

let mut signers = BTreeMap::new();
for (idx, htlc_descriptor) in htlc_descriptors.iter().enumerate() {
let keys_id = htlc_descriptor.channel_derivation_parameters.keys_id;
let signer = signers
.entry(keys_id)
.or_insert_with(|| self.signer_provider.derive_channel_signer(keys_id));
let htlc_sig =
signer.sign_holder_htlc_transaction(&htlc_tx, idx, htlc_descriptor, &self.secp)?;
let witness_script = htlc_descriptor.witness_script(&self.secp);
htlc_tx.input[idx].witness =
htlc_descriptor.tx_input_witness(&htlc_sig, &witness_script);
}
let expected_signed_tx_fee =
fee_for_weight(target_feerate_sat_per_1000_weight, signed_tx_weight);
let signed_tx_fee = total_input_amount
- htlc_tx.output.iter().map(|output| output.value.to_sat()).sum::<u64>();
// Our feerate should always be at least what we were seeking. It may overshoot if
// the coin selector burned funds to an OP_RETURN without a change output.
assert!(signed_tx_fee >= expected_signed_tx_fee);
}

#[cfg(debug_assertions)]
{
let signed_tx_weight = htlc_tx.weight().to_wu();
let expected_signed_tx_weight = unsigned_tx_weight + total_satisfaction_weight;
// Our estimate should be within a 1% error margin of the actual weight and we should
// never underestimate.
assert!(expected_signed_tx_weight >= signed_tx_weight);
assert!(expected_signed_tx_weight * 99 / 100 <= signed_tx_weight);

let expected_signed_tx_fee =
fee_for_weight(target_feerate_sat_per_1000_weight, signed_tx_weight);
let signed_tx_fee = total_input_amount
- htlc_tx.output.iter().map(|output| output.value.to_sat()).sum::<u64>();
// Our feerate should always be at least what we were seeking. It may overshoot if
// the coin selector burned funds to an OP_RETURN without a change output.
assert!(signed_tx_fee >= expected_signed_tx_fee);
log_info!(self.logger, "Broadcasting {}", log_tx!(htlc_tx));
self.broadcaster.broadcast_transactions(&[&htlc_tx]);
}

log_info!(self.logger, "Broadcasting {}", log_tx!(htlc_tx));
self.broadcaster.broadcast_transactions(&[&htlc_tx]);
Ok(())
}

Expand Down Expand Up @@ -1046,7 +1095,6 @@ where
log_iter!(htlc_descriptors.iter().map(|d| d.outpoint()))
);
self.handle_htlc_resolution(
*claim_id,
*target_feerate_sat_per_1000_weight,
htlc_descriptors,
*tx_lock_time,
Expand Down
3 changes: 3 additions & 0 deletions lightning/src/ln/chan_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ pub const P2A_ANCHOR_INPUT_WITNESS_WEIGHT: u64 = 1;
/// The maximum value of a P2A anchor.
pub const P2A_MAX_VALUE: u64 = 240;

/// The maximum weight of a TRUC transaction.
pub const TRUC_MAX_WEIGHT: u64 = 10_000 * bitcoin::constants::WITNESS_SCALE_FACTOR as u64;

/// The upper bound weight of an HTLC timeout input from a commitment transaction with keyed anchor outputs.
pub const HTLC_TIMEOUT_INPUT_KEYED_ANCHOR_WITNESS_WEIGHT: u64 = 288;
/// The upper bound weight of an HTLC timeout input from a commitment transaction with a p2a anchor output.
Expand Down
Loading