Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to a max counterparty's dust_limit_satoshis constant #845

Merged
merged 3 commits into from
May 4, 2021
Merged

Switch to a max counterparty's dust_limit_satoshis constant #845

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
16619ff
Replace config max counterpary `dust_limit_satoshis` by a constant.
ariard Mar 16, 2021
ce56e3f
Add bolt2_open_channel_sane_dust_limit
ariard Apr 8, 2020
b307c1f
Make third HTLC non-dust again in the full_stack_target demo seed
TheBlueMatt May 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,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,
Expand All @@ -339,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 mut monitors = HashMap::new();
let mut old_monitors = $old_monitors.latest_monitors.lock().unwrap();
Expand Down
20 changes: 8 additions & 12 deletions fuzz/src/full_stack.rs
View file
Open in desktop

Large diffs are not rendered by default.

58 changes: 34 additions & 24 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,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;
Expand All @@ -453,6 +452,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
ariard marked this conversation as resolved.
Show resolved Hide resolved
/// 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.
Expand Down Expand Up @@ -496,10 +511,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>,
Expand All @@ -519,9 +530,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);
Expand Down Expand Up @@ -578,7 +589,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,
Expand Down Expand Up @@ -699,11 +710,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:
Expand All @@ -719,13 +730,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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we select the holder_selected_channel_reserve_satoshis, could we just ensure that we never select a reserve below MIN_DUST_LIMIT_SATOSHIS?

Copy link
Author

@ariard ariard May 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean instead of returning an API error, rouding up the channel_reserve_satoshis with MIN_DUST_LIMIT_SATOSHIS.

I think I prefer the user to swallow the error and having manually to bounce up the channel value instead of us doing it automatically. We might silently encroach on its expected liquidity ready to use and falsify higher application logic like an accounting app... Though not a strong opinion here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just seem like it doesn't make sense for get_holder_selected_channel_reserve_satoshis to ever return a value less than 330. But, fine to leave that for follow-up

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think our API prevent a user to try a new_outbound with less than 330 sat ? And if does so get_holder_selected_channel_reserve_satoshis will return the exact value.

That said there is a TODO to make more sense of get_holder_selected_channel_reserve_satoshis. We can address it at that time.

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)));
Expand Down Expand Up @@ -817,7 +827,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,
Expand Down Expand Up @@ -1441,11 +1451,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)));
Expand Down Expand Up @@ -4935,14 +4945,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.
Expand Down
27 changes: 26 additions & 1 deletion lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1792,7 +1792,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.
Expand Down Expand Up @@ -5927,6 +5927,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
Expand Down
18 changes: 0 additions & 18 deletions lightning/src/util/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down Expand Up @@ -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,
Expand Down