💬 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.
💬 dergoegge commented on pull request "p2p: Prevent block index fingerprinting by sending additional getheaders messages":
(https://github.com/bitcoin/bitcoin/pull/24571#issuecomment-1475286226)
@stratospher Thank you for having a look at this! There is an alternative approach to the one in this PR that I have been meaning to implement and I forgot to put this PR in draft mode.
We made some changes to the headers sync logic (https://github.com/bitcoin/bitcoin/pull/25454), which causes us to now only have one `getheaders` request in flight for the same peer. If we start storing the locator that is sent in the `getheaders` message and only allow `headers` in response that connect to th
...
(https://github.com/bitcoin/bitcoin/pull/24571#issuecomment-1475286226)
@stratospher Thank you for having a look at this! There is an alternative approach to the one in this PR that I have been meaning to implement and I forgot to put this PR in draft mode.
We made some changes to the headers sync logic (https://github.com/bitcoin/bitcoin/pull/25454), which causes us to now only have one `getheaders` request in flight for the same peer. If we start storing the locator that is sent in the `getheaders` message and only allow `headers` in response that connect to th
...
📝 dergoegge converted_to_draft a pull request: "p2p: Prevent block index fingerprinting by sending additional getheaders messages"
(https://github.com/bitcoin/bitcoin/pull/24571)
The block index might contain stale blocks that are not part of the main chain. If a malicious peer is able to probe a node's block index for certain stale blocks then it can use this information to fingerprint the node.
When receiving headers (either through a `cmpctblock` or `headers` messages) a node will send `getheaders` if the predecessor of the first header does not exist. This leaks information from the block index if the predecessor of the header is a stale block because no `getheade
...
(https://github.com/bitcoin/bitcoin/pull/24571)
The block index might contain stale blocks that are not part of the main chain. If a malicious peer is able to probe a node's block index for certain stale blocks then it can use this information to fingerprint the node.
When receiving headers (either through a `cmpctblock` or `headers` messages) a node will send `getheaders` if the predecessor of the first header does not exist. This leaks information from the block index if the predecessor of the header is a stale block because no `getheade
...
💬 ryanofsky commented on pull request "rpc, test: remove newline escape sequence from wallet warning fields":
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1475286648)
> I'm not sure it makes sense to patch around this by adding code to our CLI to parse newline escape sequences
Yes agree no need for this as we already have a JSON parser. CLI could see simply if check result object has `"warning"` field which is a string, and if it does, write the string to stderr.
This would be really simple, and make warnings more obvious and readable, and be compatible with other bitcoin-cli improvements in the future.
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1475286648)
> I'm not sure it makes sense to patch around this by adding code to our CLI to parse newline escape sequences
Yes agree no need for this as we already have a JSON parser. CLI could see simply if check result object has `"warning"` field which is a string, and if it does, write the string to stderr.
This would be really simple, and make warnings more obvious and readable, and be compatible with other bitcoin-cli improvements in the future.
💬 Sjors commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141394521)
Done
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141394521)
Done
💬 Sjors commented on pull request "Log successful AcceptBlockHeader()":
(https://github.com/bitcoin/bitcoin/pull/27276#discussion_r1141395250)
Done
(https://github.com/bitcoin/bitcoin/pull/27276#discussion_r1141395250)
Done
💬 jonatack commented on pull request "rpc, test: remove newline escape sequence from wallet warning fields":
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1475293514)
If these four remaining "warning" fields are behind a deprecation flag (opening a PR today), hopefully that avoids the need to do anything further.
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1475293514)
If these four remaining "warning" fields are behind a deprecation flag (opening a PR today), hopefully that avoids the need to do anything further.
💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1475297075)
Concept ACK
Unconditionally logging headers (outside of IBD) makes sense to me. It doubles the number of messages per block, but since we check their proof-of-work, it's not too bad.
Similarly for compact blocks we check the PoW before logging.
utACK dbd144206a491e3e16d662d0daeffac71bb5b8c7
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1475297075)
Concept ACK
Unconditionally logging headers (outside of IBD) makes sense to me. It doubles the number of messages per block, but since we check their proof-of-work, it's not too bad.
Similarly for compact blocks we check the PoW before logging.
utACK dbd144206a491e3e16d662d0daeffac71bb5b8c7
💬 Sjors commented on pull request "Log successful AcceptBlockHeader()":
(https://github.com/bitcoin/bitcoin/pull/27276#issuecomment-1475297700)
At this point I prefer #27278, but I'll leave this open in case that PR doesn't make it. This one is (slightly) easier to review.
(https://github.com/bitcoin/bitcoin/pull/27276#issuecomment-1475297700)
At this point I prefer #27278, but I'll leave this open in case that PR doesn't make it. This one is (slightly) easier to review.
👍 Sjors approved a pull request: "Log new headers"
(https://github.com/bitcoin/bitcoin/pull/27278)
Concept ACK
Unconditionally logging headers (outside of IBD) makes sense to me. It doubles the number of messages per block, but since we check their proof-of-work, it's not too bad.
Similarly for compact blocks we check the PoW before logging.
utACK dbd144206a491e3e16d662d0daeffac71bb5b8c7
(https://github.com/bitcoin/bitcoin/pull/27278)
Concept ACK
Unconditionally logging headers (outside of IBD) makes sense to me. It doubles the number of messages per block, but since we check their proof-of-work, it's not too bad.
Similarly for compact blocks we check the PoW before logging.
utACK dbd144206a491e3e16d662d0daeffac71bb5b8c7
💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141396013)
Maybe add an anti-footgun note here, since I almost made that mistake:
```
// Not all valid headers have a known height, so we don't log pindex->nHeight.
```
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141396013)
Maybe add an anti-footgun note here, since I almost made that mistake:
```
// Not all valid headers have a known height, so we don't log pindex->nHeight.
```