Rotate debug.log file

LarryRuane commented at 3:17 pm on June 27, 2021: contributor

EDIT: Change the default behavior to be that log rotation is not enabled.

This PR will mitigate a disk-fill DoS attack (although such an attack is not known to exist today).

If log rotation is enabled, then when the debug.log file reaches a certain size (default, 10 MB), rename it to a similar name that includes the current time, and reset (truncate) debug.log. Keep a limited number of rotated files. This provides a sliding window of debug messages so that disk space isn’t consumed without limit.

This is similar to Unix logrotate(8) but requires no external tools and complex configuration, and works on Windows (which doesn’t have logrotate).

LarryRuane commented at 3:19 pm on June 27, 2021: contributor

This is an alternative to #21603. That PR’s motivation also applies to this PR:

A disk fill attack is an attack where an untrusted party (such as a peer) is able to cheaply make your node log to disk excessively. The excessive logging may fill your disk and thus make your node crash either cleanly (best case: if disk fill rate is relatively slow) or uncleanly (worst case: if disk fill rate is relatively fast).

An advantage I see with this approach is that it addresses the concern with log messages being dropped. With this approach, log messages are never dropped; they just don’t go back an infinite amount of time :).

Another advantage that could prove very useful is that debug log categories can be enabled for an indefinite amount of time (#21603 sensibly does not limit debug logging). It’s quite possible that a bug occurs rarely and at an unpredictable time. By the time the bug occurs, it’s too late to enable the relevant debug log category. Also, it might be nice to not have to worry so much about adding more logging in general (without even having to condition it on a category). There may be useful stuff to log that we don’t currently, because of this disk space concern.

Some background information: Currently, debug.log grows unbounded until bitcoind restarts. During startup, if debug.log is greater than 11 MB, it’s truncated to 10 MB (keeping only the most recent log messages). This PR will make this unnecessary.

The rotated log files are kept in a separate directory, to avoid cluttering up the data directory (see discussion below). This directory has the fixed name debug-rotated and is located in the same directory as the debug log file. That is, if you move the debug log file to a different location (using -debuglogfile=path), then debug-rotated also moves to the same directory as the debug log file. (This way they will be on the same file system, allowing renames.)

An easy way to manually test this without having to add artificial logging is to enable log rotation by adding these lines to bitcoin.conf (or specify on the command line):

0debuglogrotatekeep=10
1debugloglimit=1

Then start bitcoind -debug (enable all debug categories), and observe the debug.log file and the rotated files:

0$ cd .bitcoin
1$ ls -l debug.log
2-rw------- 1 larry larry     87513 Jun 27 09:11 debug.log
3$ ls -l debug-rotated
4-rw------- 1 larry larry   1000076 Jun 27 09:03 2021-06-27T15:03:56Z
5-rw------- 1 larry larry   1000190 Jun 27 09:05 2021-06-27T15:05:28Z
6-rw------- 1 larry larry   1000002 Jun 27 09:07 2021-06-27T15:07:09Z
7-rw------- 1 larry larry   1000042 Jun 27 09:07 2021-06-27T15:07:56Z
8-rw------- 1 larry larry   1000045 Jun 27 09:10 2021-06-27T15:10:55Z

There’s no harm in manually removing these rotated log files, even if bitcoind is running. So you could set up some kind of external agent that compresses the rotated files, or ships them off somewhere else, or whatever. (To prevent bitcoind from ever deleting rotated log files, set debuglogrotatekeep=9999999 or something like that.)

Please comment or review: @jnewbery @practicalswift @dergoegge

in src/logging.cpp:365 in 91d796a21a outdated

350+                fs::path rotate{m_file_path};
351+                rotate.remove_filename();
352+                rotate /= "debug-" +  now + ".log";
353+                try {
354+                    fs::rename(m_file_path, rotate);
355+                } catch (const fs::filesystem_error&) {};

LarryRuane commented at 3:26 pm on June 27, 2021:

We’re ignoring any error from rename, not sure that’s right.

in src/logging.cpp:334 in 91d796a21a outdated

332+        };
333+
334+        // reopen the log file, if requested
335+        if (m_reopen_file) {
336+            m_reopen_file = false;
337+            reopen_file();

LarryRuane commented at 3:36 pm on June 27, 2021:

If logrotate is being used, sending a SIGHUP will continue to cause debug.log to be reopened (this PR doesn’t change that behavior), but if the user wants to continue using that mechanism (logrotate(8)), they should disable this PR’s log rotation (by setting -debuglogrotatekeep=0 in the configuration). But if they forget (definitely need release notes on this), I think it will be okay, because either this PR’s log rotation will always happen (since it waits only until debug.log reaches 1 MB by default), or the logrotate(8) will happen (if the file size threshold is less than 1 MB). It would be confusing if both were happening! Maybe more thought is needed here.

An alternative might be to just remove this behavior (SIGHUP causing debug.log to be reopened), but then logrotate(8) will no longer work, and there may be existing users that are all set up to use logrotate(8).

DrahtBot commented at 4:40 pm on June 27, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23203 ([POC] build: static musl libc based bitcoind (with LTO) by fanquake)
#22937 (refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method by ryanofsky)
#21603 (log: Mitigate disk filling attacks by rate limiting LogPrintf by dergoegge)
#20744 (Use std::filesystem. Remove Boost Filesystem & System by fanquake)
#15719 (Wallet passive startup 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.

LarryRuane force-pushed on Jun 27, 2021

LarryRuane force-pushed on Jun 28, 2021

fanquake added the label Utils/log/libs on Jun 28, 2021

dergoegge commented at 10:11 am on June 28, 2021: member

I think this is an interesting approach and i like the concept of log rotation but this is a bigger behavior change than #21603 and i am not sure if always dropping (old) logs should be the default behavior.

An advantage I see with this approach is that it addresses the concern with log messages being dropped. With this approach, log messages are never dropped; they just don’t go back an infinite amount of time :).

Continuously dropping logs seems like an aggressive change to me because why drop the logs when there is no attack? I haven’t had problems with logfile sizes in normal node operation. (maybe thats just me)

Some background information: Currently, debug.log grows unbounded until bitcoind restarts. During startup, if debug.log is greater than 11 MB, it’s truncated to 10 MB (keeping only the most recent log messages). This PR will make this unnecessary.

I think that the current behavior is actually quite useful because in case of an issue (e.g.: a crash) we have the entire log of the nodes lifespan.

This also seems to opens a similar attack vector as the global rate limiting did, an attacker (that knows of two bugs one of which is a disk filling bug) can execute an attack and then use the disk filling bug to rotate the logs out that had info about the attack.

in src/logging.cpp:103 in b526922724 outdated

85@@ -53,6 +86,17 @@ bool BCLog::Logger::StartLogging()
86         if (!m_fileout) {
87             return false;
88         }
89+        // Special files (e.g. device nodes) may not have a size.
90+        try {
91+            m_file_size = fs::file_size(m_file_path);
92+        } catch (const fs::filesystem_error&) {
93+            // We can't do log rotation if it's not a regular file.
94+            m_rotate_keep = 0;

luke-jr commented at 6:11 pm on June 29, 2021:

This should probably error if the user has configured rotation explicitly.

in src/logging.cpp:353 in b526922724 outdated

330         }
331         FileWriteStr(str_prefixed, m_fileout);
332+        m_file_size += str_prefixed.size();
333+
334+        // If debug.log is large, rotate it (rename and recreate).
335+        if (m_rotate_keep > 0 && m_file_size > m_file_limit * 1e6) {

luke-jr commented at 6:12 pm on June 29, 2021:

What if I want to rotate, but never delete?

LarryRuane commented at 1:50 am on July 6, 2021:

What if I want to rotate, but never delete?

-debuglogrotatekeep=9999999 or is that ugly? Should there be a separate -debuglogenable boolean (default true; if set to false, behave as today), and then -debuglogrotatekeep=0 could mean “never delete”. Would that be preferable? Or something else? Thanks for the comments!

in src/logging.cpp:395 in b526922724 outdated

366@@ -294,6 +367,9 @@ void BCLog::Logger::ShrinkDebugFile()
367 
368     assert(!m_file_path.empty());
369 
370+    // File rotation makes this unnecessary.
371+    if (m_rotate_keep > 0) return;

luke-jr commented at 6:13 pm on June 29, 2021:

Again, error if the user has configured both

luke-jr changes_requested

LarryRuane commented at 2:15 am on July 6, 2021: contributor

Continuously dropping logs seems like an aggressive change to me because why drop the logs when there is no attack? I haven’t had problems with logfile sizes in normal node operation. (maybe thats just me)

I come from a storage system background, and dropping the oldest logs was standard practice. (This is why there’s a logrotate facility on Unix and similar systems.) A general principle is that any demon process that can run for an arbitrary amount of time should not use resources in proportion to how long it’s been running. Yes, resource usage can increase quickly early, and after reaching “steady-state” can go up and down over time. But the system shouldn’t, for example, gradually use more memory or gradually require more CPU (requiring a restart to fix). The current logging design violates this principle as regards disk usage. So even without an active attack, we should be pruning the log file.

I think that the current behavior is actually quite useful because in case of an issue (e.g.: a crash) we have the entire log of the nodes lifespan.

That’s true, but for reasons I already stated, that’s not practical. The most useful information is likely to be in the most recent logs. The user can always increase the amount of logging (in effect, to infinity if needed). Or disable this feature (work as today). Consider another option the user has: Run some kind of small agent (even just a small shell script) that watches for new rotated debug log files, and copies them off to some other place with tons of storage. Or compresses them. Or filters out interesting stuff from them and deletes them.

This also seems to opens a similar attack vector as the global rate limiting did, an attacker (that knows of two bugs one of which is a disk filling bug) can execute an attack and then use the disk filling bug to rotate the logs out that had info about the attack.

Well, this is unlikely-squared. This isn’t something I’m worried about, but maybe I’m wrong.

Thanks for your comments!

LarryRuane commented at 6:12 pm on July 6, 2021: contributor

Added commit d885f0b3fad93b10cf400c596d57063687d05efb to address @luke-jr’s review comments (aside from “What if I want to rotate, but never delete” because I’m not sure which is the best approach). There are no tests or release notes, because still looking for approach or concept ACK.

LarryRuane force-pushed on Jul 6, 2021

LarryRuane commented at 7:31 pm on July 6, 2021: contributor

Force-pushed to fix CI failure (missing #include directive)

LarryRuane force-pushed on Jul 9, 2021

LarryRuane commented at 2:34 am on July 9, 2021: contributor

Force-pushed to fix CI failures, also added a functional test.

LarryRuane force-pushed on Jul 9, 2021

in src/logging.h:29 in c82eece30c outdated

24@@ -24,6 +25,8 @@ static const bool DEFAULT_LOGTIMESTAMPS = true;
25 static const bool DEFAULT_LOGTHREADNAMES = false;
26 static const bool DEFAULT_LOGSOURCELOCATIONS = false;
27 extern const char * const DEFAULT_DEBUGLOGFILE;
28+static const int DEFAULT_DEBUGLOGROTATEKEEP = 10;
29+static const int DEFAULT_DEBUGLOGMB = 1;

narula commented at 5:18 pm on July 14, 2021:

~~To match current behavior, wouldn’t you want DEFAULT_DEBUGLOGMB to be 10 and DEFAULT_LOGROTATEKEEP to be 1?~~

Nevermind, that isn’t current behavior either. Just curious about the constants.

LarryRuane commented at 7:51 pm on July 14, 2021:

Thanks, @narula, that’s still a good question. The idea of those defaults is that between 10 MB and 11 MB of log messages would be retained (just after a log rotation, 10 MB because there’d be 10 rotated files at 1 MB each plus an empty debug.log; just before log rotation, 10 MB for the same reason plus a 1 MB debug.log. This matches the current disk usage if bitcoind is restarted often (if not, then the debug.log size can increase without limit).

If the defaults were DEFAULT_DEBUGLOGMB=10 and DEFAULT_LOGROTATEKEEP=1 then the logging storage would vary from 10 MB (just after rotation) to 20 MB (just before rotation).

One could argue that the defaults could be quite a bit larger because the current default (10 MB minimum) was chosen when disks were smaller than they are typically today. But there could be that one user whose disk is almost full, and upgrading to a release with this PR would soon run their disk out to space if the defaults were larger. (You might say that a node’s disk space requirement grows indefinitely anyway as more blocks are added to the blockchain, but that’s not necessarily so because nodes can be configured with block pruning enabled.)

MarcoFalke added the label Review club on Jul 15, 2021

LarryRuane force-pushed on Aug 8, 2021

LarryRuane commented at 9:56 pm on August 8, 2021: contributor

There doesn’t seem to be much support for this PR, and at least part of the problem may be that it changes default behavior, and that could surprise some users. So I force-pushed a small change so that by default, there is no functional change (preserve existing behavior). If you want to enable log rotation, add lines like these to your bitcoin.conf file:

0debuglogrotatekeep=10
1debugloglimit=1

(This will retain 10 rotated files, each around 1 MB.) Starting with -debug will generate a lot more logging.

I think this version is much safer; if log rotation proves to be popular, we can consider making it default behavior later.

josibake commented at 3:18 pm on August 11, 2021: member

Concept ACK

I strongly agree with the principle above:

A general principle is that any demon process that can run for an arbitrary amount of time should not use resources in proportion to how long it’s been running. Yes, resource usage can increase quickly early, and after reaching “steady-state” can go up and down over time. But the system shouldn’t, for example, gradually use more memory or gradually require more CPU (requiring a restart to fix). The current logging design violates this principle as regards disk usage. So even without an active attack, we should be pruning the log file.

I also agree with the decision to make this optional for users (i.e it will not change default behavior). Having this as a config option would make me feel more comfortable running a node with -debug.

Also, not really important, but a personal preference: I like having logs split into timestamped files as it makes it much easier for me to search logs during the period where I believe the issue occurred as opposed to opening and searching a very large file

in doc/release-notes-22350.md:18 in 7db585f34c outdated

13+  (default 0, log rotation disabled). The `-debugloglimit=n`
14+  configuration option specifies how large the `debug.log` file must
15+  become (in MB) before it is rotated (default is 10). If you enable
16+  log file rotation and specify an
17+  alternate debug log file name using `-debuglogfile=path`, the last
18+  component of the path must be `debug.log`. (#22350)

josibake commented at 5:53 pm on August 11, 2021:

previously when using -debuglogfile=path, i could specify any name for the file. now, if anything but debug.log is specified for the name, bitcoind fails to start without error (confirmed this while testing).

imo, rotation should work regardless of the filename, but at the very least there should be an error if i don’t specify debug.log with log rotation enabled

LarryRuane commented at 5:47 am on August 12, 2021:

Thanks so much for your comments; these are very useful.

… rotation should work regardless of the filename …

I like your idea. After giving this more thought, perhaps the best approach is to let -debuglogfile=path to work as usual, but create a directory with a fixed name like rotated-logs (or rotated-debug-logs) and put all the rotated log files within that directory. (The active debug log file, debug.log or as specified by -debuglogfile, would be as it is today, no change.) Because the rotated log files are in a directory with a specific purpose, they can have simple names like 2021-06-27T15:10:55Z.log. They wouldn’t have to be named according to the -debuglogfile argument.

This would have an added benefit: As I’ve been running this code to test it, my data directory now looks “messy” or polluted, with all these rotated log files that usually aren’t very important – why should they be so visible and prominent?

0$ ls ~/.bitcoin
1banlist.dat   debug-2021-06-27T21:19:42Z.log  fee_estimates.dat  signet
2banlist.json  debug-2021-07-09T00:38:43Z.log  indexes            testnet3
3bitcoin.conf  debug-2021-07-14T16:32:30Z.log  mempool.dat        wallets
4bitcoind.pid  debug-2021-07-17T14:01:27Z.log  peers.dat          wallets-empty
5blocks        debug-2021-08-10T20:03:04Z.log  regtest
6chainstate    debug.log                       settings.json
7$

(And I’ve got only 5 rotated log files; there could be many more.) A small disadvantage is that it would be slightly more difficult to concatenate (or grep) all of the log files in time-order:

0$ cat rotated-logs/* debug.log

(instead of just cat debug*.log) but that’s not bad.

So unless there are objections, I’ll make this change within the next few days.

josibake commented at 8:59 am on August 12, 2021:

regarding many log files creating clutter in the root directory, i agree having a logs folder definitely makes sense. personally, i would prefer a logs/ folder containing all the logs as this makes it clear to the user there is nothing special about the debug.log file vs what’s in the logs directory. it also keeps things simple for concatenating and grepping.

since the debuglogfile custom name is known, wouldn’t it be possible to attempt to split the filename into a name and extension (<name>-<timestamp>.<ext>), and then handle the special case where the user doesn’t supply an extension (i.e logs, which would become logs, logs-<timestamp>)?

also, feel free to wait for more feedback from others before making changes :)

in src/logging.cpp:77 in 7db585f34c outdated

110+                return strprintf(
111+                    "The debug log file %s is not named \"debug.log\"; "
112+                    "this prevents log rotation. Please disable log rotation by "
113+                    "specifying -debuglogrotatekeep=0 or specify a debug log file "
114+                    "path that ends in \"/debug.log\" using -debuglogfile.",
115+                    m_file_path.string());

josibake commented at 6:05 pm on August 11, 2021:

this output isn’t showing (not sure why). i ran:

0./src/bitcoind -daemon -debuglogrotatekeep=3 -debugloglimit=1 -debuglogfile=/home/josibake/bitcoin/j.log

and got “Bitcoin Core Starting,” but it fails to start without any errors.

LarryRuane commented at 5:47 am on August 12, 2021:

I think that’s because you’ve started it with -daemon. It hasn’t initialized the logging yet, so it has nowhere to write the error message. If you start without -daemon you should see the error.

josibake commented at 9:06 am on August 12, 2021:

makes sense. i do think its important to throw an error when -daemon is used, else the user is likely to assume there node has started fine after seeing “Bitcoin Core Starting.” for example, if i try to start my node with:

0./src/bitcoind -datadir=foo -daemon

i will get:

0Error: Specified data directory "foo" does not exist.

LarryRuane commented at 11:17 pm on August 13, 2021:

@josibake

i do think its important to throw an error when -daemon is used …

I looked into this more, and it’s not trivial to fix. The problem is that bitcoind has already “daemonized” itself by the time it gets around to initializing the logging, which is later than when it tries to open the data directory. This problem happens without the current PR:

0$ src/bitcoind -daemon -debuglogfile=some/non-existent/path
1Bitcoin Core starting
2$

and there’s no indication that anything went wrong (the exit status is even 0, success). So I think this can be fixed in a separate PR.

in src/logging.cpp:121 in 7db585f34c outdated

117+            m_rotate_keep = 0;
118+        }
119+        if (m_file_limit == 0) {
120+            return strprintf(
121+                "The debug log file %s size limit in MB must be greater than zero.",
122+                m_file_path.string());

josibake commented at 6:06 pm on August 11, 2021:

i also got a silent failure trying to specify a logsize value < 1. i ran with -debugloglimit=0.5, got no errors, but the node failed to start. i also ran with -debugloglimit=1.5 without issue so i dont think its the decimal

LarryRuane commented at 5:47 am on August 12, 2021:

That’s because it’s parsing the argument as an integer, and when it gets to the decimal point, it stops parsing, interpreting the argument as zero. Specifying fractions of a megabyte is probably unnecessary, but maybe it should print a good error message. I’ll see what I can do.

josibake commented at 9:12 am on August 12, 2021:

i went back and confirmed, even with 1.5 it is still rotating in 1mb chunks. might be worth adding something to the option help that debugloglimit is in MB and must be specified as an integer

josibake commented at 6:15 pm on August 11, 2021: member

while testing, I noticed there were silent failures when I specified a name other than debug.log with log rotation requested and silent failures if I specified a debugloglimit of < 1 (but greater than zero). I see there is code to handle the filename error, but it doesn’t appear to be working as intended.

I agree with the concept of log rotation, but I’m not crazy about the approach of restricting the user to a hard coded name for the log file, especially since before this they could specify an arbitrary filename. Seems worth the effort to enable log rotation even with an arbitrary filename from the user, imo.

LarryRuane marked this as a draft on Aug 12, 2021

theStack commented at 10:12 am on August 13, 2021: member

Strong Concept ACK

I think one of the main reasons why restricting unlimited growth of the log file was never seen as particularly problematic in Bitcoin Core is the fact that running bitcoind with default parameters (that is, w/o pruning) already consumes (potentially) infinite disk space over time. Having a growth rate of > 1 GB per week of the block chain on mainnet, one would hardly be worried about the additional space consumption of relatively small log messages.

That said, I still fully agree that we should support log-rotating for the following reasons:

log file growth is less deterministic than block chain growth; there could be a scenario that we are not aware of yet that leads to massive log ouputs, that an attacker could exploit
running pruned nodes will be more and more common in the future, and for those it makes even more sense to also limit the log-file size (I could imagine that users with very limited disk-space, running with -prune=550 ran already into problems due to unexpected unlimited log file growth)
fulfilling the principle you mentioned:

[…] any demon process that can run for an arbitrary amount of time should not use resources in proportion to how long it’s been running.

I also think it’s better to keep the default behaviour by now as it is. Maybe it’s worth it mentioning in the docs that using this new feature is recommended for pruned node users?

Feel free to tag me if this PR is out of draft, will be happy to review.

add FormatISO8601DateTimeBasic()

See https://en.wikipedia.org/wiki/ISO_8601#Times
This is needed because log file rotation filenames are constructed using
this "basic" format, because it has no ":" (colon) characters, which
Windows does not allow.

fddc900065

Logger::StartLogging() returns an error string instead of bool

No functional change.

4f510dc193

add log file rotation configuration arguments

-debuglogrotatekeep: number of rotated log files to keep
-debugloglimit: debug.log size (in MB) before it is rotated

The default -debuglogrotatekeep value is 0, which disables rotation.

9fb0529a9a

LarryRuane force-pushed on Aug 13, 2021

LarryRuane commented at 11:32 pm on August 13, 2021: contributor

Thanks for the concept review, @theStack, I just force-pushed a new version that’s a lot better, I think, and implements the suggestions here: #22350 (review)

LarryRuane force-pushed on Aug 13, 2021

LarryRuane marked this as ready for review on Aug 13, 2021

Implement debug.log file rotation

No functional change by default. Add support for log file rotation.
If enabled, then when the debug.log file reaches a certain size
(default, 10MB), rename it to a file within a separate directory,
debug-rotate, named according to the current date and time, and
restart (truncate) debug.log. This keeps debug.log from growing
unbounded. Retain a limited number of rotated files.

1fd9de1692

add release note 13511cec51

LarryRuane force-pushed on Aug 16, 2021

LarryRuane commented at 5:23 am on August 16, 2021: contributor

Force-pushed a small change to ensure that rotated debug log files don’t have partial lines. Each line is guaranteed to be a full line beginning with a timestamp.

This is an improvement over master, because with master (or with this PR without log rotation enabled), if the debug log file is larger than 11 MB when bitcoind starts, it will be truncated to exactly 10 MB, and this almost always results in the file beginning with a partial line. This can mess up attempts to parse the debug log file.

laanwj commented at 2:06 pm on August 16, 2021: member

Concept ~0 on this. We always used to be against integrating log rotation because it can be handled by external tools (sure, on some operating systems this is more difficult, but I’m sure there are solutions for WIndows too). We should avoid the case where bitcoind becomes a systemd-like project that absorbs any possible functionality of a system. It’s a maintainance nightmare.

LarryRuane commented at 4:59 pm on August 16, 2021: contributor

It seems to me bitcoind should manage its own log file appropriately. In contrast, systemd supports other components, outside itself. I also want to point out that the OS-specific calls added by this PR:

fs::create_directories
fs::remove
fs::rename

are already being used elsewhere; we’re not introducing any new dependencies on filesystem calls. So I don’t know that maintenance would be more difficult.

josibake commented at 5:20 pm on August 16, 2021: member

It seems to me bitcoind should manage its own log file appropriately

i think it’s also worth mentioning this isn’t only about log management and is (more importantly, imo) a solution to mitigate a disk fill attack

A disk fill attack is an attack where an untrusted party (such as a peer) is able to cheaply make your node log to disk excessively. The excessive logging may fill your disk and thus make your node crash either cleanly (best case: if disk fill rate is relatively slow) or uncleanly (worst case: if disk fill rate is relatively fast)

as mentioned in the comments, a log rotation solution (as opposed to the rate limiting solution proposed in #21603 ) doesn’t result in partial logs, which i think is important. @LarryRuane perhaps it would help to update the description of the PR to be more clear about how this is a mitigation for disk fill attacks (without the reviewer needing context from #21603 )

fanquake commented at 3:07 am on August 17, 2021: member

are already being used elsewhere; we’re not introducing any new dependencies on filesystem calls. So I don’t know that maintenance would be more difficult.

The point is less about what filesystem calls might be used, but the higher level observation that bitcoind (and this repository in general) should not be a catch-all for code and functionality that actually has nothing to-do with bitcoin.

Adding and having to maintain large amounts, of sometimes complex code, to do non-bitcoin related things, i.e log management, settings parsing, cli’s, ELF binary parsing, etc, adds a lot of (development) noise, maintenance, and back-compat overhead.

Ideally this repository is moving in the opposite direction, where instead of adding more and more code, to do more and more things, we’re actually removing more and more code, to the point where we end up with just the code that is bitcoin, and more (useful) libconsensus type things. That effort is somewhat underway, i.e with the separation of the GUI development, de-globalization of the chainstate etc.

LarryRuane commented at 8:33 pm on September 28, 2021: contributor

Thanks, @fanquake and @laanwj! Very good points.

… separation of the GUI …

You’ve given me an idea, tell me what you think of this (also @jnewbery and @theStack) as an alternative to this PR:

I just did a proof-of-concept experiment on Linux (I don’t know if this would work on Windows):

0$ mknod /tmp/bitcoind-log p # p means named pipe, super-user (root) permissions are not needed
1$ cat /tmp/bitcoind-log     # this will just wait

In another window

0$ bitcoind -shrinkdebugfile=0 -debuglogfile=/tmp/bitcoind-log

You’ll see the debug.log messages emanating from the cat command. Named pipes are cool. (When the writer, i.e. bitcoind, exits, cat gets an end-of-file and cleanly exits too.)

Proposal: What do you think about adding a Python script to contrib that’s just a wrapper around bitcoind – it would create the named pipe, start bitcoind (with passed-in arguments plus the -shrinkdebugfile=0 -debuglogfile=/tmp/bitcoind-log arguments), and then reads from the named pipe and writes to debug.log. It would be an alternate, entirely optional way to run the client.

This would reproduce the current functionality (except for shrinking debug.log at startup, but it could do that too).

But this python script could be enhanced to do log rotation – and in the future, maybe other stuff like compression, filtering, sending some kind of alert or notification if certain log messages appear, who knows – all without any changes to bitcoind.

DrahtBot added the label Needs rebase on Oct 15, 2021

DrahtBot commented at 9:39 am on October 15, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

LarryRuane commented at 3:26 pm on January 26, 2022: contributor

Closing due to insufficient support; I still think it would be valuable to implement what I described above #22350 (comment) and that would be much less intrusive (only adds to contrib/), but that can be a new PR.

LarryRuane closed this on Jan 26, 2022

DrahtBot locked this on Mar 8, 2023

Rotate debug.log file #22350

Conflicts