Synchronization problems with wallet. #9584

issue morcos openend this issue on January 19, 2017
  1. morcos commented at 4:28 pm on January 19, 2017: member

    Since #7946 we don’t hold cs_main while syncing transactions connected in a block with our wallet. There have been several previously identified open issues such as #9148 and proposed fixes before 0.14.

    There are at least two more potential unsolved problems:

    1. There is a race condition where if two blocks are connected in different threads the wallet could receive the transactions in the connected blocks out of order. In particular if there was a reorg, and a wallet transaction appeared on both chains of the reorg, and the wallet received notification of the transaction in the stale block after receiving notification of the transaction in the current tip block, then this could lead to the wallet believing a confirmed transaction is currently conflicted because it will have the wrong hashBlock. One potential scenario that could cause this race is a miner issuing a preciousblock rpc call (for a block tied with the current tip) simultaneously with receiving a new block building off the current tip.
    2. CreateTransaction calls take cs_main, which used to mean that the wallet, mempool and chainstate were necessarily synced. However it is unknown whether problems could be caused by trying to create a new transaction while the mempool and chain state might be ahead of where the wallet is synced. This is probably not an actual problem, but needs further review.
      • Certainly it is possible that transactions might be created which are no longer valid and won’t be accepted to the mempool.
      • Additionally it needs to be carefully thought about whether it is ok for the wallet transaction to be created based on chain state information from the current tip but then possibly updated afterwards with stale information.
  2. TheBlueMatt commented at 11:13 pm on January 19, 2017: member
    For 2. There dont seem to be too many issues here, however, there is at least one - you could go to select coins, need to use 0-conf change, but such 0-conf change may have been included in a block who’s callbacks have not yet been processed - resulting in thinking they are not in mempool and, thus, not selectable.
  3. fanquake added the label Wallet on Jan 20, 2017
  4. TheBlueMatt referenced this in commit 97ff645460 on Jan 20, 2017
  5. TheBlueMatt referenced this in commit 4c1fe09b3f on Jan 20, 2017
  6. TheBlueMatt referenced this in commit a5bb650528 on Jan 20, 2017
  7. TheBlueMatt referenced this in commit dbe3075fae on Jan 20, 2017
  8. TheBlueMatt referenced this in commit 67f800c039 on Jan 20, 2017
  9. TheBlueMatt referenced this in commit a8c28a1984 on Jan 20, 2017
  10. TheBlueMatt referenced this in commit 12d0b923ef on Jan 20, 2017
  11. TheBlueMatt referenced this in commit 7bf80cbeab on Jan 20, 2017
  12. TheBlueMatt referenced this in commit c92fda773a on Jan 20, 2017
  13. TheBlueMatt referenced this in commit b802a5bb8c on Jan 20, 2017
  14. TheBlueMatt referenced this in commit 99c493ed1f on Jan 20, 2017
  15. TheBlueMatt referenced this in commit 4977fec015 on Jan 20, 2017
  16. TheBlueMatt referenced this in commit ce8029dacb on Jan 23, 2017
  17. TheBlueMatt referenced this in commit cd8491ad13 on Jan 23, 2017
  18. TheBlueMatt referenced this in commit c91ad39b76 on Jan 25, 2017
  19. TheBlueMatt referenced this in commit b43b9309ac on Feb 8, 2017
  20. TheBlueMatt referenced this in commit af18f6ef58 on Feb 8, 2017
  21. TheBlueMatt referenced this in commit 7986485cd6 on Feb 8, 2017
  22. TheBlueMatt referenced this in commit 67d86dcd85 on Feb 18, 2017
  23. TheBlueMatt referenced this in commit 791202c527 on Feb 18, 2017
  24. TheBlueMatt referenced this in commit 423fb08934 on Mar 6, 2017
  25. TheBlueMatt referenced this in commit e5d1abd4a1 on Mar 7, 2017
  26. TheBlueMatt referenced this in commit 28f6984ec3 on Mar 8, 2017
  27. TheBlueMatt referenced this in commit 0a9a54b502 on Apr 10, 2017
  28. TheBlueMatt referenced this in commit d5066c86fc on Apr 17, 2017
  29. TheBlueMatt referenced this in commit f6f07c1ebf on Apr 26, 2017
  30. TheBlueMatt referenced this in commit 4828775bdd on Apr 26, 2017
  31. TheBlueMatt referenced this in commit 038431e6e2 on Apr 26, 2017
  32. TheBlueMatt referenced this in commit a5621fd843 on Apr 26, 2017
  33. TheBlueMatt referenced this in commit c9a2c89ad0 on Apr 26, 2017
  34. TheBlueMatt referenced this in commit f428a93c8b on Apr 26, 2017
  35. TheBlueMatt referenced this in commit ed6056d5f8 on Apr 26, 2017
  36. TheBlueMatt referenced this in commit beaf85a434 on Apr 26, 2017
  37. TheBlueMatt referenced this in commit 49683b5400 on Apr 27, 2017
  38. TheBlueMatt referenced this in commit 4ec6e2f482 on Apr 27, 2017
  39. TheBlueMatt referenced this in commit 8305f1e1ae on Apr 27, 2017
  40. TheBlueMatt referenced this in commit b8255f8102 on May 3, 2017
  41. ryanofsky commented at 1:47 am on May 4, 2017: member
    @TheBlueMatt, are all the “unsolved problems” mentioned in your and Alex’s comments above fixed by #10286, or will some still remain?
  42. TheBlueMatt commented at 2:19 am on May 4, 2017: member
    @ryanofsky they were technically fixed in #9583, but I do not believe #10286 introduces any new (serious) bugs. It is the case that you may not select some coins based on a mistaken belief that they are unconfirmed when they are, but this should still only happen if they confirmed in a block you processed after the RPC call was entered (ie after the block-until-caught-up call) but before the actual CreateTransaction call. I do not believe it ever is an issue, but its probably worth re-reading (and re-re-reading) a bunch of wallet logic to make sure the new InMempool()-may-be-out-of-sync-with-GetDepthInMainChain() semantics dont cause issues.
  43. TheBlueMatt referenced this in commit 9dc2c6af69 on May 4, 2017
  44. TheBlueMatt referenced this in commit 0f83c3848a on May 5, 2017
  45. TheBlueMatt referenced this in commit e94bb54960 on Jun 8, 2017
  46. TheBlueMatt referenced this in commit 3a91288150 on Jun 8, 2017
  47. TheBlueMatt referenced this in commit 1db7d16fad on Jun 8, 2017
  48. TheBlueMatt referenced this in commit 5caf2037fc on Jun 9, 2017
  49. TheBlueMatt referenced this in commit 24c78cd325 on Jun 9, 2017
  50. TheBlueMatt referenced this in commit 3e5a39aba4 on Jun 21, 2017
  51. TheBlueMatt referenced this in commit f70640cd32 on Jun 21, 2017
  52. TheBlueMatt referenced this in commit 8146b7d914 on Jun 21, 2017
  53. TheBlueMatt referenced this in commit 751cfbb95e on Jun 21, 2017
  54. TheBlueMatt referenced this in commit f3aa696ca0 on Jul 7, 2017
  55. TheBlueMatt referenced this in commit 323f59563e on Jul 7, 2017
  56. TheBlueMatt referenced this in commit b9bd72959b on Jul 11, 2017
  57. TheBlueMatt referenced this in commit daab5ea6d8 on Aug 14, 2017
  58. TheBlueMatt referenced this in commit 29b5fe6163 on Aug 17, 2017
  59. TheBlueMatt referenced this in commit 975f071b49 on Sep 12, 2017
  60. jnewbery referenced this in commit 2c37f67d60 on Sep 13, 2017
  61. TheBlueMatt referenced this in commit 7abf2a820b on Oct 1, 2017
  62. TheBlueMatt referenced this in commit a0b7531bff on Oct 13, 2017
  63. TheBlueMatt referenced this in commit 816685899f on Oct 13, 2017
  64. TheBlueMatt referenced this in commit 17220d6325 on Oct 13, 2017
  65. laanwj referenced this in commit 898f560b55 on Jan 18, 2018
  66. HashUnlimited referenced this in commit 5a8dade659 on Mar 13, 2018
  67. PastaPastaPasta referenced this in commit bfad945f7f on Jun 13, 2020
  68. PastaPastaPasta referenced this in commit 34ea4efebe on Jun 13, 2020
  69. PastaPastaPasta referenced this in commit b5ab77a266 on Jun 17, 2020
  70. PastaPastaPasta referenced this in commit a022cc5ec7 on Jun 18, 2020
  71. jarolrod commented at 11:32 pm on January 16, 2021: member

    This appears to have been fixed by #9583 and #10286.

    Additionally, there appears to be test coverage for this in test/functional/wallet_reorgrestore.py

  72. ryanofsky commented at 10:33 pm on January 20, 2021: member
    Will close this issue. #18842 is a current example of a wallet state & notification race, but I think we settled into a general approach to dealing with these problems: updating wallet transaction state in anticipation of node notifications where possible, and blocking to wait for notifications where it’s not possible. I think there’s no need to keep an umbrella issue open, but this can be reopened if there are more problems.
  73. ryanofsky closed this on Jan 20, 2021
  74. furszy referenced this in commit f2bce7c829 on Feb 10, 2021
  75. furszy referenced this in commit c5ab37d058 on Feb 18, 2021
  76. furszy referenced this in commit b173965cf5 on Feb 19, 2021
  77. furszy referenced this in commit 31a8790f22 on Feb 21, 2021
  78. gades referenced this in commit f08e50275f on Mar 22, 2022
  79. DrahtBot locked this on Aug 18, 2022


morcos TheBlueMatt ryanofsky jarolrod

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-05 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me