Concept ACK. Tested on macOS.

Needs update for -printtoconsole help text.

Strong concept ACK. Very nice usability improvement! I'm also running bitcoind in the foreground mode for the most part.

What about reducing the number of \n:s from 20 to say 5? That should be more than enough to distinguish between to subsequent executions.

Just a few concerns (Mainly on Windows)

1. There's now no way of turning the stdout logging off when launched in foreground. It's either on or (on by default)
2. On Windows output will always be shown now since deamonising isn't possible?. (AFAIK it's not possible to run bitcoind as a windows service?
3. stdout isn't buffered very well (if at all?) so it can potentially have a perf penalty (especially if custom high freq custom logging is enabled)

BTW fPrintToDebugLog is set to true and not changed ever so the code can be simplified.

Concept ACK, cool 👍.

Now that I've thought about this more I don't think the isatty() check is necessary. The situation where it wouldn't be a TTY would be if you were running commands like:

# stdout is a pipe, not a TTY.
bitcoind | less

# stdout is a file, not a TTY.
bitcoind > redundantcopyofdebug.log

I don't think either of these are things people are going to actually run, but if they did they would probably expect to see output printed. The usual use of isatty() is actually to test for features like using console escape sequences (e.g. the ASCII "bell" character) or to tell if it makes sense to print ANSI color codes.

New version (with squashed commit) removes the isatty() check and reduces the number of newlines we print from 20 to 5. I've also updated the PR description to remove the commentary about isatty().

My understanding is now logging will not be sent to the log file unless -daemon is passed via command line. If that's the case won't this be unexpected and annoying behavior?

LogPrintStr only prints to one or the other.

I'm on the phone so it's difficult to check

Not sure this makes sense. I don't know of any other software that guesses stdout vs log based on whether stdout is a tty. It seems likely to break users who do bitcoin-qt &

Why not just put printtoconsole=1 in bitcoin.conf if you prefer it by default?

@lukejr The code no longer uses isatty(), I removed that code. Adding printtoconsole=1 to bitcoin.conf has the side effect that it completely suppresses logging to debug.log, which is annoying if sometimes you run bitcoin as a daemon and sometimes you run it in foreground mode.

We should probably change the LogPrintf() logic to log to both if both flags are set. But even in that case if you add printtoconsole=1 to your bitcoin.conf file you'll get behavior where bitcoind would log all messages twice, with the stdout one going to /dev/null. This is harmless but inefficient.

@donaloconnor I also just noticed that which I think is bad behavior. Updated to make these flags independent (printing to console will not affect logging to debug.log).

utACK 23bc0aa

Tested 23bc0aa on macOS, still works.

Removing the isatty() check makes sense; another example would be using bitcoind | grep the_thing_I_care_about.

@luke-jr bitcoin-qt does not output anything to the console (not sure why).

utACK 23bc0aa modulo @Sjors' nits :-)

utACK 23bc0aa though I do agree with @laanwj that disabling logging would make sense now.

I could not resist the temptation to add -nodebuglog because I found the "node bug log" thing to be so funny. I added it as a "secret" option (meaning it's suppressed by default from -help output). You can also use -debuglogfile=/dev/null and -debuglogfile=0 to disable logging. There are also Python test cases for all three methods. So I believe everyone got what they asked for :-D

Interestingly, before this change -debuglogfile=/dev/null did not work, because FileTruncate() does not check that its argument is a regular file. This is yet-another Unix compatibility thing that I would like Bitcoin to support better! I believe this means that master will fail if you try to log to e.g. a named pipe created with mkfifo(1), which is not common but still an unfortunate bug.

One question I have about this change: I was originally going to update the -debuglogfile help string but realized it would require it to be re-translated. Since developers are probably the only ones who want this option it might just make sense to have a one line changelog note about the change? I'm not sure what the protocol is for this.

I tested and you actually can use a named pipe, because the operation that's failing is a file size check in boost. But you can't use other special files, e.g. /dev/zero which swallows input the same was as /dev/null.

# this is allowed
$ echo foo > /dev/zero

# yep, it worked
$ echo $?
0

# this fails
$ ./src/bitcoind -debuglogfile=/dev/zero


************************
EXCEPTION: N5boost10filesystem16filesystem_errorE       
boost::filesystem::file_size: Operation not permitted: "/dev/zero"       
bitcoin in AppInit()       

Something to fix another day I guess.

use -debuglogfile=/dev/null

Right, what bothered me about /dev/null is that it's OS-specific. I don't think providing arbitrary device files as debug log files (such as /dev/zero, /dev/urandom etc) should be really a goal. It mentions file in the name :) Better to have a dedicated option for it, thanks.

I spent way longer on this than I'd like to admit, due to strange behavior in the Bitcoin option parser.

There's a little-known feature in the option parser where -nofoo is the same as -foo=0 when updating the value map. So you can't actually create an option with a name like -nodebuglog, because looking up the value from the map still uses the nodebuglog value, but the value is actually set in the map as debuglog. I thought this was really neat, because my last update to this diff had special cased -nodebuglog and -debuglogfile=0, so I thought I could just use gArgs.GetBoolArg("-debuglogfile"). That would be really nice because then I could delete a bunch of lines from my last diff and not treat those as special cases.

Nope, doesn't work. I digged in and here's where it gets really weird. The option parser uses atoi() to interpret boolean arguments. The thing is, the return value of atoi() is undefined if you pass it non-integer values, as there's no way to tell from the return value whether an error occurred.

Here's where it gets kind of weird. We had a test in ResendWalletTransactionsTest() that was setting a flag like this:

class ResendWalletTransactionsTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 1
        self.extra_args = [['--walletbroadcast=false']]

So what happens when the option parser sees this... is it calls atoi("false"). That returns 0 like we want, but only because atoi() happens to return false for invalid input... a fact that I want to again emphasize is undefined. On another libc implementation atoi() is allowed to e.g. return an uninitialized value in this situation. This also means that -walletbroadcast=true would disable wallet broadcast, for the same reason.

Since there's a test case in Bitcoin that was using a "false" flag, I think it's possible that there are other people in the wild relying on this behavior. So now the option parser treasts only 0 and false as false, and no other values. If people had configs that were setting values to false, this will still work. I really wanted to add C++ tests for this code, but the methods I wanted to test are defined static in the util.cpp file.**

I also made it so if you use -debuglogfile=/dev/null on a Unix system, it won't crash. But that's just because I fixed ShrinkDebugLogFile(), not because I'm special casing it. Setting -debuglogfile=/dev/null will actually open /dev/null and write data to it.

A lot yak shaving, but it makes the option parser more robust so probably worth it.

** Technically this means that -debuglogfile=false will not write to a file named "false". Of course, -debuglogfile=./false would be perfectly fine. I thought about abusing this to add a Python test case for the option parsing by testing those two flavors in this test, but it felt too weird.

Actually it looks like I can add C++ test for the option parser, I'm going to put up another commit adding that.

atoi is well-defined for all null-terminated input strings.

The strtol, strtoll, strtoul, and strtoull functions return the converted
value, if any. If no conversion could be performed, zero is returned.

Interesting, that's not documented in the man pages on my system, but cppreference.com agrees that it must return 0 in such a case.

utACK - looks good

utACK 60086ad871cb1a756c152a3d3e19fcea52660123. I think that commit can be squashed with the previous.

Needs rebase to get rid of the already-merged diff.

Marked "up for grabs"

./contrib/devtools/clang-format-diff.py:74:1: F401 'string' imported but unused