The code in `logging.cpp` needs to:
* Get the category name given the flag (e.g. `BCLog::PRUNE` -> `"prune"`)
* Get the flag given the category name (e.g. `"prune"` -> `BCLog::PRUNE`)
* Get the list of category names sorted in alphabetical order
Achieve this by using the proper std containers. The result is
* less code (this diff is +62 / -129)
* faster code (to linear search and no copy+sort)
* more maintainable code (the categories are no longer duplicated in
`LogCategories[]` and `LogCategoryToStr()`)
This behavior is preserved:
`BCLog::NONE` -> `""` (lookup by `LogCategoryToStr()`)
`""` -> `BCLog::ALL` (lookup by `GetLogCategory("")`)
This option tells the logging system to always include a "[cat:level]"
prefix, so [net] becomes [net:debug], LogInfo/LogPrint statements will have
an [all:info] prefix, and LogWarning and LogError logs will become
[all:warning] and [all:error]. This may be easier for automated parsing
of logs, particularly if additional prefixes such as thread or source
location are enabled.
Previously Info-level logging when a category was specified (via
LogPrintLevel) would only print the corresponding log message if
`-debug=category` were specified, while Info-level logging without a
category would always be printed. Make this more consistent by having
Info messages always be logged, whether they include a category or not.
73f4eb511c Check that the Timestamp String is valid (John Moffett)
Pull request description:
Follow-up to https://github.com/bitcoin/bitcoin/pull/27233
The current `FormatISO8601DateTime` function will return an empty string if it encounters an error when converting the `int64_t` seconds-since-epoch to a formatted date time. In the unlikely case that happens, here `strStamped.pop_back()` would be undefined behavior.
ACKs for top commit:
MarcoFalke:
lgtm ACK 73f4eb511c
stickies-v:
ACK 73f4eb511c
Tree-SHA512: 089ed639c193deb98870a8385039b31b4baed821ea907937bfc6f65a5b0981bbf8284b2afec81b2d0a922e2340716b48cf55349640eb6b8c311ef7af25abc361
00e9b97f37 refactor: Move fs.* to util/fs.* (TheCharlatan)
106b46d9d2 Add missing fs.h includes (TheCharlatan)
b202b3dd63 Add missing cstddef include in assumptions.h (TheCharlatan)
18fb36367a refactor: Extract util/fs_helpers from util/system (Ben Woosley)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". This commit was originally authored by empact and is taken from its parent PR #25152.
#### Context
There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel library (https://github.com/bitcoin/bitcoin/pull/25290, https://github.com/bitcoin/bitcoin/pull/25487, https://github.com/bitcoin/bitcoin/pull/25527, https://github.com/bitcoin/bitcoin/pull/25862, https://github.com/bitcoin/bitcoin/pull/26177, and https://github.com/bitcoin/bitcoin/pull/27125). The `ArgsManager` is defined in `system.h`. A similar pull request extracting functionality from `system.h` has been merged in https://github.com/bitcoin/bitcoin/pull/27238.
#### Changes
Next to providing better code organization, this PR removes some reliance of the tree of libbitcoinkernel header includes on `system.h` (and thus the `ArgsManager` definition) by moving filesystem related functions out of the `system.*` files.
There is already a pair of `fs.h` / `fs.cpp` in the top-level `src/` directory. They were not combined with the files introduced here, to keep the patch cleaner and more importantly because they are often included without the utility functions. The new files are therefore named `fs_helpers` and the existing `fs` files are moved into the util directory.
Further commits splitting more functionality out of `system.h` are still in #25152 and will be submitted in separate PRs once this PR has been processed.
ACKs for top commit:
hebasto:
ACK 00e9b97f37
Tree-SHA512: 31422f148d14ba3c843b99b1550a6fd77c77f350905ca324f93d4f97b652246bc58fa9696c64d1201979cf88733e40be02d262739bb7d417cf22bf506fdb7666
The current `FormatISO8601DateTime` function will
return an empty string if it encounters an error
when converting the `int64_t` seconds since epoch
to a formatted date time. In the unlikely case that happens,
`strStamped.pop_back()` would be undefined behavior.
The fs.* files are already part of the libbitcoin_util library. With the
introduction of the fs_helpers.* it makes sense to move fs.* into the
util/ directory as well.
9580480570 Update debug logging section in the developer notes (Jon Atack)
1abaa31aa3 Update -debug and -debugexclude help docs for severity level logging (Jon Atack)
45f9282162 Create BCLog::Level::Trace log severity level (Jon Atack)
2a8712db4f Unit test coverage for -loglevel configuration option (klementtan)
eb7bee5f84 Create -loglevel configuration option (klementtan)
98a1f9c687 Unit test coverage for log severity levels (klementtan)
9c7507bf76 Create BCLog::Logger::LogLevelsString() helper function (klementtan)
8fe3457dbb Update LogAcceptCategory() and unit tests with log severity levels (klementtan)
c2797cfc60 Add BCLog::Logger::SetLogLevel()/SetCategoryLogLevel() for string inputs (klementtan)
f6c0cc0350 Add BCLog::Logger::m_category_log_levels data member and getter/setter (Jon Atack)
2978b387bf Add BCLog::Logger::m_log_level data member and getter/setter (Jon Atack)
f1379aeca9 Simplify BCLog::Level enum class and LogLevelToStr() function (Jon Atack)
Pull request description:
This is an updated version of https://github.com/bitcoin/bitcoin/pull/25287 and the next steps in parent PR #25203 implementing, with Klement Tan, user-configurable, per-category severity log levels based on an idea by John Newbery and refined in GitHub discussions by Wladimir Van der Laan and Marco Falke.
- simplify the `BCLog::Level` enum class and the `LogLevelToStr()` function and add documentation
- update the logging logic to filter logs by log level both globally and per-category
- add a hidden `-loglevel` help-debug config option to allow testing setting the global or per-category severity level on startup for logging categories enabled with the `-debug` configuration option or the logging RPC (Klement Tan)
- add a `trace` log severity level selectable by the user; the plan is for the current debug messages to become trace, LogPrint ones to become debug, and LogPrintf ones to become info, warning, or error
```
$ ./src/bitcoind -help-debug | grep -A10 loglevel
-loglevel=<level>|<category>:<level>
Set the global or per-category severity level for logging categories
enabled with the -debug configuration option or the logging RPC:
info, debug, trace (default=info); warning and error levels are
always logged. If <category>:<level> is supplied, the setting
will override the global one and may be specified multiple times
to set multiple category-specific levels. <category> can be:
addrman, bench, blockstorage, cmpctblock, coindb, estimatefee,
http, i2p, ipc, leveldb, libevent, lock, mempool, mempoolrej,
net, proxy, prune, qt, rand, reindex, rpc, selectcoins, tor,
util, validation, walletdb, zmq.
```
See the individual commit messages for details.
ACKs for top commit:
jonatack:
One final push per `git range-diff a5d5569 ce3c4c9 9580480` (should be trivial to re-ACK) to ensure this pull changes no default behavior in any way for users or the tests/CI in order to be completely v24 compatible, to update the unit test setup in general, and to update the debug logging section in the developer notes.
klementtan:
reACK 9580480570
1440000bytes:
reACK 9580480570
vasild:
ACK 9580480570
dunxen:
reACK 9580480
brunoerg:
reACK 9580480570
Tree-SHA512: 476a638e0581f40b5d058a9992691722e8b546471ec85e07cbc990798d1197fbffbd02e1b3d081b4978404e07a428378cdc8e159c0004b81f58be7fb01b7cba0
Avoid permanently storing headers from a peer, unless the headers are part of a
chain with sufficiently high work. This prevents memory attacks using low-work
headers.
Designed and co-authored with Pieter Wuille.
for verbose log messages for development or debugging only, as bitcoind may run
more slowly, that are more granular/frequent than the Debug log level, i.e. for
very high-frequency, low-level messages to be logged distinctly from
higher-level, less-frequent debug logging that could still be usable in production.
An example would be to log higher-level peer events (connection, disconnection,
misbehavior, eviction) as Debug, versus Trace for low-level, high-volume p2p
messages in the BCLog::NET category. This will enable the user to log only the
former without the latter, in order to focus on high-level peer management events.
With respect to the name, "trace" is suggested as the most granular level
in resources like the following:
- https://sematext.com/blog/logging-levels
- https://howtodoinjava.com/log4j2/logging-levels
Update the test framework and add test coverage.
- add a -loglevel=<level>|<category:level> config option to allow users
to set a global -loglevel and category-specific log levels. LogPrintLevel
messages with a higher severity level than -loglevel will not be printed
in the debug log.
- for now, this config option is debug-only during the migration to
severity-based logging
- update unit and functional tests
Co-authored-by: "Jon Atack <jon@atack.com>"
- simplify the BCLog::Level enum class (and future changes to it) by
only setting the value of the first enumerator
- move the BCLog::Level:None enumerator to the end of the BCLog::Level
enum class and LogLevelToStr() member function, as the None enumerator
is only used internally, and by being the highest BCLog::Level value it
can be used to iterate over the enumerators
- replace the unused BCLog::Level:None string "none" with an empty string
as the case will never be hit
- add documentation
Warning: Replacing fs::system_complete calls with fs::absolute calls
in this commit may cause minor changes in behaviour because fs::absolute
no longer strips trailing slashes; however these changes are believed to
be safe.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Implement the following commands from the I2P SAM protocol:
* HELLO: needed for all of the remaining ones
* DEST GENERATE: to generate our private key and destination
* NAMING LOOKUP: to convert .i2p addresses to destinations
* SESSION CREATE: needed for STREAM CONNECT and STREAM ACCEPT
* STREAM CONNECT: to make outgoing connections
* STREAM ACCEPT: to accept incoming connections
b4511e2e2e log: Prefix log messages with function name if -logsourcelocations is set (practicalswift)
Pull request description:
Prefix log messages with function name if `-logfunctionnames` is set.
Yes, exactly like `-logthreadnames` but for function names instead of thread names :)
This is a small developer ergonomics improvement: I've found this to be a cheap/simple way to correlate log output and originating function.
For me it beats the ordinary cycle of 1.) try to figure out a regexp matching the static part of the dynamic log message, 2.) `git grep -E 'Using .* MiB out of .* requested for signature cache'`, 3.) `mcedit filename.cpp` (`openemacs filename.cpp` works too!) and 4.) search for log message and scroll up to find the function name :)
Without any logging parameters:
```
$ src/bitcoind -regtest
2020-08-25T03:29:04Z Using RdRand as an additional entropy source
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z block tree size = 1
2020-08-25T03:29:04Z nBestHeight = 0
2020-08-25T03:29:04Z Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z 0 addresses found from DNS seeds
```
With `-logthreadnames` and `-logfunctionnames`:
```
$ src/bitcoind -regtest -logthreadnames -logfunctionnames
2020-08-25T03:29:04Z [init] [ReportHardwareRand] Using RdRand as an additional entropy source
2020-08-25T03:29:04Z [init] [InitSignatureCache] Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [InitScriptExecutionCache] Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [LoadChainTip] Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z [init] [AppInitMain] block tree size = 1
2020-08-25T03:29:04Z [init] [AppInitMain] nBestHeight = 0
2020-08-25T03:29:04Z [loadblk] [LoadMempool] Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z [dnsseed] [ThreadDNSAddressSeed] 0 addresses found from DNS seeds
```
ACKs for top commit:
laanwj:
Code review ACK b4511e2e2e
MarcoFalke:
review ACK b4511e2e2e🌃
Tree-SHA512: d100f5364630c323f31d275259864c597f7725e462d5f4bdedcc7033ea616d7fc0d16ef1b2af557e692f4deea73c6773ccfc681589e7bf6ba970b9ec169040c7
5478d6c099 logging: thread safety annotations (Anthony Towns)
e685ca1992 util/system.cpp: add thread safety annotations for dir_locks (Anthony Towns)
a788789948 test/checkqueue_tests: thread safety annotations (Anthony Towns)
479c5846f7 rpc/blockchain.cpp: thread safety annotations for latestblock (Anthony Towns)
8b5af3d4c1 net: fMsgProcWake use LOCK instead of lock_guard (Anthony Towns)
de7c5f41ab wallet/wallet.h: Remove mutexScanning which was only protecting a single atomic bool (Anthony Towns)
c3cf2f5501 rpc/blockchain.cpp: Remove g_utxosetscan mutex that is only protecting a single atomic variable (Anthony Towns)
Pull request description:
In a few cases we need to use `std::mutex` rather than the sync.h primitives. But `std::lock_guard<std::mutex>` doesn't include the clang thread safety annotations unless you also use clang's C library, which means you can't indicate when variables should be guarded by `std::mutex` mutexes.
This adds an annotated version of `std::lock_guard<std::mutex>` to threadsafety.h to fix that, and modifies places where `std::mutex` is used to take advantage of the annotations.
It's based on top of #16112, and turns the thread safety comments included there into annotations.
It also changes the RAII classes in wallet/wallet.h and rpc/blockchain.cpp to just use the atomic<bool> flag for synchronisation rather than having a mutex that doesn't actually guard anything as well.
ACKs for top commit:
MarcoFalke:
ACK 5478d6c099🗾
hebasto:
re-ACK 5478d6c099, only renamed s/`MutexGuard`/`LockGuard`/, and dropped the commit "test/util_threadnames_tests: add thread safety annotations" since the [previous](https://github.com/bitcoin/bitcoin/pull/16127#pullrequestreview-414184113) review.
ryanofsky:
Code review ACK 5478d6c099. Thanks for taking suggestions! Only changes since last review are dropping thread rename test commit d53072ec730d8eec5a5b72f7e65a54b141e62b19 and renaming mutex guard to lock guard
Tree-SHA512: 7b00d31f6f2b5a222ec69431eb810a74abf0542db3a65d1bbad54e354c40df2857ec89c00b4a5e466c81ba223267ca95f3f98d5fbc1a1d052a2c3a7d2209790a
