Abort sync_unspent if blockheight RPC call fails #1348
Conversation
|
I've done some testing and I believe this is functionally correct. There is a slightly suboptimal aspect of each of the effects, on CLI and on Qt, but imo it's fine (since it's such a pain to get everything exactly perfect, and this does the job, and also this is a rare condition). On CLI it looks like this: suboptimal because last two lines contradict, but color coded RED failure message and the fact that it shuts down,is good enough. And on Qt you get a stack trace after the Qt app shuts down, but only after you got a modal dialog telling you that the RPC call fails, so the user is forced to see the actual reason, the stack trace won't really matter. |
| for _ in range(5): | ||
| current_blockheight = self.bci.get_current_block_height() | ||
| if current_blockheight: | ||
| break |
There was a problem hiding this comment.
Should we add some delay here maybe? In case bitcoind is temporary not responding, five very fast consecutive calls has higher probability to fail than, let's say, five calls in five seconds.
There was a problem hiding this comment.
Not a bad idea but I'm worried about doing it synchronously.
There is already a delay where you wait for the RPC call to return (that part of the code is synchronous now).
There was a problem hiding this comment.
On second thoughts this is a blocker anyway (and of course, usually not - usually it just works first time), so there's not much reason to not wait before repeating the RPC call.
It feels really bad to put a sleep call in here, but maybe that's the right thing to do?
There was a problem hiding this comment.
See 84686b9 . I thought about this for a while and decided: this is a step before the main monitoring loop begins, and it is by design a synchronous operation; I'm not actually sure an extra half second sleep is needed (since we are in any case waiting for the RPC call to return), but, just in case it helps, I added it.
|
@kristapsk let me know what to do about the Py3.6 fail (assuming you know!) and also if the above answers your concern about delays. If so, I will merge it. |
You should rebase, it was solved with #1409. Python 3.6 is not available for ubuntu-latest anymore.
Yes, it's ok with me, minor detail anyway. |
AdamISZ
force-pushed
the
utxo-sync-no-blockheight
branch
from
84686b9 to
4775689
Compare
|
Huh, weird, I rebased on master (which includes #1409 ) but it is still trying to do py3.6 . |
|
I'm gonna assume it's just a weird CI error and merge and see what happens. |
Fixes #510. Prior to this commit, if the call to jmclient.BitcoinCoreInterface.get_current_blockheight() failed and returned None, the function jmclient.WalletService.sync_unspent() would simply ignore the failure and the wallet syncing process would complete, without ever resetting and updating the wallet's utxo set, resulting in a situation where users saw out of date utxo sets in their wallets (and concomitant spending failures). After this commit, if that blockheight access call fails 5 times, we instead abandon the attempt to sync the wallet, and shut down the application.
AdamISZ
force-pushed
the
utxo-sync-no-blockheight
branch
from
4775689 to
0b34e0b
Compare
Fixes #510.
Prior to this commit, if the call to
jmclient.BitcoinCoreInterface.get_current_blockheight() failed and returned None, the function jmclient.WalletService.sync_unspent() would simply ignore the failure and the wallet syncing process would complete, without ever resetting and updating the wallet's utxo set, resulting in a situation where users saw out of date utxo sets in their wallets (and concomitant spending failures).
After this commit, if that blockheight access call fails 5 times, we instead abandon the attempt to sync the wallet, and shut down the application.