search

decred / dcrwallet

A secure Decred wallet daemon written in Go (golang).
https://decred.org
ISC License
211 stars 153 forks source link

wallet: Add birthday. #2347

Closed JoeGruffins closed 1 month ago

JoeGruffins commented 2 months ago

closes #2346 closes #320

JoeGruffins commented 2 months ago

I'm not sure if I need to do anything for the rescans. They will start from zero by default but you can set a block header or height. Should the birthday info be included in walletinfo or a new rpc call to help there?

matheusd commented 2 months ago

I'm not sure if I need to do anything for the rescans. They will start from zero by default but you can set a block header or height. Should the birthday info be included in walletinfo or a new rpc call to help there?

Yeah, I'm pretty sure you want both account/address discovery and rescans to start from the birthday (when it is set) for performance reasons, unless a specific block height/hash has been specified on a particular rescan call.

JoeGruffins commented 2 months ago

Yeah, I'm pretty sure you want both account/address discovery and rescans to start from the birthday (when it is set) for performance reasons, unless a specific block height/hash has been specified on a particular rescan call.

Hard to do this without changing a lot I think. Default height is zero so there's not a way to differentiate between default height and if a user really wants to rescan from zero. They may want to rescan before the birthday for example if some data was imported into the wallet.

I guess we can have -1 be a special case? Would that be ok?

JoeGruffins commented 2 months ago

https://github.com/decred/dcrwallet/compare/3da2ec7844f042ca78d71eee63219cbc7c08ba03..1486597870ae26b392ded4fd46648f96e8c3db10

@matheusd Just one birthstate now. Added the 24 hour buffer. It is missing a lot of comments.

Still unsure:

  1. Is this better in ChainSwitch or done inside sync (chain/spv) during the initial sync?
  2. Do we need to change up the rescan rpc/grpc requests to allow a "default" alongside allowing from zero? imo giving the user a way to see the birth block is enough, then one can rescan from the birth block after they know it.
JoeGruffins commented 2 months ago

I guess I will try adding the birthblock to walletinfo? imo that is enough, but we can do more... now or later.

JoeGruffins commented 2 months ago

~I haven't had a chance to test with the last changes so converted to draft temporarily.~

Added allowing restoring from block to the prompt. Moved the birthday verification there. Added methods for rpc and grpc to see the birthday https://github.com/decred/dcrwallet/compare/1486597870ae26b392ded4fd46648f96e8c3db10..9449b2c2fe8fb3ce846b87f891209f1b0a8dc7c5

matheusd commented 2 months ago

Is this ready for testing or are you planning on adding any other features to this? Asking so I can start functional testing on it...

JoeGruffins commented 2 months ago

Is this ready for testing or are you planning on adding any other features to this? Asking so I can start functional testing on it...

Yes it is ready. I don't plan to add anything unless requested.

JoeGruffins commented 2 months ago

I could add a unit test for the db functions SetBirthState and BirthState. I have one half made.

JoeGruffins commented 2 months ago

Just to double check, but additional rescanwallet with no arguments will NOT rely on the birthday to perform the rescan, right? It will be up to the users to call walletinfo and then use the birthblock as argument to ensure the rescan only goes through blocks after the birthblock.

Yes that keeps this pr simple. I think this work can still be done though if necessary. As I said I think this will require changes to both the grpc and rpc api to distinguish between a rescan from the height 0 and from birthday.

JoeGruffins commented 2 months ago

Just rebased.

JoeGruffins commented 2 months ago

Added a db upgrade https://github.com/decred/dcrwallet/compare/475cabd342ed69e40b9b31831a187837c24358f3..70da522eb5cb5163e44d3091a217c0bde51f9804

I changed that one spot for spv wallets that you showed and looking if there are others... Are there other spots that need changing in chainntfs.go?

matheusd commented 2 months ago

This is looking pretty good:

Time to discover accounts and addresses (mainnet, empty wallet)
Without birthday: 1m38s
   With birthday: 315ms

One more thing I remembered is adding the birthday ({time,height}) to the CreateWallet gRPC endpoint. This will then allow me to pass this info from dcrlnd when creating new wallets from there.

JoeGruffins commented 2 months ago

Add time and height params to the createwallet grpc request https://github.com/decred/dcrwallet/compare/70da522eb5cb5163e44d3091a217c0bde51f9804..47e49d626db7d68ed51d51fc409e04fa5bd66116

Also change the main if block to exclude block zero although it seems to always start from block one anyhow

JoeGruffins commented 1 month ago

Using time.Since https://github.com/decred/dcrwallet/compare/df07037476bc1978502d9d67857b8d2feb632beb..2d9989e86ae7c4c52d11ef3f3ee15c9feb88a846

JoeGruffins commented 1 month ago

Avoid possible panic https://github.com/decred/dcrwallet/compare/2d9989e86ae7c4c52d11ef3f3ee15c9feb88a846..32e15c4cd02a3b89545cae29a29904cc69aab177