Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Aug 29, 2024

LogPrint has many issues:

  • It seems to indicate that something is being "printed", however config options such as -printtoconsole actually control what and where something is logged.
  • It does not mention the log severity (debug).
  • It is a deprecated alias for LogDebug, according to the dev notes.
  • It wastes review cycles, because reviewers sometimes point out that it is deprecated.
  • It makes the code inconsistent, when both are used, possibly even in lines right next to each other (like in InitHTTPServer)

Fix all issues by removing the deprecated alias.

I checked all conflicting pull requests and at the time of writing there are no conflicts, except in pull requests that are marked as draft, are yet unreviewed, or are blocked on feedback for other reasons. So I think it is fine to do now.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 29, 2024

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #30673 (coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries by andrewtoth)
  • #30661 (fuzz: Test headers pre-sync through p2p by marcofleon)
  • #30635 (rpc: add optional blockhash to waitfornewblock by Sjors)
  • #30409 (Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block by Sjors)
  • #30110 (refactor: TxDownloadManager + fuzzing by glozow)
  • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
  • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
  • #29256 (Improve new LogDebug/Trace/Info/Warning/Error Macros by ryanofsky)
  • #28676 ([WIP] Cluster mempool implementation by sdaftuar)
  • #28521 (net, net_processing: additional and consistent disconnect logging by Sjors)
  • #28031 (Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling by glozow)
  • #27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)
  • #25832 (tracing: network connection tracepoints by 0xB10C)
  • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic 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.

@DrahtBot DrahtBot changed the title scripted-diff: LogPrint -> LogDebug scripted-diff: LogPrint -> LogDebug Aug 29, 2024
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa6b80b

As already addressed in PR description, there are a fair amount of conflicts with other PRs, but the fact that it's a trivial scripted-diff makes it very easy to incorporate this change with git rebase --exec.

It wastes review cycles, because reviewers sometimes point out that it is deprecated.

This alone is worth the (again, trivial) merge conflicts imo.

-BEGIN VERIFY SCRIPT-
 sed -i 's/\<LogPrint\>/LogDebug/g' $( git grep -l '\<LogPrint\>'  -- ./contrib/ ./src/ ./test/ ':(exclude)src/logging.h' )
-END VERIFY SCRIPT-
@maflcko maflcko force-pushed the 2408-log branch 2 times, most recently from fa21f9f to faa7db7 Compare August 29, 2024 13:25
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa09cb4

Copy link
Contributor

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fa09cb4

Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa09cb4

@fanquake fanquake merged commit d4cc0c6 into bitcoin:master Sep 2, 2024
16 checks passed
@maflcko maflcko deleted the 2408-log branch September 2, 2024 11:46
@Sjors
Copy link
Member

Sjors commented Sep 3, 2024

The merge conflicts for #28521 were indeed easy to handle.

ryanofsky added a commit that referenced this pull request Dec 27, 2024
…ct logging

06443b8 net: clarify if we ever sent or received from peer (Sjors Provoost)
1d01ad4 net: add LogIP() helper, use in net_processing (Sjors Provoost)
937ef9e net_processing: use CNode::DisconnectMsg helper (Sjors Provoost)
ad22442 net: additional disconnection logging (Sjors Provoost)

Pull request description:

  While debugging unexpected disconnections, possibly related to #28331, I found some additional [net] logging to be useful.

  All cases where we disconnect now come with a log message that has the word `disconnecting`:

  * all calls to `CloseSocketDisconnect()` log `disconnecting peer=…`
  * wherever we set `pnode->fDisconnect = true;`
  * for all `InactivityCheck` cases (which in turn sets `fDisconnect`)
  * replaces "dropping" with "disconnecting" in `Network not active, dropping peer=…`

  A few exceptions are listed here: #28521 (comment)

  I changed `CloseSocketDisconnect()` to no longer log `disconnecting`, and instead have all the call sites do so.

  This PR introduces two helper functions on `CNode`: `DisconnectMsg` and `LogIP`. The second and third commit use these helpers in `net_processing.cpp` so these disconnect messages are more consistent now (e.g. some didn't log the IP). No new messages are added there though.

  The `LogIP()` helper is rarely used outside of a disconnect event, but it's available for future use.

  Any `LogPrint` this PR touches is replaced with `LogDebug` (superseded by #30750), and every `LogPrintf ` with `LogInfo`.

ACKs for top commit:
  davidgumberg:
    reACK 06443b8
  vasild:
    ACK 06443b8
  danielabrozzoni:
    ACK 06443b8
  hodlinator:
    ACK 06443b8

Tree-SHA512: 525f4c11568616e1d48455a3fcab9e923da7432377fe9230468c15403d2e9b7ce712112df8fbd547cfec01dce0d1f26107cfc1b90f78cfc1fe13e08d57b08464
pinheadmz added a commit to pinheadmz/gui-qml that referenced this pull request Jul 11, 2025
@bitcoin bitcoin locked and limited conversation to collaborators Sep 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants