fix: Various log improvements #6800
Conversation
show penalty changes when banning MNs
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThe changes across multiple source files primarily update logging behavior for clarity and specificity. In Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/masternode/payments.cpp (1)
134-135: Enhanced error logging with payment amount.Good improvement to include the payment amount in the error message. This provides valuable context for debugging payment validation failures.
Consider using
%lldinstead of%dfor the amount parameter to be more explicit about the 64-bit integer format, consistent with other logging in the codebase:- LogPrintf("CMNPaymentsProcessor::%s -- ERROR! Failed to find expected payee %s amount=%d height=%d\n", + LogPrintf("CMNPaymentsProcessor::%s -- ERROR! Failed to find expected payee %s amount=%lld height=%d\n",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/evo/deterministicmns.cpp(2 hunks)src/evo/specialtxman.cpp(3 hunks)src/masternode/payments.cpp(1 hunks)src/net.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/payments.cppsrc/evo/specialtxman.cppsrc/evo/deterministicmns.cppsrc/net.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/payments.cppsrc/evo/specialtxman.cppsrc/evo/deterministicmns.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: UdjinM6
PR: dashpay/dash#6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/net.cpp:4329-4329
Timestamp: 2025-01-14T09:07:12.446Z
Learning: Keep suggestions focused on the scope of the current commit/PR. Avoid suggesting unrelated improvements that should be handled in separate PRs, even if technically valid.
Learnt from: knst
PR: dashpay/dash#6533
File: test/functional/feature_llmq_singlenode.py:98-106
Timestamp: 2025-01-22T08:33:31.405Z
Learning: When reviewing PRs, ensure that suggestions are directly related to the PR's primary objectives rather than general code improvements that could be addressed separately.
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
📚 Learning: in dash's masternode transaction validation, `isversionchangevalid()` is only called by transaction ...
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Applied to files:
src/masternode/payments.cppsrc/evo/deterministicmns.cpp
📚 Learning: in the test framework consolidation pr (#6718), user kwvg prefers to limit functional changes to tho...
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/masternode/payments.cppsrc/evo/deterministicmns.cppsrc/net.cpp
📚 Learning: the `getwallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null chec...
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/masternode/payments.cpp
📚 Learning: pull request #6543 is focused on move-only changes and refactoring, specifically backporting from bi...
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Applied to files:
src/masternode/payments.cpp
📚 Learning: applies to src/evo/specialtx.h : special transactions use payload extensions defined in src/evo/spec...
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Applied to files:
src/evo/specialtxman.cpp
📚 Learning: the functions `migratedbifneeded` and `migratedbifneeded2` in `src/evo/deterministicmns.cpp` are tem...
Learnt from: knst
PR: dashpay/dash#6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/evo/deterministicmns.cpp
📚 Learning: applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : masternode lists must use immutable data s...
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/evo/deterministicmns.cpp
📚 Learning: in the current implementation of `netinfoentry`, only `cservice` is a valid type and `std::monostate...
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/net.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (12)
src/net.cpp (6)
1927-1927: Granular logging category is a welcome improvementSwitching from the generic
BCLog::NETto the more focusedBCLog::NET_NETCONNwill make connection-level diagnostics easier to filter.
No further issues spotted.
1948-1948: Consistent category for banned-peer drops👍 The new category keeps all connection-drop events together. Looks good.
1956-1956: Discouraged-peer reject now logs under NET_NETCONNChange is correct and in line with the PR objective.
No additional action required.
1969-1969: Eviction-failure message moved to NET_NETCONNGood catch—this log belongs with the other connection handling messages.
1977-1977: Inbound skip due to unsynced chain now grouped correctlyLGTM. Behaviour unchanged; only category refined.
2094-2094: Disconnect notice re-categorisedAppropriate use of the new logging channel; no concerns.
src/masternode/payments.cpp (1)
130-133: LGTM! Improved logging clarity.The addition of descriptive prefixes ("address=" and "scriptPubKey=") makes the error logs more informative and easier to parse for debugging purposes.
src/evo/specialtxman.cpp (3)
585-585: LGTM! More descriptive benchmark logging.Adding ".ProcessBlock" to the log message makes it clearer which specific operation is being timed, improving debugging and performance monitoring.
604-605: LGTM! Consistent benchmark logging improvement.The addition of ".ProcessBlock" maintains consistency with other benchmark log messages and clearly identifies the specific operation being timed.
659-660: LGTM! Completes consistent benchmark logging improvements.This change maintains the consistent pattern of making benchmark logs more descriptive by clearly identifying the specific ProcessBlock operation being timed across all processing components.
src/evo/deterministicmns.cpp (2)
314-323: LGTM! Excellent logging improvements.The changes enhance both logic and diagnostics:
- Logic improvement: Only attempts to ban masternodes that aren't already banned (Line 314)
- Enhanced logging: Now consistently logs punishment/banning activity when
debugLogsis enabled- Better visibility: Log messages now differentiate between "banned" vs "punished" states and include detailed penalty progression (before→after, max)
These changes align perfectly with the PR's objective of improving logging clarity and specificity.
608-608: LGTM! More accurate error message.Removing "duplicate" from the error message is appropriate since this error occurs during masternode deletion when the unique property deletion operation fails, not when encountering a duplicate. This improves message accuracy and aligns with the PR's goal of enhancing logging clarity.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Should help debugging when you have a payment with a correct address but with an invalid amount.
|
Applied suggestion from #6800 (review) |
| } | ||
| LogPrintf("CMNPaymentsProcessor::%s -- ERROR! Failed to find expected payee %s in block at height %s\n", | ||
| __func__, str_payout, nBlockHeight); | ||
| LogPrintf("CMNPaymentsProcessor::%s -- ERROR! Failed to find expected payee %s amount=%lld height=%d\n", |
There was a problem hiding this comment.
nit: coderabbit doesn't know what it says, you should not put here "%lld".
Consider using %lld instead of %d for the amount parameter to be more explicit about the 64-bit integer format, consistent with other logging in the codebase:
It is not true; tinyformat ignores wide of type specified here, see https://github.com/c42f/tinyformat/blob/master/tinyformat_test.cpp#L250-L256
// Check that length modifiers are ignored
Because tinyformat uses template output '<<' to determine wideness of type
3 participants
Issue being fixed or feature implemented
Pls see individual commits
What was done?
How Has This Been Tested?
Check logs
Breaking Changes
n/a
Checklist: