refactor: Avoid wallet code writing node settings file #22217

1. 
   ryanofsky commented at 10:09 pm on June 10, 2021: contributor

   Change wallet loading code to access settings through the Chain interface instead of writing settings.json directly. This is for running wallet and node in separate processes, since multiprocess code wouldn’t easily work with different processes updating the same file.

   ---

   This PR is part of the process separation project. The commit was first part of larger PR #10102.

2. Avoid wallet code writing node settings file

   Change wallet loading code to access settings through the Chain
   interface instead of writing settings.json directly.

   49ee2a0ad8
3. DrahtBot added the label interfaces on Jun 10, 2021
4. DrahtBot added the label Refactoring on Jun 10, 2021
5. DrahtBot added the label Utils/log/libs on Jun 10, 2021
6. DrahtBot added the label Wallet on Jun 10, 2021
7. 
   DrahtBot commented at 3:22 am on June 11, 2021: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
   • #17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
   • #17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
   • #16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

8. 
   meshcollider commented at 1:42 am on August 9, 2021: contributor

   Code review ACK 49ee2a0ad88e0e656234b769d806987784ff1e28
9. 
   jamesob commented at 6:25 pm on August 11, 2021: member

   ACK 49ee2a0ad88e0e656234b769d806987784ff1e28 (jamesob/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co)

   Change seems straightforward and benign. Just want to confirm that there’s no need to update the two other calls to updateRwSetting in wallet/wallet.cpp (AddWalletSetting/RemoveWalletSetting).

10. 
    ryanofsky commented at 7:26 pm on August 11, 2021: contributor

    ACK 49ee2a0 (jamesob/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co)

    Thanks!

    Change seems straightforward and benign. Just want to confirm that there’s no need to update the two other calls to updateRwSetting in wallet/wallet.cpp (AddWalletSetting/RemoveWalletSetting).

    That’s right. It’s safe for the wallet to use the Chain interface to update settings file, because the Chain interface methods are callable from the bitcoin-wallet process but execute in the bitcoin-node process. It is not safe for wallet code to use the gArgs methods to read and write json settings, because then the bitcoin-wallet process would be accessing the same settings as the bitcoin-node process and corrupting them or getting them out of sync between the two processes.

11. 
    Zero-1729 commented at 2:01 pm on August 19, 2021: contributor

    crACK 49ee2a0ad88e0e656234b769d806987784ff1e28
12. meshcollider merged this on Aug 22, 2021
13. meshcollider closed this on Aug 22, 2021

    ---

14. sidhujag referenced this in commit cf1ffb4097 on Oct 15, 2021
15. bitcoin locked this on Aug 22, 2022
16. 
    ryanofsky commented at 6:17 pm on September 5, 2024: contributor

    This PR caused an unintended change in behavior. Before this PR, passing -nowallet=0would cause a wallet “1” to be loaded. After this PR, it triggers a vague “JSON value of type bool is not of expected type string” error and uncaught exception. #30684 improves this by avoiding the exception and making the error clearer.

Contributors
ryanofsky DrahtBot meshcollider jamesob Zero-1729

Labels
Refactoring Wallet Utils/log/libs interfaces