When a developer is examining debug.log after something goes wrong, it's often useful to know the exact options the failing instance of bitcoind was started with. Sometimes the debug.log file is all that's available for the analysis. This PR logs the bitcoin.conf entries and command-line arguments to debug.log on startup.
On bitcoind startup, write config args to debug.log #16115
pull LarryRuane wants to merge 1 commits into bitcoin:master from LarryRuane:args-to-debug-log changing 6 files +113 −6-
LarryRuane commented at 4:37 AM on May 29, 2019: contributor
-
DrahtBot commented at 6:20 AM on May 29, 2019: member
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #17812 (config, test: asmap functional tests and feature refinements by jonatack)
- #17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
- #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)
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.
-
jonasschnelli commented at 6:57 AM on May 29, 2019: contributor
Knowing the startup parameters for debug purposes is certainly helpful. Unsure about logging rpcpassword (maybe add a blacklist?).
- DrahtBot added the label Utils/log/libs on May 29, 2019
-
laanwj commented at 8:56 AM on May 29, 2019: member
Knowing the startup parameters for debug purposes is certainly helpful. Unsure about logging rpcpassword (maybe add a blacklist?).
Yepp. Agree this is very useful for troubleshooting, but be careful with logging sensitive information (people do post these things to the internet sometimes).
-
in src/util/system.cpp:969 in 8bf605fc56 outdated
963 | @@ -964,6 +964,20 @@ std::string ArgsManager::GetChainName() const 964 | return CBaseChainParams::MAIN; 965 | } 966 | 967 | +void ArgsManager::LogArgs() const 968 | +{ 969 | + for (const auto arg: m_config_args) {
practicalswift commented at 8:47 AM on May 30, 2019:Accessing
m_config_argsrequires holdingcs_args.Also: use const reference to avoid copying. The const reference comment applies also for the
forloops below.in src/util/system.cpp:974 in 8bf605fc56 outdated
969 | + for (const auto arg: m_config_args) { 970 | + for (const auto value: arg.second) { 971 | + LogPrintf("config-file arg: %s=%s\n", arg.first, value); 972 | + } 973 | + } 974 | + for (const auto arg: m_override_args) {
practicalswift commented at 8:48 AM on May 30, 2019:And accessing
m_override_argsrequires holdingcs_args:-)LarryRuane force-pushed on May 30, 2019LarryRuane commented at 3:39 PM on May 30, 2019: contributorThank all of you for the good comments! Force-pushed (since this PR is so small) to address all review comments. I added a blacklist, but please verify that it contains the correct items. I tested this manually,
bitcoin.confcontains:debug=1 debug=xxx torpassword=hellotorand then I ran
bitcoind --debug=cmdline111 --rpcuser=hellouserand the following appeared indebug.log:2019-05-30T15:31:29Z config-file arg: -debug=1 2019-05-30T15:31:29Z config-file arg: -debug=xxx 2019-05-30T15:31:29Z config-file arg: -torpassword=**** 2019-05-30T15:31:29Z command-line arg: -debug=cmdline111 2019-05-30T15:31:29Z command-line arg: -rpcuser=**** 2019-05-30T15:31:29Z command-line arg: -server=1in src/util/system.cpp:967 in 2cfbac279b outdated
963 | @@ -964,6 +964,31 @@ std::string ArgsManager::GetChainName() const 964 | return CBaseChainParams::MAIN; 965 | } 966 | 967 | +void ArgsManager::LogArgs() const
fanquake commented at 3:51 PM on May 30, 2019:@ryanofsky You might have some thoughts on this?
ryanofsky commented at 8:55 PM on July 15, 2019:@ryanofsky You might have some thoughts on this?
I don't object to this change, but it does seems a little unusual to write the config file to the debug log. Especially when you have to mask passwords and such.
For debugging purposes, my preferred alternative would be to have a
-printconfigflag (similar to existing-helpor-versionflags) that would print the current settings in ini format. This could be more convenient for debugging because you'd be able to parse the config and get instant feedback without having to bring the daemon up and down. I was also thinking about this as a way of implementing A.J's suggestion from #15935 (comment) to help debug dynamic settings ("viewing/exporting the current dynamic settings [...], so you can paste them into your bitcoin.conf")kristapsk commented at 11:00 PM on June 11, 2019: contributorConcept ACK, but found two issues.
2019-06-11T22:57:43Z config-file arg: -regtest.rpcpassword=123456abcdef 2019-06-11T22:57:43Z config-file arg: -regtest.rpcuser=bitcoinrpc 2019-06-11T22:57:43Z config-file arg: -rpcpassword=**** 2019-06-11T22:57:43Z config-file arg: -rpcuser=**** 2019-06-11T22:57:43Z config-file arg: -server=1 2019-06-11T22:57:43Z config-file arg: -test.bind=127.0.0.1 2019-06-11T22:57:43Z config-file arg: -test.prune=1000 2019-06-11T22:57:43Z command-line arg: -testnet=- Blacklisted args does not seem to work for regtest (I assume the same problem is for testnet).
- Probably there isn't need for a "=" part for no value arguments (or use "=1"?).
dongcarl commented at 2:53 PM on June 12, 2019: memberNot 100% sure if this is possible or in scope, but it'd be very useful to have the effective command line configuration at-a-glance, especially when trying to debug for others from looking at their logs.
promag commented at 3:01 PM on June 12, 2019: member@dongcarl that's a bigger change. Basically all default values are defined when the arg is used, so what you want would require to duplicate all of that or move the defaults to one place elsewhere. I don't think debug log should have all args but maybe we could add an option to control that.
In #15493 I propose to add a RPC to also see effective arg values.
LarryRuane force-pushed on Jun 14, 2019LarryRuane commented at 12:25 AM on June 14, 2019: contributor@kristapsk, I force-pushed a fix for both of your suggestions, thanks! I decided to leave the
=off if there are no value arguments, because I didn't want to assume that having no values would always be the same as=1.LarryRuane commented at 5:59 AM on June 15, 2019: contributorI just noticed this PR is incomplete. To provide a little background for those not very familiar with the logging code: The client shrinks
debug.logupon startup if it's larger than a moderate size (11 MB), preserving the most recent 10 MB. This happens only at startup, so ifbitcoindruns for a long time, and happens to do a lot of logging, the log file can become enormous.External log rotation tools can be configured to prevent the log file from growing unbounded, see #917. Typically the way these tools work is, they first move (as in, the
renamesystem call)debug.logto a different name. Because the client has an open file handle to this file, it continues to log to it even though it's been moved. Then the log rotation tool sends the client aSIGHUPsignal to cause it to close its file descriptor and reopendebug.log. Now it's actually creating a newdebug.logfile, and future logging goes there. No data are lost during this switch. The log rotation tool can now do whatever it wants with the renamed (old) log file. It may compress it, offload it, or, either immediately or after some time, delete it.If it deletes it, the configuration information that this PR causes to be logged during startup no longer exists anywhere.
The latest commit, https://github.com/bitcoin/bitcoin/pull/16115/commits/df891038ccce1ea15d2cabcd6717acf60ca9e9ee, re-logs the configuration information when
debug.logis reopened. This guarantees thatdebug.logalways contains the configuration messages. My first idea was to re-log only the configuration file and command line arguments (that are being written by the first part of this PR). But there's a lot of other useful stuff that gets logged at just after startup. Here's what appears indebug.logduring my testing, after it's reopened (this does not appear on the console):2019-06-15T05:38:03Z Logging: start relogging initial messages 2019-06-15T05:37:09Z Bitcoin Core version v0.18.99.0-df891038c (release build) 2019-06-15T05:37:09Z Assuming ancestors of block 0000000000000037a8cd3e06cd5edbfe9dd1dbcc5dacab279376ef7cfc2b4c75 have valid signatures. 2019-06-15T05:37:09Z Setting nMinimumChainWork=00000000000000000000000000000000000000000000007dbe94253893cbd463 2019-06-15T05:37:09Z Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation 2019-06-15T05:37:09Z Using RdSeed as additional entropy source 2019-06-15T05:37:09Z Using RdRand as an additional entropy source 2019-06-15T05:37:09Z Default data directory /home/larry/.bitcoin 2019-06-15T05:37:09Z Using data directory /home/larry/.bitcoin/testnet3 2019-06-15T05:37:09Z Config file: /home/larry/.bitcoin/bitcoin.conf 2019-06-15T05:37:09Z config-file arg: -testnet=1 2019-06-15T05:37:09Z command-line arg: -server=1 2019-06-15T05:37:09Z Using at most 125 automatic connections (1024 file descriptors available) 2019-06-15T05:37:09Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements 2019-06-15T05:37:10Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements 2019-06-15T05:37:10Z Using 4 threads for script verification 2019-06-15T05:37:10Z No wallet support compiled in! 2019-06-15T05:37:10Z scheduler thread start 2019-06-15T05:37:10Z HTTP: creating work queue of depth 16 2019-06-15T05:37:10Z No rpcpassword set - using random cookie authentication. 2019-06-15T05:37:10Z Generated RPC authentication cookie /home/larry/.bitcoin/testnet3/.cookie 2019-06-15T05:37:10Z HTTP: starting 4 worker threads 2019-06-15T05:37:10Z init message: Loading banlist... 2019-06-15T05:37:10Z Cache configuration: 2019-06-15T05:37:10Z * Using 2.0 MiB for block index database 2019-06-15T05:37:10Z * Using 8.0 MiB for chain state database 2019-06-15T05:37:10Z * Using 440.0 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space) 2019-06-15T05:38:03Z Logging: end relogging initial messages 2019-06-15T05:38:03Z UpdateTip: new best=0000000000000046799df07296169b65f1a203c13b22ffe6df812454ca1e6e45 height=1382445 version=0x20000000 log2_work=71.151478 tx=27610552 date='2018-08-09T15:30:58Z' progress=0.622489 cache=1.6MiB(12574txo) 2019-06-15T05:38:03Z UpdateTip: new best=000000000000007cdf5e7a5a62f116cad1bb1ba492df4c8aef6b2f9a56075d9f height=1382446 version=0x20000000 log2_work=71.151519 tx=27611112 date='2018-08-09T15:31:47Z' progress=0.622494 cache=1.6MiB(12585txo) [.....]It's great to know the exact version number, that this client doesn't include a wallet, and other configuration things. Note the two "extra" lines that begin with
Logging:to show the start and end of this re-logging. The timestamps of the re-logged messages are the original timestamps from when the client actually started (in this case only about a minute earlier); out-of-order timestamps are a little odd, but on the other hand it's nice to know when the client actually started. After theLogging: end relogging initial messagesline, the logging continues where it left off before the file reopen (I was doing reindex at the time).DrahtBot added the label Needs rebase on Jun 18, 2019LarryRuane force-pushed on Jun 19, 2019DrahtBot removed the label Needs rebase on Jun 19, 2019DrahtBot added the label Needs rebase on Aug 2, 2019LarryRuane force-pushed on Aug 3, 2019DrahtBot removed the label Needs rebase on Aug 3, 2019in src/util/system.cpp:982 in 44f444dcb9 outdated
977 | + LogPrintf("%s %s\n", prefix, arg.first); 978 | + continue; 979 | + } 980 | + std::string v = value; 981 | + for (const auto& b : blacklist) { 982 | + if (arg.first.find(b) != std::string::npos) {
jkczyz commented at 6:54 PM on August 12, 2019:This may inadvertently match an arg that contains a blacklisted arg name as a substring (but nonetheless should not be matched). Consider stripping any ignorable prefix from the arg and matching exactly instead. An added benefit is you can use a set for the blacklist as you had before.
jkczyz commented at 5:57 PM on August 14, 2019:During the PR review club session, it came up that a flag could be added for sensitive args.
https://github.com/bitcoin/bitcoin/blob/a7aa809027633556dd3280c6e29ca98eb3235a3d/src/util/system.h#L138 https://github.com/bitcoin/bitcoin/blob/a7aa809027633556dd3280c6e29ca98eb3235a3d/src/util/system.cpp#L542
Then rather than maintaining a blacklist, the flag could be checked before logging.
LarryRuane commented at 2:59 AM on August 16, 2019:@jkczyz, thanks, adding a flag is an excellent idea, implemented in b6e55d566a92427f1af9775d676ab629eaacbe41. Does that also address your concern about matching a substring? It seems to me it does, since we're not doing any string or pattern matching at all.
in src/util/system.h:302 in 44f444dcb9 outdated
296 | @@ -297,6 +297,12 @@ class ArgsManager 297 | * Return ArgsManager::NONE for unknown arg. 298 | */ 299 | unsigned int FlagsOfKnownArg(const std::string& key) const; 300 | + 301 | + /** 302 | + * During startup, LogPrintf() the config file options and
jkczyz commented at 9:56 PM on August 12, 2019:A couple nits:
- Drop "During startup" as this is referring to the call site.
- s/LogPrintf()/Log/ to avoid leaking implementation details in the documentation.
These will help keep the documentation in sync with the code as it changes.
LarryRuane commented at 3:00 AM on August 16, 2019:Good suggestion, implemented in latest commit, b6e55d566a92427f1af9775d676ab629eaacbe41.
in test/functional/feature_config_args.py:88 in 371278f727 outdated
81 | @@ -82,10 +82,23 @@ def test_log_buffer(self): 82 | self.start_node(0, extra_args=['-noconnect=0']) 83 | self.stop_node(0) 84 | 85 | + def test_args_log(self): 86 | + with self.nodes[0].assert_debug_log(expected_msgs=[ 87 | + 'command-line arg: -addnode=some.node', 88 | + 'command-line arg: -torpassword=****',
jkczyz commented at 10:44 PM on August 12, 2019:Could you add any test cases for redacted args beginning with an ignorable prefix (e.g
-regtest.)?
jkczyz commented at 5:59 PM on August 14, 2019:Also during the PR review club, it was suggested to update
ArgsManager's unit tests rather than using a functional test.https://github.com/bitcoin/bitcoin/blob/master/src/test/getarg_tests.cpp
jonatack commented at 6:57 PM on August 14, 2019:In addition to the other comments e.g. testing of edge cases and using unit tests where possible, tests should also cover each of the blacklisted terms and not just one of them.
jkczyz changes_requestedjkczyz commented at 11:21 PM on August 12, 2019: contributorExternal log rotation tools can be configured to prevent the log file from growing unbounded, see #917. Typically the way these tools work is, they first move (as in, the
renamesystem call)debug.logto a different name. Because the client has an open file handle to this file, it continues to log to it even though it's been moved. Then the log rotation tool sends the client aSIGHUPsignal to cause it to close its file descriptor and reopendebug.log. Now it's actually creating a newdebug.logfile, and future logging goes there. No data are lost during this switch. The log rotation tool can now do whatever it wants with the renamed (old) log file. It may compress it, offload it, or, after some time, delete it.If it deletes it, the configuration information that this PR causes to be logged during startup no longer exists anywhere.
The latest commit, df89103, re-logs the configuration information when
debug.logis reopened. This guarantees thatdebug.logalways contains the configuration messages. My first idea was to re-log only the configuration file and command line arguments (that are being written by the first part of this PR). But there's a lot of other useful stuff that gets logged at just after startup. Here's what appears indebug.logduring my testing, after it's reopened (this does not appear on the console):There's quite a bit of complexity added to the code to handle this case (e.g. the additional logic in
BCLog::Logger::LogPrintStrand the factm_relog_saveis modified across different functions).In practice, how often does this situation arise and where the missing information would have been helpful? Would it be reasonable for users to configure their log rotation tools to preserve the first log file instead?
fjahr commented at 5:38 PM on August 14, 2019: memberThe commit message for the test is incorrect: it is a functional test, not a unit test.
jkczyz changes_requestedin src/logging.cpp:247 in 371278f727 outdated
245 | m_started_new_line = !str.empty() && str[str.size()-1] == '\n'; 246 | 247 | + // While the system is initializing, save some reasonable number 248 | + // of (configuration-related) log messages for re-logging when 249 | + // the log file rotates (gets reopened). 250 | + if (m_relog_save && m_msgs_relog.size() < 200) {
narula commented at 6:05 PM on August 14, 2019:200 seems arbitrary
jnewbery commented at 6:07 PM on August 14, 2019: memberI think this PR should be split in two. The two commits on startup, write config options to debug.log and re-log configuration when debug.log is reopened both address logging, but are essentially independent.
I'm a concept ACK for the first commit (on startup, write config options to debug.log) and would like to see it merged, but a weak concept NACK on the second (re-log configuration when debug.log is reopened) since I think it adds unnecessary complexity to the bitcoind code for something that should really be handled externally.
narula commented at 6:10 PM on August 14, 2019: contributorI think it would be better for saving startup debug information to be handled outside of bitcoind. That way there's less confusion about what should/shouldn't get relogged, less code complexity, and users who save all their logs files will not be logging things redundantly.
in src/util/system.cpp:967 in 371278f727 outdated
958 | @@ -959,6 +959,43 @@ std::string ArgsManager::GetChainName() const 959 | return CBaseChainParams::MAIN; 960 | } 961 | 962 | +static void logArgs( 963 | + const std::string& prefix, 964 | + const std::map<std::string, std::vector<std::string>>& args) 965 | +{ 966 | + // don't log passwords and other sensitive values 967 | + static const std::string blacklist[] = {
jonatack commented at 7:02 PM on August 14, 2019:Short of refactoring this PR to use a flag as suggested elsewhere and during the PR Review Club (log here);
- a whitelist would be safer than a blacklist
- there needs to be some mechanism - code docs and perhaps also in the developer notes docs - to help ensure this list is updated on future arg changes
LarryRuane commented at 2:58 AM on August 16, 2019:@jonatack, great suggestion, modified to use a flag in b6e55d566a92427f1af9775d676ab629eaacbe41. (I also agree that a whitelist would be better than a blacklist.) I will investigate to find a good place to document this. (I'm new here, so any suggestions would be appreciated.)
jonatack commented at 7:05 PM on August 14, 2019: memberConcept ACK with reserves regarding the approach and implementation so the changes can be robust, maintainable, safe and adequately covered by tests.
LarryRuane force-pushed on Aug 16, 2019LarryRuane commented at 3:11 AM on August 16, 2019: contributorThanks for the review comments; they're very helpful. The latest commit, b6e55d566a92427f1af9775d676ab629eaacbe41, addresses some of the comments (I'll address the others soon). This was a force-push because of rebase with master.
in test/functional/feature_config_args.py:112 in 99931694ed outdated
97 | + ]): 98 | self.start_node(0, extra_args=[ 99 | '-addnode=some.node', 100 | - '-torpassword=mypass', 101 | + '-torpassword=torpassword', 102 | ])
LarryRuane commented at 7:39 PM on August 16, 2019:This test was not verifying that arguments that should not be logged to
debug.logare indeed not logged. I wanted to add test the full list of what should not be logged (torpassword, rpcauth, rpcbind, rpcpassword, rpcuser), but specifying any of those excepttorpasswordon the command line causes the test framework to fail ("Error: no RPC connection), so I couldn't think of a way to test that the logging of those is suppressed. I think this test is still useful, however, because it's pretty easy to see ininit.cppthattorpasswordis treated the same as the others (namely, that they all includeArgsManager::DONT_LOG).
jonatack commented at 2:54 PM on January 12, 2020:Thanks @LarryRuane for taking the suggestion to cover all the
DONT_LOGcases and your change to pass only the values to unexpected_messages makes it even better. The only thing I regret is having suggested "satoshi" as therpcuserbecause it pollutes git grepping; a name that isn't already used by the codebase like "charlie" or "abc123", etc., would have been better.LarryRuane force-pushed on Aug 17, 2019LarryRuane commented at 5:13 PM on August 17, 2019: contributorThanks for all the review feedback! The most recent force-push removes the second part of this PR (the extra logging that happens when the log rotates). I agree with @narula's and others' opinions that this can be handled outside of bitcoind. I will probably submit a separate PR for relogging the config information on log rotation.
A good way for the log rotation tools to work is to either save all the log files (which may not be practical), or, when rotating the log file, check to see if the file being rotated out includes the start of the currently-running
bitcoindinstance (it could search for the stringBitcoin Core version). If found, keep the file, or at least that section of the file. Then remember to send that part to a developer or support person requestingdebug.log. (It's that "remember" part that makes me wonder if relogging on log rotation might still be a good idea.)LarryRuane commented at 2:18 AM on August 19, 2019: contributorAn additional thought related to this part of my most recent comment:
Then remember to send that part to a developer or support person requesting
debug.logHere's a better procedure that the log rotation tool can follow: After moving
debug.logto a new name, it can create a newdebug.logfile and write the initial (configuration-related) messages to it. Then send the SIGHUP signal. The next log message will be appended todebug.log-- after the initial messages. (I verified this behavior.)LarryRuane force-pushed on Aug 19, 2019DrahtBot added the label Needs rebase on Aug 21, 2019LarryRuane force-pushed on Aug 21, 2019DrahtBot removed the label Needs rebase on Aug 21, 2019LarryRuane force-pushed on Aug 21, 2019in src/util/system.cpp:979 in 10ff7f645e outdated
974 | + if (FlagsOfKnownArg(arg.first) & DONT_LOG) 975 | + continue; 976 | + for (auto& value : arg.second) { 977 | + if (value.empty()) { 978 | + LogPrintf("%s %s\n", prefix, arg.first); 979 | + continue;
jkczyz commented at 7:37 PM on August 22, 2019:Rather than use
continue, it would be more natural to use anelseclause.
LarryRuane commented at 2:59 PM on August 23, 2019:Good suggestion, thanks, fixed 7f4640338de05fce1cf59bc567b6674a94931c23.
in src/util/system.cpp:975 in 10ff7f645e outdated
970 | + const std::string& prefix, 971 | + const std::map<std::string, std::vector<std::string>>& args) const 972 | +{ 973 | + for (const auto& arg : args) { 974 | + if (FlagsOfKnownArg(arg.first) & DONT_LOG) 975 | + continue;
jkczyz commented at 7:47 PM on August 22, 2019:Don't you still want to log that the arg was set only with the value redacted?
LarryRuane commented at 3:05 PM on August 23, 2019:What do you think? Somehow I changed my mind and thought it might be better to not show it at all. But it is more informative to show that the argument was specified. I can easily change it back.
LarryRuane commented at 1:00 PM on August 25, 2019:@jkczyz, after more thought, I agree your idea is better, 676eb23315cd23c84bbfe5bc4c41652b1f6d8bb5.
jkczyz commented at 8:46 PM on August 22, 2019: contributor@jkczyz, thanks, adding a flag is an excellent idea, implemented in b6e55d5. Does that also address your concern about matching a substring? It seems to me it does, since we're not doing any string or pattern matching at all.
Yeah, looks like
FlagsOfKnownArghandles this for you.in test/functional/test_framework/test_node.py:303 in f67dc7bbbd outdated
306 | @@ -307,7 +307,9 @@ def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT): 307 | wait_until(self.is_node_stopped, timeout=timeout) 308 | 309 | @contextlib.contextmanager 310 | - def assert_debug_log(self, expected_msgs, unexpected_msgs=[], timeout=2): 311 | + def assert_debug_log(self, expected_msgs, unexpected_msgs=None, timeout=2): 312 | + if unexpected_msgs is None: 313 | + unexpected_msgs = []
LarryRuane commented at 3:21 AM on August 26, 2019:@practicalswift found this bug while reviewing #16673 but the bug originated in the current PR (which #16673 is based on). Fixed here, will rebase #16673 to pick up the fix there.
in src/util/system.cpp:890 in f67dc7bbbd outdated
972 | +{ 973 | + for (const auto& arg : args) { 974 | + for (auto& value : arg.second) { 975 | + if (value.empty()) { 976 | + LogPrintf("%s %s\n", prefix, arg.first); 977 | + } else {
jkczyz commented at 3:39 PM on August 26, 2019:You can reduce one level of nesting by making this an else-if.
DrahtBot added the label Needs rebase on Sep 5, 2019LarryRuane force-pushed on Sep 5, 2019DrahtBot removed the label Needs rebase on Sep 5, 2019laanwj added the label Feature on Sep 30, 2019ysangkok commented at 7:51 PM on November 9, 2019: noneShould I wait for a rebase to test this, or is it better if I test it as it is? Or should I try to rebase myself?
LarryRuane force-pushed on Nov 18, 2019LarryRuane force-pushed on Nov 18, 2019LarryRuane force-pushed on Nov 19, 2019LarryRuane commented at 6:25 AM on November 19, 2019: contributor@ysangkok, I integrated with the conflicting recent change (7f40528cd50fc43ac0bd3e785de24d661adddb7a) using a separate commit (https://github.com/bitcoin/bitcoin/pull/16115/commits/07f11f171c82bcbd9d79272c8f87002e39a7521b), force-pushed rebase. Now would be a good time to test, thanks.
LarryRuane commented at 6:47 AM on November 19, 2019: contributorTo give you a little better understanding of what this PR does, here's part of the
debug.logfile after running one of the functional (python) tests with the-nocleanupoption. After the most recent rebase, the config file section string gets printed out (within square brackets):2019-11-18T22:47:48.371866Z [init] Default data directory /home/larry/.bitcoin 2019-11-18T22:47:48.371882Z [init] Using data directory /tmp/bitcoin_func_test_z99aclji/node0/regtest 2019-11-18T22:47:48.371901Z [init] Config file: /tmp/bitcoin_func_test_z99aclji/node0/bitcoin.conf 2019-11-18T22:47:48.371922Z [init] config-file arg: regtest=1 2019-11-18T22:47:48.371937Z [init] config-file arg: [regtest] bind=127.0.0.1 2019-11-18T22:47:48.371950Z [init] config-file arg: [regtest] discover=0 2019-11-18T22:47:48.371963Z [init] config-file arg: [regtest] dnsseed=0 2019-11-18T22:47:48.371976Z [init] config-file arg: [regtest] fallbackfee=0.0002 2019-11-18T22:47:48.371990Z [init] config-file arg: [regtest] keypool=1 2019-11-18T22:47:48.372002Z [init] config-file arg: [regtest] listenonion=0 2019-11-18T22:47:48.372015Z [init] config-file arg: [regtest] port=14243 2019-11-18T22:47:48.372031Z [init] config-file arg: [regtest] printtoconsole=0 2019-11-18T22:47:48.372049Z [init] config-file arg: [regtest] rpcport=19243 2019-11-18T22:47:48.372072Z [init] config-file arg: [regtest] server=1 2019-11-18T22:47:48.372088Z [init] config-file arg: [regtest] shrinkdebugfile=0 2019-11-18T22:47:48.372106Z [init] config-file arg: [regtest] upnp=0 2019-11-18T22:47:48.372119Z [init] command-line arg: datadir=/tmp/bitcoin_func_test_z99aclji/node0 2019-11-18T22:47:48.372135Z [init] command-line arg: debug 2019-11-18T22:47:48.372150Z [init] command-line arg: debugexclude=libevent 2019-11-18T22:47:48.372165Z [init] command-line arg: debugexclude=leveldb 2019-11-18T22:47:48.372179Z [init] command-line arg: logthreadnames 2019-11-18T22:47:48.372194Z [init] command-line arg: logtimemicros 2019-11-18T22:47:48.372209Z [init] command-line arg: uacomment=testnode0 2019-11-18T22:47:48.372223Z [init] command-line arg: wallet=0 2019-11-18T22:47:48.372236Z [init] Using at most 125 automatic connections (1024 file descriptors available)It actually could be useful to have this information available when trying to debug or analyze a functional test or the code being tested (although you can figure it out if you study the code enough), because some of these values differ across the tests.
LarryRuane force-pushed on Nov 21, 2019in src/util/system.cpp:879 in d95fdc78c9 outdated
891 | + if (valueStr.size()) { 892 | + LogPrintf("%s %s%s=%s\n", prefix, sectionStr, arg.first, valueStr); 893 | + } else { 894 | + LogPrintf("%s %s%s\n", prefix, sectionStr, arg.first); 895 | + } 896 | + }
ryanofsky commented at 4:50 PM on November 25, 2019:In commit "merge with 'Deduplicate settings' commit" (d95fdc78c965708866156898588221a8d983396f)
Would suggest:
if (FlagsOfKnownArg(arg.first) & DONT_LOG) { LogPrintf("%s %s%s=****\n", prefix, sectionStr, arg.first); } else { LogPrintf("%s %s%s=%s\n", prefix, sectionStr, arg.first, value.write()); }This is simpler than formatting code in the PR, more future proof, and also less ambiguous about negated options.
Would also suggest squashing commits.
LarryRuane commented at 5:10 PM on November 26, 2019:@ryanofsky, thanks, I like that much better. I implemented your suggestion (with a small difference, only a single call to
LogPrintf()), rebased, squashed, force-pushed (eb973ff916f88ed36b59ae6466ec16dd30811215). The formatting is slightly different now, but I like that the code is much simpler and the output is more consistent (this is from one of the functional tests):2019-11-26T16:09:54.048744Z [init] config-file arg: regtest="1" 2019-11-26T16:09:54.048755Z [init] config-file arg: [regtest] bind="127.0.0.1" 2019-11-26T16:09:54.048765Z [init] config-file arg: [regtest] discover="0" 2019-11-26T16:09:54.048774Z [init] config-file arg: [regtest] dnsseed="0" 2019-11-26T16:09:54.048784Z [init] config-file arg: [regtest] fallbackfee="0.0002" 2019-11-26T16:09:54.048793Z [init] config-file arg: [regtest] keypool="1" 2019-11-26T16:09:54.048802Z [init] config-file arg: [regtest] listenonion="0" 2019-11-26T16:09:54.048812Z [init] config-file arg: [regtest] port="11068" 2019-11-26T16:09:54.048822Z [init] config-file arg: [regtest] printtoconsole="0" 2019-11-26T16:09:54.048833Z [init] config-file arg: [regtest] rpcport="16068" 2019-11-26T16:09:54.048843Z [init] config-file arg: [regtest] server="1" 2019-11-26T16:09:54.048854Z [init] config-file arg: [regtest] shrinkdebugfile="0" 2019-11-26T16:09:54.048865Z [init] config-file arg: [regtest] upnp="0" 2019-11-26T16:09:54.048874Z [init] command-line arg: datadir="/tmp/bitcoin_func_test_wmar_kd1/node0" 2019-11-26T16:09:54.048884Z [init] command-line arg: debug="" 2019-11-26T16:09:54.048895Z [init] command-line arg: debugexclude="libevent" 2019-11-26T16:09:54.048904Z [init] command-line arg: debugexclude="leveldb" 2019-11-26T16:09:54.048914Z [init] command-line arg: logthreadnames="" 2019-11-26T16:09:54.048924Z [init] command-line arg: logtimemicros="" 2019-11-26T16:09:54.048934Z [init] command-line arg: uacomment="testnode0"I like the way negative arguments are displayed; as you say, they are less ambiguous. There are no examples of such from above, but I ran
src/bitcoind -noreindexand the following appears indebug.log:2019-11-26T17:00:24Z command-line arg: reindex=falseI added a unit test for this too. Thank you for taking the time to review!
LarryRuane force-pushed on Nov 26, 2019LarryRuane force-pushed on Nov 26, 2019in src/util/system.cpp:882 in eb973ff916 outdated
873 | @@ -874,6 +874,29 @@ std::string ArgsManager::GetChainName() const 874 | return GetArg("-chain", CBaseChainParams::MAIN); 875 | } 876 | 877 | +void ArgsManager::logArgsPrefix( 878 | + const std::string& prefix, 879 | + const std::string& section, 880 | + const std::map<std::string, std::vector<util::SettingsValue>>& args) const 881 | +{ 882 | + std::string sectionStr = section.empty() ? "" : "[" + section + "] ";
ryanofsky commented at 5:31 PM on November 26, 2019:In commit "on startup, write config options to debug.log" (eb973ff916f88ed36b59ae6466ec16dd30811215)
sectionStris an older convention for variable names. Newer code is usingsection_strhttps://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-cryanofsky approvedryanofsky commented at 9:01 PM on November 26, 2019: memberCode review ACK eb973ff916f88ed36b59ae6466ec16dd30811215. Looks good!
I don't know how to write an automated test for these changes, suggestions are welcome.
Could remove this line from the PR description since there are tests now.
LarryRuane force-pushed on Nov 27, 2019LarryRuane commented at 4:14 PM on November 27, 2019: contributorThanks, @ryanofsky, fixed variable names (in two places), rebased, force-pushed. Also, I changed the PR description as suggested, and simplified it (since it goes into the commit history), no need to show sample output there, which was out of date anyway.
ryanofsky approvedryanofsky commented at 5:47 PM on November 27, 2019: memberCode review ACK 85f44c083d75b71bce149eff4ecdc9b543dd7dfa. Just variable renames and updated PR description since last review
DrahtBot added the label Needs rebase on Dec 13, 2019LarryRuane force-pushed on Dec 13, 2019DrahtBot removed the label Needs rebase on Dec 13, 2019ryanofsky approvedryanofsky commented at 8:02 PM on December 16, 2019: memberCode review ACK 6d195a8da92b60161669e56de6cbb4a5baafdf84. No changes since last review other than rebase due to conflict from adjacent rpcwhitelist line
DrahtBot added the label Needs rebase on Dec 19, 2019jnewbery commented at 9:16 PM on December 19, 2019: memberDefinite concept ACK. This needs a rebase, but I've read the current code and it looks good. I'll re-review once this has been rebased on master.
LarryRuane force-pushed on Dec 20, 2019LarryRuane commented at 7:10 AM on December 20, 2019: contributorRebased onto master, force-pushed 3028a27
DrahtBot removed the label Needs rebase on Dec 20, 2019in src/util/system.cpp:887 in 3028a27c5f outdated
882 | + 883 | +void ArgsManager::LogArgs() const 884 | +{ 885 | + LOCK(cs_args); 886 | + for (const auto& section : m_settings.ro_config) { 887 | + logArgsPrefix("config-file arg:", section.first, section.second);
jnewbery commented at 4:38 PM on December 20, 2019:nit: s/config-file/Config file/ (config file is not usually hyphenated)
in src/util/system.cpp:889 in 3028a27c5f outdated
884 | +{ 885 | + LOCK(cs_args); 886 | + for (const auto& section : m_settings.ro_config) { 887 | + logArgsPrefix("config-file arg:", section.first, section.second); 888 | + } 889 | + logArgsPrefix("command-line arg:", "", m_settings.command_line_options);
jnewbery commented at 4:40 PM on December 20, 2019:s/command-line/Command-line/ (first letter of logs are usually capitalized)
LarryRuane commented at 10:04 PM on December 20, 2019:Thanks, done, separate commit 717099eef1cc5670180a58d0fec6d027a1e2f95d. I'll squash after review (or, at least before merge).
hebasto commented at 10:26 PM on December 20, 2019:I'll squash after review (or, at least before merge)
It is better to squash before review as a merge commit should contain ACKs with the latest commit hash.
jnewbery commented at 4:41 PM on December 20, 2019: memberTested ACK 3028a27c5fde8e341cccb743b31a02c1bd7d934d
I've left a couple of small style nit. Feel free to ignore.
jnewbery commented at 11:15 PM on December 20, 2019: memberACK 717099eef1cc5670180a58d0fec6d027a1e2f95d
LarryRuane force-pushed on Dec 21, 2019LarryRuane commented at 3:22 PM on December 21, 2019: contributorRebased onto master, squashed, force-pushed.
LarryRuane force-pushed on Dec 22, 2019jnewbery commented at 3:04 PM on December 24, 2019: memberCode review ACK 4a88ddac84235398a0ea8c041baee7192a6b4d09
Rebased onto master ...
I don't think this needed rebasing on master. It's better not to rebase on master unless necessary, since it generates additional work for reviewers
LarryRuane commented at 7:56 PM on December 24, 2019: contributorI don't think this needed rebasing on master. It's better not to rebase on master unless necessary, since it generates additional work for reviewers
Absolutely right, sorry about that; I actually thought of that just after I did that. I'll avoid unnecessary rebases from now on. Thanks for the new ACK.
hebasto commented at 8:58 AM on December 25, 2019: memberConcept ACK. Will test.
hebasto approvedhebasto commented at 10:27 AM on December 25, 2019: memberACK 4a88ddac84235398a0ea8c041baee7192a6b4d09, I have tested the code on Linux Mint 19.3, including:
- no
bitcoin.conf - duplicated option names in
bitcoin.confand in the command line
Every time it works as expected.
ryanofsky approvedryanofsky commented at 10:05 PM on January 8, 2020: memberCode review ACK 4a88ddac84235398a0ea8c041baee7192a6b4d09. Just rebase and spelling/capitalization tweaks since last review
jnewbery commented at 3:37 AM on January 9, 2020: member@ryanofsky : current branch is 4a88dda
ryanofsky commented at 5:58 PM on January 9, 2020: member@ryanofsky : current branch is 4a88dda
Thanks fixed comment. That was the hash I meant
jonatack commented at 7:02 PM on January 9, 2020: memberWill re-review and test tomorrow.
in test/functional/test_framework/test_node.py:303 in 4a88ddac84 outdated
297 | @@ -298,7 +298,9 @@ def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT): 298 | wait_until(self.is_node_stopped, timeout=timeout) 299 | 300 | @contextlib.contextmanager 301 | - def assert_debug_log(self, expected_msgs, timeout=2): 302 | + def assert_debug_log(self, expected_msgs, unexpected_msgs=None, timeout=2): 303 | + if unexpected_msgs is None: 304 | + unexpected_msgs = []
jonatack commented at 8:46 PM on January 10, 2020:Can simplify?
- def assert_debug_log(self, expected_msgs, unexpected_msgs=None, timeout=2): - if unexpected_msgs is None: - unexpected_msgs = [] + def assert_debug_log(self, expected_msgs, unexpected_msgs=[], timeout=2):
jnewbery commented at 10:13 PM on January 10, 2020:It's generally best to use
Noneas the default argument instead of a mutable default arg: https://docs.python-guide.org/writing/gotchas/#what-you-should-do-instead
in test/functional/feature_config_args.py:86 in 4a88ddac84 outdated
82 | @@ -83,10 +83,28 @@ def test_log_buffer(self): 83 | self.start_node(0, extra_args=['-noconnect=0']) 84 | self.stop_node(0) 85 | 86 | + def test_args_log(self):
jonatack commented at 8:48 PM on January 10, 2020:Could use a docstring or logging here, e.g.
+ self.log.info('Test config args logging')That said, the whole file is lacking logging, not just this test, so feel free to ignore.
jonatack commented at 9:17 PM on January 10, 2020: memberTested ACK 4a88ddac84235398a0ea8c041baee7192a6b4d09 modulo comments below.
in test/functional/feature_config_args.py:99 in 4a88ddac84 outdated
94 | + unexpected_msgs=[ 95 | + 'torpassword=torpassword', 96 | + ]): 97 | + self.start_node(0, extra_args=[ 98 | + '-addnode=some.node', 99 | + '-torpassword=torpassword',
jonatack commented at 9:22 PM on January 10, 2020:This test can be improved with (tested change):
expected_msgs=[ 'Command-line arg: addnode="some.node"', + 'Command-line arg: rpcauth=****', + 'Command-line arg: rpcbind=****', + 'Command-line arg: rpcpassword=****', + 'Command-line arg: rpcuser=****', 'Command-line arg: torpassword=****', 'Config file arg: regtest="1"', 'Config file arg: [regtest] server="1"', ], unexpected_msgs=[ + 'rpcauth=alice:f7efda5c189b999524f151318c0c86$d5b51b3beffbc0', + 'rpcbind=127.1.1.1', + 'rpcuser=satoshi', 'torpassword=torpassword', ]): self.start_node(0, extra_args=[ '-addnode=some.node', + '-rpcauth=alice:f7efda5c189b999524f151318c0c86$d5b51b3beffbc0', + '-rpcbind=127.1.1.1', + '-rpcpassword=', + '-rpcuser=satoshi', '-torpassword=torpassword',jonatack commented at 9:25 PM on January 10, 2020: member- My main comment was not saved by GitHub in my review so I am re-adding it below.
- I tested this PR and the suggested changes after rebasing to current master at e7f84503571c171a7e6728cd2d77dd4103bd7a6f.
LarryRuane force-pushed on Jan 11, 2020LarryRuane commented at 4:39 PM on January 11, 2020: contributor@jonatack, thanks for the suggestions, force-pushed (without rebasing onto master) both changes. I made one other change that I hope you agree is an improvement: I removed the keywords from the list of unexpected messages, for example, changed
rpcuser=satoshito justsatoshi. The reason is, if there was a weird bug by which the log contained (let's say)ruser=satoshiorrpcuser = satoshi(note the spaces around=) or many other variants, this test would still pass. What we really care about is not leaking the values of these settings (the stringsatoshi) to the log file, so those values are what we should make sure aren't present in the log file.hebasto commented at 4:44 PM on January 11, 2020: member... force-pushed (without rebasing onto master)...
Needless rebasing makes reviewing of consequent changes harder ;) Thank you.
hebasto approvedhebasto commented at 4:54 PM on January 11, 2020: memberACK 8c09bf9508b12e3593e4f6d23ed4dcfdabda7f2b, the only change in
feature_config_args.pytest, tested locally on Linux Mint 19.3.in src/test/getarg_tests.cpp:200 in 8c09bf9508 outdated
194 | + 195 | + // Log the arguments to debug.log 196 | + gArgs.LogArgs(); 197 | + 198 | + // Open and read debug.log. 199 | + FILE* file = fsbridge::fopen(LogInstance().m_file_path, "rb");
jonatack commented at 2:49 PM on January 12, 2020:Should
#include <fs.h>be explicitly added to this file? Other code and unit tests usingfsbridgeseem to include it, and according to developer-notes.md:- Every `.cpp` and `.h` file should `#include` every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers. - *Rationale*: Excluding headers because they are already indirectly included results in compilation failures when those indirect dependencies change. Furthermore, it obscures what the real code dependencies are.
jonatack commented at 2:57 PM on January 12, 2020:Other than that, the new unit tests look good and provide helpful feedback when run with
src/test/test_bitcoin -t getarg_tests/logargs -l all
laanwj commented at 3:31 PM on January 15, 2020:I think we could test this without reading an actual file by registering a log callback with
PushBackCallback(andDeleteCallbackafterwards).
MarcoFalke commented at 3:46 PM on January 15, 2020:I think there is also an
ASSERT_DEBUG_LOGwhich might come in handyjonatack commented at 2:59 PM on January 12, 2020: memberACK 8c09bf9508b12e3593e4f6d23ed4dcfdabda7f2b Two comments, feel free to ignore or handle them in a follow-up.
in src/init.cpp:1234 in 8c09bf9508 outdated
1217 | @@ -1218,6 +1218,7 @@ bool AppInitMain(NodeContext& node) 1218 | // Not categorizing as "Warning" because it's the default behavior 1219 | LogPrintf("Config file: %s (not found, skipping)\n", config_file_path.string()); 1220 | } 1221 | + gArgs.LogArgs();
jonatack commented at 3:05 PM on January 12, 2020:nit: this is commented in the unit test but not here; perhaps add:
+ + // Log the config arguments to debug.log gArgs.LogArgs();
hebasto commented at 8:49 AM on January 13, 2020:It seems this comment does not add any value here, no?
jonatack commented at 11:03 AM on January 13, 2020:I think it may gain time for occasional contributors/readers, and perhaps for those who haven't reviewed this PR, by confirming what the laconic
gArgs.LogArgs();is doing exactly (which args, logged where) without the need to git grep. At least, more help than harm.LarryRuane force-pushed on Jan 13, 2020LarryRuane commented at 4:07 PM on January 13, 2020: contributorThanks, I force-pushed the suggested changes.
ryanofsky approvedryanofsky commented at 7:27 PM on January 13, 2020: memberCode review ACK 8317bb35b63062e0d54de3af19d923456e1cb467. Just new comments, include, test cases since last review
jonatack commented at 7:30 PM on January 13, 2020: memberTested ACK 8317bb35b63062e0d54de3af19d923456e1cb467
git diff 8c09bf9..8317bb3since last push:diff --git a/src/init.cpp b/src/init.cpp index 04305c9b23..6a91c8c1ce 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1218,6 +1218,8 @@ bool AppInitMain(NodeContext& node) // Not categorizing as "Warning" because it's the default behavior LogPrintf("Config file: %s (not found, skipping)\n", config_file_path.string()); } + + // Log the config arguments to debug.log gArgs.LogArgs(); LogPrintf("Using at most %i automatic connections (%i file descriptors available)\n", nMaxConnections, nFD); diff --git a/src/test/getarg_tests.cpp b/src/test/getarg_tests.cpp index 090c544be3..bebc56c57b 100644 --- a/src/test/getarg_tests.cpp +++ b/src/test/getarg_tests.cpp @@ -2,6 +2,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include <fs.h> #include <util/strencodings.h> #include <util/system.h> #include <test/util/setup_common.h> diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index de8461ea5a..ec6268f5fb 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -99,16 +99,16 @@ class ConfArgsTest(BitcoinTestFramework): unexpected_msgs=[ 'alice:f7efda5c189b999524f151318c0c86$d5b51b3beffbc0', '127.1.1.1', - 'satoshi', - 'tpass', + 'secret-rpcuser', + 'secret-torpassword', ]): self.start_node(0, extra_args=[ '-addnode=some.node', '-rpcauth=alice:f7efda5c189b999524f151318c0c86$d5b51b3beffbc0', '-rpcbind=127.1.1.1', '-rpcpassword=', - '-rpcuser=satoshi', - '-torpassword=tpass', + '-rpcuser=secret-rpcuser', + '-torpassword=secret-torpassword', ]) self.stop_node(0)Launching bitcoind with this PR adds the following lines to the debug log that correctly list all of the custom settings, re-sorted in alphabetical order (first the config file settings, followed by the command line args) between
Config file:andUsing at most 512 automatic connections:2020-01-13T19:14:02Z Config file arg: addresstype="bech32" 2020-01-13T19:14:02Z Config file arg: blocksonly="0" 2020-01-13T19:14:02Z Config file arg: dbcache="8192" 2020-01-13T19:14:02Z Config file arg: logips="1" 2020-01-13T19:14:02Z Config file arg: maxconnections="512" 2020-01-13T19:14:02Z Config file arg: maxuploadtarget="0" 2020-01-13T19:14:02Z Config file arg: par="0" 2020-01-13T19:14:02Z Config file arg: rpcpassword=**** 2020-01-13T19:14:02Z Config file arg: rpcuser=**** 2020-01-13T19:14:02Z Config file arg: server="1" 2020-01-13T19:14:02Z Config file arg: txindex="1" 2020-01-13T19:14:02Z Config file arg: walletbroadcast="1" 2020-01-13T19:14:02Z Command-line arg: dbcache="256" 2020-01-13T19:14:02Z Command-line arg: logips="0" 2020-01-13T19:14:02Z Command-line arg: walletbroadcast="0"jnewbery commented at 8:00 PM on January 13, 2020: memberACK 8317bb35b63062e0d54de3af19d923456e1cb467
hebasto approvedhebasto commented at 9:29 AM on January 14, 2020: memberre-ACK 8317bb35b63062e0d54de3af19d923456e1cb467
in src/util/system.h:149 in 8317bb35b6 outdated
144 | @@ -145,6 +145,8 @@ class ArgsManager 145 | * between mainnet and regtest/testnet won't cause problems due to these 146 | * parameters by accident. */ 147 | NETWORK_ONLY = 0x200, 148 | + // This argument's value is sensitive (such as a password). 149 | + DONT_LOG = 0x400,
MarcoFalke commented at 3:49 PM on January 15, 2020:I'd prefer to call this
SENSITIVEorPRIVATEor similar to avoid a negated option (which might lead to double negations), but more importantly make it agnostic of how the values are reported. If in the future someone adds agetconfigargsRPC, the internal wording "log" seems a bit off.LarryRuane force-pushed on Jan 19, 2020LarryRuane force-pushed on Jan 19, 2020LarryRuane commented at 1:54 AM on January 19, 2020: contributorImplemented review suggestions:
- rename
DONT_LOGtoSENSITIVE - in the unit test, verify log file contents without opening and reading
debug.log
Here are the changes in the last force-push (I think this PR should remain a single commit), in case that's helpful to reviewers: https://github.com/LarryRuane/bitcoin/compare/8317bb35b63062e0d54de3af19d923456e1cb467..03fdf86b6d4bc036c707a0b0d84995f438283b11
LarryRuane force-pushed on Jan 20, 2020jnewbery commented at 7:50 PM on January 20, 2020: memberACK 03fdf86b6d4bc036c707a0b0d84995f438283b11
DrahtBot added the label Needs rebase on Jan 29, 2020on startup, write config options to debug.log b951b0973cLarryRuane force-pushed on Jan 29, 2020DrahtBot removed the label Needs rebase on Jan 29, 2020jonatack commented at 9:13 PM on January 30, 2020: memberACK b951b0973c reviewed diff, re-code review, built, ran tests, launched bitcoind and reviewed debug log output, verified value of
strdebug log in the added unit test.$ src/test/test_bitcoin -t getarg_tests/logargs -l test_suite .../... 2020-01-30T20:48:38Z Command-line arg: dontlog=**** 2020-01-30T20:48:38Z Command-line arg: okaylog="public" 2020-01-30T20:48:38Z Command-line arg: okaylog-bool="" 2020-01-30T20:48:38Z Command-line arg: okaylog-negbool=false$ bitcoind -dbcache=512 -logips=0 -walletbroadcast=0 2020-01-30T19:01:24Z Bitcoin Core version v0.19.99.0-b951b0973c (debug build) 2020-01-30T19:01:24Z Assuming ancestors of block 00000000000000000005f8920febd3925f8272a6a71237563d78c2edfdd09ddf have valid signatures. 2020-01-30T19:01:24Z Setting nMinimumChainWork=000000000000000000000000000000000000000008ea3cf107ae0dec57f03fe8 2020-01-30T19:01:24Z Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation 2020-01-30T19:01:24Z Using RdSeed as additional entropy source 2020-01-30T19:01:24Z Using RdRand as an additional entropy source 2020-01-30T19:01:24Z Default data directory /home/jon/.bitcoin 2020-01-30T19:01:24Z Using data directory /home/jon/.bitcoin 2020-01-30T19:01:24Z Config file: /home/jon/.bitcoin/bitcoin.conf 2020-01-30T19:01:24Z Config file arg: addresstype="bech32" 2020-01-30T19:01:24Z Config file arg: blocksonly="0" 2020-01-30T19:01:24Z Config file arg: dbcache="10240" 2020-01-30T19:01:24Z Config file arg: logips="1" 2020-01-30T19:01:24Z Config file arg: maxconnections="512" 2020-01-30T19:01:24Z Config file arg: maxuploadtarget="0" 2020-01-30T19:01:24Z Config file arg: par="0" 2020-01-30T19:01:24Z Config file arg: rpcpassword=**** 2020-01-30T19:01:24Z Config file arg: rpcuser=**** 2020-01-30T19:01:24Z Config file arg: server="1" 2020-01-30T19:01:24Z Config file arg: txindex="1" 2020-01-30T19:01:24Z Config file arg: walletbroadcast="1" 2020-01-30T19:01:24Z Command-line arg: dbcache="512" 2020-01-30T19:01:24Z Command-line arg: logips="0" 2020-01-30T19:01:24Z Command-line arg: walletbroadcast="0" 2020-01-30T19:01:24Z Using at most 512 automatic connections (1024 file descriptors available)MarcoFalke commented at 10:10 PM on January 30, 2020: memberACK b951b0973cfd4e0db4607a00d434a04afb0d6199 🐪
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 ACK b951b0973cfd4e0db4607a00d434a04afb0d6199 🐪 -----BEGIN PGP SIGNATURE----- iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p pUgHewwAixdou/20sx/EQar5cdubgAQpvKEAXE9fZwjP6zu0ZBrabem2sc8gsVkD bvQPvnTscdaIvHkOzkZ860STB+nr77x483AKc+Dl4G4Z4LecMv8iXSQmoVOwlxl2 rsdUVFQnLAUB9phu9RccJwXUyEsoaGAy/sUcoaHwv3nuApqgNZZptjmCUO3lK5sw vQ/XT30gVZ6xSlfP21ZQaMaqps4MHOB9Z1X5hjnxm49/XMg+4Et3TJKbGyTNwcm6 Ue/raLdaPtq6htfkhMIro9cc95Bfy1Nr9X4fGpte7IAu4waT5bJQVF2EeoQPyh0d LEOHe5cQPRsUOd5RvzYmCFqGsM5qRZw9BrQ6yUG13c7tYOi4xc7tdCabE8BUgFuR NqAw90SZMFl8wdDsQHub0H0V5EDxo92YzukM1J+h40OgG3ozZc3dS7SbPhqNpnoN uul5igOr4HICcNDPDHufDW4WAzvh8PpUlqCQnzF/BKnbYQjGZvhtB7asR4v0lH3w LtVy1Blz =hrjv -----END PGP SIGNATURE-----Timestamp of file with hash
7113306c6076473032eb8637f29927398a935b895b4ee90467fca953dfb9ba72 -</details>
MarcoFalke referenced this in commit 1d1f8bbf57 on Jan 30, 2020MarcoFalke merged this on Jan 30, 2020MarcoFalke closed this on Jan 30, 2020LarryRuane deleted the branch on Jan 31, 2020LarryRuane referenced this in commit f4213afab3 on Mar 20, 2020deadalnix referenced this in commit d116c4e9a2 on May 2, 2020DrahtBot locked this on Feb 15, 2022
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 21:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me