-
Notifications
You must be signed in to change notification settings - Fork 91
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
wallet: add ability to set account depth on first import; remove hsd soft fork #301
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy changes. merge when ready
If you are going to switch back to handshake-org/hsd v2.3, the wallet lookahead is only one byte and the default value is This will also require (I'm pretty sure) a walletDB migration for all users. Bob currently forces a lookahead of 1000 but that value isn't written to the database, meaning everyones' database still has lookahead The increased lookahead in hsd from 10 to 200 should be sufficient for most users. The most active wallets may require a second or third rescan to fully recover. I started working on a real fix for this -- re-re-scanning a block if lookahead is exceeded: but after finally talking to JJ about it and reading this bcoin issue I realized my approach was wrong. So I abandoned that and went back to reviewing other PRs and addressing dns issues, etc. I'll try to get back on it starting next week. As you've discovered it's a big mess. Not to mention that hsd must continue to support full node, spv node, and remote wallet node. The asynchronicity between wallet and node is a big issue. In fact, the current Bob construction with remote wallet node may start to cause bigger issues during rescans since the wallet is totally async from the node, and as blocks get added it can fill up memory: bcoin-org/bcoin#929 (comment) |
We've also already discussed user configurable lookahead: #203 (comment) |
Thanks for the review @pinheadmz
We did not change data type to
We are not changing lookahead value, we are only pre-generating address keys before first rescan. Lookahead value stay consistent with
happy to discuss on a separate issue as it's unrelated to this PR. Really like to be able to merge back to |
@chikeichan OHHHhhhh gotcha so sorry for misinterpreting. Yeah I see, you're setting the depth not the lookahead, that is clever. The wallet just derives a ton of keys in advance, concept ACK! |
62e3b0b
to
d9e889a
Compare
if (changeDepth) { | ||
for (let i = initChange; i < changeDepth; i++) { | ||
const key = account.deriveChange(i); | ||
await account.saveKey(b, key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think this is missing something actually: if you look at account.syncDepth()
it also sets the depth in the account metadata which then gets written to the DB. If I'm reading this correctly, what this means is that although the wallet may have indeed saved all the addresses to the database, it still thinks its depth is:
latest address index + lookahead
I'm not sure what the effect of this will be, it might just mean that the wallet will try to derive and save new addresses (that it already knows about, so no problem) up until the user-selected account depth actually matches the wallets internal account depth. In other words, the walletDB may have all the address, but the wallet/account object doesn't "know" that.
When users ask Bob for a new receive address, they will probably get the address at the wallet's internal depth index, which may be an address they have already used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see that actually in 646164f is where the accounts depths are being set. So, this is bad because then users are going to put in a high number like 10,000 and end up giving the 10,000th address away for payment, and then not realize they will always need to generate 10,000 addresses every time they recover the wallet. That's what I was getting at here: #203 (comment)
@@ -147,7 +147,7 @@ | |||
"electron-updater": "4.1.2", | |||
"history": "4.7.2", | |||
"hs-client": "https://github.com/handshake-org/hs-client.git", | |||
"hsd": "https://github.com/kyokan/hsd/tarball/1e1292d", | |||
"hsd": "https://github.com/handshake-org/hsd/tarball/a94ce87", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hsd master branch should be considered unstable, I think it'd be more prudent to follow hsd releases either from github or npm (latest is v2.3.0) I think this is fine for now, we need a better hsd release process anyway with release candidates etc to ensure we don't release a bug. Whats interesting about you using this commit in particular is that now Bob will have the "presigned REVEAL" endpoint API: handshake-org/hsd#529
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I am so late to review this, I left a few things to think about and the warning about address reuse may be worth looking into / fixing before release.
Main goal of the PR is to remove kyokan/hsd fork. Since this issue in hsd is not resolved yet, I have also added a new UI feature to manually set account depth on account import. For users who were previously affected by an issue where wallets with high lookahead values could not rescan properly, this provides a way to bypass the issue.
This should allow Bob to go back to using official hsd and resolve a known issue re: rescan loop!
TODO: