don't attempt mempool entry for wallet transactions on startup if alr… #11839

1. 
   instagibbs commented at 4:50 PM on December 6, 2017: member

   …eady in mempool

   Mempool loads first, wallet second. Second attempt fails, marking that transaction !fInMempool. Those funds will disappear until confirmation is reached.

2. 
   TheBlueMatt commented at 5:11 PM on December 6, 2017: member

   Hmm, seems like its working around the issue instead of fixing, I think CWalletTx::AcceptToMemoryPool shouldn't unset fInMempool if its already set.

3. 
   MarcoFalke commented at 5:14 PM on December 6, 2017: member

   You could add the test to mempool_persist.py instead, as wallet is already a "test everything that has something to do with sending bitcoin"

4. 
   instagibbs commented at 5:15 PM on December 6, 2017: member

   @TheBlueMatt right, feels quite brittle, but wanted to throw up a working solution/test.

   Knowing whether a boolean is already set sounds like boost::optional(?) @MarcoFalke noted, will move

5. 
   instagibbs commented at 5:17 PM on December 6, 2017: member

   Well, I suppose you mean if it's set true, don't every change the value from that call.

6. 
   TheBlueMatt commented at 5:17 PM on December 6, 2017: member

   @instagibbs huh? No, just change the third-to-last line in wallet.cpp to fInMempool |= ret from fInMempool = ret.

7. 
   instagibbs commented at 5:24 PM on December 6, 2017: member

   @TheBlueMatt I honestly still don't understand the current comment in the logic. As long as this is only being called once on startup, your suggestion seems like a correct fix at least.

8. 
   Sjors commented at 5:26 PM on December 6, 2017: member

   Knowing whether a boolean is already set sounds like boost::optional(?)

   Or std::optional as of C++17 :-)

9. 
   TheBlueMatt commented at 5:29 PM on December 6, 2017: member

   @instagibbs the entered-mempool callback referred to there is async, so you can create a transaction, put it in the mempool, and then the IsInMempool() call will return false for some time. Thus, change outputs will "not be spendable" until the callbacks catch up if we didnt set fInMempool to true in AcceptToMemoryPool.

10. 
    MarcoFalke commented at 5:30 PM on December 6, 2017: member

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA512
    
    Tested ACK f72f5e2576650c3fc314e16f33dbcf24e85520ec
    -----BEGIN PGP SIGNATURE-----
    
    iQIcBAEBCgAGBQJaKCkKAAoJENLqSFDnUoslHSkP/2QJqJa6aKFbp4ripP81rep5
    jB074r5uPkh6Oa4MgbCI4dte9BQNkfIZ8wTl9sfNax50j8TMeglSHubn6P/F50A/
    zo0HJG767dvQMxGd+KSxYAS9nk9Oh4iY6SZs44lVYvOxUjnzlRTl9ElbzmDW+BOY
    deqwbH3xv25Ii9+xrvjzHBt/3jiWmdMDJ6NjqJofCmsrLARY3NHL/IQhAG8sqSPR
    QSu81crg25IshaO55JPOu2EyXMhnv5M3AF3O4kqaxSFZ3CyioXJhMgggs+n/ZZbI
    0hGOxNFHjAznTOFD3/a8/6ritVtpyhUbtPe00k0LcAssTWiqjLtfU2mEpF/HudYV
    HvNdDogxXoXjsQTKjxtKYetm1oe/NrGtRpI207cMw5TL+q2XINRApgCJ48Sb5qVr
    cOsVokhy1VdPS8OGQHGusiEfmUC60nX8thixbKbhf/z76NEFiPDNT4Nyq5qjIs82
    ps2EbX9L66IPCKwP2w25L2EMK3cZaTkox2XNfQS0zKM/reDSzUSlypENA7V/R7dz
    CaE7rCOwllHUu/mr9ANKcug3jeikDkrI7kFDg11Dxr9Pdt97YUrJtblYmL97Nlyp
    NDZihNY1xCfFLUqQXwpxDma+VEcafeTw1R7mZDHWQwg04Vtd+U/EX+6Rw2Cs4TAU
    awhmaenWj7pKls9ql2gW
    =MeI2
    -----END PGP SIGNATURE-----
    

11. 
    Sjors commented at 5:30 PM on December 6, 2017: member

    @TheBlueMatt although unrelated to this bug, isn't it a good idea in general to check !mempool.exists(...) before trying to add a transaction to the mempool? Or perhaps AcceptToMemoryPool should check if already contains the transaction before proceeding?

    Or does that not work due to the concurrency issue you described?

12. 
    MarcoFalke commented at 5:33 PM on December 6, 2017: member

    If you move the fix to CWalletTx::AcceptToMemoryPool, it should check for mempool.exists() instead of the |= logic. Imo this better documents by code what is being done.

13. 
    MarcoFalke commented at 6:03 PM on December 6, 2017: member

    @Sjors Probably posted that to the wrong thread

14. 
    TheBlueMatt commented at 6:08 PM on December 6, 2017: member

    I think |= is more correct than it is now anyway - if a transaction isn't in mempool and fails to be added, we shoul unset fInMempool in the callback, not in CWalletTx::AcceptToMemoryPool.

15. fanquake added the label Wallet on Dec 6, 2017
16. 
    instagibbs commented at 3:55 PM on December 7, 2017: member

    mark as 0.16 milestone? This is a potentially scary bug for users.

17. laanwj added this to the milestone 0.16.0 on Dec 7, 2017
18. 
    Sjors commented at 11:31 AM on December 11, 2017: member

    Merging this would make testing SegWit wallet changes in #11403 easier.

19. don't attempt mempool entry for wallet transactions on startup if already in mempool 6ba8f30e7b
20. add test for unconfirmed balance between restarts 6697a70894
21. instagibbs force-pushed on Dec 11, 2017
22. 
    instagibbs commented at 2:16 PM on December 11, 2017: member

    moved mempool check logic closer to source, moved test to mempool_persist.py

23. in src/wallet/wallet.cpp:4117 in 6697a70894

    4113 | @@ -4114,6 +4114,11 @@ int CMerkleTx::GetBlocksToMaturity() const
    4114 |  
    4115 |  bool CWalletTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& state)
    4116 |  {
    4117 | +    // Quick check to avoid re-setting fInMempool to false
    

    ---

    ---

    

    Sjors commented at 3:03 PM on December 11, 2017:

    And to avoid wasting time?

24. 
    laanwj commented at 3:16 PM on December 11, 2017: member

    utACK 6697a7089441deded836332e7ce3b4a1a9a3cbcd

25. laanwj merged this on Dec 11, 2017
26. laanwj closed this on Dec 11, 2017

    ---

27. laanwj referenced this in commit 8ab6c0b09e on Dec 11, 2017
28. 
    promag commented at 3:36 PM on December 11, 2017: member

    post-utACK.

29. laanwj referenced this in commit 6bb9c13f9a on Feb 14, 2018
30. PastaPastaPasta referenced this in commit 592a3cc439 on Feb 13, 2020
31. PastaPastaPasta referenced this in commit 6f3693b281 on Feb 27, 2020
32. PastaPastaPasta referenced this in commit 8ffcaf7e9f on Mar 14, 2020
33. PastaPastaPasta referenced this in commit 791719ea3d on Mar 19, 2020
34. PastaPastaPasta referenced this in commit 34abb3bc22 on Jun 9, 2020
35. PastaPastaPasta referenced this in commit 0adef962d1 on Jun 9, 2020
36. PastaPastaPasta referenced this in commit 11cdaaacfb on Jun 10, 2020
37. PastaPastaPasta referenced this in commit a4d693f38e on Jun 10, 2020
38. PastaPastaPasta referenced this in commit 7e426dcf0f on Jun 11, 2020
39. PastaPastaPasta referenced this in commit 60830846cf on Jun 11, 2020
40. ckti referenced this in commit 5f6bcf3956 on Mar 28, 2021
41. DrahtBot locked this on Sep 8, 2021

Milestone
0.16.0