This RPC is very expensive, and SD calls it like crazy. SD should only run this RPC if itself has created a ztx and is waiting to see if it is accepted. The only way this data is used is to tell the user if something went wrong with a new ztx, otherwise the data is never shown or rendered to the user. It's a big waste of resources to constantly call it.
This RPC is very expensive, and SD calls it like crazy. SD should only run this RPC if itself has created a ztx and is waiting to see if it is accepted. The only way this data is used is to tell the user if something went wrong with a new ztx, otherwise the data is never shown or rendered to the user. It's a big waste of resources to constantly call it.
Related to #33
There is a quicker timer that will run every 3 seconds (quickUpdateSpeed) if watchingOps is not empty. I did not search yet if watchTxStatus gets called anywhere else but sounds like it should be removed from starting on timer and constantly polling. If watchingOps is empty, it should probably also not start a timer here either https://git.hush.is/hush/SilentDragon/src/branch/dev/src/rpc.cpp#L1183. That might cause it to keep polling too after the first time it's called.
I'll try playing around with some of this code.
@duke I was looking into this and the only thing that calls `z_getoperationstatus` is `watchTxStatus` here: https://git.hush.is/hush/SilentDragon/src/branch/dev/src/rpc.cpp#L1141
This looks to be running on a timer every 10 seconds (`updateSpeed`) along with some other RPCs:
https://git.hush.is/hush/SilentDragon/src/branch/dev/src/rpc.cpp#L59
There is a quicker timer that will run every 3 seconds (`quickUpdateSpeed`) if `watchingOps` is not empty. I did not search yet if `watchTxStatus` gets called anywhere else but sounds like it should be removed from starting on timer and constantly polling. If `watchingOps` is empty, it should probably also not start a timer here either https://git.hush.is/hush/SilentDragon/src/branch/dev/src/rpc.cpp#L1183. That might cause it to keep polling too after the first time it's called.
I'll try playing around with some of this code.
@fekt you are right, it's only called in 2 places, which makes it easier to modify. Currently it starts polling no matter what, when the wallet starts and it also calls it when a new tx is made.
The reason for always looking is that maybe hushd was started manually and/or there is code or hush-cli or cron jobs/etc that make transactions. Only looking when a new tx is made is much more efficient.
One way we could change this is to assume that if people have an autostarted embedded hushd, they are not using external code or crons and to not always call watchTxStatus() on wallet start for those people. For people where SD detects an already running hushd, we want to be able to detect changes from the CLI, so it's probably good to always run it. Your thoughts @fekt ?
@fekt you are right, it's only called in 2 places, which makes it easier to modify. Currently it starts polling no matter what, when the wallet starts and it also calls it when a new tx is made.
The reason for always looking is that maybe hushd was started manually and/or there is code or hush-cli or cron jobs/etc that make transactions. Only looking when a new tx is made is much more efficient.
One way we could change this is to assume that if people have an autostarted embedded hushd, they are not using external code or crons and to not always call `watchTxStatus()` on wallet start for those people. For people where SD detects an already running hushd, we want to be able to detect changes from the CLI, so it's probably good to always run it. Your thoughts @fekt ?
@fekt we could also add an option, if you right click anywhere in the Transaction list, that one option/action is "Look for new transactions" which calls watchTxStatus()
@fekt we could also add an option, if you right click anywhere in the Transaction list, that one option/action is "Look for new transactions" which calls `watchTxStatus()`
@duke I think it's fixed in this but probably needs to be tested further if it causes any issues. May need to test with a failed tx. Not sure if anything else may rely on this being polled regularly or if started on timer for a reason: 26a9788f7f
I changed to not start the txTimer on load and stop it when watchingOps is empty. I tested sending txs and z_getoperationstatus seems to now only run when sending a tx, polls with quickUpdateSpeed, and stops when tx successfully sent.
@duke I think it's fixed in this but probably needs to be tested further if it causes any issues. May need to test with a failed tx. Not sure if anything else may rely on this being polled regularly or if started on timer for a reason: https://git.hush.is/hush/SilentDragon/commit/26a9788f7fb105dab55be0e36736cd664fc69bac
I changed to not start the txTimer on load and stop it when `watchingOps` is empty. I tested sending txs and z_getoperationstatus seems to now only run when sending a tx, polls with `quickUpdateSpeed`, and stops when tx successfully sent.
@fekt we could also add an option, if you right click anywhere in the Transaction list, that one option/action is "Look for new transactions" which calls watchTxStatus()
Had some comment lag. Not sure if commit I added will cause issues with not running it on start and polling regularly.
I get what you are saying about it possibly being needed to run regularly to detect changes from CLI. This option may be a workaround. I guess I need to test sending txs from CLI with SD open.
> @fekt we could also add an option, if you right click anywhere in the Transaction list, that one option/action is "Look for new transactions" which calls `watchTxStatus()`
Had some comment lag. Not sure if commit I added will cause issues with not running it on start and polling regularly.
I get what you are saying about it possibly being needed to run regularly to detect changes from CLI. This option may be a workaround. I guess I need to test sending txs from CLI with SD open.
Sending from cli updates GUI balance but transactions table never seems to show the txs. I added option to "Look for new transactions" that calls watchTxStatus(). This only calls z_getoperationstatus once and timer doesn't keep running.
I also see the table refresh every time there's a block update, but these cli sent txs don't show. They don't show on restarts either. Txs sent from GUI show fine. Could be my wallet.
This was a fairly new wallet.dat, but I may have some weird data on disk. On this machine, for whatever reason, when I create a new wallet.dat, it shows me a parital tx history in SD from a previous wallet.dat that I didn't even import keys for. I may have imported the keys to another wallet.dat that's long been renamed or deleted. Is there some data I can try deleting rather than deleting everything and waiting to index?
Sending from cli updates GUI balance but transactions table never seems to show the txs. I added option to "Look for new transactions" that calls `watchTxStatus()`. This only calls z_getoperationstatus once and timer doesn't keep running.
I also see the table refresh every time there's a block update, but these cli sent txs don't show. They don't show on restarts either. Txs sent from GUI show fine. Could be my wallet.
This was a fairly new wallet.dat, but I may have some weird data on disk. On this machine, for whatever reason, when I create a new wallet.dat, it shows me a parital tx history in SD from a previous wallet.dat that I didn't even import keys for. I may have imported the keys to another wallet.dat that's long been renamed or deleted. Is there some data I can try deleting rather than deleting everything and waiting to index?
I was impatient and deleted everything but was still showing old txs from a different wallet. Discovered it was from "Remember shielded transactions" option in settings and stores in senttxstore.dat that's stored elsewhere. Chilling until it finishes indexing lol.
I was impatient and deleted everything but was still showing old txs from a different wallet. Discovered it was from "Remember shielded transactions" option in settings and stores in `senttxstore.dat` that's stored elsewhere. Chilling until it finishes indexing lol.
@fekt you are right, SD does some weirdness when the underlying wallet.dat changes, which causes senttxstore.dat to be out of sync. Currently, if that happens, you will see lots of error API responses in the console, but no weirdness in the GUI. We really should delete data from senttxstore.dat, if it's not in the current wallet.dat, but SD doesn't do that right now. See this issue I created a while ago about this issue: #55
Do you have thoughts on the best way to deal with this? Maybe, if we find that senttxstore.dat has data that isn't in the current wallet, we backup the whole file and start fresh? There are edge cases, like a wallet or hushd crashing and maybe just being out of sync with some data, not all data.
The "right" thing to do is to always move addresslabels.dat and senttxstore.dat out of the way, if you are changing wallet.dat, but I never remember to do that and often I am only doing tests on hushd with new wallets and then later on do tests with SD, so it's not easy or intuitive to do the "right" thing.
@fekt you are right, SD does some weirdness when the underlying wallet.dat changes, which causes senttxstore.dat to be out of sync. Currently, if that happens, you will see lots of error API responses in the console, but no weirdness in the GUI. We really should delete data from senttxstore.dat, if it's not in the current wallet.dat, but SD doesn't do that right now. See this issue I created a while ago about this issue: https://git.hush.is/hush/SilentDragon/issues/55
Do you have thoughts on the best way to deal with this? Maybe, if we find that senttxstore.dat has data that isn't in the current wallet, we backup the whole file and start fresh? There are edge cases, like a wallet or hushd crashing and maybe just being out of sync with some data, not all data.
The "right" thing to do is to always move addresslabels.dat and senttxstore.dat out of the way, if you are changing wallet.dat, but I never remember to do that and often I am only doing tests on hushd with new wallets and then later on do tests with SD, so it's not easy or intuitive to do the "right" thing.
Still indexing but I tested sending a tx from cli on another machine running SD 1.3.0. Same issue where the tx never shows in SD. Seems like a pre-existing bug unrelated to this change and may need own issue raised if it should be compatible or fixed under #55
It may be that SD only displays txs from senttxstore.dat and hush-cli doesn't use that file or write anything to it. On Linux, this file should be located here: /home/user/.local/share/Hush/SilentDragon/senttxstore.dat
Can't seem to locate on Windows. Thought it'd be in: C:\Users\user\AppData\Roaming\Hush\SilentDragon There is an addresslabels.dat and SilentDragon.log. The log could probably use a check to keep under certain size. Mine is ~500 MB and has data from when still KMD assetchain.
Not sure on best way to fix. Could be something weird with how/when SD decides to write data to senttxstore.dat.
Still indexing but I tested sending a tx from cli on another machine running SD 1.3.0. Same issue where the tx never shows in SD. Seems like a pre-existing bug unrelated to this change and may need own issue raised if it should be compatible or fixed under https://git.hush.is/hush/SilentDragon/issues/55
It may be that SD only displays txs from `senttxstore.dat` and hush-cli doesn't use that file or write anything to it. On Linux, this file should be located here: `/home/user/.local/share/Hush/SilentDragon/senttxstore.dat`
Can't seem to locate on Windows. Thought it'd be in: `C:\Users\user\AppData\Roaming\Hush\SilentDragon` There is an addresslabels.dat and SilentDragon.log. The log could probably use a check to keep under certain size. Mine is ~500 MB and has data from when still KMD assetchain.
Not sure on best way to fix. Could be something weird with how/when SD decides to write data to `senttxstore.dat`.
@fekt ever since we inherited the feature from the original ZecWallet code, senttxstore.dat only stores data sent from SD, it doesn't know anything about tx's sent via hush-cli. I think it's reasonable to call it a bug, and it's maybe possible to fill in the data from z_getoperationstatus, i.e. "if we see an opid that is successful for a sent tx, and our senttxstore.dat doesn't have that data, fill it in".
We also could improve our RPC's to show all sent/received tx's for a zaddr. SD still doesn't use the getalldata RPC yet, which would be much more efficient, and maybe has the data we need.
@fekt ever since we inherited the feature from the original ZecWallet code, `senttxstore.dat` only stores data sent *from SD*, it doesn't know anything about tx's sent via hush-cli. I think it's reasonable to call it a bug, and it's maybe possible to fill in the data from `z_getoperationstatus`, i.e. "if we see an opid that is successful for a sent tx, and our senttxstore.dat doesn't have that data, fill it in".
We also could improve our RPC's to show all sent/received tx's for a zaddr. SD still doesn't use the `getalldata` RPC yet, which would be much more efficient, and maybe has the data we need.
This RPC is very expensive, and SD calls it like crazy. SD should only run this RPC if itself has created a ztx and is waiting to see if it is accepted. The only way this data is used is to tell the user if something went wrong with a new ztx, otherwise the data is never shown or rendered to the user. It's a big waste of resources to constantly call it.
Related to #33
@duke I was looking into this and the only thing that calls
z_getoperationstatusiswatchTxStatushere: https://git.hush.is/hush/SilentDragon/src/branch/dev/src/rpc.cpp#L1141This looks to be running on a timer every 10 seconds (
updateSpeed) along with some other RPCs:https://git.hush.is/hush/SilentDragon/src/branch/dev/src/rpc.cpp#L59
There is a quicker timer that will run every 3 seconds (
quickUpdateSpeed) ifwatchingOpsis not empty. I did not search yet ifwatchTxStatusgets called anywhere else but sounds like it should be removed from starting on timer and constantly polling. IfwatchingOpsis empty, it should probably also not start a timer here either https://git.hush.is/hush/SilentDragon/src/branch/dev/src/rpc.cpp#L1183. That might cause it to keep polling too after the first time it's called.I'll try playing around with some of this code.
@fekt you are right, it's only called in 2 places, which makes it easier to modify. Currently it starts polling no matter what, when the wallet starts and it also calls it when a new tx is made.
The reason for always looking is that maybe hushd was started manually and/or there is code or hush-cli or cron jobs/etc that make transactions. Only looking when a new tx is made is much more efficient.
One way we could change this is to assume that if people have an autostarted embedded hushd, they are not using external code or crons and to not always call
watchTxStatus()on wallet start for those people. For people where SD detects an already running hushd, we want to be able to detect changes from the CLI, so it's probably good to always run it. Your thoughts @fekt ?@fekt we could also add an option, if you right click anywhere in the Transaction list, that one option/action is "Look for new transactions" which calls
watchTxStatus()@duke I think it's fixed in this but probably needs to be tested further if it causes any issues. May need to test with a failed tx. Not sure if anything else may rely on this being polled regularly or if started on timer for a reason:
26a9788f7fI changed to not start the txTimer on load and stop it when
watchingOpsis empty. I tested sending txs and z_getoperationstatus seems to now only run when sending a tx, polls withquickUpdateSpeed, and stops when tx successfully sent.Had some comment lag. Not sure if commit I added will cause issues with not running it on start and polling regularly.
I get what you are saying about it possibly being needed to run regularly to detect changes from CLI. This option may be a workaround. I guess I need to test sending txs from CLI with SD open.
Sending from cli updates GUI balance but transactions table never seems to show the txs. I added option to "Look for new transactions" that calls
watchTxStatus(). This only calls z_getoperationstatus once and timer doesn't keep running.I also see the table refresh every time there's a block update, but these cli sent txs don't show. They don't show on restarts either. Txs sent from GUI show fine. Could be my wallet.
This was a fairly new wallet.dat, but I may have some weird data on disk. On this machine, for whatever reason, when I create a new wallet.dat, it shows me a parital tx history in SD from a previous wallet.dat that I didn't even import keys for. I may have imported the keys to another wallet.dat that's long been renamed or deleted. Is there some data I can try deleting rather than deleting everything and waiting to index?
I was impatient and deleted everything but was still showing old txs from a different wallet. Discovered it was from "Remember shielded transactions" option in settings and stores in
senttxstore.datthat's stored elsewhere. Chilling until it finishes indexing lol.@fekt you are right, SD does some weirdness when the underlying wallet.dat changes, which causes senttxstore.dat to be out of sync. Currently, if that happens, you will see lots of error API responses in the console, but no weirdness in the GUI. We really should delete data from senttxstore.dat, if it's not in the current wallet.dat, but SD doesn't do that right now. See this issue I created a while ago about this issue: #55
Do you have thoughts on the best way to deal with this? Maybe, if we find that senttxstore.dat has data that isn't in the current wallet, we backup the whole file and start fresh? There are edge cases, like a wallet or hushd crashing and maybe just being out of sync with some data, not all data.
The "right" thing to do is to always move addresslabels.dat and senttxstore.dat out of the way, if you are changing wallet.dat, but I never remember to do that and often I am only doing tests on hushd with new wallets and then later on do tests with SD, so it's not easy or intuitive to do the "right" thing.
Still indexing but I tested sending a tx from cli on another machine running SD 1.3.0. Same issue where the tx never shows in SD. Seems like a pre-existing bug unrelated to this change and may need own issue raised if it should be compatible or fixed under #55
It may be that SD only displays txs from
senttxstore.datand hush-cli doesn't use that file or write anything to it. On Linux, this file should be located here:/home/user/.local/share/Hush/SilentDragon/senttxstore.datCan't seem to locate on Windows. Thought it'd be in:
C:\Users\user\AppData\Roaming\Hush\SilentDragonThere is an addresslabels.dat and SilentDragon.log. The log could probably use a check to keep under certain size. Mine is ~500 MB and has data from when still KMD assetchain.Not sure on best way to fix. Could be something weird with how/when SD decides to write data to
senttxstore.dat.@fekt ever since we inherited the feature from the original ZecWallet code,
senttxstore.datonly stores data sent from SD, it doesn't know anything about tx's sent via hush-cli. I think it's reasonable to call it a bug, and it's maybe possible to fill in the data fromz_getoperationstatus, i.e. "if we see an opid that is successful for a sent tx, and our senttxstore.dat doesn't have that data, fill it in".We also could improve our RPC's to show all sent/received tx's for a zaddr. SD still doesn't use the
getalldataRPC yet, which would be much more efficient, and maybe has the data we need.z_getoperationstatus is now called less often, calling this good for now