Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours. - #1754

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs
Oct 11, 2022
Merged

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours.#1754
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Depends on #1625.

Comment threadlightning/src/routing/scoring.rs Outdated
/// with high payment volume or that actively probe the [`NetworkGraph`]. Nodes with low payment
/// volume are more likely to experience failed payment paths, which would need to be retried.
/// This probability is converted into a linear score by `log10`'ing it and multiplying it with the
/// [`liquidity_penalty_multiplier_msat`] and [`liquidity_penalty_amount_multiplier_msat`]

@tlulutluluOct 5, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A formula would be easier to read imo
amount = log10(probability) * liquidity_penalty_multiplier_msat * liquidity_penalty_amount_multiplier_msat

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it okay if we just leave the formulas on the docs for parameters themselves?

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.

Also, should be -log10.

Comment threadlightning/src/routing/scoring.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 8702f17 to 14432b8CompareOctober 5, 2022 19:40
tlulu
tlulu previously approved these changes Oct 5, 2022
tlulu
tlulu previously approved these changes Oct 5, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +322 to +324
/// These bounds are then used to determine a success probability using the formula from
/// *Optimally Reliable & Cheap Payment Flows on the Lightning Network* by Rene Pickhardt
/// and Stefan Richter [[1]] (i.e. `(payment amount + lower-bound) / (upper-bound - lower-bound)`).

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.

Technically, the numerator and denominator both need + 1 to avoid division by zero.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I figured skip over that level of detail here.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +317 to +321
/// Channels are tracked with upper- and lower- liquidity bounds - when an HTLC fails at a channel,
/// we learn that the upper-bound on the available liquidity is lower than the amount of the HTLC.
/// When a payment is forwarded through a channel (but fails later in the route), we learn the
/// lower-bound on the channel's available liquidity must be at least the value of the HTLC.

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.

There is a third case where a successful payment lowers the upper bound by the amount.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I thought about spelling it out, but that just seemed like too much detail. I can add something like "We also learn some information about upper-bounds after a successful payment." or so?

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@codecov-commenter

codecov-commenter commented Oct 5, 2022

Copy link
Copy Markdown

Codecov Report

Base: 90.79% // Head: 90.80% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (bd6a5f1) compared to base (6738fd5).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head bd6a5f1 differs from pull request most recent head 9e84966. Consider uploading reports for the commit 9e84966 to get more accurate results

Additional details and impacted files
@@ Coverage Diff @@## main #1754 +/- ##
=======================================
Coverage 90.79% 90.80% =======================================
Files 87 87 Lines 46969 46969 Branches 46969 46969 =======================================
+ Hits 42646 42648 +2 + Misses 4323 4321 -2 
Impacted FilesCoverage Δ
lightning/src/routing/scoring.rs96.61% <100.00%> (ø)
lightning/src/chain/onchaintx.rs94.71% <0.00%> (-0.92%)⬇️
lightning/src/ln/functional_tests.rs97.05% <0.00%> (+0.04%)⬆️
lightning-net-tokio/src/lib.rs77.64% <0.00%> (+0.90%)⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tnull
tnull self-requested a review October 6, 2022 09:45
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
/// gives tighter bounds on the channel liquidity balance. Thus, halving the offsets decreases
/// the certainty of the channel liquidity balance.
/// For example, if the channel's capacity is 1 million sats, and the current upper- and lower-
/// liquidity bounds are 200,000 sats and 600,000 sats, after this amount of time the upper-

@tnulltnullOct 6, 2022

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.

nit: Move "after this amount of time" to the end of the sentence. Also, since this is an example it would be nice to make it more concrete, e.g., use the default values or variable name?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Well, the documentation is on the variable itself, so it seems strange to refer to it "in the third person"?

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.

Alright, still think would read better if "after this amount of time" would be at the end of the sentence, but feel to leave as is if you prefer.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 25766b1 to e01091cCompareOctober 6, 2022 17:46

@tnulltnull 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.

Looks good mod failing CI.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from e01091c to 3e14f9eCompareOctober 7, 2022 17:34
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased after #1625 landed, should pass CI now.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 3e14f9e to 0aec44bCompareOctober 7, 2022 21:06
tnull
tnull previously approved these changes Oct 8, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +416 to +417
/// Because halving the liquidity bounds grows the uncertainty on the channel's liquidity,
/// penalties for payments that are within the liquidity bounds will be decreased. See the

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 think whether or not penalties are decrease depends on whether the amount lies closer to the upper bound or the lower bound.

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.

Would consider re-wording as:

"Because halving the liquidity bounds increases the uncertainty of the channel's liquidity, the penalty for an amount within the new bounds may increase or decrease depending on whether the new success probability decreased or increased, respectively."

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it worth specifying the last clause? I just changed it to "it may change", basically.

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.

SGTM

jkczyz
jkczyz previously approved these changes Oct 11, 2022
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased on usptream:

$ git range-diff 7544030bb63fee6484fc178bb2ac8f382fe3b5b1...2e572e8affe4688c84ea15c5d3b836296ee60392 6738fd56cf74227a12ecf6f3934515f42f9f9452...2026634631816b8bfac4e20993fa0814e53dbe00
1: 56d91c276 = 1: d7d7c2e61 Rewrite documentation some on `ProbabilisticScorer`
2: a7502e09a ! 2: 37783f944 f point to params for formulas
@@ lightning/src/routing/scoring.rs: pub struct ProbabilisticScoringParameters {
///
+ /// `-log10(success_probability) * liquidity_penalty_multiplier_msat`
+ ///
- /// Default value: 40,000 msat
+ /// Default value: 30,000 msat
///
/// [`liquidity_offset_half_life`]: Self::liquidity_offset_half_life
3: db948baf4 = 3: 8c61f751c f add decay example
4: ecdcffa3c = 4: e433f0cad f missing link
5: 877d9cf5c = 5: 0ab1e4b5f f address docs feedback
6: a982b27f7 = 6: f44f36989 f nits from tnull
7: 48976ddf2 = 7: 98620f372 f docs
8: 4fb50c34e = 8: f5a967c65 f docs
9: 2e572e8af ! 9: 202663463 Increase the default `liquidity_offset_half_life` to six hours
@@ lightning/src/routing/scoring.rs: impl ProbabilisticScoringParameters {
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
liquidity_penalty_amount_multiplier_msat: 0,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 0,
+ historical_liquidity_penalty_multiplier_msat: 0,
+ historical_liquidity_penalty_amount_multiplier_msat: 0,
@@ lightning/src/routing/scoring.rs: impl Default for ProbabilisticScoringParameters {
base_penalty_msat: 500,
base_penalty_amount_multiplier_msat: 8192,
- liquidity_penalty_multiplier_msat: 40_000,
+ liquidity_penalty_multiplier_msat: 30_000,
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
- liquidity_penalty_amount_multiplier_msat: 256,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 250,
+ liquidity_penalty_amount_multiplier_msat: 192,
+ historical_liquidity_penalty_multiplier_msat: 10_000,
+ historical_liquidity_penalty_amount_multiplier_msat: 64,

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 2026634 to bd6a5f1CompareOctober 11, 2022 15:28
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Then squashed with no further changes:

$ git diff-tree -U1 20266346 bd6a5f17
$

tnull
tnull previously approved these changes Oct 11, 2022
jkczyz
jkczyz previously approved these changes Oct 11, 2022
We had some user confusion on how the probabilistic scorer works,
especially in reference to the half-life parameter. This attempts
to clarify how the bounds work, and how they are decayed.
Even at relatively high payment volumes, decaying knowledge of each
individual channel every hour causes aggressive retrying of
channels as we quickly forget the state of a channel. Even with the
historical tracker, this isn't fully remedied, as we'll track the
history bounds with the decayed value.
Instead, we decay every six hours here, reducing how often we'll
retry a channel due to decay.
In addition to this, the decay likely needs to be substantially
more linear, as tracked in lightningdevkit#1752.
@TheBlueMatt
TheBlueMatt dismissed stale reviews from jkczyz and tnull via 9e84966October 11, 2022 18:25
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from bd6a5f1 to 9e84966CompareOctober 11, 2022 18:25
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Oops, had screwed up the docs, force-pushed:

$ git diff-tree -U1 bd6a5f17b 9e8496659
diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
index f9c188857..fefdebfcc 100644
--- a/lightning/src/routing/scoring.rs+++ b/lightning/src/routing/scoring.rs@@ -349,4 +349,4 @@ type ConfiguredTime = Eternity;
/// [`liquidity_offset_half_life`]: ProbabilisticScoringParameters::liquidity_offset_half_life
-/// [`liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_multiplier_msat-/// [`liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_amount_multiplier_msat+/// [`historical_liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_multiplier_msat+/// [`historical_liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_amount_multiplier_msat

@TheBlueMatt
TheBlueMatt merged commit fdfd4f0 into lightningdevkit:mainOct 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@tnull@jkczyz@tlulu
, '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

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours. - #1754

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs
Oct 11, 2022
Merged

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours.#1754
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Depends on #1625.

Comment threadlightning/src/routing/scoring.rs Outdated
/// with high payment volume or that actively probe the [`NetworkGraph`]. Nodes with low payment
/// volume are more likely to experience failed payment paths, which would need to be retried.
/// This probability is converted into a linear score by `log10`'ing it and multiplying it with the
/// [`liquidity_penalty_multiplier_msat`] and [`liquidity_penalty_amount_multiplier_msat`]

@tlulutluluOct 5, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A formula would be easier to read imo
amount = log10(probability) * liquidity_penalty_multiplier_msat * liquidity_penalty_amount_multiplier_msat

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it okay if we just leave the formulas on the docs for parameters themselves?

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.

Also, should be -log10.

Comment threadlightning/src/routing/scoring.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 8702f17 to 14432b8CompareOctober 5, 2022 19:40
tlulu
tlulu previously approved these changes Oct 5, 2022
tlulu
tlulu previously approved these changes Oct 5, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +322 to +324
/// These bounds are then used to determine a success probability using the formula from
/// *Optimally Reliable & Cheap Payment Flows on the Lightning Network* by Rene Pickhardt
/// and Stefan Richter [[1]] (i.e. `(payment amount + lower-bound) / (upper-bound - lower-bound)`).

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.

Technically, the numerator and denominator both need + 1 to avoid division by zero.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I figured skip over that level of detail here.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +317 to +321
/// Channels are tracked with upper- and lower- liquidity bounds - when an HTLC fails at a channel,
/// we learn that the upper-bound on the available liquidity is lower than the amount of the HTLC.
/// When a payment is forwarded through a channel (but fails later in the route), we learn the
/// lower-bound on the channel's available liquidity must be at least the value of the HTLC.

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.

There is a third case where a successful payment lowers the upper bound by the amount.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I thought about spelling it out, but that just seemed like too much detail. I can add something like "We also learn some information about upper-bounds after a successful payment." or so?

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@codecov-commenter

codecov-commenter commented Oct 5, 2022

Copy link
Copy Markdown

Codecov Report

Base: 90.79% // Head: 90.80% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (bd6a5f1) compared to base (6738fd5).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head bd6a5f1 differs from pull request most recent head 9e84966. Consider uploading reports for the commit 9e84966 to get more accurate results

Additional details and impacted files
@@ Coverage Diff @@## main #1754 +/- ##
=======================================
Coverage 90.79% 90.80% =======================================
Files 87 87 Lines 46969 46969 Branches 46969 46969 =======================================
+ Hits 42646 42648 +2 + Misses 4323 4321 -2 
Impacted FilesCoverage Δ
lightning/src/routing/scoring.rs96.61% <100.00%> (ø)
lightning/src/chain/onchaintx.rs94.71% <0.00%> (-0.92%)⬇️
lightning/src/ln/functional_tests.rs97.05% <0.00%> (+0.04%)⬆️
lightning-net-tokio/src/lib.rs77.64% <0.00%> (+0.90%)⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tnull
tnull self-requested a review October 6, 2022 09:45
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
/// gives tighter bounds on the channel liquidity balance. Thus, halving the offsets decreases
/// the certainty of the channel liquidity balance.
/// For example, if the channel's capacity is 1 million sats, and the current upper- and lower-
/// liquidity bounds are 200,000 sats and 600,000 sats, after this amount of time the upper-

@tnulltnullOct 6, 2022

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.

nit: Move "after this amount of time" to the end of the sentence. Also, since this is an example it would be nice to make it more concrete, e.g., use the default values or variable name?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Well, the documentation is on the variable itself, so it seems strange to refer to it "in the third person"?

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.

Alright, still think would read better if "after this amount of time" would be at the end of the sentence, but feel to leave as is if you prefer.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 25766b1 to e01091cCompareOctober 6, 2022 17:46

@tnulltnull 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.

Looks good mod failing CI.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from e01091c to 3e14f9eCompareOctober 7, 2022 17:34
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased after #1625 landed, should pass CI now.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 3e14f9e to 0aec44bCompareOctober 7, 2022 21:06
tnull
tnull previously approved these changes Oct 8, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +416 to +417
/// Because halving the liquidity bounds grows the uncertainty on the channel's liquidity,
/// penalties for payments that are within the liquidity bounds will be decreased. See the

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 think whether or not penalties are decrease depends on whether the amount lies closer to the upper bound or the lower bound.

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.

Would consider re-wording as:

"Because halving the liquidity bounds increases the uncertainty of the channel's liquidity, the penalty for an amount within the new bounds may increase or decrease depending on whether the new success probability decreased or increased, respectively."

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it worth specifying the last clause? I just changed it to "it may change", basically.

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.

SGTM

jkczyz
jkczyz previously approved these changes Oct 11, 2022
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased on usptream:

$ git range-diff 7544030bb63fee6484fc178bb2ac8f382fe3b5b1...2e572e8affe4688c84ea15c5d3b836296ee60392 6738fd56cf74227a12ecf6f3934515f42f9f9452...2026634631816b8bfac4e20993fa0814e53dbe00
1: 56d91c276 = 1: d7d7c2e61 Rewrite documentation some on `ProbabilisticScorer`
2: a7502e09a ! 2: 37783f944 f point to params for formulas
@@ lightning/src/routing/scoring.rs: pub struct ProbabilisticScoringParameters {
///
+ /// `-log10(success_probability) * liquidity_penalty_multiplier_msat`
+ ///
- /// Default value: 40,000 msat
+ /// Default value: 30,000 msat
///
/// [`liquidity_offset_half_life`]: Self::liquidity_offset_half_life
3: db948baf4 = 3: 8c61f751c f add decay example
4: ecdcffa3c = 4: e433f0cad f missing link
5: 877d9cf5c = 5: 0ab1e4b5f f address docs feedback
6: a982b27f7 = 6: f44f36989 f nits from tnull
7: 48976ddf2 = 7: 98620f372 f docs
8: 4fb50c34e = 8: f5a967c65 f docs
9: 2e572e8af ! 9: 202663463 Increase the default `liquidity_offset_half_life` to six hours
@@ lightning/src/routing/scoring.rs: impl ProbabilisticScoringParameters {
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
liquidity_penalty_amount_multiplier_msat: 0,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 0,
+ historical_liquidity_penalty_multiplier_msat: 0,
+ historical_liquidity_penalty_amount_multiplier_msat: 0,
@@ lightning/src/routing/scoring.rs: impl Default for ProbabilisticScoringParameters {
base_penalty_msat: 500,
base_penalty_amount_multiplier_msat: 8192,
- liquidity_penalty_multiplier_msat: 40_000,
+ liquidity_penalty_multiplier_msat: 30_000,
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
- liquidity_penalty_amount_multiplier_msat: 256,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 250,
+ liquidity_penalty_amount_multiplier_msat: 192,
+ historical_liquidity_penalty_multiplier_msat: 10_000,
+ historical_liquidity_penalty_amount_multiplier_msat: 64,

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 2026634 to bd6a5f1CompareOctober 11, 2022 15:28
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Then squashed with no further changes:

$ git diff-tree -U1 20266346 bd6a5f17
$

tnull
tnull previously approved these changes Oct 11, 2022
jkczyz
jkczyz previously approved these changes Oct 11, 2022
We had some user confusion on how the probabilistic scorer works,
especially in reference to the half-life parameter. This attempts
to clarify how the bounds work, and how they are decayed.
Even at relatively high payment volumes, decaying knowledge of each
individual channel every hour causes aggressive retrying of
channels as we quickly forget the state of a channel. Even with the
historical tracker, this isn't fully remedied, as we'll track the
history bounds with the decayed value.
Instead, we decay every six hours here, reducing how often we'll
retry a channel due to decay.
In addition to this, the decay likely needs to be substantially
more linear, as tracked in lightningdevkit#1752.
@TheBlueMatt
TheBlueMatt dismissed stale reviews from jkczyz and tnull via 9e84966October 11, 2022 18:25
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from bd6a5f1 to 9e84966CompareOctober 11, 2022 18:25
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Oops, had screwed up the docs, force-pushed:

$ git diff-tree -U1 bd6a5f17b 9e8496659
diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
index f9c188857..fefdebfcc 100644
--- a/lightning/src/routing/scoring.rs+++ b/lightning/src/routing/scoring.rs@@ -349,4 +349,4 @@ type ConfiguredTime = Eternity;
/// [`liquidity_offset_half_life`]: ProbabilisticScoringParameters::liquidity_offset_half_life
-/// [`liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_multiplier_msat-/// [`liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_amount_multiplier_msat+/// [`historical_liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_multiplier_msat+/// [`historical_liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_amount_multiplier_msat

@TheBlueMatt
TheBlueMatt merged commit fdfd4f0 into lightningdevkit:mainOct 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@tnull@jkczyz@tlulu
, '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

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours. - #1754

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs
Oct 11, 2022
Merged

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours.#1754
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Depends on #1625.

Comment threadlightning/src/routing/scoring.rs Outdated
/// with high payment volume or that actively probe the [`NetworkGraph`]. Nodes with low payment
/// volume are more likely to experience failed payment paths, which would need to be retried.
/// This probability is converted into a linear score by `log10`'ing it and multiplying it with the
/// [`liquidity_penalty_multiplier_msat`] and [`liquidity_penalty_amount_multiplier_msat`]

@tlulutluluOct 5, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A formula would be easier to read imo
amount = log10(probability) * liquidity_penalty_multiplier_msat * liquidity_penalty_amount_multiplier_msat

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it okay if we just leave the formulas on the docs for parameters themselves?

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.

Also, should be -log10.

Comment threadlightning/src/routing/scoring.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 8702f17 to 14432b8CompareOctober 5, 2022 19:40
tlulu
tlulu previously approved these changes Oct 5, 2022
tlulu
tlulu previously approved these changes Oct 5, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +322 to +324
/// These bounds are then used to determine a success probability using the formula from
/// *Optimally Reliable & Cheap Payment Flows on the Lightning Network* by Rene Pickhardt
/// and Stefan Richter [[1]] (i.e. `(payment amount + lower-bound) / (upper-bound - lower-bound)`).

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.

Technically, the numerator and denominator both need + 1 to avoid division by zero.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I figured skip over that level of detail here.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +317 to +321
/// Channels are tracked with upper- and lower- liquidity bounds - when an HTLC fails at a channel,
/// we learn that the upper-bound on the available liquidity is lower than the amount of the HTLC.
/// When a payment is forwarded through a channel (but fails later in the route), we learn the
/// lower-bound on the channel's available liquidity must be at least the value of the HTLC.

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.

There is a third case where a successful payment lowers the upper bound by the amount.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I thought about spelling it out, but that just seemed like too much detail. I can add something like "We also learn some information about upper-bounds after a successful payment." or so?

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@codecov-commenter

codecov-commenter commented Oct 5, 2022

Copy link
Copy Markdown

Codecov Report

Base: 90.79% // Head: 90.80% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (bd6a5f1) compared to base (6738fd5).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head bd6a5f1 differs from pull request most recent head 9e84966. Consider uploading reports for the commit 9e84966 to get more accurate results

Additional details and impacted files
@@ Coverage Diff @@## main #1754 +/- ##
=======================================
Coverage 90.79% 90.80% =======================================
Files 87 87 Lines 46969 46969 Branches 46969 46969 =======================================
+ Hits 42646 42648 +2 + Misses 4323 4321 -2 
Impacted FilesCoverage Δ
lightning/src/routing/scoring.rs96.61% <100.00%> (ø)
lightning/src/chain/onchaintx.rs94.71% <0.00%> (-0.92%)⬇️
lightning/src/ln/functional_tests.rs97.05% <0.00%> (+0.04%)⬆️
lightning-net-tokio/src/lib.rs77.64% <0.00%> (+0.90%)⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tnull
tnull self-requested a review October 6, 2022 09:45
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
/// gives tighter bounds on the channel liquidity balance. Thus, halving the offsets decreases
/// the certainty of the channel liquidity balance.
/// For example, if the channel's capacity is 1 million sats, and the current upper- and lower-
/// liquidity bounds are 200,000 sats and 600,000 sats, after this amount of time the upper-

@tnulltnullOct 6, 2022

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.

nit: Move "after this amount of time" to the end of the sentence. Also, since this is an example it would be nice to make it more concrete, e.g., use the default values or variable name?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Well, the documentation is on the variable itself, so it seems strange to refer to it "in the third person"?

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.

Alright, still think would read better if "after this amount of time" would be at the end of the sentence, but feel to leave as is if you prefer.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 25766b1 to e01091cCompareOctober 6, 2022 17:46

@tnulltnull 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.

Looks good mod failing CI.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from e01091c to 3e14f9eCompareOctober 7, 2022 17:34
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased after #1625 landed, should pass CI now.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 3e14f9e to 0aec44bCompareOctober 7, 2022 21:06
tnull
tnull previously approved these changes Oct 8, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +416 to +417
/// Because halving the liquidity bounds grows the uncertainty on the channel's liquidity,
/// penalties for payments that are within the liquidity bounds will be decreased. See the

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 think whether or not penalties are decrease depends on whether the amount lies closer to the upper bound or the lower bound.

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.

Would consider re-wording as:

"Because halving the liquidity bounds increases the uncertainty of the channel's liquidity, the penalty for an amount within the new bounds may increase or decrease depending on whether the new success probability decreased or increased, respectively."

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it worth specifying the last clause? I just changed it to "it may change", basically.

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.

SGTM

jkczyz
jkczyz previously approved these changes Oct 11, 2022
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased on usptream:

$ git range-diff 7544030bb63fee6484fc178bb2ac8f382fe3b5b1...2e572e8affe4688c84ea15c5d3b836296ee60392 6738fd56cf74227a12ecf6f3934515f42f9f9452...2026634631816b8bfac4e20993fa0814e53dbe00
1: 56d91c276 = 1: d7d7c2e61 Rewrite documentation some on `ProbabilisticScorer`
2: a7502e09a ! 2: 37783f944 f point to params for formulas
@@ lightning/src/routing/scoring.rs: pub struct ProbabilisticScoringParameters {
///
+ /// `-log10(success_probability) * liquidity_penalty_multiplier_msat`
+ ///
- /// Default value: 40,000 msat
+ /// Default value: 30,000 msat
///
/// [`liquidity_offset_half_life`]: Self::liquidity_offset_half_life
3: db948baf4 = 3: 8c61f751c f add decay example
4: ecdcffa3c = 4: e433f0cad f missing link
5: 877d9cf5c = 5: 0ab1e4b5f f address docs feedback
6: a982b27f7 = 6: f44f36989 f nits from tnull
7: 48976ddf2 = 7: 98620f372 f docs
8: 4fb50c34e = 8: f5a967c65 f docs
9: 2e572e8af ! 9: 202663463 Increase the default `liquidity_offset_half_life` to six hours
@@ lightning/src/routing/scoring.rs: impl ProbabilisticScoringParameters {
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
liquidity_penalty_amount_multiplier_msat: 0,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 0,
+ historical_liquidity_penalty_multiplier_msat: 0,
+ historical_liquidity_penalty_amount_multiplier_msat: 0,
@@ lightning/src/routing/scoring.rs: impl Default for ProbabilisticScoringParameters {
base_penalty_msat: 500,
base_penalty_amount_multiplier_msat: 8192,
- liquidity_penalty_multiplier_msat: 40_000,
+ liquidity_penalty_multiplier_msat: 30_000,
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
- liquidity_penalty_amount_multiplier_msat: 256,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 250,
+ liquidity_penalty_amount_multiplier_msat: 192,
+ historical_liquidity_penalty_multiplier_msat: 10_000,
+ historical_liquidity_penalty_amount_multiplier_msat: 64,

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 2026634 to bd6a5f1CompareOctober 11, 2022 15:28
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Then squashed with no further changes:

$ git diff-tree -U1 20266346 bd6a5f17
$

tnull
tnull previously approved these changes Oct 11, 2022
jkczyz
jkczyz previously approved these changes Oct 11, 2022
We had some user confusion on how the probabilistic scorer works,
especially in reference to the half-life parameter. This attempts
to clarify how the bounds work, and how they are decayed.
Even at relatively high payment volumes, decaying knowledge of each
individual channel every hour causes aggressive retrying of
channels as we quickly forget the state of a channel. Even with the
historical tracker, this isn't fully remedied, as we'll track the
history bounds with the decayed value.
Instead, we decay every six hours here, reducing how often we'll
retry a channel due to decay.
In addition to this, the decay likely needs to be substantially
more linear, as tracked in lightningdevkit#1752.
@TheBlueMatt
TheBlueMatt dismissed stale reviews from jkczyz and tnull via 9e84966October 11, 2022 18:25
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from bd6a5f1 to 9e84966CompareOctober 11, 2022 18:25
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Oops, had screwed up the docs, force-pushed:

$ git diff-tree -U1 bd6a5f17b 9e8496659
diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
index f9c188857..fefdebfcc 100644
--- a/lightning/src/routing/scoring.rs+++ b/lightning/src/routing/scoring.rs@@ -349,4 +349,4 @@ type ConfiguredTime = Eternity;
/// [`liquidity_offset_half_life`]: ProbabilisticScoringParameters::liquidity_offset_half_life
-/// [`liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_multiplier_msat-/// [`liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_amount_multiplier_msat+/// [`historical_liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_multiplier_msat+/// [`historical_liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_amount_multiplier_msat

@TheBlueMatt
TheBlueMatt merged commit fdfd4f0 into lightningdevkit:mainOct 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@tnull@jkczyz@tlulu
, '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

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours. - #1754

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs
Oct 11, 2022
Merged

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours.#1754
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Depends on #1625.

Comment threadlightning/src/routing/scoring.rs Outdated
/// with high payment volume or that actively probe the [`NetworkGraph`]. Nodes with low payment
/// volume are more likely to experience failed payment paths, which would need to be retried.
/// This probability is converted into a linear score by `log10`'ing it and multiplying it with the
/// [`liquidity_penalty_multiplier_msat`] and [`liquidity_penalty_amount_multiplier_msat`]

@tlulutluluOct 5, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A formula would be easier to read imo
amount = log10(probability) * liquidity_penalty_multiplier_msat * liquidity_penalty_amount_multiplier_msat

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it okay if we just leave the formulas on the docs for parameters themselves?

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.

Also, should be -log10.

Comment threadlightning/src/routing/scoring.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 8702f17 to 14432b8CompareOctober 5, 2022 19:40
tlulu
tlulu previously approved these changes Oct 5, 2022
tlulu
tlulu previously approved these changes Oct 5, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +322 to +324
/// These bounds are then used to determine a success probability using the formula from
/// *Optimally Reliable & Cheap Payment Flows on the Lightning Network* by Rene Pickhardt
/// and Stefan Richter [[1]] (i.e. `(payment amount + lower-bound) / (upper-bound - lower-bound)`).

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.

Technically, the numerator and denominator both need + 1 to avoid division by zero.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I figured skip over that level of detail here.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +317 to +321
/// Channels are tracked with upper- and lower- liquidity bounds - when an HTLC fails at a channel,
/// we learn that the upper-bound on the available liquidity is lower than the amount of the HTLC.
/// When a payment is forwarded through a channel (but fails later in the route), we learn the
/// lower-bound on the channel's available liquidity must be at least the value of the HTLC.

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.

There is a third case where a successful payment lowers the upper bound by the amount.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I thought about spelling it out, but that just seemed like too much detail. I can add something like "We also learn some information about upper-bounds after a successful payment." or so?

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@codecov-commenter

codecov-commenter commented Oct 5, 2022

Copy link
Copy Markdown

Codecov Report

Base: 90.79% // Head: 90.80% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (bd6a5f1) compared to base (6738fd5).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head bd6a5f1 differs from pull request most recent head 9e84966. Consider uploading reports for the commit 9e84966 to get more accurate results

Additional details and impacted files
@@ Coverage Diff @@## main #1754 +/- ##
=======================================
Coverage 90.79% 90.80% =======================================
Files 87 87 Lines 46969 46969 Branches 46969 46969 =======================================
+ Hits 42646 42648 +2 + Misses 4323 4321 -2 
Impacted FilesCoverage Δ
lightning/src/routing/scoring.rs96.61% <100.00%> (ø)
lightning/src/chain/onchaintx.rs94.71% <0.00%> (-0.92%)⬇️
lightning/src/ln/functional_tests.rs97.05% <0.00%> (+0.04%)⬆️
lightning-net-tokio/src/lib.rs77.64% <0.00%> (+0.90%)⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tnull
tnull self-requested a review October 6, 2022 09:45
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
/// gives tighter bounds on the channel liquidity balance. Thus, halving the offsets decreases
/// the certainty of the channel liquidity balance.
/// For example, if the channel's capacity is 1 million sats, and the current upper- and lower-
/// liquidity bounds are 200,000 sats and 600,000 sats, after this amount of time the upper-

@tnulltnullOct 6, 2022

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.

nit: Move "after this amount of time" to the end of the sentence. Also, since this is an example it would be nice to make it more concrete, e.g., use the default values or variable name?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Well, the documentation is on the variable itself, so it seems strange to refer to it "in the third person"?

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.

Alright, still think would read better if "after this amount of time" would be at the end of the sentence, but feel to leave as is if you prefer.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 25766b1 to e01091cCompareOctober 6, 2022 17:46

@tnulltnull 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.

Looks good mod failing CI.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from e01091c to 3e14f9eCompareOctober 7, 2022 17:34
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased after #1625 landed, should pass CI now.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 3e14f9e to 0aec44bCompareOctober 7, 2022 21:06
tnull
tnull previously approved these changes Oct 8, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +416 to +417
/// Because halving the liquidity bounds grows the uncertainty on the channel's liquidity,
/// penalties for payments that are within the liquidity bounds will be decreased. See the

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 think whether or not penalties are decrease depends on whether the amount lies closer to the upper bound or the lower bound.

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.

Would consider re-wording as:

"Because halving the liquidity bounds increases the uncertainty of the channel's liquidity, the penalty for an amount within the new bounds may increase or decrease depending on whether the new success probability decreased or increased, respectively."

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it worth specifying the last clause? I just changed it to "it may change", basically.

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.

SGTM

jkczyz
jkczyz previously approved these changes Oct 11, 2022
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased on usptream:

$ git range-diff 7544030bb63fee6484fc178bb2ac8f382fe3b5b1...2e572e8affe4688c84ea15c5d3b836296ee60392 6738fd56cf74227a12ecf6f3934515f42f9f9452...2026634631816b8bfac4e20993fa0814e53dbe00
1: 56d91c276 = 1: d7d7c2e61 Rewrite documentation some on `ProbabilisticScorer`
2: a7502e09a ! 2: 37783f944 f point to params for formulas
@@ lightning/src/routing/scoring.rs: pub struct ProbabilisticScoringParameters {
///
+ /// `-log10(success_probability) * liquidity_penalty_multiplier_msat`
+ ///
- /// Default value: 40,000 msat
+ /// Default value: 30,000 msat
///
/// [`liquidity_offset_half_life`]: Self::liquidity_offset_half_life
3: db948baf4 = 3: 8c61f751c f add decay example
4: ecdcffa3c = 4: e433f0cad f missing link
5: 877d9cf5c = 5: 0ab1e4b5f f address docs feedback
6: a982b27f7 = 6: f44f36989 f nits from tnull
7: 48976ddf2 = 7: 98620f372 f docs
8: 4fb50c34e = 8: f5a967c65 f docs
9: 2e572e8af ! 9: 202663463 Increase the default `liquidity_offset_half_life` to six hours
@@ lightning/src/routing/scoring.rs: impl ProbabilisticScoringParameters {
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
liquidity_penalty_amount_multiplier_msat: 0,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 0,
+ historical_liquidity_penalty_multiplier_msat: 0,
+ historical_liquidity_penalty_amount_multiplier_msat: 0,
@@ lightning/src/routing/scoring.rs: impl Default for ProbabilisticScoringParameters {
base_penalty_msat: 500,
base_penalty_amount_multiplier_msat: 8192,
- liquidity_penalty_multiplier_msat: 40_000,
+ liquidity_penalty_multiplier_msat: 30_000,
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
- liquidity_penalty_amount_multiplier_msat: 256,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 250,
+ liquidity_penalty_amount_multiplier_msat: 192,
+ historical_liquidity_penalty_multiplier_msat: 10_000,
+ historical_liquidity_penalty_amount_multiplier_msat: 64,

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 2026634 to bd6a5f1CompareOctober 11, 2022 15:28
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Then squashed with no further changes:

$ git diff-tree -U1 20266346 bd6a5f17
$

tnull
tnull previously approved these changes Oct 11, 2022
jkczyz
jkczyz previously approved these changes Oct 11, 2022
We had some user confusion on how the probabilistic scorer works,
especially in reference to the half-life parameter. This attempts
to clarify how the bounds work, and how they are decayed.
Even at relatively high payment volumes, decaying knowledge of each
individual channel every hour causes aggressive retrying of
channels as we quickly forget the state of a channel. Even with the
historical tracker, this isn't fully remedied, as we'll track the
history bounds with the decayed value.
Instead, we decay every six hours here, reducing how often we'll
retry a channel due to decay.
In addition to this, the decay likely needs to be substantially
more linear, as tracked in lightningdevkit#1752.
@TheBlueMatt
TheBlueMatt dismissed stale reviews from jkczyz and tnull via 9e84966October 11, 2022 18:25
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from bd6a5f1 to 9e84966CompareOctober 11, 2022 18:25
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Oops, had screwed up the docs, force-pushed:

$ git diff-tree -U1 bd6a5f17b 9e8496659
diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
index f9c188857..fefdebfcc 100644
--- a/lightning/src/routing/scoring.rs+++ b/lightning/src/routing/scoring.rs@@ -349,4 +349,4 @@ type ConfiguredTime = Eternity;
/// [`liquidity_offset_half_life`]: ProbabilisticScoringParameters::liquidity_offset_half_life
-/// [`liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_multiplier_msat-/// [`liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_amount_multiplier_msat+/// [`historical_liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_multiplier_msat+/// [`historical_liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_amount_multiplier_msat

@TheBlueMatt
TheBlueMatt merged commit fdfd4f0 into lightningdevkit:mainOct 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@tnull@jkczyz@tlulu
, '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

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours. - #1754

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs
Oct 11, 2022
Merged

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours.#1754
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Depends on #1625.

Comment threadlightning/src/routing/scoring.rs Outdated
/// with high payment volume or that actively probe the [`NetworkGraph`]. Nodes with low payment
/// volume are more likely to experience failed payment paths, which would need to be retried.
/// This probability is converted into a linear score by `log10`'ing it and multiplying it with the
/// [`liquidity_penalty_multiplier_msat`] and [`liquidity_penalty_amount_multiplier_msat`]

@tlulutluluOct 5, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A formula would be easier to read imo
amount = log10(probability) * liquidity_penalty_multiplier_msat * liquidity_penalty_amount_multiplier_msat

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it okay if we just leave the formulas on the docs for parameters themselves?

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.

Also, should be -log10.

Comment threadlightning/src/routing/scoring.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 8702f17 to 14432b8CompareOctober 5, 2022 19:40
tlulu
tlulu previously approved these changes Oct 5, 2022
tlulu
tlulu previously approved these changes Oct 5, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +322 to +324
/// These bounds are then used to determine a success probability using the formula from
/// *Optimally Reliable & Cheap Payment Flows on the Lightning Network* by Rene Pickhardt
/// and Stefan Richter [[1]] (i.e. `(payment amount + lower-bound) / (upper-bound - lower-bound)`).

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.

Technically, the numerator and denominator both need + 1 to avoid division by zero.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I figured skip over that level of detail here.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +317 to +321
/// Channels are tracked with upper- and lower- liquidity bounds - when an HTLC fails at a channel,
/// we learn that the upper-bound on the available liquidity is lower than the amount of the HTLC.
/// When a payment is forwarded through a channel (but fails later in the route), we learn the
/// lower-bound on the channel's available liquidity must be at least the value of the HTLC.

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.

There is a third case where a successful payment lowers the upper bound by the amount.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I thought about spelling it out, but that just seemed like too much detail. I can add something like "We also learn some information about upper-bounds after a successful payment." or so?

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@codecov-commenter

codecov-commenter commented Oct 5, 2022

Copy link
Copy Markdown

Codecov Report

Base: 90.79% // Head: 90.80% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (bd6a5f1) compared to base (6738fd5).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head bd6a5f1 differs from pull request most recent head 9e84966. Consider uploading reports for the commit 9e84966 to get more accurate results

Additional details and impacted files
@@ Coverage Diff @@## main #1754 +/- ##
=======================================
Coverage 90.79% 90.80% =======================================
Files 87 87 Lines 46969 46969 Branches 46969 46969 =======================================
+ Hits 42646 42648 +2 + Misses 4323 4321 -2 
Impacted FilesCoverage Δ
lightning/src/routing/scoring.rs96.61% <100.00%> (ø)
lightning/src/chain/onchaintx.rs94.71% <0.00%> (-0.92%)⬇️
lightning/src/ln/functional_tests.rs97.05% <0.00%> (+0.04%)⬆️
lightning-net-tokio/src/lib.rs77.64% <0.00%> (+0.90%)⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tnull
tnull self-requested a review October 6, 2022 09:45
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
/// gives tighter bounds on the channel liquidity balance. Thus, halving the offsets decreases
/// the certainty of the channel liquidity balance.
/// For example, if the channel's capacity is 1 million sats, and the current upper- and lower-
/// liquidity bounds are 200,000 sats and 600,000 sats, after this amount of time the upper-

@tnulltnullOct 6, 2022

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.

nit: Move "after this amount of time" to the end of the sentence. Also, since this is an example it would be nice to make it more concrete, e.g., use the default values or variable name?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Well, the documentation is on the variable itself, so it seems strange to refer to it "in the third person"?

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.

Alright, still think would read better if "after this amount of time" would be at the end of the sentence, but feel to leave as is if you prefer.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 25766b1 to e01091cCompareOctober 6, 2022 17:46

@tnulltnull 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.

Looks good mod failing CI.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from e01091c to 3e14f9eCompareOctober 7, 2022 17:34
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased after #1625 landed, should pass CI now.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 3e14f9e to 0aec44bCompareOctober 7, 2022 21:06
tnull
tnull previously approved these changes Oct 8, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +416 to +417
/// Because halving the liquidity bounds grows the uncertainty on the channel's liquidity,
/// penalties for payments that are within the liquidity bounds will be decreased. See the

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 think whether or not penalties are decrease depends on whether the amount lies closer to the upper bound or the lower bound.

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.

Would consider re-wording as:

"Because halving the liquidity bounds increases the uncertainty of the channel's liquidity, the penalty for an amount within the new bounds may increase or decrease depending on whether the new success probability decreased or increased, respectively."

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it worth specifying the last clause? I just changed it to "it may change", basically.

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.

SGTM

jkczyz
jkczyz previously approved these changes Oct 11, 2022
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased on usptream:

$ git range-diff 7544030bb63fee6484fc178bb2ac8f382fe3b5b1...2e572e8affe4688c84ea15c5d3b836296ee60392 6738fd56cf74227a12ecf6f3934515f42f9f9452...2026634631816b8bfac4e20993fa0814e53dbe00
1: 56d91c276 = 1: d7d7c2e61 Rewrite documentation some on `ProbabilisticScorer`
2: a7502e09a ! 2: 37783f944 f point to params for formulas
@@ lightning/src/routing/scoring.rs: pub struct ProbabilisticScoringParameters {
///
+ /// `-log10(success_probability) * liquidity_penalty_multiplier_msat`
+ ///
- /// Default value: 40,000 msat
+ /// Default value: 30,000 msat
///
/// [`liquidity_offset_half_life`]: Self::liquidity_offset_half_life
3: db948baf4 = 3: 8c61f751c f add decay example
4: ecdcffa3c = 4: e433f0cad f missing link
5: 877d9cf5c = 5: 0ab1e4b5f f address docs feedback
6: a982b27f7 = 6: f44f36989 f nits from tnull
7: 48976ddf2 = 7: 98620f372 f docs
8: 4fb50c34e = 8: f5a967c65 f docs
9: 2e572e8af ! 9: 202663463 Increase the default `liquidity_offset_half_life` to six hours
@@ lightning/src/routing/scoring.rs: impl ProbabilisticScoringParameters {
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
liquidity_penalty_amount_multiplier_msat: 0,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 0,
+ historical_liquidity_penalty_multiplier_msat: 0,
+ historical_liquidity_penalty_amount_multiplier_msat: 0,
@@ lightning/src/routing/scoring.rs: impl Default for ProbabilisticScoringParameters {
base_penalty_msat: 500,
base_penalty_amount_multiplier_msat: 8192,
- liquidity_penalty_multiplier_msat: 40_000,
+ liquidity_penalty_multiplier_msat: 30_000,
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
- liquidity_penalty_amount_multiplier_msat: 256,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 250,
+ liquidity_penalty_amount_multiplier_msat: 192,
+ historical_liquidity_penalty_multiplier_msat: 10_000,
+ historical_liquidity_penalty_amount_multiplier_msat: 64,

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 2026634 to bd6a5f1CompareOctober 11, 2022 15:28
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Then squashed with no further changes:

$ git diff-tree -U1 20266346 bd6a5f17
$

tnull
tnull previously approved these changes Oct 11, 2022
jkczyz
jkczyz previously approved these changes Oct 11, 2022
We had some user confusion on how the probabilistic scorer works,
especially in reference to the half-life parameter. This attempts
to clarify how the bounds work, and how they are decayed.
Even at relatively high payment volumes, decaying knowledge of each
individual channel every hour causes aggressive retrying of
channels as we quickly forget the state of a channel. Even with the
historical tracker, this isn't fully remedied, as we'll track the
history bounds with the decayed value.
Instead, we decay every six hours here, reducing how often we'll
retry a channel due to decay.
In addition to this, the decay likely needs to be substantially
more linear, as tracked in lightningdevkit#1752.
@TheBlueMatt
TheBlueMatt dismissed stale reviews from jkczyz and tnull via 9e84966October 11, 2022 18:25
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from bd6a5f1 to 9e84966CompareOctober 11, 2022 18:25
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Oops, had screwed up the docs, force-pushed:

$ git diff-tree -U1 bd6a5f17b 9e8496659
diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
index f9c188857..fefdebfcc 100644
--- a/lightning/src/routing/scoring.rs+++ b/lightning/src/routing/scoring.rs@@ -349,4 +349,4 @@ type ConfiguredTime = Eternity;
/// [`liquidity_offset_half_life`]: ProbabilisticScoringParameters::liquidity_offset_half_life
-/// [`liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_multiplier_msat-/// [`liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_amount_multiplier_msat+/// [`historical_liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_multiplier_msat+/// [`historical_liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_amount_multiplier_msat

@TheBlueMatt
TheBlueMatt merged commit fdfd4f0 into lightningdevkit:mainOct 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@tnull@jkczyz@tlulu
, '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

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours. - #1754

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs
Oct 11, 2022
Merged

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours.#1754
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Depends on #1625.

Comment threadlightning/src/routing/scoring.rs Outdated
/// with high payment volume or that actively probe the [`NetworkGraph`]. Nodes with low payment
/// volume are more likely to experience failed payment paths, which would need to be retried.
/// This probability is converted into a linear score by `log10`'ing it and multiplying it with the
/// [`liquidity_penalty_multiplier_msat`] and [`liquidity_penalty_amount_multiplier_msat`]

@tlulutluluOct 5, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A formula would be easier to read imo
amount = log10(probability) * liquidity_penalty_multiplier_msat * liquidity_penalty_amount_multiplier_msat

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it okay if we just leave the formulas on the docs for parameters themselves?

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.

Also, should be -log10.

Comment threadlightning/src/routing/scoring.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 8702f17 to 14432b8CompareOctober 5, 2022 19:40
tlulu
tlulu previously approved these changes Oct 5, 2022
tlulu
tlulu previously approved these changes Oct 5, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +322 to +324
/// These bounds are then used to determine a success probability using the formula from
/// *Optimally Reliable & Cheap Payment Flows on the Lightning Network* by Rene Pickhardt
/// and Stefan Richter [[1]] (i.e. `(payment amount + lower-bound) / (upper-bound - lower-bound)`).

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.

Technically, the numerator and denominator both need + 1 to avoid division by zero.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I figured skip over that level of detail here.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +317 to +321
/// Channels are tracked with upper- and lower- liquidity bounds - when an HTLC fails at a channel,
/// we learn that the upper-bound on the available liquidity is lower than the amount of the HTLC.
/// When a payment is forwarded through a channel (but fails later in the route), we learn the
/// lower-bound on the channel's available liquidity must be at least the value of the HTLC.

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.

There is a third case where a successful payment lowers the upper bound by the amount.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I thought about spelling it out, but that just seemed like too much detail. I can add something like "We also learn some information about upper-bounds after a successful payment." or so?

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@codecov-commenter

codecov-commenter commented Oct 5, 2022

Copy link
Copy Markdown

Codecov Report

Base: 90.79% // Head: 90.80% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (bd6a5f1) compared to base (6738fd5).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head bd6a5f1 differs from pull request most recent head 9e84966. Consider uploading reports for the commit 9e84966 to get more accurate results

Additional details and impacted files
@@ Coverage Diff @@## main #1754 +/- ##
=======================================
Coverage 90.79% 90.80% =======================================
Files 87 87 Lines 46969 46969 Branches 46969 46969 =======================================
+ Hits 42646 42648 +2 + Misses 4323 4321 -2 
Impacted FilesCoverage Δ
lightning/src/routing/scoring.rs96.61% <100.00%> (ø)
lightning/src/chain/onchaintx.rs94.71% <0.00%> (-0.92%)⬇️
lightning/src/ln/functional_tests.rs97.05% <0.00%> (+0.04%)⬆️
lightning-net-tokio/src/lib.rs77.64% <0.00%> (+0.90%)⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tnull
tnull self-requested a review October 6, 2022 09:45
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
/// gives tighter bounds on the channel liquidity balance. Thus, halving the offsets decreases
/// the certainty of the channel liquidity balance.
/// For example, if the channel's capacity is 1 million sats, and the current upper- and lower-
/// liquidity bounds are 200,000 sats and 600,000 sats, after this amount of time the upper-

@tnulltnullOct 6, 2022

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.

nit: Move "after this amount of time" to the end of the sentence. Also, since this is an example it would be nice to make it more concrete, e.g., use the default values or variable name?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Well, the documentation is on the variable itself, so it seems strange to refer to it "in the third person"?

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.

Alright, still think would read better if "after this amount of time" would be at the end of the sentence, but feel to leave as is if you prefer.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 25766b1 to e01091cCompareOctober 6, 2022 17:46

@tnulltnull 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.

Looks good mod failing CI.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from e01091c to 3e14f9eCompareOctober 7, 2022 17:34
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased after #1625 landed, should pass CI now.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 3e14f9e to 0aec44bCompareOctober 7, 2022 21:06
tnull
tnull previously approved these changes Oct 8, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +416 to +417
/// Because halving the liquidity bounds grows the uncertainty on the channel's liquidity,
/// penalties for payments that are within the liquidity bounds will be decreased. See the

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 think whether or not penalties are decrease depends on whether the amount lies closer to the upper bound or the lower bound.

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.

Would consider re-wording as:

"Because halving the liquidity bounds increases the uncertainty of the channel's liquidity, the penalty for an amount within the new bounds may increase or decrease depending on whether the new success probability decreased or increased, respectively."

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it worth specifying the last clause? I just changed it to "it may change", basically.

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.

SGTM

jkczyz
jkczyz previously approved these changes Oct 11, 2022
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased on usptream:

$ git range-diff 7544030bb63fee6484fc178bb2ac8f382fe3b5b1...2e572e8affe4688c84ea15c5d3b836296ee60392 6738fd56cf74227a12ecf6f3934515f42f9f9452...2026634631816b8bfac4e20993fa0814e53dbe00
1: 56d91c276 = 1: d7d7c2e61 Rewrite documentation some on `ProbabilisticScorer`
2: a7502e09a ! 2: 37783f944 f point to params for formulas
@@ lightning/src/routing/scoring.rs: pub struct ProbabilisticScoringParameters {
///
+ /// `-log10(success_probability) * liquidity_penalty_multiplier_msat`
+ ///
- /// Default value: 40,000 msat
+ /// Default value: 30,000 msat
///
/// [`liquidity_offset_half_life`]: Self::liquidity_offset_half_life
3: db948baf4 = 3: 8c61f751c f add decay example
4: ecdcffa3c = 4: e433f0cad f missing link
5: 877d9cf5c = 5: 0ab1e4b5f f address docs feedback
6: a982b27f7 = 6: f44f36989 f nits from tnull
7: 48976ddf2 = 7: 98620f372 f docs
8: 4fb50c34e = 8: f5a967c65 f docs
9: 2e572e8af ! 9: 202663463 Increase the default `liquidity_offset_half_life` to six hours
@@ lightning/src/routing/scoring.rs: impl ProbabilisticScoringParameters {
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
liquidity_penalty_amount_multiplier_msat: 0,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 0,
+ historical_liquidity_penalty_multiplier_msat: 0,
+ historical_liquidity_penalty_amount_multiplier_msat: 0,
@@ lightning/src/routing/scoring.rs: impl Default for ProbabilisticScoringParameters {
base_penalty_msat: 500,
base_penalty_amount_multiplier_msat: 8192,
- liquidity_penalty_multiplier_msat: 40_000,
+ liquidity_penalty_multiplier_msat: 30_000,
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
- liquidity_penalty_amount_multiplier_msat: 256,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 250,
+ liquidity_penalty_amount_multiplier_msat: 192,
+ historical_liquidity_penalty_multiplier_msat: 10_000,
+ historical_liquidity_penalty_amount_multiplier_msat: 64,

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 2026634 to bd6a5f1CompareOctober 11, 2022 15:28
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Then squashed with no further changes:

$ git diff-tree -U1 20266346 bd6a5f17
$

tnull
tnull previously approved these changes Oct 11, 2022
jkczyz
jkczyz previously approved these changes Oct 11, 2022
We had some user confusion on how the probabilistic scorer works,
especially in reference to the half-life parameter. This attempts
to clarify how the bounds work, and how they are decayed.
Even at relatively high payment volumes, decaying knowledge of each
individual channel every hour causes aggressive retrying of
channels as we quickly forget the state of a channel. Even with the
historical tracker, this isn't fully remedied, as we'll track the
history bounds with the decayed value.
Instead, we decay every six hours here, reducing how often we'll
retry a channel due to decay.
In addition to this, the decay likely needs to be substantially
more linear, as tracked in lightningdevkit#1752.
@TheBlueMatt
TheBlueMatt dismissed stale reviews from jkczyz and tnull via 9e84966October 11, 2022 18:25
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from bd6a5f1 to 9e84966CompareOctober 11, 2022 18:25
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Oops, had screwed up the docs, force-pushed:

$ git diff-tree -U1 bd6a5f17b 9e8496659
diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
index f9c188857..fefdebfcc 100644
--- a/lightning/src/routing/scoring.rs+++ b/lightning/src/routing/scoring.rs@@ -349,4 +349,4 @@ type ConfiguredTime = Eternity;
/// [`liquidity_offset_half_life`]: ProbabilisticScoringParameters::liquidity_offset_half_life
-/// [`liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_multiplier_msat-/// [`liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_amount_multiplier_msat+/// [`historical_liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_multiplier_msat+/// [`historical_liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_amount_multiplier_msat

@TheBlueMatt
TheBlueMatt merged commit fdfd4f0 into lightningdevkit:mainOct 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@tnull@jkczyz@tlulu
, '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

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours. - #1754

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs
Oct 11, 2022
Merged

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours.#1754
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Depends on #1625.

Comment threadlightning/src/routing/scoring.rs Outdated
/// with high payment volume or that actively probe the [`NetworkGraph`]. Nodes with low payment
/// volume are more likely to experience failed payment paths, which would need to be retried.
/// This probability is converted into a linear score by `log10`'ing it and multiplying it with the
/// [`liquidity_penalty_multiplier_msat`] and [`liquidity_penalty_amount_multiplier_msat`]

@tlulutluluOct 5, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A formula would be easier to read imo
amount = log10(probability) * liquidity_penalty_multiplier_msat * liquidity_penalty_amount_multiplier_msat

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it okay if we just leave the formulas on the docs for parameters themselves?

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.

Also, should be -log10.

Comment threadlightning/src/routing/scoring.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 8702f17 to 14432b8CompareOctober 5, 2022 19:40
tlulu
tlulu previously approved these changes Oct 5, 2022
tlulu
tlulu previously approved these changes Oct 5, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +322 to +324
/// These bounds are then used to determine a success probability using the formula from
/// *Optimally Reliable & Cheap Payment Flows on the Lightning Network* by Rene Pickhardt
/// and Stefan Richter [[1]] (i.e. `(payment amount + lower-bound) / (upper-bound - lower-bound)`).

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.

Technically, the numerator and denominator both need + 1 to avoid division by zero.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I figured skip over that level of detail here.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +317 to +321
/// Channels are tracked with upper- and lower- liquidity bounds - when an HTLC fails at a channel,
/// we learn that the upper-bound on the available liquidity is lower than the amount of the HTLC.
/// When a payment is forwarded through a channel (but fails later in the route), we learn the
/// lower-bound on the channel's available liquidity must be at least the value of the HTLC.

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.

There is a third case where a successful payment lowers the upper bound by the amount.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I thought about spelling it out, but that just seemed like too much detail. I can add something like "We also learn some information about upper-bounds after a successful payment." or so?

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@codecov-commenter

codecov-commenter commented Oct 5, 2022

Copy link
Copy Markdown

Codecov Report

Base: 90.79% // Head: 90.80% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (bd6a5f1) compared to base (6738fd5).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head bd6a5f1 differs from pull request most recent head 9e84966. Consider uploading reports for the commit 9e84966 to get more accurate results

Additional details and impacted files
@@ Coverage Diff @@## main #1754 +/- ##
=======================================
Coverage 90.79% 90.80% =======================================
Files 87 87 Lines 46969 46969 Branches 46969 46969 =======================================
+ Hits 42646 42648 +2 + Misses 4323 4321 -2 
Impacted FilesCoverage Δ
lightning/src/routing/scoring.rs96.61% <100.00%> (ø)
lightning/src/chain/onchaintx.rs94.71% <0.00%> (-0.92%)⬇️
lightning/src/ln/functional_tests.rs97.05% <0.00%> (+0.04%)⬆️
lightning-net-tokio/src/lib.rs77.64% <0.00%> (+0.90%)⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tnull
tnull self-requested a review October 6, 2022 09:45
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
/// gives tighter bounds on the channel liquidity balance. Thus, halving the offsets decreases
/// the certainty of the channel liquidity balance.
/// For example, if the channel's capacity is 1 million sats, and the current upper- and lower-
/// liquidity bounds are 200,000 sats and 600,000 sats, after this amount of time the upper-

@tnulltnullOct 6, 2022

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.

nit: Move "after this amount of time" to the end of the sentence. Also, since this is an example it would be nice to make it more concrete, e.g., use the default values or variable name?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Well, the documentation is on the variable itself, so it seems strange to refer to it "in the third person"?

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.

Alright, still think would read better if "after this amount of time" would be at the end of the sentence, but feel to leave as is if you prefer.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 25766b1 to e01091cCompareOctober 6, 2022 17:46

@tnulltnull 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.

Looks good mod failing CI.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from e01091c to 3e14f9eCompareOctober 7, 2022 17:34
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased after #1625 landed, should pass CI now.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 3e14f9e to 0aec44bCompareOctober 7, 2022 21:06
tnull
tnull previously approved these changes Oct 8, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +416 to +417
/// Because halving the liquidity bounds grows the uncertainty on the channel's liquidity,
/// penalties for payments that are within the liquidity bounds will be decreased. See the

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 think whether or not penalties are decrease depends on whether the amount lies closer to the upper bound or the lower bound.

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.

Would consider re-wording as:

"Because halving the liquidity bounds increases the uncertainty of the channel's liquidity, the penalty for an amount within the new bounds may increase or decrease depending on whether the new success probability decreased or increased, respectively."

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it worth specifying the last clause? I just changed it to "it may change", basically.

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.

SGTM

jkczyz
jkczyz previously approved these changes Oct 11, 2022
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased on usptream:

$ git range-diff 7544030bb63fee6484fc178bb2ac8f382fe3b5b1...2e572e8affe4688c84ea15c5d3b836296ee60392 6738fd56cf74227a12ecf6f3934515f42f9f9452...2026634631816b8bfac4e20993fa0814e53dbe00
1: 56d91c276 = 1: d7d7c2e61 Rewrite documentation some on `ProbabilisticScorer`
2: a7502e09a ! 2: 37783f944 f point to params for formulas
@@ lightning/src/routing/scoring.rs: pub struct ProbabilisticScoringParameters {
///
+ /// `-log10(success_probability) * liquidity_penalty_multiplier_msat`
+ ///
- /// Default value: 40,000 msat
+ /// Default value: 30,000 msat
///
/// [`liquidity_offset_half_life`]: Self::liquidity_offset_half_life
3: db948baf4 = 3: 8c61f751c f add decay example
4: ecdcffa3c = 4: e433f0cad f missing link
5: 877d9cf5c = 5: 0ab1e4b5f f address docs feedback
6: a982b27f7 = 6: f44f36989 f nits from tnull
7: 48976ddf2 = 7: 98620f372 f docs
8: 4fb50c34e = 8: f5a967c65 f docs
9: 2e572e8af ! 9: 202663463 Increase the default `liquidity_offset_half_life` to six hours
@@ lightning/src/routing/scoring.rs: impl ProbabilisticScoringParameters {
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
liquidity_penalty_amount_multiplier_msat: 0,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 0,
+ historical_liquidity_penalty_multiplier_msat: 0,
+ historical_liquidity_penalty_amount_multiplier_msat: 0,
@@ lightning/src/routing/scoring.rs: impl Default for ProbabilisticScoringParameters {
base_penalty_msat: 500,
base_penalty_amount_multiplier_msat: 8192,
- liquidity_penalty_multiplier_msat: 40_000,
+ liquidity_penalty_multiplier_msat: 30_000,
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
- liquidity_penalty_amount_multiplier_msat: 256,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 250,
+ liquidity_penalty_amount_multiplier_msat: 192,
+ historical_liquidity_penalty_multiplier_msat: 10_000,
+ historical_liquidity_penalty_amount_multiplier_msat: 64,

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 2026634 to bd6a5f1CompareOctober 11, 2022 15:28
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Then squashed with no further changes:

$ git diff-tree -U1 20266346 bd6a5f17
$

tnull
tnull previously approved these changes Oct 11, 2022
jkczyz
jkczyz previously approved these changes Oct 11, 2022
We had some user confusion on how the probabilistic scorer works,
especially in reference to the half-life parameter. This attempts
to clarify how the bounds work, and how they are decayed.
Even at relatively high payment volumes, decaying knowledge of each
individual channel every hour causes aggressive retrying of
channels as we quickly forget the state of a channel. Even with the
historical tracker, this isn't fully remedied, as we'll track the
history bounds with the decayed value.
Instead, we decay every six hours here, reducing how often we'll
retry a channel due to decay.
In addition to this, the decay likely needs to be substantially
more linear, as tracked in lightningdevkit#1752.
@TheBlueMatt
TheBlueMatt dismissed stale reviews from jkczyz and tnull via 9e84966October 11, 2022 18:25
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from bd6a5f1 to 9e84966CompareOctober 11, 2022 18:25
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Oops, had screwed up the docs, force-pushed:

$ git diff-tree -U1 bd6a5f17b 9e8496659
diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
index f9c188857..fefdebfcc 100644
--- a/lightning/src/routing/scoring.rs+++ b/lightning/src/routing/scoring.rs@@ -349,4 +349,4 @@ type ConfiguredTime = Eternity;
/// [`liquidity_offset_half_life`]: ProbabilisticScoringParameters::liquidity_offset_half_life
-/// [`liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_multiplier_msat-/// [`liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_amount_multiplier_msat+/// [`historical_liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_multiplier_msat+/// [`historical_liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_amount_multiplier_msat

@TheBlueMatt
TheBlueMatt merged commit fdfd4f0 into lightningdevkit:mainOct 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@tnull@jkczyz@tlulu
, '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

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours. - #1754

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs
Oct 11, 2022
Merged

Rewrite some documentation on ProbabilisticScorer and increase half-life to 6 hours.#1754
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-10-better-liq-halflife-docs

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Depends on #1625.

Comment threadlightning/src/routing/scoring.rs Outdated
/// with high payment volume or that actively probe the [`NetworkGraph`]. Nodes with low payment
/// volume are more likely to experience failed payment paths, which would need to be retried.
/// This probability is converted into a linear score by `log10`'ing it and multiplying it with the
/// [`liquidity_penalty_multiplier_msat`] and [`liquidity_penalty_amount_multiplier_msat`]

@tlulutluluOct 5, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A formula would be easier to read imo
amount = log10(probability) * liquidity_penalty_multiplier_msat * liquidity_penalty_amount_multiplier_msat

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it okay if we just leave the formulas on the docs for parameters themselves?

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.

Also, should be -log10.

Comment threadlightning/src/routing/scoring.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 8702f17 to 14432b8CompareOctober 5, 2022 19:40
tlulu
tlulu previously approved these changes Oct 5, 2022
tlulu
tlulu previously approved these changes Oct 5, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +322 to +324
/// These bounds are then used to determine a success probability using the formula from
/// *Optimally Reliable & Cheap Payment Flows on the Lightning Network* by Rene Pickhardt
/// and Stefan Richter [[1]] (i.e. `(payment amount + lower-bound) / (upper-bound - lower-bound)`).

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.

Technically, the numerator and denominator both need + 1 to avoid division by zero.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I figured skip over that level of detail here.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +317 to +321
/// Channels are tracked with upper- and lower- liquidity bounds - when an HTLC fails at a channel,
/// we learn that the upper-bound on the available liquidity is lower than the amount of the HTLC.
/// When a payment is forwarded through a channel (but fails later in the route), we learn the
/// lower-bound on the channel's available liquidity must be at least the value of the HTLC.

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.

There is a third case where a successful payment lowers the upper bound by the amount.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yea, I thought about spelling it out, but that just seemed like too much detail. I can add something like "We also learn some information about upper-bounds after a successful payment." or so?

Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@codecov-commenter

codecov-commenter commented Oct 5, 2022

Copy link
Copy Markdown

Codecov Report

Base: 90.79% // Head: 90.80% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (bd6a5f1) compared to base (6738fd5).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head bd6a5f1 differs from pull request most recent head 9e84966. Consider uploading reports for the commit 9e84966 to get more accurate results

Additional details and impacted files
@@ Coverage Diff @@## main #1754 +/- ##
=======================================
Coverage 90.79% 90.80% =======================================
Files 87 87 Lines 46969 46969 Branches 46969 46969 =======================================
+ Hits 42646 42648 +2 + Misses 4323 4321 -2 
Impacted FilesCoverage Δ
lightning/src/routing/scoring.rs96.61% <100.00%> (ø)
lightning/src/chain/onchaintx.rs94.71% <0.00%> (-0.92%)⬇️
lightning/src/ln/functional_tests.rs97.05% <0.00%> (+0.04%)⬆️
lightning-net-tokio/src/lib.rs77.64% <0.00%> (+0.90%)⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tnull
tnull self-requested a review October 6, 2022 09:45
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
/// gives tighter bounds on the channel liquidity balance. Thus, halving the offsets decreases
/// the certainty of the channel liquidity balance.
/// For example, if the channel's capacity is 1 million sats, and the current upper- and lower-
/// liquidity bounds are 200,000 sats and 600,000 sats, after this amount of time the upper-

@tnulltnullOct 6, 2022

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.

nit: Move "after this amount of time" to the end of the sentence. Also, since this is an example it would be nice to make it more concrete, e.g., use the default values or variable name?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Well, the documentation is on the variable itself, so it seems strange to refer to it "in the third person"?

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.

Alright, still think would read better if "after this amount of time" would be at the end of the sentence, but feel to leave as is if you prefer.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 25766b1 to e01091cCompareOctober 6, 2022 17:46

@tnulltnull 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.

Looks good mod failing CI.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from e01091c to 3e14f9eCompareOctober 7, 2022 17:34
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased after #1625 landed, should pass CI now.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 3e14f9e to 0aec44bCompareOctober 7, 2022 21:06
tnull
tnull previously approved these changes Oct 8, 2022
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +416 to +417
/// Because halving the liquidity bounds grows the uncertainty on the channel's liquidity,
/// penalties for payments that are within the liquidity bounds will be decreased. See the

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 think whether or not penalties are decrease depends on whether the amount lies closer to the upper bound or the lower bound.

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.

Would consider re-wording as:

"Because halving the liquidity bounds increases the uncertainty of the channel's liquidity, the penalty for an amount within the new bounds may increase or decrease depending on whether the new success probability decreased or increased, respectively."

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Is it worth specifying the last clause? I just changed it to "it may change", basically.

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.

SGTM

jkczyz
jkczyz previously approved these changes Oct 11, 2022
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Rebased on usptream:

$ git range-diff 7544030bb63fee6484fc178bb2ac8f382fe3b5b1...2e572e8affe4688c84ea15c5d3b836296ee60392 6738fd56cf74227a12ecf6f3934515f42f9f9452...2026634631816b8bfac4e20993fa0814e53dbe00
1: 56d91c276 = 1: d7d7c2e61 Rewrite documentation some on `ProbabilisticScorer`
2: a7502e09a ! 2: 37783f944 f point to params for formulas
@@ lightning/src/routing/scoring.rs: pub struct ProbabilisticScoringParameters {
///
+ /// `-log10(success_probability) * liquidity_penalty_multiplier_msat`
+ ///
- /// Default value: 40,000 msat
+ /// Default value: 30,000 msat
///
/// [`liquidity_offset_half_life`]: Self::liquidity_offset_half_life
3: db948baf4 = 3: 8c61f751c f add decay example
4: ecdcffa3c = 4: e433f0cad f missing link
5: 877d9cf5c = 5: 0ab1e4b5f f address docs feedback
6: a982b27f7 = 6: f44f36989 f nits from tnull
7: 48976ddf2 = 7: 98620f372 f docs
8: 4fb50c34e = 8: f5a967c65 f docs
9: 2e572e8af ! 9: 202663463 Increase the default `liquidity_offset_half_life` to six hours
@@ lightning/src/routing/scoring.rs: impl ProbabilisticScoringParameters {
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
liquidity_penalty_amount_multiplier_msat: 0,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 0,
+ historical_liquidity_penalty_multiplier_msat: 0,
+ historical_liquidity_penalty_amount_multiplier_msat: 0,
@@ lightning/src/routing/scoring.rs: impl Default for ProbabilisticScoringParameters {
base_penalty_msat: 500,
base_penalty_amount_multiplier_msat: 8192,
- liquidity_penalty_multiplier_msat: 40_000,
+ liquidity_penalty_multiplier_msat: 30_000,
- liquidity_offset_half_life: Duration::from_secs(3600),
+ liquidity_offset_half_life: Duration::from_secs(6 * 60 * 60),
- liquidity_penalty_amount_multiplier_msat: 256,
- manual_node_penalties: HashMap::new(),
- anti_probing_penalty_msat: 250,
+ liquidity_penalty_amount_multiplier_msat: 192,
+ historical_liquidity_penalty_multiplier_msat: 10_000,
+ historical_liquidity_penalty_amount_multiplier_msat: 64,

@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from 2026634 to bd6a5f1CompareOctober 11, 2022 15:28
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Then squashed with no further changes:

$ git diff-tree -U1 20266346 bd6a5f17
$

tnull
tnull previously approved these changes Oct 11, 2022
jkczyz
jkczyz previously approved these changes Oct 11, 2022
We had some user confusion on how the probabilistic scorer works,
especially in reference to the half-life parameter. This attempts
to clarify how the bounds work, and how they are decayed.
Even at relatively high payment volumes, decaying knowledge of each
individual channel every hour causes aggressive retrying of
channels as we quickly forget the state of a channel. Even with the
historical tracker, this isn't fully remedied, as we'll track the
history bounds with the decayed value.
Instead, we decay every six hours here, reducing how often we'll
retry a channel due to decay.
In addition to this, the decay likely needs to be substantially
more linear, as tracked in lightningdevkit#1752.
@TheBlueMatt
TheBlueMatt dismissed stale reviews from jkczyz and tnull via 9e84966October 11, 2022 18:25
@TheBlueMatt
TheBlueMattforce-pushed the 2022-10-better-liq-halflife-docs branch from bd6a5f1 to 9e84966CompareOctober 11, 2022 18:25
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Oops, had screwed up the docs, force-pushed:

$ git diff-tree -U1 bd6a5f17b 9e8496659
diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
index f9c188857..fefdebfcc 100644
--- a/lightning/src/routing/scoring.rs+++ b/lightning/src/routing/scoring.rs@@ -349,4 +349,4 @@ type ConfiguredTime = Eternity;
/// [`liquidity_offset_half_life`]: ProbabilisticScoringParameters::liquidity_offset_half_life
-/// [`liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_multiplier_msat-/// [`liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::liquidity_penalty_amount_multiplier_msat+/// [`historical_liquidity_penalty_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_multiplier_msat+/// [`historical_liquidity_penalty_amount_multiplier_msat`]: ProbabilisticScoringParameters::historical_liquidity_penalty_amount_multiplier_msat

@TheBlueMatt
TheBlueMatt merged commit fdfd4f0 into lightningdevkit:mainOct 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@tnull@jkczyz@tlulu