wallet: Tidy CWallet::SetUsedDestinationState #17354

1. 
   promag commented at 4:23 pm on November 2, 2019: member

   This PR makes 2 distinct changes around CWallet::SetUsedDestinationState:

   • 1st the recursive lock is removed and now it requires the lock to be held;
   • 2nd change is to support, in the best case, just a wallet database flush when transaction is added to the wallet.
2. wallet: Avoid recursive lock in CWallet::SetUsedDestinationState 01f45dd00e
3. fanquake added the label Wallet on Nov 2, 2019
4. 
   promag commented at 4:25 pm on November 2, 2019: member

   I’ve noticed that SetUsedDestinationState is not called with used=false so at the moment half of the function is unused code - is someone planning to change this or should we drop the unused code?
5. 
   promag commented at 4:35 pm on November 2, 2019: member

   Maybe we should add WalletBatch& argument to all wallet “mutation” methods?
6. promag force-pushed on Nov 2, 2019
7. wallet: Reuse existing batch in CWallet::SetUsedDestinationState 0b75a7f068
8. promag force-pushed on Nov 2, 2019
9. 
   promag commented at 8:49 am on November 7, 2019: member

   @achow101 @meshcollider friendly review request.
10. 
    ryanofsky commented at 11:58 am on November 7, 2019: member

    Maybe we should add WalletBatch& argument to all wallet “mutation” methods?

    I’d like this convention since it would make it clearer which methods could hit the database. I’d like it even more if it was extended to methods that read and didn’t just mutate the database and was extended to KeyMan and SigningProvider classes so the ugly encrypted_batch hack could go away. (WalletBatch could be a forward declared type so SigningProvider wouldn’t depend on wallet code, or SigningProvider methods could just pass a generic context argument.)

11. ryanofsky approved
12. 
    ryanofsky commented at 12:06 pm on November 7, 2019: member

    Code review ACK 0b75a7f0680d16a41043864a897470324917b1e8. Code changes looks fine but PR description should be updated to say what benefits of the change are. I might have missed something, but I didn’t see a place where multiple batches were used previously and a single batch was used now. So the main benefit of this change appears to be removing a recursive lock? And maybe moving toward a consistent convention for passing batch instances?
13. 
    promag commented at 12:13 pm on November 7, 2019: member

    @ryanofsky original goal was to remove the recursive lock. Note that now SetUsedDestinationState uses the existing batch in AddToWallet - resulting in just one flush per wallet transaction (IIUC).

    Thanks for the review!

    Updated OP.

14. 
    ryanofsky commented at 12:20 pm on November 7, 2019: member

    @ryanofsky original goal was to remove the recursive lock. Note that now SetUsedDestinationState uses the existing batch in AddToWallet - resulting in just one flush per wallet transaction (IIUC).

    Good to mention the motivation and brag about improvements like this in the PR description. It helps to understand the change and motivate reviews

15. 
    promag commented at 12:24 pm on November 7, 2019: member

    And maybe moving toward a consistent convention for passing batch instances?

    I understand that you support this, if others also then I can follow up - just add 👍 here.

16. 
    achow101 commented at 11:26 pm on November 7, 2019: member

    ACK 0b75a7f0680d16a41043864a897470324917b1e8
17. 
    MarcoFalke commented at 11:53 pm on November 7, 2019: member

    ACK 0b75a7f0680d16a41043864a897470324917b1e8
18. fanquake referenced this in commit 4a3b6f47cd on Nov 8, 2019
19. fanquake merged this on Nov 8, 2019
20. fanquake closed this on Nov 8, 2019

    ---

21. promag deleted the branch on Nov 8, 2019
22. sidhujag referenced this in commit 30bd7efd72 on Nov 9, 2019
23. jasonbcox referenced this in commit 0b16c6b948 on Sep 22, 2020
24. sidhujag referenced this in commit 5647cc9904 on Nov 10, 2020
25. DrahtBot locked this on Dec 16, 2021

Contributors
promag ryanofsky achow101 MarcoFalke

Labels
Wallet