util: Add util::NotNull<SmartPtrType> #34844

maflcko commented at 7:42 PM on March 17, 2026: member

The C++ standard library lacks a type to denote a smart pointer is not null. For raw pointer there is std::reference_wrapper, or a plain reference.

Other third-party libraries provide such a type, such as gsl::strict_not_null.

Fix all issues by adding util::NotNull<SmartPtrType>, which documents (and checks) that the inner pointer is never null.

This type can be used when passing never-null smart pointers between functions. It removes the need to Assert() the pointer before dereference. For example, in a getter function:

util::NotNull<std::unique_ptr<Stats>> GetStats()
{
    return util::NotNull{std::make_unique<Stats>()};
}

int main()
{
    auto stats{GetStats()};
    stats->foo; // This can never lead to a nullptr deref
    // Assert(stats)->foo; // This is redundant and won't compile
}

Fixes https://github.com/bitcoin/bitcoin/issues/24423

DrahtBot added the label Utils/log/libs on Mar 17, 2026

DrahtBot commented at 7:43 PM on March 17, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34844.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	sedited, l0rinc
Stale ACK	stickies-v

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36014 (init: ignore repeated -addnode startup values by w0xlt)
#35557 (kernel, validation: Add btck_chainstate_manager_set_clock_time by ryanofsky)
#35511 (RFC: consensus: Make CAmount a class by hodlinator)
#34132 (coins, dbwrapper: remove error catcher, make point-read failures fatal by l0rinc)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

StageAddition(...) in src/test/rbf_tests.cpp
StageAddition(...) in src/test/rbf_tests.cpp
StageAddition(...) in src/test/rbf_tests.cpp
StageAddition(...) in src/test/rbf_tests.cpp
StageAddition(...) in src/test/rbf_tests.cpp

2026-09-22 09:56:22

maflcko force-pushed on Mar 17, 2026

DrahtBot added the label CI failed on Mar 17, 2026

DrahtBot commented at 8:03 PM on March 17, 2026: contributor

🚧 At least one of the CI tasks failed. Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/23213194537/job/67467180922 LLM reason (✨ experimental): Build failed during the cmake build, due to a compilation error in src/test/util_pointers_tests.cpp.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

stickies-v commented at 8:08 PM on March 17, 2026: contributor

Concept ACK

maflcko force-pushed on Mar 17, 2026

DrahtBot removed the label CI failed on Mar 17, 2026

maflcko force-pushed on Mar 24, 2026

DrahtBot added the label CI failed on Mar 24, 2026

DrahtBot removed the label CI failed on Mar 24, 2026

maflcko force-pushed on Apr 3, 2026

DrahtBot added the label CI failed on Apr 3, 2026

DrahtBot removed the label CI failed on Apr 3, 2026

maflcko commented at 11:38 AM on April 17, 2026: member

Probably not going to push here, but another place where this would be suitable is GetWalletForJSONRPCRequest

Edit: diff in #34844 (review)

Other suggested diffs: #34844 (review)

edit: or even https://www.github.com/bitcoin/bitcoin/pull/35569#issuecomment-4835740603

edit: or add annotations/attributes, see #34844 (review)

maflcko force-pushed on Apr 21, 2026

DrahtBot added the label CI failed on Apr 21, 2026

DrahtBot removed the label CI failed on Apr 21, 2026

maflcko commented at 4:28 PM on April 21, 2026: member

Removed the movable feature of NotNull for now, which can be added/reviewed later

edit: added this back again

maflcko force-pushed on Apr 23, 2026

maflcko force-pushed on May 22, 2026

in src/test/util_pointers_tests.cpp:30 in fa1e0bb7d0

  25 | +        ThrowingMoveNullPtr(ThrowingMoveNullPtr&& other) {};
  26 | +        bool operator==(std::nullptr_t) const { return true; }
  27 | +        int* p{};
  28 | +    };
  29 | +    static_assert(!std::is_nothrow_move_constructible_v<ThrowingMoveNullPtr>);
  30 | +    BOOST_CHECK_EXCEPTION(util::NotNull{ThrowingMoveNullPtr{}}, NonFatalCheckError, HasReason{"Internal bug detected: ptr_ != nullptr"});

l0rinc commented at 10:40 AM on May 23, 2026:

fa1e0bb util: Add util::NotNull<AnyPtrType>:

nit: This failure reason is a bit hard to read: ptr_ != nullptr is the invariant that failed, so this test is actually checking the null case. In other words, the message prints the failed condition, not the observed value. Likely not something we can fix here...

maflcko commented at 11:42 AM on May 26, 2026:

Sure, but this is just how it works on master. E.g. in a release build with an injected bug:

-> getchaintxstats <-

Internal bug detected: pindex != nullptr
rpc/blockchain.cpp:1876
...

Happy to review a pull request changing/improving that (e.g. by appending "failed"), but it seems unrelated to the changes here.

l0rinc commented at 10:39 AM on June 2, 2026:

Sure, but this is just how it works on master

I know, it's why I wrote:

Likely not something we can fix here..

optout21 commented at 11:27 AM on June 4, 2026:

Proposed improvement for this: #35461

in src/dbwrapper.h:192 in faef857079

 188 | @@ -188,7 +189,7 @@ class CDBWrapper
 189 |      friend const Obfuscation& dbwrapper_private::GetObfuscation(const CDBWrapper&);
 190 |  private:
 191 |      //! holds all leveldb-specific fields of this class
 192 | -    std::unique_ptr<LevelDBContext> m_db_context;
 193 | +    util::NotNull<std::unique_ptr<LevelDBContext>> m_db_context;

l0rinc commented at 11:10 AM on May 23, 2026:

faef857 refactor: Use util::NotNull<std::unique_ptr<LevelDBContext>> m_db_context:

Could we avoid repeating the verbose template instantiation for all non-null smart pointers? I like the extra guarantee, but I don't like how verbose the results are. We could add NotNullUniquePtr and NotNullSharedPtr aliases and demo them at the mentioned call sites:

diff --git a/src/dbwrapper.h b/src/dbwrapper.h
index 4c0b6e2629..46ce177c5c 100644
--- a/src/dbwrapper.h
+++ b/src/dbwrapper.h
@@ -189,7 +189,7 @@ class CDBWrapper
     friend const Obfuscation& dbwrapper_private::GetObfuscation(const CDBWrapper&);
 private:
     //! holds all leveldb-specific fields of this class
-    util::NotNull<std::unique_ptr<LevelDBContext>> m_db_context;
+    util::NotNullUniquePtr<LevelDBContext> m_db_context;
 
     //! the name of this database
     std::string m_name;
diff --git a/src/net.cpp b/src/net.cpp
index f4c5dbd157..fab392814e 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -4004,12 +4004,12 @@ ServiceFlags CConnman::GetLocalServices() const
     return m_local_services;
 }
 
-static util::NotNull<std::unique_ptr<Transport>> MakeTransport(NodeId id, bool use_v2transport, bool inbound) noexcept
+static util::NotNullUniquePtr<Transport> MakeTransport(NodeId id, bool use_v2transport, bool inbound) noexcept
 {
     if (use_v2transport) {
-        return util::NotNull<std::unique_ptr<Transport>>{std::make_unique<V2Transport>(id, /*initiating=*/!inbound)};
+        return util::NotNullUniquePtr<Transport>{std::make_unique<V2Transport>(id, /*initiating=*/!inbound)};
     } else {
-        return util::NotNull<std::unique_ptr<Transport>>{std::make_unique<V1Transport>(id)};
+        return util::NotNullUniquePtr<Transport>{std::make_unique<V1Transport>(id)};
     }
 }
 
diff --git a/src/net.h b/src/net.h
index e6a680e048..0dbb692149 100644
--- a/src/net.h
+++ b/src/net.h
@@ -682,7 +682,7 @@ class CNode
 public:
     /** Transport serializer/deserializer. The receive side functions are only called under cs_vRecv, while
      * the sending side functions are only called under cs_vSend. */
-    const util::NotNull<std::unique_ptr<Transport>> m_transport;
+    const util::NotNullUniquePtr<Transport> m_transport;
 
     const NetPermissionFlags m_permission_flags;
 
diff --git a/src/test/util_pointers_tests.cpp b/src/test/util_pointers_tests.cpp
index 5d77e89e15..38c49a5cd6 100644
--- a/src/test/util_pointers_tests.cpp
+++ b/src/test/util_pointers_tests.cpp
@@ -7,9 +7,14 @@
 
 #include <boost/test/unit_test.hpp>
 
+#include <memory>
 #include <set>
+#include <type_traits>
 #include <unordered_set>
 
+static_assert(std::is_same_v<util::NotNullUniquePtr<int>, util::NotNull<std::unique_ptr<int>>>);
+static_assert(std::is_same_v<util::NotNullSharedPtr<int>, util::NotNull<std::shared_ptr<int>>>);
+
 BOOST_AUTO_TEST_SUITE(util_pointers_tests)
 
 BOOST_AUTO_TEST_CASE(check_nullptr)
diff --git a/src/util/pointers.h b/src/util/pointers.h
index 08af3c1b28..e757c6012b 100644
--- a/src/util/pointers.h
+++ b/src/util/pointers.h
@@ -16,6 +16,8 @@
 //    - strict_make_not_null, because it is not needed.
 // * Remove the not_null->strict_not_null converting constructors, because they
 //   are not needed.
+// * Add NotNullUniquePtr and NotNullSharedPtr aliases to keep smart-pointer
+//   call sites readable.
 //
 // All original code is covered by:
 
@@ -357,6 +359,12 @@ struct NotNull : public gsl_detail::strict_not_null<T> {
 template <typename T>
 NotNull(T) -> NotNull<T>;
 
+template <typename T, typename Deleter = std::default_delete<T>>
+using NotNullUniquePtr = NotNull<std::unique_ptr<T, Deleter>>;
+
+template <typename T>
+using NotNullSharedPtr = NotNull<std::shared_ptr<T>>;
+
 } // namespace util
 
 namespace std

maflcko commented at 6:25 PM on May 26, 2026:

thx, done

in src/util/pointers.h:115 in fa1e0bb7d0 outdated

 110 | +    static_assert(details::is_comparable_to_nullptr<T>::value, "T cannot be compared to nullptr.");
 111 | +
 112 | +    using element_type = T;
 113 | +
 114 | +    template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
 115 | +    constexpr not_null(U&& u) noexcept(std::is_nothrow_move_constructible<T>::value) : ptr_(std::forward<U>(u))

l0rinc commented at 11:17 AM on May 23, 2026:

fa1e0bb util: Add util::NotNull<AnyPtrType>:

The noexcept annotations are a bit confusing since that's exactly how we're testing the methods - check_nullptr works because it deliberately avoids the noexcept path. I understand that test_only_CheckFailuresAreExceptionsNotAborts isn't used in prod and we don't like modifying prod for tests, but we do have a failure case inside and I'm just not sure what happens if we are explicit about not throwing while deliberately relying on throwing behavior (currently this terminates before the test can observe NonFatalCheckError. Maybe that is acceptable for production, but it makes the main smart-pointer null checks harder to test).

This way we can't test for example:

BOOST_AUTO_TEST_CASE(check_null_smart_pointer)
{
    test_only_CheckFailuresAreExceptionsNotAborts mock_checks{};

    BOOST_CHECK_THROW(util::NotNull{std::unique_ptr<int>{}}, NonFatalCheckError);
    BOOST_CHECK_THROW(util::NotNull{std::shared_ptr<int>{}}, NonFatalCheckError);
}

it just fails with

unknown location:0: fatal error: in "util_pointers_tests/check_null_smart_pointer": signal: SIGABRT (application abort requested)

But without the noexcept the above passes:

diff --git a/src/util/pointers.h b/src/util/pointers.h
index f985c5331b..28dd458f58 100644
--- a/src/util/pointers.h
+++ b/src/util/pointers.h
@@ -116,19 +116,19 @@ public:
     using element_type = T;
 
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr not_null(U&& u) noexcept(std::is_nothrow_move_constructible<T>::value) : ptr_(std::forward<U>(u))
+    constexpr not_null(U&& u) : ptr_(std::forward<U>(u))
     {
         Assert(ptr_ != nullptr);
     }
 
     template <typename = std::enable_if_t<!std::is_same<std::nullptr_t, T>::value>>
-    constexpr not_null(T u) noexcept(std::is_nothrow_move_constructible<T>::value) : ptr_(std::move(u))
+    constexpr not_null(T u) : ptr_(std::move(u))
     {
         Assert(ptr_ != nullptr);
     }
 
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr not_null(const not_null<U>& other) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null(other.get())
+    constexpr not_null(const not_null<U>& other) : not_null(other.get())
     {}
 
     not_null(const not_null& other) = default;
@@ -169,7 +169,7 @@ void swap(not_null<T>& a, not_null<T>& b) noexcept
 }
 
 template <class T>
-auto make_not_null(T&& t) noexcept
+auto make_not_null(T&& t)
 {
     return not_null<std::remove_cv_t<std::remove_reference_t<T>>>{std::forward<T>(t)};
 }
@@ -284,15 +284,15 @@ class strict_not_null : public not_null<T>
 {
 public:
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr explicit strict_not_null(U&& u) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null<T>(std::forward<U>(u))
+    constexpr explicit strict_not_null(U&& u) : not_null<T>(std::forward<U>(u))
     {}
 
     template <typename = std::enable_if_t<!std::is_same<std::nullptr_t, T>::value>>
-    constexpr explicit strict_not_null(T u) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null<T>(std::move(u))
+    constexpr explicit strict_not_null(T u) : not_null<T>(std::move(u))
     {}
 
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr strict_not_null(const strict_not_null<U>& other) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null<T>(other)
+    constexpr strict_not_null(const strict_not_null<U>& other) : not_null<T>(other)
     {}
 
     // To avoid invalidating the "not null" invariant, the contained pointer is actually copied
@@ -326,7 +326,7 @@ template <class T>
 strict_not_null<T> operator+(std::ptrdiff_t, const strict_not_null<T>&) = delete;
 
 template <class T>
-auto make_strict_not_null(T&& t) noexcept
+auto make_strict_not_null(T&& t)
 {
     return strict_not_null<std::remove_cv_t<std::remove_reference_t<T>>>{std::forward<T>(t)};
 }

maflcko commented at 4:57 PM on May 26, 2026:

Is this still relevant after the last push?

l0rinc commented at 10:27 AM on June 2, 2026:

Can be resolved, thanks

in src/net.cpp:4012 in 77773e8c85

4010 |      if (use_v2transport) {
4011 | -        return std::make_unique<V2Transport>(id, /*initiating=*/!inbound);
4012 | +        return util::NotNull<std::unique_ptr<Transport>>{std::make_unique<V2Transport>(id, /*initiating=*/!inbound)};
4013 |      } else {
4014 | -        return std::make_unique<V1Transport>(id);
4015 | +        return util::NotNull<std::unique_ptr<Transport>>{std::make_unique<V1Transport>(id)};

l0rinc commented at 11:48 AM on May 23, 2026:

77773e8 refactor: In CNode use util::NotNull<std::unique_ptr<Transport>> m_transport:

This extra verbosity is a bit of a turn-off. Can we add a forwarding constructor for class-type pointer wrappers (diff assumes the previous NotNullUniquePtr suggestion)?

diff --git a/src/net.cpp b/src/net.cpp
index fab392814e..aaf946f4ab 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -4007,9 +4007,9 @@ ServiceFlags CConnman::GetLocalServices() const
 static util::NotNullUniquePtr<Transport> MakeTransport(NodeId id, bool use_v2transport, bool inbound) noexcept
 {
     if (use_v2transport) {
-        return util::NotNullUniquePtr<Transport>{std::make_unique<V2Transport>(id, /*initiating=*/!inbound)};
+        return std::make_unique<V2Transport>(id, /*initiating=*/!inbound);
     } else {
-        return util::NotNullUniquePtr<Transport>{std::make_unique<V1Transport>(id)};
+        return std::make_unique<V1Transport>(id);
     }
 }
 
diff --git a/src/test/util_pointers_tests.cpp b/src/test/util_pointers_tests.cpp
index 38c49a5cd6..85807f8e73 100644
--- a/src/test/util_pointers_tests.cpp
+++ b/src/test/util_pointers_tests.cpp
@@ -63,6 +63,7 @@ BOOST_AUTO_TEST_CASE(check_swap)
 BOOST_AUTO_TEST_CASE(check_deref)
 {
     int v{2};
+    static_assert(!std::is_convertible_v<int*, util::NotNull<int*>>); // Keep raw-pointer NotNull construction explicit.
     util::NotNull p(&v);
     *p = 3;
     BOOST_CHECK_EQUAL(v, 3);
diff --git a/src/util/pointers.h b/src/util/pointers.h
index e757c6012b..73bb45e3e7 100644
--- a/src/util/pointers.h
+++ b/src/util/pointers.h
@@ -18,6 +18,7 @@
 //   are not needed.
 // * Add NotNullUniquePtr and NotNullSharedPtr aliases to keep smart-pointer
 //   call sites readable.
+// * Add a forwarding constructor for concise non-null smart-pointer returns.
 //
 // All original code is covered by:
 
@@ -353,8 +354,13 @@ struct hash<gsl_detail::strict_not_null<T>> : gsl_detail::not_null_hash<gsl_deta
 namespace util {
 
 template <class T>
-struct NotNull : public gsl_detail::strict_not_null<T> {
-    using gsl_detail::strict_not_null<T>::strict_not_null;
+struct NotNull : gsl_detail::strict_not_null<T> {
+    using Base = gsl_detail::strict_not_null<T>;
+    using Base::Base;
+
+    template <typename U>
+    requires (!std::is_pointer_v<T> && std::is_convertible_v<U, T>)
+    constexpr NotNull(U&& u) : Base{std::forward<U>(u)} {}
 };
 template <typename T>
 NotNull(T) -> NotNull<T>;

Which obviously begs the question: do we ever want to convert back. Given your hint for converting GetWalletForJSONRPCRequest it may be necessary (unless we propagate the type further), but I agree with you that this could also be done in a followup.

<details><summary>Migrate `GetWalletForJSONRPCRequest` to `util::NotNullSharedPtr<CWallet>`</summary>

diff --git a/src/util/pointers.h b/src/util/pointers.h
index 8f740472a3..5826e21cdc 100644
--- a/src/util/pointers.h
+++ b/src/util/pointers.h
@@ -21,6 +21,8 @@
 // * Add a forwarding constructor for concise non-null smart-pointer returns.
 // * Delete util::NotNull moves so the wrapper does not advertise misleading
 //   move operations.
+// * Add compatible shared_ptr conversions so NotNullSharedPtr<T> can initialize
+//   std::shared_ptr<const T> callers without rebuilding the handle.
 //
 // All original code is covered by:
 
@@ -78,6 +80,12 @@ namespace details
                                             const T,
                                             const T&>;
 
+    template <typename T>
+    struct is_shared_ptr : std::false_type {};
+
+    template <typename T>
+    struct is_shared_ptr<std::shared_ptr<T>> : std::true_type {};
+
 } // namespace details
 
 //
@@ -380,6 +388,10 @@ struct NotNull : gsl_detail::strict_not_null<T> {
     // underlying smart pointer at transfer boundaries.
     NotNull(NotNull&&) = delete;
     NotNull& operator=(NotNull&&) = delete;
+
+    template <typename U>
+        requires (!std::is_same_v<U, T> && gsl_detail::details::is_shared_ptr<T>::value && gsl_detail::details::is_shared_ptr<U>::value && std::is_convertible_v<T, U>)
+    constexpr operator U() const { return this->get(); }
 };
 template <typename T>
 NotNull(T) -> NotNull<T>;
diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index ed966d8944..9082407d6d 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -39,7 +39,6 @@ RPCMethod getnewaddress()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     LOCK(pwallet->cs_wallet);
 
@@ -88,7 +87,6 @@ RPCMethod getrawchangeaddress()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     LOCK(pwallet->cs_wallet);
 
@@ -132,7 +130,6 @@ RPCMethod setlabel()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     LOCK(pwallet->cs_wallet);
 
@@ -183,7 +180,6 @@ RPCMethod listaddressgroupings()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -232,7 +228,6 @@ RPCMethod keypoolrefill()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     LOCK(pwallet->cs_wallet);
 
@@ -423,7 +418,6 @@ RPCMethod getaddressinfo()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     LOCK(pwallet->cs_wallet);
 
@@ -536,7 +530,6 @@ RPCMethod getaddressesbylabel()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     LOCK(pwallet->cs_wallet);
 
@@ -600,7 +593,6 @@ RPCMethod listlabels()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     LOCK(pwallet->cs_wallet);
 
@@ -648,7 +640,6 @@ RPCMethod walletdisplayaddress()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
         {
             std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
-            if (!wallet) return UniValue::VNULL;
             CWallet* const pwallet = wallet.get();
 
             LOCK(pwallet->cs_wallet);
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 396be62825..a1cc43613c 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -50,7 +50,6 @@ RPCMethod importprunedfunds()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     CMutableTransaction tx;
     if (!DecodeHexTx(tx, request.params[0].get_str())) {
@@ -108,7 +107,6 @@ RPCMethod removeprunedfunds()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     LOCK(pwallet->cs_wallet);
 
@@ -377,7 +375,6 @@ RPCMethod importdescriptors()
         [](const RPCMethod& self, const JSONRPCRequest& main_request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(main_request);
-    if (!pwallet) return UniValue::VNULL;
     CWallet& wallet{*pwallet};
 
     // Make sure the results are valid at least up to the most recent block
@@ -515,7 +512,6 @@ RPCMethod listdescriptors()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> wallet = GetWalletForJSONRPCRequest(request);
-    if (!wallet) return UniValue::VNULL;
 
     const bool priv = !request.params[0].isNull() && request.params[0].get_bool();
     if (wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && priv) {
@@ -608,7 +604,6 @@ RPCMethod backupwallet()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
index ab869b0d3f..8eadaf1ded 100644
--- a/src/wallet/rpc/coins.cpp
+++ b/src/wallet/rpc/coins.cpp
@@ -105,7 +105,6 @@ RPCMethod getreceivedbyaddress()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -147,7 +146,6 @@ RPCMethod getreceivedbylabel()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -188,7 +186,6 @@ RPCMethod getbalance()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -258,7 +255,6 @@ RPCMethod lockunspent()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -376,7 +372,6 @@ RPCMethod listlockunspent()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     LOCK(pwallet->cs_wallet);
 
@@ -424,7 +419,6 @@ RPCMethod getbalances()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> rpc_wallet = GetWalletForJSONRPCRequest(request);
-    if (!rpc_wallet) return UniValue::VNULL;
     const CWallet& wallet = *rpc_wallet;
 
     // Make sure the results are valid at least up to the most recent block
@@ -520,7 +514,6 @@ RPCMethod listunspent()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     int nMinDepth = 1;
     if (!request.params[0].isNull()) {
diff --git a/src/wallet/rpc/encrypt.cpp b/src/wallet/rpc/encrypt.cpp
index 68a80eb80e..ee2327ab98 100644
--- a/src/wallet/rpc/encrypt.cpp
+++ b/src/wallet/rpc/encrypt.cpp
@@ -35,7 +35,6 @@ RPCMethod walletpassphrase()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
-    if (!wallet) return UniValue::VNULL;
     CWallet* const pwallet = wallet.get();
 
     int64_t nSleepTime;
@@ -132,7 +131,6 @@ RPCMethod walletpassphrasechange()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     if (!pwallet->HasEncryptionKeys()) {
         throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an unencrypted wallet, but walletpassphrasechange was called.");
@@ -197,7 +195,6 @@ RPCMethod walletlock()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     if (!pwallet->HasEncryptionKeys()) {
         throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an unencrypted wallet, but walletlock was called.");
@@ -250,7 +247,6 @@ RPCMethod encryptwallet()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
         throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: wallet does not contain private keys, nothing to encrypt.");
diff --git a/src/wallet/rpc/signmessage.cpp b/src/wallet/rpc/signmessage.cpp
index bd49f3e393..9073bf5762 100644
--- a/src/wallet/rpc/signmessage.cpp
+++ b/src/wallet/rpc/signmessage.cpp
@@ -37,7 +37,6 @@ RPCMethod signmessage()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
         {
             const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-            if (!pwallet) return UniValue::VNULL;
 
             LOCK(pwallet->cs_wallet);
 
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index b6cdc8600f..e90e0772fa 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -288,7 +288,6 @@ RPCMethod sendtoaddress()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -392,7 +391,6 @@ RPCMethod sendmany()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -799,7 +797,6 @@ RPCMethod fundrawtransaction()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // parse hex string from parameter
     CMutableTransaction tx;
@@ -900,7 +897,6 @@ RPCMethod signrawtransactionwithwallet()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     CMutableTransaction mtx;
     if (!DecodeHexTx(mtx, request.params[0].get_str())) {
@@ -1033,7 +1029,6 @@ static RPCMethod bumpfee_helper(std::string method_name)
         [want_psbt](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !pwallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER) && !want_psbt) {
         throw JSONRPCError(RPC_WALLET_ERROR, "bumpfee is not available with wallets that have private keys disabled. Use psbtbumpfee instead.");
@@ -1263,7 +1258,6 @@ RPCMethod send()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
         {
             std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-            if (!pwallet) return UniValue::VNULL;
 
             UniValue options{request.params[4].isNull() ? UniValue::VOBJ : request.params[4]};
             InterpretFeeEstimationInstructions(/*conf_target=*/request.params[1], /*estimate_mode=*/request.params[2], /*fee_rate=*/request.params[3], options);
@@ -1377,7 +1371,6 @@ RPCMethod sendall()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
         {
             std::shared_ptr<CWallet> const pwallet{GetWalletForJSONRPCRequest(request)};
-            if (!pwallet) return UniValue::VNULL;
             // Make sure the results are valid at least up to the most recent block
             // the user could have gotten from another RPC command prior to now
             pwallet->BlockUntilSyncedToCurrentChain();
@@ -1623,7 +1616,6 @@ RPCMethod walletprocesspsbt()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     const CWallet& wallet{*pwallet};
     // Make sure the results are valid at least up to the most recent block
@@ -1755,7 +1747,6 @@ RPCMethod walletcreatefundedpsbt()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     CWallet& wallet{*pwallet};
     // Make sure the results are valid at least up to the most recent block
diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp
index 038e30fceb..784f8c2f92 100644
--- a/src/wallet/rpc/transactions.cpp
+++ b/src/wallet/rpc/transactions.cpp
@@ -225,7 +225,6 @@ RPCMethod listreceivedbyaddress()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -270,7 +269,6 @@ RPCMethod listreceivedbylabel()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -466,7 +464,6 @@ RPCMethod listtransactions()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -577,7 +574,6 @@ RPCMethod listsinceblock()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     const CWallet& wallet = *pwallet;
     // Make sure the results are valid at least up to the most recent block
@@ -719,7 +715,6 @@ RPCMethod gettransaction()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -796,7 +791,6 @@ RPCMethod abandontransaction()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -844,7 +838,6 @@ RPCMethod rescanblockchain()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
     CWallet& wallet{*pwallet};
 
     // Make sure the results are valid at least up to the most recent block
@@ -932,7 +925,6 @@ RPCMethod abortrescan()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     if (!pwallet->IsScanning() || pwallet->IsAbortingRescan()) return false;
     pwallet->AbortRescan();
diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp
index 77a8745ced..80afdf7107 100644
--- a/src/wallet/rpc/util.cpp
+++ b/src/wallet/rpc/util.cpp
@@ -59,7 +59,7 @@ std::optional<std::string> GetWalletNameFromJSONRPCRequest(const JSONRPCRequest&
     return std::nullopt;
 }
 
-std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
+util::NotNullSharedPtr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
 {
     CHECK_NONFATAL(request.mode == JSONRPCRequest::EXECUTE);
     WalletContext& context = EnsureWalletContext(request.context);
diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h
index 88fdc6639f..0a77c0d906 100644
--- a/src/wallet/rpc/util.h
+++ b/src/wallet/rpc/util.h
@@ -7,6 +7,7 @@
 
 #include <rpc/util.h>
 #include <script/script.h>
+#include <util/pointers.h>
 #include <wallet/wallet.h>
 
 #include <any>
@@ -36,9 +37,9 @@ static const RPCResult RESULT_LAST_PROCESSED_BLOCK { RPCResult::Type::OBJ, "last
  * Figures out what wallet, if any, to use for a JSONRPCRequest.
  *
  * [@param](/bitcoin-bitcoin/contributor/param/)[in] request JSONRPCRequest that wishes to access a wallet
- * [@return](/bitcoin-bitcoin/contributor/return/) nullptr if no wallet should be used, or a pointer to the CWallet
+ * [@return](/bitcoin-bitcoin/contributor/return/) a pointer to the selected CWallet, or throws if no wallet can be selected
  */
-std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request);
+util::NotNullSharedPtr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request);
 std::optional<std::string> GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request);
 /**
  * Ensures that a wallet name is specified across the endpoint and wallet_name.
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
index 8aa15c7ec5..72beaa6078 100644
--- a/src/wallet/rpc/wallet.cpp
+++ b/src/wallet/rpc/wallet.cpp
@@ -72,7 +72,6 @@ static RPCMethod getwalletinfo()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -304,7 +303,6 @@ static RPCMethod setwalletflag()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
 
     std::string flag_str = request.params[0].get_str();
     bool value = request.params[1].isNull() || request.params[1].get_bool();
@@ -516,7 +514,6 @@ RPCMethod simulaterawtransaction()
     [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
 {
     const std::shared_ptr<const CWallet> rpc_wallet = GetWalletForJSONRPCRequest(request);
-    if (!rpc_wallet) return UniValue::VNULL;
     const CWallet& wallet = *rpc_wallet;
 
     LOCK(wallet.cs_wallet);
@@ -672,7 +669,6 @@ RPCMethod gethdkeys()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
         {
             const std::shared_ptr<const CWallet> wallet = GetWalletForJSONRPCRequest(request);
-            if (!wallet) return UniValue::VNULL;
 
             LOCK(wallet->cs_wallet);
 
@@ -770,7 +766,6 @@ static RPCMethod createwalletdescriptor()
         [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
         {
             std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-            if (!pwallet) return UniValue::VNULL;
 
             std::optional<OutputType> output_type = ParseOutputType(request.params[0].get_str());
             if (!output_type) {
@@ -860,7 +855,6 @@ RPCMethod addhdkey()
         [&](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
         {
             std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
-            if (!wallet) return UniValue::VNULL;
 
             if (wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
                 throw JSONRPCError(RPC_WALLET_ERROR, "addhdkey is not available for wallets without private keys");

</details>

maflcko commented at 6:19 PM on May 26, 2026:

GetWalletForJSONRPCRequest

Thx, but I think the diff is not quite right:

I think not-null ctors should be explicit, where possible, so the function should return util::NotNull{pwallet};
I think we want to ideally keep the not-null type for as long as possible. So decaying to a nullable shared_ptr and removing the null checks seems inconsistent.

The correct diff would be:

diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index ed966d8944..a13f2baaeb 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -40,4 +40,3 @@ RPCMethod getnewaddress()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -89,4 +88,3 @@ RPCMethod getrawchangeaddress()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -133,4 +131,3 @@ RPCMethod setlabel()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -184,4 +181,3 @@ RPCMethod listaddressgroupings()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -233,4 +229,3 @@ RPCMethod keypoolrefill()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -424,4 +419,3 @@ RPCMethod getaddressinfo()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -537,4 +531,3 @@ RPCMethod getaddressesbylabel()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -601,4 +594,3 @@ RPCMethod listlabels()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -649,5 +641,3 @@ RPCMethod walletdisplayaddress()
         {
-            std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
-            if (!wallet) return UniValue::VNULL;
-            CWallet* const pwallet = wallet.get();
+            const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 396be62825..7113799f1c 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -51,4 +51,3 @@ RPCMethod importprunedfunds()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -109,4 +108,3 @@ RPCMethod removeprunedfunds()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -378,4 +376,3 @@ RPCMethod importdescriptors()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(main_request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(main_request)};
     CWallet& wallet{*pwallet};
@@ -516,4 +513,3 @@ RPCMethod listdescriptors()
 {
-    const std::shared_ptr<const CWallet> wallet = GetWalletForJSONRPCRequest(request);
-    if (!wallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> wallet{GetWalletForJSONRPCRequest(request)};
 
@@ -609,4 +605,3 @@ RPCMethod backupwallet()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
index ab869b0d3f..0e954e9146 100644
--- a/src/wallet/rpc/coins.cpp
+++ b/src/wallet/rpc/coins.cpp
@@ -106,4 +106,3 @@ RPCMethod getreceivedbyaddress()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -148,4 +147,3 @@ RPCMethod getreceivedbylabel()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -189,4 +187,3 @@ RPCMethod getbalance()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -259,4 +256,3 @@ RPCMethod lockunspent()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -377,4 +373,3 @@ RPCMethod listlockunspent()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -425,4 +420,3 @@ RPCMethod getbalances()
 {
-    const std::shared_ptr<const CWallet> rpc_wallet = GetWalletForJSONRPCRequest(request);
-    if (!rpc_wallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> rpc_wallet{GetWalletForJSONRPCRequest(request)};
     const CWallet& wallet = *rpc_wallet;
@@ -521,4 +515,3 @@ RPCMethod listunspent()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
diff --git a/src/wallet/rpc/encrypt.cpp b/src/wallet/rpc/encrypt.cpp
index 68a80eb80e..85bd0b4791 100644
--- a/src/wallet/rpc/encrypt.cpp
+++ b/src/wallet/rpc/encrypt.cpp
@@ -36,5 +36,3 @@ RPCMethod walletpassphrase()
 {
-    std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
-    if (!wallet) return UniValue::VNULL;
-    CWallet* const pwallet = wallet.get();
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -100,3 +98,3 @@ RPCMethod walletpassphrase()
     // is acquired in the callback then the wallet is still loaded.
-    std::weak_ptr<CWallet> weak_wallet = wallet;
+    std::weak_ptr<CWallet> weak_wallet{pwallet.get()};
     context.scheduler->scheduleFromNow([weak_wallet, relock_time] {
@@ -133,4 +131,3 @@ RPCMethod walletpassphrasechange()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -198,4 +195,3 @@ RPCMethod walletlock()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -251,4 +247,3 @@ RPCMethod encryptwallet()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
diff --git a/src/wallet/rpc/signmessage.cpp b/src/wallet/rpc/signmessage.cpp
index bd49f3e393..df7cee1a28 100644
--- a/src/wallet/rpc/signmessage.cpp
+++ b/src/wallet/rpc/signmessage.cpp
@@ -38,4 +38,3 @@ RPCMethod signmessage()
         {
-            const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-            if (!pwallet) return UniValue::VNULL;
+            const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index b6cdc8600f..f1623fa662 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -289,4 +289,3 @@ RPCMethod sendtoaddress()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -393,4 +392,3 @@ RPCMethod sendmany()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -800,4 +798,3 @@ RPCMethod fundrawtransaction()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -901,4 +898,3 @@ RPCMethod signrawtransactionwithwallet()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -1034,4 +1030,3 @@ static RPCMethod bumpfee_helper(std::string method_name)
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -1264,4 +1259,3 @@ RPCMethod send()
         {
-            std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-            if (!pwallet) return UniValue::VNULL;
+            const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -1379,3 +1373,2 @@ RPCMethod sendall()
             std::shared_ptr<CWallet> const pwallet{GetWalletForJSONRPCRequest(request)};
-            if (!pwallet) return UniValue::VNULL;
             // Make sure the results are valid at least up to the most recent block
@@ -1624,4 +1617,3 @@ RPCMethod walletprocesspsbt()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -1756,4 +1748,3 @@ RPCMethod walletcreatefundedpsbt()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp
index f69082e1e9..8d0d65aea1 100644
--- a/src/wallet/rpc/transactions.cpp
+++ b/src/wallet/rpc/transactions.cpp
@@ -228,4 +228,3 @@ RPCMethod listreceivedbyaddress()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -273,4 +272,3 @@ RPCMethod listreceivedbylabel()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -469,4 +467,3 @@ RPCMethod listtransactions()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -580,4 +577,3 @@ RPCMethod listsinceblock()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -722,4 +718,3 @@ RPCMethod gettransaction()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -799,4 +794,3 @@ RPCMethod abandontransaction()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -847,4 +841,3 @@ RPCMethod rescanblockchain()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
     CWallet& wallet{*pwallet};
@@ -935,4 +928,3 @@ RPCMethod abortrescan()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp
index 77a8745ced..9d29661eca 100644
--- a/src/wallet/rpc/util.cpp
+++ b/src/wallet/rpc/util.cpp
@@ -61,3 +61,3 @@ std::optional<std::string> GetWalletNameFromJSONRPCRequest(const JSONRPCRequest&
 
-std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
+util::NotNullSharedPtr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
 {
@@ -69,3 +69,3 @@ std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& reques
         if (!pwallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
-        return pwallet;
+        return util::NotNull{pwallet};
     }
@@ -74,3 +74,3 @@ std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& reques
     auto wallet = GetDefaultWallet(context, count);
-    if (wallet) return wallet;
+    if (wallet) return util::NotNull{wallet};
 
diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h
index 88fdc6639f..0a77c0d906 100644
--- a/src/wallet/rpc/util.h
+++ b/src/wallet/rpc/util.h
@@ -9,2 +9,3 @@
 #include <script/script.h>
+#include <util/pointers.h>
 #include <wallet/wallet.h>
@@ -38,5 +39,5 @@ static const RPCResult RESULT_LAST_PROCESSED_BLOCK { RPCResult::Type::OBJ, "last
  * [@param](/bitcoin-bitcoin/contributor/param/)[in] request JSONRPCRequest that wishes to access a wallet
- * [@return](/bitcoin-bitcoin/contributor/return/) nullptr if no wallet should be used, or a pointer to the CWallet
+ * [@return](/bitcoin-bitcoin/contributor/return/) a pointer to the selected CWallet, or throws if no wallet can be selected
  */
-std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request);
+util::NotNullSharedPtr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request);
 std::optional<std::string> GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request);
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
index 8aa15c7ec5..c3569ec118 100644
--- a/src/wallet/rpc/wallet.cpp
+++ b/src/wallet/rpc/wallet.cpp
@@ -73,4 +73,3 @@ static RPCMethod getwalletinfo()
 {
-    const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -305,4 +304,3 @@ static RPCMethod setwalletflag()
 {
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -517,4 +515,3 @@ RPCMethod simulaterawtransaction()
 {
-    const std::shared_ptr<const CWallet> rpc_wallet = GetWalletForJSONRPCRequest(request);
-    if (!rpc_wallet) return UniValue::VNULL;
+    const util::NotNullSharedPtr<const CWallet> rpc_wallet{GetWalletForJSONRPCRequest(request)};
     const CWallet& wallet = *rpc_wallet;
@@ -673,4 +670,3 @@ RPCMethod gethdkeys()
         {
-            const std::shared_ptr<const CWallet> wallet = GetWalletForJSONRPCRequest(request);
-            if (!wallet) return UniValue::VNULL;
+            const util::NotNullSharedPtr<const CWallet> wallet{GetWalletForJSONRPCRequest(request)};
 
@@ -771,4 +767,3 @@ static RPCMethod createwalletdescriptor()
         {
-            std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-            if (!pwallet) return UniValue::VNULL;
+            const util::NotNullSharedPtr<CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
 
@@ -861,4 +856,3 @@ RPCMethod addhdkey()
         {
-            std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
-            if (!wallet) return UniValue::VNULL;
+            const util::NotNullSharedPtr<CWallet> wallet{GetWalletForJSONRPCRequest(request)};

</details>

However, this raises the concern that thread-safety annotations will be broken by this.

I guess this makes sense, as clang can not figure out that the operator-> returns the same smart wallet pointer every time it is called. So I guess my above diff is still incomplete and would likely require a change to first assign a wallet reference:

const util::NotNullSharedPtr<const CWallet> pwallet{GetWalletForJSONRPCRequest(request)};
const CWallet& wallet{*pwallet};
LOCK(wallet.cs_wallet);
wallet.DoThing();
...

l0rinc commented at 10:43 AM on June 2, 2026:

static_assert(!std::is_convertible_v<int*, util::NotNull<int*>>); // Keep raw-pointer NotNull construction explicit.

This still seems like something we could still assert for safety, i.e. that raw pointers do not implicitly convert to util::NotNull.

The verbosity still bothers me, but every other workaround I found was too complicated - we can simplify in a followup, if needed.

maflcko commented at 5:19 PM on June 16, 2026:

static_assert(!std::is_convertible_v<int*, util::NotNull<int*>>); // Keep raw-pointer NotNull construction explicit.

This still seems like something we could still assert for safety, i.e. that raw pointers do not implicitly convert to util::NotNull.

This should be true for all pointer types, because the constructors are intentionally marked explicit. The rationale is that the constructor acts like an Assert (and calls Assert).

Also, raw pointers should just use reference_wrapper, not this class. Happy to add checks for smart pointer types, if you want.

The verbosity still bothers me, but every other workaround I found was too complicated - we can simplify in a followup, if needed.

So this is intentional and shouldn't be changed in the future :sweat_smile:

maflcko commented at 5:48 PM on June 16, 2026:

However, this raises the concern that thread-safety annotations will be broken by this.

Added a unit test to document this, and the recommended workaround of taking a reference while holding the lock.

maflcko commented at 2:58 PM on June 18, 2026:

I guess this thread can be resolved? The only remaining thing could be to apply the diff (https://github.com/bitcoin/bitcoin/pull/34844#discussion_r3305943200) in the future?

in src/test/CMakeLists.txt:1 in fa1e0bb7d0 outdated

l0rinc commented at 12:39 PM on May 23, 2026:

It can be reviewed by calling curl -fL 'https://raw.githubusercontent.com/microsoft/GSL/688ffcde9018910bef22dae9da9de974803dc982/include/gsl/pointers' -o ./src/util/pointers.h

Could the first commit stay closer to the imported GSL header, with Bitcoin-specific changes applied in follow-up commits?

We could even cherry-pick it and add the above curl reproducer as a scripted diff if you want to push the boundaries of #35275 :D

maflcko commented at 11:08 AM on May 26, 2026:

if you want to push the boundaries

thx, done

stickies-v commented at 2:48 PM on July 10, 2026:

I'm not sure using gsl as a base makes sense for us. It seems very unlikely that this will ever be standardized, and we're already customizing it here quite a bit already. It seems like we don't care about the strict_not_null vs not_null separation, so we might as well avoid the complexity altogether?

I prototyped one such approach with claude (may still be missing edge cases etc, but overall looks pretty complete to me):

https://github.com/stickies-v/bitcoin/commit/3c10a9588ea12f081e1dc933e9f49291e44d8395 implements from scratch, with a much smaller diff. Slightly different behaviour, e.g. get() doesn't have the value_or_reference_return_t optimization, but since we don't want this to be used for raw pointers I think that's okay?
https://github.com/stickies-v/bitcoin/commit/5a99e4461ae628094e3665ad78f00a7560654d48 then goes a step further and prevents raw pointers from being used, since we can be as opinionated as we like on our own types.

(note: not sure if we should/need to better attribute gsl, i just removed the copyright notice for now as it's a rewrite)

maflcko commented at 10:32 AM on July 17, 2026:

Yeah, I liked the gsl header-only version and was hoping it is battle-tested, but given that (1) I had to fix-up some noexecpt edge-cases upstream, (2) we are making it movable, (3) we likely want to easily and freely fix it up ourselves, (4) the code should be easy to read in one go, and not as a patch on top of something else, it makes sense to just write a minimal, clean, and C++20 impl ourselves.

So I went over all lines in your branch, applied some small test fixups, and then pushed it here. Added you as co-author, as you pushed the commit/diff, but let me know if you prefer to be dropped as co-author.

stickies-v commented at 4:18 PM on July 21, 2026:

fa27f2e49fe6875769477f536735607c55140e16 nit: why not do this in the previous commit right away?

maflcko commented at 1:56 PM on August 2, 2026:

thx, squashed the two commits

in src/util/pointers.h:354 in fa1e0bb7d0 outdated

 349 | +} // namespace std
 350 | +
 351 | +namespace util {
 352 | +
 353 | +template <class T>
 354 | +struct NotNull : public gsl_detail::strict_not_null<T> {

l0rinc commented at 3:17 PM on May 23, 2026:

fa1e0bb util: Add util::NotNull<AnyPtrType>:

Seeing the comments in e.g. #24423 (comment), maybe we could document when this is preferred over other solutions, e.g. that ordinary references remain preferred for simple non-owning access.

maflcko commented at 5:10 PM on May 26, 2026:

thx, added a doxygen comment here.

in src/test/util_pointers_tests.cpp:25 in fa1e0bb7d0

  20 | +
  21 | +    // This unit test only works with a type that may throw on a move
  22 | +    // construction
  23 | +    struct ThrowingMoveNullPtr {
  24 | +        ThrowingMoveNullPtr() = default;
  25 | +        ThrowingMoveNullPtr(ThrowingMoveNullPtr&& other) {};

l0rinc commented at 3:51 PM on May 23, 2026:

fa1e0bb util: Add util::NotNull<AnyPtrType>:

Nit, we could make this explicit to avoid warnings:

        ThrowingMoveNullPtr(ThrowingMoveNullPtr&&) noexcept(false) {};

maflcko commented at 4:55 PM on May 26, 2026:

Turned into compile-time test instead.

in src/util/pointers.h:295 in fa1e0bb7d0 outdated

 290 | +    template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
 291 | +    constexpr strict_not_null(const strict_not_null<U>& other) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null<T>(other)
 292 | +    {}
 293 | +
 294 | +    // To avoid invalidating the "not null" invariant, the contained pointer is actually copied
 295 | +    // instead of moved. If it is a custom pointer, its constructor could in theory throw exceptions.

l0rinc commented at 4:41 PM on May 23, 2026:

fa1e0bb util: Add util::NotNull<AnyPtrType>:

NotNullUniquePtr can look move-constructible to traits even though actual move construction fails. I understand if we keep this, but could we avoid exposing move construction for util::NotNull? Explicitly deleting move construction/assignment in the public util::NotNull wrapper would make the API surface match the intended contract more directly.

diff --git a/src/test/util_pointers_tests.cpp b/src/test/util_pointers_tests.cpp
index 85807f8e73..3640e6ccfd 100644
--- a/src/test/util_pointers_tests.cpp
+++ b/src/test/util_pointers_tests.cpp
@@ -14,6 +14,11 @@
 
 static_assert(std::is_same_v<util::NotNullUniquePtr<int>, util::NotNull<std::unique_ptr<int>>>);
 static_assert(std::is_same_v<util::NotNullSharedPtr<int>, util::NotNull<std::shared_ptr<int>>>);
+static_assert(std::is_copy_constructible_v<util::NotNullSharedPtr<int>>);
+static_assert(std::is_copy_assignable_v<util::NotNullSharedPtr<int>>);
+static_assert(!std::is_move_constructible_v<util::NotNullUniquePtr<int>>);
+static_assert(!std::is_move_constructible_v<util::NotNullSharedPtr<int>>);
+static_assert(!std::is_move_assignable_v<util::NotNullSharedPtr<int>>);
 
 BOOST_AUTO_TEST_SUITE(util_pointers_tests)
 
diff --git a/src/util/pointers.h b/src/util/pointers.h
index 73bb45e3e7..948f7021f7 100644
--- a/src/util/pointers.h
+++ b/src/util/pointers.h
@@ -19,6 +19,8 @@
 // * Add NotNullUniquePtr and NotNullSharedPtr aliases to keep smart-pointer
 //   call sites readable.
 // * Add a forwarding constructor for concise non-null smart-pointer returns.
+// * Delete util::NotNull moves so the wrapper does not advertise misleading
+//   move operations.
 //
 // All original code is covered by:
 
@@ -361,6 +363,20 @@ struct NotNull : gsl_detail::strict_not_null<T> {
     template <typename U>
     requires (!std::is_pointer_v<T> && std::is_convertible_v<U, T>)
     constexpr NotNull(U&& u) : Base{std::forward<U>(u)} {}
+
+    NotNull(const NotNull&) = default;
+    NotNull& operator=(const NotNull&) = default;
+    template <typename U, std::enable_if_t<!std::is_same_v<U, T> && std::is_convertible_v<U, T>, bool> = true>
+    NotNull& operator=(const NotNull<U>& other)
+    {
+        Base::operator=(Base{other});
+        return *this;
+    }
+    // Delete move operations so NotNull does not expose misleading move syntax.
+    // Store it where the wrapper itself does not need to be moved; use the
+    // underlying smart pointer at transfer boundaries.
+    NotNull(NotNull&&) = delete;
+    NotNull& operator=(NotNull&&) = delete;
 };
 template <typename T>
 NotNull(T) -> NotNull<T>;

maflcko commented at 5:02 PM on May 26, 2026:

Heh, good point, but I think I'd like to go the other way and make it moveable. I have a commit for this, but removed it, see #34844 (comment). Move semantics can be done in a later commit/pull request.

edit: If we wait long enough, C++29 or later may ship with trivial reloc (or similar), which makes this safer to implement from a static analysis perspective :sweat_smile:

l0rinc commented at 10:33 AM on June 2, 2026:

Could we maybe document that it's not movable yet and remove it when it becomes movable? It's not intuitive what should happen in case of a move.

maflcko commented at 10:00 AM on June 16, 2026:

I pushed a doc-only update for now, but let me write a longer reply here in the future ...

maflcko commented at 5:13 PM on June 16, 2026:

Long answer:

NotNullUniquePtr can look move-constructible to traits even though actual move construction fails.

Correct, but this is harmless, because actual move-construction will correctly fail at compile time. So the only difference here is a cleaner error message.

Looking at your diff, you seem to be =delete ing the move ctor. This is fine, because it changes the compile error message to something more explicit. Also, the comment above the =delete has some rationale and recommended workarounds.

However, personally, I'd rather move toward making it movable (https://github.com/bitcoin/bitcoin/pull/34844#issuecomment-4290176661) instead of adding code that will be removed anyway when the switch is done. I already have the commit for this and I am happy to push it. At this point, it may be easier to just review one final state (movable) instead of two states?

l0rinc commented at 9:40 PM on June 16, 2026:

At this point, it may be easier to just review one final state (movable) instead of two states?

I'm fine with both - though I'm still not sure how move can be done safely with non-null source.

maflcko commented at 2:58 PM on June 18, 2026:

It can't be done safely in the language itself (unless C++29 gets trivial reloc or so). However, with clang-tidy use-after-move, this should be safe.

So I went ahead and pushed the move commmits.

maflcko commented at 10:14 AM on July 3, 2026:

(closing this thread, based on the thumbs-up)

in src/dbwrapper.h:206 in faef857079 outdated

 202 | @@ -202,7 +203,7 @@ class CDBWrapper
 203 |      std::optional<std::string> ReadImpl(std::span<const std::byte> key) const;
 204 |      bool ExistsImpl(std::span<const std::byte> key) const;
 205 |      size_t EstimateSizeImpl(std::span<const std::byte> key1, std::span<const std::byte> key2) const;
 206 | -    auto& DBContext() const LIFETIMEBOUND { return *Assert(m_db_context); }
 207 | +    auto& DBContext() const LIFETIMEBOUND { return *m_db_context; }

l0rinc commented at 4:47 PM on May 23, 2026:

faef857 refactor: Use util::NotNull<std::unique_ptr<LevelDBContext>> m_db_context:

Another similar one is BlockTemplateImpl, it could even simplify passing it as reference:

diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 16db8692a1..299fa21c8f 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -58,6 +58,7 @@
 #include <uint256.h>
 #include <univalue.h>
 #include <util/check.h>
+#include <util/pointers.h>
 #include <util/result.h>
 #include <util/signalinterrupt.h>
 #include <util/string.h>
@@ -873,7 +874,6 @@ public:
                                                     m_block_template(std::move(block_template)),
                                                     m_node(node)
     {
-        assert(m_block_template);
     }
 
     CBlockHeader getBlockHeader() override
@@ -914,7 +914,7 @@ public:
 
     std::unique_ptr<BlockTemplate> waitNext(BlockWaitOptions options) override
     {
-        auto new_template = WaitAndCreateNewBlock(chainman(), notifications(), m_node.mempool.get(), m_block_template, options, m_assemble_options, m_interrupt_wait);
+        auto new_template = WaitAndCreateNewBlock(chainman(), notifications(), m_node.mempool.get(), *m_block_template, options, m_assemble_options, m_interrupt_wait);
         if (new_template) return std::make_unique<BlockTemplateImpl>(m_assemble_options, std::move(new_template), m_node);
         return nullptr;
     }
@@ -926,7 +926,7 @@ public:
 
     const BlockAssembler::Options m_assemble_options;
 
-    const std::unique_ptr<CBlockTemplate> m_block_template;
+    const util::NotNullUniquePtr<CBlockTemplate> m_block_template;
 
     bool m_interrupt_wait{false};
     ChainstateManager& chainman() { return *Assert(m_node.chainman); }
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
index c9a491ef23..d9cd325a41 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -364,7 +364,7 @@ void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wa
 std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainman,
                                                       KernelNotifications& kernel_notifications,
                                                       CTxMemPool* mempool,
-                                                      const std::unique_ptr<CBlockTemplate>& block_template,
+                                                      const CBlockTemplate& block_template,
                                                       const BlockWaitOptions& options,
                                                       const BlockAssembler::Options& assemble_options,
                                                       bool& interrupt_wait)
@@ -391,7 +391,7 @@ std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainma
                 // We assume tip_block is set, because this is an instance
                 // method on BlockTemplate and no template could have been
                 // generated before a tip exists.
-                tip_changed = Assume(tip_block) && tip_block != block_template->block.hashPrevBlock;
+                tip_changed = Assume(tip_block) && tip_block != block_template.block.hashPrevBlock;
                 return tip_changed || chainman.m_interrupt || interrupt_wait;
             });
             if (interrupt_wait) {
@@ -435,7 +435,7 @@ std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainma
 
             // Calculate the original template total fees if we haven't already
             if (current_fees == -1) {
-                current_fees = std::accumulate(block_template->vTxFees.begin(), block_template->vTxFees.end(), CAmount{0});
+                current_fees = std::accumulate(block_template.vTxFees.begin(), block_template.vTxFees.end(), CAmount{0});
             }
 
             // Check if fees increased enough to return the new template
diff --git a/src/node/miner.h b/src/node/miner.h
index 5c8668771f..1a0e3f8d3d 100644
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -150,7 +150,7 @@ void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wa
 std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainman,
                                                       KernelNotifications& kernel_notifications,
                                                       CTxMemPool* mempool,
-                                                      const std::unique_ptr<CBlockTemplate>& block_template,
+                                                      const CBlockTemplate& block_template,
                                                       const BlockWaitOptions& options,
                                                       const BlockAssembler::Options& assemble_options,
                                                       bool& interrupt_wait);

l0rinc commented at 4:52 PM on May 23, 2026:

faef857 refactor: Use util::NotNull<std::unique_ptr<LevelDBContext>> m_db_context:

And a few other ones that we're already treating as non-null implicitly: their constructors initialize m_impl with std::make_unique, and the members are const, so the pointer cannot later be reset or reassigned:

diff --git a/src/addrman.h b/src/addrman.h
index 94e7d3e653..7a1888d38e 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -10,6 +10,7 @@
 #include <netgroup.h>
 #include <protocol.h>
 #include <streams.h>
+#include <util/pointers.h>
 #include <util/time.h>
 
 #include <cstdint>
@@ -109,7 +110,7 @@ struct AddressPosition {
 class AddrMan
 {
 protected:
-    const std::unique_ptr<AddrManImpl> m_impl;
+    const util::NotNullUniquePtr<AddrManImpl> m_impl;
 
 public:
     explicit AddrMan(const NetGroupManager& netgroupman, bool deterministic, int32_t consistency_check_ratio);
diff --git a/src/node/txreconciliation.h b/src/node/txreconciliation.h
index 68deeabaf6..97bf1bfd44 100644
--- a/src/node/txreconciliation.h
+++ b/src/node/txreconciliation.h
@@ -7,6 +7,7 @@
 
 #include <net.h>
 #include <sync.h>
+#include <util/pointers.h>
 
 #include <memory>
 #include <tuple>
@@ -52,7 +53,7 @@ class TxReconciliationTracker
 {
 private:
     class Impl;
-    const std::unique_ptr<Impl> m_impl;
+    const util::NotNullUniquePtr<Impl> m_impl;
 
 public:
     explicit TxReconciliationTracker(uint32_t recon_version);
diff --git a/src/txrequest.h b/src/txrequest.h
index 93972a88c2..6fd92c0ec8 100644
--- a/src/txrequest.h
+++ b/src/txrequest.h
@@ -8,6 +8,7 @@
 #include <primitives/transaction.h>
 #include <net.h>
 #include <uint256.h>
+#include <util/pointers.h>
 
 #include <chrono>
 #include <cstdint>
@@ -100,7 +101,7 @@
 class TxRequestTracker {
     // Avoid littering this header file with implementation details.
     class Impl;
-    const std::unique_ptr<Impl> m_impl;
+    const util::NotNullUniquePtr<Impl> m_impl;
 
 public:
     //! Construct a TxRequestTracker.

maflcko commented at 5:37 PM on May 26, 2026:

BlockTemplateImpl

Ah, right. Though, my preference would be to make all NotNull constructor calls explicit, as they serve as a drop-in replacement for Assert. Also, one could consider making NotNull movable and then construct it at the call site here.

m_impl

Nice. Happy to push such a commit. Though, if we use it for m_impl, it should be used in all places in the codebase. E.g. the following is missing from your diff:

src/musig.h: std::unique_ptr<MuSig2SecNonceImpl> m_impl;
const std::unique_ptr<TxDownloadManagerImpl> m_impl;

If you send a full diff, I can include it here ( Let me know if you want to be listed as co-author in any commits). Or you can push a branch to your liking to your repo, and I can take it as-is.

l0rinc commented at 10:38 AM on June 2, 2026:

If you send a full diff, I can include it here

Seems simple enough to do here or in a follow-up, it's not a blocker from me.

Let me know if you want to be listed as co-author in any commits

I usually add coauthors when their comments triggered a change that makes the PR better.

maflcko commented at 3:05 PM on June 18, 2026:

If you send a full diff, I can include it here

Seems simple enough to do here or in a follow-up, it's not a blocker from me.

Let me know if you want to be listed as co-author in any commits

I usually add coauthors when their comments triggered a change that makes the PR better.

Heh, I never know when to include or not include. I guess I just try to preserve whoever wrote the bulk of the commit initially.

I guess I can close this thread and the diffs can be submitted later?

l0rinc changes_requested

l0rinc commented at 5:18 PM on May 23, 2026: contributor

Concept ACK, I think this could help in a few cases where the pointer-like type is part of the contract and a plain reference would not preserve the ownership/storage semantics. This also feels aligned with ongoing efforts like #35229 (cc: @optout21).

The part that bothers me most is that util::NotNull<std::unique_ptr<T>> does not quite feel like a first-class type yet. The construction and call sites are fairly verbose, which makes the stronger invariant harder to adopt/read. I left a few suggestions around making common smart-pointer usage more compact, adding a few assertions/tests so the API is less surprising, and pointing out a few other places where this pattern may fit.

maflcko force-pushed on May 26, 2026

maflcko commented at 6:23 PM on May 26, 2026: member

Thx for the review. I pushed some changes and replied to all comments.

l0rinc approved

l0rinc commented at 10:55 AM on June 2, 2026: contributor

ACK fae0a346c1ea2c96160448f660b37749d695265e

Remaining nits aren't critical, we can do them in follow-ups.

DrahtBot requested review from stickies-v on Jun 2, 2026

maflcko force-pushed on Jun 16, 2026

DrahtBot added the label CI failed on Jun 16, 2026

maflcko force-pushed on Jun 16, 2026

l0rinc commented at 10:27 AM on June 16, 2026: contributor

untested diff review ACK 6389aa8f15aa4ed55d76afda36c0b1c870f1c9d0reACK

Mostly whitespace and formatting and comment changes since last review.

<details><summary>Changes since my last ack</summary>

diff --git a/src/util/pointers.h b/src/util/pointers.h
index aff946a558..e7dca4b2ba 100644
--- a/src/util/pointers.h
+++ b/src/util/pointers.h
@@ -3,7 +3,7 @@
 // file COPYING or https://opensource.org/license/mit/.
 
 // This file is based on
-// https://github.com/microsoft/GSL/blob/756c91ab895aa52f650599bb1a3fc131f1f4b5ef/include/gsl/pointers,
+// https://github.com/microsoft/GSL/blob/main/include/gsl/pointers,
 // with some modifications:
 // * Remove everything around GSL_DEPRECATED and GSL_NO_IOSTREAMS, because it
 //   is not needed.
@@ -16,8 +16,7 @@
 //    - strict_make_not_null, because it is not needed.
 // * Remove the not_null->strict_not_null converting constructors, because they
 //   are not needed.
-// * Add NotNullUniquePtr and NotNullSharedPtr aliases to keep smart-pointer
-//   call sites readable.
+// * Add util namespace for aliases to be used by Bitcoin Core code.
 //
 // All original code is covered by:
 
@@ -42,11 +41,11 @@
 
 #include <util/check.h>
 
-#include <cstddef>
-#include <functional>
-#include <memory>
-#include <type_traits>
-#include <utility>
+#include <cstddef>     // for ptrdiff_t, nullptr_t, size_t
+#include <functional>  // for less, greater
+#include <memory>      // for shared_ptr, unique_ptr, hash
+#include <type_traits> // for enable_if_t, is_convertible, is_assignable
+#include <utility>     // for declval, forward
 
 namespace gsl_detail
 {
@@ -66,24 +65,26 @@ namespace details
     {
     };
 
-    // Resolves to the more efficient of `const T` or `const T&`, in the context of returning a const-qualified value
-    // of type T.
+    // Resolves to the more efficient of `const T` or `const T&`, in the context of returning a
+    // const-qualified value of type T.
     //
-    // Copied from cppfront's implementation of the CppCoreGuidelines F.16 (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rf-in)
-    template<typename T>
-    using value_or_reference_return_t = std::conditional_t<
-                                            sizeof(T) <= 2*sizeof(void*) && std::is_trivially_copy_constructible<T>::value,
-                                            const T,
-                                            const T&>;
+    // Copied from cppfront's implementation of the CppCoreGuidelines F.16
+    // (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rf-in)
+    template <typename T>
+    using value_or_reference_return_t =
+        std::conditional_t<sizeof(T) <= 2 * sizeof(void*) &&
+                               std::is_trivially_copy_constructible<T>::value,
+                           const T, const T&>;
 
 } // namespace details
 
 //
 // owner
 //
-// `gsl::owner<T>` is designed as a safety mechanism for code that must deal directly with raw pointers that own memory.
-// Ideally such code should be restricted to the implementation of low-level abstractions. `gsl::owner` can also be used
-// as a stepping point in converting legacy code to use more modern RAII constructs, such as smart pointers.
+// `gsl::owner<T>` is designed as a safety mechanism for code that must deal directly with raw
+// pointers that own memory. Ideally such code should be restricted to the implementation of
+// low-level abstractions. `gsl::owner` can also be used as a stepping point in converting legacy
+// code to use more modern RAII constructs, such as smart pointers.
 //
 // T must be a pointer type
 // - disallow construction from any type other than pointer type
@@ -114,19 +115,23 @@ public:
     using element_type = T;
 
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr not_null(U&& u) noexcept(std::is_nothrow_move_constructible<T>::value) : ptr_(std::forward<U>(u))
+    constexpr not_null(U&& u) noexcept(std::is_nothrow_move_constructible<T>::value)
+        : ptr_(std::forward<U>(u))
     {
         Assert(ptr_ != nullptr);
     }
 
     template <typename = std::enable_if_t<!std::is_same<std::nullptr_t, T>::value>>
-    constexpr not_null(T u) noexcept(std::is_nothrow_move_constructible<T>::value) : ptr_(std::move(u))
+    constexpr not_null(T u) noexcept(std::is_nothrow_move_constructible<T>::value)
+        : ptr_(std::move(u))
     {
         Assert(ptr_ != nullptr);
     }
 
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr not_null(const not_null<U>& other) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null(other.get())
+    constexpr not_null(const not_null<U>& other) noexcept(
+        std::is_nothrow_move_constructible<T>::value)
+        : not_null(other.get())
     {}
 
     not_null(const not_null& other) = default;
@@ -160,7 +165,9 @@ private:
     T ptr_;
 };
 
-template <typename T, std::enable_if_t<std::is_move_assignable<T>::value && std::is_move_constructible<T>::value, bool> = true>
+template <typename T, std::enable_if_t<std::is_move_assignable<T>::value &&
+                                           std::is_move_constructible<T>::value,
+                                       bool> = true>
 void swap(not_null<T>& a, not_null<T>& b) noexcept
 {
     a.swap(b);
@@ -174,7 +181,7 @@ auto make_not_null(T&& t) noexcept
 
 template <class T, class U>
 constexpr auto operator==(const not_null<T>& lhs,
-                const not_null<U>& rhs) noexcept(noexcept(lhs.get() == rhs.get()))
+                          const not_null<U>& rhs) noexcept(noexcept(lhs.get() == rhs.get()))
     -> decltype(lhs.get() == rhs.get())
 {
     return lhs.get() == rhs.get();
@@ -182,39 +189,41 @@ constexpr auto operator==(const not_null<T>& lhs,
 
 template <class T, class U>
 constexpr auto operator!=(const not_null<T>& lhs,
-                const not_null<U>& rhs) noexcept(noexcept(lhs.get() != rhs.get()))
+                          const not_null<U>& rhs) noexcept(noexcept(lhs.get() != rhs.get()))
     -> decltype(lhs.get() != rhs.get())
 {
     return lhs.get() != rhs.get();
 }
 
 template <class T, class U>
-constexpr auto operator<(const not_null<T>& lhs,
-               const not_null<U>& rhs) noexcept(noexcept(std::less<>{}(lhs.get(), rhs.get())))
-    -> decltype(std::less<>{}(lhs.get(), rhs.get()))
+constexpr auto operator<(const not_null<T>& lhs, const not_null<U>& rhs) noexcept(
+    noexcept(std::less<>{}(lhs.get(), rhs.get()))) -> decltype(std::less<>{}(lhs.get(), rhs.get()))
 {
     return std::less<>{}(lhs.get(), rhs.get());
 }
 
 template <class T, class U>
-constexpr auto operator<=(const not_null<T>& lhs,
-                const not_null<U>& rhs) noexcept(noexcept(std::less_equal<>{}(lhs.get(), rhs.get())))
+constexpr auto
+operator<=(const not_null<T>& lhs,
+           const not_null<U>& rhs) noexcept(noexcept(std::less_equal<>{}(lhs.get(), rhs.get())))
     -> decltype(std::less_equal<>{}(lhs.get(), rhs.get()))
 {
     return std::less_equal<>{}(lhs.get(), rhs.get());
 }
 
 template <class T, class U>
-constexpr auto operator>(const not_null<T>& lhs,
-               const not_null<U>& rhs) noexcept(noexcept(std::greater<>{}(lhs.get(), rhs.get())))
+constexpr auto
+operator>(const not_null<T>& lhs,
+          const not_null<U>& rhs) noexcept(noexcept(std::greater<>{}(lhs.get(), rhs.get())))
     -> decltype(std::greater<>{}(lhs.get(), rhs.get()))
 {
     return std::greater<>{}(lhs.get(), rhs.get());
 }
 
 template <class T, class U>
-constexpr auto operator>=(const not_null<T>& lhs,
-                const not_null<U>& rhs) noexcept(noexcept(std::greater_equal<>{}(lhs.get(), rhs.get())))
+constexpr auto
+operator>=(const not_null<T>& lhs,
+           const not_null<U>& rhs) noexcept(noexcept(std::greater_equal<>{}(lhs.get(), rhs.get())))
     -> decltype(std::greater_equal<>{}(lhs.get(), rhs.get()))
 {
     return std::greater_equal<>{}(lhs.get(), rhs.get());
@@ -230,9 +239,10 @@ not_null<T> operator+(const not_null<T>&, std::ptrdiff_t) = delete;
 template <class T>
 not_null<T> operator+(std::ptrdiff_t, const not_null<T>&) = delete;
 
-
-// T is conceptually a pointer so we don't have to worry about it being a reference and violating std::hash requirements
-template <class T, class U = typename T::element_type, bool = std::is_default_constructible<std::hash<U>>::value>
+// T is conceptually a pointer so we don't have to worry about it being a reference and violating
+// std::hash requirements
+template <class T, class U = typename T::element_type,
+          bool = std::is_default_constructible<std::hash<U>>::value>
 struct not_null_hash
 {
     std::size_t operator()(const T& value) const noexcept { return std::hash<U>{}(value.get()); }
@@ -282,20 +292,26 @@ class strict_not_null : public not_null<T>
 {
 public:
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr explicit strict_not_null(U&& u) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null<T>(std::forward<U>(u))
+    constexpr explicit strict_not_null(U&& u) noexcept(std::is_nothrow_move_constructible<T>::value)
+        : not_null<T>(std::forward<U>(u))
     {}
 
     template <typename = std::enable_if_t<!std::is_same<std::nullptr_t, T>::value>>
-    constexpr explicit strict_not_null(T u) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null<T>(std::move(u))
+    constexpr explicit strict_not_null(T u) noexcept(std::is_nothrow_move_constructible<T>::value)
+        : not_null<T>(std::move(u))
     {}
 
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr strict_not_null(const strict_not_null<U>& other) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null<T>(other)
+    constexpr strict_not_null(const strict_not_null<U>& other) noexcept(
+        std::is_nothrow_move_constructible<T>::value)
+        : not_null<T>(other)
     {}
 
     // To avoid invalidating the "not null" invariant, the contained pointer is actually copied
-    // instead of moved. If it is a custom pointer, its constructor could in theory throw exceptions.
-    strict_not_null(strict_not_null&& other) noexcept(std::is_nothrow_copy_constructible<T>::value) = default;
+    // instead of moved. If it is a custom pointer, its constructor could in theory throw
+    // exceptions.
+    strict_not_null(strict_not_null&& other) noexcept(
+        std::is_nothrow_copy_constructible<T>::value) = default;
     strict_not_null(const strict_not_null& other) = default;
     strict_not_null& operator=(const strict_not_null& other) = default;
 
@@ -359,6 +375,11 @@ namespace util {
 /// The C++ language provides raw references for this use case, and the C++
 /// standard library provides std::reference_wrapper, where raw references can
 /// not be used.
+///
+/// This type is currently not movable, meaning that the inner pointer must be
+/// copied for any move or copy operation, such that the moved-from pointer
+/// remains a valid NotNull pointer. Thus, NotNullUniquePtr can not be moved,
+/// because a unique pointer can not be copied.
 template <class T>
 struct NotNull : public gsl_detail::strict_not_null<T> {
     using gsl_detail::strict_not_null<T>::strict_not_null;

</details>

DrahtBot removed the label CI failed on Jun 16, 2026

maflcko force-pushed on Jun 16, 2026

l0rinc commented at 8:45 AM on June 17, 2026: contributor

lightly tested diff review ACK fa951b422c43d7a3b51aaefbaf25f3d9d53e1a69

sedited commented at 1:05 PM on June 21, 2026: contributor

Concept ACK

l0rinc commented at 7:49 PM on July 3, 2026: contributor

The new move changes are a bit surprising to me at first glance, but I'll circle back to this later, unless you think it's urgent.

DrahtBot added the label Needs rebase on Jul 9, 2026

maflcko force-pushed on Jul 9, 2026

DrahtBot removed the label Needs rebase on Jul 9, 2026

maflcko commented at 12:02 PM on July 9, 2026: member

rebased and added a commit

in src/util/pointers.h:139 in fa53bbcb11

 134 | +        std::is_nothrow_move_constructible<T>::value)
 135 | +        : not_null(other.get())
 136 | +    {}
 137 | +
 138 | +    template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
 139 | +    constexpr not_null(not_null<U>&& other) noexcept(std::is_nothrow_move_constructible<T>::value) : ptr_(std::move(other.ptr_))

stickies-v commented at 2:03 PM on July 10, 2026:

if we use the public interface here, we can skip the friend class (, and it might (not sure) help with some use-after-move detection by marking other as moved, instead of just its ptr_?)

diff --git a/src/util/pointers.h b/src/util/pointers.h
index a138e94624..ce8f06ab5f 100644
--- a/src/util/pointers.h
+++ b/src/util/pointers.h
@@ -136,7 +136,7 @@ public:
     {}
 
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr not_null(not_null<U>&& other) noexcept(std::is_nothrow_move_constructible<T>::value) : ptr_(std::move(other.ptr_))
+    constexpr not_null(not_null<U>&& other) noexcept(std::is_nothrow_move_constructible<T>::value) : ptr_(std::move(other).get())
     {}
 
     constexpr not_null(const not_null& other) = default;
@@ -172,7 +172,6 @@ public:
     void swap(not_null<T>& other) noexcept { std::swap(ptr_, other.ptr_); }
 
 private:
-    template <class U> friend class not_null;
     T ptr_;
 };

</details>

maflcko commented at 10:34 AM on July 17, 2026:

I think other is already moved, so it shouldn't change any detection. But I pushed your branch, so this is now included.

in src/util/pointers.h:150 in fa53bbcb11

 145 | +    constexpr not_null& operator=(not_null&& other) = default;
 146 | +
 147 | +    constexpr details::value_or_reference_return_t<T> get() const &
 148 | +        noexcept(noexcept(details::value_or_reference_return_t<T>(std::declval<T&>())))
 149 | +    {
 150 | +        return ptr_;

stickies-v commented at 2:39 PM on July 10, 2026:

It's not ideal (but I think the best choice out of the options) that this class relies on clang-tidy for its safe usage wrt use-after-move. I think one easy and free (in release) belt-and-suspenders check we can add is to Assume the invariant where we can?

diff --git a/src/util/pointers.h b/src/util/pointers.h
index a138e94624..589c6f4d44 100644
--- a/src/util/pointers.h
+++ b/src/util/pointers.h
@@ -147,9 +147,9 @@ public:
     constexpr details::value_or_reference_return_t<T> get() const &
         noexcept(noexcept(details::value_or_reference_return_t<T>(std::declval<T&>())))
     {
-        return ptr_;
+        return Assume(ptr_);
     }
-    constexpr T&& get() && noexcept { return std::move(ptr_); }
+    constexpr T&& get() && noexcept { return std::move(Assume(ptr_)); }
 
     constexpr operator T() const & { return get(); }
     constexpr operator T() && noexcept { return std::move(*this).get(); }

</details>

maflcko commented at 10:33 AM on July 17, 2026:

thx, added Assume

DrahtBot added the label Needs rebase on Jul 14, 2026

maflcko force-pushed on Jul 17, 2026

DrahtBot added the label CI failed on Jul 17, 2026

DrahtBot commented at 10:54 AM on July 17, 2026: contributor

🚧 At least one of the CI tasks failed. Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/29573818713/job/87863590921 LLM reason (✨ experimental): CI failed because ctest reported util_pointers_tests aborting on an assertion failure in util/pointers.h:73 (NotNull<NoCopyPtr>::get).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

maflcko force-pushed on Jul 17, 2026

DrahtBot removed the label Needs rebase on Jul 17, 2026

DrahtBot removed the label CI failed on Jul 17, 2026

in src/util/pointers.h:97 in fa0227405c outdated

  92 | +{
  93 | +    a.swap(b);
  94 | +}
  95 | +
  96 | +template <class T, class U>
  97 | +constexpr auto operator<=>(const NotNull<T>& a, const NotNull<U>& b)

stickies-v commented at 3:34 PM on July 21, 2026:

nit: I think we typically implement these as member functions

diff --git a/src/util/pointers.h b/src/util/pointers.h
index 442ae7c160..51300c2ce9 100644
--- a/src/util/pointers.h
+++ b/src/util/pointers.h
@@ -80,6 +80,11 @@ public:
 
     void swap(NotNull& other) noexcept { std::swap(m_ptr, other.m_ptr); }
 
+    template <class U>
+    constexpr auto operator<=>(const NotNull<U>& other) const { return std::compare_three_way{}(get(), other.get()); }
+    template <class U>
+    constexpr bool operator==(const NotNull<U>& other) const { return get() == other.get(); }
+
 private:
     T m_ptr;
 };
@@ -93,18 +98,6 @@ void swap(NotNull<T>& a, NotNull<T>& b) noexcept
     a.swap(b);
 }
 
-template <class T, class U>
-constexpr auto operator<=>(const NotNull<T>& a, const NotNull<U>& b)
-{
-    return std::compare_three_way{}(a.get(), b.get());
-}
-
-template <class T, class U>
-constexpr bool operator==(const NotNull<T>& a, const NotNull<U>& b)
-{
-    return a.get() == b.get();
-}
-
 template <typename T, typename Deleter = std::default_delete<T>>
 using NotNullUniquePtr = NotNull<std::unique_ptr<T, Deleter>>;

</details>

maflcko commented at 1:56 PM on August 2, 2026:

thx, done

in src/util/pointers.h:91 in fa0227405c outdated

  86 | +
  87 | +template <class T>
  88 | +NotNull(T) -> NotNull<T>;
  89 | +
  90 | +template <class T>
  91 | +void swap(NotNull<T>& a, NotNull<T>& b) noexcept

stickies-v commented at 4:00 PM on July 21, 2026:

nit: swap is currently unused, might make sense to add if/when we need it?

maflcko commented at 1:56 PM on August 2, 2026:

Hmm, I guess move is noexcept for the smart pointers we care about, so we can just use the default swap fallback and let the compiler optimize down to the same binary.

Removed.

DrahtBot added the label Needs rebase on Jul 21, 2026

maflcko force-pushed on Jul 22, 2026

DrahtBot added the label CI failed on Jul 22, 2026

DrahtBot removed the label CI failed on Jul 22, 2026

DrahtBot removed the label Needs rebase on Jul 22, 2026

in src/test/rbf_tests.cpp:288 in fa6228d332

 284 | @@ -285,15 +285,15 @@ BOOST_FIXTURE_TEST_CASE(improves_feerate, TestChain100Setup)
 285 |      BOOST_CHECK(res1.value().second == "insufficient feerate: does not improve feerate diagram");
 286 |  
 287 |      // With one more satoshi it does
 288 | -    changeset.reset();
 289 | +    std::move(changeset).get().reset();

stickies-v commented at 11:01 AM on July 22, 2026:

It increases the diff, but I think using scopes here would be the better choice, instead of documenting how to abuse NotNull.

maflcko commented at 1:56 PM on August 2, 2026:

Yeah, that is also less code and a cleaner. Thx, done.

in src/util/pointers.h:38 in fa6228d332

  33 | +/// standard library provides std::reference_wrapper, where raw references can
  34 | +/// not be used.
  35 | +///
  36 | +/// This type is movable, meaning that the inner pointer *is* null after a
  37 | +/// move. Clang-tidy static analysis is used to enforce use-after-move
  38 | +/// violations, so that the null can never be observed. Moreover, the runtime

stickies-v commented at 11:30 AM on July 22, 2026:

nit: iiuc use-after-move static analysis does not catch everything, so perhaps "can never be observed" is misleading / too strong.

maflcko commented at 1:56 PM on August 2, 2026:

thx, s/can never/should never/

in src/util/pointers.h:72 in fa6228d332

  67 | +    constexpr NotNull(NotNull&&) = default;
  68 | +    constexpr NotNull& operator=(NotNull&&) = default;
  69 | +    constexpr NotNull(const NotNull&) = default;
  70 | +    constexpr NotNull& operator=(const NotNull&) = default;
  71 | +
  72 | +    constexpr const T& get() const& noexcept { return Assume(m_ptr); }

stickies-v commented at 2:11 PM on July 22, 2026:

nit: could do with some LIFETIMEBOUND:

diff --git a/src/util/pointers.h b/src/util/pointers.h
index 442ae7c160..9fcfae6bc8 100644
--- a/src/util/pointers.h
+++ b/src/util/pointers.h
@@ -5,6 +5,7 @@
 #ifndef BITCOIN_UTIL_POINTERS_H
 #define BITCOIN_UTIL_POINTERS_H
 
+#include <attributes.h>
 #include <util/check.h>
 
 #include <compare>
@@ -69,10 +70,10 @@ public:
     constexpr NotNull(const NotNull&) = default;
     constexpr NotNull& operator=(const NotNull&) = default;
 
-    constexpr const T& get() const& noexcept { return Assume(m_ptr); }
-    constexpr T&& get() && noexcept { return std::move(Assume(m_ptr)); }
+    constexpr const T& get() const& noexcept LIFETIMEBOUND { return Assume(m_ptr); }
+    constexpr T&& get() && noexcept LIFETIMEBOUND { return std::move(Assume(m_ptr)); }
 
-    constexpr decltype(auto) operator->() const { return get(); }
+    constexpr decltype(auto) operator->() const LIFETIMEBOUND { return get(); }
     constexpr decltype(auto) operator*() const { return *get(); }
 
     constexpr operator T() const& { return get(); }

</details>

maflcko commented at 1:56 PM on August 2, 2026:

thx, done for all references

in src/util/not_null.h:46 in fa6228d332 outdated

  41 | +    requires(!std::is_pointer_v<T>) && requires(T t) { { t != nullptr } -> std::convertible_to<bool>; }
  42 | +class NotNull
  43 | +{
  44 | +public:
  45 | +    template <std::convertible_to<T> U>
  46 | +    constexpr explicit NotNull(U&& u) noexcept(std::is_nothrow_constructible_v<T, U&&>)

stickies-v commented at 3:23 PM on July 22, 2026:

For a non-const lvalue NotNull<T> under direct-init, the forwarding ctor is selected instead of the copy ctor.

NotNull<std::shared_ptr<int>> t{std::make_shared<int>(1)};
NotNull<std::shared_ptr<int>> u{t};

diff --git a/src/util/pointers.h b/src/util/pointers.h
index 442ae7c160..9325dd1e69 100644
--- a/src/util/pointers.h
+++ b/src/util/pointers.h
@@ -43,6 +43,7 @@ class NotNull
 {
 public:
     template <std::convertible_to<T> U>
+        requires (!std::same_as<std::remove_cvref_t<U>, NotNull>)
     constexpr explicit NotNull(U&& u) noexcept(std::is_nothrow_constructible_v<T, U&&>)
         : m_ptr(std::forward<U>(u))
     {

</details>

maflcko commented at 1:56 PM on August 2, 2026:

Sure, but there can't be an issue? operator T creates a copy, just like .get() creates a copy. And any compiler failure before will remain a compile failure after?

I guess you are mostly worried about this style-wise and for trait stuff like:

std::is_constructible_v<NotNullUniquePtr<int>, NotNullUniquePtr<int>&> ?

stickies-v commented at 8:59 AM on August 3, 2026:

Sorry, should have stated why I raised this. My main concern was the ambiguity this creates, i.e. NotNull<std::shared_ptr<int>> u{t}; and NotNull<std::shared_ptr<int>> u = t; use different constructors, with the former adding an extra Assert. Indeed this shouldn't lead to any issues, except for when we're dealing with a moved-from object (which shouldn't be possible, but is not guaranteed), in which case they will have different behaviour. I think it's prudent to prevent this from happening.

I also assumed the copy constructor would be more performant, but thinking about it again I suppose the difference is negligible, with just the (very) minimal runtime Assert overhead.

maflcko commented at 9:56 AM on August 3, 2026:

Ok, the additional assert makes sense. My eyes didn't see that, and I guess compilers won't optimize it away either, unless there are annotations on the getter/opT(), see #24423 (comment)

In any case, I've applied your diff, so this should be fixed.

stickies-v commented at 4:05 PM on July 22, 2026: contributor

Approach ACK, code lgtm fa0227405c497e94ddb7a7603b37993a7703c0c1 but the forwarding ctor issue should probably be fixed, nothing else blocking

maflcko force-pushed on Aug 2, 2026

stickies-v approved

stickies-v commented at 5:07 PM on August 3, 2026: contributor

ACK fa0e12e77bb90bef05bfd8a960e1a28673b575be

DrahtBot requested review from l0rinc on Aug 3, 2026

DrahtBot requested review from sedited on Aug 3, 2026

in src/util/pointers.h:75 in fa3bda5fb8

  70 | +    constexpr NotNull& operator=(NotNull&&) = default;
  71 | +    constexpr NotNull(const NotNull&) = default;
  72 | +    constexpr NotNull& operator=(const NotNull&) = default;
  73 | +
  74 | +    constexpr const T& get() const& noexcept LIFETIMEBOUND { return Assume(m_ptr); }
  75 | +    constexpr T&& get() && noexcept LIFETIMEBOUND { return std::move(Assume(m_ptr)); }

sedited commented at 3:26 PM on September 21, 2026:

Might it be preferable to return T here instead of T&& and have it construct the value directly? That would get rid of the LIFETIMEBOUND, no?

maflcko commented at 6:45 PM on September 21, 2026:

Sure, there could be an extra move in some cases, but removing the need for LIFETIMEBOUND seems also ok.

in src/util/pointers.h:81 in fa3bda5fb8

  76 | +
  77 | +    constexpr decltype(auto) operator->() const LIFETIMEBOUND { return get(); }
  78 | +    constexpr decltype(auto) operator*() const LIFETIMEBOUND { return *get(); }
  79 | +
  80 | +    constexpr operator T() const& { return get(); }
  81 | +    constexpr operator T() && noexcept { return std::move(*this).get(); }

sedited commented at 3:27 PM on September 21, 2026:

Should this noexcept be conditional?

maflcko commented at 6:45 PM on September 21, 2026:

Sure, it could be. However, it doesn't make a difference for uniq ptr and shared ptr. It could only make a difference for pointer types whose copy-ctor may throw, like https://www.boost.org/doc/libs/latest/libs/smart_ptr/doc/html/smart_ptr.html#intrusive_ptr

I don't think this is ever needed, but I pushed it.

sedited approved

sedited commented at 3:54 PM on September 21, 2026: contributor

ACK fa0e12e77bb90bef05bfd8a960e1a28673b575be

There are a few other pimpls that could get this treatment. How did you select the current ones?

maflcko force-pushed on Sep 21, 2026

maflcko commented at 6:45 PM on September 21, 2026: member

There are a few other pimpls that could get this treatment. How did you select the current ones?

I didn't want to explode this pull with boring mechanical changes. There are a bunch of ideas for later in comment #34844 (comment), and changing the boring pimpls could be done as well.

sedited approved

sedited commented at 7:33 PM on September 21, 2026: contributor

ACK 5555d39c68f02e274e99205736088cc4df67a10d

Forgot to mention to mention this before, but why not call the new file not_null.h?

DrahtBot requested review from stickies-v on Sep 21, 2026

in src/util/not_null.h:95 in 199999bd0b outdated

  90 | +    }
  91 | +
  92 | +    template <class U>
  93 | +    constexpr auto operator<=>(const NotNull<U>& other) const
  94 | +    {
  95 | +        return std::compare_three_way{}(get(), other.get());

l0rinc commented at 8:21 PM on September 21, 2026:

199999b util: Add util::NotNull<SmartPtrType>:

Nit: I like spaceships 🚀. You don't like spaceships? 🚀

        return get() <=> other.get();

maflcko commented at 9:50 AM on September 22, 2026:

Sure, it could be changed, but I like it as a hint:

If the class is ever changed to support raw pointers (which may not have a strict total order)
If someone implements a smart pointer class on top of a raw pointer, so that they'll copy-paste the defined ordering instead of the unspecified one.

so I'll leave as-is for now.

in src/util/pointers.h:85 in 199999bd0b

  80 | +    }
  81 | +
  82 | +    constexpr decltype(auto) operator->() const LIFETIMEBOUND { return get(); }
  83 | +    constexpr decltype(auto) operator*() const LIFETIMEBOUND { return *get(); }
  84 | +
  85 | +    constexpr operator T() const& { return get(); }

l0rinc commented at 8:22 PM on September 21, 2026:

199999b util: Add util::NotNull<SmartPtrType>:

nit: a bit confusing that this copies while get() const& returns a reference, maybe worth a short comment on why.

maflcko commented at 9:50 AM on September 22, 2026:

I think it is clear that operator T() copies, so I'll leave as-is for now.

in src/util/not_null.h:76 in 199999bd0b outdated

  71 | +    constexpr NotNull& operator=(NotNull&&) = default;
  72 | +    constexpr NotNull(const NotNull&) = default;
  73 | +    constexpr NotNull& operator=(const NotNull&) = default;
  74 | +
  75 | +    constexpr const T& get() const& noexcept LIFETIMEBOUND { return Assume(m_ptr); }
  76 | +    constexpr T get() && noexcept(std::is_nothrow_move_constructible_v<T>)

l0rinc commented at 8:24 PM on September 21, 2026:

199999b util: Add util::NotNull<SmartPtrType>:

nit: Corecheck thinks these should be explicit

<details><summary>make pointer extraction explicit</summary>

diff --git a/src/test/util_pointers_tests.cpp b/src/test/util_pointers_tests.cpp
index 1d66293eae..bdcc30b7a7 100644
--- a/src/test/util_pointers_tests.cpp
+++ b/src/test/util_pointers_tests.cpp
@@ -61,6 +61,9 @@ BOOST_AUTO_TEST_CASE(check_ctor_conv)
     static_assert(std::is_constructible_v<NNPtr, const NNPtr&>);
     static_assert(!std::is_constructible_v<NNUniqPtr, NNUniqPtr&>);
     static_assert(!std::is_constructible_v<NNUniqPtr, const NNUniqPtr&>);
+
+    static_assert(!std::convertible_to<NNPtr, Ptr>);
+    static_assert(!std::convertible_to<NNUniqPtr, std::unique_ptr<int>>);
 }
 
 template <class P>
diff --git a/src/util/pointers.h b/src/util/pointers.h
index 889582e699..a3ce05efcc 100644
--- a/src/util/pointers.h
+++ b/src/util/pointers.h
@@ -82,8 +82,8 @@ public:
     constexpr decltype(auto) operator->() const LIFETIMEBOUND { return get(); }
     constexpr decltype(auto) operator*() const LIFETIMEBOUND { return *get(); }
 
-    constexpr operator T() const& { return get(); }
-    constexpr operator T() && noexcept(std::is_nothrow_move_constructible_v<T>)
+    explicit constexpr operator T() const& { return get(); }
+    explicit constexpr operator T() && noexcept(std::is_nothrow_move_constructible_v<T>)
         requires std::move_constructible<T>
     {
         return std::move(*this).get();
diff --git a/src/validation.cpp b/src/validation.cpp
index 25772dcb2b..4dad4e30f5 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -919,7 +919,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     // reorg to be marked earlier than any child txs that were already in the mempool.
     const uint64_t entry_sequence = bypass_limits ? 0 : m_pool.GetSequence();
     if (!m_subpackage.m_changeset) {
-        m_subpackage.m_changeset = m_pool.GetChangeSet();
+        m_subpackage.m_changeset = m_pool.GetChangeSet().get();
     }
     ws.m_tx_handle = m_subpackage.m_changeset->StageAddition(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence, fSpendsCoinbase, nSigOpsCost, lock_points.value());
 
@@ -1261,7 +1261,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
         }
         // Remove first failing tx and all subsequent in package
         if (!all_submitted) {
-            if (!m_subpackage.m_changeset) m_subpackage.m_changeset = m_pool.GetChangeSet();
+            if (!m_subpackage.m_changeset) m_subpackage.m_changeset = m_pool.GetChangeSet().get();
             m_subpackage.m_changeset->StageRemoval(m_pool.GetIter(ws.m_ptx->GetHash()).value());
         }
     }

</details>

maflcko commented at 9:49 AM on September 22, 2026:

This is only about raw pointers or raw integral types (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c164-avoid-implicit-conversion-operators), so doesn't apply here?

l0rinc commented at 9:17 PM on September 22, 2026:

C.164 seems broader to me, but I'm fine either way

maflcko commented at 6:11 AM on September 23, 2026:

Yeah, I am happy to push the change, if there is a reason in this case. Maybe a reason is found in the future, but right now I don't see one. Resolving for now.

in src/txmempool.h:720 in 5555d39c68

 716 | @@ -716,10 +717,11 @@ class CTxMemPool
 717 |          friend class CTxMemPool;
 718 |      };
 719 |  
 720 | -    std::unique_ptr<ChangeSet> GetChangeSet() EXCLUSIVE_LOCKS_REQUIRED(cs) {
 721 | +    util::NotNull<std::unique_ptr<ChangeSet>> GetChangeSet() EXCLUSIVE_LOCKS_REQUIRED(cs)

l0rinc commented at 8:26 PM on September 21, 2026:

5555d39 refactor: Return util::NotNull<std::unique_ptr<ChangeSet>> from CTxMemPool::GetChangeSet()🧹:

Nit:

    util::NotNullUniquePtr<ChangeSet> GetChangeSet() EXCLUSIVE_LOCKS_REQUIRED(cs)

maflcko commented at 9:49 AM on September 22, 2026:

sure, done

in src/test/util_pointers_tests.cpp:92 in 199999bd0b outdated

  87 | +        NoCopyPtr extract{std::move(no_copy)};
  88 | +        (void)extract;
  89 | +    }
  90 | +    {
  91 | +        util::NotNull no_copy{NoCopyPtr{new int{}}};
  92 | +        NoCopyPtr extract{std::move(no_copy).get()};

l0rinc commented at 8:29 PM on September 21, 2026:

199999b util: Add util::NotNull<SmartPtrType>:

What's the purpose of this test case? Isn't it basically the same as the one below?

maflcko commented at 9:49 AM on September 22, 2026:

thx, removed the one below.

in src/util/pointers.h:32 in 199999bd0b

  27 | +/// util::NotNull p{std::make_shared<Thing>()};
  28 | +///
  29 | +/// This makes it obvious that a pointer really is not null and that the
  30 | +/// constructor will not fail.
  31 | +///
  32 | +/// NotNull can *not* be used to denote a raw pointer can not be nullptr.

l0rinc commented at 8:33 PM on September 21, 2026:

199999b util: Add util::NotNull<SmartPtrType>:

I read this a few times, maybe:

/// NotNull does *not* wrap raw pointers, so it can not denote a non-null raw pointer.

or simply

/// NotNull does *not* support raw pointers.

maflcko commented at 9:49 AM on September 22, 2026:

sure, done

stickies-v commented at 8:40 PM on September 21, 2026: contributor

re-ACK 5555d39c68f02e274e99205736088cc4df67a10d

Forgot to mention to mention this before, but why not call the new file not_null.h?

I was going to leave the same suggestion in my previous review cycle until I noticed I already used up my nit budget. Would prefer that too.

in src/coins.h:741 in fa8e085c92 outdated

 737 | @@ -737,8 +738,8 @@ class CoinsViewOverlay : public CCoinsViewCache
 738 |          return base->PeekCoin(outpoint);
 739 |      }
 740 |  
 741 | -    //! Non-null. May have zero workers when input fetching is disabled.
 742 | -    std::shared_ptr<ThreadPool> m_thread_pool;
 743 | +    /// May have zero workers when input fetching is disabled.

l0rinc commented at 8:42 PM on September 21, 2026:

fa8e085 refactor: Use NotNull pointer to input fetching pool:

nit: developer-notes accepts both, but the rest of this file uses //!

in src/util/not_null.h:83 in 199999bd0b outdated

  78 | +    {
  79 | +        return std::move(Assume(m_ptr));
  80 | +    }
  81 | +
  82 | +    constexpr decltype(auto) operator->() const LIFETIMEBOUND { return get(); }
  83 | +    constexpr decltype(auto) operator*() const LIFETIMEBOUND { return *get(); }

l0rinc commented at 8:46 PM on September 21, 2026:

199999b util: Add util::NotNull<SmartPtrType>:

Is my understanding correct that the constexpr is only applicable after C++23 - and that it's still fine in C++20, just a noop?

maflcko commented at 9:49 AM on September 22, 2026:

I think the constexpr should work in C++20 as well for hand-written smart pointer types. Though, I can't see a use-case for it, because UB is not allowed at compile-time, so there is no need for NotNull at compile-time.

in src/util/pointers.h:61 in 199999bd0b

  56 | +    constexpr NotNull(NotNull<U>&& other) noexcept(std::is_nothrow_move_constructible_v<U> && std::is_nothrow_constructible_v<T, U&&>)
  57 | +        : m_ptr(std::move(other).get())
  58 | +    {
  59 | +    }
  60 | +
  61 | +    template <std::convertible_to<T> U>

l0rinc commented at 9:01 PM on September 21, 2026:

199999b util: Add util::NotNull<SmartPtrType>:

The constraint checks an rvalue U, but the body converts a const U&, so they don't match:

    template <class U>
        requires std::convertible_to<const U&, T>

Same for the lvalue operator T() const&, which lacks the requires and noexcept its rvalue twin has.

<details><summary>tests to constrain pointer copies</summary>

    static_assert(!std::constructible_from<NNPtr, const NNUniqPtr&>);
    static_assert(std::constructible_from<NNPtr, NNUniqPtr&&>);
    static_assert(!std::constructible_from<std::unique_ptr<int>, const NNUniqPtr&>);
    static_assert(std::is_nothrow_constructible_v<Ptr, const NNPtr&>);

</details>

maflcko commented at 9:49 AM on September 22, 2026:

Thx, fixed up some of the explicit, requires, and noexcept a bit.

Added you as co-author for the small nits here. Hope that is fine.

in src/util/not_null.h:40 in 5555d39c68 outdated

  35 | +/// not be used.
  36 | +///
  37 | +/// This type is movable, meaning that the inner pointer *is* null after a
  38 | +/// move. Clang-tidy static analysis is used to enforce use-after-move
  39 | +/// violations, so that the null should never be observed. Moreover, the
  40 | +/// runtime Assume check in the get() member function will ensure the same.

l0rinc commented at 9:42 PM on September 21, 2026:

My understanding is that clang-tidy’s bugprone-use-after-move only checks local variables and function parameters, but misses moves from members and globals.

And Assume is only triggered in debug and fuzz builds - and since we canot have Assert - we should probably update the doc.

maflcko commented at 9:49 AM on September 22, 2026:

Right, but nullptr deref later on will be UB (usually a crash), which is caught by tests. If someone manages to trick clang-tidy, and reviewers, while not writing tests, one of the red teams will hopefully come to the rescue.

in src/test/util_pointers_tests.cpp:73 in 199999bd0b outdated

  68 | +template <class P>
  69 | +concept HasOperatorBool = requires(P val) { bool{val}; };
  70 | +
  71 | +BOOST_AUTO_TEST_CASE(check_nullptr_compare)
  72 | +{
  73 | +    // The deleted ctor for nullptr also disallows nullptr compare:

l0rinc commented at 10:18 PM on September 21, 2026:

199999b util: Add util::NotNull<SmartPtrType>:

NotNull only compares with another NotNull, so nullptr is rejected even without the deleted constructor. Could the comment say that instead?

    // Comparisons are only defined between NotNull values, so nullptr compare is rejected:

maflcko commented at 9:49 AM on September 22, 2026:

I think my shorter comment is clearer, so leaving as-is for now.

l0rinc approved

l0rinc commented at 11:04 PM on September 21, 2026: contributor

ACK 5555d39c68f02e274e99205736088cc4df67a10d

The results are quite simple now, remaining comments are nits (doc wording, alias, test dedup, an over-advertised converting-constructor constraint) and can be follow-ups given how long this one's been open. The explicit conversion suggestion touches validation.cpp, so I'm fine deferring that one as well.

maflcko force-pushed on Sep 22, 2026

util: Add util::NotNull<SmartPtrType>

Co-Authored-By: stickies-v <stickies-v@protonmail.com>
Co-Authored-By: l0rinc <pap.lorinc@gmail.com>

fa73ede72d

refactor: Use util::NotNull<std::unique_ptr<LevelDBContext>> m_db_context

This allows to drop the runtime Assert every time the context is
accessed, because the type is known to be not null at compile-time.

Note there is still the runtime Assert inside the util::NotNull
constructor itself. However, this is called at most once, with the
possibility of the compiler being able to optimize it away.

fa2e546978

refactor: Use NotNull pointer to input fetching pool fafb01a58d

refactor: In CNode use util::NotNull<std::unique_ptr<Transport>> m_transport

This documents (and checks) that the transport pointer is never null.

fae0ef60ea

refactor: Return util::NotNull<std::unique_ptr<ChangeSet>> from CTxMemPool::GetChangeSet()

This documents (and checks) that the change set is never null.

This also nudges the test code to be cleaner, because the changeset is
not manually re-set and re-used over a large scope with several sub-test
cases. Instead each change set for each sub-test case is in a small and
dedicated scope.

Can be reviewed via --ignore-all-space

33333abbaa

maflcko force-pushed on Sep 22, 2026

DrahtBot added the label CI failed on Sep 22, 2026

DrahtBot commented at 9:56 AM on September 22, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/35712238912/job/106695317694 LLM reason (✨ experimental): CI failed because the lint-include-guards check reported that src/util/not_null.h is missing the expected include guard.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Sep 22, 2026

maflcko requested review from l0rinc on Sep 22, 2026

sedited approved

sedited commented at 2:35 PM on September 22, 2026: contributor

Re-ACK 33333abbaa8f242f03c8b6e838195a00e8b1e72e

in src/test/util_pointers_tests.cpp:16 in 33333abbaa

  11 | +#include <memory>
  12 | +#include <set>
  13 | +#include <type_traits>
  14 | +#include <unordered_set>
  15 | +
  16 | +BOOST_AUTO_TEST_SUITE(util_pointers_tests)

l0rinc commented at 8:54 PM on September 22, 2026:

nit: now that not_null.h was renamed, consider applying that to the test file as well:

BOOST_AUTO_TEST_SUITE(util_not_null_tests)

in src/dbwrapper.h:197 in fa2e546978

 193 | @@ -193,7 +194,7 @@ class CDBWrapper
 194 |      friend const Obfuscation& dbwrapper_private::GetObfuscation(const CDBWrapper&);
 195 |  private:
 196 |      //! holds all leveldb-specific fields of this class
 197 | -    std::unique_ptr<LevelDBContext> m_db_context;
 198 | +    util::NotNullUniquePtr<LevelDBContext> m_db_context;

l0rinc commented at 8:56 PM on September 22, 2026:

fa2e546 refactor: Use util::NotNull<std::unique_ptr<LevelDBContext>> m_db_context:

nit: we could harden these by consting some of the new migrated values

in src/util/not_null.h:51 in fa73ede72d

  46 | +    template <std::convertible_to<T> U>
  47 | +        requires(!std::same_as<std::remove_cvref_t<U>, NotNull>)
  48 | +    constexpr explicit NotNull(U&& u) noexcept(std::is_nothrow_constructible_v<T, U&&>)
  49 | +        : m_ptr(std::forward<U>(u))
  50 | +    {
  51 | +        Assert(m_ptr != nullptr);

l0rinc commented at 9:09 PM on September 22, 2026:

fa73ede util: Add util::NotNull<SmartPtrType>:

If you need to touch again, consider adding a test for this Assert with a null smart pointer. Constructing NotNullSharedPtr<int> from a null unique_ptr<int> is not noexcept, so the existing exception test hook can observe the failure.

BOOST_AUTO_TEST_CASE(check_null_smart_pointer)
{
    // This conversion can throw, allowing the test hook to observe Assert
    static_assert(!std::is_nothrow_constructible_v<util::NotNullSharedPtr<int>, std::unique_ptr<int>>);
    test_only_CheckFailuresAreExceptionsNotAborts mock_checks{};
    BOOST_CHECK_THROW(util::NotNullSharedPtr<int>{std::unique_ptr<int>{}}, NonFatalCheckError);
}

l0rinc approved

l0rinc commented at 9:18 PM on September 22, 2026: contributor

ACK 33333abbaa8f242f03c8b6e838195a00e8b1e72e

Left some non-blocking nits in case the author decided to push again.

maflcko commented at 6:15 AM on September 23, 2026: member

(I'll leave the three nits open for now and roll them into the next pull or push)