backport: make adjusted time type-safe (bitcoin#25786)#7459
backport: make adjusted time type-safe (bitcoin#25786)#7459PastaPastaPasta wants to merge 2 commits into
Conversation
e2693d1 to
f00fdbb
Compare
03b9dcf to
a1df653
Compare
a1df653 to
f5e834c
Compare
|
⛔ Blockers found — Sonnet deferred (commit 40e554c) |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
🚧 Files skipped from review as they are similar to previous changes (16)
WalkthroughThe pull request migrates node time handling from integer Unix timestamps to Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The backport has no missing-prerequisite gap, but the Dash adaptation introduces signed-overflow undefined behavior when extreme serialized timestamps are compared with native-resolution NodeClock values. The intermediate upstream commit is also non-buildable until the following Dash adaptation commit, violating the repository's atomic-commit requirement.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (failed),gpt-5.6-sol— backport-reviewer (failed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/spork.cpp`:
- [BLOCKING] src/spork.cpp:128: Peer-supplied spork timestamps overflow before authentication
TimeSigned() represents the serialized int64_t timestamp as NodeSeconds, while GetAdjustedTime() uses NodeClock's finer native duration. Chrono comparison converts both operands to that finer duration, overflowing when an extreme timestamp such as INT64_MAX is multiplied by the microsecond or nanosecond scale. This happens before signer recovery or authentication in the SPORK network handler, so an unauthenticated peer can reach it. A minimal build using the project's debug -ftrapv setting terminated with exit 133; optimized builds retain signed-overflow undefined behavior. Normalize adjusted time to seconds before comparing. The activation comparison at line 229 has the same unsafe conversion for signed SporkValue timestamps.
In `src/governance/vote.cpp`:
- [BLOCKING] src/governance/vote.cpp:161-162: Extreme governance timestamps overflow chrono comparisons
CGovernanceVote::Time() wraps the signed 64-bit wire timestamp in NodeSeconds, but max_time retains NodeClock's finer native duration. Comparing them implicitly scales the wire value and overflows for values such as INT64_MAX. A peer can reach this before signature verification by using a known governance parent, valid signal fields, and a public masternode outpoint. The same PR-introduced conversion occurs before governance-object signature verification in MasternodeRateCheck at governance.cpp lines 742-749, and RPC/GUI proposal validation at object.cpp line 147 also accepts an int64_t epoch. Cast adjusted time to NodeSeconds at each serialized-time comparison and add extreme-value coverage.
In `<commit:ccb8bcebfca>`:
- [BLOCKING] <commit:ccb8bcebfca>:1: Squash the Dash adaptation into the type-change commit
Commit ccb8bcebfca changes GetAdjustedTime() to return NodeClock::time_point while leaving Dash callers that assign it directly to int64_t, use it as an int64_t default argument, or add integer values to it. Examples at that revision include governance.cpp:286, coinjoin.h:220, vote.cpp:94, and rpc/governance.cpp:120; these expressions have no valid implicit chrono-to-integer conversion, so the intermediate revision cannot compile. The following commit fixes those consumers, but the repository guide requires every commit to build independently. Fold the required Dash adaptations into the type-change commit while retaining the upstream identifiers and provenance.
| std::optional<CKeyID> CSporkManager::GetValidSporkSigner(const CSporkMessage& spork) const | ||
| { | ||
| if (spork.nTimeSigned > GetAdjustedTime() + 2 * 60 * 60) { | ||
| if (spork.TimeSigned() > GetAdjustedTime() + 2h) { |
There was a problem hiding this comment.
🔴 Blocking: Peer-supplied spork timestamps overflow before authentication
TimeSigned() represents the serialized int64_t timestamp as NodeSeconds, while GetAdjustedTime() uses NodeClock's finer native duration. Chrono comparison converts both operands to that finer duration, overflowing when an extreme timestamp such as INT64_MAX is multiplied by the microsecond or nanosecond scale. This happens before signer recovery or authentication in the SPORK network handler, so an unauthenticated peer can reach it. A minimal build using the project's debug -ftrapv setting terminated with exit 133; optimized builds retain signed-overflow undefined behavior. Normalize adjusted time to seconds before comparing. The activation comparison at line 229 has the same unsafe conversion for signed SporkValue timestamps.
| if (spork.TimeSigned() > GetAdjustedTime() + 2h) { | |
| const auto max_time{std::chrono::time_point_cast<std::chrono::seconds>(GetAdjustedTime() + 2h)}; | |
| if (spork.TimeSigned() > max_time) { |
source: ['codex']
| const auto max_time{GetAdjustedTime() + 1h}; | ||
| if (Time() > max_time) { |
There was a problem hiding this comment.
🔴 Blocking: Extreme governance timestamps overflow chrono comparisons
CGovernanceVote::Time() wraps the signed 64-bit wire timestamp in NodeSeconds, but max_time retains NodeClock's finer native duration. Comparing them implicitly scales the wire value and overflows for values such as INT64_MAX. A peer can reach this before signature verification by using a known governance parent, valid signal fields, and a public masternode outpoint. The same PR-introduced conversion occurs before governance-object signature verification in MasternodeRateCheck at governance.cpp lines 742-749, and RPC/GUI proposal validation at object.cpp line 147 also accepts an int64_t epoch. Cast adjusted time to NodeSeconds at each serialized-time comparison and add extreme-value coverage.
| const auto max_time{GetAdjustedTime() + 1h}; | |
| if (Time() > max_time) { | |
| const auto max_time{std::chrono::time_point_cast<std::chrono::seconds>(GetAdjustedTime() + 1h)}; | |
| if (Time() > max_time) { |
source: ['codex']
Convert nearby CoinJoin, governance, spork, LLMQ, RPC, and test code to chrono time points and durations. Preserve serialized, signed, hashed, and RPC-facing timestamps as integer Unix seconds with explicit conversions at those boundaries.
f5e834c to
40e554c
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: prior-3 is STILL_VALID because ccb8bce changes GetAdjustedTime() to NodeClock::time_point while leaving incompatible Dash-only consumers until the following commit. Latest-delta reconciliation: 4baa5e1fa44e and e011eb8a496e are FIXED through seconds-resolution comparisons and focused extreme-timestamp tests; no new latest-delta findings were found. The remaining non-buildable commit boundary is blocking, so Sonnet coverage remains deferred by the Codex blocker gate.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:ccb8bcebfca>`:
- [BLOCKING] <commit:ccb8bcebfca>:1: Dash adaptations still follow the type-changing cherry-pick
Commit ccb8bcebfca changes GetAdjustedTime() from int64_t to NodeClock::time_point but leaves Dash-only consumers unchanged. At that revision, governance.cpp:286 and rpc/governance.cpp:120 assign the result to int64_t, vote.cpp:94 initializes an integer timestamp from it, and coinjoin.h:220 uses it as an int64_t default argument; these conversions are ill-formed. Commit 40e554c929c supplies the required adaptations only afterward, leaving ccb8bcebfca as a non-buildable permanent bisect point. Fold the Dash adaptations into ccb8bcebfca, or otherwise restructure the stack so every revision builds.
| return current_time - nTime > COINJOIN_QUEUE_TIMEOUT || | ||
| nTime - current_time > COINJOIN_QUEUE_TIMEOUT; | ||
| const auto queue_time{Time()}; | ||
| if (current_time < NodeSeconds{} || queue_time < NodeSeconds{}) return true; |
There was a problem hiding this comment.
nit: technically, it's valid code, but looks a bit strange without 0s
if (current_time < NodeSeconds{0} || queue_time < NodeSeconds{0}) return true;| [[nodiscard]] bool IsTimeOutOfBounds(NodeSeconds current_time) const; | ||
| [[nodiscard]] bool IsTimeOutOfBounds() const; |
There was a problem hiding this comment.
2 methods with the same name is a bit strange too.
consider renaming one from IsTimeOutOfBounds to IsTimeOutOfBoundsAtTime or something
There was a problem hiding this comment.
it's for the overload?
| int64_t nNow = GetAdjustedTime(); | ||
| int64_t nSuperblockCycleSeconds = Params().GetConsensus().nSuperblockCycle * Params().GetConsensus().nPowTargetSpacing; | ||
| const auto timestamp{govobj.CreationTime()}; | ||
| const auto now{std::chrono::time_point_cast<std::chrono::seconds>(GetAdjustedTime())}; |
There was a problem hiding this comment.
nit: why not just
const std::chrono::time_point_cast<std::chrono::seconds> now{(GetAdjustedTime())};
Type is already specified, auto seems useless here
There was a problem hiding this comment.
time point cast is a function not a type
| // Perform additional relays for triggers | ||
| int64_t nNow = GetAdjustedTime(); | ||
| int64_t nSuperblockCycleSeconds = Params().GetConsensus().nSuperblockCycle * Params().GetConsensus().nPowTargetSpacing; | ||
| const auto now{std::chrono::time_point_cast<std::chrono::seconds>(GetAdjustedTime())}; |
There was a problem hiding this comment.
nit: same here for auto - type is already specified on the same line
There was a problem hiding this comment.
same; time_point_cast is a function not a type
| static constexpr int64_t GOVERNANCE_FEE_CONFIRMATIONS = 6; | ||
| static constexpr int64_t GOVERNANCE_MIN_RELAY_FEE_CONFIRMATIONS = 1; | ||
| static constexpr int64_t GOVERNANCE_UPDATE_MIN = 60 * 60; | ||
| static constexpr std::chrono::hours GOVERNANCE_UPDATE_MIN{1}; |
There was a problem hiding this comment.
nit: unify style with our code base, see for example net_processing.cpp
static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1ms;-static constexpr std::chrono::hours GOVERNANCE_UPDATE_MIN{1};
+static constexpr auto GOVERNANCE_UPDATE_MIN{1h};
3 participants
Issue being fixed or feature implemented
Backports bitcoin/bitcoin#25786, which makes adjusted network time type-safe by returning a
NodeClock::time_pointand using chrono types for time arithmetic.Dash has additional adjusted-time consumers in CoinJoin, governance, spork, LLMQ, and RPC code. Those consumers must be converted without changing timestamps that form part of serialized, signed, hashed, or RPC-facing data.
What was done?
nMaxTipAgeand the existing relay guard.NodeClock,NodeSeconds, and chrono durations for internal comparisons and arithmetic. NoGetTime()workaround was introduced.backport-reviewworkflow; no missing prerequisites or incorrectly resolved upstream hunks were found.How Has This Been Tested?
Tested on macOS arm64 (
aarch64-apple-darwin25.3.0) with:./configure --without-gui --disable-bench --disable-fuzz-binary --disable-hardening make -C src -j"$(sysctl -n hw.ncpu)" test/test_dashThe following focused unit-test suites passed:
coinjoin_inouts_testscoinjoin_queue_teststimedata_testsgovernance_validators_testsgovernance_vote_wire_testsgovernance_inv_testsminer_testsvalidation_chainstatemanager_testsllmq_dkg_testsgit diff --check upstream/develop..HEADalso passed.Breaking Changes
None. Serialized, signed, hashed, and RPC timestamp representations remain integer Unix seconds.
Checklist: