Merge pull request #975 from TheBlueMatt/2021-06-fix-fee-calc

Make the base fee configurable in ChannelConfig

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -338,7 +338,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 			let monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), fee_est.clone(), Arc::new(TestPersister{}), Arc::clone(&keys_manager)));
 
 			let mut config = UserConfig::default();
-			config.channel_options.fee_proportional_millionths = 0;
+			config.channel_options.forwarding_fee_proportional_millionths = 0;
 			config.channel_options.announced_channel = true;
 			let network = Network::Bitcoin;
 			let params = ChainParameters {
@@ -357,7 +357,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 			let chain_monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), fee_est.clone(), Arc::new(TestPersister{}), Arc::clone(& $keys_manager)));
 
 			let mut config = UserConfig::default();
-			config.channel_options.fee_proportional_millionths = 0;
+			config.channel_options.forwarding_fee_proportional_millionths = 0;
 			config.channel_options.announced_channel = true;
 
 			let mut monitors = HashMap::new();

fuzz/src/full_stack.rs

@@ -238,7 +238,7 @@ mod tests {
 		let mut nodes = Vec::new();
 		for i in 0..num_nodes {
 			let tx_broadcaster = Arc::new(test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), blocks: Arc::new(Mutex::new(Vec::new()))});
-			let fee_estimator = Arc::new(test_utils::TestFeeEstimator { sat_per_kw: 253 });
+			let fee_estimator = Arc::new(test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) });
 			let chain_source = Arc::new(test_utils::TestChainSource::new(Network::Testnet));
 			let logger = Arc::new(test_utils::TestLogger::with_id(format!("node {}", i)));
 			let persister = Arc::new(FilesystemPersister::new(format!("{}_persister_{}", persist_dir, i)));

lightning-background-processor/src/lib.rs

@@ -2852,7 +2852,7 @@ mod tests {
 		let secp_ctx = Secp256k1::new();
 		let logger = Arc::new(TestLogger::new());
 		let broadcaster = Arc::new(TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), blocks: Arc::new(Mutex::new(Vec::new()))});
-		let fee_estimator = Arc::new(TestFeeEstimator { sat_per_kw: 253 });
+		let fee_estimator = Arc::new(TestFeeEstimator { sat_per_kw: Mutex::new(253) });
 
 		let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let dummy_tx = Transaction { version: 0, lock_time: 0, input: Vec::new(), output: Vec::new() };

lightning/src/chain/channelmonitor.rs

@@ -456,7 +456,6 @@ struct CommitmentTxInfoCached {
 }
 
 pub const OUR_MAX_HTLCS: u16 = 50; //TODO
-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
 
 #[cfg(not(test))]
 const COMMITMENT_TX_BASE_WEIGHT: u64 = 724;
@@ -3426,7 +3425,7 @@ impl<Signer: Sign> Channel<Signer> {
 	}
 
 	pub fn get_fee_proportional_millionths(&self) -> u32 {
-		self.config.fee_proportional_millionths
+		self.config.forwarding_fee_proportional_millionths
 	}
 
 	pub fn get_cltv_expiry_delta(&self) -> u16 {
@@ -3499,24 +3498,8 @@ impl<Signer: Sign> Channel<Signer> {
 
 	/// Gets the fee we'd want to charge for adding an HTLC output to this Channel
 	/// Allowed in any state (including after shutdown)
-	pub fn get_holder_fee_base_msat<F: Deref>(&self, fee_estimator: &F) -> u32
-		where F::Target: FeeEstimator
-	{
-		// For lack of a better metric, we calculate what it would cost to consolidate the new HTLC
-		// output value back into a transaction with the regular channel output:
-
-		// the fee cost of the HTLC-Success/HTLC-Timeout transaction:
-		let mut res = self.feerate_per_kw as u64 * cmp::max(HTLC_TIMEOUT_TX_WEIGHT, HTLC_SUCCESS_TX_WEIGHT) / 1000;
-
-		if self.is_outbound() {
-			// + the marginal fee increase cost to us in the commitment transaction:
-			res += self.feerate_per_kw as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC / 1000;
-		}
-
-		// + the marginal cost of an input which spends the HTLC-Success/HTLC-Timeout output:
-		res += fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal) as u64 * SPENDING_INPUT_FOR_A_OUTPUT_WEIGHT / 1000;
-
-		res as u32
+	pub fn get_outbound_forwarding_fee_base_msat(&self) -> u32 {
+		self.config.forwarding_fee_base_msat
 	}
 
 	/// Returns true if we've ever received a message from the remote end for this Channel
@@ -4460,7 +4443,7 @@ fn is_unsupported_shutdown_script(their_features: &InitFeatures, script: &Script
 	return !script.is_p2pkh() && !script.is_p2sh() && !script.is_v0_p2wpkh() && !script.is_v0_p2wsh()
 }
 
-const SERIALIZATION_VERSION: u8 = 1;
+const SERIALIZATION_VERSION: u8 = 2;
 const MIN_SERIALIZATION_VERSION: u8 = 1;
 
 impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
@@ -4502,7 +4485,13 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 		write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);
 
 		self.user_id.write(writer)?;
-		self.config.write(writer)?;
+
+		// Write out the old serialization for the config object. This is read by version-1
+		// deserializers, but we will read the version in the TLV at the end instead.
+		self.config.forwarding_fee_proportional_millionths.write(writer)?;
+		self.config.cltv_expiry_delta.write(writer)?;
+		self.config.announced_channel.write(writer)?;
+		self.config.commit_upfront_shutdown_pubkey.write(writer)?;
 
 		self.channel_id.write(writer)?;
 		(self.channel_state | ChannelState::PeerDisconnected as u32).write(writer)?;
@@ -4661,10 +4650,15 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 		self.counterparty_dust_limit_satoshis.write(writer)?;
 		self.holder_dust_limit_satoshis.write(writer)?;
 		self.counterparty_max_htlc_value_in_flight_msat.write(writer)?;
+
+		// Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 		self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0).write(writer)?;
+
 		self.counterparty_htlc_minimum_msat.write(writer)?;
 		self.holder_htlc_minimum_msat.write(writer)?;
 		self.counterparty_max_accepted_htlcs.write(writer)?;
+
+		// Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 		self.minimum_depth.unwrap_or(0).write(writer)?;
 
 		match &self.counterparty_forwarding_info {
@@ -4700,6 +4694,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 			// override that.
 			(1, self.minimum_depth, option),
 			(3, self.counterparty_selected_channel_reserve_satoshis, option),
+			(5, self.config, required),
 		});
 
 		Ok(())
@@ -4710,10 +4705,21 @@ const MAX_ALLOC_SIZE: usize = 64*1024;
 impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 		where K::Target: KeysInterface<Signer = Signer> {
 	fn read<R : ::std::io::Read>(reader: &mut R, keys_source: &'a K) -> Result<Self, DecodeError> {
-		let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
+		let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
 
 		let user_id = Readable::read(reader)?;
-		let config: ChannelConfig = Readable::read(reader)?;
+
+		let mut config = Some(ChannelConfig::default());
+		if ver == 1 {
+			// Read the old serialization of the ChannelConfig from version 0.0.98.
+			config.as_mut().unwrap().forwarding_fee_proportional_millionths = Readable::read(reader)?;
+			config.as_mut().unwrap().cltv_expiry_delta = Readable::read(reader)?;
+			config.as_mut().unwrap().announced_channel = Readable::read(reader)?;
+			config.as_mut().unwrap().commit_upfront_shutdown_pubkey = Readable::read(reader)?;
+		} else {
+			// Read the 8 bytes of backwards-compatibility ChannelConfig data.
+			let mut _val: u64 = Readable::read(reader)?;
+		}
 
 		let channel_id = Readable::read(reader)?;
 		let channel_state = Readable::read(reader)?;
@@ -4843,20 +4849,25 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 		let counterparty_dust_limit_satoshis = Readable::read(reader)?;
 		let holder_dust_limit_satoshis = Readable::read(reader)?;
 		let counterparty_max_htlc_value_in_flight_msat = Readable::read(reader)?;
-		let mut counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
-		if counterparty_selected_channel_reserve_satoshis == Some(0) {
-			// Versions up to 0.0.98 had counterparty_selected_channel_reserve_satoshis as a
-			// non-option, writing 0 for what we now consider None.
-			counterparty_selected_channel_reserve_satoshis = None;
+		let mut counterparty_selected_channel_reserve_satoshis = None;
+		if ver == 1 {
+			// Read the old serialization from version 0.0.98.
+			counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
+		} else {
+			// Read the 8 bytes of backwards-compatibility data.
+			let _dummy: u64 = Readable::read(reader)?;
 		}
 		let counterparty_htlc_minimum_msat = Readable::read(reader)?;
 		let holder_htlc_minimum_msat = Readable::read(reader)?;
 		let counterparty_max_accepted_htlcs = Readable::read(reader)?;
-		let mut minimum_depth = Some(Readable::read(reader)?);
-		if minimum_depth == Some(0) {
-			// Versions up to 0.0.98 had minimum_depth as a non-option, writing 0 for what we now
-			// consider None.
-			minimum_depth = None;
+
+		let mut minimum_depth = None;
+		if ver == 1 {
+			// Read the old serialization from version 0.0.98.
+			minimum_depth = Some(Readable::read(reader)?);
+		} else {
+			// Read the 4 bytes of backwards-compatibility data.
+			let _dummy: u32 = Readable::read(reader)?;
 		}
 
 		let counterparty_forwarding_info = match <u8 as Readable>::read(reader)? {
@@ -4887,6 +4898,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 			(0, announcement_sigs, option),
 			(1, minimum_depth, option),
 			(3, counterparty_selected_channel_reserve_satoshis, option),
+			(5, config, option), // Note that if none is provided we will *not* overwrite the existing one.
 		});
 
 		let mut secp_ctx = Secp256k1::new();
@@ -4895,7 +4907,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 		Ok(Channel {
 			user_id,
 
-			config,
+			config: config.unwrap(),
 			channel_id,
 			channel_state,
 			secp_ctx,

lightning/src/ln/channel.rs

@@ -1137,6 +1137,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	///
 	/// Raises APIError::APIMisuseError when channel_value_satoshis > 2**24 or push_msat is
 	/// greater than channel_value_satoshis * 1k or channel_value_satoshis is < 1000.
+	///
+	/// Note that we do not check if you are currently connected to the given peer. If no
+	/// connection is available, the outbound `open_channel` message may fail to send, resulting in
+	/// the channel eventually being silently forgotten.
 	pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, override_config: Option<UserConfig>) -> Result<(), APIError> {
 		if channel_value_satoshis < 1000 {
 			return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) });
@@ -1555,15 +1559,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			// short_channel_id is non-0 in any ::Forward.
 			if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
 				let id_option = channel_state.as_ref().unwrap().short_to_id.get(&short_channel_id).cloned();
-				let forwarding_id = match id_option {
-					None => { // unknown_next_peer
-						return_err!("Don't have available channel for forwarding as requested.", 0x4000 | 10, &[0;0]);
-					},
-					Some(id) => id.clone(),
-				};
 				if let Some((err, code, chan_update)) = loop {
+					let forwarding_id = match id_option {
+						None => { // unknown_next_peer
+							break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
+						},
+						Some(id) => id.clone(),
+					};
+
 					let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap();
 
+					if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
+						// Note that the behavior here should be identical to the above block - we
+						// should NOT reveal the existence or non-existence of a private channel if
+						// we don't allow forwards outbound over them.
+						break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
+					}
+
 					// Note that we could technically not return an error yet here and just hope
 					// that the connection is reestablished or monitor updated by the time we get
 					// around to doing the actual forward, but better to fail early if we can and
@@ -1575,7 +1587,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					if *amt_to_forward < chan.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
 						break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update_for_unicast(chan).unwrap())));
 					}
-					let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_holder_fee_base_msat(&self.fee_estimator) as u64) });
+					let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64)
+						.and_then(|prop_fee| { (prop_fee / 1000000)
+						.checked_add(chan.get_outbound_forwarding_fee_base_msat() as u64) });
 					if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
 						break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update_for_unicast(chan).unwrap())));
 					}
@@ -1660,7 +1674,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			cltv_expiry_delta: chan.get_cltv_expiry_delta(),
 			htlc_minimum_msat: chan.get_counterparty_htlc_minimum_msat(),
 			htlc_maximum_msat: OptionalField::Present(chan.get_announced_htlc_max_msat()),
-			fee_base_msat: chan.get_holder_fee_base_msat(&self.fee_estimator),
+			fee_base_msat: chan.get_outbound_forwarding_fee_base_msat(),
 			fee_proportional_millionths: chan.get_fee_proportional_millionths(),
 			excess_data: Vec::new(),
 		};
@@ -5107,7 +5121,7 @@ pub mod bench {
 		let genesis_hash = bitcoin::blockdata::constants::genesis_block(network).header.block_hash();
 
 		let tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), blocks: Arc::new(Mutex::new(Vec::new()))};
-		let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
+		let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
 
 		let mut config: UserConfig = Default::default();
 		config.own_channel_config.minimum_depth = 1;

lightning/src/ln/channelmanager.rs

@@ -269,7 +269,7 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
 			// Check that if we serialize and then deserialize all our channel monitors we get the
 			// same set of outputs to watch for on chain as we have now. Note that if we write
 			// tests that fully close channels and remove the monitors at some point this may break.
-			let feeest = test_utils::TestFeeEstimator { sat_per_kw: 253 };
+			let feeest = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
 			let mut deserialized_monitors = Vec::new();
 			{
 				let old_monitors = self.chain_monitor.chain_monitor.monitors.read().unwrap();
@@ -295,7 +295,7 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
 				<(BlockHash, ChannelManager<EnforcingSigner, &test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestLogger>)>::read(&mut ::std::io::Cursor::new(w.0), ChannelManagerReadArgs {
 					default_config: *self.node.get_current_default_configuration(),
 					keys_manager: self.keys_manager,
-					fee_estimator: &test_utils::TestFeeEstimator { sat_per_kw: 253 },
+					fee_estimator: &test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) },
 					chain_monitor: self.chain_monitor,
 					tx_broadcaster: &test_utils::TestBroadcaster {
 						txn_broadcasted: Mutex::new(self.tx_broadcaster.txn_broadcasted.lock().unwrap().clone()),
@@ -1206,7 +1206,10 @@ pub const TEST_FINAL_CLTV: u32 = 70;
 pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret) {
 	let net_graph_msg_handler = &origin_node.net_graph_msg_handler;
 	let logger = test_utils::TestLogger::new();
-	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), recv_value, TEST_FINAL_CLTV, &logger).unwrap();
+	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(),
+		&expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()),
+		Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()), &[],
+		recv_value, TEST_FINAL_CLTV, &logger).unwrap();
 	assert_eq!(route.paths.len(), 1);
 	assert_eq!(route.paths[0].len(), expected_route.len());
 	for (node, hop) in expected_route.iter().zip(route.paths[0].iter()) {
@@ -1316,7 +1319,7 @@ pub fn create_chanmon_cfgs(node_count: usize) -> Vec<TestChanMonCfg> {
 			txn_broadcasted: Mutex::new(Vec::new()),
 			blocks: Arc::new(Mutex::new(vec![(genesis_block(Network::Testnet).header, 0)])),
 		};
-		let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
+		let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
 		let chain_source = test_utils::TestChainSource::new(Network::Testnet);
 		let logger = test_utils::TestLogger::with_id(format!("node {}", i));
 		let persister = test_utils::TestPersister::new();
@@ -1341,22 +1344,29 @@ pub fn create_node_cfgs<'a>(node_count: usize, chanmon_cfgs: &'a Vec<TestChanMon
 	nodes
 }
 
+pub fn test_default_channel_config() -> UserConfig {
+	let mut default_config = UserConfig::default();
+	// Set cltv_expiry_delta slightly lower to keep the final CLTV values inside one byte in our
+	// tests so that our script-length checks don't fail (see ACCEPTED_HTLC_SCRIPT_WEIGHT).
+	default_config.channel_options.cltv_expiry_delta = 6*6;
+	default_config.channel_options.announced_channel = true;
+	default_config.peer_channel_config_limits.force_announced_channel_preference = false;
+	// When most of our tests were written, the default HTLC minimum was fixed at 1000.
+	// It now defaults to 1, so we simply set it to the expected value here.
+	default_config.own_channel_config.our_htlc_minimum_msat = 1000;
+	default_config
+}
+
 pub fn create_node_chanmgrs<'a, 'b>(node_count: usize, cfgs: &'a Vec<NodeCfg<'b>>, node_config: &[Option<UserConfig>]) -> Vec<ChannelManager<EnforcingSigner, &'a TestChainMonitor<'b>, &'b test_utils::TestBroadcaster, &'a test_utils::TestKeysInterface, &'b test_utils::TestFeeEstimator, &'b test_utils::TestLogger>> {
 	let mut chanmgrs = Vec::new();
 	for i in 0..node_count {
-		let mut default_config = UserConfig::default();
-		// Set cltv_expiry_delta slightly lower to keep the final CLTV values inside one byte in our
-		// tests so that our script-length checks don't fail (see ACCEPTED_HTLC_SCRIPT_WEIGHT).
-		default_config.channel_options.cltv_expiry_delta = 6*6;
-		default_config.channel_options.announced_channel = true;
-		default_config.peer_channel_config_limits.force_announced_channel_preference = false;
-		default_config.own_channel_config.our_htlc_minimum_msat = 1000; // sanitization being done by the sender, to exerce receiver logic we need to lift of limit
 		let network = Network::Testnet;
 		let params = ChainParameters {
 			network,
 			best_block: BestBlock::from_genesis(network),
 		};
-		let node = ChannelManager::new(cfgs[i].fee_estimator, &cfgs[i].chain_monitor, cfgs[i].tx_broadcaster, cfgs[i].logger, cfgs[i].keys_manager, if node_config[i].is_some() { node_config[i].clone().unwrap() } else { default_config }, params);
+		let node = ChannelManager::new(cfgs[i].fee_estimator, &cfgs[i].chain_monitor, cfgs[i].tx_broadcaster, cfgs[i].logger, cfgs[i].keys_manager,
+			if node_config[i].is_some() { node_config[i].clone().unwrap() } else { test_default_channel_config() }, params);
 		chanmgrs.push(node);
 	}