Skip to content

Enforce a bounded max dust_limit_satoshis on remote transactions #575

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

Closed
ariard wants to merge 2 commits into from
Closed

Enforce a bounded max dust_limit_satoshis on remote transactions #575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a9def6d
Bound value of max_dust_limit_satoshis to 10*bitcoin network dust
ariard Apr 8, 2020
31a9ee9
Add bolt2_open_channel_sane_dust_limit
ariard Apr 8, 2020
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
25 changes: 25 additions & 0 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5743,6 +5743,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 = 100000;
node0_to_1_send_open_channel.channel_reserve_satoshis = 100001;

nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::supported(), &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 is greater than the user specified limit");
}

// BOLT 2 Requirements for the Sender when constructing and sending an update_add_htlc message.
// BOLT 2 Requirement: MUST NOT offer amount_msat it cannot pay for in the remote commitment transaction at the current feerate_per_kw (see "Updating Fees") while maintaining its channel reserve.
//TODO: I don't believe this is explicitly enforced when sending an HTLC but as the Fee aspect of the BOLT specs is in flux leaving this as a TODO.
Expand Down
9 changes: 4 additions & 5 deletions lightning/src/util/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,10 @@ pub struct ChannelHandshakeLimits {
///
/// 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.
/// Maximum allowed value for considering an output as dust on their commitment+HTLC
/// transactions, above which channel opening is rejected.
///
/// Default value: u64::max_value.
/// Default value: 10*546
Copy link
Copy Markdown
Collaborator

@TheBlueMatt TheBlueMatt Apr 9, 2020

Choose a reason for hiding this comment

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

Hmm. Why 10? Can you write up what the fee costs are of claiming an HTLC on-chain and then provide some guidance for what this value should be at different fee levels? eg if fees go up 100x, 10*546 would likely be pretty low, no?

Copy link
Copy Markdown
Author

@ariard ariard Apr 9, 2020

Choose a reason for hiding this comment

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

Okay here full-rational.

Let's say Mallory announces a channel to Alice with dust-limit-satoshi set to 20% of channel value. Alice should accept incoming channels as long as its under her max_dust_limit_satoshis. Now Mallory can route 4 dust-HTLCs to Bob through Alice which will claim ~80% of channel value (dust_limit_satoshis MUST be under channel_reserve).

Bob, in collusion with Mallory, claim the whole set of HTLCs and at same moment Mallory broadcast her latest commitment transaction on which there is HTLCs because all of them are dust. Alice can't claim them onchain but has paid Bob forward.

As we talked before, this attack is at first-look not economically rational because dust-HTLCs are committed as fees. But if you assume Mallory can collude with some mining pool, economics change completely because it's a zero_cost to add Mallory commitment transaction in a block, hashrate won't be wasted. Fees are going back to miner, and Alice is still robbed. Mallory commitment transaction may stay in miner pool as long block isn't found and HTLCs timelocks don't expire. And attack may still stealth if block isn't signed.

It's that kind low-probability-and-hard-to-exploit-vulnerability but still you prefer not to have to think about your big LSP hub being targeted by a rogue mining pool employee.

To avoid this, max dust_limit_satoshis must be pretty low, max value open to exploitation is of course max_htlc_value_in_flight_msat but you can still offer dust-htlc in parallel to try to reach it. And as far as I remember we don't limit number of offered htlcs to us, as some implementation are doing.

Back to the pure fact of fee spikes, I think that's a spec default, dust_limit_satoshis should be a floating channel parameter to mirror dynamic conditions. Maybe we should take opportunity of dynamic channel upgrade introduced for anchor_ouputs to also integrate this. I'm not setup on 10*546 and we may have instead a max_trusted_dust_value from which to reason on.

Copy link
Copy Markdown
Collaborator

@TheBlueMatt TheBlueMatt Apr 15, 2020

Choose a reason for hiding this comment

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

Nice writeup, can you put it in a doc here, though :). I agree with you - this should mostly be dynamic, which also implies we should tell the users how to set it (probably X*BackgroundFeerate).

re: max htlcs: we should expose it. its currently set by the OUR_MAX_HTLCs constant in channel.rs.

re: floating dust value: true, though it only kinda requries a spec update. Its somewhat anti-social, but in theory we could try to get this line removed from the spec (and only indicate that it must be irrevocably committed in the senders commitment tx) and then fail HTLCs below a floating dust limit. I'm not sure this is actually a good idea (we should, instead, just update the htlc_minimum_msat and then bounce dust HTLCs, really).

until the corresponding HTLC is irrevocably committed in both sides' commitment transactions: MUST NOT send an update_fulfill_htlc, update_fail_htlc, or update_fail_malformed_htlc.

Copy link
Copy Markdown
Contributor

@naumenkogs naumenkogs Apr 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

+1 to keep the whole bad miners reasoning (but really it's about aggregating dust) somewhere for the reference.

because it's a zero_cost to add Mallory commitment transaction in a block, hashrate won't be wasted

The opportunity to mine some other transaction is lost, right? (assuming the blocks are full). So technically there is a cost I think.

Overall I generally agree with everything.

Copy link
Copy Markdown
Author

@ariard ariard Apr 20, 2020

Choose a reason for hiding this comment

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

The opportunity to mine some other transaction is lost, right? (assuming the blocks are full). So technically there is a cost I think.

With current static-fee model, commitment should have competitive fees, and you may not have to bear them if it's not your opening. But yeah assuming attacker is opening channel, there is a slight loss, but if you can reap until max_htlc_value_in_flight, and with wumbo_channel I expect this to increase with time.

Note, as with most LN attacks, attacker is limited by staking coins but for a mining-pool you likely assume they have fresh coins available.

Note, currently max CLTV network value is 2048 blocks IIRC, but even if you mining pool doesn't find a block in this range, you can reset a long-lived HTLC through the network even before block 2048+1. So this attack seems doable by any entity who has reasonable probability to mine a block ever ?

Copy link
Copy Markdown
Collaborator

@TheBlueMatt TheBlueMatt May 18, 2020

Choose a reason for hiding this comment

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

Any plans to update the doc?

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
Expand Down Expand Up @@ -158,7 +157,7 @@ impl Default for ChannelHandshakeLimits {
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_dust_limit_satoshis: 10*546,
max_minimum_depth: 144,
force_announced_channel_preference: true,
their_to_self_delay: MAX_LOCAL_BREAKDOWN_TIMEOUT,
Expand Down