Few more ChannelMonitor Cleanups

fuzz/src/chanmon_deser.rs

@@ -32,8 +32,6 @@ pub fn do_test(data: &[u8]) {
  let deserialized_copy = <(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(&w.0), logger.clone()).unwrap();
  assert!(latest_block_hash == deserialized_copy.0);
  assert!(monitor == deserialized_copy.1);
- w.0.clear();
- monitor.write_for_watchtower(&mut w).unwrap();
 	}
 }
 

lightning/src/chain/keysinterface.rs

@@ -222,15 +222,15 @@ pub trait ChannelKeys : Send+Clone {
 	/// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
 	/// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method
 	/// making the callee generate it via some util function we expose)!
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Create a signature for a local commitment transaction without enforcing one-time signing.
 	///
 	/// Testing revocation logic by our test framework needs to sign multiple local commitment
 	/// transactions. This unsafe test-only version doesn't enforce one-time signing security
 	/// requirement.
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Signs a transaction created by build_htlc_transaction. If the transaction is an
 	/// HTLC-Success transaction, preimage must be set!
@@ -363,13 +363,21 @@ impl ChannelKeys for InMemoryChannelKeys {
  Ok((commitment_sig, htlc_sigs))
 	}
 
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {

lightning/src/ln/chan_utils.rs

@@ -485,9 +485,8 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s
 /// just pass in the SecretKeys required.
 pub struct LocalCommitmentTransaction {
 	tx: Transaction,
-	//TODO: modify Channel methods to integrate HTLC material at LocalCommitmentTransaction generation to drop Option here
-	local_keys: Option<TxCreationKeys>,
-	feerate_per_kw: Option<u64>,
+	pub(crate) local_keys: TxCreationKeys,
+	pub(crate) feerate_per_kw: u64,
 	per_htlc: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>
 }
 impl LocalCommitmentTransaction {
@@ -502,21 +501,30 @@ impl LocalCommitmentTransaction {
  sequence: 0,
  witness: vec![vec![], vec![], vec![]]
  };
- Self { tx: Transaction {
- version: 2,
- input: vec![dummy_input],
- output: Vec::new(),
- lock_time: 0,
- },
- local_keys: None,
- feerate_per_kw: None,
+ let dummy_key = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&[42; 32]).unwrap());
+ Self {
+ tx: Transaction {
+ version: 2,
+ input: vec![dummy_input],
+ output: Vec::new(),
+ lock_time: 0,
+ },
+ local_keys: TxCreationKeys {
+ per_commitment_point: dummy_key.clone(),
+ revocation_key: dummy_key.clone(),
+ a_htlc_key: dummy_key.clone(),
+ b_htlc_key: dummy_key.clone(),
+ a_delayed_payment_key: dummy_key.clone(),
+ b_payment_key: dummy_key.clone(),
+ },
+ feerate_per_kw: 0,
  per_htlc: Vec::new()
  }
 	}
 
 	/// Generate a new LocalCommitmentTransaction based on a raw commitment transaction,
 	/// remote signature and both parties keys
-	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction {
+	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, mut htlc_data: Vec<(HTLCOutputInCommitment, Option<Signature>)>) -> LocalCommitmentTransaction {
  if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); }
  if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); }
 
@@ -533,9 +541,10 @@ impl LocalCommitmentTransaction {
  }
 
  Self { tx,
- local_keys: None,
- feerate_per_kw: None,
- per_htlc: Vec::new()
+ local_keys,
+ feerate_per_kw,
+ // TODO: Avoid the conversion of a Vec created likely just for this:
+ per_htlc: htlc_data.drain(..).map(|(a, b)| (a, b, None)).collect(),
  }
 	}
 
@@ -594,33 +603,23 @@ impl LocalCommitmentTransaction {
  &self.tx
 	}
 
-	/// Set HTLC cache to generate any local HTLC transaction spending one of htlc ouput
-	/// from this local commitment transaction
-	pub(crate) fn set_htlc_cache(&mut self, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>) {
- self.local_keys = Some(local_keys);
- self.feerate_per_kw = Some(feerate_per_kw);
- self.per_htlc = htlc_outputs;
-	}
-
 	/// Add local signature for a htlc transaction, do nothing if a cached signed transaction is
 	/// already present
 	pub fn add_htlc_sig<T: secp256k1::Signing>(&mut self, htlc_base_key: &SecretKey, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
- if self.local_keys.is_none() || self.feerate_per_kw.is_none() { return; }
- let local_keys = self.local_keys.as_ref().unwrap();
  let txid = self.txid();
  for this_htlc in self.per_htlc.iter_mut() {
- if this_htlc.0.transaction_output_index.unwrap() == htlc_index {
+ if this_htlc.0.transaction_output_index == Some(htlc_index) {
  if this_htlc.2.is_some() { return; } // we already have a cached htlc transaction at provided index
- let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw.unwrap(), local_csv, &this_htlc.0, &local_keys.a_delayed_payment_key, &local_keys.revocation_key);
+ let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key);
  if !this_htlc.0.offered && preimage.is_none() { return; } // if we don't have preimage for HTLC-Success, don't try to generate
  let htlc_secret = if !this_htlc.0.offered { preimage } else { None }; // if we have a preimage for HTLC-Timeout, don't use it that's likely a duplicate HTLC hash
  if this_htlc.1.is_none() { return; } // we don't have any remote signature for this htlc
  if htlc_tx.input.len() != 1 { return; }
  if htlc_tx.input[0].witness.len() != 0 { return; }
 
- let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &local_keys.a_htlc_key, &local_keys.b_htlc_key, &local_keys.revocation_key);
+ let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
 
- if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &local_keys.per_commitment_point, htlc_base_key) {
+ if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &self.local_keys.per_commitment_point, htlc_base_key) {
  let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]);
  let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);
 

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1629,12 +1629,9 @@ fn monitor_update_claim_fail_no_response() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
-// Note that restore_between_fails with !fail_on_generate is useless
-// Also note that !fail_on_generate && !fail_on_signed is useless
-// Finally, note that !fail_on_signed is not possible with fail_on_generate && !restore_between_fails
 // confirm_a_first and restore_b_before_conf are wholly unrelated to earlier bools and
 // restore_b_before_conf has no meaning if !confirm_a_first
-fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails: bool, fail_on_signed: bool, confirm_a_first: bool, restore_b_before_conf: bool) {
+fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: bool) {
 	// Test that if the monitor update generated by funding_transaction_generated fails we continue
 	// the channel setup happily after the update is restored.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
@@ -1648,52 +1645,25 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 	let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43);
 
-	if fail_on_generate {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	}
 	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
-	check_added_monitors!(nodes[0], 1);
+	check_added_monitors!(nodes[0], 0);
 
 	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
 	let channel_id = OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
 	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
 	check_added_monitors!(nodes[1], 1);
 
-	if restore_between_fails {
- assert!(fail_on_generate);
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-	}
-
-	if fail_on_signed {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	} else {
- assert!(restore_between_fails || !fail_on_generate); // We can't switch to good now (there's no monitor update)
- assert!(fail_on_generate); // Somebody has to fail
-	}
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
-	if fail_on_signed || !restore_between_fails {
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
- if fail_on_generate && !restore_between_fails {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented funding_signed from allowing funding broadcast".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- } else {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- }
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
-	} else {
- check_added_monitors!(nodes[0], 1);
-	}
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+	nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
+	check_added_monitors!(nodes[0], 1);
+	assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
+	let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
+	nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
+	check_added_monitors!(nodes[0], 0);
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
@@ -1757,8 +1727,7 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 #[test]
 fn during_funding_monitor_fail() {
-	do_during_funding_monitor_fail(false, false, true, true, true);
-	do_during_funding_monitor_fail(true, false, true, false, false);
-	do_during_funding_monitor_fail(true, true, true, true, false);
-	do_during_funding_monitor_fail(true, true, false, false, false);
+	do_during_funding_monitor_fail(true, true);
+	do_during_funding_monitor_fail(true, false);
+	do_during_funding_monitor_fail(false, false);
 }

fuzz/src/chanmon_deser.rs

@@ -32,8 +32,6 @@ pub fn do_test(data: &[u8]) {
  let deserialized_copy = <(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(&w.0), logger.clone()).unwrap();
  assert!(latest_block_hash == deserialized_copy.0);
  assert!(monitor == deserialized_copy.1);
- w.0.clear();
- monitor.write_for_watchtower(&mut w).unwrap();
 	}
 }
 

lightning/src/chain/keysinterface.rs

@@ -222,15 +222,15 @@ pub trait ChannelKeys : Send+Clone {
 	/// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
 	/// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method
 	/// making the callee generate it via some util function we expose)!
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Create a signature for a local commitment transaction without enforcing one-time signing.
 	///
 	/// Testing revocation logic by our test framework needs to sign multiple local commitment
 	/// transactions. This unsafe test-only version doesn't enforce one-time signing security
 	/// requirement.
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Signs a transaction created by build_htlc_transaction. If the transaction is an
 	/// HTLC-Success transaction, preimage must be set!
@@ -363,13 +363,21 @@ impl ChannelKeys for InMemoryChannelKeys {
  Ok((commitment_sig, htlc_sigs))
 	}
 
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {

lightning/src/ln/chan_utils.rs

@@ -485,9 +485,8 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s
 /// just pass in the SecretKeys required.
 pub struct LocalCommitmentTransaction {
 	tx: Transaction,
-	//TODO: modify Channel methods to integrate HTLC material at LocalCommitmentTransaction generation to drop Option here
-	local_keys: Option<TxCreationKeys>,
-	feerate_per_kw: Option<u64>,
+	pub(crate) local_keys: TxCreationKeys,
+	pub(crate) feerate_per_kw: u64,
 	per_htlc: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>
 }
 impl LocalCommitmentTransaction {
@@ -502,21 +501,30 @@ impl LocalCommitmentTransaction {
  sequence: 0,
  witness: vec![vec![], vec![], vec![]]
  };
- Self { tx: Transaction {
- version: 2,
- input: vec![dummy_input],
- output: Vec::new(),
- lock_time: 0,
- },
- local_keys: None,
- feerate_per_kw: None,
+ let dummy_key = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&[42; 32]).unwrap());
+ Self {
+ tx: Transaction {
+ version: 2,
+ input: vec![dummy_input],
+ output: Vec::new(),
+ lock_time: 0,
+ },
+ local_keys: TxCreationKeys {
+ per_commitment_point: dummy_key.clone(),
+ revocation_key: dummy_key.clone(),
+ a_htlc_key: dummy_key.clone(),
+ b_htlc_key: dummy_key.clone(),
+ a_delayed_payment_key: dummy_key.clone(),
+ b_payment_key: dummy_key.clone(),
+ },
+ feerate_per_kw: 0,
  per_htlc: Vec::new()
  }
 	}
 
 	/// Generate a new LocalCommitmentTransaction based on a raw commitment transaction,
 	/// remote signature and both parties keys
-	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction {
+	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, mut htlc_data: Vec<(HTLCOutputInCommitment, Option<Signature>)>) -> LocalCommitmentTransaction {
  if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); }
  if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); }
 
@@ -533,9 +541,10 @@ impl LocalCommitmentTransaction {
  }
 
  Self { tx,
- local_keys: None,
- feerate_per_kw: None,
- per_htlc: Vec::new()
+ local_keys,
+ feerate_per_kw,
+ // TODO: Avoid the conversion of a Vec created likely just for this:
+ per_htlc: htlc_data.drain(..).map(|(a, b)| (a, b, None)).collect(),
  }
 	}
 
@@ -594,33 +603,23 @@ impl LocalCommitmentTransaction {
  &self.tx
 	}
 
-	/// Set HTLC cache to generate any local HTLC transaction spending one of htlc ouput
-	/// from this local commitment transaction
-	pub(crate) fn set_htlc_cache(&mut self, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>) {
- self.local_keys = Some(local_keys);
- self.feerate_per_kw = Some(feerate_per_kw);
- self.per_htlc = htlc_outputs;
-	}
-
 	/// Add local signature for a htlc transaction, do nothing if a cached signed transaction is
 	/// already present
 	pub fn add_htlc_sig<T: secp256k1::Signing>(&mut self, htlc_base_key: &SecretKey, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
- if self.local_keys.is_none() || self.feerate_per_kw.is_none() { return; }
- let local_keys = self.local_keys.as_ref().unwrap();
  let txid = self.txid();
  for this_htlc in self.per_htlc.iter_mut() {
- if this_htlc.0.transaction_output_index.unwrap() == htlc_index {
+ if this_htlc.0.transaction_output_index == Some(htlc_index) {
  if this_htlc.2.is_some() { return; } // we already have a cached htlc transaction at provided index
- let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw.unwrap(), local_csv, &this_htlc.0, &local_keys.a_delayed_payment_key, &local_keys.revocation_key);
+ let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key);
  if !this_htlc.0.offered && preimage.is_none() { return; } // if we don't have preimage for HTLC-Success, don't try to generate
  let htlc_secret = if !this_htlc.0.offered { preimage } else { None }; // if we have a preimage for HTLC-Timeout, don't use it that's likely a duplicate HTLC hash
  if this_htlc.1.is_none() { return; } // we don't have any remote signature for this htlc
  if htlc_tx.input.len() != 1 { return; }
  if htlc_tx.input[0].witness.len() != 0 { return; }
 
- let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &local_keys.a_htlc_key, &local_keys.b_htlc_key, &local_keys.revocation_key);
+ let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
 
- if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &local_keys.per_commitment_point, htlc_base_key) {
+ if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &self.local_keys.per_commitment_point, htlc_base_key) {
  let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]);
  let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);
 

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1629,12 +1629,9 @@ fn monitor_update_claim_fail_no_response() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
-// Note that restore_between_fails with !fail_on_generate is useless
-// Also note that !fail_on_generate && !fail_on_signed is useless
-// Finally, note that !fail_on_signed is not possible with fail_on_generate && !restore_between_fails
 // confirm_a_first and restore_b_before_conf are wholly unrelated to earlier bools and
 // restore_b_before_conf has no meaning if !confirm_a_first
-fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails: bool, fail_on_signed: bool, confirm_a_first: bool, restore_b_before_conf: bool) {
+fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: bool) {
 	// Test that if the monitor update generated by funding_transaction_generated fails we continue
 	// the channel setup happily after the update is restored.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
@@ -1648,52 +1645,25 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 	let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43);
 
-	if fail_on_generate {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	}
 	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
-	check_added_monitors!(nodes[0], 1);
+	check_added_monitors!(nodes[0], 0);
 
 	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
 	let channel_id = OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
 	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
 	check_added_monitors!(nodes[1], 1);
 
-	if restore_between_fails {
- assert!(fail_on_generate);
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-	}
-
-	if fail_on_signed {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	} else {
- assert!(restore_between_fails || !fail_on_generate); // We can't switch to good now (there's no monitor update)
- assert!(fail_on_generate); // Somebody has to fail
-	}
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
-	if fail_on_signed || !restore_between_fails {
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
- if fail_on_generate && !restore_between_fails {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented funding_signed from allowing funding broadcast".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- } else {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- }
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
-	} else {
- check_added_monitors!(nodes[0], 1);
-	}
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+	nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
+	check_added_monitors!(nodes[0], 1);
+	assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
+	let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
+	nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
+	check_added_monitors!(nodes[0], 0);
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
@@ -1757,8 +1727,7 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 #[test]
 fn during_funding_monitor_fail() {
-	do_during_funding_monitor_fail(false, false, true, true, true);
-	do_during_funding_monitor_fail(true, false, true, false, false);
-	do_during_funding_monitor_fail(true, true, true, true, false);
-	do_during_funding_monitor_fail(true, true, false, false, false);
+	do_during_funding_monitor_fail(true, true);
+	do_during_funding_monitor_fail(true, false);
+	do_during_funding_monitor_fail(false, false);
 }

fuzz/src/chanmon_deser.rs

@@ -32,8 +32,6 @@ pub fn do_test(data: &[u8]) {
  let deserialized_copy = <(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(&w.0), logger.clone()).unwrap();
  assert!(latest_block_hash == deserialized_copy.0);
  assert!(monitor == deserialized_copy.1);
- w.0.clear();
- monitor.write_for_watchtower(&mut w).unwrap();
 	}
 }
 

lightning/src/chain/keysinterface.rs

@@ -222,15 +222,15 @@ pub trait ChannelKeys : Send+Clone {
 	/// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
 	/// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method
 	/// making the callee generate it via some util function we expose)!
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Create a signature for a local commitment transaction without enforcing one-time signing.
 	///
 	/// Testing revocation logic by our test framework needs to sign multiple local commitment
 	/// transactions. This unsafe test-only version doesn't enforce one-time signing security
 	/// requirement.
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Signs a transaction created by build_htlc_transaction. If the transaction is an
 	/// HTLC-Success transaction, preimage must be set!
@@ -363,13 +363,21 @@ impl ChannelKeys for InMemoryChannelKeys {
  Ok((commitment_sig, htlc_sigs))
 	}
 
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {

lightning/src/ln/chan_utils.rs

@@ -485,9 +485,8 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s
 /// just pass in the SecretKeys required.
 pub struct LocalCommitmentTransaction {
 	tx: Transaction,
-	//TODO: modify Channel methods to integrate HTLC material at LocalCommitmentTransaction generation to drop Option here
-	local_keys: Option<TxCreationKeys>,
-	feerate_per_kw: Option<u64>,
+	pub(crate) local_keys: TxCreationKeys,
+	pub(crate) feerate_per_kw: u64,
 	per_htlc: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>
 }
 impl LocalCommitmentTransaction {
@@ -502,21 +501,30 @@ impl LocalCommitmentTransaction {
  sequence: 0,
  witness: vec![vec![], vec![], vec![]]
  };
- Self { tx: Transaction {
- version: 2,
- input: vec![dummy_input],
- output: Vec::new(),
- lock_time: 0,
- },
- local_keys: None,
- feerate_per_kw: None,
+ let dummy_key = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&[42; 32]).unwrap());
+ Self {
+ tx: Transaction {
+ version: 2,
+ input: vec![dummy_input],
+ output: Vec::new(),
+ lock_time: 0,
+ },
+ local_keys: TxCreationKeys {
+ per_commitment_point: dummy_key.clone(),
+ revocation_key: dummy_key.clone(),
+ a_htlc_key: dummy_key.clone(),
+ b_htlc_key: dummy_key.clone(),
+ a_delayed_payment_key: dummy_key.clone(),
+ b_payment_key: dummy_key.clone(),
+ },
+ feerate_per_kw: 0,
  per_htlc: Vec::new()
  }
 	}
 
 	/// Generate a new LocalCommitmentTransaction based on a raw commitment transaction,
 	/// remote signature and both parties keys
-	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction {
+	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, mut htlc_data: Vec<(HTLCOutputInCommitment, Option<Signature>)>) -> LocalCommitmentTransaction {
  if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); }
  if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); }
 
@@ -533,9 +541,10 @@ impl LocalCommitmentTransaction {
  }
 
  Self { tx,
- local_keys: None,
- feerate_per_kw: None,
- per_htlc: Vec::new()
+ local_keys,
+ feerate_per_kw,
+ // TODO: Avoid the conversion of a Vec created likely just for this:
+ per_htlc: htlc_data.drain(..).map(|(a, b)| (a, b, None)).collect(),
  }
 	}
 
@@ -594,33 +603,23 @@ impl LocalCommitmentTransaction {
  &self.tx
 	}
 
-	/// Set HTLC cache to generate any local HTLC transaction spending one of htlc ouput
-	/// from this local commitment transaction
-	pub(crate) fn set_htlc_cache(&mut self, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>) {
- self.local_keys = Some(local_keys);
- self.feerate_per_kw = Some(feerate_per_kw);
- self.per_htlc = htlc_outputs;
-	}
-
 	/// Add local signature for a htlc transaction, do nothing if a cached signed transaction is
 	/// already present
 	pub fn add_htlc_sig<T: secp256k1::Signing>(&mut self, htlc_base_key: &SecretKey, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
- if self.local_keys.is_none() || self.feerate_per_kw.is_none() { return; }
- let local_keys = self.local_keys.as_ref().unwrap();
  let txid = self.txid();
  for this_htlc in self.per_htlc.iter_mut() {
- if this_htlc.0.transaction_output_index.unwrap() == htlc_index {
+ if this_htlc.0.transaction_output_index == Some(htlc_index) {
  if this_htlc.2.is_some() { return; } // we already have a cached htlc transaction at provided index
- let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw.unwrap(), local_csv, &this_htlc.0, &local_keys.a_delayed_payment_key, &local_keys.revocation_key);
+ let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key);
  if !this_htlc.0.offered && preimage.is_none() { return; } // if we don't have preimage for HTLC-Success, don't try to generate
  let htlc_secret = if !this_htlc.0.offered { preimage } else { None }; // if we have a preimage for HTLC-Timeout, don't use it that's likely a duplicate HTLC hash
  if this_htlc.1.is_none() { return; } // we don't have any remote signature for this htlc
  if htlc_tx.input.len() != 1 { return; }
  if htlc_tx.input[0].witness.len() != 0 { return; }
 
- let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &local_keys.a_htlc_key, &local_keys.b_htlc_key, &local_keys.revocation_key);
+ let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
 
- if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &local_keys.per_commitment_point, htlc_base_key) {
+ if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &self.local_keys.per_commitment_point, htlc_base_key) {
  let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]);
  let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);
 

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1629,12 +1629,9 @@ fn monitor_update_claim_fail_no_response() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
-// Note that restore_between_fails with !fail_on_generate is useless
-// Also note that !fail_on_generate && !fail_on_signed is useless
-// Finally, note that !fail_on_signed is not possible with fail_on_generate && !restore_between_fails
 // confirm_a_first and restore_b_before_conf are wholly unrelated to earlier bools and
 // restore_b_before_conf has no meaning if !confirm_a_first
-fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails: bool, fail_on_signed: bool, confirm_a_first: bool, restore_b_before_conf: bool) {
+fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: bool) {
 	// Test that if the monitor update generated by funding_transaction_generated fails we continue
 	// the channel setup happily after the update is restored.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
@@ -1648,52 +1645,25 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 	let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43);
 
-	if fail_on_generate {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	}
 	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
-	check_added_monitors!(nodes[0], 1);
+	check_added_monitors!(nodes[0], 0);
 
 	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
 	let channel_id = OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
 	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
 	check_added_monitors!(nodes[1], 1);
 
-	if restore_between_fails {
- assert!(fail_on_generate);
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-	}
-
-	if fail_on_signed {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	} else {
- assert!(restore_between_fails || !fail_on_generate); // We can't switch to good now (there's no monitor update)
- assert!(fail_on_generate); // Somebody has to fail
-	}
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
-	if fail_on_signed || !restore_between_fails {
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
- if fail_on_generate && !restore_between_fails {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented funding_signed from allowing funding broadcast".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- } else {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- }
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
-	} else {
- check_added_monitors!(nodes[0], 1);
-	}
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+	nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
+	check_added_monitors!(nodes[0], 1);
+	assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
+	let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
+	nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
+	check_added_monitors!(nodes[0], 0);
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
@@ -1757,8 +1727,7 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 #[test]
 fn during_funding_monitor_fail() {
-	do_during_funding_monitor_fail(false, false, true, true, true);
-	do_during_funding_monitor_fail(true, false, true, false, false);
-	do_during_funding_monitor_fail(true, true, true, true, false);
-	do_during_funding_monitor_fail(true, true, false, false, false);
+	do_during_funding_monitor_fail(true, true);
+	do_during_funding_monitor_fail(true, false);
+	do_during_funding_monitor_fail(false, false);
 }

fuzz/src/chanmon_deser.rs

@@ -32,8 +32,6 @@ pub fn do_test(data: &[u8]) {
  let deserialized_copy = <(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(&w.0), logger.clone()).unwrap();
  assert!(latest_block_hash == deserialized_copy.0);
  assert!(monitor == deserialized_copy.1);
- w.0.clear();
- monitor.write_for_watchtower(&mut w).unwrap();
 	}
 }
 

lightning/src/chain/keysinterface.rs

@@ -222,15 +222,15 @@ pub trait ChannelKeys : Send+Clone {
 	/// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
 	/// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method
 	/// making the callee generate it via some util function we expose)!
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Create a signature for a local commitment transaction without enforcing one-time signing.
 	///
 	/// Testing revocation logic by our test framework needs to sign multiple local commitment
 	/// transactions. This unsafe test-only version doesn't enforce one-time signing security
 	/// requirement.
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Signs a transaction created by build_htlc_transaction. If the transaction is an
 	/// HTLC-Success transaction, preimage must be set!
@@ -363,13 +363,21 @@ impl ChannelKeys for InMemoryChannelKeys {
  Ok((commitment_sig, htlc_sigs))
 	}
 
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {

lightning/src/ln/chan_utils.rs

@@ -485,9 +485,8 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s
 /// just pass in the SecretKeys required.
 pub struct LocalCommitmentTransaction {
 	tx: Transaction,
-	//TODO: modify Channel methods to integrate HTLC material at LocalCommitmentTransaction generation to drop Option here
-	local_keys: Option<TxCreationKeys>,
-	feerate_per_kw: Option<u64>,
+	pub(crate) local_keys: TxCreationKeys,
+	pub(crate) feerate_per_kw: u64,
 	per_htlc: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>
 }
 impl LocalCommitmentTransaction {
@@ -502,21 +501,30 @@ impl LocalCommitmentTransaction {
  sequence: 0,
  witness: vec![vec![], vec![], vec![]]
  };
- Self { tx: Transaction {
- version: 2,
- input: vec![dummy_input],
- output: Vec::new(),
- lock_time: 0,
- },
- local_keys: None,
- feerate_per_kw: None,
+ let dummy_key = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&[42; 32]).unwrap());
+ Self {
+ tx: Transaction {
+ version: 2,
+ input: vec![dummy_input],
+ output: Vec::new(),
+ lock_time: 0,
+ },
+ local_keys: TxCreationKeys {
+ per_commitment_point: dummy_key.clone(),
+ revocation_key: dummy_key.clone(),
+ a_htlc_key: dummy_key.clone(),
+ b_htlc_key: dummy_key.clone(),
+ a_delayed_payment_key: dummy_key.clone(),
+ b_payment_key: dummy_key.clone(),
+ },
+ feerate_per_kw: 0,
  per_htlc: Vec::new()
  }
 	}
 
 	/// Generate a new LocalCommitmentTransaction based on a raw commitment transaction,
 	/// remote signature and both parties keys
-	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction {
+	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, mut htlc_data: Vec<(HTLCOutputInCommitment, Option<Signature>)>) -> LocalCommitmentTransaction {
  if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); }
  if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); }
 
@@ -533,9 +541,10 @@ impl LocalCommitmentTransaction {
  }
 
  Self { tx,
- local_keys: None,
- feerate_per_kw: None,
- per_htlc: Vec::new()
+ local_keys,
+ feerate_per_kw,
+ // TODO: Avoid the conversion of a Vec created likely just for this:
+ per_htlc: htlc_data.drain(..).map(|(a, b)| (a, b, None)).collect(),
  }
 	}
 
@@ -594,33 +603,23 @@ impl LocalCommitmentTransaction {
  &self.tx
 	}
 
-	/// Set HTLC cache to generate any local HTLC transaction spending one of htlc ouput
-	/// from this local commitment transaction
-	pub(crate) fn set_htlc_cache(&mut self, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>) {
- self.local_keys = Some(local_keys);
- self.feerate_per_kw = Some(feerate_per_kw);
- self.per_htlc = htlc_outputs;
-	}
-
 	/// Add local signature for a htlc transaction, do nothing if a cached signed transaction is
 	/// already present
 	pub fn add_htlc_sig<T: secp256k1::Signing>(&mut self, htlc_base_key: &SecretKey, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
- if self.local_keys.is_none() || self.feerate_per_kw.is_none() { return; }
- let local_keys = self.local_keys.as_ref().unwrap();
  let txid = self.txid();
  for this_htlc in self.per_htlc.iter_mut() {
- if this_htlc.0.transaction_output_index.unwrap() == htlc_index {
+ if this_htlc.0.transaction_output_index == Some(htlc_index) {
  if this_htlc.2.is_some() { return; } // we already have a cached htlc transaction at provided index
- let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw.unwrap(), local_csv, &this_htlc.0, &local_keys.a_delayed_payment_key, &local_keys.revocation_key);
+ let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key);
  if !this_htlc.0.offered && preimage.is_none() { return; } // if we don't have preimage for HTLC-Success, don't try to generate
  let htlc_secret = if !this_htlc.0.offered { preimage } else { None }; // if we have a preimage for HTLC-Timeout, don't use it that's likely a duplicate HTLC hash
  if this_htlc.1.is_none() { return; } // we don't have any remote signature for this htlc
  if htlc_tx.input.len() != 1 { return; }
  if htlc_tx.input[0].witness.len() != 0 { return; }
 
- let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &local_keys.a_htlc_key, &local_keys.b_htlc_key, &local_keys.revocation_key);
+ let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
 
- if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &local_keys.per_commitment_point, htlc_base_key) {
+ if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &self.local_keys.per_commitment_point, htlc_base_key) {
  let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]);
  let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);
 

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1629,12 +1629,9 @@ fn monitor_update_claim_fail_no_response() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
-// Note that restore_between_fails with !fail_on_generate is useless
-// Also note that !fail_on_generate && !fail_on_signed is useless
-// Finally, note that !fail_on_signed is not possible with fail_on_generate && !restore_between_fails
 // confirm_a_first and restore_b_before_conf are wholly unrelated to earlier bools and
 // restore_b_before_conf has no meaning if !confirm_a_first
-fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails: bool, fail_on_signed: bool, confirm_a_first: bool, restore_b_before_conf: bool) {
+fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: bool) {
 	// Test that if the monitor update generated by funding_transaction_generated fails we continue
 	// the channel setup happily after the update is restored.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
@@ -1648,52 +1645,25 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 	let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43);
 
-	if fail_on_generate {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	}
 	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
-	check_added_monitors!(nodes[0], 1);
+	check_added_monitors!(nodes[0], 0);
 
 	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
 	let channel_id = OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
 	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
 	check_added_monitors!(nodes[1], 1);
 
-	if restore_between_fails {
- assert!(fail_on_generate);
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-	}
-
-	if fail_on_signed {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	} else {
- assert!(restore_between_fails || !fail_on_generate); // We can't switch to good now (there's no monitor update)
- assert!(fail_on_generate); // Somebody has to fail
-	}
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
-	if fail_on_signed || !restore_between_fails {
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
- if fail_on_generate && !restore_between_fails {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented funding_signed from allowing funding broadcast".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- } else {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- }
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
-	} else {
- check_added_monitors!(nodes[0], 1);
-	}
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+	nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
+	check_added_monitors!(nodes[0], 1);
+	assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
+	let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
+	nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
+	check_added_monitors!(nodes[0], 0);
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
@@ -1757,8 +1727,7 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 #[test]
 fn during_funding_monitor_fail() {
-	do_during_funding_monitor_fail(false, false, true, true, true);
-	do_during_funding_monitor_fail(true, false, true, false, false);
-	do_during_funding_monitor_fail(true, true, true, true, false);
-	do_during_funding_monitor_fail(true, true, false, false, false);
+	do_during_funding_monitor_fail(true, true);
+	do_during_funding_monitor_fail(true, false);
+	do_during_funding_monitor_fail(false, false);
 }

fuzz/src/chanmon_deser.rs

@@ -32,8 +32,6 @@ pub fn do_test(data: &[u8]) {
  let deserialized_copy = <(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(&w.0), logger.clone()).unwrap();
  assert!(latest_block_hash == deserialized_copy.0);
  assert!(monitor == deserialized_copy.1);
- w.0.clear();
- monitor.write_for_watchtower(&mut w).unwrap();
 	}
 }
 

lightning/src/chain/keysinterface.rs

@@ -222,15 +222,15 @@ pub trait ChannelKeys : Send+Clone {
 	/// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
 	/// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method
 	/// making the callee generate it via some util function we expose)!
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Create a signature for a local commitment transaction without enforcing one-time signing.
 	///
 	/// Testing revocation logic by our test framework needs to sign multiple local commitment
 	/// transactions. This unsafe test-only version doesn't enforce one-time signing security
 	/// requirement.
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Signs a transaction created by build_htlc_transaction. If the transaction is an
 	/// HTLC-Success transaction, preimage must be set!
@@ -363,13 +363,21 @@ impl ChannelKeys for InMemoryChannelKeys {
  Ok((commitment_sig, htlc_sigs))
 	}
 
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {

lightning/src/ln/chan_utils.rs

@@ -485,9 +485,8 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s
 /// just pass in the SecretKeys required.
 pub struct LocalCommitmentTransaction {
 	tx: Transaction,
-	//TODO: modify Channel methods to integrate HTLC material at LocalCommitmentTransaction generation to drop Option here
-	local_keys: Option<TxCreationKeys>,
-	feerate_per_kw: Option<u64>,
+	pub(crate) local_keys: TxCreationKeys,
+	pub(crate) feerate_per_kw: u64,
 	per_htlc: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>
 }
 impl LocalCommitmentTransaction {
@@ -502,21 +501,30 @@ impl LocalCommitmentTransaction {
  sequence: 0,
  witness: vec![vec![], vec![], vec![]]
  };
- Self { tx: Transaction {
- version: 2,
- input: vec![dummy_input],
- output: Vec::new(),
- lock_time: 0,
- },
- local_keys: None,
- feerate_per_kw: None,
+ let dummy_key = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&[42; 32]).unwrap());
+ Self {
+ tx: Transaction {
+ version: 2,
+ input: vec![dummy_input],
+ output: Vec::new(),
+ lock_time: 0,
+ },
+ local_keys: TxCreationKeys {
+ per_commitment_point: dummy_key.clone(),
+ revocation_key: dummy_key.clone(),
+ a_htlc_key: dummy_key.clone(),
+ b_htlc_key: dummy_key.clone(),
+ a_delayed_payment_key: dummy_key.clone(),
+ b_payment_key: dummy_key.clone(),
+ },
+ feerate_per_kw: 0,
  per_htlc: Vec::new()
  }
 	}
 
 	/// Generate a new LocalCommitmentTransaction based on a raw commitment transaction,
 	/// remote signature and both parties keys
-	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction {
+	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, mut htlc_data: Vec<(HTLCOutputInCommitment, Option<Signature>)>) -> LocalCommitmentTransaction {
  if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); }
  if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); }
 
@@ -533,9 +541,10 @@ impl LocalCommitmentTransaction {
  }
 
  Self { tx,
- local_keys: None,
- feerate_per_kw: None,
- per_htlc: Vec::new()
+ local_keys,
+ feerate_per_kw,
+ // TODO: Avoid the conversion of a Vec created likely just for this:
+ per_htlc: htlc_data.drain(..).map(|(a, b)| (a, b, None)).collect(),
  }
 	}
 
@@ -594,33 +603,23 @@ impl LocalCommitmentTransaction {
  &self.tx
 	}
 
-	/// Set HTLC cache to generate any local HTLC transaction spending one of htlc ouput
-	/// from this local commitment transaction
-	pub(crate) fn set_htlc_cache(&mut self, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>) {
- self.local_keys = Some(local_keys);
- self.feerate_per_kw = Some(feerate_per_kw);
- self.per_htlc = htlc_outputs;
-	}
-
 	/// Add local signature for a htlc transaction, do nothing if a cached signed transaction is
 	/// already present
 	pub fn add_htlc_sig<T: secp256k1::Signing>(&mut self, htlc_base_key: &SecretKey, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
- if self.local_keys.is_none() || self.feerate_per_kw.is_none() { return; }
- let local_keys = self.local_keys.as_ref().unwrap();
  let txid = self.txid();
  for this_htlc in self.per_htlc.iter_mut() {
- if this_htlc.0.transaction_output_index.unwrap() == htlc_index {
+ if this_htlc.0.transaction_output_index == Some(htlc_index) {
  if this_htlc.2.is_some() { return; } // we already have a cached htlc transaction at provided index
- let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw.unwrap(), local_csv, &this_htlc.0, &local_keys.a_delayed_payment_key, &local_keys.revocation_key);
+ let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key);
  if !this_htlc.0.offered && preimage.is_none() { return; } // if we don't have preimage for HTLC-Success, don't try to generate
  let htlc_secret = if !this_htlc.0.offered { preimage } else { None }; // if we have a preimage for HTLC-Timeout, don't use it that's likely a duplicate HTLC hash
  if this_htlc.1.is_none() { return; } // we don't have any remote signature for this htlc
  if htlc_tx.input.len() != 1 { return; }
  if htlc_tx.input[0].witness.len() != 0 { return; }
 
- let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &local_keys.a_htlc_key, &local_keys.b_htlc_key, &local_keys.revocation_key);
+ let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
 
- if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &local_keys.per_commitment_point, htlc_base_key) {
+ if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &self.local_keys.per_commitment_point, htlc_base_key) {
  let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]);
  let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);
 

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1629,12 +1629,9 @@ fn monitor_update_claim_fail_no_response() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
-// Note that restore_between_fails with !fail_on_generate is useless
-// Also note that !fail_on_generate && !fail_on_signed is useless
-// Finally, note that !fail_on_signed is not possible with fail_on_generate && !restore_between_fails
 // confirm_a_first and restore_b_before_conf are wholly unrelated to earlier bools and
 // restore_b_before_conf has no meaning if !confirm_a_first
-fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails: bool, fail_on_signed: bool, confirm_a_first: bool, restore_b_before_conf: bool) {
+fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: bool) {
 	// Test that if the monitor update generated by funding_transaction_generated fails we continue
 	// the channel setup happily after the update is restored.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
@@ -1648,52 +1645,25 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 	let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43);
 
-	if fail_on_generate {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	}
 	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
-	check_added_monitors!(nodes[0], 1);
+	check_added_monitors!(nodes[0], 0);
 
 	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
 	let channel_id = OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
 	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
 	check_added_monitors!(nodes[1], 1);
 
-	if restore_between_fails {
- assert!(fail_on_generate);
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-	}
-
-	if fail_on_signed {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	} else {
- assert!(restore_between_fails || !fail_on_generate); // We can't switch to good now (there's no monitor update)
- assert!(fail_on_generate); // Somebody has to fail
-	}
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
-	if fail_on_signed || !restore_between_fails {
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
- if fail_on_generate && !restore_between_fails {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented funding_signed from allowing funding broadcast".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- } else {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- }
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
-	} else {
- check_added_monitors!(nodes[0], 1);
-	}
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+	nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
+	check_added_monitors!(nodes[0], 1);
+	assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
+	let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
+	nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
+	check_added_monitors!(nodes[0], 0);
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
@@ -1757,8 +1727,7 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 #[test]
 fn during_funding_monitor_fail() {
-	do_during_funding_monitor_fail(false, false, true, true, true);
-	do_during_funding_monitor_fail(true, false, true, false, false);
-	do_during_funding_monitor_fail(true, true, true, true, false);
-	do_during_funding_monitor_fail(true, true, false, false, false);
+	do_during_funding_monitor_fail(true, true);
+	do_during_funding_monitor_fail(true, false);
+	do_during_funding_monitor_fail(false, false);
 }

fuzz/src/chanmon_deser.rs

@@ -32,8 +32,6 @@ pub fn do_test(data: &[u8]) {
  let deserialized_copy = <(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(&w.0), logger.clone()).unwrap();
  assert!(latest_block_hash == deserialized_copy.0);
  assert!(monitor == deserialized_copy.1);
- w.0.clear();
- monitor.write_for_watchtower(&mut w).unwrap();
 	}
 }
 

lightning/src/chain/keysinterface.rs

@@ -222,15 +222,15 @@ pub trait ChannelKeys : Send+Clone {
 	/// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
 	/// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method
 	/// making the callee generate it via some util function we expose)!
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Create a signature for a local commitment transaction without enforcing one-time signing.
 	///
 	/// Testing revocation logic by our test framework needs to sign multiple local commitment
 	/// transactions. This unsafe test-only version doesn't enforce one-time signing security
 	/// requirement.
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Signs a transaction created by build_htlc_transaction. If the transaction is an
 	/// HTLC-Success transaction, preimage must be set!
@@ -363,13 +363,21 @@ impl ChannelKeys for InMemoryChannelKeys {
  Ok((commitment_sig, htlc_sigs))
 	}
 
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {

lightning/src/ln/chan_utils.rs

@@ -485,9 +485,8 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s
 /// just pass in the SecretKeys required.
 pub struct LocalCommitmentTransaction {
 	tx: Transaction,
-	//TODO: modify Channel methods to integrate HTLC material at LocalCommitmentTransaction generation to drop Option here
-	local_keys: Option<TxCreationKeys>,
-	feerate_per_kw: Option<u64>,
+	pub(crate) local_keys: TxCreationKeys,
+	pub(crate) feerate_per_kw: u64,
 	per_htlc: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>
 }
 impl LocalCommitmentTransaction {
@@ -502,21 +501,30 @@ impl LocalCommitmentTransaction {
  sequence: 0,
  witness: vec![vec![], vec![], vec![]]
  };
- Self { tx: Transaction {
- version: 2,
- input: vec![dummy_input],
- output: Vec::new(),
- lock_time: 0,
- },
- local_keys: None,
- feerate_per_kw: None,
+ let dummy_key = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&[42; 32]).unwrap());
+ Self {
+ tx: Transaction {
+ version: 2,
+ input: vec![dummy_input],
+ output: Vec::new(),
+ lock_time: 0,
+ },
+ local_keys: TxCreationKeys {
+ per_commitment_point: dummy_key.clone(),
+ revocation_key: dummy_key.clone(),
+ a_htlc_key: dummy_key.clone(),
+ b_htlc_key: dummy_key.clone(),
+ a_delayed_payment_key: dummy_key.clone(),
+ b_payment_key: dummy_key.clone(),
+ },
+ feerate_per_kw: 0,
  per_htlc: Vec::new()
  }
 	}
 
 	/// Generate a new LocalCommitmentTransaction based on a raw commitment transaction,
 	/// remote signature and both parties keys
-	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction {
+	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, mut htlc_data: Vec<(HTLCOutputInCommitment, Option<Signature>)>) -> LocalCommitmentTransaction {
  if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); }
  if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); }
 
@@ -533,9 +541,10 @@ impl LocalCommitmentTransaction {
  }
 
  Self { tx,
- local_keys: None,
- feerate_per_kw: None,
- per_htlc: Vec::new()
+ local_keys,
+ feerate_per_kw,
+ // TODO: Avoid the conversion of a Vec created likely just for this:
+ per_htlc: htlc_data.drain(..).map(|(a, b)| (a, b, None)).collect(),
  }
 	}
 
@@ -594,33 +603,23 @@ impl LocalCommitmentTransaction {
  &self.tx
 	}
 
-	/// Set HTLC cache to generate any local HTLC transaction spending one of htlc ouput
-	/// from this local commitment transaction
-	pub(crate) fn set_htlc_cache(&mut self, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>) {
- self.local_keys = Some(local_keys);
- self.feerate_per_kw = Some(feerate_per_kw);
- self.per_htlc = htlc_outputs;
-	}
-
 	/// Add local signature for a htlc transaction, do nothing if a cached signed transaction is
 	/// already present
 	pub fn add_htlc_sig<T: secp256k1::Signing>(&mut self, htlc_base_key: &SecretKey, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
- if self.local_keys.is_none() || self.feerate_per_kw.is_none() { return; }
- let local_keys = self.local_keys.as_ref().unwrap();
  let txid = self.txid();
  for this_htlc in self.per_htlc.iter_mut() {
- if this_htlc.0.transaction_output_index.unwrap() == htlc_index {
+ if this_htlc.0.transaction_output_index == Some(htlc_index) {
  if this_htlc.2.is_some() { return; } // we already have a cached htlc transaction at provided index
- let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw.unwrap(), local_csv, &this_htlc.0, &local_keys.a_delayed_payment_key, &local_keys.revocation_key);
+ let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key);
  if !this_htlc.0.offered && preimage.is_none() { return; } // if we don't have preimage for HTLC-Success, don't try to generate
  let htlc_secret = if !this_htlc.0.offered { preimage } else { None }; // if we have a preimage for HTLC-Timeout, don't use it that's likely a duplicate HTLC hash
  if this_htlc.1.is_none() { return; } // we don't have any remote signature for this htlc
  if htlc_tx.input.len() != 1 { return; }
  if htlc_tx.input[0].witness.len() != 0 { return; }
 
- let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &local_keys.a_htlc_key, &local_keys.b_htlc_key, &local_keys.revocation_key);
+ let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
 
- if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &local_keys.per_commitment_point, htlc_base_key) {
+ if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &self.local_keys.per_commitment_point, htlc_base_key) {
  let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]);
  let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);
 

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1629,12 +1629,9 @@ fn monitor_update_claim_fail_no_response() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
-// Note that restore_between_fails with !fail_on_generate is useless
-// Also note that !fail_on_generate && !fail_on_signed is useless
-// Finally, note that !fail_on_signed is not possible with fail_on_generate && !restore_between_fails
 // confirm_a_first and restore_b_before_conf are wholly unrelated to earlier bools and
 // restore_b_before_conf has no meaning if !confirm_a_first
-fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails: bool, fail_on_signed: bool, confirm_a_first: bool, restore_b_before_conf: bool) {
+fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: bool) {
 	// Test that if the monitor update generated by funding_transaction_generated fails we continue
 	// the channel setup happily after the update is restored.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
@@ -1648,52 +1645,25 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 	let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43);
 
-	if fail_on_generate {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	}
 	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
-	check_added_monitors!(nodes[0], 1);
+	check_added_monitors!(nodes[0], 0);
 
 	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
 	let channel_id = OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
 	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
 	check_added_monitors!(nodes[1], 1);
 
-	if restore_between_fails {
- assert!(fail_on_generate);
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-	}
-
-	if fail_on_signed {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	} else {
- assert!(restore_between_fails || !fail_on_generate); // We can't switch to good now (there's no monitor update)
- assert!(fail_on_generate); // Somebody has to fail
-	}
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
-	if fail_on_signed || !restore_between_fails {
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
- if fail_on_generate && !restore_between_fails {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented funding_signed from allowing funding broadcast".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- } else {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- }
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
-	} else {
- check_added_monitors!(nodes[0], 1);
-	}
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+	nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
+	check_added_monitors!(nodes[0], 1);
+	assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
+	let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
+	nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
+	check_added_monitors!(nodes[0], 0);
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
@@ -1757,8 +1727,7 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 #[test]
 fn during_funding_monitor_fail() {
-	do_during_funding_monitor_fail(false, false, true, true, true);
-	do_during_funding_monitor_fail(true, false, true, false, false);
-	do_during_funding_monitor_fail(true, true, true, true, false);
-	do_during_funding_monitor_fail(true, true, false, false, false);
+	do_during_funding_monitor_fail(true, true);
+	do_during_funding_monitor_fail(true, false);
+	do_during_funding_monitor_fail(false, false);
 }

fuzz/src/chanmon_deser.rs

@@ -32,8 +32,6 @@ pub fn do_test(data: &[u8]) {
  let deserialized_copy = <(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(&w.0), logger.clone()).unwrap();
  assert!(latest_block_hash == deserialized_copy.0);
  assert!(monitor == deserialized_copy.1);
- w.0.clear();
- monitor.write_for_watchtower(&mut w).unwrap();
 	}
 }
 

lightning/src/chain/keysinterface.rs

@@ -222,15 +222,15 @@ pub trait ChannelKeys : Send+Clone {
 	/// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
 	/// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method
 	/// making the callee generate it via some util function we expose)!
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Create a signature for a local commitment transaction without enforcing one-time signing.
 	///
 	/// Testing revocation logic by our test framework needs to sign multiple local commitment
 	/// transactions. This unsafe test-only version doesn't enforce one-time signing security
 	/// requirement.
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Signs a transaction created by build_htlc_transaction. If the transaction is an
 	/// HTLC-Success transaction, preimage must be set!
@@ -363,13 +363,21 @@ impl ChannelKeys for InMemoryChannelKeys {
  Ok((commitment_sig, htlc_sigs))
 	}
 
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {

lightning/src/ln/chan_utils.rs

@@ -485,9 +485,8 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s
 /// just pass in the SecretKeys required.
 pub struct LocalCommitmentTransaction {
 	tx: Transaction,
-	//TODO: modify Channel methods to integrate HTLC material at LocalCommitmentTransaction generation to drop Option here
-	local_keys: Option<TxCreationKeys>,
-	feerate_per_kw: Option<u64>,
+	pub(crate) local_keys: TxCreationKeys,
+	pub(crate) feerate_per_kw: u64,
 	per_htlc: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>
 }
 impl LocalCommitmentTransaction {
@@ -502,21 +501,30 @@ impl LocalCommitmentTransaction {
  sequence: 0,
  witness: vec![vec![], vec![], vec![]]
  };
- Self { tx: Transaction {
- version: 2,
- input: vec![dummy_input],
- output: Vec::new(),
- lock_time: 0,
- },
- local_keys: None,
- feerate_per_kw: None,
+ let dummy_key = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&[42; 32]).unwrap());
+ Self {
+ tx: Transaction {
+ version: 2,
+ input: vec![dummy_input],
+ output: Vec::new(),
+ lock_time: 0,
+ },
+ local_keys: TxCreationKeys {
+ per_commitment_point: dummy_key.clone(),
+ revocation_key: dummy_key.clone(),
+ a_htlc_key: dummy_key.clone(),
+ b_htlc_key: dummy_key.clone(),
+ a_delayed_payment_key: dummy_key.clone(),
+ b_payment_key: dummy_key.clone(),
+ },
+ feerate_per_kw: 0,
  per_htlc: Vec::new()
  }
 	}
 
 	/// Generate a new LocalCommitmentTransaction based on a raw commitment transaction,
 	/// remote signature and both parties keys
-	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction {
+	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, mut htlc_data: Vec<(HTLCOutputInCommitment, Option<Signature>)>) -> LocalCommitmentTransaction {
  if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); }
  if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); }
 
@@ -533,9 +541,10 @@ impl LocalCommitmentTransaction {
  }
 
  Self { tx,
- local_keys: None,
- feerate_per_kw: None,
- per_htlc: Vec::new()
+ local_keys,
+ feerate_per_kw,
+ // TODO: Avoid the conversion of a Vec created likely just for this:
+ per_htlc: htlc_data.drain(..).map(|(a, b)| (a, b, None)).collect(),
  }
 	}
 
@@ -594,33 +603,23 @@ impl LocalCommitmentTransaction {
  &self.tx
 	}
 
-	/// Set HTLC cache to generate any local HTLC transaction spending one of htlc ouput
-	/// from this local commitment transaction
-	pub(crate) fn set_htlc_cache(&mut self, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>) {
- self.local_keys = Some(local_keys);
- self.feerate_per_kw = Some(feerate_per_kw);
- self.per_htlc = htlc_outputs;
-	}
-
 	/// Add local signature for a htlc transaction, do nothing if a cached signed transaction is
 	/// already present
 	pub fn add_htlc_sig<T: secp256k1::Signing>(&mut self, htlc_base_key: &SecretKey, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
- if self.local_keys.is_none() || self.feerate_per_kw.is_none() { return; }
- let local_keys = self.local_keys.as_ref().unwrap();
  let txid = self.txid();
  for this_htlc in self.per_htlc.iter_mut() {
- if this_htlc.0.transaction_output_index.unwrap() == htlc_index {
+ if this_htlc.0.transaction_output_index == Some(htlc_index) {
  if this_htlc.2.is_some() { return; } // we already have a cached htlc transaction at provided index
- let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw.unwrap(), local_csv, &this_htlc.0, &local_keys.a_delayed_payment_key, &local_keys.revocation_key);
+ let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key);
  if !this_htlc.0.offered && preimage.is_none() { return; } // if we don't have preimage for HTLC-Success, don't try to generate
  let htlc_secret = if !this_htlc.0.offered { preimage } else { None }; // if we have a preimage for HTLC-Timeout, don't use it that's likely a duplicate HTLC hash
  if this_htlc.1.is_none() { return; } // we don't have any remote signature for this htlc
  if htlc_tx.input.len() != 1 { return; }
  if htlc_tx.input[0].witness.len() != 0 { return; }
 
- let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &local_keys.a_htlc_key, &local_keys.b_htlc_key, &local_keys.revocation_key);
+ let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
 
- if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &local_keys.per_commitment_point, htlc_base_key) {
+ if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &self.local_keys.per_commitment_point, htlc_base_key) {
  let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]);
  let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);
 

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1629,12 +1629,9 @@ fn monitor_update_claim_fail_no_response() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
-// Note that restore_between_fails with !fail_on_generate is useless
-// Also note that !fail_on_generate && !fail_on_signed is useless
-// Finally, note that !fail_on_signed is not possible with fail_on_generate && !restore_between_fails
 // confirm_a_first and restore_b_before_conf are wholly unrelated to earlier bools and
 // restore_b_before_conf has no meaning if !confirm_a_first
-fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails: bool, fail_on_signed: bool, confirm_a_first: bool, restore_b_before_conf: bool) {
+fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: bool) {
 	// Test that if the monitor update generated by funding_transaction_generated fails we continue
 	// the channel setup happily after the update is restored.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
@@ -1648,52 +1645,25 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 	let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43);
 
-	if fail_on_generate {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	}
 	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
-	check_added_monitors!(nodes[0], 1);
+	check_added_monitors!(nodes[0], 0);
 
 	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
 	let channel_id = OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
 	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
 	check_added_monitors!(nodes[1], 1);
 
-	if restore_between_fails {
- assert!(fail_on_generate);
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-	}
-
-	if fail_on_signed {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	} else {
- assert!(restore_between_fails || !fail_on_generate); // We can't switch to good now (there's no monitor update)
- assert!(fail_on_generate); // Somebody has to fail
-	}
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
-	if fail_on_signed || !restore_between_fails {
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
- if fail_on_generate && !restore_between_fails {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented funding_signed from allowing funding broadcast".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- } else {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- }
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
-	} else {
- check_added_monitors!(nodes[0], 1);
-	}
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+	nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
+	check_added_monitors!(nodes[0], 1);
+	assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
+	let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
+	nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
+	check_added_monitors!(nodes[0], 0);
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
@@ -1757,8 +1727,7 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 #[test]
 fn during_funding_monitor_fail() {
-	do_during_funding_monitor_fail(false, false, true, true, true);
-	do_during_funding_monitor_fail(true, false, true, false, false);
-	do_during_funding_monitor_fail(true, true, true, true, false);
-	do_during_funding_monitor_fail(true, true, false, false, false);
+	do_during_funding_monitor_fail(true, true);
+	do_during_funding_monitor_fail(true, false);
+	do_during_funding_monitor_fail(false, false);
 }

fuzz/src/chanmon_deser.rs

@@ -32,8 +32,6 @@ pub fn do_test(data: &[u8]) {
  let deserialized_copy = <(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(&w.0), logger.clone()).unwrap();
  assert!(latest_block_hash == deserialized_copy.0);
  assert!(monitor == deserialized_copy.1);
- w.0.clear();
- monitor.write_for_watchtower(&mut w).unwrap();
 	}
 }
 

lightning/src/chain/keysinterface.rs

@@ -222,15 +222,15 @@ pub trait ChannelKeys : Send+Clone {
 	/// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
 	/// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method
 	/// making the callee generate it via some util function we expose)!
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Create a signature for a local commitment transaction without enforcing one-time signing.
 	///
 	/// Testing revocation logic by our test framework needs to sign multiple local commitment
 	/// transactions. This unsafe test-only version doesn't enforce one-time signing security
 	/// requirement.
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Signs a transaction created by build_htlc_transaction. If the transaction is an
 	/// HTLC-Success transaction, preimage must be set!
@@ -363,13 +363,21 @@ impl ChannelKeys for InMemoryChannelKeys {
  Ok((commitment_sig, htlc_sigs))
 	}
 
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
- local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+ let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+ let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+ let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+ local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {

lightning/src/ln/chan_utils.rs

@@ -485,9 +485,8 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s
 /// just pass in the SecretKeys required.
 pub struct LocalCommitmentTransaction {
 	tx: Transaction,
-	//TODO: modify Channel methods to integrate HTLC material at LocalCommitmentTransaction generation to drop Option here
-	local_keys: Option<TxCreationKeys>,
-	feerate_per_kw: Option<u64>,
+	pub(crate) local_keys: TxCreationKeys,
+	pub(crate) feerate_per_kw: u64,
 	per_htlc: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>
 }
 impl LocalCommitmentTransaction {
@@ -502,21 +501,30 @@ impl LocalCommitmentTransaction {
  sequence: 0,
  witness: vec![vec![], vec![], vec![]]
  };
- Self { tx: Transaction {
- version: 2,
- input: vec![dummy_input],
- output: Vec::new(),
- lock_time: 0,
- },
- local_keys: None,
- feerate_per_kw: None,
+ let dummy_key = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&[42; 32]).unwrap());
+ Self {
+ tx: Transaction {
+ version: 2,
+ input: vec![dummy_input],
+ output: Vec::new(),
+ lock_time: 0,
+ },
+ local_keys: TxCreationKeys {
+ per_commitment_point: dummy_key.clone(),
+ revocation_key: dummy_key.clone(),
+ a_htlc_key: dummy_key.clone(),
+ b_htlc_key: dummy_key.clone(),
+ a_delayed_payment_key: dummy_key.clone(),
+ b_payment_key: dummy_key.clone(),
+ },
+ feerate_per_kw: 0,
  per_htlc: Vec::new()
  }
 	}
 
 	/// Generate a new LocalCommitmentTransaction based on a raw commitment transaction,
 	/// remote signature and both parties keys
-	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction {
+	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, mut htlc_data: Vec<(HTLCOutputInCommitment, Option<Signature>)>) -> LocalCommitmentTransaction {
  if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); }
  if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); }
 
@@ -533,9 +541,10 @@ impl LocalCommitmentTransaction {
  }
 
  Self { tx,
- local_keys: None,
- feerate_per_kw: None,
- per_htlc: Vec::new()
+ local_keys,
+ feerate_per_kw,
+ // TODO: Avoid the conversion of a Vec created likely just for this:
+ per_htlc: htlc_data.drain(..).map(|(a, b)| (a, b, None)).collect(),
  }
 	}
 
@@ -594,33 +603,23 @@ impl LocalCommitmentTransaction {
  &self.tx
 	}
 
-	/// Set HTLC cache to generate any local HTLC transaction spending one of htlc ouput
-	/// from this local commitment transaction
-	pub(crate) fn set_htlc_cache(&mut self, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>) {
- self.local_keys = Some(local_keys);
- self.feerate_per_kw = Some(feerate_per_kw);
- self.per_htlc = htlc_outputs;
-	}
-
 	/// Add local signature for a htlc transaction, do nothing if a cached signed transaction is
 	/// already present
 	pub fn add_htlc_sig<T: secp256k1::Signing>(&mut self, htlc_base_key: &SecretKey, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
- if self.local_keys.is_none() || self.feerate_per_kw.is_none() { return; }
- let local_keys = self.local_keys.as_ref().unwrap();
  let txid = self.txid();
  for this_htlc in self.per_htlc.iter_mut() {
- if this_htlc.0.transaction_output_index.unwrap() == htlc_index {
+ if this_htlc.0.transaction_output_index == Some(htlc_index) {
  if this_htlc.2.is_some() { return; } // we already have a cached htlc transaction at provided index
- let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw.unwrap(), local_csv, &this_htlc.0, &local_keys.a_delayed_payment_key, &local_keys.revocation_key);
+ let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key);
  if !this_htlc.0.offered && preimage.is_none() { return; } // if we don't have preimage for HTLC-Success, don't try to generate
  let htlc_secret = if !this_htlc.0.offered { preimage } else { None }; // if we have a preimage for HTLC-Timeout, don't use it that's likely a duplicate HTLC hash
  if this_htlc.1.is_none() { return; } // we don't have any remote signature for this htlc
  if htlc_tx.input.len() != 1 { return; }
  if htlc_tx.input[0].witness.len() != 0 { return; }
 
- let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &local_keys.a_htlc_key, &local_keys.b_htlc_key, &local_keys.revocation_key);
+ let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
 
- if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &local_keys.per_commitment_point, htlc_base_key) {
+ if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &self.local_keys.per_commitment_point, htlc_base_key) {
  let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]);
  let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);
 

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1629,12 +1629,9 @@ fn monitor_update_claim_fail_no_response() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
-// Note that restore_between_fails with !fail_on_generate is useless
-// Also note that !fail_on_generate && !fail_on_signed is useless
-// Finally, note that !fail_on_signed is not possible with fail_on_generate && !restore_between_fails
 // confirm_a_first and restore_b_before_conf are wholly unrelated to earlier bools and
 // restore_b_before_conf has no meaning if !confirm_a_first
-fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails: bool, fail_on_signed: bool, confirm_a_first: bool, restore_b_before_conf: bool) {
+fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: bool) {
 	// Test that if the monitor update generated by funding_transaction_generated fails we continue
 	// the channel setup happily after the update is restored.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
@@ -1648,52 +1645,25 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 	let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43);
 
-	if fail_on_generate {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	}
 	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
-	check_added_monitors!(nodes[0], 1);
+	check_added_monitors!(nodes[0], 0);
 
 	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
 	let channel_id = OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
 	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
 	check_added_monitors!(nodes[1], 1);
 
-	if restore_between_fails {
- assert!(fail_on_generate);
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-	}
-
-	if fail_on_signed {
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	} else {
- assert!(restore_between_fails || !fail_on_generate); // We can't switch to good now (there's no monitor update)
- assert!(fail_on_generate); // Somebody has to fail
-	}
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
-	if fail_on_signed || !restore_between_fails {
- assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
- if fail_on_generate && !restore_between_fails {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented funding_signed from allowing funding broadcast".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- } else {
- nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
- check_added_monitors!(nodes[0], 1);
- }
- assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
- *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
- let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
- nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
- check_added_monitors!(nodes[0], 0);
-	} else {
- check_added_monitors!(nodes[0], 1);
-	}
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+	nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
+	check_added_monitors!(nodes[0], 1);
+	assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
+	let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
+	nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
+	check_added_monitors!(nodes[0], 0);
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
@@ -1757,8 +1727,7 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 #[test]
 fn during_funding_monitor_fail() {
-	do_during_funding_monitor_fail(false, false, true, true, true);
-	do_during_funding_monitor_fail(true, false, true, false, false);
-	do_during_funding_monitor_fail(true, true, true, true, false);
-	do_during_funding_monitor_fail(true, true, false, false, false);
+	do_during_funding_monitor_fail(true, true);
+	do_during_funding_monitor_fail(true, false);
+	do_during_funding_monitor_fail(false, false);
 }