💬 dergoegge commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1141365401)
Done!
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1141365401)
Done!
💬 dergoegge commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#issuecomment-1475261198)
Rebased
(https://github.com/bitcoin/bitcoin/pull/27257#issuecomment-1475261198)
Rebased
💬 Sjors commented on pull request "Move tx enqueue messages to mempool log":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141366114)
Note: I don't know if there was any particular reason why the endline characters are vertically aligned. If there's no reason, then it would be easier to move them all to the left.
cc @kczyz
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141366114)
Note: I don't know if there was any particular reason why the endline characters are vertically aligned. If there's no reason, then it would be easier to move them all to the left.
cc @kczyz
💬 jamesob commented on pull request "Log successful AcceptBlockHeader()":
(https://github.com/bitcoin/bitcoin/pull/27276#issuecomment-1475264929)
Beat me to it! I was just about to open this PR :).
For what it's worth, I think that the right approach here is to make this message `LogPrintf()` and omit it when in initial block download, the rationale being that if something weird happens on the network (like the inspiration for this PR last night), having a lot of data available to investigate would be nice, and most people don't run with verbose logging. Since the additional log volume here is quite minimal (one extra line per block, b
...
(https://github.com/bitcoin/bitcoin/pull/27276#issuecomment-1475264929)
Beat me to it! I was just about to open this PR :).
For what it's worth, I think that the right approach here is to make this message `LogPrintf()` and omit it when in initial block download, the rationale being that if something weird happens on the network (like the inspiration for this PR last night), having a lot of data available to investigate would be nice, and most people don't run with verbose logging. Since the additional log volume here is quite minimal (one extra line per block, b
...
💬 Sjors commented on pull request "Log successful AcceptBlockHeader()":
(https://github.com/bitcoin/bitcoin/pull/27276#issuecomment-1475269967)
I opened #27277 to make `-debug=validation` less noisy, making it a more attractive setting. At least _after_ IBD, since it produces ~4 lines per block: AcceptBlockHeader, NewPoWValidBlock, Pre-allocating and BlockChecked.
(https://github.com/bitcoin/bitcoin/pull/27276#issuecomment-1475269967)
I opened #27277 to make `-debug=validation` less noisy, making it a more attractive setting. At least _after_ IBD, since it produces ~4 lines per block: AcceptBlockHeader, NewPoWValidBlock, Pre-allocating and BlockChecked.
💬 fanquake commented on pull request "Log successful AcceptBlockHeader()":
(https://github.com/bitcoin/bitcoin/pull/27276#issuecomment-1475270418)
cc @0xB10C
(https://github.com/bitcoin/bitcoin/pull/27276#issuecomment-1475270418)
cc @0xB10C
📝 jamesob opened a pull request: "Log new headers"
(https://github.com/bitcoin/bitcoin/pull/27278)
Alternate to #27276.
Devs were [suprised to realize](https://twitter.com/jamesob/status/1637191706691903488) last night that we don't have definitive logging for when a given header was first received.
This logs to the main stream when new headers are received outside of IBD, as well as when headers come in over cmpctblocks. The rationale of not hiding these under log categories is that they may be useful to have widely available when debugging strange network activity, and the marginal v
...
(https://github.com/bitcoin/bitcoin/pull/27278)
Alternate to #27276.
Devs were [suprised to realize](https://twitter.com/jamesob/status/1637191706691903488) last night that we don't have definitive logging for when a given header was first received.
This logs to the main stream when new headers are received outside of IBD, as well as when headers come in over cmpctblocks. The rationale of not hiding these under log categories is that they may be useful to have widely available when debugging strange network activity, and the marginal v
...
💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1475273413)
The second commit 8828f03db8fab18d56a638729f4a6afa007c259e (compact blocks) is useful regardless of whether this PR or #27276 makes it in. I'll let you rebase it in the latter case, since it makes my PR less trivial to review :-)
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1475273413)
The second commit 8828f03db8fab18d56a638729f4a6afa007c259e (compact blocks) is useful regardless of whether this PR or #27276 makes it in. I'll let you rebase it in the latter case, since it makes my PR less trivial to review :-)
💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141374362)
Maybe use `LogPrint(BCLog::VALIDATION,` when in IBD?
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141374362)
Maybe use `LogPrint(BCLog::VALIDATION,` when in IBD?
💬 jonatack commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141375638)
An alternative you could consider would be to use the `trace` log level, so this would be logged only if the `trace` loglevel is set with -loglevel, either globally or for this category.
```suggestion
LogPrintLevel(BCLog::VALIDATION, BCLog::Level::Trace, "Pre-allocating up to position 0x%x in %s%05u.dat\n", new_size, m_prefix, pos.nFile);
```
```
$ ./src/bitcoind -help-debug | grep -A10 loglevel
-loglevel=<level>|<category>:<level>
Set the global or per-catego
...
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141375638)
An alternative you could consider would be to use the `trace` log level, so this would be logged only if the `trace` loglevel is set with -loglevel, either globally or for this category.
```suggestion
LogPrintLevel(BCLog::VALIDATION, BCLog::Level::Trace, "Pre-allocating up to position 0x%x in %s%05u.dat\n", new_size, m_prefix, pos.nFile);
```
```
$ ./src/bitcoind -help-debug | grep -A10 loglevel
-loglevel=<level>|<category>:<level>
Set the global or per-catego
...
💬 jonatack commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141376463)
(Just changing the category might only shift verbose logging from one place to another. The idea of the log levels is to allow more granular control over detail).
```cpp
from src/logging.h
enum class Level {
Trace = 0, // High-volume or detailed logging for development/debugging
Debug, // Reasonably noisy logging, but still usable in production
Info, // Default
Warning,
Error,
```
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141376463)
(Just changing the category might only shift verbose logging from one place to another. The idea of the log levels is to allow more granular control over detail).
```cpp
from src/logging.h
enum class Level {
Trace = 0, // High-volume or detailed logging for development/debugging
Debug, // Reasonably noisy logging, but still usable in production
Info, // Default
Warning,
Error,
```
💬 Sjors commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141376879)
I think blockstorage is correct place for this in any case. E.g. it also logs `Leaving block file`. I don't know if it makes sense to reduce the log level.
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141376879)
I think blockstorage is correct place for this in any case. E.g. it also logs `Leaving block file`. I don't know if it makes sense to reduce the log level.
💬 jamesob commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141377239)
Good call - done.
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141377239)
Good call - done.
💬 jonatack commented on pull request "Log successful AcceptBlockHeader()":
(https://github.com/bitcoin/bitcoin/pull/27276#discussion_r1141377590)
`LogPrint` is being migrated to `LogPrintLevel` -- you can use that when adding logging (or `LogPrintfCategory` if you decide to make this logged unconditionally, but the former with a level of, say, info or warning, may suffice).
(https://github.com/bitcoin/bitcoin/pull/27276#discussion_r1141377590)
`LogPrint` is being migrated to `LogPrintLevel` -- you can use that when adding logging (or `LogPrintfCategory` if you decide to make this logged unconditionally, but the former with a level of, say, info or warning, may suffice).
💬 jonatack commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141378414)
See https://github.com/bitcoin/bitcoin/pull/27276#discussion_r1141377590.
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141378414)
See https://github.com/bitcoin/bitcoin/pull/27276#discussion_r1141377590.
💬 Sjors commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141378848)
Looking at the history of this category, it was introduced as part of a larger change in #23235 to reduce the default log noise. There was no discussion about this particular line, and my guess is that it was overlooked. cc @ajtowns
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141378848)
Looking at the history of this category, it was introduced as part of a larger change in #23235 to reduce the default log noise. There was no discussion about this particular line, and my guess is that it was overlooked. cc @ajtowns
💬 jamesob commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141379233)
Done
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141379233)
Done
💬 jonatack commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141379531)
The info level is logged by default and it would ease the review burden to use the new macros instead of the ones that will have to be updated. You may be right about BLOCKSTORE being a better place.
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141379531)
The info level is logged by default and it would ease the review burden to use the new macros instead of the ones that will have to be updated. You may be right about BLOCKSTORE being a better place.
💬 Sjors commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141379744)
(added a note to the PR description)
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141379744)
(added a note to the PR description)
💬 Sjors commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141380000)
I'll look into switching to the `LogPrintLevel` function.
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141380000)
I'll look into switching to the `LogPrintLevel` function.