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.

ACK 49ee2a0 (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).

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.

crACK 49ee2a0

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.