fix: make the wrapped successor explicit in CRangesSet under -fsanitize=integer - #7590
Conversation
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The PR makes sanitizer-safe successor handling without changing range behavior. The only current-head issue is a stale adjacent comment for UINT64_MAX's wrapped representation; it has no runtime or user impact, so the PR is merge-ready after normal checks with no actionable merge-blocking risk remaining. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 8b200cd) |
PastaPastaPasta
changed the title
|
This pull request has conflicts, please rebase. |
…ze=integer The boundary tests merged in dashpay#7587 exercise Add(UINT64_MAX), whose value + 1 half-open end intentionally wraps to 0. The linux64_asan job runs with -fsanitize=integer, which reports the wrap as unsigned integer overflow at util/ranges_set.cpp:27 and fails make check on develop. Spell the successor as an explicit branch (WrappedSuccessor) at the three arithmetic sites so the wrap is stated intent instead of overflow. No behavior change: the guarded value compares and inserts exactly as the wrapped arithmetic did. Verified with a --with-sanitizers=undefined,integer build: util_tests/test_CRanges reproduces the CI failure unfixed and passes fixed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50b398d148
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public: | ||
| explicit ScopedBLSLegacyScheme(std::optional<bool> enter = std::nullopt) noexcept : | ||
| //! Capture the current scheme without changing it. | ||
| ScopedBLSLegacyScheme() noexcept : m_saved(bls::bls_legacy_scheme.load()) {} |
There was a problem hiding this comment.
When this commit is reviewed, reverted, or cherry-picked as the advertised CRangesSet sanitizer fix, this independent refactor of ScopedBLSLegacyScheme is silently included even though the commit message describes only the ranges change. That hides a validation-path change from reviewers and prevents the two fixes from being managed independently; move the constructor-overload change into its own commit with its own rationale and validation.
AGENTS.md reference: AGENTS.md:L13-L14
Useful? React with 👍 / 👎.
50b398d to
8b200cd
Compare
PastaPastaPasta
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/util/ranges_set.cpp`:
- Line 37: Update the comment adjacent to Range new_range in Add() to describe
the endpoint using WrappedSuccessor(value), including the UINT64_MAX case where
the representation wraps to 0 instead of always claiming [value, value + 1).
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9847e4f6-4960-4388-b43f-1b50c69ef320
📒 Files selected for processing (2)
src/util/ranges_set.cppsrc/validation.cpp
| // - if there's exist a range [x, value) - we need to replace it to new range [x, value + 1) | ||
| // - if there's exist a range [value + 1, y) - we need to replace it to new range [value, y) | ||
| Range new_range{value, value + 1}; | ||
| Range new_range{value, WrappedSuccessor(value)}; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the adjacent Add() range comment.
At Line 37, Range new_range uses WrappedSuccessor(value), but the comment still describes [value, value + 1) unconditionally. For UINT64_MAX, the stored representation is {UINT64_MAX, 0}. Reference WrappedSuccessor(value) or document this wrap explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/util/ranges_set.cpp` at line 37, Update the comment adjacent to Range
new_range in Add() to describe the endpoint using WrappedSuccessor(value),
including the UINT64_MAX case where the representation wraps to 0 instead of
always claiming [value, value + 1).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The explicit successor helper preserves the existing wrapped range representation while avoiding sanitizer-reported unsigned overflow at all three affected arithmetic sites. One in-scope nitpick remains: Add()'s algorithm comments still describe value + 1 unconditionally instead of the wrapped endpoint used for UINT64_MAX.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
💬 1 nitpick(s)
🤖 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/util/ranges_set.cpp`:
- [NITPICK] src/util/ranges_set.cpp:31-36: Describe the wrapped successor in Add() comments
The implementation now deliberately uses `WrappedSuccessor(value)`, but the adjacent algorithm comments still describe every endpoint as `value + 1`. For `UINT64_MAX`, the stored endpoint is actually `0`, so these comments obscure the boundary behavior this PR makes explicit. Use the helper's name in the range descriptions and document its wrapped endpoint.
| // all 3 of them should be merged in one range [x, y) | ||
| // - if there's exist a range [x, value) - we need to replace it to new range [x, value + 1) | ||
| // - if there's exist a range [value + 1, y) - we need to replace it to new range [value, y) |
There was a problem hiding this comment.
💬 Nitpick: Describe the wrapped successor in Add() comments
The implementation now deliberately uses WrappedSuccessor(value), but the adjacent algorithm comments still describe every endpoint as value + 1. For UINT64_MAX, the stored endpoint is actually 0, so these comments obscure the boundary behavior this PR makes explicit. Use the helper's name in the range descriptions and document its wrapped endpoint.
| // all 3 of them should be merged in one range [x, y) | |
| // - if there's exist a range [x, value) - we need to replace it to new range [x, value + 1) | |
| // - if there's exist a range [value + 1, y) - we need to replace it to new range [value, y) | |
| // If element is not in CRangesSet, add the encoded range | |
| // [value, WrappedSuccessor(value)); UINT64_MAX uses a wrapped end of 0. | |
| // This operation can cause two merges (three cases): | |
| // - if ranges [x, value) and [WrappedSuccessor(value), y) exist, | |
| // merge all three ranges into [x, y) | |
| // - if [x, value) exists, extend it through value | |
| // - if [WrappedSuccessor(value), y) exists, prepend value to it |
source: ['coderabbit']
2 participants
Issue being fixed or feature implemented
develop's
linux64_asanjob is red since #7587 merged. The boundary tests merged there exerciseAdd(UINT64_MAX), whose half-open endvalue + 1intentionally wraps to0— the representation the rest of #7587 teaches the class to understand. The asan job builds with-fsanitize=integer, which reports the intentional wrap asunsigned integer overflow: 18446744073709551615 + 1atutil/ranges_set.cpp:27and failsmake check. My verification of #7587 ran the full unit suite but not under sanitizers, which is exactly the gap this slipped through; apologies for the breakage.What was done?
Spelled the successor as an explicit branch — a file-local
WrappedSuccessor(value)(value == UINT64_MAX ? 0 : value + 1) — at the three arithmetic sites inAdd()/Remove(). This states the wrap as intent instead of overflow, which is preferable to a sanitizer suppression here: unlike the quorum-snapshot skip-list encoding (suppressed by symbol in eacd9e0 because its wraparound is consensus wire format), this is a private in-memory representation that can simply be written unambiguously.No behavior change: for every
value != UINT64_MAXthe expression isvalue + 1as before, and forUINT64_MAXit produces the same0the wrap produced.How Has This Been Tested?
Built with
--with-sanitizers=undefined,integer(the failing job's relevant checks):util_tests/test_CRangesreproduces the exact CI failure without the fix and passes with it, using the repo's ubsan suppressions file. Full unit suite green on a regular--enable-werrorbuild, rebased on current develop (thelinux64_nowalletfailure visible on this branch's earlier CI was the pre-existingScopedBLSLegacySchemegcc-14 warning, fixed independently by #7586).Breaking Changes
None.
Checklist: