sync: Add rescan progress. #4
Conversation
| w.mainWallet = nil | ||
| w.db = nil |
There was a problem hiding this comment.
Not related to the problem but I don't understand why this needs to be done and I'm concerned it could cause panics.
JoeGruffins
force-pushed
the
rescanprogress
branch
from
f8f2969
to
a28d2cf
Compare
| if w.rescanning { | ||
| w.syncStatusMtx.Unlock() | ||
| return | ||
| } |
There was a problem hiding this comment.
It looks like these are being called during the rescan, which is strange. This may be why the status doesnt say synced without the bandaid.
There was a problem hiding this comment.
We probably don't want to allow rescanning when sync is on. If we add that check in the rescan method, then we may not need to bother about this. w.Sync should ideally stop calling these callbacks once the wallet is synced. But if it doesn't do so currently, then we'd need these if statements to play safe.
There was a problem hiding this comment.
We probably don't want to allow rescanning when sync is on.
Do you mean stop sync then restart it? Is that what other wallets do?
There was a problem hiding this comment.
No, reject rescan requests if sync is ongoing. Rescan works best if sync is completed and the block filters are downloaded.
There was a problem hiding this comment.
Oh sorry missed this. Now checking if synced before rescan https://github.com/decred/libwallet/compare/f2eca88e6d25a98ab03ee087bf65b69bed3fe73c..1f2c9ca6c4eb07607509400459b8e1164e6fe83b
Not sure why I was holding the wallet lock. Looks ok without the lock?
JoeGruffins
force-pushed
the
rescanprogress
branch
from
f8f2969
to
a28d2cf
Compare
| if w.rescanning { | ||
| w.syncStatusMtx.Unlock() | ||
| return | ||
| } |
There was a problem hiding this comment.
We probably don't want to allow rescanning when sync is on. If we add that check in the rescan method, then we may not need to bother about this. w.Sync should ideally stop calling these callbacks once the wallet is synced. But if it doesn't do so currently, then we'd need these if statements to play safe.
cgo/walletloader.go
Outdated
| if err := w.CloseWallet(); err != nil { | ||
| return errCResponse("close wallet %q error: %v", name, err.Error()) | ||
| } | ||
| close(w.shutdown) | ||
| w.Wait() |
There was a problem hiding this comment.
If rescan is in progress when w.CloseWallet() is called, I'm not sure the rescan will be stopped before closing the wallet db? For sync for example, we call w.StopSync() and w.WaitForSyncToStop() in w.CloseWallet before closing the wallet db.
Might be a good idea to do something similar for rescans. One approach would be to create a cancellable context in w.RescanProgressFromHeight and cancel the context when w.CloseWallet() is called. Would also need a way to ensure that the rescan has actually stopped before closing the wallet db.
There was a problem hiding this comment.
I thought closing the wallet db would just cause functions trying to use it to error?
so, close w.shutdown and wait before CloseWallet?
There was a problem hiding this comment.
I thought closing the wallet db would just cause functions trying to use it to error?
Eh. No idea. I suppose it could result in a panic but I absolutely don't know. Can leave as is, if that's okay by you.
There was a problem hiding this comment.
Closing w.shutdown before CloseWallet
JoeGruffins
force-pushed
the
rescanprogress
branch
from
a28d2cf
to
7419307
Compare
|
I'm pretty sure decrediton and probably cryptopower allow rescanning while sync is active. But maybe not. Anyway I think this solves the other comments https://github.com/decred/libwallet/compare/a28d2cf76b1853ea81ca31b04847e786b512d1bf..7419307d7d5185ec785928157b296390dbde47d5 |
|
Ohh, are notifications only for initial sync? hmmm. I guess thats obvious but wasn't to me. So we don't need them at all after initial sync? |
|
We do want the peer notifications still. |
|
We can get the peers from the syncer. I can't think of a way to change this to actually lower allocations and what not at the moment. Maybe getting some of the notifications when we are not in initial sync is a wallet bug even. I think the rescanning checks in the notifications are enough for now. |
cgo/walletloader.go
Outdated
| @@ -217,6 +223,8 @@ func closeWallet(cName *C.char) *C.char { | |||
| if !exists { | |||
| return errCResponse("wallet with name %q does not exist", name) | |||
| } | |||
| close(w.shutdown) | |||
There was a problem hiding this comment.
I'm still looking at this. This signals that wallet shutdown has been requested, but I wonder, why not just cancel the ctx used by most background activities including rescan? The only code that acts when this channel is closed is the rescan goroutine, and that also listens for the ctx cancellation and returns as well. An advantage to using just ctx cancellation would be that the rescan process itself would receive a trigger to abort.
Since ctx is currently used by multiple wallets, we can make each wallet have a ctx that is created from the general ctx.
There was a problem hiding this comment.
shutdown chan -> context.Context?
JoeGruffins
force-pushed
the
rescanprogress
branch
from
7419307
to
f0a2b59
Compare
|
Replace shutdown channel with a context and use the new wallet context for all functions https://github.com/decred/libwallet/compare/7419307d7d5185ec785928157b296390dbde47d5..f0a2b5976d08147998460d85c76ff72ead79f0f7 |
JoeGruffins
force-pushed
the
rescanprogress
branch
from
f0a2b59
to
f2eca88
Compare
JoeGruffins
force-pushed
the
rescanprogress
branch
from
f2eca88
to
1f2c9ca
Compare
No description provided.