Repository navigation
fix: report the real source location for GetCreditPool's not_null precondition - #7760
Conversation
|
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. Walkthrough
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to This change improves the location reported for a null-precondition failure without changing the stored pointer behavior. No PR-introduced merge risk was identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes appear to preserve credit-pool validation while addressing builder-specific paths in macOS binaries. No new security exposure was identified, but the resulting release binaries were not available to verify the reproducibility outcome. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Hardening Proposals
🚥 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 |
|
why do we need to replace emplace() to push() if sdk path is fixed now? @UdjinM6 |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified both commits against exact head ba1fafa and found no actionable defects. Source inspection confirms that push restores call-site construction without changing credit-pool calculations, and that the SDK prefix mapping applies to cross-compilation while preserving native macOS overrides; an independently compiled four-case probe reproduced the reported SDK-path leakage and confirmed both fixes. The diff passes whitespace checks; full Guix release reproducibility was not independently tested.
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The diff is small and contained, replacing emplace with push to preserve caller source locations and adding a macOS SDK prefix-map build flag without changing credit-pool logic or consensus behavior. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— dash-core-commit-history (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 97% left, weekly 34% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
I would drop the second commit if you'd rather keep the diff minimal — the first one is the actual fix:
|
ba1fafa to
df48b91
Compare
not_null construction at the call site in GetCreditPool
|
Dropped ba1fafa. After investigating a bit more I think fixing the SDK mount Pasta proposed originally is a more focused approach but it belongs to a separate guix-only PR. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at df48b91 and found no actionable defects. The change preserves credit-pool traversal and accounting while constructing not_null at the application call site; a local C++20 probe using the repository's GSL headers confirmed the source-location difference between push and emplace. The diff whitespace check passed; full project builds and test suites were not run.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The change is a small, contained replacement of emplace() with push() in GetCreditPool to preserve not_null diagnostic source locations, with a lint suppression and explanatory comment, without changing credit-pool or consensus logic. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— dash-core-commit-history (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 93% left, weekly 27% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort medium); agentphase2-reviewer,gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
df48b91 to
4989a84
Compare
not_null construction at the call site in GetCreditPool…condition 966f532 turned `to_calculate.push(block_index)` into `to_calculate.emplace(block_index)` to satisfy clang-tidy's modernize-use-emplace. `to_calculate` holds `gsl::not_null<const CBlockIndex*>`, whose converting constructor takes `nostd::source_location loc = nostd::source_location::current()` (src/gsl/pointers.h) and forwards it to `Expects()`. A default argument is evaluated at the point of the call, so `push()` evaluates `__builtin_FILE()` here in creditpool.cpp, while `emplace()` forwards the raw pointer and evaluates it inside the standard library's construction helper. That location is not merely stored, it is printed. `gsl::details::terminate()` (src/gsl/assert.cpp) writes `file_name()`, `line()`, `column()` and `function_name()` to stderr and to debug.log before calling `std::terminate()`, in release builds as well as debug ones. Since the refactor, a null `pprev` reaching `GetCreditPool` therefore names a standard library internal rather than the loop that produced it, on every platform: `__memory/construct_at.h` with libc++, `bits/stl_construct.h` with libstdc++. On darwin it also breaks reproducibility. The libc++ headers resolve through `-isysroot$(OSX_SDK)` (depends/hosts/darwin.mk) and contrib/guix/guix-build shares the SDK into the container at the builder's own path, so the embedded string differs between builders. That is how this was found: guix attestation for v24.0.0-rc.1 disagreed on the macOS hashes for dashd, dash-qt and test_dash while every non-darwin output matched. Linux binaries carry the same wrong location, but at a path the guix build already normalises, so comparing hashes could never have surfaced it there. modernize-use-emplace is a false positive whenever the element type's constructor captures the caller's location, so suppress it here rather than let the refactor be reapplied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4989a84 to
00aabf5
Compare
…is relocated be3c436 fix: don't re-capture a source_location when a not_null is relocated (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Relocating a `gsl::not_null` re-records where it was moved, so a failed precondition names whatever code moved it rather than the code that produced the null pointer. When the relocation happens inside a standard library header, that header's path is embedded in the shipped binaries. `not_null`'s converting constructor takes a Dash-specific defaulted `nostd::source_location loc = nostd::source_location::current()` (`src/gsl/pointers.h:122-126`) and forwards it to `Expects()`. Upstream Microsoft GSL has no such parameter, so while its own overload resolution behaves the same way, there is nothing there to recapture. The other candidates were `not_null(const not_null&)` and the templated `not_null(const not_null<U>&)`. For an rvalue or a non-const lvalue, the forwarding template binds `U&&` exactly while both of those need an added `const`, so overload resolution picked the template, and every relocation re-evaluated `source_location::current()` and re-ran the null check on a value already known to be non-null. That location is printed, not merely stored: `gsl::details::terminate()` (`src/gsl/assert.cpp:26-37`) writes `file_name()`, `line()`, `column()` and `function_name()` to stderr and to `debug.log` before `std::terminate()`, in release builds as well as debug ones. `std::stack<gsl::not_null<const CBlockIndex*>>` in `evo/creditpool.cpp` moves its element into a freshly allocated deque node, so the linux release binaries embed `/usr/include/c++/13/bits/stl_construct.h` and would report it as the origin of a failed precondition. Confirmed by symbolizing the only reference to that string in a release `dashd`: it resolves through `_M_push_back_aux` and `_M_allocate_node` to a `source_location` built with file `stl_construct.h` and line 97, followed by the null-check branch. The darwin binaries are clean only because clang drops the unreachable `terminate()` path that keeps the string alive; gcc does not. This is the remaining half of #7760. That PR stopped a raw pointer being forwarded into the standard library and converted there; this stops an already-constructed `not_null` re-capturing when a container relocates it. Neither subsumes the other, and with only #7760 the linux binaries still carried the path. ## What was done? Excluded every `not_null` specialisation from the location-capturing constructor, through a `details::is_not_null` trait rather than `is_same`, so that converting between specialisations does not select it either. Relocation then binds to the copy constructor. No move constructor is added, deliberately. Moving would leave a smart-pointer `T` null and break the invariant the type exists to enforce, which is why upstream omits one and why `strict_not_null`'s own move can be defaulted - it copies the base rather than moving it, as its comment at `src/gsl/pointers.h:333-335` says. The `not_null<U>` to `not_null<T>` constructor no longer delegates, which recorded this header as the origin of any failure, but keeps its check: `U` is non-null, yet nothing constrains a `U` to `T` conversion to preserve that. It takes its own location so the check names the caller. Two assertions pin the result. The first fails if the location-capturing constructor is selected for a relocation again, since as written it is not `noexcept` - it would not catch one that someone explicitly declared `noexcept`, which is a smaller hole than leaving it unguarded. The second fails if a usable move constructor is added, which the first would not catch, since a defaulted move constructor satisfies it too; a move-only `T` that must not be movable is what detects one. Deliberately not addressed, to keep this to the defect: `make_not_null` and the `strict_not_null` constructors still capture locations inside `pointers.h`, and `strict_not_null` is not covered by the trait because it derives from `not_null` rather than being a specialisation. Neither embeds a standard library path, and `strict_not_null` is unused outside `src/gsl/`. ## How Has This Been Tested? No unit test was added. The captured location is observable only by making the precondition fail, which needs a death or subprocess test around a deliberately null pointer; that seemed a poor trade for the compile-time guarantee below. The two `static_assert`s are the regression test, and both were checked in each direction: they pass against this header, and fail against `develop`'s header and against a header with a move constructor re-added. Verified by compiling probes that use `#line` to attribute a relocation to a synthetic header: - before: moving a `not_null`, copying a non-const lvalue, and converting `not_null<B*>` to `not_null<const B*>` all recorded the mover's file - after: all three record only the genuine construction site, and a 600-element `std::deque` forcing node allocation embeds no standard library path at all - a `not_null<std::shared_ptr<int>>` was moved and the source then checked at runtime: still non-null, so the invariant holds Full build plus the `evo_*` and `llmq_*` suites (228 cases) pass locally. The guix build passes on all seven hosts - `x86_64-linux-gnu`, `riscv64`, `aarch64`, `powerpc64`, `x86_64-w64-mingw32` and both darwin - which is the real coverage here, since only two of those toolchains are reachable locally. ## Breaking Changes None. No consensus, P2P, RPC, wallet or on-disk behaviour changes, and no change to the class layout - the sole data member is still `T`. The `not_null<U>` to `not_null<T>` constructor gains a defaulted parameter, and overload selection among the inline constructors changes, so this is not source-identical for anything that named those overloads explicitly. For the copyable `T` used in this tree, raw pointers and `shared_ptr`, relocation already copied the pointer; it simply no longer re-runs a redundant check or records a location while doing so. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: d24e899cbd1376d2482cc7d8f302b1f550c375a40e34e513a1e5da1d1090265aac1f1f314c72fe45ed2aee5cdcc27dd8ebbe60092a5792e56e0c69ee6ce1f0b1
…library header path b6b9b75 ci: run the stdlib header path check on the linux64 and mac builds (UdjinM6) 4471e14 guix: fail the build when a binary embeds a stdlib header path (UdjinM6) Pull request description: ## Issue being fixed or feature implemented A source location captured inside a standard library header is wrong twice over: `gsl::details::terminate()` prints it, so a failed precondition names a standard library internal instead of the code that broke it, and the recorded path belongs to the toolchain, so on darwin it comes from the macOS SDK - whose location differs between builders and makes the release binaries irreproducible. Nothing catches that today. 966f532 introduced exactly this bug in `evo/creditpool.cpp` and it went unnoticed until guix attestation for v24.0.0-rc.1 disagreed on the macOS hashes: just over seven days after the commit landed, and only because one builder happened to keep their SDK somewhere other than the rest. Comparing hashes across builders is a poor detector. It needs a second builder, a full release build, and a difference in their setups before it fires at all, and it fires at the worst possible moment. ## What was done? A check that fires on a single build instead. `contrib/guix/stdlib-path-check.py` parses each executable with LIEF and fails if one embeds a C++ standard library header path, wired as a `check-stdlib-paths` make target beside the existing `check-symbols` and `check-security`, and invoked from the guix build. It also runs in regular CI, behind `RUN_STDLIB_PATH_CHECK`, enabled for the `linux64` and `mac` targets. `check-symbols` could not be run there - it caps GLIBC at 2.31 (`contrib/guix/symbol-check.py:31-38`) while a native CI build links against the container's far newer glibc, so it asserts a property of release artifacts that a native build legitimately violates - and `check-security` is simply not wired into regular CI today. This check has no such dependence: outside the instrumented builds noted below, an embedded standard library header path is wrong regardless of host or toolchain. Regular CI builds from a `make distdir` tree (`ci/dash/build_src.sh:29-37`), so the script is listed in `BIN_CHECKS`/`EXTRA_DIST`. Both targets matter, and neither is redundant. On libc++ the relocation form of this bug is invisible, because clang proves the moved-from pointer non-null and drops the branch that keeps the string alive; gcc does not. On libstdc++ it is visible. Conversely the original rc.1 form - a raw pointer forwarded into the standard library - is visible on both. So darwin alone cannot cover this class, and `linux64` should not be dropped on the assumption that it does. Only sections that hold string literals are examined: `.rodata`, `.rdata`, `__cstring`, `__const`, and anything with those prefixes, so ELF variants such as `.rodata.str1.1` are included. DWARF legitimately names standard library headers for inlined template code, and skipping debug sections by name is not portable - PE stores a long section name as an offset into the string table, so they read as `/81` rather than `.debug_*`. A first version denied those by name and drowned the mingw build in thousands of DWARF false positives. Naming the sections we want cannot pick up debug data by accident. The trade is deliberate: a literal in an unusual section would be missed, which for a gate that blocks builds is the safer direction. Each hit reports its section, how many times the path occurs, and the first few virtual addresses, so a real literal is distinguishable at a glance from debug data, and can be attributed to the referencing code with `objdump` without a rebuild. A file LIEF cannot parse is also a failure rather than a silent pass. `RUN_STDLIB_PATH_CHECK` defaults to false, and only `linux64` and `mac` opt in. That is a default rather than a hard exclusion - a target that inherited the variable as true would run the check - but no target sets it today. Sanitizer and fuzz builds are deliberately not opted in: their instrumentation records source locations for its own diagnostics and is expected to name standard library headers legitimately, so enabling it there would report instrumentation as a defect. They can be revisited once we know what they actually embed. ## How Has This Been Tested? The check found a real second bug on its first run, which is the strongest evidence for it. After #7760 the linux release binaries still embedded `bits/stl_construct.h`; that turned out to be a separate defect in `gsl::not_null`, fixed in #7772, which source review had missed. Against real artifacts: - the pre-fix guix build of 0f87636 fails on `dashd`, `dash-qt` and `test_dash` - precisely the three binaries that differed between builders on rc.1 - and passes on `dash-cli`, `dash-tx`, `dash-util` and `dash-wallet`, precisely the four that were byte-identical - discrimination was checked directly: an object file containing twelve standard library paths in total reports exactly one, the string literal in `__TEXT,__cstring` - no false positives on an ordinary local non-guix build of `dashd`, `test_dash`, `dash-qt` and `dash-cli` The two halves were separated across branches so each can be observed on its own, since this PR on its own is expected to fail until #7772 merges: - [`check-stdlib-paths`](https://github.com/UdjinM6/dash/commits/refs/heads/check-stdlib-paths/) - this PR, the check without the fix. Regular CI `linux64` and the guix libstdc++ hosts fail on `/usr/include/c++/13/bits/stl_construct.h`; `mac` and both guix darwin hosts pass. That asymmetry is the coverage point above: clang drops the branch that keeps the string alive, gcc does not. - [`check-stdlib-paths-with-fix`](https://github.com/UdjinM6/dash/commits/refs/heads/check-stdlib-paths-with-fix/) - this PR rebased on top of #7772, kept purely as evidence and not proposed for merge. Every guix host passes - `x86_64-linux-gnu`, `riscv64`, `aarch64`, `powerpc64`, `x86_64-w64-mingw32` and both darwin - along with `linux64` and `mac` in regular CI. Taken together: the check fails on a tree with the bug, passes on a tree with the fix, and produces no false positives across four object-format and standard-library combinations. ## Breaking Changes None at runtime. This adds a build-time check; a build that would have produced a binary embedding such a path now fails instead. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: b4941bfb9627910a4152b7b836a1bc92031eeecd5e38bc1765c89dc6509f570933e681193b106461bad01afb560e5ace939c10b0dbc38785aaf8ccf7b44e2a44
Issue being fixed or feature implemented
CCreditPoolManager::GetCreditPool()records the wrong source location for itsnot_nullprecondition, so a failure blames a standard library header instead of the code that produced the null pointer. This affects every platform, and on darwin it additionally makes the release binaries irreproducible.966f532, from #7693, changed
evo/creditpool.cppto callemplace()rather thanpush()on astd::stack<gsl::not_null<const CBlockIndex*>>, to satisfy clang-tidy'smodernize-use-emplace.gsl::not_null's converting constructor takesnostd::source_location loc = nostd::source_location::current()(src/gsl/pointers.h:102), backed by__builtin_FILE(), and forwards it toExpects(). A default argument is evaluated at the point of the call:push()constructs thenot_nullat the call site increditpool.cpp, whileemplace()forwards the raw pointer and constructs it inside the standard library's construction helper.That location is not merely stored, it is printed.
gsl::details::terminate()(src/gsl/assert.cpp:26-35) writesfile_name(),line(),column()andfunction_name()to stderr and todebug.logviaLogPrintfbefore callingstd::terminate(), in release builds as well as debug ones. So since that commit, a nullpprevreachingGetCreditPoolreports a standard library internal rather than the loop that produced it —__memory/construct_at.hwith libc++,bits/stl_construct.hwith libstdc++. The linux release binaries name/usr/include/c++/bits/stl_construct.h, a path that exists on no user's machine.modernize-use-emplaceis a false positive whenever the element type's constructor captures the caller's location this way.The reproducibility symptom is what led here. On darwin the libc++ headers resolve through
-isysroot$(OSX_SDK)(depends/hosts/darwin.mk:75-81), andcontrib/guix/guix-build:452shares the SDK into the container at the builder's own path —--share="$SDK_PATH", with no=/targetremap, unlike--share="$PWD"=/dash. The embedded string therefore differs between builders, and guix attestation for v24.0.0-rc.1 disagreed on the macOS hashes fordashd,dash-qtandtest_dashwhile every Linux, Windows and source output matched (dashpay/guix.sigs#190 vs #191).Worth stating plainly, since it is the obvious question: the non-darwin binaries carry the same wrong location. They just carry it at a path the guix build already normalises (
contrib/guix/libexec/build.sh:221rewrites/gnu/store/...to/usr), so it is identical for every builder and comparing hashes could never have surfaced it. The leak was invisible on those platforms, not absent. It also predates no release: the commit landed 2026-09-18 and v24.0.0-rc.1 was tagged 2026-09-24, so this is the first release that ever contained it.What was done?
Reverted the
emplace()topush(), with aNOLINT(modernize-use-emplace)and a comment recording why, so the tidy rewrite is not reapplied.gsl::not_nullis a thin wrapper over a raw pointer, so both forms emit the same store;emplace()bought nothing here beyond silencing the check.How Has This Been Tested?
No test was added. Observing the logged location requires the precondition to fail, which means a null
pprev— a state that would mean the block index is already corrupt.Verified against the real build artifacts instead. The guix build of the parent commit (0f87636) embeds the standard library path in exactly
dashd,dash-qtandtest_dash, and in none ofdash-cli,dash-tx,dash-utilordash-wallet— matching the three-versus-four split that was observed between builders on rc.1. The guix build of this branch embeds it in none of them, on both darwin and linux. https://github.com/UdjinM6/dash/actions/runs/36436424685Also audited the tree for the same pattern elsewhere.
gsl::not_nullappears in 145 places outsidesrc/gsl, all parameters or locals except two stored ones: the stack fixed here, andUtilParameters::m_base_index(src/llmq/utils.h:36). EveryUtilParametersconstruction site is braced aggregate initialisation at the call site, so none forwards a raw pointer through a standard library header.src/gslis also the only user ofnostd::source_locationin the tree.Breaking Changes
None. No consensus, P2P, RPC, wallet or on-disk behaviour changes. The only difference is the file, line and function reported if the
not_nullprecondition inGetCreditPoolever fires.Checklist:
🤖 Generated with Claude Code