hsd
hsd copied to clipboard
wallet: fix missing tx when rescan with filter update
CHANGE:
- add migration-id
1to change lookahead value fromuint8touint32 - when either
changeDepthorreceiveDepthexceed current lookahead value during rescan, increment lookahead value by 20, and then send updated filter and block height to tonode/http.js - when filter is updated during rescan,
node/httpwill halt the current rescan and start a new one with the new filter from the new height
Summary
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.
Are you sure we need all this work on the lookahead value? I was hoping that if we merge https://github.com/handshake-org/hsd/pull/506 (bump lookahead to 200, still within the U8 value) and the filter update commit from bcoin we'd basically be done with this. There is a test as well in this other commit: https://github.com/bcoin-org/bcoin/commit/46967f8da604412051d0eff223c45ff6ecc15e3a
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.
Are you sure we need all this work on the lookahead value? I was hoping that if we merge #506 (bump lookahead to 200, still within the U8 value) and the filter update commit from bcoin we'd basically be done with this. There is a test as well in this other commit: bcoin-org/bcoin@46967f8
I see what you mean now - after some digging ~i believe there is a bug in the syncDepth method where some new account keys are not being saved. To workaround,~ I basically was just calling setLookahead with account depth to force account keys to be generated. This has since been modified to match closer to the bcoin commit.
~Let me try to fix the bug in syncDepth instead -- that should allow us to leave lookahead value unchanged.~
Are you sure we need all this work on the lookahead value?
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 1000 for now so that lookahead datatype stays the same.
@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?
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 rollback() -- were you able to solve the issue here? Can you provide a test script with bash or anything to evaluate the changes? This issue is going to be my next top priority, I may start a new branch with my modified test and Braydon's bcoin commit and then add back in some of your changes.
hello @pinheadmz - nice to see you active again!
For this PR, the general changes to scan is:
- wallet client will call
setFilterwith current height - wallet node receive
set filterin the socket event - wallet node stop current scan progress (kinda hacky now by throwing an error)
- wallet db rollback to current height minus 1
- wallet node restart new scan from new filter height minus 1
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!