Skip to content

Commit add0d7b

Browse files
author
Antoine Riard
committed
Move local commitment tx generation in OnchainTxHandler
Local Commitment Transaction can't be bumped without anchor outputs so their generation is one-time for now. We move them in OnchainTxHandler for simplifying ChannelMonitor and to prepare storage of keys material behind one external signer interface. Some tests break due to change in transaction broadcast order but number of transactions broadcast should stay the same.
1 parent 0ce46bd commit add0d7b

File tree

3 files changed

+141
-92
lines changed

3 files changed

+141
-92
lines changed

‎lightning/src/ln/channelmonitor.rs

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -443,6 +443,9 @@ pub(crate) enum InputMaterial {
443443
sigs: (Signature, Signature),
444444
preimage: Option<PaymentPreimage>,
445445
amount: u64,
446+
},
447+
Funding {
448+
channel_value: u64,
446449
}
447450
}
448451

@@ -472,6 +475,10 @@ impl Writeable for InputMaterial {
472475
sigs.1.write(writer)?;
473476
preimage.write(writer)?;
474477
writer.write_all(&byte_utils::be64_to_array(*amount))?;
478+
},
479+
&InputMaterial::Funding { ref channel_value } => {
480+
writer.write_all(&[3; 1])?;
481+
channel_value.write(writer)?;
475482
}
476483
}
477484
Ok(())
@@ -521,6 +528,12 @@ impl Readable for InputMaterial {
521528
preimage,
522529
amount
523530
}
531+
},
532+
3 => {
533+
let channel_value = Readable::read(reader)?;
534+
InputMaterial::Funding {
535+
channel_value
536+
}
524537
}
525538
_ => return Err(DecodeError::InvalidValue),
526539
};
@@ -1887,15 +1900,11 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
18871900
let should_broadcast = if let Some(_) = self.current_local_signed_commitment_tx {
18881901
self.would_broadcast_at_height(height)
18891902
} else { false };
1890-
if let Some(ref mut cur_local_tx) = self.current_local_signed_commitment_tx {
1891-
if should_broadcast {
1892-
self.onchain_detection.keys.sign_local_commitment(&mut cur_local_tx.tx, self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &mut self.secp_ctx);
1893-
}
1903+
if should_broadcast {
1904+
claimable_outpoints.push(ClaimRequest { absolute_timelock: height, aggregable: false, outpoint: BitcoinOutPoint { txid: self.onchain_detection.funding_info.as_ref().unwrap().0.txid.clone(), vout: self.onchain_detection.funding_info.as_ref().unwrap().0.index as u32 }, witness_data: InputMaterial::Funding { channel_value: self.channel_value_satoshis.unwrap() }});
18941905
}
18951906
if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx {
18961907
if should_broadcast {
1897-
log_trace!(self, "Broadcast onchain {}", log_tx!(cur_local_tx.tx.with_valid_witness()));
1898-
broadcaster.broadcast_transaction(&cur_local_tx.tx.with_valid_witness());
18991908
let (txs, new_outputs, _) = self.broadcast_by_local_state(&cur_local_tx);
19001909
if !new_outputs.is_empty() {
19011910
watch_outputs.push((cur_local_tx.txid.clone(), new_outputs));

‎lightning/src/ln/functional_tests.rs

Lines changed: 32 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2353,24 +2353,32 @@ fn claim_htlc_outputs_single_tx() {
23532353
// ChannelMonitor: local commitment + local HTLC-timeout (2)
23542354

23552355
// Check the pair local commitment and HTLC-timeout broadcast due to HTLC expiration
2356+
assert_eq!(node_txn[3].input.len(), 1);
2357+
check_spends!(node_txn[3], chan_1.3.clone());
23562358
assert_eq!(node_txn[0].input.len(), 1);
2357-
check_spends!(node_txn[0], chan_1.3);
2358-
assert_eq!(node_txn[1].input.len(), 1);
2359-
let witness_script = node_txn[1].input[0].witness.last().unwrap();
2359+
let witness_script = node_txn[0].input[0].witness.last().unwrap();
23602360
assert_eq!(witness_script.len(), OFFERED_HTLC_SCRIPT_WEIGHT); //Spending an offered htlc output
2361-
check_spends!(node_txn[1], node_txn[0]);
2361+
check_spends!(node_txn[0], node_txn[3].clone());
23622362

2363-
// Justice transactions are indices 2-3-4
2363+
// Justice transactions are indices 1-2-4
2364+
assert_eq!(node_txn[1].input.len(), 1);
23642365
assert_eq!(node_txn[2].input.len(), 1);
2365-
assert_eq!(node_txn[3].input.len(), 1);
23662366
assert_eq!(node_txn[4].input.len(), 1);
2367-
check_spends!(node_txn[2], revoked_local_txn[0]);
2368-
check_spends!(node_txn[3], revoked_local_txn[0]);
2369-
check_spends!(node_txn[4], revoked_local_txn[0]);
2367+
2368+
fn get_txout(out_point: &BitcoinOutPoint, tx: &Transaction) -> Option<TxOut> {
2369+
if out_point.txid == tx.txid() {
2370+
tx.output.get(out_point.vout as usize).cloned()
2371+
} else {
2372+
None
2373+
}
2374+
}
2375+
node_txn[1].verify(|out|get_txout(out, &revoked_local_txn[0])).unwrap();
2376+
node_txn[2].verify(|out|get_txout(out, &revoked_local_txn[0])).unwrap();
2377+
node_txn[4].verify(|out|get_txout(out, &revoked_local_txn[0])).unwrap();
23702378

23712379
let mut witness_lens = BTreeSet::new();
2380+
witness_lens.insert(node_txn[1].input[0].witness.last().unwrap().len());
23722381
witness_lens.insert(node_txn[2].input[0].witness.last().unwrap().len());
2373-
witness_lens.insert(node_txn[3].input[0].witness.last().unwrap().len());
23742382
witness_lens.insert(node_txn[4].input[0].witness.last().unwrap().len());
23752383
assert_eq!(witness_lens.len(), 3);
23762384
assert_eq!(*witness_lens.iter().skip(0).next().unwrap(), 77); // revoked to_local
@@ -2622,18 +2630,19 @@ fn test_htlc_on_chain_timeout() {
26222630
{
26232631
let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
26242632
assert_eq!(node_txn.len(), 5); // ChannelManager : 2 (commitment tx, HTLC-Timeout tx), ChannelMonitor : (local commitment tx + HTLC-timeout), timeout tx
2625-
assert_eq!(node_txn[0], node_txn[3]);
2626-
assert_eq!(node_txn[1], node_txn[4]);
26272633

2628-
check_spends!(node_txn[2], commitment_tx[0]);
2629-
assert_eq!(node_txn[2].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
2634+
assert_eq!(node_txn[2], node_txn[3]);
2635+
assert_eq!(node_txn[0], node_txn[4]);
26302636

2631-
check_spends!(node_txn[0], chan_2.3);
2632-
check_spends!(node_txn[1], node_txn[0]);
2633-
assert_eq!(node_txn[0].clone().input[0].witness.last().unwrap().len(), 71);
2634-
assert_eq!(node_txn[1].clone().input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
2637+
check_spends!(node_txn[1], commitment_tx[0]);
2638+
assert_eq!(node_txn[1].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
2639+
2640+
check_spends!(node_txn[2], chan_2.3);
2641+
check_spends!(node_txn[0], node_txn[2]);
2642+
assert_eq!(node_txn[2].clone().input[0].witness.last().unwrap().len(), 71);
2643+
assert_eq!(node_txn[0].clone().input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
26352644

2636-
timeout_tx = node_txn[2].clone();
2645+
timeout_tx = node_txn[1].clone();
26372646
node_txn.clear();
26382647
}
26392648

@@ -7336,11 +7345,11 @@ fn test_set_outpoints_partial_claiming() {
73367345
let partial_claim_tx = {
73377346
let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
73387347
assert_eq!(node_txn.len(), 3);
7339-
check_spends!(node_txn[1], node_txn[0]);
7340-
check_spends!(node_txn[2], node_txn[0]);
7348+
check_spends!(node_txn[0], node_txn[2].clone());
7349+
check_spends!(node_txn[1], node_txn[2].clone());
7350+
assert_eq!(node_txn[0].input.len(), 1);
73417351
assert_eq!(node_txn[1].input.len(), 1);
7342-
assert_eq!(node_txn[2].input.len(), 1);
7343-
node_txn[1].clone()
7352+
node_txn[0].clone()
73447353
};
73457354

73467355
// Broadcast partial claim on node A, should regenerate a claiming tx with HTLC dropped

‎lightning/src/ln/onchaintx.rs

Lines changed: 94 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ enum OnchainEvent {
5252
pub struct ClaimTxBumpMaterial {
5353
// At every block tick, used to check if pending claiming tx is taking too
5454
// much time for confirmation and we need to bump it.
55-
height_timer: u32,
55+
height_timer: Option<u32>,
5656
// Tracked in case of reorg to wipe out now-superflous bump material
5757
feerate_previous: u64,
5858
// Soonest timelocks among set of outpoints claimed, used to compute
@@ -64,7 +64,7 @@ pub struct ClaimTxBumpMaterial {
6464

6565
impl Writeable for ClaimTxBumpMaterial {
6666
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
67-
writer.write_all(&byte_utils::be32_to_array(self.height_timer))?;
67+
self.height_timer.write(writer)?;
6868
writer.write_all(&byte_utils::be64_to_array(self.feerate_previous))?;
6969
writer.write_all(&byte_utils::be32_to_array(self.soonest_timelock))?;
7070
writer.write_all(&byte_utils::be64_to_array(self.per_input_material.len() as u64))?;
@@ -357,7 +357,7 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
357357

358358
/// Lightning security model (i.e being able to redeem/timeout HTLC or penalize coutnerparty onchain) lays on the assumption of claim transactions getting confirmed before timelock expiration
359359
/// (CSV or CLTV following cases). In case of high-fee spikes, claim tx may stuck in the mempool, so you need to bump its feerate quickly using Replace-By-Fee or Child-Pay-For-Parent.
360-
fn generate_claim_tx<F: Deref>(&self, height: u32, cached_claim_datas: &ClaimTxBumpMaterial, fee_estimator: F) -> Option<(u32, u64, Transaction)>
360+
fn generate_claim_tx<F: Deref>(&self, height: u32, cached_claim_datas: &ClaimTxBumpMaterial, fee_estimator: F) -> Option<(Option<u32>, u64, Transaction)>
361361
where F::Target: FeeEstimator
362362
{
363363
if cached_claim_datas.per_input_material.len() == 0 { return None } // But don't prune pending claiming request yet, we may have to resurrect HTLCs
@@ -422,9 +422,10 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
422422

423423
// Compute new height timer to decide when we need to regenerate a new bumped version of the claim tx (if we
424424
// didn't receive confirmation of it before, or not enough reorg-safe depth on top of it).
425-
let new_timer = Self::get_height_timer(height, cached_claim_datas.soonest_timelock);
425+
let new_timer = Some(Self::get_height_timer(height, cached_claim_datas.soonest_timelock));
426426
let mut inputs_witnesses_weight = 0;
427427
let mut amt = 0;
428+
let mut dynamic_fee = true;
428429
for per_outp_material in cached_claim_datas.per_input_material.values() {
429430
match per_outp_material {
430431
&InputMaterial::Revoked { ref witness_script, ref is_htlc, ref amount, .. } => {
@@ -436,71 +437,99 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
436437
amt += *amount;
437438
},
438439
&InputMaterial::LocalHTLC { .. } => { return None; }
439-
}
440-
}
441-
442-
let predicted_weight = bumped_tx.get_weight() + inputs_witnesses_weight;
443-
let mut new_feerate;
444-
// If old feerate is 0, first iteration of this claim, use normal fee calculation
445-
if cached_claim_datas.feerate_previous != 0 {
446-
if let Some((new_fee, feerate)) = RBF_bump!(amt, cached_claim_datas.feerate_previous, fee_estimator, predicted_weight as u64) {
447-
// If new computed fee is superior at the whole claimable amount burn all in fees
448-
if new_fee > amt {
449-
bumped_tx.output[0].value = 0;
450-
} else {
451-
bumped_tx.output[0].value = amt - new_fee;
440+
&InputMaterial::Funding { .. } => {
441+
dynamic_fee = false;
452442
}
453-
new_feerate = feerate;
454-
} else { return None; }
455-
} else {
456-
if subtract_high_prio_fee!(self, fee_estimator, amt, predicted_weight, new_feerate) {
457-
bumped_tx.output[0].value = amt;
458-
} else { return None; }
443+
}
459444
}
460-
assert!(new_feerate != 0);
461445

462-
for (i, (outp, per_outp_material)) in cached_claim_datas.per_input_material.iter().enumerate() {
463-
match per_outp_material {
464-
&InputMaterial::Revoked { ref witness_script, ref pubkey, ref key, ref is_htlc, ref amount } => {
465-
let sighash_parts = bip143::SighashComponents::new(&bumped_tx);
466-
let sighash = hash_to_message!(&sighash_parts.sighash_all(&bumped_tx.input[i], &witness_script, *amount)[..]);
467-
let sig = self.secp_ctx.sign(&sighash, &key);
468-
bumped_tx.input[i].witness.push(sig.serialize_der().to_vec());
469-
bumped_tx.input[i].witness[0].push(SigHashType::All as u8);
470-
if *is_htlc {
471-
bumped_tx.input[i].witness.push(pubkey.unwrap().clone().serialize().to_vec());
446+
if dynamic_fee {
447+
let predicted_weight = bumped_tx.get_weight() + inputs_witnesses_weight;
448+
let mut new_feerate;
449+
// If old feerate is 0, first iteration of this claim, use normal fee calculation
450+
if cached_claim_datas.feerate_previous != 0 {
451+
if let Some((new_fee, feerate)) = RBF_bump!(amt, cached_claim_datas.feerate_previous, fee_estimator, predicted_weight as u64) {
452+
// If new computed fee is superior at the whole claimable amount burn all in fees
453+
if new_fee > amt {
454+
bumped_tx.output[0].value = 0;
472455
} else {
473-
bumped_tx.input[i].witness.push(vec!(1));
456+
bumped_tx.output[0].value = amt - new_fee;
474457
}
475-
bumped_tx.input[i].witness.push(witness_script.clone().into_bytes());
476-
log_trace!(self, "Going to broadcast Penalty Transaction {} claiming revoked {} output {} from {} with new feerate {}...", bumped_tx.txid(), if !is_htlc { "to_local" } else if HTLCType::scriptlen_to_htlctype(witness_script.len()) == Some(HTLCType::OfferedHTLC) { "offered" } else if HTLCType::scriptlen_to_htlctype(witness_script.len()) == Some(HTLCType::AcceptedHTLC) { "received" } else { "" }, outp.vout, outp.txid, new_feerate);
477-
},
478-
&InputMaterial::RemoteHTLC { ref witness_script, ref key, ref preimage, ref amount, ref locktime } => {
479-
if !preimage.is_some() { bumped_tx.lock_time = *locktime }; // Right now we don't aggregate time-locked transaction, if we do we should set lock_time before to avoid breaking hash computation
480-
let sighash_parts = bip143::SighashComponents::new(&bumped_tx);
481-
let sighash = hash_to_message!(&sighash_parts.sighash_all(&bumped_tx.input[i], &witness_script, *amount)[..]);
482-
let sig = self.secp_ctx.sign(&sighash, &key);
483-
bumped_tx.input[i].witness.push(sig.serialize_der().to_vec());
484-
bumped_tx.input[i].witness[0].push(SigHashType::All as u8);
485-
if let &Some(preimage) = preimage {
486-
bumped_tx.input[i].witness.push(preimage.clone().0.to_vec());
487-
} else {
488-
bumped_tx.input[i].witness.push(vec![0]);
458+
new_feerate = feerate;
459+
} else { return None; }
460+
} else {
461+
if subtract_high_prio_fee!(self, fee_estimator, amt, predicted_weight, new_feerate) {
462+
bumped_tx.output[0].value = amt;
463+
} else { return None; }
464+
}
465+
assert!(new_feerate != 0);
466+
467+
for (i, (outp, per_outp_material)) in cached_claim_datas.per_input_material.iter().enumerate() {
468+
match per_outp_material {
469+
&InputMaterial::Revoked { ref witness_script, ref pubkey, ref key, ref is_htlc, ref amount } => {
470+
let sighash_parts = bip143::SighashComponents::new(&bumped_tx);
471+
let sighash = hash_to_message!(&sighash_parts.sighash_all(&bumped_tx.input[i], &witness_script, *amount)[..]);
472+
let sig = self.secp_ctx.sign(&sighash, &key);
473+
bumped_tx.input[i].witness.push(sig.serialize_der().to_vec());
474+
bumped_tx.input[i].witness[0].push(SigHashType::All as u8);
475+
if *is_htlc {
476+
bumped_tx.input[i].witness.push(pubkey.unwrap().clone().serialize().to_vec());
477+
} else {
478+
bumped_tx.input[i].witness.push(vec!(1));
479+
}
480+
bumped_tx.input[i].witness.push(witness_script.clone().into_bytes());
481+
log_trace!(self, "Going to broadcast Penalty Transaction {} claiming revoked {} output {} from {} with new feerate {}...", bumped_tx.txid(), if !is_htlc { "to_local" } else if HTLCType::scriptlen_to_htlctype(witness_script.len()) == Some(HTLCType::OfferedHTLC) { "offered" } else if HTLCType::scriptlen_to_htlctype(witness_script.len()) == Some(HTLCType::AcceptedHTLC) { "received" } else { "" }, outp.vout, outp.txid, new_feerate);
482+
},
483+
&InputMaterial::RemoteHTLC { ref witness_script, ref key, ref preimage, ref amount, ref locktime } => {
484+
if !preimage.is_some() { bumped_tx.lock_time = *locktime }; // Right now we don't aggregate time-locked transaction, if we do we should set lock_time before to avoid breaking hash computation
485+
let sighash_parts = bip143::SighashComponents::new(&bumped_tx);
486+
let sighash = hash_to_message!(&sighash_parts.sighash_all(&bumped_tx.input[i], &witness_script, *amount)[..]);
487+
let sig = self.secp_ctx.sign(&sighash, &key);
488+
bumped_tx.input[i].witness.push(sig.serialize_der().to_vec());
489+
bumped_tx.input[i].witness[0].push(SigHashType::All as u8);
490+
if let &Some(preimage) = preimage {
491+
bumped_tx.input[i].witness.push(preimage.clone().0.to_vec());
492+
} else {
493+
bumped_tx.input[i].witness.push(vec![0]);
494+
}
495+
bumped_tx.input[i].witness.push(witness_script.clone().into_bytes());
496+
log_trace!(self, "Going to broadcast Claim Transaction {} claiming remote {} htlc output {} from {} with new feerate {}...", bumped_tx.txid(), if preimage.is_some() { "offered" } else { "received" }, outp.vout, outp.txid, new_feerate);
497+
},
498+
_ => unreachable!()
499+
}
500+
}
501+
log_trace!(self, "...with timer {}", new_timer.unwrap());
502+
assert!(predicted_weight >= bumped_tx.get_weight());
503+
return Some((new_timer, new_feerate, bumped_tx))
504+
} else {
505+
for (_, (outp, per_outp_material)) in cached_claim_datas.per_input_material.iter().enumerate() {
506+
match per_outp_material {
507+
&InputMaterial::LocalHTLC { .. } => {
508+
//TODO : Given that Local Commitment Transaction and HTLC-Timeout/HTLC-Success are counter-signed by peer, we can't
509+
// RBF them. Need a Lightning specs change and package relay modification :
510+
// https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
511+
return None;
512+
},
513+
&InputMaterial::Funding { ref channel_value } => {
514+
if self.local_commitment.is_some() {
515+
let mut local_commitment = self.local_commitment.clone().unwrap();
516+
self.key_storage.sign_local_commitment(&mut local_commitment, &self.funding_redeemscript, *channel_value, &self.secp_ctx);
517+
let signed_tx = local_commitment.with_valid_witness().clone();
518+
let mut amt_outputs = 0;
519+
for outp in signed_tx.output.iter() {
520+
amt_outputs += outp.value;
521+
}
522+
let feerate = (channel_value - amt_outputs) * 1000 / signed_tx.get_weight() as u64;
523+
// Timer set to $NEVER given we can't bump tx without anchor outputs
524+
log_trace!(self, "Going to broadcast Local Transaction {} claiming funding output {} from {}...", signed_tx.txid(), outp.vout, outp.txid);
525+
return Some((None, feerate, signed_tx));
526+
}
489527
}
490-
bumped_tx.input[i].witness.push(witness_script.clone().into_bytes());
491-
log_trace!(self, "Going to broadcast Claim Transaction {} claiming remote {} htlc output {} from {} with new feerate {}...", bumped_tx.txid(), if preimage.is_some() { "offered" } else { "received" }, outp.vout, outp.txid, new_feerate);
492-
},
493-
&InputMaterial::LocalHTLC { .. } => {
494-
//TODO : Given that Local Commitment Transaction and HTLC-Timeout/HTLC-Success are counter-signed by peer, we can't
495-
// RBF them. Need a Lightning specs change and package relay modification :
496-
// https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
497-
return None;
528+
_ => unreachable!()
498529
}
499530
}
500531
}
501-
log_trace!(self, "...with timer {}", new_timer);
502-
assert!(predicted_weight >= bumped_tx.get_weight());
503-
Some((new_timer, new_feerate, bumped_tx))
532+
None
504533
}
505534

506535
pub(super) fn block_connected<B: Deref, F: Deref>(&mut self, txn_matched: &[&Transaction], claimable_outpoints: Vec<ClaimRequest>, height: u32, broadcaster: B, fee_estimator: F)
@@ -535,7 +564,7 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
535564
// Generate claim transactions and track them to bump if necessary at
536565
// height timer expiration (i.e in how many blocks we're going to take action).
537566
for claim in new_claims {
538-
let mut claim_material = ClaimTxBumpMaterial { height_timer: 0, feerate_previous: 0, soonest_timelock: claim.0, per_input_material: claim.1.clone() };
567+
let mut claim_material = ClaimTxBumpMaterial { height_timer: None, feerate_previous: 0, soonest_timelock: claim.0, per_input_material: claim.1.clone() };
539568
if let Some((new_timer, new_feerate, tx)) = self.generate_claim_tx(height, &claim_material, &*fee_estimator) {
540569
claim_material.height_timer = new_timer;
541570
claim_material.feerate_previous = new_feerate;
@@ -653,8 +682,10 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
653682

654683
// Check if any pending claim request must be rescheduled
655684
for (first_claim_txid, ref claim_data) in self.pending_claim_requests.iter() {
656-
if claim_data.height_timer == height {
657-
bump_candidates.insert(*first_claim_txid);
685+
if let Some(h) = claim_data.height_timer {
686+
if h == height {
687+
bump_candidates.insert(*first_claim_txid);
688+
}
658689
}
659690
}
660691

0 commit comments

Comments
 (0)