The current rescan functionality of SD was inherited from ZecWallet and ZEC, which did not have a rescan RPC. Now that Hush has a rescan RPC, we don't need to restart the node and write rescan=1 to the config file, then remove it.
The code to do a full rescan should use the rescan RPC with optional height parameter.
The current rescan functionality of SD was inherited from ZecWallet and ZEC, which did not have a rescan RPC. Now that Hush has a rescan RPC, we don't need to restart the node and write rescan=1 to the config file, then remove it.
The code to do a full rescan should use the rescan RPC with optional height parameter.
I am looking at this now. Planning to replace the rescan checkbox under the current troubleshooting settings with a button that opens dialog for block height. Let me know if you were thinking something different.
I am looking at this now. Planning to replace the rescan checkbox under the current troubleshooting settings with a button that opens dialog for block height. Let me know if you were thinking something different.
I have some UI for this but when I tried calling rescan RPC it gave this error: JSON value is not an integer as expected
This is what it logs for request: RPC: "rescan" QJsonValue(object, QJsonObject({"id":"42","jsonrpc":"1.0","method":"rescan","params":["1109400"]}))
I am passing an integer that's 1109400 in this example. I had to add another makePayload that accepts integer instead of string so it's not wrapped in quotes.
It seems to work and starts scanning at submitted blockheight but I need to look into how to update the progress while it's rescanning. You can see it rescanning in log and restarting SD will keep splash screen open with no indication of rescanning like #87 Taking break for now.
I have some UI for this but when I tried calling `rescan` RPC it gave this error:
`JSON value is not an integer as expected`
This is what it logs for request:
`RPC: "rescan" QJsonValue(object, QJsonObject({"id":"42","jsonrpc":"1.0","method":"rescan","params":["1109400"]}))`
I am passing an integer that's 1109400 in this example. I had to add another `makePayload` that accepts integer instead of string so it's not wrapped in quotes.
It seems to work and starts scanning at submitted blockheight but I need to look into how to update the progress while it's rescanning. You can see it rescanning in log and restarting SD will keep splash screen open with no indication of rescanning like https://git.hush.is/hush/SilentDragon/issues/87 Taking break for now.
@fekt just to be sure, are you using hushd 3.9.2 in this example? Older hushd's have a bug where rescanning from a height doesn't work. You also sometimes need to deal with issues where something is a string in JSON but hush-cli wants integers.
As for keeping progress updated, the rescan RPC gives no progress updates, the only way to tell is for us to look at debug.log and extract the info from there.
@fekt just to be sure, are you using hushd 3.9.2 in this example? Older hushd's have a bug where rescanning from a height doesn't work. You also sometimes need to deal with issues where something is a string in JSON but hush-cli wants integers.
As for keeping progress updated, the `rescan` RPC gives no progress updates, the only way to tell is for us to look at debug.log and extract the info from there.
@fekt just to be sure, are you using hushd 3.9.2 in this example?
Yes, 3.9.2. hush-cli seems to complain about the height being wrapped in quotes from payload. Adding payload that accepts integer and doesn't wrap in quotes works but let me know if doing it wrong. Latest commit has changes for this.
I am looking into QFileSystemWatcher or something else to read debug.log when it changes. Rescan appears to work and I'll see something like this in debug.log:
2022-10-09 15:01:06 Rescanning from height=1110000
2022-10-09 15:01:09 Done rescanning from height=1110000
Specifying an older height does the same, but takes longer and will have something like: 2022-10-09 15:04:15 Still rescanning. At block 802930. Progress=0.589521
So need something that reads the file and sees rescanning starting and keeps checking and displaying progress until it sees it's done.
> @fekt just to be sure, are you using hushd 3.9.2 in this example?
Yes, 3.9.2. hush-cli seems to complain about the height being wrapped in quotes from payload. Adding payload that accepts integer and doesn't wrap in quotes works but let me know if doing it wrong. Latest commit has changes for this.
I am looking into QFileSystemWatcher or something else to read debug.log when it changes. Rescan appears to work and I'll see something like this in debug.log:
```
2022-10-09 15:01:06 Rescanning from height=1110000
2022-10-09 15:01:09 Done rescanning from height=1110000
```
Specifying an older height does the same, but takes longer and will have something like:
`2022-10-09 15:04:15 Still rescanning. At block 802930. Progress=0.589521`
So need something that reads the file and sees rescanning starting and keeps checking and displaying progress until it sees it's done.
@fekt yes, making a new makePayload() variant sounds like the right way to do it.
And yes, the best/only way to do what we want is I think to read lines of debug.log that match a regex (QT has them) and then extract the "at block XXX, progress=yyy" data and render that to the user
@fekt yes, making a new makePayload() variant sounds like the right way to do it.
And yes, the best/only way to do what we want is I think to read lines of debug.log that match a regex (QT has them) and then extract the "at block XXX, progress=yyy" data and render that to the user
I am going to try and work on this more late tonight or over the weekend. Do you think it makes most sense to display splash screen that can't be closed for this showing progress or where would you want progress to show? Another option is status bar in lower left, but if wallet is doing anything else, the status may change to something else. If still rescanning it'll probably show again on next progress check though.
I was seeing some different behavior on testing rescans too and not sure if reproducible. If you restart the wallet or maybe kill hushd, it might force a rescan from height 0. Possibly weird stuff with deletetx and zsweep or other RPCs still running too. After one rescan supposedly finished, I was only seeing some weird txs for mining. They were showing an address I hadn't used for some time but saying the rewards were for yesterday. Another rescan from a height before then did fix it and I disabled deletetx before running it again.
On the splash screen when restarting before a rescan finished, I'd sometimes see nothing like #87 while other times it would say "Rescanning". I need to look into what determines if the text is set or not when rescanning. I know there is a check to display "popcorn" message or not.
I am going to try and work on this more late tonight or over the weekend. Do you think it makes most sense to display splash screen that can't be closed for this showing progress or where would you want progress to show? Another option is status bar in lower left, but if wallet is doing anything else, the status may change to something else. If still rescanning it'll probably show again on next progress check though.
I was seeing some different behavior on testing rescans too and not sure if reproducible. If you restart the wallet or maybe kill hushd, it might force a rescan from height 0. Possibly weird stuff with deletetx and zsweep or other RPCs still running too. After one rescan supposedly finished, I was only seeing some weird txs for mining. They were showing an address I hadn't used for some time but saying the rewards were for yesterday. Another rescan from a height before then did fix it and I disabled deletetx before running it again.
On the splash screen when restarting before a rescan finished, I'd sometimes see nothing like #87 while other times it would say "Rescanning". I need to look into what determines if the text is set or not when rescanning. I know there is a check to display "popcorn" message or not.
When starting a rescan, give a cancelable modal that tells users "read only mode until rescan completes" or something like that
Pause all timers/clocks/etc to update balances/txs/etc
Let users view their current data, don't update anything until rescan is complete
Update status bar text with rescan %
Prevent new transactions from being created, tell users to wait until rescan finishes if they try to
When rescan completes, give user cancelable modal that tells them "Rescan complete! read-only mode is over" and that they can make transactions again
Restart any paused timers/clocks/polling functions
@fekt really good question. Here is my idea:
1) When starting a rescan, give a cancelable modal that tells users "read only mode until rescan completes" or something like that
2) Pause all timers/clocks/etc to update balances/txs/etc
3) Let users view their current data, don't update anything until rescan is complete
4) Update status bar text with rescan %
5) Prevent new transactions from being created, tell users to wait until rescan finishes if they try to
6) When rescan completes, give user cancelable modal that tells them "Rescan complete! read-only mode is over" and that they can make transactions again
7) Restart any paused timers/clocks/polling functions
@fekt as for the bugs with killing hushd, they are somewhat expected. C++ destructors and cleanup code are not executed when you kill a process with kill -9 or hit a coredump or similar. So if an async zaddr operation was running (like z_sweep or z_sendmany or z_migratetoaddress), it either marked a zutxo as spent when it isn't, or vice versa. This is because the process might have exited before writing data to wallet.dat and then gets out of sync with the network. Rescanning from a height before the loss of syncedness fixes the problem. As for deletetx, it does a lot of wallet.dat writing, so it has a similar issue
@fekt as for the bugs with killing `hushd`, they are somewhat expected. C++ destructors and cleanup code are not executed when you kill a process with `kill -9` or hit a coredump or similar. So if an async zaddr operation was running (like z_sweep or z_sendmany or z_migratetoaddress), it either marked a zutxo as spent when it isn't, or vice versa. This is because the process might have exited before writing data to wallet.dat and then gets out of sync with the network. Rescanning from a height before the loss of syncedness fixes the problem. As for deletetx, it does a lot of wallet.dat writing, so it has a similar issue
@fekt just to explain a bit more about why I suggest stopping all the polling functions during a rescan: Just about every RPC will "hang" and wait for a rescan to complete before returning data, and will almost always timeout. So running RPC's during a rescan just wastes resources and could lead to rare bugs if too many RPC's queue up waiting on each other. We had issues with that a long time ago which is why we increased our RPC queue max depth. It's much better to just not run them at all during a rescan and it's more performant for users.
@fekt just to explain a bit more about why I suggest stopping all the polling functions during a rescan: Just about every RPC will "hang" and wait for a rescan to complete before returning data, and will almost always timeout. So running RPC's during a rescan just wastes resources and could lead to rare bugs if too many RPC's queue up waiting on each other. We had issues with that a long time ago which is why we increased our RPC queue max depth. It's much better to just not run them at all during a rescan and it's more performant for users.
@fekt one more thing to mention is that the rescan RPC was fixed in the 3.6.1 release. I found this out by running git log -S rescan -p src/rpc/ and finding the commit id of it being fixed, which is cff8d114eaeab5ab0ae97ef1eae912c7b8bcbb9e and then doing git describe cff8d114eaeab5ab0ae97ef1eae912c7b8bcbb9e which returns v3.6.0-380-gcff8d114e which means this commit is 380 commits past git tag v3.8.0 in the git commit starting with cff8d. The letter g is not a valid hex character, and denotes that what is after it is a shortened commit id.
I think it's reasonable to not support older hushd's that do not support the fixed rescan RPC (which are 2+ years old), but also it's pretty simple to see that rescan HEIGHT returns an error immediately and then use the old way of doing things. I am fine with with either way. If we choose not to support hushd 3.6.0 and older, we should remove all the SD code that does rescan the old way and document in release notes that SD requires hushd 3.6.1 or higher.
@fekt one more thing to mention is that the `rescan` RPC was fixed in the 3.6.1 release. I found this out by running `git log -S rescan -p src/rpc/` and finding the commit id of it being fixed, which is cff8d114eaeab5ab0ae97ef1eae912c7b8bcbb9e and then doing `git describe cff8d114eaeab5ab0ae97ef1eae912c7b8bcbb9e` which returns `v3.6.0-380-gcff8d114e` which means `this commit is 380 commits past git tag v3.8.0 in the git commit starting with cff8d`. The letter `g` is not a valid hex character, and denotes that what is after it is a shortened commit id.
I think it's reasonable to not support older hushd's that do not support the fixed `rescan` RPC (which are 2+ years old), but also it's pretty simple to see that `rescan HEIGHT` returns an error immediately and then use the old way of doing things. I am fine with with either way. If we choose not to support hushd 3.6.0 and older, we should remove all the SD code that does `rescan` the old way and document in release notes that SD requires hushd 3.6.1 or higher.
@duke I like your idea and stopping all other functions from polling makes sense. I'm just not sure how easy it is to implement a read only mode without potentially breaking things or causing unexpected behavior since I don't know all the code.
I have code for reading debug.log and showing percent status and height in splash screen. It is more for fixing #87 and showing rescan progress on a restart but can be adapted. I have been testing by submitting height to rescan at and then restarting SD. It works, but it's probably not the best implementation for processing the file and has some issues.
Related to other things polling, it causes issues with reading debug.log to grep rescan data due to those functions writing to the log. On an SD restart that is still rescanning, there is something that keeps trying to connect while rescanning and keeps logging connection errors. On another system, if I start the rescan in SD, then close SD and never restart, hushd will still run while rescanning, but only log rescan data lines.
I am trying to read as little of the log as possible to grab next rescan data. The issue with this is if I don't look at enough data, what I'm looking at may contain nothing related to rescanning. If I look at too much data, then it's going to potentially match an older line in log and not necessarily show the latest progress until log is written to more. Along with this, it is hard to grep when rescanning is done without reading a lot of the file due to a bunch of calls that happen right after finishing that write to the log. If I don't read enough of the file, I miss it entirely.
I am doing this in a timer currently but may need to change things around to use QFileSystemWatcher and only read when the file changes. It looked like rescan data is written every 1 minute. It might work better if I stop whatever keeps trying to connect so only rescan progress is being logged.
@duke I like your idea and stopping all other functions from polling makes sense. I'm just not sure how easy it is to implement a read only mode without potentially breaking things or causing unexpected behavior since I don't know all the code.
I have code for reading debug.log and showing percent status and height in splash screen. It is more for fixing #87 and showing rescan progress on a restart but can be adapted. I have been testing by submitting height to rescan at and then restarting SD. It works, but it's probably not the best implementation for processing the file and has some issues.
Related to other things polling, it causes issues with reading debug.log to grep rescan data due to those functions writing to the log. On an SD restart that is still rescanning, there is something that keeps trying to connect while rescanning and keeps logging connection errors. On another system, if I start the rescan in SD, then close SD and never restart, hushd will still run while rescanning, but only log rescan data lines.
I am trying to read as little of the log as possible to grab next rescan data. The issue with this is if I don't look at enough data, what I'm looking at may contain nothing related to rescanning. If I look at too much data, then it's going to potentially match an older line in log and not necessarily show the latest progress until log is written to more. Along with this, it is hard to grep when rescanning is done without reading a lot of the file due to a bunch of calls that happen right after finishing that write to the log. If I don't read enough of the file, I miss it entirely.
I am doing this in a timer currently but may need to change things around to use QFileSystemWatcher and only read when the file changes. It looked like rescan data is written every 1 minute. It might work better if I stop whatever keeps trying to connect so only rescan progress is being logged.
@fekt I was wrong about when rescan was fixed, it didn't work correctly until 3.9.1, so we should keep the old way around of doing a rescan with restart. We don't need to check versions, we can simply do the old way of doing things if rescan HEIGHT returns an error.
@fekt I was wrong about when `rescan` was fixed, it didn't work correctly until 3.9.1, so we should keep the old way around of doing a rescan with restart. We don't need to check versions, we can simply do the old way of doing things if `rescan HEIGHT` returns an error.
@fekt what if I can write an RPC that would return current rescan height, during a rescan? Would that avoid a huge amount of complication? Seems like it might.
If we go that route, we might need to make SD use a binary from the dev branch of hushd, but since master and dev are basically the same in hush3.git, that seems OK
@fekt what if I can write an RPC that would return current rescan height, during a rescan? Would that avoid a huge amount of complication? Seems like it might.
If we go that route, we might need to make SD use a binary from the `dev` branch of hushd, but since `master` and `dev` are basically the same in hush3.git, that seems OK
@fekt according to https://developer.bitcoin.org/reference/rpc/rescanblockchain.html the rescan progress is part of `getwalletinfo` output, we could do the same and avoid another RPC, since we need to call `getwalletinfo` already
"scanning" : {
"duration" : n, (numeric) elapsed seconds since scan start
"progress" : n (numeric) scanning progress percentage [0.0, 1.0]
}
I'll try to get my code working with this instead. Won't have current block height it's at and only progress percentage.
> @fekt according to https://developer.bitcoin.org/reference/rpc/rescanblockchain.html the rescan progress is part of `getwalletinfo` output
Nice! That would be better than reading debug.log and probably where it's getting that data to begin with. I saw this code but didn't look into where "rescan" gets passed to it. https://git.hush.is/hush/SilentDragon/src/branch/dev/src/connection.cpp#L528
`getwalletinfo` doc says it returns like this
```
"scanning" : {
"duration" : n, (numeric) elapsed seconds since scan start
"progress" : n (numeric) scanning progress percentage [0.0, 1.0]
}
```
I'll try to get my code working with this instead. Won't have current block height it's at and only progress percentage.
@fekt yeah, this stuff was added in BTC Core 0.17.x which is after our internals forked, so we don't have the stuff in those docs, but it shows it's possible.
I believe BTC Core made some changes that allow some RPCs to return data even during a rescan, I am looking into it.
@fekt yeah, this stuff was added in BTC Core 0.17.x which is after our internals forked, so we don't have the stuff in those docs, but it shows it's possible.
I believe BTC Core made some changes that allow some RPCs to return data even during a rescan, I am looking into it.
Latest commit to dev branch has this implemented. For the time being, it will update in bottom left status bar only when using rescan option under settings. Need to figure out how to handle on startup or when restarting.
I also need to test on a large wallet to see what happens with other timers and look into pausing them for performance while rescanning or see if they create issues.
The status will update every 1 second but can be changed to not be as frequent. I had to set it fast for testing to see progress because rescans are too quick on a small wallet with not a lot of txs.
Latest commit to dev branch has this implemented. For the time being, it will update in bottom left status bar only when using rescan option under settings. Need to figure out how to handle on startup or when restarting.
I also need to test on a large wallet to see what happens with other timers and look into pausing them for performance while rescanning or see if they create issues.
The status will update every 1 second but can be changed to not be as frequent. I had to set it fast for testing to see progress because rescans are too quick on a small wallet with not a lot of txs.
@duke I tested on a large wallet and it's a bit janky. Seems like starting the rescan never even happens possibly due to other RPCs/timers running and causing it to timeout or something. I tried multiple times where it never showed it starting in log and transaction data kept trying to load at same time. The times it did start, it seemed like getrescaninfo was slow in being called or responding too. Probably need to look into and implement pausing everything else when starting rescan. On small wallets it seems to work as intended.
@duke I tested on a large wallet and it's a bit janky. Seems like starting the rescan never even happens possibly due to other RPCs/timers running and causing it to timeout or something. I tried multiple times where it never showed it starting in log and transaction data kept trying to load at same time. The times it did start, it seemed like getrescaninfo was slow in being called or responding too. Probably need to look into and implement pausing everything else when starting rescan. On small wallets it seems to work as intended.
@fekt I think we should put your code onto a rescan branch and yes, we probably want to pause other timers for it to work well with large wallets. This is a big enough change that we should get testing from a few different people on different wallets to make sure it works correctly.
Thanks for working on this, I think it will end up being a huge improvement, once we shake out some bugs.
@fekt I think we should put your code onto a `rescan` branch and yes, we probably want to pause other timers for it to work well with large wallets. This is a big enough change that we should get testing from a few different people on different wallets to make sure it works correctly.
Thanks for working on this, I think it will end up being a huge improvement, once we shake out some bugs.
@duke I can create a rescan branch that's a copy of current dev and then revert rescan option back to original behavior if that's what you want. Let me know. I may have some time tonight or should be free tomorrow.
@duke I can create a rescan branch that's a copy of current dev and then revert rescan option back to original behavior if that's what you want. Let me know. I may have some time tonight or should be free tomorrow.
@fekt I am in the process of reviewing the code and testing. I think it's ok to leave it on the dev branch right now, since it makes it easier for others to test it together with all the other recent SD changes. If we want to make an SD release and the code isn't ready for "prime time" yet, then we can put it on it's own branch and work on it for the next release.
@jahway603@onryo do you have time to help test these rescan changes?
@fekt I am in the process of reviewing the code and testing. I think it's ok to leave it on the `dev` branch right now, since it makes it easier for others to test it together with all the other recent SD changes. If we want to make an SD release and the code isn't ready for "prime time" yet, then we can put it on it's own branch and work on it for the next release.
@jahway603 @onryo do you have time to help test these rescan changes?
@fekt with my latest commits to dev, I think this works "good enough". Users can rescan from an arbitrary height without a restart. I did not implement pausing other timers because I was seeing issues with that, at least with my large wallet I used for testing. I am still not seeing the statusbar update correctly but I am thinking that is an issue with my large wallet. I want to do some testing with a small wallet before closing this.
@fekt with my latest commits to `dev`, I think this works "good enough". Users can rescan from an arbitrary height without a restart. I did not implement pausing other timers because I was seeing issues with that, at least with my large wallet I used for testing. I am still not seeing the statusbar update correctly but I am thinking that is an issue with my large wallet. I want to do some testing with a small wallet before closing this.
@duke Seems to work fine on my small Mac wallet. I tested pausing other timers previously and didn't seem to help on large wallets. I can test latest changes on a larger wallet. The height initialization should at least fix the issue of it sometimes showing 100% at first.
@duke Seems to work fine on my small Mac wallet. I tested pausing other timers previously and didn't seem to help on large wallets. I can test latest changes on a larger wallet. The height initialization should at least fix the issue of it sometimes showing 100% at first.
We might want to fiddle with that stuff for a future release, but it seems to be a bigger can of worms than we want to open for our next SD release.
@fekt the problem with large wallets seems to be that we don't have enough QThread's doing different things, so some stuff blocks the UI updating. https://doc.qt.io/qt-5/qthread.html This is related to how we inherit from SingleApplication : https://git.hush.is/hush/SilentDragon/src/branch/master/singleapplication
We might want to fiddle with that stuff for a future release, but it seems to be a bigger can of worms than we want to open for our next SD release.
Yeah, large wallet still funky with updating status and a possible new issue. I tried a rescan a couple of times and getrescaninfo shows it's 100% complete but keeps saying rescanning is true for awhile. The UI status in bottom right seems to stop updating and stays on block height from when rescan started unless right clicking to refresh. I didn't check code but maybe just from stopping other timers if you did that. Once I refresh once, it continues to update regularly.
Yeah, large wallet still funky with updating status and a possible new issue. I tried a rescan a couple of times and getrescaninfo shows it's 100% complete but keeps saying rescanning is true for awhile. The UI status in bottom right seems to stop updating and stays on block height from when rescan started unless right clicking to refresh. I didn't check code but maybe just from stopping other timers if you did that. Once I refresh once, it continues to update regularly.
@fekt not sure which code you are using, but my latest dev branch code does not pause the main UI timer. Maybe it should, not sure. I haven't tried right clicking the statusbar and refreshing, I should try that.
@fekt not sure which code you are using, but my latest dev branch code does not pause the main UI timer. Maybe it should, not sure. I haven't tried right clicking the statusbar and refreshing, I should try that.
duke
The current rescan functionality of SD was inherited from ZecWallet and ZEC, which did not have a rescan RPC. Now that Hush has a rescan RPC, we don't need to restart the node and write rescan=1 to the config file, then remove it.
The code to do a full rescan should use the rescan RPC with optional height parameter.
I am looking at this now. Planning to replace the rescan checkbox under the current troubleshooting settings with a button that opens dialog for block height. Let me know if you were thinking something different.
I have some UI for this but when I tried calling
rescanRPC it gave this error:JSON value is not an integer as expectedThis is what it logs for request:
RPC: "rescan" QJsonValue(object, QJsonObject({"id":"42","jsonrpc":"1.0","method":"rescan","params":["1109400"]}))I am passing an integer that's 1109400 in this example. I had to add another
makePayloadthat accepts integer instead of string so it's not wrapped in quotes.It seems to work and starts scanning at submitted blockheight but I need to look into how to update the progress while it's rescanning. You can see it rescanning in log and restarting SD will keep splash screen open with no indication of rescanning like #87 Taking break for now.
@fekt just to be sure, are you using hushd 3.9.2 in this example? Older hushd's have a bug where rescanning from a height doesn't work. You also sometimes need to deal with issues where something is a string in JSON but hush-cli wants integers.
As for keeping progress updated, the
rescanRPC gives no progress updates, the only way to tell is for us to look at debug.log and extract the info from there.Yes, 3.9.2. hush-cli seems to complain about the height being wrapped in quotes from payload. Adding payload that accepts integer and doesn't wrap in quotes works but let me know if doing it wrong. Latest commit has changes for this.
I am looking into QFileSystemWatcher or something else to read debug.log when it changes. Rescan appears to work and I'll see something like this in debug.log:
Specifying an older height does the same, but takes longer and will have something like:
2022-10-09 15:04:15 Still rescanning. At block 802930. Progress=0.589521So need something that reads the file and sees rescanning starting and keeps checking and displaying progress until it sees it's done.
@fekt yes, making a new makePayload() variant sounds like the right way to do it.
And yes, the best/only way to do what we want is I think to read lines of debug.log that match a regex (QT has them) and then extract the "at block XXX, progress=yyy" data and render that to the user
I am going to try and work on this more late tonight or over the weekend. Do you think it makes most sense to display splash screen that can't be closed for this showing progress or where would you want progress to show? Another option is status bar in lower left, but if wallet is doing anything else, the status may change to something else. If still rescanning it'll probably show again on next progress check though.
I was seeing some different behavior on testing rescans too and not sure if reproducible. If you restart the wallet or maybe kill hushd, it might force a rescan from height 0. Possibly weird stuff with deletetx and zsweep or other RPCs still running too. After one rescan supposedly finished, I was only seeing some weird txs for mining. They were showing an address I hadn't used for some time but saying the rewards were for yesterday. Another rescan from a height before then did fix it and I disabled deletetx before running it again.
On the splash screen when restarting before a rescan finished, I'd sometimes see nothing like #87 while other times it would say "Rescanning". I need to look into what determines if the text is set or not when rescanning. I know there is a check to display "popcorn" message or not.
@fekt really good question. Here is my idea:
@fekt as for the bugs with killing
hushd, they are somewhat expected. C++ destructors and cleanup code are not executed when you kill a process withkill -9or hit a coredump or similar. So if an async zaddr operation was running (like z_sweep or z_sendmany or z_migratetoaddress), it either marked a zutxo as spent when it isn't, or vice versa. This is because the process might have exited before writing data to wallet.dat and then gets out of sync with the network. Rescanning from a height before the loss of syncedness fixes the problem. As for deletetx, it does a lot of wallet.dat writing, so it has a similar issue@fekt just to explain a bit more about why I suggest stopping all the polling functions during a rescan: Just about every RPC will "hang" and wait for a rescan to complete before returning data, and will almost always timeout. So running RPC's during a rescan just wastes resources and could lead to rare bugs if too many RPC's queue up waiting on each other. We had issues with that a long time ago which is why we increased our RPC queue max depth. It's much better to just not run them at all during a rescan and it's more performant for users.
@fekt one more thing to mention is that the
rescanRPC was fixed in the 3.6.1 release. I found this out by runninggit log -S rescan -p src/rpc/and finding the commit id of it being fixed, which is cff8d114eaeab5ab0ae97ef1eae912c7b8bcbb9e and then doinggit describe cff8d114eaeab5ab0ae97ef1eae912c7b8bcbb9ewhich returnsv3.6.0-380-gcff8d114ewhich meansthis commit is 380 commits past git tag v3.8.0 in the git commit starting with cff8d. The lettergis not a valid hex character, and denotes that what is after it is a shortened commit id.I think it's reasonable to not support older hushd's that do not support the fixed
rescanRPC (which are 2+ years old), but also it's pretty simple to see thatrescan HEIGHTreturns an error immediately and then use the old way of doing things. I am fine with with either way. If we choose not to support hushd 3.6.0 and older, we should remove all the SD code that doesrescanthe old way and document in release notes that SD requires hushd 3.6.1 or higher.@duke I like your idea and stopping all other functions from polling makes sense. I'm just not sure how easy it is to implement a read only mode without potentially breaking things or causing unexpected behavior since I don't know all the code.
I have code for reading debug.log and showing percent status and height in splash screen. It is more for fixing #87 and showing rescan progress on a restart but can be adapted. I have been testing by submitting height to rescan at and then restarting SD. It works, but it's probably not the best implementation for processing the file and has some issues.
Related to other things polling, it causes issues with reading debug.log to grep rescan data due to those functions writing to the log. On an SD restart that is still rescanning, there is something that keeps trying to connect while rescanning and keeps logging connection errors. On another system, if I start the rescan in SD, then close SD and never restart, hushd will still run while rescanning, but only log rescan data lines.
I am trying to read as little of the log as possible to grab next rescan data. The issue with this is if I don't look at enough data, what I'm looking at may contain nothing related to rescanning. If I look at too much data, then it's going to potentially match an older line in log and not necessarily show the latest progress until log is written to more. Along with this, it is hard to grep when rescanning is done without reading a lot of the file due to a bunch of calls that happen right after finishing that write to the log. If I don't read enough of the file, I miss it entirely.
I am doing this in a timer currently but may need to change things around to use QFileSystemWatcher and only read when the file changes. It looked like rescan data is written every 1 minute. It might work better if I stop whatever keeps trying to connect so only rescan progress is being logged.
@fekt I was wrong about when
rescanwas fixed, it didn't work correctly until 3.9.1, so we should keep the old way around of doing a rescan with restart. We don't need to check versions, we can simply do the old way of doing things ifrescan HEIGHTreturns an error.@fekt what if I can write an RPC that would return current rescan height, during a rescan? Would that avoid a huge amount of complication? Seems like it might.
If we go that route, we might need to make SD use a binary from the
devbranch of hushd, but sincemasteranddevare basically the same in hush3.git, that seems OK@fekt according to https://developer.bitcoin.org/reference/rpc/rescanblockchain.html the rescan progress is part of
getwalletinfooutput, we could do the same and avoid another RPC, since we need to callgetwalletinfoalreadyNice! That would be better than reading debug.log and probably where it's getting that data to begin with. I saw this code but didn't look into where "rescan" gets passed to it. https://git.hush.is/hush/SilentDragon/src/branch/dev/src/connection.cpp#L528
getwalletinfodoc says it returns like thisI'll try to get my code working with this instead. Won't have current block height it's at and only progress percentage.
Our
getwalletinfocurrently doesn't have this support. When I try calling it when rescanning, it also just hangs.@fekt yeah, this stuff was added in BTC Core 0.17.x which is after our internals forked, so we don't have the stuff in those docs, but it shows it's possible.
I believe BTC Core made some changes that allow some RPCs to return data even during a rescan, I am looking into it.
This is the BTC Core PR that adds rescan data to getwalletinfo: https://github.com/bitcoin/bitcoin/pull/15730
See hush/hush3#222 for details about the new RPC
getrescaninfoLatest commit to dev branch has this implemented. For the time being, it will update in bottom left status bar only when using rescan option under settings. Need to figure out how to handle on startup or when restarting.
I also need to test on a large wallet to see what happens with other timers and look into pausing them for performance while rescanning or see if they create issues.
The status will update every 1 second but can be changed to not be as frequent. I had to set it fast for testing to see progress because rescans are too quick on a small wallet with not a lot of txs.
@fekt super awesome, I will test soon
@duke I tested on a large wallet and it's a bit janky. Seems like starting the rescan never even happens possibly due to other RPCs/timers running and causing it to timeout or something. I tried multiple times where it never showed it starting in log and transaction data kept trying to load at same time. The times it did start, it seemed like getrescaninfo was slow in being called or responding too. Probably need to look into and implement pausing everything else when starting rescan. On small wallets it seems to work as intended.
@fekt I think we should put your code onto a
rescanbranch and yes, we probably want to pause other timers for it to work well with large wallets. This is a big enough change that we should get testing from a few different people on different wallets to make sure it works correctly.Thanks for working on this, I think it will end up being a huge improvement, once we shake out some bugs.
@duke I can create a rescan branch that's a copy of current dev and then revert rescan option back to original behavior if that's what you want. Let me know. I may have some time tonight or should be free tomorrow.
@fekt I am in the process of reviewing the code and testing. I think it's ok to leave it on the
devbranch right now, since it makes it easier for others to test it together with all the other recent SD changes. If we want to make an SD release and the code isn't ready for "prime time" yet, then we can put it on it's own branch and work on it for the next release.@jahway603 @onryo do you have time to help test these rescan changes?
@fekt with my latest commits to
dev, I think this works "good enough". Users can rescan from an arbitrary height without a restart. I did not implement pausing other timers because I was seeing issues with that, at least with my large wallet I used for testing. I am still not seeing the statusbar update correctly but I am thinking that is an issue with my large wallet. I want to do some testing with a small wallet before closing this.@duke Seems to work fine on my small Mac wallet. I tested pausing other timers previously and didn't seem to help on large wallets. I can test latest changes on a larger wallet. The height initialization should at least fix the issue of it sometimes showing 100% at first.
@fekt the problem with large wallets seems to be that we don't have enough QThread's doing different things, so some stuff blocks the UI updating. https://doc.qt.io/qt-5/qthread.html This is related to how we inherit from SingleApplication : https://git.hush.is/hush/SilentDragon/src/branch/master/singleapplication
We might want to fiddle with that stuff for a future release, but it seems to be a bigger can of worms than we want to open for our next SD release.
Yeah, large wallet still funky with updating status and a possible new issue. I tried a rescan a couple of times and getrescaninfo shows it's 100% complete but keeps saying rescanning is true for awhile. The UI status in bottom right seems to stop updating and stays on block height from when rescan started unless right clicking to refresh. I didn't check code but maybe just from stopping other timers if you did that. Once I refresh once, it continues to update regularly.
@fekt not sure which code you are using, but my latest dev branch code does not pause the main UI timer. Maybe it should, not sure. I haven't tried right clicking the statusbar and refreshing, I should try that.
@fekt going to call this "done" and we can open up specific issues for improving it, as it definitely could be improved