Decrease Variance of LogNormal to Converge on Expected Value Sooner#301
Open
carlaKC wants to merge 3 commits intobitcoin-dev-project:mainfrom
Open
Decrease Variance of LogNormal to Converge on Expected Value Sooner#301carlaKC wants to merge 3 commits intobitcoin-dev-project:mainfrom
carlaKC wants to merge 3 commits intobitcoin-dev-project:mainfrom
Conversation
Previously we created our normal distribution with a very large sigma, which would result in our samples taking a long time to converge on our expected payment amount (around 100,000 samples required to get close with previous values). This commit changes our variance to still depend on payment channel size, but do so more gently. The values picked in this PR are somewhat reverse engineered - we ran a few experiments to find acceptable variance ranges, took a look at typical channel sizes in lightning and then worked backwards to find a relation between channel size and acceptable variance that would fit.
Speed up this test by using our sim clock that can speed up the simulation. The downside of this clock is that we may stop one payment over/under if we try to match exactly to the number of payments that we expect in a given period of time. Instead, we generate our set of expected payment and then run the simulation for much longer than we need to get this expected list, so we should always get at least this subset. We then just assert that the payments we get exactly match this first set.
Contributor
Author
|
Review note: I don't expect folks to go through the math of this one, Clara and I did it offline before this came up and she's been over the code. General review of code, any under/over flows that we could have with this math, test coverage etc is still very appreciated! |
Contributor
Author
|
@elnosh has pointed out that when you run this patch for ~6 months it drops down to around 20% payment successes (vs main which is around ~70%), at least on the test topologies we have. Perhaps this is a more realistic simulator, as network traffic isn't perfectly circular and channels do deplete, but it doesn't make for a very useful simulator. Going to spend some more time on design here to figure out what the best option is. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See rationale here.
This PR makes a fix to the way we pick our
LogNormaldistribution's parameters which results in our getting to an average of ourexpected_payment_amountmuch sooner.As this changes the way we pick payments, the hard-coded ordering of some of our tests has to change.
While I'm here, I also make some changes to long-running unit tests to speed them up a bit.