backport: 0.26 batch 415 #6786
backport: 0.26 batch 415 #6786
Conversation
|
WalkthroughThis update removes the legacy Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Complexity label: Moderate
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
ci/test/00_setup_env_native_valgrind.sh (1)
9-9: Samelibclang-rt-devversion-mismatch concern as above – see previous comment.
🧹 Nitpick comments (4)
src/wallet/wallet.cpp (2)
491-498: PreferSteadyClock(monotonic) for short-lived elapsed-time measurements
ChangeWalletPassphrase()usesSystemClock::now()to compute a sub-second interval that is immediately turned into a ratio.
The wall clock can jump (NTP, VM suspend, manual change) while this function is running, skewing the elapsed value and leading to an off-by-orders-of-magnitudenDeriveIterations.A monotonic clock (e.g.
std::chrono::steady_clock) is designed for this exact scenario and is unaffected by system-time adjustments or mock-time overrides.- int64_t nStartTime = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); + const auto start = SteadyClock::now(); ... - pMasterKey.second.nDeriveIterations = static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * (100 / ((double)(TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nStartTime)))); + const auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(SteadyClock::now() - start).count(); + pMasterKey.second.nDeriveIterations = + static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * (100 / static_cast<double>(elapsed)));Same applies to the second calibration block a few lines below.
AdoptingSteadyClockeverywhere you removedGetTimeMillis()keeps behaviour deterministic, avoids mocking pitfalls, and leaves the public-time API (SystemClock) for places where absolute timestamps are really needed.
697-704: Replicate the monotonic-clock change inEncryptWallet()The two calibration loops in
EncryptWallet()repeat the exact pattern addressed above and are subject to the same wall-clock jump risk.For consistency and robustness, apply the
SteadyClock‐based pattern here as well (see previous comment for diff structure).src/llmq/chainlocks.cpp (2)
112-116: Capturenowonce to cut redundantSystemClock::now()calls
SystemClock::now()is invoked every time aCChainLockSigis inserted. While not a hot path, you already pay the cost of hashing + allocation here, so an extra call is negligible but still unnecessary. A small micro-refactor makes the intent clearer and avoids double-calling the clock helper.- if (!seenChainLocks.emplace(hash, TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())).second) { + const auto now_ms{TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())}; + if (!seenChainLocks.emplace(hash, now_ms).second) {
614-618: Reuse the same timestamp throughout oneCleanup()pass
Cleanup()callsSystemClock::now()three times in quick succession (interval check, assignment, and per-entry age test).
This is safe but a bit wasteful and slightly skews age calculations by a few microseconds. Grabbing the time once at the beginning keeps the maths consistent and removes needless work.- if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - lastCleanupTime < CLEANUP_INTERVAL) { + const auto now_ms{TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())}; + if (now_ms - lastCleanupTime < CLEANUP_INTERVAL) { return; } - lastCleanupTime = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); + lastCleanupTime = now_ms; ... - if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - it->second >= CLEANUP_SEEN_TIMEOUT) { + if (now_ms - it->second >= CLEANUP_SEEN_TIMEOUT) {No behavioural change, just leaner and more consistent.
Also applies to: 622-623
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
ci/test/00_setup_env_native_fuzz_with_valgrind.sh(1 hunks)ci/test/00_setup_env_native_valgrind.sh(1 hunks)src/bench/util_time.cpp(1 hunks)src/instantsend/instantsend.cpp(2 hunks)src/llmq/chainlocks.cpp(2 hunks)src/llmq/dkgsessionhandler.cpp(1 hunks)src/llmq/dkgsessionmgr.cpp(2 hunks)src/llmq/signing.cpp(2 hunks)src/llmq/signing_shares.cpp(1 hunks)src/masternode/sync.cpp(1 hunks)src/net.cpp(3 hunks)src/net_processing.cpp(10 hunks)src/netbase.cpp(2 hunks)src/outputtype.cpp(0 hunks)src/qt/clientmodel.cpp(1 hunks)src/rpc/net.cpp(2 hunks)src/util/time.cpp(0 hunks)src/util/time.h(1 hunks)src/wallet/wallet.cpp(2 hunks)src/wallet/wallet.h(2 hunks)test/functional/test_framework/test_framework.py(2 hunks)test/functional/tool_wallet.py(1 hunks)test/functional/wallet_basic.py(1 hunks)
💤 Files with no reviewable changes (2)
- src/outputtype.cpp
- src/util/time.cpp
🧰 Additional context used
📓 Path-based instructions (5)
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/netbase.cppsrc/llmq/signing.cppsrc/masternode/sync.cppsrc/util/time.hsrc/qt/clientmodel.cppsrc/rpc/net.cppsrc/bench/util_time.cppsrc/instantsend/instantsend.cppsrc/llmq/dkgsessionhandler.cppsrc/llmq/signing_shares.cppsrc/llmq/chainlocks.cppsrc/llmq/dkgsessionmgr.cppsrc/net_processing.cppsrc/wallet/wallet.cppsrc/wallet/wallet.hsrc/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/sync.cpp
ci/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/test/00_setup_env_native_fuzz_with_valgrind.shci/test/00_setup_env_native_valgrind.sh
src/bench/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Performance benchmarks should be placed in src/bench/ and use nanobench
Files:
src/bench/util_time.cpp
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/wallet_basic.pytest/functional/tool_wallet.pytest/functional/test_framework/test_framework.py
🧠 Learnings (11)
📓 Common learnings
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#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#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.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
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.
ci/test/00_setup_env_native_fuzz_with_valgrind.sh (2)
Learnt from: kwvg
PR: #6516
File: ci/test/00_setup_env_mac.sh:11-11
Timestamp: 2025-01-02T08:33:26.751Z
Learning: The removal of DMG support in the macOS packaging process eliminates the need for python-based scripts or python3-setuptools in the build environment. The PACKAGES variable in ci/test/00_setup_env_mac.sh is vestigial due to the use of a general-purpose Docker container defined in contrib/containers/ci/Dockerfile.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : Fuzzing harnesses should be placed in src/fuzz/
src/bench/util_time.cpp (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be placed in src/bench/ and use nanobench
test/functional/wallet_basic.py (2)
Learnt from: kwvg
PR: #6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
Learnt from: kwvg
PR: #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.
src/instantsend/instantsend.cpp (1)
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
src/llmq/signing_shares.cpp (1)
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
ci/test/00_setup_env_native_valgrind.sh (1)
Learnt from: kwvg
PR: #6516
File: ci/test/00_setup_env_mac.sh:11-11
Timestamp: 2025-01-02T08:33:26.751Z
Learning: The removal of DMG support in the macOS packaging process eliminates the need for python-based scripts or python3-setuptools in the build environment. The PACKAGES variable in ci/test/00_setup_env_mac.sh is vestigial due to the use of a general-purpose Docker container defined in contrib/containers/ci/Dockerfile.
test/functional/tool_wallet.py (1)
Learnt from: kwvg
PR: #6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
src/net_processing.cpp (1)
Learnt from: kwvg
PR: #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.
src/wallet/wallet.cpp (1)
Learnt from: kwvg
PR: #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.
src/wallet/wallet.h (1)
Learnt from: kwvg
PR: #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.
🧬 Code Graph Analysis (12)
src/netbase.cpp (2)
src/util/time.cpp (2)
now(29-38)now(29-29)src/util/time.h (1)
now(21-55)
src/llmq/signing.cpp (3)
src/util/time.cpp (2)
now(29-38)now(29-29)src/util/time.h (1)
now(21-55)src/llmq/signing.h (1)
lastCleanupTime(173-173)
src/masternode/sync.cpp (2)
src/util/time.cpp (2)
now(29-38)now(29-29)src/util/time.h (1)
now(21-55)
src/qt/clientmodel.cpp (2)
src/util/time.cpp (2)
now(29-38)now(29-29)src/util/time.h (1)
now(21-55)
src/rpc/net.cpp (2)
src/util/time.cpp (2)
now(29-38)now(29-29)src/util/time.h (1)
now(21-55)
src/bench/util_time.cpp (2)
src/util/time.cpp (2)
now(29-38)now(29-29)src/util/time.h (1)
now(21-55)
test/functional/wallet_basic.py (2)
test/functional/test_framework/test_framework.py (1)
sync_all(872-874)test/functional/test_framework/util.py (2)
assert_equal(69-74)count_bytes(237-238)
src/llmq/dkgsessionhandler.cpp (2)
src/util/time.cpp (2)
now(29-38)now(29-29)src/util/time.h (1)
now(21-55)
src/llmq/signing_shares.cpp (3)
src/net_processing.cpp (4)
now(664-664)now(972-973)SendMessages(5787-6388)SendMessages(5787-5787)src/util/time.cpp (2)
now(29-38)now(29-29)src/util/time.h (1)
now(21-55)
src/llmq/dkgsessionmgr.cpp (2)
src/util/time.cpp (2)
now(29-38)now(29-29)src/util/time.h (1)
now(21-55)
src/wallet/wallet.cpp (2)
src/util/time.cpp (2)
now(29-38)now(29-29)src/util/time.h (1)
now(21-55)
src/net.cpp (3)
src/util/time.cpp (2)
now(29-38)now(29-29)src/util/time.h (1)
now(21-55)src/net.h (2)
ForEachNode(1384-1387)AllNodes(1294-1294)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/dkgsessionmgr.cpp
[error] 331-335: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix the formatting issues.
⏰ 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). (5)
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: Lint / Run linters
🔇 Additional comments (32)
ci/test/00_setup_env_native_fuzz_with_valgrind.sh (1)
10-10: Verifylibclang-rt-devmatches the pinned Clang 18 toolchain
libclang-rt-devresolves to the default LLVM runtime for the distro. Because the script explicitly pinsCC=clang-18/CXX=clang++-18, a mismatch (e.g. default = 17) will cause missing sanitizer/RT libraries at link time and break the job.
Please confirm the CI container provideslibclang-rt-18-devor switch to that version-specific package name.src/netbase.cpp (1)
323-323: LGTM! Time function replacement implemented correctly.The replacement of
GetTimeMillis()withTicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())is consistent with the codebase-wide refactoring. Both changes maintain identical functionality while adopting the modern C++ chrono-based approach for time retrieval.Also applies to: 348-348
src/util/time.h (1)
32-32: LGTM! SystemClock alias added appropriately.The addition of
SystemClockas an alias forstd::chrono::system_clockfollows the existing naming convention in the file and provides clean abstraction for the time function replacement across the codebase.src/masternode/sync.cpp (1)
122-122: LGTM! Static initialization updated correctly.The replacement of
GetTimeMillis()withTicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())in the static variable initialization maintains identical functionality while adopting the modern chrono-based approach. The change preserves the unique sync identifier generation behavior.src/llmq/signing.cpp (1)
659-659: LGTM! Cleanup timing logic updated correctly.Both time retrieval points in the
Cleanup()method have been properly updated fromGetTimeMillis()toTicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()). The 5-second throttling mechanism for cleanup operations is preserved while adopting the modern chrono-based time retrieval approach.Also applies to: 669-669
src/qt/clientmodel.cpp (1)
266-266: LGTM! GUI throttling logic updated correctly.The conditional time retrieval for GUI update throttling has been properly updated from
GetTimeMillis()toTicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()). The optimization of only getting the time when throttling is active is preserved while adopting the modern chrono-based approach.src/bench/util_time.cpp (1)
35-35: LGTM! Consistent with codebase-wide time function standardization.The replacement of
GetTimeMillis()withTicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())is part of the broader refactoring effort to standardize time retrieval across the codebase. This change maintains the same functionality while using the new templated approach with explicit system clock usage, which is appropriate for benchmarking system time retrieval performance.src/rpc/net.cpp (3)
545-546: LGTM: Help text improvements enhance clarity.The removal of the leading newline and clarification that it's "current system time" improve the documentation quality.
553-553: LGTM: Consistent terminology clarification.Adding "system" to the description aligns with the implementation change to explicitly use system clock time.
577-577: LGTM: Proper migration to explicit system time handling.The replacement of
GetTimeMillis()withTicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())is functionally equivalent but more explicit about using system clock time. This change aligns with the broader codebase modernization described in the AI summary.src/llmq/signing_shares.cpp (1)
1491-1493: LGTM! Correct replacement of GetTimeMillis() with new time utility.The changes correctly replace
GetTimeMillis()calls withTicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()), maintaining the same timing behavior for message sending control. This is consistent with the broader codebase refactoring effort to standardize time retrieval methods.test/functional/tool_wallet.py (1)
35-35: LGTM! Good refactoring to centralize binary path management.The change from manually constructing the wallet binary path to using
self.options.bitcoinwalletis part of a beneficial test framework refactoring. This centralizes binary path setup through the newset_binary_pathsmethod, improving maintainability and consistency across functional tests.src/llmq/dkgsessionhandler.cpp (2)
304-304: LGTM - Correct time function replacement.The replacement of
GetTimeMillis()withTicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())maintains the same semantics for calculating the end time in the sleep phase logic.
310-310: LGTM - Consistent time function usage.The while loop condition correctly uses the same time function replacement, maintaining the original timing logic for the sleep duration control.
src/instantsend/instantsend.cpp (2)
159-159: LGTM - Proper timing calculation replacement.The time difference calculation for InstantSend lock timing metrics correctly uses the new time function, maintaining the same semantics for measuring the delay between transaction receipt and lock receipt.
552-552: LGTM - Consistent timestamp recording.The replacement correctly records the first-seen timestamp for transactions using the new time function, maintaining the same behavior for InstantSend timing tracking.
test/functional/wallet_basic.py (1)
279-291: LGTM! Well-structured test for sendmany with fee subtraction from multiple outputs.The new test case effectively validates the
sendmanyRPC behavior when subtracting fees from multiple addresses. The test logic is comprehensive:
- Properly creates two new addresses and sends equal amounts to both
- Correctly verifies the sender's balance decrease (10 DASH total)
- Validates the receiver's total balance increase (10 DASH minus fee)
- Ensures each individual address receives the expected amount (5 DASH minus half the fee)
This enhances test coverage for an important wallet functionality.
src/llmq/dkgsessionmgr.cpp (2)
334-334: LGTM! Modernization of time handling is consistent with codebase-wide refactor.The replacement of
GetTimeMillis()withTicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())is functionally equivalent but provides better type safety and explicitness. This change aligns with the broader modernization effort across the codebase to use C++ chrono facilities consistently.Both usage locations are appropriate:
- Line 334: Setting cache entry timestamp when storing verified contributions
- Line 381: Getting current time for cache cleanup expiration logic
Also applies to: 381-381
331-335: Cannot verify formatting automatically – run clang-format locally
CI indicates formatting discrepancies insrc/llmq/dkgsessionmgr.cpparound lines 331–335. Please run:clang-format -i src/llmq/dkgsessionmgr.cppor inspect the CI logs to apply the required formatting fixes.
src/net_processing.cpp (9)
1183-1186: LGTM!Function implementation signature correctly matches the updated declaration.
1193-1196: LGTM!The peer-specific removal logic is well-implemented. This correctly prevents cross-peer interference by only removing block requests when they match the specified peer, improving peer isolation.
1233-1233: LGTM!Using
std::nulloptis appropriate here since we want to remove the block request regardless of the requesting peer when marking a block as in flight.
3497-3501: LGTM!Using
std::nulloptis correct here. The comment clearly explains that since the block may have come from a different peer than originally requested, we should remove the request regardless of the requesting peer.
4722-4722: LGTM!Using
pfrom.GetId()is appropriate here since we're handling an invalid compact block from a specific peer and want to reset only that peer's in-flight state.
4816-4816: LGTM!Using
std::nulloptis correct here since after successfully processing the block, we want to remove any pending requests regardless of the requesting peer to prevent interference with block relay.
4848-4848: LGTM!Using
pfrom.GetId()is appropriate here since we're handling invalid block transactions from a specific peer and should only reset that peer's in-flight state.
4874-4874: LGTM!Using
pfrom.GetId()is appropriate here since we successfully processed the compact block transactions from this specific peer and should remove that peer's block request.
4956-4956: LGTM!Using
pfrom.GetId()is correct here since we're processing a block from a specific peer and should remove that peer's block request.test/functional/test_framework/test_framework.py (2)
266-281: Excellent refactoring! Clean consolidation of binary path setup.The new method effectively centralizes and extends binary path configuration. The dictionary-based approach is maintainable, and the implementation correctly preserves the original logic while adding support for
dash-utilanddash-wallet.
292-292: LGTM! Clean method call integration.The replacement of inline binary path setup with the centralized method call maintains functionality while improving code organization.
src/wallet/wallet.h (2)
577-577: LGTM! Time modernization is correctly implemented.The replacement of
GetTimeMillis()withTicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())maintains the same semantics while using the standardized system clock approach. The logic for calculating scanning duration remains correct.
1088-1088: LGTM! Consistent time function modernization.The assignment to
m_scanning_startcorrectly uses the new standardized time function, maintaining consistency with theScanningDuration()method above.
src/net_processing.cpp
Outdated
| /** Remove this block from our tracked requested blocks. Called if: | ||
| * - the block has been recieved from a peer | ||
| * - the request for the block has timed out | ||
| * If "from_peer" is specified, then only remove the block if it is in | ||
| * flight from that peer (to avoid one peer's network traffic from | ||
| * affecting another's state). | ||
| */ | ||
| void RemoveBlockRequest(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); | ||
| void RemoveBlockRequest(const uint256& hash, std::optional<NodeId> from_peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main); |
There was a problem hiding this comment.
Fix typo in comment.
Line 989 has a spelling error: "recieved" should be "received".
The function signature change looks good - adding the optional from_peer parameter enables peer-specific block request removal while maintaining compatibility.
- * - the block has been recieved from a peer
+ * - the block has been received from a peer📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Remove this block from our tracked requested blocks. Called if: | |
| * - the block has been recieved from a peer | |
| * - the request for the block has timed out | |
| * If "from_peer" is specified, then only remove the block if it is in | |
| * flight from that peer (to avoid one peer's network traffic from | |
| * affecting another's state). | |
| */ | |
| void RemoveBlockRequest(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); | |
| void RemoveBlockRequest(const uint256& hash, std::optional<NodeId> from_peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main); | |
| /** Remove this block from our tracked requested blocks. Called if: | |
| * - the block has been received from a peer | |
| * - the request for the block has timed out | |
| * If "from_peer" is specified, then only remove the block if it is in | |
| * flight from that peer (to avoid one peer's network traffic from | |
| * affecting another's state). | |
| */ | |
| void RemoveBlockRequest(const uint256& hash, std::optional<NodeId> from_peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main); |
🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 988 to 995, fix the typo in the comment
on line 989 by changing "recieved" to "received" to correct the spelling error.
| // being called before DisconnectNodes and also by the linger time | ||
| // 2. Internal socket send buffers must be flushed. This is ensured solely by the linger time | ||
| pnode->nDisconnectLingerTime = GetTimeMillis() + 100; | ||
| pnode->nDisconnectLingerTime = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) + 100; | ||
| } | ||
| if (GetTimeMillis() < pnode->nDisconnectLingerTime) { | ||
| if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) < pnode->nDisconnectLingerTime) { | ||
| // everything flushed to the kernel? |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use NodeClock (or GetTime<>) instead of raw SystemClock to keep mock-time working
SystemClock::now() bypasses g_mock_time that tests set via -mocktime.
Replacing the former GetTimeMillis() call with
TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())breaks determinism in unit / functional tests and can make time-based logic diverge between prod and test runs.
Switch back to the node-aware clock (or the existing GetTime<std::chrono::milliseconds>()) so the value still honours MockTime().
- pnode->nDisconnectLingerTime = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) + 100;
+ pnode->nDisconnectLingerTime = TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now()) + 100;
...
- if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) < pnode->nDisconnectLingerTime) {
+ if (TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now()) < pnode->nDisconnectLingerTime) {🤖 Prompt for AI Agents
In src/net.cpp around lines 2118 to 2123, replace the use of SystemClock::now()
with NodeClock::now() or use GetTime<std::chrono::milliseconds>() instead to
ensure the code respects mock time settings. This change will maintain
determinism in tests by honoring g_mock_time and prevent divergence between
production and test time-based logic.
| SocketHandler(mn_sync); | ||
| if (GetTimeMillis() - nLastCleanupNodes > 1000) { | ||
| if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nLastCleanupNodes > 1000) { | ||
| ForEachNode(AllNodes, [&](CNode* pnode) { | ||
| if (InactivityCheck(*pnode)) pnode->fDisconnect = true; | ||
| }); | ||
| nLastCleanupNodes = GetTimeMillis(); | ||
| nLastCleanupNodes = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clock source ignored mock-time – fix for predictable cleanup cadence
Same issue here: SystemClock::now() skips the mock-time layer, so the 1-second cleanup cadence in ThreadSocketHandler drifts under test. Use NodeClock::now() (or GetTime<>) instead.
- if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nLastCleanupNodes > 1000) {
+ if (TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now()) - nLastCleanupNodes > 1000) {
...
- nLastCleanupNodes = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now());
+ nLastCleanupNodes = TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SocketHandler(mn_sync); | |
| if (GetTimeMillis() - nLastCleanupNodes > 1000) { | |
| if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nLastCleanupNodes > 1000) { | |
| ForEachNode(AllNodes, [&](CNode* pnode) { | |
| if (InactivityCheck(*pnode)) pnode->fDisconnect = true; | |
| }); | |
| nLastCleanupNodes = GetTimeMillis(); | |
| nLastCleanupNodes = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); | |
| } | |
| SocketHandler(mn_sync); | |
| if (TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now()) - nLastCleanupNodes > 1000) { | |
| ForEachNode(AllNodes, [&](CNode* pnode) { | |
| if (InactivityCheck(*pnode)) pnode->fDisconnect = true; | |
| }); | |
| nLastCleanupNodes = TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now()); | |
| } |
🤖 Prompt for AI Agents
In src/net.cpp around lines 2640 to 2646, the code uses SystemClock::now() which
ignores the mock-time layer, causing the 1-second cleanup timing to drift during
tests. Replace all instances of SystemClock::now() with NodeClock::now() or use
GetTime<> to ensure the cleanup cadence respects mock-time and remains
predictable in testing.
| bool fSkipSendMessagesForMasternodes = true; | ||
| if (GetTimeMillis() - nLastSendMessagesTimeMasternodes >= 100) { | ||
| if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nLastSendMessagesTimeMasternodes >= 100) { | ||
| fSkipSendMessagesForMasternodes = false; | ||
| nLastSendMessagesTimeMasternodes = GetTimeMillis(); | ||
| nLastSendMessagesTimeMasternodes = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Message-handler timer should respect mock-time
The 100 ms pacing logic for masternode messages will ignore any mocked time after this change. Revert to NodeClock::now() (or GetTime<ms>()) to keep tests deterministic.
- if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nLastSendMessagesTimeMasternodes >= 100) {
+ if (TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now()) - nLastSendMessagesTimeMasternodes >= 100) {
...
- nLastSendMessagesTimeMasternodes = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now());
+ nLastSendMessagesTimeMasternodes = TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool fSkipSendMessagesForMasternodes = true; | |
| if (GetTimeMillis() - nLastSendMessagesTimeMasternodes >= 100) { | |
| if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nLastSendMessagesTimeMasternodes >= 100) { | |
| fSkipSendMessagesForMasternodes = false; | |
| nLastSendMessagesTimeMasternodes = GetTimeMillis(); | |
| nLastSendMessagesTimeMasternodes = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); | |
| } | |
| bool fSkipSendMessagesForMasternodes = true; | |
| if (TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now()) - nLastSendMessagesTimeMasternodes >= 100) { | |
| fSkipSendMessagesForMasternodes = false; | |
| nLastSendMessagesTimeMasternodes = TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now()); | |
| } |
🤖 Prompt for AI Agents
In src/net.cpp around lines 3652 to 3656, the timing logic uses
SystemClock::now(), which ignores mocked time and breaks test determinism.
Replace SystemClock::now() with NodeClock::now() or
GetTime<std::chrono::milliseconds>() to ensure the 100 ms pacing respects mock
time and keeps tests deterministic.
| @@ -574,7 +574,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati | |||
| void AbortRescan() { fAbortRescan = true; } | |||
| bool IsAbortingRescan() const { return fAbortRescan; } | |||
| bool IsScanning() const { return fScanningWallet; } | |||
| int64_t ScanningDuration() const { return fScanningWallet ? GetTimeMillis() - m_scanning_start : 0; } | |||
| int64_t ScanningDuration() const { return fScanningWallet ? TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - m_scanning_start : 0; } | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that required includes are present for the new time functions.
The changes use TicksSinceEpoch and SystemClock which should be defined in the included headers. Please ensure the necessary includes are available.
Also applies to: 1088-1088
🏁 Script executed:
#!/bin/bash
# Description: Check if the required time-related includes are present in the file or its dependencies
# Expected: Find definitions of TicksSinceEpoch and SystemClock
echo "Checking for TicksSinceEpoch and SystemClock definitions..."
# Search for TicksSinceEpoch definition
echo "=== Searching for TicksSinceEpoch ==="
ast-grep --pattern 'TicksSinceEpoch'
# Search for SystemClock definition
echo "=== Searching for SystemClock definition ==="
ast-grep --pattern 'SystemClock'
# Check what time-related headers are included in wallet.h
echo "=== Time-related includes in wallet.h ==="
rg -A 2 -B 2 "time|chrono" src/wallet/wallet.h
# Check util/time.h for the new definitions
echo "=== Contents of util/time.h ==="
fd -t f "time.h" src/util/ --exec cat {}Length of output: 15262
Ensure wallet.h Includes the Time Utilities
The ScanningDuration() accessor (line 577) and the assignment to m_scanning_start (line 1088) rely on TicksSinceEpoch and SystemClock, which are defined in util/time.h (and pull in <chrono>). Currently, src/wallet/wallet.h does not include this header.
Please add the following include at the top of src/wallet/wallet.h:
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@
#include <atomic>
#include <functional>
+#include <util/time.h> // for TicksSinceEpoch, SystemClock, and <chrono>
#include <vector>
#include <map>This will ensure that TicksSinceEpoch and SystemClock are properly declared before use.
🤖 Prompt for AI Agents
In src/wallet/wallet.h at line 577 and also relevant at line 1088, the code uses
TicksSinceEpoch and SystemClock from util/time.h but does not include this
header. To fix this, add #include "util/time.h" at the top of
src/wallet/wallet.h to ensure these utilities are declared before use and
prevent compilation errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/README.md (1)
314-314: Fix markdown table formatting.The documentation update correctly adds lief as a dependency, but the table row is missing the trailing pipe character.
Apply this diff to fix the markdown table formatting:
-| [`lint-python.py`](lint/lint-python.py) | [lief](https://github.com/lief-project/LIEF) +| [`lint-python.py`](lint/lint-python.py) | [lief](https://github.com/lief-project/LIEF) |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ci/lint/04_install.sh(1 hunks)ci/test/04_install.sh(1 hunks)contrib/devtools/security-check.py(1 hunks)contrib/devtools/symbol-check.py(1 hunks)contrib/devtools/test-security-check.py(1 hunks)src/Makefile.am(3 hunks)src/Makefile.qt.include(3 hunks)src/Makefile.test.include(1 hunks)test/README.md(1 hunks)test/lint/lint-python.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- contrib/devtools/symbol-check.py
- contrib/devtools/security-check.py
- src/Makefile.test.include
- ci/lint/04_install.sh
🧰 Additional context used
📓 Path-based instructions (2)
contrib/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/devtools/test-security-check.py
ci/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/test/04_install.sh
🧠 Learnings (4)
📓 Common learnings
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#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#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.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
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.
test/lint/lint-python.py (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Always run linting before commits using test/lint/all-lint.py
src/Makefile.qt.include (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
test/README.md (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Always run linting before commits using test/lint/all-lint.py
🪛 markdownlint-cli2 (0.17.2)
test/README.md
314-314: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
⏰ 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). (9)
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (9)
contrib/devtools/test-security-check.py (1)
8-8: LGTM! Clean import statement.Removing the
#type:ignorecomment standardizes the lief import and aligns with the dependency management improvements across the codebase.test/lint/lint-python.py (1)
16-16: LGTM! Consistent dependency management.Adding 'lief' to the DEPS list ensures proper dependency checking in the lint script, which aligns with the standardization of lief usage across the codebase.
src/Makefile.qt.include (4)
447-447: LGTM: Improved error handling for missing RCC tool.The explicit check for
rcc(Qt resource compiler) with a clear error message ensures the build fails immediately when the required tool is missing, rather than continuing with potentially confusing errors later.
453-453: LGTM: Consistent error handling pattern for RCC tool.This change maintains the same error handling pattern as line 447, ensuring consistent behavior across different RCC usage contexts.
466-466: LGTM: Added error handling for missing UIC tool.The explicit check for
uic(Qt user interface compiler) follows the same robust error handling pattern established for the RCC tool, improving build system reliability.
477-477: LGTM: Added error handling for missing LRELEASE tool.The explicit check for
lrelease(Qt translation release tool) completes the consistent error handling pattern across all Qt build tools, ensuring comprehensive build validation.src/Makefile.am (3)
437-438: LGTM: Improved build verbosity control for build.h generation.The change from silent command execution to using
$(AM_V_GEN)follows automake best practices for verbosity control, making the build process more transparent while still respecting the user's verbosity preferences.
1074-1074: LGTM: Added error handling for missing windres tool.The explicit check for
windres(Windows resource compiler) follows the same robust error handling pattern used for Qt tools in the companion file, ensuring consistent build failure behavior when required tools are missing.
1133-1137: LGTM: Improved build verbosity control for raw header generation.The change to use
$(AM_V_GEN)for the raw header generation rule improves build output consistency and follows automake conventions for verbosity control.
| CI_EXEC free -m -h | ||
| CI_EXEC echo "Number of CPUs \(nproc\):" \$\(nproc\) | ||
| CI_EXEC echo "Number of CPUs (nproc): $(nproc)" | ||
| CI_EXEC echo "$(lscpu | grep Endian)" |
There was a problem hiding this comment.
$(nproc) is expanded on the host, not inside the container
Because of the function definition of CI_EXEC, the command substitution is performed before the arguments are handed over to docker exec. This means the host’s CPU count is echoed, defeating the stated goal of reporting the container’s resources.
A minimal, container-side evaluation would be:
-CI_EXEC echo "Number of CPUs (nproc): $(nproc)"
+CI_EXEC bash -c 'echo "Number of CPUs (nproc): $(nproc)"'or, even simpler:
-CI_EXEC echo "Number of CPUs (nproc): $(nproc)"
+CI_EXEC bash -c 'printf "Number of CPUs (nproc): %s\n" "$(nproc)"'This defers $(nproc) expansion until the subshell running inside the container executes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CI_EXEC free -m -h | |
| CI_EXEC echo "Number of CPUs \(nproc\):" \$\(nproc\) | |
| CI_EXEC echo "Number of CPUs (nproc): $(nproc)" | |
| CI_EXEC echo "$(lscpu | grep Endian)" | |
| CI_EXEC free -m -h | |
| CI_EXEC bash -c 'echo "Number of CPUs (nproc): $(nproc)"' | |
| CI_EXEC echo "$(lscpu | grep Endian)" |
🤖 Prompt for AI Agents
In ci/test/04_install.sh around lines 99 to 101, the use of $(nproc) is expanded
on the host instead of inside the container due to how CI_EXEC is defined. To
fix this, modify the command so that the $(nproc) expansion happens inside the
container by wrapping the command in a shell invocation like sh -c 'nproc' or sh
-c "echo $(nproc)". This ensures the CPU count reported is from the container
environment, not the host.
There was a problem hiding this comment.
27674: 👀 could probably fix it in a separate commit/PR
There was a problem hiding this comment.
@UdjinM6 That makes perfect sense! Based on the project's approach I've observed in previous PRs, deferring non-critical improvements to separate PRs helps maintain focus on the primary backport objectives and avoids scope creep.
The $(nproc) expansion issue isn't breaking anything functionally - it just reports the host's CPU count instead of the container's, which is more of a minor reporting inconsistency than a critical bug.
Would you like me to create a follow-up issue to track this improvement for a future PR?
🐰 thoughtfully noting the project's consistent approach to scope management
✏️ Learnings added
Learnt from: UdjinM6
PR: dashpay/dash#6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.817Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.783Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.
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#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
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#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: 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.
|
@coderabbitai, identify which commit causes the failure in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll help you identify which commit causes the failure in 🏁 Script executed: Length of output: 0 Let me try a different approach to examine the repository and find the relevant commits. 🏁 Script executed: Length of output: 163198 Let me examine the specific changes to understand what could be causing the failure. The commit appears to be a large batch commit with many changes. 🏁 Script executed: Length of output: 265707 @PastaPastaPasta The failure in 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/feature_addrman.py (1)
119-131: Excellent improvement: Dynamic maximum calculation enhances maintainability.The replacement of hardcoded values with dynamically computed maximums using the imported constants is a significant improvement. The test logic correctly validates both negative values and values exceeding the computed maximum.
Note: There's a potential naming conflict on line 36 where
ADDRMAN_NEW_BUCKET_COUNTis redefined locally in theserialize_addrmanfunction, while the same constant is now imported. Consider renaming the local variable to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/functional/feature_addrman.py(2 hunks)test/functional/test_framework/netutil.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/functional/test_framework/netutil.py
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/feature_addrman.py
🧠 Learnings (2)
📓 Common learnings
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#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#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring 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.
test/functional/feature_addrman.py (1)
Learnt from: kwvg
PR: #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.
🧬 Code Graph Analysis (1)
test/functional/feature_addrman.py (1)
test/functional/test_framework/test_node.py (2)
assert_start_raises_init_error(603-646)ErrorMatch(50-53)
⏰ 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: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (2)
test/functional/feature_addrman.py (2)
12-12: LGTM: Import of addrman constants improves maintainability.The import of addrman sizing constants from the test framework is a good improvement that replaces hardcoded values with reusable constants.
135-148: LGTM: Consistent improvement for len_new validation.The changes mirror the improvements made to the len_tried test, replacing hardcoded values with computed maximums and adding comprehensive test coverage for both negative and excessive values. The explicit use of
ErrorMatch.FULL_REGEXis consistent and appropriate.
| @@ -1935,7 +1935,6 @@ friend class CNode; | |||
| */ | |||
| static constexpr size_t MAX_UNUSED_I2P_SESSIONS_SIZE{10}; | |||
|
|
|||
| friend struct CConnmanTest; | |||
There was a problem hiding this comment.
(all commits) does your agent lost all commit description intentionally or it's a bug?
Merge bitcoin/bitcoin#27957: net: remove unused `CConnmanTest`
Co-authored-by: fanquake <fanquake@gmail.com>
I prefer to have them for easier lookup to history without opening github
There was a problem hiding this comment.
I think it's a limitation of the fact I do each one via a PR, and then the PR is squash merged in. I think during the squash merge it is lost.
There was a problem hiding this comment.
If you look here the original commit has the full description: DashCoreAutoGuix#443
but when it's merged in via squash it is lost for some reason
2dfe0d7 to
a1f1a6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/README.md (1)
311-317: Add trailing pipe to satisfy markdownlint (MD055)
markdownlint-cliflags the newly-added row for missing a trailing|.
Keeping the table consistently inleading_and_trailingpipe style silences the linter and avoids future CI noise.-| [`lint-python.py`](lint/lint-python.py) | [lief](https://github.com/lief-project/LIEF) +| [`lint-python.py`](lint/lint-python.py) | [lief](https://github.com/lief-project/LIEF) |(Other rows in this table already violate MD055, but the linter only reports on touched lines. Adjusting just this row keeps the diff minimal while unblocking the warning.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/build-src.yml(1 hunks)ci/dash/build_src.sh(1 hunks)ci/lint/04_install.sh(1 hunks)ci/test/00_setup_env.sh(1 hunks)ci/test/00_setup_env_mac_native_x86_64.sh(1 hunks)ci/test/00_setup_env_native_fuzz_with_valgrind.sh(2 hunks)ci/test/00_setup_env_native_valgrind.sh(1 hunks)ci/test/04_install.sh(1 hunks)contrib/devtools/security-check.py(1 hunks)contrib/devtools/symbol-check.py(1 hunks)contrib/devtools/test-security-check.py(1 hunks)src/Makefile.am(3 hunks)src/Makefile.qt.include(3 hunks)src/Makefile.test.include(1 hunks)src/net.h(0 hunks)src/outputtype.cpp(0 hunks)test/README.md(1 hunks)test/functional/feature_addrman.py(2 hunks)test/functional/test_framework/netutil.py(1 hunks)test/functional/test_framework/test_framework.py(2 hunks)test/functional/tool_wallet.py(1 hunks)test/lint/lint-python.py(1 hunks)
💤 Files with no reviewable changes (2)
- src/outputtype.cpp
- src/net.h
✅ Files skipped from review due to trivial changes (9)
- .github/workflows/build-src.yml
- contrib/devtools/security-check.py
- ci/lint/04_install.sh
- ci/test/00_setup_env.sh
- ci/dash/build_src.sh
- contrib/devtools/symbol-check.py
- src/Makefile.test.include
- test/functional/test_framework/netutil.py
- ci/test/04_install.sh
🚧 Files skipped from review as they are similar to previous changes (10)
- ci/test/00_setup_env_mac_native_x86_64.sh
- contrib/devtools/test-security-check.py
- test/lint/lint-python.py
- test/functional/tool_wallet.py
- test/functional/feature_addrman.py
- src/Makefile.qt.include
- ci/test/00_setup_env_native_valgrind.sh
- src/Makefile.am
- ci/test/00_setup_env_native_fuzz_with_valgrind.sh
- test/functional/test_framework/test_framework.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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#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#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring 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.
📚 Learning: always run linting before commits using test/lint/all-lint.py...
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Always run linting before commits using test/lint/all-lint.py
Applied to files:
test/README.md
🪛 markdownlint-cli2 (0.17.2)
test/README.md
314-314: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
⏰ 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). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
| "dashd": ("dashd", "DASHD"), | ||
| "dash-cli": ("dashcli", "DASHCLI"), | ||
| "dash-util": ("dashutil", "DASHUTIL"), | ||
| "dash-wallet": ("dashwallet", "DASHWALLET"), |
There was a problem hiding this comment.
sorry, it seems as these lines should be
"dashd": ("bitcoind", "DASHD"),
"dash-cli": ("bitcoincli", "DASHCLI"),
"dash-util": ("bitcoinutil", "DASHUTIL"),
"dash-wallet": ("bitcoinwallet", "DASHWALLET"),
| CI_EXEC free -m -h | ||
| CI_EXEC echo "Number of CPUs \(nproc\):" \$\(nproc\) | ||
| CI_EXEC echo "Number of CPUs (nproc): $(nproc)" | ||
| CI_EXEC echo "$(lscpu | grep Endian)" |
There was a problem hiding this comment.
27674: 👀 could probably fix it in a separate commit/PR
Co-authored-by: fanquake <fanquake@gmail.com>
…way as others Co-authored-by: fanquake <fanquake@gmail.com> fixup: 27554 Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: Claude Code <claude@anthropic.com>
…nd jobs Co-authored-by: fanquake <fanquake@gmail.com>
… Makefiles Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: Claude Code <claude@anthropic.com>
Co-authored-by: fanquake <fanquake@gmail.com>
…ventions Co-authored-by: Claude Code <claude@anthropic.com>
Co-authored-by: fanquake <fanquake@gmail.com> Co-authored-by: Claude Code <claude@anthropic.com>
…CCACHE_SIZE` Co-authored-by: Claude Code <claude@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> fix: 28188 Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
3110e18 to
06ca9a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/README.md (1)
312-315: Fix markdown table pipe style for the newly-added row
markdownlint(MD055) flags the new line for missing a trailing pipe.
Keeping the table compliant avoids future CI noise.-| [`lint-python.py`](lint/lint-python.py) | [lief](https://github.com/lief-project/LIEF) +| [`lint-python.py`](lint/lint-python.py) | [lief](https://github.com/lief-project/LIEF) |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.github/workflows/build-src.yml(1 hunks)ci/dash/build_src.sh(1 hunks)ci/lint/04_install.sh(1 hunks)ci/test/00_setup_env.sh(1 hunks)ci/test/00_setup_env_mac_native_x86_64.sh(1 hunks)ci/test/00_setup_env_native_fuzz_with_valgrind.sh(2 hunks)ci/test/00_setup_env_native_valgrind.sh(1 hunks)ci/test/04_install.sh(1 hunks)contrib/devtools/security-check.py(1 hunks)contrib/devtools/symbol-check.py(1 hunks)contrib/devtools/test-security-check.py(1 hunks)src/Makefile.am(3 hunks)src/Makefile.qt.include(3 hunks)src/Makefile.test.include(1 hunks)src/bench/util_time.cpp(1 hunks)src/instantsend/instantsend.cpp(2 hunks)src/llmq/chainlocks.cpp(2 hunks)src/llmq/dkgsessionhandler.cpp(1 hunks)src/llmq/dkgsessionmgr.cpp(2 hunks)src/llmq/signing.cpp(2 hunks)src/llmq/signing_shares.cpp(1 hunks)src/masternode/sync.cpp(1 hunks)src/net.cpp(3 hunks)src/net.h(0 hunks)src/netbase.cpp(2 hunks)src/outputtype.cpp(0 hunks)src/qt/clientmodel.cpp(1 hunks)src/rpc/net.cpp(2 hunks)src/util/time.cpp(0 hunks)src/util/time.h(1 hunks)src/wallet/wallet.cpp(2 hunks)src/wallet/wallet.h(2 hunks)test/README.md(1 hunks)test/functional/feature_addrman.py(2 hunks)test/functional/test_framework/netutil.py(1 hunks)test/functional/test_framework/test_framework.py(2 hunks)test/functional/tool_wallet.py(1 hunks)test/lint/lint-python.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/util/time.cpp
- src/net.h
- src/outputtype.cpp
✅ Files skipped from review due to trivial changes (14)
- contrib/devtools/security-check.py
- ci/lint/04_install.sh
- ci/test/00_setup_env.sh
- .github/workflows/build-src.yml
- ci/dash/build_src.sh
- src/Makefile.test.include
- contrib/devtools/symbol-check.py
- src/llmq/chainlocks.cpp
- src/llmq/signing_shares.cpp
- src/wallet/wallet.h
- src/llmq/dkgsessionhandler.cpp
- ci/test/04_install.sh
- ci/test/00_setup_env_native_valgrind.sh
- src/net.cpp
🚧 Files skipped from review as they are similar to previous changes (20)
- src/util/time.h
- src/Makefile.qt.include
- src/qt/clientmodel.cpp
- src/netbase.cpp
- test/functional/feature_addrman.py
- contrib/devtools/test-security-check.py
- src/llmq/signing.cpp
- src/masternode/sync.cpp
- test/functional/tool_wallet.py
- src/wallet/wallet.cpp
- src/rpc/net.cpp
- ci/test/00_setup_env_mac_native_x86_64.sh
- src/bench/util_time.cpp
- test/lint/lint-python.py
- src/instantsend/instantsend.cpp
- test/functional/test_framework/netutil.py
- src/llmq/dkgsessionmgr.cpp
- src/Makefile.am
- ci/test/00_setup_env_native_fuzz_with_valgrind.sh
- test/functional/test_framework/test_framework.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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: 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#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring 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.
📚 Learning: always run linting before commits using test/lint/all-lint.py...
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Always run linting before commits using test/lint/all-lint.py
Applied to files:
test/README.md
🪛 markdownlint-cli2 (0.17.2)
test/README.md
314-314: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
⏰ 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). (9)
- GitHub Check: linux64_sqlite-test / Test source
- GitHub Check: linux64_nowallet-test / Test source
- GitHub Check: linux64-test / Test source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: arm-linux-build / Build source
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.