hsd icon indicating copy to clipboard operation
hsd copied to clipboard

wallet: fix missing tx when rescan with filter update

Open chikeichan opened this issue 5 years ago • 6 comments

CHANGE:

  • add migration-id 1 to change lookahead value from uint8 to uint32
  • when either changeDepth or receiveDepth exceed current lookahead value during rescan, increment lookahead value by 20, and then send updated filter and block height to to node/http.js
  • when filter is updated during rescan, node/http will 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.

chikeichan avatar Oct 14 '20 04:10 chikeichan

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.

pinheadmz avatar Oct 14 '20 14:10 pinheadmz

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.~

chikeichan avatar Oct 14 '20 23:10 chikeichan

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?

chikeichan avatar Oct 17 '20 05:10 chikeichan

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.

pinheadmz avatar Oct 26 '20 04:10 pinheadmz

@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.

pinheadmz avatar Jan 04 '21 20:01 pinheadmz

hello @pinheadmz - nice to see you active again!

For this PR, the general changes to scan is:

  • wallet client will call setFilter with current height
  • wallet node receive set filter in 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!

chikeichan avatar Jan 04 '21 23:01 chikeichan