Merge bitcoin/bitcoin#27405: util: Use steady clock instead of system clock to measure durations #1144
Conversation
…o measure durations fa83fb3 wallet: Use steady clock to calculate number of derive iterations (MarcoFalke) fa2c099 wallet: Use steady clock to measure scanning duration (MarcoFalke) fa97621 qt: Use steady clock to throttle GUI notifications (MarcoFalke) fa1d804 test: Use steady clock in index tests (MarcoFalke) fa454dc net: Use steady clock in InterruptibleRecv (MarcoFalke) Pull request description: `GetTimeMillis` has multiple issues: * It doesn't denote the underlying clock type * It isn't type-safe * It is used incorrectly in places that should use a steady clock Fix all issues here. ACKs for top commit: willcl-ark: ACK fa83fb3 martinus: Code review ACK bitcoin@fa83fb3, also ran all tests. All usages of the steady_clock are just for duration measurements, so the change to a different epoch is ok. Tree-SHA512: 5ec4fede8c7f97e2e08863c011856e8304f16ba30a68fdeb42f96a50a04961092cbe46ccf9ea6ac99ff5203c09f9e0924eb483eb38d7df0759addc85116c8a9f
WalkthroughThis change migrates various timeouts and timing logic to std::chrono and steady clock across networking, GUI, wallet, and tests. It updates SOCKS5 receive timeout types, throttling mechanics, wallet scanning state/duration APIs, derives iteration timing, RPC duration reporting, and replaces index sync helpers with explicit polling loops. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/clientmodel.cpp (1)
271-279: Use .count() for QTimer interval calls with MODEL_UPDATE_DELAY
- QTimer::setInterval and QTimer::start expect an integer millisecond count, but MODEL_UPDATE_DELAY is a std::chrono::milliseconds. Update as follows:
- src/qt/clientmodel.cpp (line 56):
timer->setInterval(MODEL_UPDATE_DELAY.count());- src/qt/peertablemodel.cpp (line 24):
timer->setInterval(MODEL_UPDATE_DELAY.count());- src/qt/walletmodel.cpp (line 80):
timer->start(MODEL_UPDATE_DELAY.count());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
src/netbase.cpp(4 hunks)src/qt/clientmodel.cpp(2 hunks)src/test/blockfilter_index_tests.cpp(1 hunks)src/test/fuzz/socks5.cpp(2 hunks)src/test/txindex_tests.cpp(1 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/wallet.cpp(2 hunks)src/wallet/wallet.h(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
C++20 codebase should be placed under src/
Files:
src/test/blockfilter_index_tests.cppsrc/qt/clientmodel.cppsrc/wallet/rpc/wallet.cppsrc/wallet/wallet.hsrc/test/txindex_tests.cppsrc/netbase.cppsrc/wallet/wallet.cppsrc/test/fuzz/socks5.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/blockfilter_index_tests.cppsrc/test/txindex_tests.cppsrc/test/fuzz/socks5.cpp
**
⚙️ CodeRabbit configuration file
**: # CodeRabbit AI Review Instructions for Dash BackportsYour Role
You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.
Critical Validation Rules
1. File Operations Must Match (AUTO-REJECT if violated)
- If Bitcoin modifies an existing file → Dash MUST modify (not create new)
- If Bitcoin creates a new file → Dash creates
- If Bitcoin deletes a file → Dash deletes
- Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys
2. Size Ratio Check (80-150% of Bitcoin)
- Count functional lines changed (exclude comments/whitespace)
- Dash changes should be 80-150% of Bitcoin's size
- Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash
3. No Scope Creep
- Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
- No unrelated refactoring or style changes
- Only Bitcoin's intended changes + minimal Dash adaptations
4. Bitcoin-Specific Code Detection
- Auto-reject witness/segwit code:
msg_wtxidrelay,MSG_WTX, witness imports- Auto-reject RBF (replace-by-fee) functionality
- Note: PSBT is supported in Dash (don't flag)
5. Mandatory Adaptations Only
bitcoin→dashin strings/pathsBitcoin→Dashin user-facing text- Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
- Hardcoded test values specific to Dash
- No other changes unless absolutely required
6. Completeness Check
- All files changed in Bitcoin must be present
- Extra files need clear justification (Dash-specific compatibility)
- Missing files = incomplete backport
Review Process
- First: Check file operations match exactly
- Second: Calculate size ratio
- Third: Scan for scope creep patterns
- Fourth: Detect Bitcoin-specific code
- Fifth: Verify all changes are minimal adaptations
Output Format
VALIDATION: [PASS/FAIL] File Operatio...
Files:
src/test/blockfilter_index_tests.cppsrc/qt/clientmodel.cppsrc/wallet/rpc/wallet.cppsrc/wallet/wallet.hsrc/test/txindex_tests.cppsrc/netbase.cppsrc/wallet/wallet.cppsrc/test/fuzz/socks5.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
🧬 Code graph analysis (3)
src/qt/clientmodel.cpp (1)
src/util/time.h (1)
now(21-55)
src/wallet/wallet.h (1)
src/util/time.h (1)
now(21-55)
src/wallet/wallet.cpp (1)
src/util/time.h (1)
now(21-55)
🔇 Additional comments (12)
src/test/txindex_tests.cpp (1)
34-39: LGTM! Steady clock migration correctly implemented.The replacement of
IndexWaitSynced(txindex)with an explicit polling loop usingSteadyClock::now()for timeout tracking is correct and aligns with the PR objective to migrate from system clock to steady clock for duration measurements.src/test/blockfilter_index_tests.cpp (1)
145-150: LGTM! Consistent steady clock implementation.The migration to
SteadyClock::now()for timeout tracking in the polling loop is correct and consistent with the identical change intxindex_tests.cpp. Both files now use steady clock instead of system clock for synchronization timing.src/qt/clientmodel.cpp (1)
34-35: Steady clock globals LGTMStatic SteadyClock::time_point vars are an appropriate replacement for ms-since-epoch ints; scope and defaults look fine.
src/wallet/wallet.h (1)
580-582: Accessors align with steady-clock migrationReturning a chrono duration and computing it against an atomic start time-point is fine once the include is added.
Optional: consider documenting the unit expectation for callers (RPC uses seconds).
src/wallet/rpc/wallet.cpp (1)
264-266: Steady duration formatting LGTMUsing
Ticks<std::chrono::seconds>(pwallet->ScanningDuration())correctly reports elapsed seconds with the new steady-clock duration.src/test/fuzz/socks5.cpp (1)
37-38: Remove unnecessary chrono change suggestion
socks5.cppalready includestest/util/setup_common.h, which pulls inusing namespace std::chrono_literals;fromutil/time.h, so the1msliteral is valid—no change needed.Likely an incorrect or invalid review comment.
src/netbase.cpp (6)
38-39: LGTM - Type migration to std::chrono::millisecondsThe global timeout variable has been correctly migrated from an integer (representing milliseconds) to
std::chrono::millisecondswith a chrono literal20s. This aligns with the PR objective of using steady clock for duration measurements.
303-303: LGTM - Documentation updated for type-safe parameterComment appropriately simplified by removing "in milliseconds" since the parameter type
std::chrono::millisecondsis now self-documenting.
313-313: LGTM - Function signature updated for type-safe timeoutThe
InterruptibleRecvfunction signature correctly updated to acceptstd::chrono::millisecondsinstead ofint, providing type safety for timeout handling.
315-316: LGTM - Critical migration to steady clock for reliable timeoutsThis is the core change implementing the PR objective. The timing logic now uses
Now<SteadyMilliseconds>()(steady clock) instead of system clock, ensuring that socket timeouts are not affected by system time adjustments. This is particularly important for long-running operations like Tor proxy negotiations.
340-340: LGTM - Consistent steady clock usage in loopThe
curTimeupdate correctly usesNow<SteadyMilliseconds>()to maintain consistency with the steady clock approach throughout the timeout loop.
38-340: BACKPORT VALIDATION: PASSFile Operations: ✓ MATCH - Bitcoin modifies existing file, Dash modifies existing file
Size Ratio: Appropriate - 5 functional lines changed, all directly related to clock type migration
Scope Creep: ✓ NONE - All changes strictly related to migrating from system clock (int milliseconds) to steady clock (std::chrono::milliseconds)
Bitcoin-Specific Code: ✓ NONE - No witness, segwit, RBF, or other Bitcoin-specific features
Completeness: ✓ COMPLETE for this file - All timeout-related code in InterruptibleRecv and g_socks5_recv_timeout properly migrated
Changes Summary:
- Global timeout variable migrated to type-safe chrono duration
- Function signature updated for type safety
- Timing calculations switched from epoch-based to steady-time points
- Documentation updated appropriately
This is a faithful, minimal backport of Bitcoin Core PR bitcoin#27405 for the netbase.cpp component. The changes ensure socket timeouts are immune to system clock adjustments, which is critical for reliable network operations especially with slow proxies like Tor.
| extern std::chrono::milliseconds g_socks5_recv_timeout; | ||
|
|
||
| namespace { | ||
| int default_socks5_recv_timeout; | ||
| decltype(g_socks5_recv_timeout) default_socks5_recv_timeout; | ||
| }; |
There was a problem hiding this comment.
Add required header for chrono types
This TU now uses std::chrono::milliseconds; explicitly include to avoid relying on transitive includes:
+#include <chrono>📝 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.
| extern std::chrono::milliseconds g_socks5_recv_timeout; | |
| namespace { | |
| int default_socks5_recv_timeout; | |
| decltype(g_socks5_recv_timeout) default_socks5_recv_timeout; | |
| }; | |
| #include <chrono> | |
| extern std::chrono::milliseconds g_socks5_recv_timeout; | |
| namespace { | |
| decltype(g_socks5_recv_timeout) default_socks5_recv_timeout; | |
| }; |
🤖 Prompt for AI Agents
In src/test/fuzz/socks5.cpp around lines 16 to 20, the translation unit uses
std::chrono::milliseconds but does not include the <chrono> header; add an
explicit #include <chrono> at the top of the file (or before any use of
std::chrono types) so the code does not rely on transitive includes and builds
correctly across compilers.
| constexpr MillisecondsDouble target{100}; | ||
| auto start{SteadyClock::now()}; | ||
| crypter.SetKeyFromPassphrase(strNewWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod); | ||
| pMasterKey.second.nDeriveIterations = static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * (100 / ((double)(TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nStartTime)))); | ||
| pMasterKey.second.nDeriveIterations = static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * target / (SteadyClock::now() - start)); | ||
|
|
||
| nStartTime = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); | ||
| start = SteadyClock::now(); | ||
| crypter.SetKeyFromPassphrase(strNewWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod); | ||
| pMasterKey.second.nDeriveIterations = (pMasterKey.second.nDeriveIterations + static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * 100 / ((double)(TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nStartTime)))) / 2; | ||
| pMasterKey.second.nDeriveIterations = (pMasterKey.second.nDeriveIterations + static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * target / (SteadyClock::now() - start))) / 2; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Use a standard chrono type; avoid undefined MillisecondsDouble
MillisecondsDouble may not exist in Dash’s util/time.h. Prefer std::chrono types:
- constexpr MillisecondsDouble target{100};
+ constexpr std::chrono::milliseconds target{100};
auto start{SteadyClock::now()};
...
- pMasterKey.second.nDeriveIterations = static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * target / (SteadyClock::now() - start));
+ pMasterKey.second.nDeriveIterations = static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * (target / (SteadyClock::now() - start)));
...
- start = SteadyClock::now();
+ start = SteadyClock::now();
...
- pMasterKey.second.nDeriveIterations = (pMasterKey.second.nDeriveIterations + static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * target / (SteadyClock::now() - start))) / 2;
+ pMasterKey.second.nDeriveIterations = (pMasterKey.second.nDeriveIterations + static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * (target / (SteadyClock::now() - start)))) / 2;This keeps the intended ratio (100 ms per measurement window) while eliminating a likely missing alias.
Quick check for alias presence:
🏁 Script executed:
#!/bin/bash
rg -n 'MillisecondsDouble' src/util src/walletLength of output: 155
🏁 Script executed:
#!/bin/bash
set -e
# Locate util/time.h
file=$(fd --full-path time.h src/util)
echo "Inspecting $file"
# Check for Double aliases
rg -n 'using .*Double' "$file"
rg -n 'typedef .*Double' "$file"Length of output: 194
Replace undefined MillisecondsDouble with std::chrono::duration<double, std::milli>
MillisecondsDouble isn’t defined in src/util/time.h. Replace both occurrences with a double-precision duration and wrap the ratio in parentheses for correct precision:
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ lines 497,704
- constexpr MillisecondsDouble target{100};
+ constexpr std::chrono::duration<double, std::milli> target{100.0};
@@ lines 500,707
- pMasterKey.second.nDeriveIterations = static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * target / (SteadyClock::now() - start));
+ pMasterKey.second.nDeriveIterations = static_cast<unsigned int>(
+ pMasterKey.second.nDeriveIterations * (target / (SteadyClock::now() - start))
+ );
@@ lines 504,711
- pMasterKey.second.nDeriveIterations = (pMasterKey.second.nDeriveIterations + static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * target / (SteadyClock::now() - start))) / 2;
+ pMasterKey.second.nDeriveIterations = static_cast<unsigned int>(
+ (pMasterKey.second.nDeriveIterations +
+ pMasterKey.second.nDeriveIterations * (target / (SteadyClock::now() - start)))
+ / 2
+ );This eliminates the undefined alias while preserving fractional-millisecond precision.
🤖 Prompt for AI Agents
In src/wallet/wallet.cpp around lines 497 to 505, the code uses an undefined
MillisecondsDouble type; replace both occurrences with
std::chrono::duration<double, std::milli> to preserve fractional-millisecond
precision, and wrap the division used to scale nDeriveIterations in parentheses
so the ratio is computed in double precision (e.g. static_cast<unsigned
int>(pMasterKey.second.nDeriveIterations * (target / (SteadyClock::now() -
start)))); update both lines that compute nDeriveIterations accordingly.
| CCrypter crypter; | ||
| int64_t nStartTime = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); | ||
| constexpr MillisecondsDouble target{100}; | ||
| auto start{SteadyClock::now()}; | ||
| crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, 25000, kMasterKey.nDerivationMethod); | ||
| kMasterKey.nDeriveIterations = static_cast<unsigned int>(2500000 / ((double)(TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nStartTime))); | ||
| kMasterKey.nDeriveIterations = static_cast<unsigned int>(25000 * target / (SteadyClock::now() - start)); | ||
|
|
||
| nStartTime = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); | ||
| start = SteadyClock::now(); | ||
| crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, kMasterKey.nDeriveIterations, kMasterKey.nDerivationMethod); | ||
| kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + static_cast<unsigned int>(kMasterKey.nDeriveIterations * 100 / ((double)(TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nStartTime)))) / 2; | ||
| kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + static_cast<unsigned int>(kMasterKey.nDeriveIterations * target / (SteadyClock::now() - start))) / 2; | ||
|
|
||
| if (kMasterKey.nDeriveIterations < 25000) |
There was a problem hiding this comment.
Same MillisecondsDouble issue here
Mirror the change as above:
- constexpr MillisecondsDouble target{100};
+ constexpr std::chrono::milliseconds target{100};
auto start{SteadyClock::now()};
...
- kMasterKey.nDeriveIterations = static_cast<unsigned int>(25000 * target / (SteadyClock::now() - start));
+ kMasterKey.nDeriveIterations = static_cast<unsigned int>(25000 * (target / (SteadyClock::now() - start)));
...
- start = SteadyClock::now();
+ start = SteadyClock::now();
...
- kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + static_cast<unsigned int>(kMasterKey.nDeriveIterations * target / (SteadyClock::now() - start))) / 2;
+ kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + static_cast<unsigned int>(kMasterKey.nDeriveIterations * (target / (SteadyClock::now() - start)))) / 2;📝 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.
| CCrypter crypter; | |
| int64_t nStartTime = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); | |
| constexpr MillisecondsDouble target{100}; | |
| auto start{SteadyClock::now()}; | |
| crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, 25000, kMasterKey.nDerivationMethod); | |
| kMasterKey.nDeriveIterations = static_cast<unsigned int>(2500000 / ((double)(TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nStartTime))); | |
| kMasterKey.nDeriveIterations = static_cast<unsigned int>(25000 * target / (SteadyClock::now() - start)); | |
| nStartTime = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); | |
| start = SteadyClock::now(); | |
| crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, kMasterKey.nDeriveIterations, kMasterKey.nDerivationMethod); | |
| kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + static_cast<unsigned int>(kMasterKey.nDeriveIterations * 100 / ((double)(TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nStartTime)))) / 2; | |
| kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + static_cast<unsigned int>(kMasterKey.nDeriveIterations * target / (SteadyClock::now() - start))) / 2; | |
| if (kMasterKey.nDeriveIterations < 25000) | |
| CCrypter crypter; | |
| constexpr std::chrono::milliseconds target{100}; | |
| auto start{SteadyClock::now()}; | |
| crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, 25000, kMasterKey.nDerivationMethod); | |
| kMasterKey.nDeriveIterations = static_cast<unsigned int>(25000 * (target / (SteadyClock::now() - start))); | |
| start = SteadyClock::now(); | |
| crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, kMasterKey.nDeriveIterations, kMasterKey.nDerivationMethod); | |
| kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations | |
| + static_cast<unsigned int>(kMasterKey.nDeriveIterations * (target / (SteadyClock::now() - start)))) | |
| / 2; | |
| if (kMasterKey.nDeriveIterations < 25000) |
🤖 Prompt for AI Agents
In src/wallet/wallet.cpp around lines 703 to 713, the timing math uses
SteadyClock durations implicitly against MillisecondsDouble which causes the
same MillisecondsDouble-type issue noted earlier; change both places so you
first compute the elapsed time as a MillisecondsDouble from (SteadyClock::now()
- start) and use that explicit MillisecondsDouble value in the nDeriveIterations
calculations (i.e., divide target by the elapsed MillisecondsDouble), and guard
the divisor against zero before assigning to kMasterKey.nDeriveIterations.
| std::atomic<bool> m_scanning_with_passphrase{false}; | ||
| std::atomic<SteadyClock::time_point> m_scanning_start{SteadyClock::time_point{}}; | ||
| std::atomic<double> m_scanning_progress{0}; |
There was a problem hiding this comment.
Missing include for SteadyClock types used in header
SteadyClock::time_point requires <util/time.h> (your alias) in this header to avoid ODR/order-dependent builds.
Add at the top near other util includes:
+#include <util/time.h>📝 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.
| std::atomic<bool> m_scanning_with_passphrase{false}; | |
| std::atomic<SteadyClock::time_point> m_scanning_start{SteadyClock::time_point{}}; | |
| std::atomic<double> m_scanning_progress{0}; | |
| #include <util/time.h> |
🤖 Prompt for AI Agents
In src/wallet/wallet.h around lines 270 to 272, the header uses
SteadyClock::time_point in an atomic member but does not include the util/time.h
alias that defines SteadyClock; add #include "util/time.h" near the other util
includes at the top of the file so SteadyClock::time_point is defined in this
translation unit and avoids ODR/order-dependent build issues.
| m_wallet.m_scanning_with_passphrase.exchange(with_passphrase); | ||
| m_wallet.m_scanning_start = SteadyClock::now(); | ||
| m_wallet.m_scanning_progress = 0; |
There was a problem hiding this comment.
Compile error: undefined identifier ‘with_passphrase’
WalletRescanReserver::reserve() references with_passphrase which is not in scope and the method takes no parameter. Minimal fix (keep prior behavior) is to drop the assignment:
- m_wallet.m_scanning_with_passphrase.exchange(with_passphrase);
+ m_wallet.m_scanning_with_passphrase = false; // or plumb a parameter from the call sites if neededAlternatively, extend reserve(bool with_passphrase) and update all call sites, but that exceeds backport scope.
📝 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.
| m_wallet.m_scanning_with_passphrase.exchange(with_passphrase); | |
| m_wallet.m_scanning_start = SteadyClock::now(); | |
| m_wallet.m_scanning_progress = 0; | |
| m_wallet.m_scanning_with_passphrase = false; // or plumb a parameter from the call sites if needed | |
| m_wallet.m_scanning_start = SteadyClock::now(); | |
| m_wallet.m_scanning_progress = 0; |
🤖 Prompt for AI Agents
In src/wallet/wallet.h around lines 1090-1092, the reserve() method references
an undefined identifier with_passphrase; remove the call that assigns
m_wallet.m_scanning_with_passphrase.exchange(with_passphrase) so the method
compiles and preserves prior behavior, leaving the m_wallet.m_scanning_start and
m_wallet.m_scanning_progress assignments intact; do not add new parameters or
change call sites.
Summary
Backports Bitcoin Core PR bitcoin#27405 to Dash Core.
This PR improves time measurement in several areas by switching from
SystemClock(which can be affected by system time changes) toSteadyClock(monotonic clock) for duration measurements.Changes:
InterruptibleRecvfor socket timeout handlingDash-Specific Adaptations:
headerparameter instead of Bitcoin'ssynctypem_scanning_with_passphrasefield in wallet scanning logicOriginal Bitcoin PR:
bitcoin#27405
Bitcoin commit: e460c0a
Test Plan
Backporting Notes
All timing-related code properly uses steady clock for duration measurements. The changes are type-safe improvements that prevent issues with system clock adjustments.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests