Addressing PR comments.

Author: philipr-za

src/ln/channel.rs

@@ -357,6 +357,7 @@ const UNCONF_THRESHOLD: u32 = 6;
 const BREAKDOWN_TIMEOUT: u16 = 6 * 24 * 7; //TODO?
 /// The amount of time we're willing to wait to claim money back to us
 const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 6 * 24 * 14;
+/// Exposing these two constants for use in test in ChannelMonitor
 pub const COMMITMENT_TX_BASE_WEIGHT: u64 = 724;
 pub const COMMITMENT_TX_WEIGHT_PER_HTLC: u64 = 172;
 const SPENDING_INPUT_FOR_A_OUTPUT_WEIGHT: u64 = 79; // prevout: 36, nSequence: 4, script len: 1, witness lengths: (3+1)/4, sig: 73/4, if-selector: 1, redeemScript: (6 ops + 2*33 pubkeys + 1*2 delay)/4
@@ -1661,16 +1662,16 @@ impl Channel {
 			self.feerate_per_kw
 		};
 
-
 		let mut local_commitment_tx = self.build_commitment_transaction(self.cur_local_commitment_transaction_number, &local_keys, true, false, feerate_per_kw);
 		let local_commitment_txid = local_commitment_tx.0.txid();
 		let local_sighash = Message::from_slice(&bip143::SighashComponents::new(&local_commitment_tx.0).sighash_all(&local_commitment_tx.0.input[0], &funding_script, self.channel_value_satoshis)[..]).unwrap();
 		secp_call!(self.secp_ctx.verify(&local_sighash, &msg.signature, &self.their_funding_pubkey.unwrap()), "Invalid commitment tx signature from peer", self.channel_id());
 
 		//If channel fee was updated by funder confirm funder can afford the new fee rate when applied to the current local commitment transaction
 		if update_fee {
-			let num_htlcs = local_commitment_tx.0.output.len();
+			let num_htlcs = local_commitment_tx.1.len();
 			let total_fee: u64 = feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;
+
 			if self.channel_value_satoshis - self.value_to_self_msat / 1000 < total_fee + self.their_channel_reserve_satoshis {
 				return Err(HandleError { err: "Funding remote cannot afford proposed new fee", action: Some(ErrorAction::DisconnectPeer { msg: None }) });
 			}
@@ -1680,8 +1681,6 @@ impl Channel {
 			return Err(HandleError{err: "Got wrong number of HTLC signatures from remote", action: None});
 		}
 
-		println!("Len: {}, Num HTLC Sigs: {}", local_commitment_tx.0.output.len(), local_commitment_tx.1.len());
-
 		let mut new_local_commitment_txn = Vec::with_capacity(local_commitment_tx.1.len() + 1);
 		self.sign_commitment_transaction(&mut local_commitment_tx.0, &msg.signature);
 		new_local_commitment_txn.push(local_commitment_tx.0.clone());

src/ln/channelmanager.rs

@@ -22,7 +22,7 @@ use secp256k1;
 
 use chain::chaininterface::{BroadcasterInterface,ChainListener,ChainWatchInterface,FeeEstimator};
 use chain::transaction::OutPoint;
-use ln::channel::{Channel, ChannelError, COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};
+use ln::channel::{Channel, ChannelError};
 use ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, ManyChannelMonitor, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
 use ln::router::{Route,RouteHop};
 use ln::msgs;
@@ -3212,6 +3212,7 @@ mod tests {
 	use chain::chaininterface::{ChainListener, ChainWatchInterface};
 	use chain::keysinterface::KeysInterface;
 	use chain::keysinterface;
+	use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};
 	use ln::channelmanager::{ChannelManager,ChannelManagerReadArgs,OnionKeys,PaymentFailReason,RAACommitmentOrder};
 	use ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS, ManyChannelMonitor};
 	use ln::router::{Route, RouteHop, Router};
@@ -4444,62 +4445,49 @@ mod tests {
 	#[test]
 	fn test_update_fee_that_funder_cannot_afford() {
 		let mut nodes = create_network(2);
-		let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1432, 200000);
+		let channel_value = 1888;
+		let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, channel_value, 700000);
 		let channel_id = chan.2;
 
 		let feerate = 260;
 		nodes[0].node.update_fee(channel_id, feerate).unwrap();
 		check_added_monitors!(nodes[0], 1);
+		let update_msg = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
 
-		let events_0 = nodes[0].node.get_and_clear_pending_msg_events();
-		assert_eq!(events_0.len(), 1);
-		let (update_msg, commitment_signed) = match events_0[0] {
-			MessageSendEvent::UpdateHTLCs {
-				node_id: _, updates: msgs::CommitmentUpdate {
-				update_add_htlcs: _, update_fulfill_htlcs: _, update_fail_htlcs: _, update_fail_malformed_htlcs: _, ref update_fee, ref commitment_signed
-			}
-			} => {
-				(update_fee.as_ref(), commitment_signed)
-			},
-			_ => panic!("Unexpected event"),
-		};
-		nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), update_msg.unwrap()).unwrap();
+		nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &update_msg.update_fee.unwrap()).unwrap();
 
-		commitment_signed_dance!(nodes[1], nodes[0], commitment_signed, false);
+		commitment_signed_dance!(nodes[1], nodes[0], update_msg.commitment_signed, false);
 
 		//Confirm that the new fee based on the last local commitment txn is what we expected based on the feerate of 260 set above.
 		//This value results in a fee that is exactly what the funder can afford (277 sat + 1000 sat channel reserve)
 		{
 			let chan_lock = nodes[1].node.channel_state.lock().unwrap();
 			let chan = chan_lock.by_id.get(&channel_id).unwrap();
 
-			let num_htlcs = chan.last_local_commitment_txn[0].output.len();
-			let total_fee: u64 = feerate * (super::COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * super::COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;
-			let fee_for_single_update_to_commitment = feerate * (super::COMMITMENT_TX_BASE_WEIGHT + 1 * super::COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;
-			assert_eq!(total_fee, fee_for_single_update_to_commitment);
+			//We made sure neither party's funds are below the dust limit so -2 non-HTLC txns from number of outputs
+			let num_htlcs = chan.last_local_commitment_txn[0].output.len() - 2;
+			let total_fee: u64 = feerate * (COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;
+			let mut actual_fee = chan.last_local_commitment_txn[0].output.iter().fold(0, |acc, output| acc + output.value);
+			actual_fee = channel_value - actual_fee;
+			assert_eq!(total_fee, actual_fee);
 		} //drop the mutex
 
-		nodes[0].node.update_fee(channel_id, feerate+1).unwrap();
+		//Add 2 to the previous fee rate to the final fee increases by 1 (with no HTLCs the fee is essentially
+		//fee_rate*(724/1000) so the increment of 1*0.724 is rounded back down)
+		nodes[0].node.update_fee(channel_id, feerate+2).unwrap();
 		check_added_monitors!(nodes[0], 1);
 
-		let events_0 = nodes[0].node.get_and_clear_pending_msg_events();
-		assert_eq!(events_0.len(), 1);
-		let (update_msg, commitment_signed) = match events_0[0] {
-			MessageSendEvent::UpdateHTLCs { node_id:_, updates: msgs::CommitmentUpdate { update_add_htlcs:_, update_fulfill_htlcs:_, update_fail_htlcs:_, update_fail_malformed_htlcs:_, ref update_fee, ref commitment_signed } } => {
-				(update_fee.as_ref(), commitment_signed)
-			},
-			_ => panic!("Unexpected event"),
-		};
+		let update2_msg = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
 
-		nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), update_msg.unwrap());
+		nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &update2_msg.update_fee.unwrap());
 
 		//While producing the commitment_signed response after handling a received update_fee request the
 		//check to see if the funder, who sent the update_fee request, can afford the new fee (funder_balance >= fee+channel_reserve)
 		//Should produce and error.
-		let err = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &commitment_signed).unwrap_err();
+		let err = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &update2_msg.commitment_signed).unwrap_err();
 
-		assert!(match err.action.unwrap() {
-			msgs::ErrorAction::DisconnectPeer{..} => true,
+		assert!(match err.err {
+			"Funding remote cannot afford proposed new fee" => true,
 			_ => false,
 		});