fix(util): handle the UINT64_MAX-containing range in CRangesSet - #7587
Merged
PastaPastaPasta merged 1 commit intoAug 12, 2026
Merged
Conversation
Adding UINT64_MAX to a CRangesSet stores the half-open range with a wrapped end of 0. Contains() then reports the value absent, so a duplicate Add() reaches the set-insert assert instead of returning false, and the asset-unlock duplicate-index check (evo/assetlocktx.cpp) would crash in GetCreditPool rather than cleanly rejecting the transaction with bad-assetunlock-duplicated-index. Size() computed the width of such a range through an unsigned wrap that -fsanitize=integer reports. Treat end == 0 as extends-through-UINT64_MAX in Contains() and Size(), and pin the boundary behavior (membership, sizing, duplicate detection, removal, re-add) in test_CRanges. The new checks fail eight ways on the previous implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕓 Ready for review — 3 ahead in queue (commit 498a1ae) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
5 tasks
knst
approved these changes
Aug 12, 2026
5 tasks
PastaPastaPasta
added a commit
to PastaPastaPasta/dash
that referenced
this pull request
Aug 12, 2026
…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>
PastaPastaPasta
added a commit
that referenced
this pull request
Aug 13, 2026
…nder -fsanitize=integer 8b200cd fix: make the wrapped successor explicit in CRangesSet under -fsanitize=integer (pasta) Pull request description: ## Issue being fixed or feature implemented **develop's `linux64_asan` job is red since #7587 merged.** The boundary tests merged there exercise `Add(UINT64_MAX)`, whose half-open end `value + 1` intentionally wraps to `0` — the representation the rest of #7587 teaches the class to understand. The asan job builds with `-fsanitize=integer`, which reports the intentional wrap as `unsigned integer overflow: 18446744073709551615 + 1` at `util/ranges_set.cpp:27` and fails `make 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 in `Add()`/`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_MAX` the expression is `value + 1` as before, and for `UINT64_MAX` it produces the same `0` the wrap produced. ## How Has This Been Tested? Built with `--with-sanitizers=undefined,integer` (the failing job's relevant checks): `util_tests/test_CRanges` reproduces 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-werror` build, rebased on current develop (the `linux64_nowallet` failure visible on this branch's earlier CI was the pre-existing `ScopedBLSLegacyScheme` gcc-14 warning, fixed independently by #7586). ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: c1810c61e8f7a9dc98023c11c86f88740c4cb8941d8e8f4152e21a8ddf8acf63711004156de04f0f88d2d3ccedd10f2c30caa98e2de5ae4a161a3368569a8e02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
CRangesSetstores half-open[begin, end)ranges ofuint64_t. AddingUINT64_MAXstoresend = UINT64_MAX + 1, which wraps to0, and the rest of the class does not understand that representation:Contains(UINT64_MAX)returns false for a set that holds it (prev->end > valueis0 > UINT64_MAX), so a duplicateAdd()walks past the duplicate guard and tripsassert(ret.second)on the underlyingstd::setinsert. Via the asset-unlock duplicate-index check (src/evo/assetlocktx.cpp:204), a platform-quorum-signed withdrawal with index2^64 - 1would crash the node inGetCreditPool()instead of being rejected withbad-assetunlock-duplicated-index. Platform assigns withdrawal indexes sequentially, so nothing produces that index today — this is a latent boundary bug, not an exploitable path — but consensus code should reject strange quorum-signed data cleanly, never abort on it.Size()computes the width of such a range through an unsigned wrap. The wrapped arithmetic happens to produce the right number, but it is exactly the class of intentional-looking overflow that-fsanitize=integerflags, and it costs nothing to state the intent explicitly.This was found while building the bounded snapshot codec for AssumeUTXO M4 (#7579). Per review feedback there (knst), the fix is split out so a small consensus-adjacent change can be reviewed on its own; nothing in it depends on the snapshot work. It is a prerequisite of the M4 series only in the sense that the snapshot codec serializes
CRangesSetand wants the boundary semantics to be trustworthy.What was done?
Contains(): treatend == 0as "extends throughUINT64_MAX".Size(): compute the width of the wrapped range explicitly instead of relying on modular subtraction.test_CRangesunit test with boundary coverage: membership, sizing, duplicate detection, removal, and re-add at and aroundUINT64_MAX, plus the single-element{UINT64_MAX}set. The new checks fail eight ways against the previous implementation.How Has This Been Tested?
./src/test/test_dash --run_test=util_tests/test_CRanges— green with the fix; verified the new assertions fail (8 failures) with the fix reverted. Full unit suite run locally on the branch.Breaking Changes
None. The serialized encoding of
CRangesSetis unchanged; only in-memory queries over the wrapped-end representation change, and no currently reachable chain state produces that representation.Checklist: