fix: don't re-capture a source_location when a not_null is relocated

[UdjinM6]

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_asserts 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:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

[coderabbitai]

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 configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 14c4e3e1-1a43-4aa8-8924-f408e4d23d5f

📥 Commits

Reviewing files that changed from the base of the PR and between 7c15a39 and be3c436.

📒 Files selected for processing (1)

• src/gsl/pointers.h

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

gsl::not_null now excludes other not_null specializations from its forwarding constructor. Its converting constructor accepts a source location and checks the converted pointer there. Compile-time assertions check move-construction properties for pointer, shared-pointer, and unique-pointer specializations.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: knst

Merge Risk: ⚪ Minimal · up to be3c4

The change preserves non-null checks and smart-pointer source state while correcting location attribution during relocation. No actionable merge-blocking risk is established; merge after normal checks pass.

Architecture Summary

Architecture risk: 🔵 Low · up to be3c4

The change affects 1 system.

Changed systems: src

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — src (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in src/gsl/pointers.h: Adds details::is_not_null, which is false by default and true for not_null<T> specializations.
• observed — Modified behavior in src/gsl/pointers.h: The forwarding constructor now excludes not_null specializations from its U constraint; its added comments describe avoiding constructor selection during copy, move, or conversion.
• observed — Modified behavior in src/gsl/pointers.h: The converting constructor now takes a source-location argument and initializes from other.get() before checking the converted pointer with Expects. This replaces delegation through the ordinary constructor, which did not accept the caller’s location.
• observed — Modified behavior in src/gsl/pointers.h: Adds compile-time checks that pointer and shared-pointer not_null types are nothrow move-constructible, while a unique-pointer not_null is not move-constructible.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: preventing source_location recapture when gsl::not_null is relocated.
Description check	✅ Passed	The description directly explains the relocation bug, the constructor-overload fix, the preserved non-null invariant, and the reported validation results.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Final review complete — no blockers (commit be3c436) · triage: low

[thepastaclaw]

Final validation — Phase 1 + Phase 2

Verified the combined reviewer evidence against the exact head and merge-base PR diff; no actionable defects were found. Independent C++20 probes confirmed that relocation avoids redundant checks, shared-pointer sources remain non-null, and cross-specialisation conversions retain caller-attributed failure checks. The regression assertions pass at head and fail against the parent header and a move-constructor mutation; the full build and multi-platform results reported in the PR were not independently rerun.

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.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — Small, self-contained change to gsl::not_null constructor overload resolution and compile-time regression checks, with no impact on consensus, funds, cryptography, networking, or storage.
• Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer, glm-5.3-flash — dash-core-commit-history (completed, effort high); agent phase1-reviewer
• Phase 1 model: glm-5.3-flash — zai quota: 5h 100% left, weekly 79% left; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 13% left, 5h 100% left)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer