Wallet RPCs can return stale info due to ProcessNewBlock Race #9148

1. 
   TheBlueMatt commented at 10:20 pm on November 12, 2016: member

   I know this issue has been pointed out a few times, but didnt see an issue for it here.

   After #7946, if you connect a block, then immediately make some RPC request to get the current block information (eg if you’re polling to see when a new block comes in) and then immediately request wallet balance/utxos/etc, the information returned can be up-to-date as of the previous block, not the newly-connected one. This is easy to see in some simple test-cases, and I assume would effect some various use-cases that people might be using in the wild. This is a regression in master, so we should fix it for 0.14.

   I think we need to be moving more towards these kinds of callback happening in a background thread anyway, so we have two options - change the API significantly and add some kind of “getwalletchainstate” so that people can wait for wallet to be caught up or add a “sleep until wallet is caught up with chainstate as of entry to RPC function” wrapper to wallet calls.

   I generally prefer the second option becuase I think the first will create a somewhat confusing API that people are likely to fuck up, and the sleep should generally never be too long unless your wallet is super slow/too many keys.

2. 
   gmaxwell commented at 10:23 pm on November 12, 2016: contributor

   the rpc returning old data right at the time of the block seems fine to me, what isn’t fine is it showing old data after various notify messages have fired– which is I assume an unstated part of what you described?
3. 
   TheBlueMatt commented at 10:34 pm on November 12, 2016: member

   Well you can make an RPC call to getbestblockhash, get the current tip, and then /after that/ call getbalance and get the wallet balance for one block back, not up-to-date with the tip (which would break people who poll for updates, as I’m sure some people do). Indeed, its also possible that notify messages have fired, but I’d have to go look at the order of callbacks to verify that.
4. MarcoFalke added the label Brainstorming on Nov 12, 2016
5. MarcoFalke added this to the milestone 0.14.0 on Nov 12, 2016
6. 
   luke-jr commented at 0:51 am on November 13, 2016: member

   Perhaps the solution ought to be to grab a lock on all the wallets before unlocking the blockchain (and unlocking each once they have synced)?
7. 
   TheBlueMatt commented at 7:40 am on November 13, 2016: member

   @luke-jr that would be a layer violation…nothing blockchain-related should have any concept of wallet(s).
8. 
   luke-jr commented at 8:54 am on November 13, 2016: member

   Should we keep a copy of the tip CBlockIndex* on the wallet then, perhaps? Then RPC calls accessing the wallet can block on that wallet matching the blockchain?
9. MarcoFalke added the label Wallet on Nov 13, 2016
10. TheBlueMatt referenced this in commit 84473fa2d2 on Jan 17, 2017
11. TheBlueMatt referenced this in commit 79be39f893 on Jan 17, 2017
12. TheBlueMatt referenced this in commit c6134a82e7 on Jan 17, 2017
13. TheBlueMatt referenced this in commit 584fe5e02f on Jan 17, 2017
14. TheBlueMatt referenced this in commit 5718a2ca22 on Jan 18, 2017
15. TheBlueMatt referenced this in commit 3c410c06d9 on Jan 18, 2017
16. TheBlueMatt referenced this in commit cbf8fb90b0 on Jan 18, 2017
17. TheBlueMatt referenced this in commit a40e0ea01b on Jan 18, 2017
18. TheBlueMatt referenced this in commit 901e985f81 on Jan 18, 2017
19. TheBlueMatt referenced this in commit 4ec00df54f on Jan 18, 2017
20. TheBlueMatt referenced this in commit bc4c4c6686 on Jan 18, 2017
21. TheBlueMatt referenced this in commit 11f2fe39ec on Jan 18, 2017
22. TheBlueMatt referenced this in commit 662439c29f on Jan 18, 2017
23. TheBlueMatt referenced this in commit b9af07bed5 on Jan 18, 2017
24. TheBlueMatt referenced this in commit e46856d7a2 on Jan 18, 2017
25. TheBlueMatt referenced this in commit 3ccf3d4ca6 on Jan 18, 2017
26. TheBlueMatt referenced this in commit 9010f29e90 on Jan 18, 2017
27. TheBlueMatt referenced this in commit 997b578c46 on Jan 18, 2017
28. TheBlueMatt referenced this in commit 593a8f144d on Jan 18, 2017
29. TheBlueMatt referenced this in commit d632a99951 on Jan 18, 2017
30. TheBlueMatt referenced this in commit 27c27eedc4 on Jan 19, 2017
31. TheBlueMatt referenced this in commit a2c1cf0753 on Jan 19, 2017
32. TheBlueMatt referenced this in commit 56b2ea116b on Jan 20, 2017
33. TheBlueMatt referenced this in commit e797cb4753 on Jan 20, 2017
34. TheBlueMatt referenced this in commit d7ee88d68e on Jan 20, 2017
35. TheBlueMatt referenced this in commit fbb1ae1c37 on Jan 20, 2017
36. TheBlueMatt referenced this in commit 035748c545 on Jan 20, 2017
37. TheBlueMatt referenced this in commit 46c571091c on Jan 20, 2017
38. TheBlueMatt referenced this in commit 056f15e29a on Jan 20, 2017
39. TheBlueMatt referenced this in commit e646cd9703 on Jan 20, 2017
40. TheBlueMatt referenced this in commit 09627ea2c2 on Jan 20, 2017
41. TheBlueMatt referenced this in commit 598bcca5b2 on Jan 20, 2017
42. TheBlueMatt referenced this in commit dd0baa66a6 on Jan 20, 2017
43. TheBlueMatt referenced this in commit c9c06bc90a on Jan 20, 2017
44. TheBlueMatt referenced this in commit db8f8a97d9 on Jan 20, 2017
45. TheBlueMatt referenced this in commit 90b852a0e9 on Jan 20, 2017
46. TheBlueMatt referenced this in commit 31bca14642 on Jan 20, 2017
47. TheBlueMatt referenced this in commit 6727bea9c9 on Jan 20, 2017
48. 
    TheBlueMatt commented at 4:23 pm on January 23, 2017: member

    Fixed (temporarily) with #9583 merge.
49. TheBlueMatt closed this on Jan 23, 2017

    ---

50. MarcoFalke locked this on Sep 8, 2021

Contributors
TheBlueMatt gmaxwell luke-jr

Labels
Brainstorming Wallet

Milestone
0.14.0