Skip to content

feat: remove useless debug logs and improve mocktime logging #6707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

knst
Copy link
Collaborator

@knst knst commented Jun 3, 2025

Issue being fixed or feature implemented

I have been lately watching to debug logs really a lot while investigating various flackiness in functional tests, and there are some adjustment that I found as useful.

What was done?

  • removed remove mapSendableNodes an remove mapReceivableNodes lines that are triggered for almost each received network package

There's still plenty logs about network connection, but that particular line is useless.

 node0 2025-06-03T06:50:11.185505Z (mocktime: 2014-12-04T17:15:48Z) [       net] [net.cpp:2029] [CreateNodeFromAcceptedSocket] [net|netconn] connection accepted, sock=46, peer=0 
[REMOVED] node0 2025-06-03T06:50:11.185760Z (mocktime: 2014-12-04T17:15:48Z) [       net] [net.cpp:2798] [SocketHandlerConnected] [net] SocketHandlerConnected -- remove mapReceivableNodes, peer=0 
 node0 2025-06-03T06:50:11.185877Z (mocktime: 2014-12-04T17:15:48Z) [   msghand] [net_processing.cpp:3518] [ProcessMessage] [net] received: version (148 bytes) peer=0 
 node0 2025-06-03T06:50:11.185927Z (mocktime: 2014-12-04T17:15:48Z) [   msghand] [net.cpp:4967] [PushMessage] [net] sending version (148 bytes) peer=0 
 node0 2025-06-03T06:50:11.185959Z (mocktime: 2014-12-04T17:15:48Z) [   msghand] [net_processing.cpp:1468] [PushNodeVersion] [net] send version message: version 70237, blocks=113, txrelay=1, peer=0 
 node0 2025-06-03T06:50:11.185977Z (mocktime: 2014-12-04T17:15:48Z) [   msghand] [net.cpp:4967] [PushMessage] [net] sending sendaddrv2 (0 bytes) peer=0 
 node0 2025-06-03T06:50:11.186004Z (mocktime: 2014-12-04T17:15:48Z) [   msghand] [net.cpp:4967] [PushMessage] [net] sending verack (0 bytes) peer=0 
  • removed multiple lines from CQuorumBlockProcessor which are logged for each block but doesn't really help to understand why any quorum is failed to form, not bringing any useful input. Also
    CQuorumBlockProcessor::ProcessCommitment is simplified to just ProcessCommitment
[REMOVED] node0 2025-06-03T06:50:08.720090Z (mocktime: 2014-12-04T17:15:38Z) [httpworker.1] [llmq/blockprocessor.cpp:412] [IsMiningPhase] [llmq] [IsMiningPhase] nHeight[2] llmqType[100] quorumCycleStartHeight[0] -- NOT mining[10-18] 
[REMOVED] node0 2025-06-03T06:50:08.720101Z (mocktime: 2014-12-04T17:15:38Z) [httpworker.1] [llmq/blockprocessor.cpp:412] [IsMiningPhase] [llmq] [IsMiningPhase] nHeight[2] llmqType[103] quorumCycleStartHeight[0] -- NOT mining[12-20] 
[REMOVED] node0 2025-06-03T06:50:08.720112Z (mocktime: 2014-12-04T17:15:38Z) [httpworker.1] [llmq/blockprocessor.cpp:412] [IsMiningPhase] [llmq] [IsMiningPhase] nHeight[2] llmqType[106] quorumCycleStartHeight[0] -- NOT mining[10-18] 

[OLD] node0 2025-06-03T06:50:13.768237Z (mocktime: 2014-12-04T17:15:56Z) [httpworker.1] [llmq/blockprocessor.cpp:236] [ProcessCommitment] [llmq] CQuorumBlockProcessor::ProcessCommitment height=114, type=100, quorumIndex=0, quorumHash=224adea5fe8b95219555319e09cc4509ff550343b306a74fcf57dac9c380c6f2, signers=0, validMembers=0, quorumPublicKey=000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 fJustCheck[1] processing commitment from block. 
[NEW]  node0 2025-06-03T06:51:07.902742Z (mocktime: 1417713341) [httpworker.1] [llmq/blockprocessor.cpp:250] [ProcessCommitment] [llmq] ProcessCommitment -- height=34, type=100, quorumIndex=0, quorumHash=793bfcce397cc0ab4a685f4fe2d2a4b71ce06402813ea1f55e626ad2f1acbe9b, signers=0, validMembers=0, quorumPublicKey=000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 fJustCheck[1] processing commitment from block. 
  • removed logs for consequent masternode payments (which is always 0 since mn_rr activated)
[REMOVED] node0 2025-06-03T06:50:20.938625Z (mocktime: 2014-12-04T17:16:09Z) [httpworker.0] [evo/deterministicmns.cpp:977] [BuildNewListFromBlock] [mnpayments] CDeterministicMNManager::BuildNewListFromBlock -- MN 8e26622b3336527f126324c71fb169b9be741ed5351832c28007571cbe864b28, nConsecutivePayments=0
  • mocktime is shown now in seconds since 1970. Comparing strings 2014-12-04T17:15:37Z and 2014-12-04T17:15:38Z I find difficult to do, especially if mocktime bumped for more than 1 second or multiple times; it requires to do some math also over module 60 (60 seconds in minute).
[OLD] node0 2025-06-03T06:47:49.402583Z (mocktime: 2014-12-04T17:15:37Z) [httpworker.3] [rpc/request.cpp:180] [parse] [rpc] ThreadRPCServer method=setmocktime user=__cookie__
[OLD] node0 2025-06-03T06:47:49.402632Z (mocktime: 2014-12-04T17:15:38Z) [httpworker.3] [httprpc.cpp:93] [~RpcHttpRequest] [bench] HTTP RPC request handled: user=__cookie__ command=setmocktime external=false status=200 elapsed_time_ms=0 
[NEW] node0 2025-06-03T06:46:35.684642Z (mocktime: 1417713337) [httpworker.2] [rpc/request.cpp:180] [parse] [rpc] ThreadRPCServer method=setmocktime user=__cookie__ 
[NEW] node0 2025-06-03T06:46:35.684674Z (mocktime: 1417713338) [httpworker.2] [httprpc.cpp:93] [~RpcHttpRequest] [bench] HTTP RPC request handled: user=__cookie__ command=setmocktime external=false status=200 elapsed_time_ms=0 

How Has This Been Tested?

It makes reading by eyes much more friendly because less items to read and lines a bit shorter, and less matches by block hash / protx hashes when you are looking for some particular records.

As positive side effect, total size of all logs of all functional tests is decreased for ~200Mb (1.7Gb -> 1.5Gb).

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

knst added 4 commits June 3, 2025 13:35
…eNodes' which triggers on each send / receive bytes
… member `IsMiningPhase` be anonymous function
Firstly, production code (even in debug mode) should not operate as unit test by calling heavy GetMN
just to be sure that implementation is not broken.

Secondly, since mn_rr hard-fork is activated it doesn't affect any new blocks; but it is used for
validation of old blocks only.

Thirdly, it removes useless log record (especially now when no more 4x payments for Evo Nodes).

    2025-05-31T19:40:51.578629Z (mocktime: 2014-12-04T17:29:28Z) [httpworker] [evo/deterministicmns.cpp:1023] [BuildNewListFromBlock] [mnpayments] CDeterministicMNManager::BuildNewListFromBlock -- MN ae67835776b01b4ad9b153c829bf30fe70e8c3699a09ddcebe431712c38cee63, nConsecutivePayments=0
Firstly, it's easier to spot changes in mocktime and calculate how long if time is logged in seconds.

Secondly, the start time `2014-12-04T17:15:37Z` (genesis block) is not round number which makes
calculation of "how much exactly time pass" even more difficult.
@knst knst added this to the 23 milestone Jun 3, 2025
Copy link

coderabbitai bot commented Jun 3, 2025

Walkthrough

This set of changes primarily focuses on reducing and simplifying debug logging across several source files. In src/evo/deterministicmns.cpp, a debug block that logged masternode payment information was removed. In src/llmq/blockprocessor.cpp and its header, logging was streamlined by removing verbose or redundant log statements, and the IsMiningPhase function was refactored from a class member to a static free function with explicit lock requirements and no logging. The formatting of mocktime in log messages within src/logging.cpp was changed from a human-readable date-time to a numeric timestamp. Finally, in src/net.cpp, debug logs related to node map modifications were deleted, leaving the underlying logic intact. No changes were made to public APIs except for the removal and refactoring of the IsMiningPhase function.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4930ee6 and 1da3eb7.

📒 Files selected for processing (5)
  • src/evo/deterministicmns.cpp (0 hunks)
  • src/llmq/blockprocessor.cpp (5 hunks)
  • src/llmq/blockprocessor.h (0 hunks)
  • src/logging.cpp (1 hunks)
  • src/net.cpp (0 hunks)
💤 Files with no reviewable changes (3)
  • src/llmq/blockprocessor.h
  • src/evo/deterministicmns.cpp
  • src/net.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/llmq/blockprocessor.cpp (1)
src/llmq/blockprocessor.h (5)
  • llmqParams (65-65)
  • llmqParams (66-66)
  • llmqParams (79-79)
  • llmqParams (80-80)
  • nHeight (78-78)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/blockprocessor.cpp

[error] 220-330: Clang format differences found. The file does not adhere to the required code style and formatting rules. Please run clang-format to fix formatting issues.

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build container / Build container
  • GitHub Check: Build slim container / Build container
🔇 Additional comments (3)
src/logging.cpp (1)

405-405: LGTM! Excellent simplification of mocktime logging format.

The change from a verbose human-readable format to epoch seconds makes the logs more compact and easier to parse, which aligns perfectly with the PR objective of reducing log verbosity while maintaining debugging utility.

src/llmq/blockprocessor.cpp (2)

223-235: LGTM! Well-executed refactoring of IsMiningPhase to a static function.

The refactoring from a class member function to a static function is well-implemented:

  • Proper lock annotations with EXCLUSIVE_LOCKS_REQUIRED(cs_main)
  • Appropriate lock assertion AssertLockHeld(cs_main)
  • Clear parameter interface instead of relying on class state
  • Correct mining phase calculation logic

This change improves code clarity and removes the logging that was mentioned in the PR objectives.


243-325: LGTM! Consistent log message simplification.

The removal of the CQuorumBlockProcessor:: prefix from log messages and using just __func__ is a good simplification that:

  • Reduces log verbosity as intended by the PR
  • Maintains sufficient context for debugging
  • Creates consistency across the logging statements
  • Makes logs more readable
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@knst knst requested review from UdjinM6 and PastaPastaPasta June 3, 2025 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant