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

[WIP] [wallet] keypool restore #10830

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
8 participants
@jnewbery
Copy link
Member

commented Jul 14, 2017

This is #10240 rebased on master, with some of the comments addressed. I've tidied the code up to current style (braces, variable names, etc) and also squashed/split/re-arranged the commits to help reviewers.

Not ready for merge yet, but I'm happy to receive early feedback.

Unaddressed feedback from #10240:

  • make this work with multiwallet. Perhaps use a counter instead of a bool for pausing block requests/tip udpates.
  • Increase the size of the keypool
  • enable keypool restore for non-HD wallets.

Thanks to @jonasschnelli for his work on #10240. Paging @ryanofsky @gmaxwell @sipa since they provided feedback on 10240.

@jnewbery jnewbery force-pushed the jnewbery:pr10240 branch 3 times, most recently Jul 14, 2017

@jnewbery jnewbery force-pushed the jnewbery:pr10240 branch to cac1824 Jul 14, 2017

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jul 14, 2017

I've done my best to address the comments from #10240, but the history on that PR is rather convoluted, so I may have missed some. @ryanofsky - do you mind checking this PR for any that I may have missed?

Obviously I still need to address the three main points in the PR description.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2017

@ryanofsky - do you mind checking this PR for any that I may have missed?

Yeah, planning to look at this first thing next week. Do you know if this needs to be tagged for 0.15? It wasn't clear from earlier if this fixes a new problem introduced in this release by #9294.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2017

Hmm, it would be nice to pull in a cut-down version of this for 0.15 - skip the pause-if-wallet-falls-behind stuff and jack up the keypool default a ton (with associated performance fixes for too-often-wallet-flushing). This should get us to a place that +/- doesnt have any of the issues folks were complaining about at the meeting and will only pretty-rarely miss transactions (ie if you run out of keypool, an issue which clearly would be worse pre-hd, and we can just set keypool very large).

@fanquake fanquake added the Wallet label Jul 15, 2017

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Jul 15, 2017

@TheBlueMatt since you listed a list of what could be left out-- the most important feature of this is marking all keys up to a used key as used.

If we want to cut down the pause stuff for now, we could couple that with something that prolongs keeping an encrypted wallet unlocked while syncing/rescanning is running. Then at least an encrypted wallet recovery is a matter of unlock then rescan. Not as elegant as pausing but workable.

@instagibbs

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

needs rebase

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2017

It wasn't clear from earlier if this fixes a new problem introduced in this release by #9294.

Still curious about the relationship with #9294 (if any).

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2017

Still curious about the relationship with #9294 (if any).

Talking offline, seems there isn't really a relationship with #9294 (#9294 might even make the problem less bad) and that isn't a new problem since 0.14. Holding off on review since most of my comments from #10240 were about the pause mechanism and John was going to work on a simpler version of this change without pausing to target 0.15.

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2017

Closing. Not required for 0.15, although may revisit later.

#10882 is a simpler alternative which doesn't include the block sync pause stuff.

@jnewbery jnewbery closed this Jul 19, 2017

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.