Switch to a max counterparty's `dust_limit_satoshis` constant #845

ariard · 2021-03-16T23:04:42Z

This is a more conservative revamp of #575, see discussion there for rational. Contrary to what we previously discussed we don't have a risk of channel closure triggered by third-party as this check is enforced at channel opening. If our counterparty announces a dust_limit_satoshis above 660 sats, we halt the opening.

TheBlueMatt

Concept ACK

lightning/src/ln/channel.rs

TheBlueMatt · 2021-03-19T23:43:41Z

Note that ln::channel::tests::channel_reestablish_no_updates and ln::channel::tests::test_holder_vs_counterparty_dust_limit are currently failing.

ariard · 2021-03-27T00:37:03Z

Fixed tests at 7c25fb0 by adding a default HOLDER_DUST_LIMIT_SATOSHIS, replacing the previous derive_holder_dust_limit_satoshis.

As this getter is function of current feerate, it doesn't guarantee to be compatible anymore with our new upper bound MAX_DUST_LIMIT_SATOSHIS. This might have lead to reject of our own channel opening.

This also removes the config option min_limit_satoshis to always requires that counterparty dust limit satoshis is between HOLDER_DUST_LIMIT_SATOSHIS and MAX_DUST_LIMIT_SATOSHIS.

I think this needs another reviewer beyond Matt.

Note also I'll manually test the new 330 satoshis limit against Core to test that my computations are good. Should be easier now we have a sample.

codecov · 2021-03-27T00:49:37Z

Codecov Report

Merging #845 (b307c1f) into main (36570f4) will increase coverage by 0.26%.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##             main     #845      +/-   ##
==========================================
+ Coverage   90.29%   90.55%   +0.26%     
==========================================
  Files          57       59       +2     
  Lines       29268    29634     +366     
==========================================
+ Hits        26427    26835     +408     
+ Misses       2841     2799      -42

Impacted Files	Coverage Δ
lightning/src/util/config.rs	`48.71% <ø> (+1.09%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.02% <94.11%> (+0.20%)`	⬆️
lightning/src/ln/channel.rs	`87.42% <100.00%> (-0.03%)`	⬇️
lightning/src/util/events.rs	`17.27% <0.00%> (-1.00%)`	⬇️
lightning-invoice/src/de.rs	`80.99% <0.00%> (-0.37%)`	⬇️
lightning/src/ln/features.rs	`98.81% <0.00%> (-0.11%)`	⬇️
lightning/src/util/test_utils.rs	`83.14% <0.00%> (-0.10%)`	⬇️
lightning/src/ln/peer_handler.rs	`44.17% <0.00%> (-0.07%)`	⬇️
lightning/src/routing/router.rs	`96.15% <0.00%> (-0.07%)`	⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36570f4...b307c1f. Read the comment docs.

TheBlueMatt · 2021-03-31T16:52:19Z

lightning/src/ln/channel.rs

@@ -511,7 +522,7 @@ impl<Signer: Sign> Channel<Signer> {
 			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) {
+		if Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(channel_value_satoshis) < HOLDER_DUST_LIMIT_SATOSHIS {


The error message here shouldn't print anything about fees anymore - the only issue, I think, is if the total channel value is < 330, plus the fee lookup one line up can be dropped.

Upgraded with a new message, right doesn't need feerate lookup anymore.

lightning/src/ln/channel.rs

valentinewallace · 2021-04-01T14:53:46Z

Hmm, I guess I'm just questioning the concept of this PR because not everyone's gonna be using bitcoind? (If I'm misunderstanding this, could someone outline the rationale more explicitly rather than a pointer to #575 ?) Plus, aren't we supposed to be the "flexible" lightning implementation? 😛 I think I'm just missing something but just trying to understand.

TheBlueMatt · 2021-04-01T18:03:49Z

not everyone's gonna be using bitcoin

As to the minimum dust limit, Bitcoin Core's relay rules are effectively consensus for us. People can run alternate nodes, but if your transaction doesn't meet Bitcoin Core's relay rules, probably it wont find its way to a miner, and even if it did the miner would have to be running something other than Bitcoin Core.

Ultimately, this PR is about addressing lightning "dust inflation" - if your counterparty sets the dust limit higher than is necessary, they can send a number of HTLCs, leave them pending, and then close the channel, burning lots of your funds to fee. We assume that lightning counterparties aren't miners largely for this reason, but ideally it wouldn't be so trivial to burn funds.

valentinewallace

I'm ACK 7c25fb0 mod fixing CI and addressing the other comments :)

valentinewallace · 2021-04-01T21:19:28Z

lightning/src/ln/channel.rs

-		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 < HOLDER_DUST_LIMIT_SATOSHIS {
+			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the user specified limit ({})", msg.dust_limit_satoshis, HOLDER_DUST_LIMIT_SATOSHIS)));


it says user-specified here

ariard · 2021-04-27T22:35:45Z

Updated with comments fixed at c418c4f. Main changes since last time is renaming HOLDER_DUST_LIMIT_SATOSHIS to MIN_DUST_LIMIT_SATOSHIS as this effectively a min required or setup by default on both holder/counterparty commitment transactions.

The only bound we don't enforce is MAX_DUST_LIMIT_SATOSHIS on our own commitment transactions, up to the counterparty to do it.

TheBlueMatt · 2021-04-28T20:31:13Z

The full_stack_target fuzz failure here looks separate from the one fixed in #902 and I assume is new in the PR here.

valentinewallace

Just one question but this looks good

lightning/src/ln/channel.rs

valentinewallace · 2021-05-03T16:12:29Z

lightning/src/ln/channel.rs

 		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 {


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

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.

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

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.

Current Bitcoin Core's policy will reject a p2wsh as a dust if it's under 330 satoshis. A typical p2wsh output is 43 bytes big to which Core's `GetDustThreshold()` sums up a minimal spend of 67 bytes (even if a p2wsh witnessScript might be smaller). `dustRelayFee` is set to 3000 sat/kb, thus 110 * 3000 / 1000 = 330. As all time-sensitive outputs are p2wsh, a value of 330 sat is the lower bound desired to ensure good propagation of transactions. We give a bit margin to our counterparty and pick up 660 satoshis as an accepted `dust_limit_satoshis` upper bound. As this reasoning is tricky and error-prone we hardcode it instead of letting the user picking up a non-sense value. Further, this lower bound of 330 sats is also hardcoded as another constant (MIN_DUST_LIMIT_SATOSHIS) instead of being dynamically computed on feerate (derive_holder_dust_limit_satoshis`). Reducing risks of non-propagating transactions in casee of failing fee festimation.

TheBlueMatt · 2021-05-03T21:15:16Z

fuzz/src/full_stack.rs

@@ -917,6 +914,5 @@ mod tests {
 		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling UpdateHTLCs event in peer_handler for node 030200000000000000000000000000000000000000000000000000000000000000 with 1 adds, 0 fulfills, 0 fails for channel 3a00000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&3)); // 7
 		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling UpdateHTLCs event in peer_handler for node 030000000000000000000000000000000000000000000000000000000000000002 with 0 adds, 1 fulfills, 0 fails for channel 3d00000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&1)); // 8
 		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling UpdateHTLCs event in peer_handler for node 030000000000000000000000000000000000000000000000000000000000000002 with 0 adds, 0 fulfills, 1 fails for channel 3d00000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&2)); // 9
-		assert_eq!(log_entries.get(&("lightning::chain::channelmonitor".to_string(), "Input spending counterparty commitment tx (0000000000000000000000000000000000000000000000000000000000000089:0) in 0000000000000000000000000000000000000000000000000000000000000074 resolves outbound HTLC with payment hash ff00000000000000000000000000000000000000000000000000000000000000 with timeout".to_string())), Some(&1)); // 10


Why do we not hit this anymore? seems like this implies we now have less coverage?

TheBlueMatt

I'll happily fix up the fuzz test after merge, I think we shouldn't hold this up on it. Looks good otherwise.

TheBlueMatt reviewed Mar 16, 2021

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

ariard force-pushed the 2021-03-hardcode-dust branch from 563bb6d to 944cdaf Compare March 19, 2021 22:18

ariard force-pushed the 2021-03-hardcode-dust branch from 3f534f8 to 7c25fb0 Compare March 27, 2021 00:39

TheBlueMatt reviewed Mar 31, 2021

View reviewed changes

valentinewallace reviewed Apr 1, 2021

View reviewed changes

TheBlueMatt added this to the 0.0.14 milestone Apr 5, 2021

ariard force-pushed the 2021-03-hardcode-dust branch from 7c25fb0 to 0057afa Compare April 27, 2021 22:33

ariard force-pushed the 2021-03-hardcode-dust branch from 0057afa to c418c4f Compare April 27, 2021 22:36

ariard force-pushed the 2021-03-hardcode-dust branch from 6a3608f to e9c626f Compare May 1, 2021 00:24

TheBlueMatt mentioned this pull request May 1, 2021

Bump versions to 0.0.14, lightning-invoice 0.5 #909

Merged

valentinewallace reviewed May 3, 2021

View reviewed changes

Antoine Riard added 2 commits May 3, 2021 15:37

Add bolt2_open_channel_sane_dust_limit

ce56e3f

ariard force-pushed the 2021-03-hardcode-dust branch from e9c626f to ce56e3f Compare May 3, 2021 19:37

TheBlueMatt reviewed May 3, 2021

View reviewed changes

TheBlueMatt approved these changes May 3, 2021

View reviewed changes

valentinewallace approved these changes May 3, 2021

View reviewed changes

Make third HTLC non-dust again in the full_stack_target demo seed

b307c1f

TheBlueMatt merged commit 58e4ce2 into lightningdevkit:main May 4, 2021

TheBlueMatt mentioned this pull request May 9, 2021

Check dust inflation #287

Closed

SomberNight mentioned this pull request Oct 25, 2021

Implement recent spec changes regarding collab channel close outputs spesmilo/electrum#7542

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to a max counterparty's `dust_limit_satoshis` constant #845

Switch to a max counterparty's `dust_limit_satoshis` constant #845

ariard commented Mar 16, 2021

TheBlueMatt left a comment

TheBlueMatt commented Mar 19, 2021

ariard commented Mar 27, 2021 •

edited

Loading

codecov bot commented Mar 27, 2021 •

edited

Loading

TheBlueMatt Mar 31, 2021

ariard Apr 27, 2021

valentinewallace commented Apr 1, 2021

TheBlueMatt commented Apr 1, 2021

valentinewallace left a comment

valentinewallace Apr 1, 2021

ariard commented Apr 27, 2021 •

edited

Loading

TheBlueMatt commented Apr 28, 2021

valentinewallace left a comment

valentinewallace May 3, 2021

ariard May 3, 2021 •

edited

Loading

valentinewallace May 3, 2021

ariard May 3, 2021

TheBlueMatt May 3, 2021

TheBlueMatt left a comment

Switch to a max counterparty's dust_limit_satoshis constant #845

Switch to a max counterparty's dust_limit_satoshis constant #845

Conversation

ariard commented Mar 16, 2021

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 19, 2021

ariard commented Mar 27, 2021 • edited Loading

codecov bot commented Mar 27, 2021 • edited Loading

Codecov Report

TheBlueMatt Mar 31, 2021

Choose a reason for hiding this comment

ariard Apr 27, 2021

Choose a reason for hiding this comment

valentinewallace commented Apr 1, 2021

TheBlueMatt commented Apr 1, 2021

valentinewallace left a comment

Choose a reason for hiding this comment

valentinewallace Apr 1, 2021

Choose a reason for hiding this comment

ariard commented Apr 27, 2021 • edited Loading

TheBlueMatt commented Apr 28, 2021

valentinewallace left a comment

Choose a reason for hiding this comment

valentinewallace May 3, 2021

Choose a reason for hiding this comment

ariard May 3, 2021 • edited Loading

Choose a reason for hiding this comment

valentinewallace May 3, 2021

Choose a reason for hiding this comment

ariard May 3, 2021

Choose a reason for hiding this comment

TheBlueMatt May 3, 2021

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Switch to a max counterparty's `dust_limit_satoshis` constant #845

Switch to a max counterparty's `dust_limit_satoshis` constant #845

ariard commented Mar 27, 2021 •

edited

Loading

codecov bot commented Mar 27, 2021 •

edited

Loading

ariard commented Apr 27, 2021 •

edited

Loading

ariard May 3, 2021 •

edited

Loading