system: cache config file path before potentially updating datadir #27303

Closes #19990

Fixes an issue where a bitcoin.conf file that contains a datadir= setting would be incorrectly reported in the logs (see examples below). This fix is implemented by caching the path to the conf file before setting datadir.

Master

Default data directory /Users/matthewzipkin/Library/Application Support/Bitcoin
Using data directory /tmp/bcdata/regtest
Config file: /tmp/bcdata/bitcoin.conf (not found, skipping)
Config file arg: blocksdir="/tmp/blocks"
Config file arg: datadir="/tmp/bcdata"
Command-line arg: regtest=""

Branch

Default data directory /Users/matthewzipkin/Library/Application Support/Bitcoin
Using data directory /tmp/bcdata/regtest
Config file: /Users/matthewzipkin/Library/Application Support/Bitcoin/bitcoin.conf
Config file arg: blocksdir="/tmp/blocks"
Config file arg: datadir="/tmp/bcdata"
Config file arg: prune="10000"
Command-line arg: regtest=""

See also #27246 and https://github.com/bitcoin/bitcoin/pull/27302

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #27273 (system: allow GUI to initialize default data dir by pinheadmz)

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.

Code review ACK 8d6114aeaa7cfc747eae068116da04af86ed8e92. I think this is the probably the simplest possible bugfix for the incorrect log print, but it also adds unnecessary code complexity and fragile code without meaningful test coverage, so I do not think it is the best approach.

Here are the specific problems with 8d6114aeaa7cfc747eae068116da04af86ed8e92:

• The new python test doesn't actually check for the bug being fixed, and passes on existing code. The bug can't be tested unless bitcoind is started without a -conf or -datadir argument, and this is not possible without test setup code similar to what is added in #27302. So it would be good for this bugfix to build on #27302 and include a real test.

• The semantics of GetConfigFilePath are even more complicated than before and unnecessarily confusing. It would be better if it simply returned configuration path read by ReadConfigFiles, and couldn't be called until ReadConfigFiles was called, and didn't have confusing behavior of returning different paths at different times and conditions.

• There are minor implementation details that make this change unappealing. Config file path is saved at end of ReadConfigFiles, not the beginning, so it is tied to the later behavior of that function. The m_cached_ prefix is misleading because unlike the other m_cached_ variables the value is not being cached as a performance improvement, but saved for correctness. Adding a ClearPathCache() call would undo the bugfix and cause the wrong path to be returned again. Also the python test is reading the debug file directly instead of using assert_debug_log, and it is using an assert which can be skipped instead of a mandatory assert function.

After seeing the drawbacks of this approach, IMO it would be better to build on test setup code in #27302 so the bugfix can actually be tested. It would also be better simplify code instead of making it more complicated, for example by unifying the GetConfigFilePath and GetConfigFile functions and saving the config path in exactly one place at the top of ReadConfigFiles.

closing for #27302