system: cache config file path before potentially updating datadir

pinheadmz commented at 7:05 pm on March 22, 2023: member

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

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

Branch

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

system: cache config file path before potentially updating datadir 8d6114aeaa

DrahtBot commented at 7:05 pm on March 22, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky

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

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.

fanquake requested review from ryanofsky on Mar 23, 2023

pinheadmz marked this as ready for review on Mar 27, 2023

ryanofsky commented at 3:23 pm on March 27, 2023: contributor

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.

DrahtBot removed review request from ryanofsky on Mar 27, 2023

ryanofsky referenced this in commit ac9fee615a on Mar 27, 2023

pinheadmz commented at 5:58 pm on March 27, 2023: member

closing for #27302

pinheadmz closed this on Mar 27, 2023

ryanofsky referenced this in commit d31816eb5a on Apr 5, 2023

ryanofsky referenced this in commit 1df16e6321 on Apr 5, 2023

ryanofsky referenced this in commit 641b66c1fa on Apr 5, 2023

ryanofsky referenced this in commit 8a2abe2401 on Apr 21, 2023

ryanofsky referenced this in commit 505ae12ef7 on Apr 21, 2023

ryanofsky referenced this in commit 0319de5cbe on Apr 25, 2023

ryanofsky referenced this in commit eefe56967b on May 23, 2023

luke-jr referenced this in commit b64ca49659 on Aug 16, 2023

janus referenced this in commit 46d072df02 on Sep 3, 2023

bitcoin locked this on Mar 26, 2024

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

Master

Branch

Reviews

Conflicts