Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Mar 22, 2023 •
edited
Loading

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 #27302

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 22, 2023 •
edited
Loading

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.

@DrahtBot DrahtBot mentioned this pull request Mar 23, 2023
Closed
@fanquake fanquake requested a review from ryanofsky March 23, 2023 11:36
@ryanofsky ryanofsky mentioned this pull request Mar 24, 2023
Merged
@pinheadmz pinheadmz mentioned this pull request Mar 27, 2023
Closed
@pinheadmz pinheadmz marked this pull request as ready for review March 27, 2023 14:52
@ryanofsky
Copy link
Contributor

ryanofsky commented Mar 27, 2023 •
edited
Loading

Code review ACK 8d6114a. 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 8d6114a:

  • 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 DrahtBot removed the request for review from ryanofsky March 27, 2023 15:23
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Mar 27, 2023
@ryanofsky @pinheadmz
bugfix: Fix incorrect debug.log config file path
ac9fee6 
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.

The bad log print was reported and originally fixed in
bitcoin#27303 by
Matthew Zipkin <pinheadmz@gmail.com>

This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
@pinheadmz
Copy link
Member Author

closing for #27302

@pinheadmz pinheadmz closed this Mar 27, 2023
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 5, 2023
@ryanofsky @pinheadmz
bugfix: Fix incorrect debug.log config file path
d31816e 
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.

The bad log print was reported and originally fixed in
bitcoin#27303 by
Matthew Zipkin <pinheadmz@gmail.com>

This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 5, 2023
@ryanofsky @pinheadmz
bugfix: Fix incorrect debug.log config file path
1df16e6 
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.

The bad log print was reported and originally fixed in
bitcoin#27303 by
Matthew Zipkin <pinheadmz@gmail.com>

This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 5, 2023
@ryanofsky @pinheadmz
bugfix: Fix incorrect debug.log config file path
641b66c 
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.

The bad log print was reported and originally fixed in
bitcoin#27303 by
Matthew Zipkin <pinheadmz@gmail.com>

This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 21, 2023
@ryanofsky @pinheadmz
bugfix: Fix incorrect debug.log config file path
8a2abe2 
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.

The bad log print was reported and originally fixed in
bitcoin#27303 by
Matthew Zipkin <pinheadmz@gmail.com>

This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 21, 2023
@ryanofsky @pinheadmz
bugfix: Fix incorrect debug.log config file path
505ae12 
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.

The bad log print was reported and originally fixed in
bitcoin#27303 by
Matthew Zipkin <pinheadmz@gmail.com>

This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 25, 2023
@ryanofsky @pinheadmz
bugfix: Fix incorrect debug.log config file path
0319de5 
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.

The bad log print was reported and originally fixed in
bitcoin#27303 by
Matthew Zipkin <pinheadmz@gmail.com>

This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 23, 2023
@ryanofsky @pinheadmz
bugfix: Fix incorrect debug.log config file path
eefe569 
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.

The bad log print was reported and originally fixed in
bitcoin#27303 by
Matthew Zipkin <pinheadmz@gmail.com>

This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023
@ryanofsky @luke-jr
bugfix: Fix incorrect debug.log config file path
b64ca49 
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.

The bad log print was reported and originally fixed in
bitcoin#27303 by
Matthew Zipkin <pinheadmz@gmail.com>

This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>

Github-Pull: bitcoin#27302
Rebased-From: eefe569
janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Sep 3, 2023
@ryanofsky @pinheadmz @janus
bugfix: Fix incorrect debug.log config file path
46d072d 
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.

The bad log print was reported and originally fixed in
bitcoin/bitcoin#27303 by
Matthew Zipkin <pinheadmz@gmail.com>

This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
@bitcoin bitcoin locked and limited conversation to collaborators Mar 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Relative paths named in the -conf parameter reset when parsing datadir in named config
3 participants
@pinheadmz @DrahtBot @ryanofsky