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

Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom
ariard:2021-03-hardcode-dust
May 4, 2021
Merged

Switch to a max counterparty's dust_limit_satoshis constant#845
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom
ariard:2021-03-hardcode-dust

Conversation

@ariard

@ariardariard commented Mar 16, 2021

Copy link
Copy Markdown

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
TheBlueMatt reviewed Mar 16, 2021
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Concept ACK

Comment threadlightning/src/ln/channel.rs
@TheBlueMatt

TheBlueMatt commented Mar 19, 2021

Copy link
Copy Markdown
Collaborator

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

@ariard

ariard commented Mar 27, 2021•
edited
Loading

Copy link
Copy Markdown
Author

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

codecovBot commented Mar 27, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@ 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 FilesCoverage Δ
lightning/src/util/config.rs48.71% <ø> (+1.09%)⬆️
lightning/src/ln/functional_tests.rs97.02% <94.11%> (+0.20%)⬆️
lightning/src/ln/channel.rs87.42% <100.00%> (-0.03%)⬇️
lightning/src/util/events.rs17.27% <0.00%> (-1.00%)⬇️
lightning-invoice/src/de.rs80.99% <0.00%> (-0.37%)⬇️
lightning/src/ln/features.rs98.81% <0.00%> (-0.11%)⬇️
lightning/src/util/test_utils.rs83.14% <0.00%> (-0.10%)⬇️
lightning/src/ln/peer_handler.rs44.17% <0.00%> (-0.07%)⬇️
lightning/src/routing/router.rs96.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
TheBlueMatt reviewed Mar 31, 2021
View reviewed changes
Comment threadlightning/src/ln/channel.rs Outdated
}
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 {

@TheBlueMattTheBlueMattMar 31, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@ariardariardApr 27, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Comment threadlightning/src/ln/channel.rs Outdated
@valentinewallace

valentinewallace commented Apr 1, 2021

Copy link
Copy Markdown
Contributor

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

TheBlueMatt commented Apr 1, 2021

Copy link
Copy Markdown
Collaborator

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
valentinewallace reviewed Apr 1, 2021
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadlightning/src/ln/channel.rs Outdated
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)));

@valentinewallacevalentinewallaceApr 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it says user-specified here

@TheBlueMattTheBlueMatt added this to the 0.0.14 milestone Apr 5, 2021
@ariard

ariard commented Apr 27, 2021•
edited
Loading

Copy link
Copy Markdown
Author

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

TheBlueMatt commented Apr 28, 2021

Copy link
Copy Markdown
Collaborator

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

@TheBlueMattTheBlueMatt mentioned this pull request May 1, 2021
Merged
valentinewallace
valentinewallace reviewed May 3, 2021
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just one question but this looks good

Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/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 {

@valentinewallacevalentinewallaceMay 3, 2021

Copy link
Copy Markdown
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?

@ariardariardMay 3, 2021•
edited
Loading

Copy link
Copy Markdown
Author

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.

@valentinewallacevalentinewallaceMay 3, 2021

Copy link
Copy Markdown
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

@ariardariardMay 3, 2021

Copy link
Copy Markdown
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.

Antoine Riard added 2 commits May 3, 2021 15:37
@ariard
Replace config max counterpary dust_limit_satoshis by a constant.
16619ff
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.
@ariard
Add bolt2_open_channel_sane_dust_limit
ce56e3f
TheBlueMatt
TheBlueMatt reviewed May 3, 2021
View reviewed changes
Comment threadfuzz/src/full_stack.rs
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

@TheBlueMattTheBlueMattMay 3, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

TheBlueMatt
TheBlueMatt approved these changes May 3, 2021
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@TheBlueMatt
TheBlueMatt merged commit 58e4ce2 into lightningdevkit:mainMay 4, 2021
@TheBlueMattTheBlueMatt mentioned this pull request May 9, 2021
Closed
@SomberNightSomberNight mentioned this pull request Oct 25, 2021
Merged
3 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.0.14

Development

Successfully merging this pull request may close these issues.

3 participants

@ariard@TheBlueMatt@valentinewallace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Switch to a max counterparty's dust_limit_satoshis constant - #845
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
May 4, 2021
Merged
Switch to a max counterparty's dust_limit_satoshis constant#845
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
Conversation
@ariard
@ariardariard
commented
Mar 16, 2021

Copy link


Copy Markdown

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
TheBlueMatt
reviewed
Mar 16, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Concept ACK
Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMatt
commented
Mar 19, 2021

Copy link


Copy Markdown

Collaborator
Note that ln::channel::tests::channel_reestablish_no_updates and ln::channel::tests::test_holder_vs_counterparty_dust_limit are currently failing.
@ariard
ariard
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
codecovBot
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #845 (b307c1f) into main (36570f4) will increase coverage by 0.26%.

The diff coverage is 96.29%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
lightning/src/util/config.rs48.71% <ø> (+1.09%)⬆️
lightning/src/ln/functional_tests.rs97.02% <94.11%> (+0.20%)⬆️
lightning/src/ln/channel.rs87.42% <100.00%> (-0.03%)⬇️
lightning/src/util/events.rs17.27% <0.00%> (-1.00%)⬇️
lightning-invoice/src/de.rs80.99% <0.00%> (-0.37%)⬇️
lightning/src/ln/features.rs98.81% <0.00%> (-0.11%)⬇️
lightning/src/util/test_utils.rs83.14% <0.00%> (-0.10%)⬇️
lightning/src/ln/peer_handler.rs44.17% <0.00%> (-0.07%)⬇️
lightning/src/routing/router.rs96.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
TheBlueMatt
reviewed
Mar 31, 2021
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

 }
 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 {
@TheBlueMattTheBlueMattMar 31, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
@ariardariardApr 27, 2021

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Upgraded with a new message, right doesn't need feerate lookup anymore.
Comment threadlightning/src/ln/channel.rs
Outdated

@valentinewallace
valentinewallace
commented
Apr 1, 2021

Copy link


Copy Markdown

Contributor
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
TheBlueMatt
commented
Apr 1, 2021

Copy link


Copy Markdown

Collaborator
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
valentinewallace
reviewed
Apr 1, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm ACK 7c25fb0 mod fixing CI and addressing the other comments :)
Comment threadlightning/src/ln/channel.rs
Outdated

 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)));
@valentinewallacevalentinewallaceApr 1, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it says user-specified here
@TheBlueMattTheBlueMatt
added this to the 0.0.14 milestone
Apr 5, 2021
@ariard
ariard
commented
Apr 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
TheBlueMatt
commented
Apr 28, 2021

Copy link


Copy Markdown

Collaborator
The full_stack_target fuzz failure here looks separate from the one fixed in #902 and I assume is new in the PR here.
@TheBlueMattTheBlueMatt
mentioned this pull request
May 1, 2021
 Merged

valentinewallace
valentinewallace
reviewed
May 3, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one question but this looks good
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/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 {
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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?
@ariardariardMay 3, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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.
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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
@ariardariardMay 3, 2021

Copy link


Copy Markdown

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.
Antoine Riard
added 2 commits
May 3, 2021 15:37
@ariard
Replace config max counterpary dust_limit_satoshis by a constant.
16619ff
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.
@ariard
Add bolt2_open_channel_sane_dust_limit
ce56e3f
TheBlueMatt
TheBlueMatt
reviewed
May 3, 2021
View reviewed changes
Comment threadfuzz/src/full_stack.rs
 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
@TheBlueMattTheBlueMattMay 3, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Why do we not hit this anymore? seems like this implies we now have less coverage?
TheBlueMatt
TheBlueMatt
approved these changes
May 3, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

I'll happily fix up the fuzz test after merge, I think we shouldn't hold this up on it. Looks good otherwise.
@TheBlueMatt
TheBlueMatt
merged commit 58e4ce2
into
lightningdevkit:mainMay 4, 2021
@TheBlueMattTheBlueMatt
mentioned this pull request
May 9, 2021

Closed

@SomberNightSomberNight
mentioned this pull request
Oct 25, 2021
 Merged

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.14

Development

Successfully merging this pull request may close these issues.

3 participants

@ariard@TheBlueMatt@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Switch to a max counterparty's dust_limit_satoshis constant - #845
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
May 4, 2021
Merged
Switch to a max counterparty's dust_limit_satoshis constant#845
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
Conversation
@ariard
@ariardariard
commented
Mar 16, 2021

Copy link


Copy Markdown

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
TheBlueMatt
reviewed
Mar 16, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Concept ACK
Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMatt
commented
Mar 19, 2021

Copy link


Copy Markdown

Collaborator
Note that ln::channel::tests::channel_reestablish_no_updates and ln::channel::tests::test_holder_vs_counterparty_dust_limit are currently failing.
@ariard
ariard
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
codecovBot
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #845 (b307c1f) into main (36570f4) will increase coverage by 0.26%.

The diff coverage is 96.29%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
lightning/src/util/config.rs48.71% <ø> (+1.09%)⬆️
lightning/src/ln/functional_tests.rs97.02% <94.11%> (+0.20%)⬆️
lightning/src/ln/channel.rs87.42% <100.00%> (-0.03%)⬇️
lightning/src/util/events.rs17.27% <0.00%> (-1.00%)⬇️
lightning-invoice/src/de.rs80.99% <0.00%> (-0.37%)⬇️
lightning/src/ln/features.rs98.81% <0.00%> (-0.11%)⬇️
lightning/src/util/test_utils.rs83.14% <0.00%> (-0.10%)⬇️
lightning/src/ln/peer_handler.rs44.17% <0.00%> (-0.07%)⬇️
lightning/src/routing/router.rs96.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
TheBlueMatt
reviewed
Mar 31, 2021
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

 }
 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 {
@TheBlueMattTheBlueMattMar 31, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
@ariardariardApr 27, 2021

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Upgraded with a new message, right doesn't need feerate lookup anymore.
Comment threadlightning/src/ln/channel.rs
Outdated

@valentinewallace
valentinewallace
commented
Apr 1, 2021

Copy link


Copy Markdown

Contributor
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
TheBlueMatt
commented
Apr 1, 2021

Copy link


Copy Markdown

Collaborator
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
valentinewallace
reviewed
Apr 1, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm ACK 7c25fb0 mod fixing CI and addressing the other comments :)
Comment threadlightning/src/ln/channel.rs
Outdated

 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)));
@valentinewallacevalentinewallaceApr 1, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it says user-specified here
@TheBlueMattTheBlueMatt
added this to the 0.0.14 milestone
Apr 5, 2021
@ariard
ariard
commented
Apr 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
TheBlueMatt
commented
Apr 28, 2021

Copy link


Copy Markdown

Collaborator
The full_stack_target fuzz failure here looks separate from the one fixed in #902 and I assume is new in the PR here.
@TheBlueMattTheBlueMatt
mentioned this pull request
May 1, 2021
 Merged

valentinewallace
valentinewallace
reviewed
May 3, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one question but this looks good
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/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 {
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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?
@ariardariardMay 3, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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.
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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
@ariardariardMay 3, 2021

Copy link


Copy Markdown

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.
Antoine Riard
added 2 commits
May 3, 2021 15:37
@ariard
Replace config max counterpary dust_limit_satoshis by a constant.
16619ff
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.
@ariard
Add bolt2_open_channel_sane_dust_limit
ce56e3f
TheBlueMatt
TheBlueMatt
reviewed
May 3, 2021
View reviewed changes
Comment threadfuzz/src/full_stack.rs
 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
@TheBlueMattTheBlueMattMay 3, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Why do we not hit this anymore? seems like this implies we now have less coverage?
TheBlueMatt
TheBlueMatt
approved these changes
May 3, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

I'll happily fix up the fuzz test after merge, I think we shouldn't hold this up on it. Looks good otherwise.
@TheBlueMatt
TheBlueMatt
merged commit 58e4ce2
into
lightningdevkit:mainMay 4, 2021
@TheBlueMattTheBlueMatt
mentioned this pull request
May 9, 2021

Closed

@SomberNightSomberNight
mentioned this pull request
Oct 25, 2021
 Merged

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.14

Development

Successfully merging this pull request may close these issues.

3 participants

@ariard@TheBlueMatt@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Switch to a max counterparty's dust_limit_satoshis constant - #845
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
May 4, 2021
Merged
Switch to a max counterparty's dust_limit_satoshis constant#845
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
Conversation
@ariard
@ariardariard
commented
Mar 16, 2021

Copy link


Copy Markdown

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
TheBlueMatt
reviewed
Mar 16, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Concept ACK
Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMatt
commented
Mar 19, 2021

Copy link


Copy Markdown

Collaborator
Note that ln::channel::tests::channel_reestablish_no_updates and ln::channel::tests::test_holder_vs_counterparty_dust_limit are currently failing.
@ariard
ariard
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
codecovBot
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #845 (b307c1f) into main (36570f4) will increase coverage by 0.26%.

The diff coverage is 96.29%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
lightning/src/util/config.rs48.71% <ø> (+1.09%)⬆️
lightning/src/ln/functional_tests.rs97.02% <94.11%> (+0.20%)⬆️
lightning/src/ln/channel.rs87.42% <100.00%> (-0.03%)⬇️
lightning/src/util/events.rs17.27% <0.00%> (-1.00%)⬇️
lightning-invoice/src/de.rs80.99% <0.00%> (-0.37%)⬇️
lightning/src/ln/features.rs98.81% <0.00%> (-0.11%)⬇️
lightning/src/util/test_utils.rs83.14% <0.00%> (-0.10%)⬇️
lightning/src/ln/peer_handler.rs44.17% <0.00%> (-0.07%)⬇️
lightning/src/routing/router.rs96.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
TheBlueMatt
reviewed
Mar 31, 2021
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

 }
 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 {
@TheBlueMattTheBlueMattMar 31, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
@ariardariardApr 27, 2021

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Upgraded with a new message, right doesn't need feerate lookup anymore.
Comment threadlightning/src/ln/channel.rs
Outdated

@valentinewallace
valentinewallace
commented
Apr 1, 2021

Copy link


Copy Markdown

Contributor
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
TheBlueMatt
commented
Apr 1, 2021

Copy link


Copy Markdown

Collaborator
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
valentinewallace
reviewed
Apr 1, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm ACK 7c25fb0 mod fixing CI and addressing the other comments :)
Comment threadlightning/src/ln/channel.rs
Outdated

 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)));
@valentinewallacevalentinewallaceApr 1, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it says user-specified here
@TheBlueMattTheBlueMatt
added this to the 0.0.14 milestone
Apr 5, 2021
@ariard
ariard
commented
Apr 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
TheBlueMatt
commented
Apr 28, 2021

Copy link


Copy Markdown

Collaborator
The full_stack_target fuzz failure here looks separate from the one fixed in #902 and I assume is new in the PR here.
@TheBlueMattTheBlueMatt
mentioned this pull request
May 1, 2021
 Merged

valentinewallace
valentinewallace
reviewed
May 3, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one question but this looks good
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/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 {
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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?
@ariardariardMay 3, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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.
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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
@ariardariardMay 3, 2021

Copy link


Copy Markdown

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.
Antoine Riard
added 2 commits
May 3, 2021 15:37
@ariard
Replace config max counterpary dust_limit_satoshis by a constant.
16619ff
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.
@ariard
Add bolt2_open_channel_sane_dust_limit
ce56e3f
TheBlueMatt
TheBlueMatt
reviewed
May 3, 2021
View reviewed changes
Comment threadfuzz/src/full_stack.rs
 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
@TheBlueMattTheBlueMattMay 3, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Why do we not hit this anymore? seems like this implies we now have less coverage?
TheBlueMatt
TheBlueMatt
approved these changes
May 3, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

I'll happily fix up the fuzz test after merge, I think we shouldn't hold this up on it. Looks good otherwise.
@TheBlueMatt
TheBlueMatt
merged commit 58e4ce2
into
lightningdevkit:mainMay 4, 2021
@TheBlueMattTheBlueMatt
mentioned this pull request
May 9, 2021

Closed

@SomberNightSomberNight
mentioned this pull request
Oct 25, 2021
 Merged

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.14

Development

Successfully merging this pull request may close these issues.

3 participants

@ariard@TheBlueMatt@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Switch to a max counterparty's dust_limit_satoshis constant - #845
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
May 4, 2021
Merged
Switch to a max counterparty's dust_limit_satoshis constant#845
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
Conversation
@ariard
@ariardariard
commented
Mar 16, 2021

Copy link


Copy Markdown

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
TheBlueMatt
reviewed
Mar 16, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Concept ACK
Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMatt
commented
Mar 19, 2021

Copy link


Copy Markdown

Collaborator
Note that ln::channel::tests::channel_reestablish_no_updates and ln::channel::tests::test_holder_vs_counterparty_dust_limit are currently failing.
@ariard
ariard
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
codecovBot
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #845 (b307c1f) into main (36570f4) will increase coverage by 0.26%.

The diff coverage is 96.29%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
lightning/src/util/config.rs48.71% <ø> (+1.09%)⬆️
lightning/src/ln/functional_tests.rs97.02% <94.11%> (+0.20%)⬆️
lightning/src/ln/channel.rs87.42% <100.00%> (-0.03%)⬇️
lightning/src/util/events.rs17.27% <0.00%> (-1.00%)⬇️
lightning-invoice/src/de.rs80.99% <0.00%> (-0.37%)⬇️
lightning/src/ln/features.rs98.81% <0.00%> (-0.11%)⬇️
lightning/src/util/test_utils.rs83.14% <0.00%> (-0.10%)⬇️
lightning/src/ln/peer_handler.rs44.17% <0.00%> (-0.07%)⬇️
lightning/src/routing/router.rs96.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
TheBlueMatt
reviewed
Mar 31, 2021
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

 }
 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 {
@TheBlueMattTheBlueMattMar 31, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
@ariardariardApr 27, 2021

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Upgraded with a new message, right doesn't need feerate lookup anymore.
Comment threadlightning/src/ln/channel.rs
Outdated

@valentinewallace
valentinewallace
commented
Apr 1, 2021

Copy link


Copy Markdown

Contributor
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
TheBlueMatt
commented
Apr 1, 2021

Copy link


Copy Markdown

Collaborator
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
valentinewallace
reviewed
Apr 1, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm ACK 7c25fb0 mod fixing CI and addressing the other comments :)
Comment threadlightning/src/ln/channel.rs
Outdated

 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)));
@valentinewallacevalentinewallaceApr 1, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it says user-specified here
@TheBlueMattTheBlueMatt
added this to the 0.0.14 milestone
Apr 5, 2021
@ariard
ariard
commented
Apr 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
TheBlueMatt
commented
Apr 28, 2021

Copy link


Copy Markdown

Collaborator
The full_stack_target fuzz failure here looks separate from the one fixed in #902 and I assume is new in the PR here.
@TheBlueMattTheBlueMatt
mentioned this pull request
May 1, 2021
 Merged

valentinewallace
valentinewallace
reviewed
May 3, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one question but this looks good
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/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 {
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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?
@ariardariardMay 3, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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.
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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
@ariardariardMay 3, 2021

Copy link


Copy Markdown

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.
Antoine Riard
added 2 commits
May 3, 2021 15:37
@ariard
Replace config max counterpary dust_limit_satoshis by a constant.
16619ff
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.
@ariard
Add bolt2_open_channel_sane_dust_limit
ce56e3f
TheBlueMatt
TheBlueMatt
reviewed
May 3, 2021
View reviewed changes
Comment threadfuzz/src/full_stack.rs
 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
@TheBlueMattTheBlueMattMay 3, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Why do we not hit this anymore? seems like this implies we now have less coverage?
TheBlueMatt
TheBlueMatt
approved these changes
May 3, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

I'll happily fix up the fuzz test after merge, I think we shouldn't hold this up on it. Looks good otherwise.
@TheBlueMatt
TheBlueMatt
merged commit 58e4ce2
into
lightningdevkit:mainMay 4, 2021
@TheBlueMattTheBlueMatt
mentioned this pull request
May 9, 2021

Closed

@SomberNightSomberNight
mentioned this pull request
Oct 25, 2021
 Merged

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.14

Development

Successfully merging this pull request may close these issues.

3 participants

@ariard@TheBlueMatt@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Switch to a max counterparty's dust_limit_satoshis constant - #845
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
May 4, 2021
Merged
Switch to a max counterparty's dust_limit_satoshis constant#845
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
Conversation
@ariard
@ariardariard
commented
Mar 16, 2021

Copy link


Copy Markdown

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
TheBlueMatt
reviewed
Mar 16, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Concept ACK
Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMatt
commented
Mar 19, 2021

Copy link


Copy Markdown

Collaborator
Note that ln::channel::tests::channel_reestablish_no_updates and ln::channel::tests::test_holder_vs_counterparty_dust_limit are currently failing.
@ariard
ariard
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
codecovBot
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #845 (b307c1f) into main (36570f4) will increase coverage by 0.26%.

The diff coverage is 96.29%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
lightning/src/util/config.rs48.71% <ø> (+1.09%)⬆️
lightning/src/ln/functional_tests.rs97.02% <94.11%> (+0.20%)⬆️
lightning/src/ln/channel.rs87.42% <100.00%> (-0.03%)⬇️
lightning/src/util/events.rs17.27% <0.00%> (-1.00%)⬇️
lightning-invoice/src/de.rs80.99% <0.00%> (-0.37%)⬇️
lightning/src/ln/features.rs98.81% <0.00%> (-0.11%)⬇️
lightning/src/util/test_utils.rs83.14% <0.00%> (-0.10%)⬇️
lightning/src/ln/peer_handler.rs44.17% <0.00%> (-0.07%)⬇️
lightning/src/routing/router.rs96.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
TheBlueMatt
reviewed
Mar 31, 2021
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

 }
 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 {
@TheBlueMattTheBlueMattMar 31, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
@ariardariardApr 27, 2021

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Upgraded with a new message, right doesn't need feerate lookup anymore.
Comment threadlightning/src/ln/channel.rs
Outdated

@valentinewallace
valentinewallace
commented
Apr 1, 2021

Copy link


Copy Markdown

Contributor
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
TheBlueMatt
commented
Apr 1, 2021

Copy link


Copy Markdown

Collaborator
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
valentinewallace
reviewed
Apr 1, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm ACK 7c25fb0 mod fixing CI and addressing the other comments :)
Comment threadlightning/src/ln/channel.rs
Outdated

 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)));
@valentinewallacevalentinewallaceApr 1, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it says user-specified here
@TheBlueMattTheBlueMatt
added this to the 0.0.14 milestone
Apr 5, 2021
@ariard
ariard
commented
Apr 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
TheBlueMatt
commented
Apr 28, 2021

Copy link


Copy Markdown

Collaborator
The full_stack_target fuzz failure here looks separate from the one fixed in #902 and I assume is new in the PR here.
@TheBlueMattTheBlueMatt
mentioned this pull request
May 1, 2021
 Merged

valentinewallace
valentinewallace
reviewed
May 3, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one question but this looks good
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/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 {
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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?
@ariardariardMay 3, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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.
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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
@ariardariardMay 3, 2021

Copy link


Copy Markdown

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.
Antoine Riard
added 2 commits
May 3, 2021 15:37
@ariard
Replace config max counterpary dust_limit_satoshis by a constant.
16619ff
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.
@ariard
Add bolt2_open_channel_sane_dust_limit
ce56e3f
TheBlueMatt
TheBlueMatt
reviewed
May 3, 2021
View reviewed changes
Comment threadfuzz/src/full_stack.rs
 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
@TheBlueMattTheBlueMattMay 3, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Why do we not hit this anymore? seems like this implies we now have less coverage?
TheBlueMatt
TheBlueMatt
approved these changes
May 3, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

I'll happily fix up the fuzz test after merge, I think we shouldn't hold this up on it. Looks good otherwise.
@TheBlueMatt
TheBlueMatt
merged commit 58e4ce2
into
lightningdevkit:mainMay 4, 2021
@TheBlueMattTheBlueMatt
mentioned this pull request
May 9, 2021

Closed

@SomberNightSomberNight
mentioned this pull request
Oct 25, 2021
 Merged

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.14

Development

Successfully merging this pull request may close these issues.

3 participants

@ariard@TheBlueMatt@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Switch to a max counterparty's dust_limit_satoshis constant - #845
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
May 4, 2021
Merged
Switch to a max counterparty's dust_limit_satoshis constant#845
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
Conversation
@ariard
@ariardariard
commented
Mar 16, 2021

Copy link


Copy Markdown

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
TheBlueMatt
reviewed
Mar 16, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Concept ACK
Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMatt
commented
Mar 19, 2021

Copy link


Copy Markdown

Collaborator
Note that ln::channel::tests::channel_reestablish_no_updates and ln::channel::tests::test_holder_vs_counterparty_dust_limit are currently failing.
@ariard
ariard
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
codecovBot
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #845 (b307c1f) into main (36570f4) will increase coverage by 0.26%.

The diff coverage is 96.29%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
lightning/src/util/config.rs48.71% <ø> (+1.09%)⬆️
lightning/src/ln/functional_tests.rs97.02% <94.11%> (+0.20%)⬆️
lightning/src/ln/channel.rs87.42% <100.00%> (-0.03%)⬇️
lightning/src/util/events.rs17.27% <0.00%> (-1.00%)⬇️
lightning-invoice/src/de.rs80.99% <0.00%> (-0.37%)⬇️
lightning/src/ln/features.rs98.81% <0.00%> (-0.11%)⬇️
lightning/src/util/test_utils.rs83.14% <0.00%> (-0.10%)⬇️
lightning/src/ln/peer_handler.rs44.17% <0.00%> (-0.07%)⬇️
lightning/src/routing/router.rs96.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
TheBlueMatt
reviewed
Mar 31, 2021
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

 }
 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 {
@TheBlueMattTheBlueMattMar 31, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
@ariardariardApr 27, 2021

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Upgraded with a new message, right doesn't need feerate lookup anymore.
Comment threadlightning/src/ln/channel.rs
Outdated

@valentinewallace
valentinewallace
commented
Apr 1, 2021

Copy link


Copy Markdown

Contributor
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
TheBlueMatt
commented
Apr 1, 2021

Copy link


Copy Markdown

Collaborator
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
valentinewallace
reviewed
Apr 1, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm ACK 7c25fb0 mod fixing CI and addressing the other comments :)
Comment threadlightning/src/ln/channel.rs
Outdated

 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)));
@valentinewallacevalentinewallaceApr 1, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it says user-specified here
@TheBlueMattTheBlueMatt
added this to the 0.0.14 milestone
Apr 5, 2021
@ariard
ariard
commented
Apr 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
TheBlueMatt
commented
Apr 28, 2021

Copy link


Copy Markdown

Collaborator
The full_stack_target fuzz failure here looks separate from the one fixed in #902 and I assume is new in the PR here.
@TheBlueMattTheBlueMatt
mentioned this pull request
May 1, 2021
 Merged

valentinewallace
valentinewallace
reviewed
May 3, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one question but this looks good
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/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 {
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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?
@ariardariardMay 3, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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.
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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
@ariardariardMay 3, 2021

Copy link


Copy Markdown

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.
Antoine Riard
added 2 commits
May 3, 2021 15:37
@ariard
Replace config max counterpary dust_limit_satoshis by a constant.
16619ff
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.
@ariard
Add bolt2_open_channel_sane_dust_limit
ce56e3f
TheBlueMatt
TheBlueMatt
reviewed
May 3, 2021
View reviewed changes
Comment threadfuzz/src/full_stack.rs
 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
@TheBlueMattTheBlueMattMay 3, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Why do we not hit this anymore? seems like this implies we now have less coverage?
TheBlueMatt
TheBlueMatt
approved these changes
May 3, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

I'll happily fix up the fuzz test after merge, I think we shouldn't hold this up on it. Looks good otherwise.
@TheBlueMatt
TheBlueMatt
merged commit 58e4ce2
into
lightningdevkit:mainMay 4, 2021
@TheBlueMattTheBlueMatt
mentioned this pull request
May 9, 2021

Closed

@SomberNightSomberNight
mentioned this pull request
Oct 25, 2021
 Merged

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.14

Development

Successfully merging this pull request may close these issues.

3 participants

@ariard@TheBlueMatt@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Switch to a max counterparty's dust_limit_satoshis constant - #845
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
May 4, 2021
Merged
Switch to a max counterparty's dust_limit_satoshis constant#845
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
ariard:2021-03-hardcode-dust
Conversation
@ariard
@ariardariard
commented
Mar 16, 2021

Copy link


Copy Markdown

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
TheBlueMatt
reviewed
Mar 16, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Concept ACK
Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMatt
commented
Mar 19, 2021

Copy link


Copy Markdown

Collaborator
Note that ln::channel::tests::channel_reestablish_no_updates and ln::channel::tests::test_holder_vs_counterparty_dust_limit are currently failing.
@ariard
ariard
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
codecovBot
commented
Mar 27, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #845 (b307c1f) into main (36570f4) will increase coverage by 0.26%.

The diff coverage is 96.29%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
lightning/src/util/config.rs48.71% <ø> (+1.09%)⬆️
lightning/src/ln/functional_tests.rs97.02% <94.11%> (+0.20%)⬆️
lightning/src/ln/channel.rs87.42% <100.00%> (-0.03%)⬇️
lightning/src/util/events.rs17.27% <0.00%> (-1.00%)⬇️
lightning-invoice/src/de.rs80.99% <0.00%> (-0.37%)⬇️
lightning/src/ln/features.rs98.81% <0.00%> (-0.11%)⬇️
lightning/src/util/test_utils.rs83.14% <0.00%> (-0.10%)⬇️
lightning/src/ln/peer_handler.rs44.17% <0.00%> (-0.07%)⬇️
lightning/src/routing/router.rs96.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
TheBlueMatt
reviewed
Mar 31, 2021
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

 }
 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 {
@TheBlueMattTheBlueMattMar 31, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
@ariardariardApr 27, 2021

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Upgraded with a new message, right doesn't need feerate lookup anymore.
Comment threadlightning/src/ln/channel.rs
Outdated

@valentinewallace
valentinewallace
commented
Apr 1, 2021

Copy link


Copy Markdown

Contributor
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
TheBlueMatt
commented
Apr 1, 2021

Copy link


Copy Markdown

Collaborator
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
valentinewallace
reviewed
Apr 1, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm ACK 7c25fb0 mod fixing CI and addressing the other comments :)
Comment threadlightning/src/ln/channel.rs
Outdated

 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)));
@valentinewallacevalentinewallaceApr 1, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it says user-specified here
@TheBlueMattTheBlueMatt
added this to the 0.0.14 milestone
Apr 5, 2021
@ariard
ariard
commented
Apr 27, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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
TheBlueMatt
commented
Apr 28, 2021

Copy link


Copy Markdown

Collaborator
The full_stack_target fuzz failure here looks separate from the one fixed in #902 and I assume is new in the PR here.
@TheBlueMattTheBlueMatt
mentioned this pull request
May 1, 2021
 Merged

valentinewallace
valentinewallace
reviewed
May 3, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one question but this looks good
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/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 {
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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?
@ariardariardMay 3, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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.
@valentinewallacevalentinewallaceMay 3, 2021

Copy link


Copy Markdown

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
@ariardariardMay 3, 2021

Copy link


Copy Markdown

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.
Antoine Riard
added 2 commits
May 3, 2021 15:37
@ariard
Replace config max counterpary dust_limit_satoshis by a constant.
16619ff
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.
@ariard
Add bolt2_open_channel_sane_dust_limit
ce56e3f
TheBlueMatt
TheBlueMatt
reviewed
May 3, 2021
View reviewed changes
Comment threadfuzz/src/full_stack.rs
 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
@TheBlueMattTheBlueMattMay 3, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Why do we not hit this anymore? seems like this implies we now have less coverage?
TheBlueMatt
TheBlueMatt
approved these changes
May 3, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

I'll happily fix up the fuzz test after merge, I think we shouldn't hold this up on it. Looks good otherwise.
@TheBlueMatt
TheBlueMatt
merged commit 58e4ce2
into
lightningdevkit:mainMay 4, 2021
@TheBlueMattTheBlueMatt
mentioned this pull request
May 9, 2021

Closed

@SomberNightSomberNight
mentioned this pull request
Oct 25, 2021
 Merged

3 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.14

Development

Successfully merging this pull request may close these issues.

3 participants

@ariard@TheBlueMatt@valentinewallace