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

cmd/lncli: increase default recovery window to 2.5k #2691

Merged

Conversation

Projects
None yet
6 participants
@cfromknecht
Copy link
Collaborator

cfromknecht commented Feb 23, 2019

Increases the default window from 250 to 25k. Many
users have reported attempting recovery with the
default value only to find an empty wallet. This
change should help ensure that the first recovery
attempt succeeds for the majority of nodes that
have modest load. It might prudent to consider
increasing this value further in the future if
the issue persists or average node age increases.

@molxyz

This comment has been minimized.

Copy link

molxyz commented Feb 28, 2019

tACK!
I have tried this on testnet and it works better than using the default look-ahead number (250) to search for addresses.
On the slack a few days ago a user could not get his funds to show up in recovery by using the default look-ahead, so I suggested him to increase the number to 25000 and it worked! Here's a screenshot of what he said:

https://i.imgur.com/h540UeJ.png

@githorray

This comment has been minimized.

Copy link

githorray commented Feb 28, 2019

Let's get this done. DOW 25000!!!!

@bruceleeroy18

This comment has been minimized.

Copy link

bruceleeroy18 commented Feb 28, 2019

Thanks again molxyz!

@Roasbeef
Copy link
Member

Roasbeef left a comment

25k seems a bit excessive. For neutrino clients, it'll also slow down the recovery process by a good bit. How about 2.5k or maybe 1k? What's the typical value that we find to work based on the reports we've received for far?

@molxyz

This comment has been minimized.

Copy link

molxyz commented Mar 2, 2019

@Roasbeef Ok here's what someone said on another thread:

I've had a similar issue and have used recovery window up to 5000 to no avail.
Only issues on LND github seem to point to that as a solution, but I'd be very surprised if any Blitz users have got through more addresses than the default recovery window covers.

https://github.com/rootzoll/raspiblitz/issues/278#issuecomment-463230615

If you want to reduce this number I don't think less than 10,000 can really help.

Another issue is some people said they used less than 250 addresses but why this default number did not help them to recover their funds. I'm wondering if btcwallet has some issue that needs to be looked into?

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Mar 13, 2019

Should we limit/warn users when they create an excessive amount of unused addresses?

cmd/lncli: increase default recovery window to 2.5k
Increases the default window from 250 to 2.5k. Many
users have reported attempting recovery with the
default value only to find an empty wallet. This
change should help ensure that the first recovery
attempt succeeds for the majority of nodes that
have modest load. It might prudent to consider
increasing this value further in the future if
the issue persists or average node age increases.

@cfromknecht cfromknecht force-pushed the cfromknecht:increase-default-recovery-window branch from d7639f1 to 32c4201 Mar 15, 2019

@cfromknecht cfromknecht changed the title cmd/lncli: increase default recovery window to 25k cmd/lncli: increase default recovery window to 2.5k Mar 15, 2019

@cfromknecht

This comment has been minimized.

Copy link
Collaborator Author

cfromknecht commented Mar 15, 2019

@halseth i think it'd be worth investigating such a mechanism, can also be used to diagnose any address inflation cases

@Roasbeef window reduced to 2.5k, indeed i think scaling up by 100x is a little ambitious for a single go :) If the issue persists we can revisit

ptal

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🥨

Will want to do more tests with these values before mainnet neutrino.

@Roasbeef Roasbeef merged commit 232dd73 into lightningnetwork:master Mar 19, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.01%) to 60.201%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cfromknecht cfromknecht deleted the cfromknecht:increase-default-recovery-window branch Mar 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.