-
Notifications
You must be signed in to change notification settings - Fork 278
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: fix missing tx when rescan with filter update #519
Conversation
Are you sure we need all this work on the lookahead value? Unfortunately my open source time for hsd is very limited for now. If you or someone at Kyokan has the bandwidth to try this suggestion I think it'll be much cleaner and easier to get community approval. |
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.
It would be nice to have some tests and also squash the commits so that they follow the conventions of the codebase
lib/wallet/walletdb.js
Outdated
'migrated lookahead (wid=%d, name=%s)', | ||
wid, name); | ||
} catch (e) { | ||
// |
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.
Skipping this on purpose?
I see what you mean now - after some digging
|
b403e77
to
0102551
Compare
So after more digging, I can confirm that there is at least one user wallet with a lookahead value of 500, testing using the rescan filter fix in this PR + hardcoding lookahead value. @pinheadmz This issue from you helped lot! I will open a separate PR for lookahead value migration and see if there are any community support for it. For Bob, I will just maintain a simple fork with a hardcode lookahead value of @tynes squashed and added unit test! However, I am having trouble getting them to run locally. The tests seems to be stucked when it tries to generate blocks. Is that a common issue when running test? |
0102551
to
58bbb27
Compare
rsPort: ports.rs, | ||
nsPort: ports.ns, | ||
env: { | ||
'BCOIN_WALLET_HTTP_PORT': ports.wallet.toString() |
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.
This might need to be changed to HSD_WALLET_HTTP_PORT
:-)
|
||
// Mature the initial coins to use for the | ||
// use in generating the test case. | ||
for (let i = 0; i < 200; i++) { |
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.
I think in hsd the coinbase maturity for regtest is much smaller so we can reduce this here.
Line 745 in c85d9b4
regtest.coinbaseMaturity = 2; |
note, Braydon doesn't like to be tagged in Handshake-org stuff but this is mostly his work, I'm not sure if we need to be explicit about the license or anything but you might consider just adding his name to the commit message. |
@chikeichan finally getting to more deeply review this. I'm playing with the test locally, converting it more hsd-friendly parameters etc. Looks like the main patch isn't just Braydon's commit but some extra work as well, including calling |
hello @pinheadmz - nice to see you active again! For this PR, the general changes to scan is:
I do not have test for this changes unfortunately; I have been using my personal wallet with a lookahead value of around 500 to test during development. I am more than happy to review new branch + merge back to Bob when it's ready! |
CHANGE:
1
to change lookahead value fromuint8
touint32
changeDepth
orreceiveDepth
exceed current lookahead value during rescan, increment lookahead value by 20, and then send updated filter and block height to tonode/http.js
node/http
will halt the current rescan and start a new one with the new filter from the new heightSummary
This borrow heavily from this issue in bcoin but with a simplified approach in
node/http
.This version of
hsd
and rescan functionality can be tested in this branch in Bob. I have tested this with a wallet with 5k+ transactions and lookahead value at 3k+ and have no issue.