Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[RFC] Expand the precision of our log10 lookup tables + add precision #1406

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

When we send values over channels of rather substantial size, the
imprecision of our log lookup tables creates a rather substantial
non-linearity between values that round up or down one bit.

For example, with the default scoring values, sending 100k sats
over channels with 1m, 2m, 3m, and 4m sats of capacity score
rather drastically differently: 3645, 2512, 500, and 1442 msat.

Here we expand the precision of our log lookup tables rather
substantially by: (a) making the multiplier 2048 instead of 1024,
which still fits inside a u16, and (b) quadrupling the size of the
lookup table to look at the top 6 bits of an input instead of the
top 4.

This makes the scores of the same channels substantially more
linear, with values of 3613, 1977, 1474, and 1223 msat.

The same channels would be scored at 3611, 1972, 1464, and 1216
msat with a non-approximating scorer.

Further, below 1.5625% failure probability (1/64), we simply treat the failure probability as 0 and drop the log-based multipliers entirely.

That's the compelling case for this. On the other hand, one could reasonably argue that the extra precision isn't all that important and we should instead be happy with non-linearity above a reasonable percentage. This would imply something like a non-linearity at 6.25% or 3.125% success probability, where we consider anything below those as 0. That way we'd avoid the lookup table expansion.

Honestly I don't have a strong feeling either way, the lookup table is pretty small in either case, I just started down that path before I decided I needed to bound it, so that code ended up here first.

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2022

Codecov Report

Merging #1406 (b92cfe4) into main (0a0f87c) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head b92cfe4 differs from pull request most recent head 30e1922. Consider uploading reports for the commit 30e1922 to get more accurate results

@@            Coverage Diff             @@
##             main    #1406      +/-   ##
==========================================
- Coverage   90.83%   90.83%   -0.01%     
==========================================
  Files          73       73              
  Lines       41266    41927     +661     
  Branches    41266    41927     +661     
==========================================
+ Hits        37486    38084     +598     
- Misses       3780     3843      +63     
Impacted Files Coverage Δ
lightning/src/routing/scoring.rs 94.36% <100.00%> (+0.12%) ⬆️
lightning-invoice/src/de.rs 80.66% <0.00%> (-0.62%) ⬇️
lightning/src/ln/functional_tests.rs 97.08% <0.00%> (-0.07%) ⬇️
lightning-invoice/src/lib.rs 89.96% <0.00%> (+1.72%) ⬆️
lightning-block-sync/src/rpc.rs 84.66% <0.00%> (+6.15%) ⬆️
lightning-block-sync/src/rest.rs 72.00% <0.00%> (+6.54%) ⬆️

Continue to review full report at Codecov.

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

@jkczyz
Copy link
Contributor

jkczyz commented Apr 4, 2022

Here we expand the precision of our log lookup tables rather
substantially by: (a) making the multiplier 2048 instead of 1024,
which still fits inside a u16, and (b) quadrupling the size of the
lookup table to look at the top 6 bits of an input instead of the
top 4.

Technically, it is the 4 or 6 bits after the most significant bit (i.e, 2^4 or 2^6 possibilities for each of the 64 possible leading bits set to 1).

That's the compelling case for this. On the other hand, one could reasonably argue that the extra precision isn't all that important and we should instead be happy with non-linearity above a reasonable percentage. This would imply something like a non-linearity at 6.25% or 3.125% success probability, where we consider anything below those as 0. That way we'd avoid the lookup table expansion.

Should that say failure probability? I take it the non-linearity occurs when failure probability is less than 1 / 2^LOWER_BITS?

Honestly I don't have a strong feeling either way, the lookup table is pretty small in either case, I just started down that path before I decided I needed to bound it, so that code ended up here first.

I'm in favor of increasing the table size.

Comment on lines +2124 to +2125
// Shows the scores of "realistic" sends of 100k sats over channels of 1-10m sats (with a
// 50k sat reserve).
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we take into account the reserve? Maybe for first hops only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean? The capacity amounts are all 50k sats short of the round number. We probably don't need to care, I just did that case I was seeing strange scores for some channels and copied the amounts over to the test while exploring.

Copy link
Contributor

Choose a reason for hiding this comment

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

As in, the capacity passed to the scorer by get_route doesn't deduct the reserve, IIRC.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yes, I believe nodes announce their HTLC max without the reserve.

let negative_log10_times_2048 =
approx::negative_log10_times_2048(numerator, denominator);
self.combined_penalty_msat(amount_msat, negative_log10_times_2048, params)
if amount_msat - min_liquidity_msat < denominator / 64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is 64 the same as LOWER_BITS_BOUND?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, uhmmmmm, I guess? I'll go ahead and change it to reference the constant cause they're tightly tied, but it was more of an experimental thing. Honestly we should probably do LOWER_BITS_BOUND / 2 here but you end up hitting the cutoff before the penalty gets close to 500, creating a new non-linearity there.

When we send values over channels of rather substantial size, the
imprecision of our log lookup tables creates a rather substantial
non-linearity between values that round up or down one bit.

For example, with the default scoring values, sending 100k sats
over channels with 1m, 2m, 3m, and 4m sats of capacity score
rather drastically differently: 3645, 2512, 500, and 1442 msat.

Here we expand the precision of our log lookup tables rather
substantially by: (a) making the multiplier 2048 instead of 1024,
which still fits inside a u16, and (b) quadrupling the size of the
lookup table to look at the top 6 bits after the most-significant
bit of an input instead of the top 4.

This makes the scores of the same channels substantially more
linear, with values of 3613, 1977, 1474, and 1223 msat.

The same channels would be scored at 3611, 1972, 1464, and 1216
msat with a non-approximating scorer.
@TheBlueMatt
Copy link
Collaborator Author

Should that say failure probability? I take it the non-linearity occurs when failure probability is less than 1 / 2^LOWER_BITS?

Hmm, good question! I think kinda no - its non-linear because we quantize too much, ultimately, but of course if the failure probability is relatively high, the quantization error is small relative to the total amount. I think ultimately its a question of how the quantization error compares to the base penalty of 500msat, less the absolute value of it.

@TheBlueMatt TheBlueMatt force-pushed the 2022-04-scorer-precision branch from 0b725f2 to cdaa016 Compare April 10, 2022 22:50
jkczyz
jkczyz previously approved these changes Apr 11, 2022
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Looks good to squash

When we start getting a numerator and divisor particularly close to
each other, the log approximation starts to get very noisy. In
order to avoid applying scores that are basically noise (and can
range upwards of 2x the default per-hop penalty), simply consider
such cases as having a success probability of 100%.
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGTM

@TheBlueMatt TheBlueMatt merged commit 9afc1ec into lightningdevkit:main Apr 13, 2022
Sign up for free to 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