backport: Merge bitcoin#26053, 28230, 28500

[vijaydasmp]

Bitcoin backports

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[thepastaclaw]

✅ Review complete (commit e6df404)

[coderabbitai]

Walkthrough

This pull request refactors key validity tracking and RPC parameter handling across multiple systems. The cryptographic key storage mechanism in src/key.cpp and src/key.h is migrated from a boolean validity flag (fValid) to pointer-based presence checking using a newly introduced secure_unique_ptr<KeyType>, with explicit allocation and clearing operations. A new secure RAII wrapper infrastructure (SecureUniqueDeleter and make_secure_unique) is added to src/support/allocators/secure.h. Concurrently, RPC parameter handling is refactored across src/rpc/util.h and src/rpc/util.cpp to replace manual request.params indexing with a type-safe template-based API (Arg<T>() and MaybeArg<T>()), with supporting implementations and default-value logic. Existing RPC handlers in mining, wallet, and other modules are updated to use the new API, and new functional test coverage is added for the modified wallet funding behavior.

Sequence Diagrams

sequenceDiagram
    participant Handler as RPC Handler
    participant HelpMan as RPCHelpMan
    participant ArgValue as ArgValue Impl
    participant Request as JSONRPCRequest
    
    Handler->>HelpMan: self.Arg<T>(i) or self.MaybeArg<T>(i)
    HelpMan->>HelpMan: store m_req = &request
    HelpMan->>ArgValue: compute ArgValue<T>(i)
    ArgValue->>Request: check param presence & type
    
    alt Arg<T>: required or default present
        ArgValue->>Request: extract and convert value
        ArgValue-->>HelpMan: return T (by value/ref)
    else MaybeArg<T>: optional without default
        ArgValue-->>HelpMan: return empty optional or nullptr
    end
    
    HelpMan-->>Handler: return typed argument
    Handler->>Handler: use typed parameter

Loading

sequenceDiagram
    participant Code as Calling Code
    participant CKey as CKey Instance
    participant Keydata as secure_unique_ptr<KeyType>
    participant Secp as libsecp256k1
    
    Code->>CKey: MakeNewKey() or Load()
    CKey->>CKey: MakeKeyData() - allocate if needed
    CKey->>Keydata: allocate via make_secure_unique
    CKey->>CKey: fill/copy key bytes into *keydata
    
    Code->>CKey: Sign() or Negate()
    CKey->>CKey: assert(keydata != nullptr)
    
    alt keydata present
        CKey->>Secp: pass keydata->data()
        Secp-->>CKey: return result
    else keydata absent (nullptr)
        CKey->>CKey: abort via assert
    end
    
    Code->>CKey: Derive() or other operation
    CKey->>CKey: on failure: ClearKeyData()
    CKey->>Keydata: reset to nullptr

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.11% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: backporting three Bitcoin PRs related to RPC improvements and memory optimization.
Description check	✅ Passed	The description is very brief but related to the changeset, indicating these are Bitcoin backports without specific details.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/support/allocators/secure.h (1)

12-14: ⚠️ Potential issue | 🔴 Critical

Add #include <utility> and fix the std::forward template argument on line 79.

Line 79 is ill-formed: std::forward requires an explicit template argument. This header is also missing a direct include of <utility>, which declares std::forward, and should not rely on transitive includes.

Proposed fix

 `#include` <memory>
 `#include` <string>
+#include <utility>
 `#include` <vector>
@@
-        return secure_unique_ptr<T>(new (p) T(std::forward(as)...));
+        return secure_unique_ptr<T>(new (p) T(std::forward<Args>(as)...));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/support/allocators/secure.h` around lines 12 - 14, Add a direct include
for <utility> at the top of secure.h and correct the ill-formed std::forward
usage by supplying the proper template parameter; e.g., if the forwarding occurs
inside a template<typename T> function or template<typename... Args> wrapper,
replace std::forward(arg) with std::forward<T>(arg) or std::forward<Args>(arg)
respectively so the template argument is explicit (update the forward call at
the site currently using std::forward without a template argument).

🧹 Nitpick comments (1)

src/rpc/util.h (1)

399-422: Constrain the Arg() / MaybeArg() template API to prevent link-time failures for unsupported types.

The templates accept broad parameter types via if constexpr dispatch, but src/rpc/util.cpp:572-579 provides only six ArgValue specializations: std::optional<double>, std::optional<bool>, const std::string*, int, uint64_t, and const std::string&.

Currently, all call sites (Arg<int>, Arg<uint64_t>, Arg<std::string>, MaybeArg<bool>, MaybeArg<std::string>) are covered. However, adding an unsupported type like Arg<int64_t> or MaybeArg<int> would compile here but fail only at link time.

Consider using static_assert, concepts, or explicit overloads to narrow the API surface and catch misuse in the header.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/rpc/util.h` around lines 399 - 422, The Arg/MaybeArg templates can accept
unsupported R and only fail at link time because ArgValue has a limited set of
specializations; add a compile-time guard in the header to reject unsupported
types: inside template<typename R> auto Arg(size_t i) and template<typename R>
auto MaybeArg(size_t i) add a static_assert (or a small concept) that constrains
R to the explicit types supported by ArgValue (for Arg: int, uint64_t, const
std::string&, and numeric types you actually specialize like double if present;
for MaybeArg: std::optional<double>, std::optional<bool>, const std::string*),
or rewrite the overloads to only provide those valid instantiations; reference
Arg, MaybeArg and ArgValue when implementing the static_assert/concept so misuse
(e.g. Arg<int64_t> or MaybeArg<int>) fails at compile time rather than link
time.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/rpc/util.cpp`:
- Around line 539-549: DetailMaybeArg() currently returns an RPCArg::Default as
if it were an actual UniValue param, causing MaybeArg<T>() to be unable to
distinguish an omitted argument from an explicitly provided default; modify
DetailMaybeArg() (and the similar block at 560-579) so that it returns nullptr
when the request omits the parameter even if param.m_fallback holds
RPCArg::Default, and only return a pointer to the documented default for callers
that expect a materialized default (e.g., Arg()); adjust callers of
DetailMaybeArg()/MaybeArg<T>() accordingly so MaybeArg<T>() treats nullptr as
"omitted" while Arg() still receives the fallback value when necessary.

In `@src/support/allocators/secure.h`:
- Around line 63-66: The SecureUniqueDeleter::operator()(T* t) currently
deallocates storage without running the destructor for non-trivial T; update
operator() to first call std::destroy_at(t) (to run T's destructor) and then
call secure_allocator<T>().deallocate(t, 1) to free the storage. Make sure to
include <memory> or <iterator> as needed for std::destroy_at and keep the
noexcept specification. This change should be made inside the
SecureUniqueDeleter struct's operator() to ensure placement-new constructed
objects are properly destroyed before deallocation.

In `@test/functional/wallet_fundrawtransaction.py`:
- Around line 1089-1090: The loop variable named input in
test/functional/wallet_fundrawtransaction.py shadows the built-in input() (Ruff
A001); rename the loop variable (both occurrences used over decoded_psbt_inputs)
to a non-builtin name like psbt_input or decoded_input and update any uses
inside the loop (e.g., input["txid"]) accordingly so the tests reference the new
variable name everywhere within each loop (check both the loop at the
decoded_psbt_inputs iteration and the later loop around line 1128).

---

Outside diff comments:
In `@src/support/allocators/secure.h`:
- Around line 12-14: Add a direct include for <utility> at the top of secure.h
and correct the ill-formed std::forward usage by supplying the proper template
parameter; e.g., if the forwarding occurs inside a template<typename T> function
or template<typename... Args> wrapper, replace std::forward(arg) with
std::forward<T>(arg) or std::forward<Args>(arg) respectively so the template
argument is explicit (update the forward call at the site currently using
std::forward without a template argument).

---

Nitpick comments:
In `@src/rpc/util.h`:
- Around line 399-422: The Arg/MaybeArg templates can accept unsupported R and
only fail at link time because ArgValue has a limited set of specializations;
add a compile-time guard in the header to reject unsupported types: inside
template<typename R> auto Arg(size_t i) and template<typename R> auto
MaybeArg(size_t i) add a static_assert (or a small concept) that constrains R to
the explicit types supported by ArgValue (for Arg: int, uint64_t, const
std::string&, and numeric types you actually specialize like double if present;
for MaybeArg: std::optional<double>, std::optional<bool>, const std::string*),
or rewrite the overloads to only provide those valid instantiations; reference
Arg, MaybeArg and ArgValue when implementing the static_assert/concept so misuse
(e.g. Arg<int64_t> or MaybeArg<int>) fails at compile time rather than link
time.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 9483933d-8d02-4b56-b844-6d794c318a00

📥 Commits

Reviewing files that changed from the base of the PR and between 31c8464 and e6df404.

📒 Files selected for processing (10)

• src/key.cpp
• src/key.h
• src/rpc/mining.cpp
• src/rpc/util.cpp
• src/rpc/util.h
• src/support/allocators/secure.h
• src/wallet/rpc/coins.cpp
• src/wallet/rpc/spend.cpp
• src/wallet/rpc/wallet.cpp
• test/functional/wallet_fundrawtransaction.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Keep MaybeArg() from materializing documented defaults.

DetailMaybeArg() feeds RPCArg::Default back to both Arg() and MaybeArg(). For MaybeArg<T>(), that makes an omitted parameter with a documented default indistinguishable from an explicitly provided value, which defeats callers that need to detect omission separately.

🛠️ One way to preserve omission for MaybeArg()

-static const UniValue* DetailMaybeArg(CheckFn* check, const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
+static const UniValue* DetailMaybeArg(CheckFn* check, bool use_default, const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
 {
     CHECK_NONFATAL(i < params.size());
     const UniValue& arg{CHECK_NONFATAL(req)->params[i]};
     const RPCArg& param{params.at(i)};
     if (check) check(param);

     if (!arg.isNull()) return &arg;
-    if (!std::holds_alternative<RPCArg::Default>(param.m_fallback)) return nullptr;
+    if (!use_default || !std::holds_alternative<RPCArg::Default>(param.m_fallback)) return nullptr;
     return &std::get<RPCArg::Default>(param.m_fallback);
 }

-#define TMPL_INST(check_param, ret_type, return_code)       \
+#define TMPL_INST(check_param, use_default, ret_type, return_code) \
     template <>                                             \
     ret_type RPCHelpMan::ArgValue<ret_type>(size_t i) const \
     {                                                       \
         const UniValue* maybe_arg{                          \
-            DetailMaybeArg(check_param, m_args, m_req, i),  \
+            DetailMaybeArg(check_param, use_default, m_args, m_req, i), \
         };                                                  \
         return return_code                                  \
     }                                                       \
     void force_semicolon(ret_type)

-TMPL_INST(nullptr, std::optional<double>, maybe_arg ? std::optional{maybe_arg->get_real()} : std::nullopt;);
-TMPL_INST(nullptr, std::optional<bool>, maybe_arg ? std::optional{maybe_arg->get_bool()} : std::nullopt;);
-TMPL_INST(nullptr, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;);
+TMPL_INST(nullptr, false, std::optional<double>, maybe_arg ? std::optional{maybe_arg->get_real()} : std::nullopt;);
+TMPL_INST(nullptr, false, std::optional<bool>, maybe_arg ? std::optional{maybe_arg->get_bool()} : std::nullopt;);
+TMPL_INST(nullptr, false, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;);

-TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>(););
-TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>(););
-TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str(););
+TMPL_INST(CheckRequiredOrDefault, true, int, CHECK_NONFATAL(maybe_arg)->getInt<int>(););
+TMPL_INST(CheckRequiredOrDefault, true, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>(););
+TMPL_INST(CheckRequiredOrDefault, true, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str(););

Also applies to: 560-579

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/rpc/util.cpp` around lines 539 - 549, DetailMaybeArg() currently returns
an RPCArg::Default as if it were an actual UniValue param, causing MaybeArg<T>()
to be unable to distinguish an omitted argument from an explicitly provided
default; modify DetailMaybeArg() (and the similar block at 560-579) so that it
returns nullptr when the request omits the parameter even if param.m_fallback
holds RPCArg::Default, and only return a pointer to the documented default for
callers that expect a materialized default (e.g., Arg()); adjust callers of
DetailMaybeArg()/MaybeArg<T>() accordingly so MaybeArg<T>() treats nullptr as
"omitted" while Arg() still receives the fallback value when necessary.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In C++20, when a std::unique_ptr uses a custom deleter for an object created with placement new, is the deleter responsible for calling std::destroy_at (or the destructor) before deallocating the raw storage?

💡 Result:

Yes.

std::unique_ptr<T, D>’s destructor does not inherently “know” whether T was created by a normal new expression or by placement new; it simply calls the stored deleter with the stored pointer, and the deleter is required to “dispose of the pointer as appropriate for that deleter” (i.e., do whatever cleanup is correct for how the object/storage were created). [1]

For an object created with placement new, destruction and storage release are separate steps: the destructor is not called implicitly later—you must call it explicitly (e.g., via std::destroy_at(p) or p->~T()), and then (optionally) deallocate the raw storage by whatever mechanism originally provided it. [2] Also, std::destroy_at is purely destruction (it does not deallocate). [3]

So if your unique_ptr owns both:

• the object lifetime created by placement new, and
• the raw storage that must later be freed,

then your custom deleter should typically do both:

struct Del {
  void operator()(T* p) const noexcept {
    std::destroy_at(p);          // end lifetime
    ::operator delete(p);        // or pool.free(p), std::free(p), etc.
  }
};

If the storage must not be freed (e.g., stack/arena freed elsewhere), the deleter should destroy only.

Sources: [1] std::unique_ptr deleter requirements / disposal semantics (cppreference) [2] placement-new requires explicit destructor call (ISO C++ FAQ) [3] destroy uses std::destroy_at (cppreference allocator_traits::destroy)

---

Call std::destroy_at() before deallocating storage.

SecureUniqueDeleter deallocates raw storage without destroying the managed object. For non-trivial types T, this skips the destructor and can leak nested resources or bypass required cleanup. Since the object is created via placement new, the deleter must explicitly destroy it before freeing the storage.

Proposed fix

 template<typename T>
 struct SecureUniqueDeleter {
     void operator()(T* t) noexcept {
-        secure_allocator<T>().deallocate(t, 1);
+        if (t) {
+            std::destroy_at(t);
+            secure_allocator<T>().deallocate(t, 1);
+        }
     }
 };

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	struct SecureUniqueDeleter {
	void operator()(T* t) noexcept {
	secure_allocator<T>().deallocate(t, 1);
	}
	struct SecureUniqueDeleter {
	void operator()(T* t) noexcept {
	if (t) {
	std::destroy_at(t);
	secure_allocator<T>().deallocate(t, 1);
	}
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/support/allocators/secure.h` around lines 63 - 66, The
SecureUniqueDeleter::operator()(T* t) currently deallocates storage without
running the destructor for non-trivial T; update operator() to first call
std::destroy_at(t) (to run T's destructor) and then call
secure_allocator<T>().deallocate(t, 1) to free the storage. Make sure to include
<memory> or <iterator> as needed for std::destroy_at and keep the noexcept
specification. This change should be made inside the SecureUniqueDeleter
struct's operator() to ensure placement-new constructed objects are properly
destroyed before deallocation.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify builtin-shadowing occurrences in this test file
rg -nP '\bfor\s+input\s+in\b' test/functional/wallet_fundrawtransaction.py

Repository: dashpay/dash

Length of output: 1769

---

🏁 Script executed:

#!/bin/bash
# Search for 'for input in' patterns in the wallet_fundrawtransaction.py test file
cd test/functional
if [ -f wallet_fundrawtransaction.py ]; then
    echo "=== Searching for 'for input in' patterns ==="
    rg -n 'for\s+input\s+in' wallet_fundrawtransaction.py
    echo ""
    echo "=== Context around first match (lines 1085-1095) ==="
    sed -n '1085,1095p' wallet_fundrawtransaction.py
    echo ""
    echo "=== Context around second match (lines 1124-1134) ==="
    sed -n '1124,1134p' wallet_fundrawtransaction.py
else
    echo "File not found"
fi

Repository: dashpay/dash

Length of output: 1445

---

Fix Ruff A001: avoid shadowing builtin input.

Lines 1089 and 1128 use input as a loop variable, which shadows Python's builtin input() function and triggers Ruff A001 linting violations.

Proposed rename

-        for input in decoded_psbt_inputs:
-            assert_equal(input["txid"], source_tx["txid"])
+        for vin in decoded_psbt_inputs:
+            assert_equal(vin["txid"], source_tx["txid"])

This applies to both occurrences at lines 1089 and 1128.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	for input in decoded_psbt_inputs:
	assert_equal(input["txid"], source_tx["txid"])
	for vin in decoded_psbt_inputs:
	assert_equal(vin["txid"], source_tx["txid"])

🧰 Tools

🪛 Ruff (0.15.9)

[error] 1089-1089: Variable input is shadowing a Python builtin

(A001)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/functional/wallet_fundrawtransaction.py` around lines 1089 - 1090, The
loop variable named input in test/functional/wallet_fundrawtransaction.py
shadows the built-in input() (Ruff A001); rename the loop variable (both
occurrences used over decoded_psbt_inputs) to a non-builtin name like psbt_input
or decoded_input and update any uses inside the loop (e.g., input["txid"])
accordingly so the tests reference the new variable name everywhere within each
loop (check both the loop at the decoded_psbt_inputs iteration and the later
loop around line 1128).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e6df404452

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Return type errors for null required RPC args

ArgValue<int>/ArgValue<const std::string&> now does CHECK_NONFATAL(maybe_arg); when a required parameter is explicitly passed as null, DetailMaybeArg() returns nullptr and this path raises NonFatalCheckError instead of a UniValue::type_error. For RPCs switched to self.Arg<...>() (e.g. generatetodescriptor), malformed input now surfaces as internal RPC_MISC_ERROR rather than a normal type error, which changes API behavior and leaks internal-bug messaging for user input.

Useful? React with 👍 / 👎.

[thepastaclaw]

Code Review

I validated the successful agent output against the checked-out source and patch scope. The backport appears mechanically clean overall, but the newly added make_secure_unique helper is not a correct general-purpose forwarding helper because its forwarding expression is malformed and fails on any non-empty argument pack.

Reviewed commit: e6df404

💬 1 nitpick(s)

[thepastaclaw]

💬 Nitpick: make_secure_unique cannot forward constructor arguments

The new make_secure_unique template is declared as a variadic helper, but it constructs T with std::forward(as)... instead of std::forward<Args>(as).... That expression cannot instantiate for any non-empty argument pack, so the helper only works for default construction despite being introduced as a generic std::make_unique-style utility. The current CKey call site default-constructs KeyType, so this backport still works as used here, but the helper itself is defective as added in this PR.

💡 Suggested change

Suggested change

	return secure_unique_ptr<T>(new (p) T(std::forward(as)...));
	return secure_unique_ptr<T>(new (p) T(std::forward<Args>(as)...));

_{source: ['codex-general']}