Merge pull request #845 from ariard/2021-03-hardcode-dust

Switch to a max counterparty's `dust_limit_satoshis` constant

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -338,7 +338,6 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 			let mut config = UserConfig::default();
 			config.channel_options.fee_proportional_millionths = 0;
 			config.channel_options.announced_channel = true;
-			config.peer_channel_config_limits.min_dust_limit_satoshis = 0;
 			let network = Network::Bitcoin;
 			let params = ChainParameters {
 				network,
@@ -358,7 +357,6 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 			let mut config = UserConfig::default();
 			config.channel_options.fee_proportional_millionths = 0;
 			config.channel_options.announced_channel = true;
-			config.peer_channel_config_limits.min_dust_limit_satoshis = 0;
 
 			let mut monitors = HashMap::new();
 			let mut old_monitors = $old_monitors.latest_monitors.lock().unwrap();

fuzz/src/full_stack.rs

@@ -439,7 +439,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
-const B_OUTPUT_PLUS_SPENDING_INPUT_WEIGHT: u64 = 104; // prevout: 40, nSequence: 4, script len: 1, witness lengths: 3/4, sig: 73/4, pubkey: 33/4, output: 31 (TODO: Wrong? Useless?)
 
 #[cfg(not(test))]
 const COMMITMENT_TX_BASE_WEIGHT: u64 = 724;
@@ -454,6 +453,22 @@ pub const COMMITMENT_TX_WEIGHT_PER_HTLC: u64 = 172;
 /// it's 2^24.
 pub const MAX_FUNDING_SATOSHIS: u64 = 1 << 24;
 
+/// Maximum counterparty `dust_limit_satoshis` allowed. 2 * standard dust threshold on p2wsh output
+/// Scales up on Bitcoin Core's proceeding policy with dust outputs. A typical p2wsh output is 43
+/// bytes to which Core's `GetDustThreshold()` sums up a minimal spend of 67 bytes (even if
+/// a p2wsh witnessScript might be *effectively* smaller), `dustRelayFee` is set to 3000sat/kb, thus
+/// 110 * 3000 / 1000 = 330. Per-protocol rules, all time-sensitive outputs are p2wsh, a value of
+/// 330 sats is the lower bound desired to ensure good propagation of transactions. We give a bit
+/// of margin to our counterparty and pick up 660 satoshis as an accepted `dust_limit_satoshis`
+/// upper bound to avoid negotiation conflicts with other implementations.
+pub const MAX_DUST_LIMIT_SATOSHIS: u64 = 2 * 330;
+
+/// A typical p2wsh output is 43 bytes to which Core's `GetDustThreshold()` sums up a minimal
+/// spend of 67 bytes (even if a p2wsh witnessScript might be *effectively* smaller), `dustRelayFee`
+/// is set to 3000sat/kb, thus 110 * 3000 / 1000 = 330. Per-protocol rules, all time-sensitive outputs
+/// are p2wsh, a value of 330 sats is the lower bound desired to ensure good propagation of transactions.
+pub const MIN_DUST_LIMIT_SATOSHIS: u64 = 330;
+
 /// Used to return a simple Error back to ChannelManager. Will get converted to a
 /// msgs::ErrorAction::SendErrorMessage or msgs::ErrorAction::IgnoreError as appropriate with our
 /// channel_id in ChannelManager.
@@ -497,10 +512,6 @@ impl<Signer: Sign> Channel<Signer> {
 		cmp::min(channel_value_satoshis, cmp::max(q, 1000)) //TODO
 	}
 
-	fn derive_holder_dust_limit_satoshis(at_open_background_feerate: u32) -> u64 {
-		cmp::max(at_open_background_feerate as u64 * B_OUTPUT_PLUS_SPENDING_INPUT_WEIGHT / 1000, 546) //TODO
-	}
-
 	// Constructors:
 	pub fn new_outbound<K: Deref, F: Deref>(fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, config: &UserConfig) -> Result<Channel<Signer>, APIError>
 	where K::Target: KeysInterface<Signer = Signer>,
@@ -520,9 +531,9 @@ impl<Signer: Sign> Channel<Signer> {
 		if holder_selected_contest_delay < BREAKDOWN_TIMEOUT {
 			return Err(APIError::APIMisuseError {err: format!("Configured with an unreasonable our_to_self_delay ({}) putting user funds at risks", holder_selected_contest_delay)});
 		}
-		let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
-		if Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(channel_value_satoshis) < Channel::<Signer>::derive_holder_dust_limit_satoshis(background_feerate) {
-			return Err(APIError::FeeRateTooHigh{err: format!("Not enough reserve above dust limit can be found at current fee rate({})", background_feerate), feerate: background_feerate});
+		let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(channel_value_satoshis);
+		if holder_selected_channel_reserve_satoshis < MIN_DUST_LIMIT_SATOSHIS {
+			return Err(APIError::APIMisuseError { err: format!("Holder selected channel  reserve below implemention limit dust_limit_satoshis {}", holder_selected_channel_reserve_satoshis) });
 		}
 
 		let feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
@@ -579,7 +590,7 @@ impl<Signer: Sign> Channel<Signer> {
 
 			feerate_per_kw: feerate,
 			counterparty_dust_limit_satoshis: 0,
-			holder_dust_limit_satoshis: Channel::<Signer>::derive_holder_dust_limit_satoshis(background_feerate),
+			holder_dust_limit_satoshis: MIN_DUST_LIMIT_SATOSHIS,
 			counterparty_max_htlc_value_in_flight_msat: 0,
 			counterparty_selected_channel_reserve_satoshis: 0,
 			counterparty_htlc_minimum_msat: 0,
@@ -700,11 +711,11 @@ impl<Signer: Sign> Channel<Signer> {
 		if msg.max_accepted_htlcs < config.peer_channel_config_limits.min_max_accepted_htlcs {
 			return Err(ChannelError::Close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", msg.max_accepted_htlcs, config.peer_channel_config_limits.min_max_accepted_htlcs)));
 		}
-		if msg.dust_limit_satoshis < config.peer_channel_config_limits.min_dust_limit_satoshis {
-			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the user specified limit ({})", msg.dust_limit_satoshis, config.peer_channel_config_limits.min_dust_limit_satoshis)));
+		if msg.dust_limit_satoshis < MIN_DUST_LIMIT_SATOSHIS {
+			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.dust_limit_satoshis, MIN_DUST_LIMIT_SATOSHIS)));
 		}
-		if msg.dust_limit_satoshis > config.peer_channel_config_limits.max_dust_limit_satoshis {
-			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the user specified limit ({})", msg.dust_limit_satoshis, config.peer_channel_config_limits.max_dust_limit_satoshis)));
+		if msg.dust_limit_satoshis >  MAX_DUST_LIMIT_SATOSHIS {
+			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.dust_limit_satoshis, MAX_DUST_LIMIT_SATOSHIS)));
 		}
 
 		// Convert things into internal flags and prep our state:
@@ -720,13 +731,12 @@ impl<Signer: Sign> Channel<Signer> {
 
 		let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
 
-		let holder_dust_limit_satoshis = Channel::<Signer>::derive_holder_dust_limit_satoshis(background_feerate);
 		let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(msg.funding_satoshis);
-		if holder_selected_channel_reserve_satoshis < holder_dust_limit_satoshis {
-			return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). dust_limit_satoshis is ({}).", holder_selected_channel_reserve_satoshis, holder_dust_limit_satoshis)));
+		if holder_selected_channel_reserve_satoshis < MIN_DUST_LIMIT_SATOSHIS {
+			return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). dust_limit_satoshis is ({}).", holder_selected_channel_reserve_satoshis, MIN_DUST_LIMIT_SATOSHIS)));
 		}
-		if msg.channel_reserve_satoshis < holder_dust_limit_satoshis {
-			return Err(ChannelError::Close(format!("channel_reserve_satoshis ({}) is smaller than our dust limit ({})", msg.channel_reserve_satoshis, holder_dust_limit_satoshis)));
+		if msg.channel_reserve_satoshis < MIN_DUST_LIMIT_SATOSHIS {
+			return Err(ChannelError::Close(format!("channel_reserve_satoshis ({}) is smaller than our dust limit ({})", msg.channel_reserve_satoshis, MIN_DUST_LIMIT_SATOSHIS)));
 		}
 		if holder_selected_channel_reserve_satoshis < msg.dust_limit_satoshis {
 			return Err(ChannelError::Close(format!("Dust limit ({}) too high for the channel reserve we require the remote to keep ({})", msg.dust_limit_satoshis, holder_selected_channel_reserve_satoshis)));
@@ -818,7 +828,7 @@ impl<Signer: Sign> Channel<Signer> {
 			feerate_per_kw: msg.feerate_per_kw,
 			channel_value_satoshis: msg.funding_satoshis,
 			counterparty_dust_limit_satoshis: msg.dust_limit_satoshis,
-			holder_dust_limit_satoshis,
+			holder_dust_limit_satoshis: MIN_DUST_LIMIT_SATOSHIS,
 			counterparty_max_htlc_value_in_flight_msat: cmp::min(msg.max_htlc_value_in_flight_msat, msg.funding_satoshis * 1000),
 			counterparty_selected_channel_reserve_satoshis: msg.channel_reserve_satoshis,
 			counterparty_htlc_minimum_msat: msg.htlc_minimum_msat,
@@ -1442,11 +1452,11 @@ impl<Signer: Sign> Channel<Signer> {
 		if msg.max_accepted_htlcs < config.peer_channel_config_limits.min_max_accepted_htlcs {
 			return Err(ChannelError::Close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", msg.max_accepted_htlcs, config.peer_channel_config_limits.min_max_accepted_htlcs)));
 		}
-		if msg.dust_limit_satoshis < config.peer_channel_config_limits.min_dust_limit_satoshis {
-			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the user specified limit ({})", msg.dust_limit_satoshis, config.peer_channel_config_limits.min_dust_limit_satoshis)));
+		if msg.dust_limit_satoshis < MIN_DUST_LIMIT_SATOSHIS {
+			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.dust_limit_satoshis, MIN_DUST_LIMIT_SATOSHIS)));
 		}
-		if msg.dust_limit_satoshis > config.peer_channel_config_limits.max_dust_limit_satoshis {
-			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the user specified limit ({})", msg.dust_limit_satoshis, config.peer_channel_config_limits.max_dust_limit_satoshis)));
+		if msg.dust_limit_satoshis > MAX_DUST_LIMIT_SATOSHIS {
+			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.dust_limit_satoshis, MAX_DUST_LIMIT_SATOSHIS)));
 		}
 		if msg.minimum_depth > config.peer_channel_config_limits.max_minimum_depth {
 			return Err(ChannelError::Close(format!("We consider the minimum depth to be unreasonably large. Expected minimum: ({}). Actual: ({})", config.peer_channel_config_limits.max_minimum_depth, msg.minimum_depth)));
@@ -4939,14 +4949,14 @@ mod tests {
 		// Create Node B's channel by receiving Node A's open_channel message
 		// Make sure A's dust limit is as we expect.
 		let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash());
-		assert_eq!(open_channel_msg.dust_limit_satoshis, 1560);
 		let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
 		let node_b_chan = Channel::<EnforcingSigner>::new_from_req(&&feeest, &&keys_provider, node_b_node_id, InitFeatures::known(), &open_channel_msg, 7, &config).unwrap();
 
 		// Node B --> Node A: accept channel, explicitly setting B's dust limit.
 		let mut accept_channel_msg = node_b_chan.get_accept_channel();
 		accept_channel_msg.dust_limit_satoshis = 546;
 		node_a_chan.accept_channel(&accept_channel_msg, &config, InitFeatures::known()).unwrap();
+		node_a_chan.holder_dust_limit_satoshis = 1560;
 
 		// Put some inbound and outbound HTLCs in A's channel.
 		let htlc_amount_msat = 11_092_000; // put an amount below A's effective dust limit but above B's.

lightning/src/ln/channel.rs

@@ -1764,7 +1764,7 @@ fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() {
 	// transaction fee with 0 HTLCs (183 sats)).
 	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 98817000, InitFeatures::known(), InitFeatures::known());
 
-	let dust_amt = 546000; // Dust amount
+	let dust_amt = 329000; // Dust amount
 	// In the previous code, routing this dust payment would cause nodes[0] to perceive a channel
 	// reserve violation even though it's a dust HTLC and therefore shouldn't count towards the
 	// commitment transaction fee.
@@ -5888,6 +5888,31 @@ fn bolt2_open_channel_sending_node_checks_part2() {
 	assert!(PublicKey::from_slice(&node0_to_1_send_open_channel.delayed_payment_basepoint.serialize()).is_ok());
 }
 
+#[test]
+fn bolt2_open_channel_sane_dust_limit() {
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let channel_value_satoshis=1000000;
+	let push_msat=10001;
+	nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), channel_value_satoshis, push_msat, 42, None).unwrap();
+	let mut node0_to_1_send_open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+	node0_to_1_send_open_channel.dust_limit_satoshis = 661;
+	node0_to_1_send_open_channel.channel_reserve_satoshis = 100001;
+
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &node0_to_1_send_open_channel);
+	let events = nodes[1].node.get_and_clear_pending_msg_events();
+	let err_msg = match events[0] {
+		MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, node_id: _ } => {
+			msg.clone()
+		},
+		_ => panic!("Unexpected event"),
+	};
+	assert_eq!(err_msg.data, "dust_limit_satoshis (661) is greater than the implementation limit (660)");
+}
+
 // Test that if we fail to send an HTLC that is being freed from the holding cell, and the HTLC
 // originated from our node, its failure is surfaced to the user. We trigger this failure to
 // free the HTLC by increasing our fee while the HTLC is in the holding cell such that the HTLC

lightning/src/ln/functional_tests.rs

@@ -98,22 +98,6 @@ pub struct ChannelHandshakeLimits {
 	///
 	/// Default value: 0.
 	pub min_max_accepted_htlcs: u16,
-	/// Outputs below a certain value will not be added to on-chain transactions. The dust value is
-	/// required to always be higher than this value so this only applies to HTLC outputs (and
-	/// potentially to-self outputs before any payments have been made).
-	/// Thus, HTLCs below this amount plus HTLC transaction fees are not enforceable on-chain.
-	/// This setting allows you to set a minimum dust limit for their commitment transactions,
-	/// reflecting the reality that tiny outputs are not considered standard transactions and will
-	/// not propagate through the Bitcoin network.
-	///
-	/// Default value: 546, the current dust limit on the Bitcoin network.
-	pub min_dust_limit_satoshis: u64,
-	/// Maximum allowed threshold above which outputs will not be generated in their commitment
-	/// transactions.
-	/// HTLCs below this amount plus HTLC transaction fees are not enforceable on-chain.
-	///
-	/// Default value: u64::max_value.
-	pub max_dust_limit_satoshis: u64,
 	/// Before a channel is usable the funding transaction will need to be confirmed by at least a
 	/// certain number of blocks, specified by the node which is not the funder (as the funder can
 	/// assume they aren't going to double-spend themselves).
@@ -145,8 +129,6 @@ impl Default for ChannelHandshakeLimits {
 			min_max_htlc_value_in_flight_msat: 0,
 			max_channel_reserve_satoshis: <u64>::max_value(),
 			min_max_accepted_htlcs: 0,
-			min_dust_limit_satoshis: 546,
-			max_dust_limit_satoshis: <u64>::max_value(),
 			max_minimum_depth: 144,
 			force_announced_channel_preference: true,
 			their_to_self_delay: MAX_LOCAL_BREAKDOWN_TIMEOUT,