Spark minting access violation

[reubenyap]

This pull request contains changes generated by a Cursor Cloud Agent

 

---

Note

Strengthens Spark mint/spend transaction creation and improves safety/diagnostics.

• Spark wallet: compute nLockTime under cs_main, fix fee subtraction (bounds checks, remainder handling, empty-output guard), improve coin selection errors, prevent invalid iterator use, and return clearer failure reasons; ensure change UTXO handling checks for empties; propagate mempool rejection reasons
• Spark spend: move coin selection and fee calculation under locks; validate fee deductions for transparent and private recipients; keep height-capped nLockTime
• libspark: derive key material using fixed-size buffers and Finalize(...data()); zero buffers between s1/s2 derivations; Address::decode now validates HRP length before prefix check
• Miner/validation/mempool/wallet RPC: correct LogPrintf format specifiers, safer pointer logging

These changes focus on correctness, thread-safety, and better error reporting without altering public APIs.

^{Written by Cursor Bugbot for commit c4fde15. This will update automatically on new commits. Configure here.}

[cursor]

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
_{Learn more about Cursor Agents}

[reubenyap]

bugbot run

[coderabbitai]

Warning

Rate limit exceeded

@cursor[bot] has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 13 minutes and 45 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

Walkthrough

Captured chain height under cs_main for consistent nLockTime; moved Spark fee/coin-selection and spend construction into wallet+chain locks; refined per-recipient fee distribution and runtime checks; corrected multiple LogPrintf/printf format specifiers across miner, mempool, validation, wallet, and libspark files.

Changes

Cohort / File(s)	Summary
Format specifier & logging fixes src/miner.cpp, src/txmempool.cpp, src/validation.cpp, src/wallet/rpcwallet.cpp, src/wallet/wallet.cpp	Corrected LogPrintf/printf specifiers to match actual types (%p, %d, %u, %zu), made miner log null-safe, and adjusted various numeric/size prints. No control-flow changes.
Spark & wallet: height capture for nLockTime src/spark/sparkwallet.cpp, src/wallet/wallet.cpp	Read chainActive.Height() under LOCK(cs_main) into a local nHeight and use it for txNew.nLockTime and assertions to avoid inconsistent reads.
Spark: coin selection, locking, and spend construction src/spark/sparkwallet.cpp	Wrapped fee/coin-selection and spend construction inside locked region (LOCK2(cs_main, pwalletMain->cs_wallet)), moved fee/amount logic into protected sections, and tightened error paths.
Spark: fee subtraction & recipient validation src/spark/sparkwallet.cpp	Reworked subtract-fee-from-output logic to compute per-recipient fee + remainder, deduct per-recipient (remainder to first), added checks to error if post-fee amounts are too small, and tightened loops/guards for remaining outputs.
libspark: key/data handling & address validation src/libspark/keys.cpp	Initialize buffers with fixed size via constructor, use .data() for hashing/finalize, zero-fill buffers preserving capacity, and add HRP length check in address decode.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client as Client
participant SparkWallet as SparkWallet
participant Chain as chainActive / cs_main
participant Wallet as pwalletMain / cs_wallet
participant CoinSel as CoinSelection
participant Mempool as Mempool/Broadcast

Client->>SparkWallet: request CreateSparkMint/Spend(params)
SparkWallet->>Chain: LOCK(cs_main) read chainActive.Height() -> nHeight
SparkWallet->>Wallet: LOCK(pwalletMain->cs_wallet) (via LOCK2)
SparkWallet->>CoinSel: perform coin selection & fee estimation
CoinSel-->>SparkWallet: selected coins + fee estimate
SparkWallet->>SparkWallet: compute feePerRecipient & feeRemainder, adjust outputs
SparkWallet->>Wallet: sign transaction
SparkWallet->>Mempool: submit/broadcast transaction
Mempool-->>Client: return txid / error

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Cmake build branch fix warnings #1572: touches src/libspark SpendKey implementation—related to buffer/initialization changes.
• Fixed mint too small amount case #1525: overlaps src/spark/sparkwallet.cpp minting logic (fee distribution and output validation).
• Mint refactor #1515: modifies spark minting flows and split behavior intersecting coin-selection/fee logic.

Suggested reviewers

• levonpetrosyan93
• psolstice
• aleflm

Poem

🐇 I guarded heights beneath the moonlit code,

I balanced fees and watched each tiny node,
Logs now honest, pointers snug and neat,
Locks held firm — the spend fell into beat,
Hopping onward where the blockchain glowed ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description significantly deviates from the required template structure. It lacks the mandatory 'PR intention' section explaining what the PR is intended to do, and instead contains only auto-generated Cursor Agent content without addressing the template requirements.	Add a 'PR intention' section that clearly describes the intended changes and issues being solved, following the provided template structure. The auto-generated summary can remain, but must not replace the mandatory template sections.
Docstring Coverage	⚠️ Warning	Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Spark minting access violation' is specific and relates to a core issue (Spark minting safety), but the changes span multiple files and systems beyond just Spark minting, including validation, mempool, wallet RPC, and logging fixes.

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

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch cursor/spark-minting-access-violation-97c8

---

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

Caution

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

⚠️ Outside diff range comments (1)

src/miner.cpp (1)

1165-1172: Fix format specifier mismatches in logging statements.

These lines pass non-string types to %s format specifiers, which can cause crashes or undefined behavior in tinyformat:

Specifier mismatches

• pindexPrev->nHeight is int, not string
• pblock->nVersion is int32_t, not string
• pblock->nTime is uint32_t, not string
• pblock->nNonce is uint32_t (currently also incorrectly passed as pointer with &)

Proposed fix

-            LogPrintf("pindexPrev->nHeight: %s\n", pindexPrev->nHeight);
+            LogPrintf("pindexPrev->nHeight: %d\n", pindexPrev->nHeight);
             LogPrintf("pblock: %s\n", pblock->ToString());
-            LogPrintf("pblock->nVersion: %s\n", pblock->nVersion);
-            LogPrintf("pblock->nTime: %s\n", pblock->nTime);
-            LogPrintf("pblock->nNonce: %s\n", &pblock->nNonce);
+            LogPrintf("pblock->nVersion: %d\n", pblock->nVersion);
+            LogPrintf("pblock->nTime: %u\n", pblock->nTime);
+            LogPrintf("pblock->nNonce: %u\n", pblock->nNonce);

🤖 Fix all issues with AI agents

In `@src/miner.cpp`:
- Around line 1109-1112: The LogPrintf in the coinbaseScript check dereferences
coinbaseScript->reserveScript.empty() causing a crash when coinbaseScript is
null; update the if block (around the coinbaseScript check in miner.cpp, where
coinbaseScript and its reserveScript are inspected) to avoid dereferencing when
coinbaseScript is null by logging coinbaseScript pointer and conditionally
logging reserveScript.empty() only if coinbaseScript is non-null (use a ternary
or separate branches) before throwing the std::runtime_error.

In `@src/txmempool.cpp`:
- Around line 521-524: The log message in the tx removal branch is misleading:
you're logging "IsSpend()=%d" while printing the result of HasPrivateInputs(),
and that value is always false in this branch; update the LogPrintf call in the
block that handles !it->GetTx().HasPrivateInputs() to either (a) change the
label to "HasPrivateInputs()=%d" so it matches the printed value, or (b) remove
the redundant boolean argument entirely and simplify the message (e.g.,
"removeUnchecked txHash=%s (no private inputs)")—locate the LogPrintf invocation
near the check of it->GetTx().HasPrivateInputs() and adjust the format string
and arguments accordingly.

🧹 Nitpick comments (1)

src/spark/sparkwallet.cpp (1)

852-853: Correct fix for format specifier.

Changing nFeeRet to CAmount and the format specifier from %s to %d correctly addresses the access violation. However, logging nFeeRet when it's always 0 at this point seems like leftover debug code.

Consider removing or relocating the debug log

If this log is for debugging, consider removing it or moving it to where nFeeRet has a meaningful value:

             CAmount nFeeRet = 0;
-            LogPrintf("nFeeRet=%d\n", nFeeRet);

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

[reubenyap]

Security Audit Report: PR #1767 (Spark Minting Access Violation)
Executive Summary
This PR addresses multiple stability and crash issues related to Spark minting operations. No security vulnerabilities were introduced by this PR. All changes are legitimate bug fixes that improve the security posture of the codebase by eliminating undefined behavior, memory corruption risks, and crash vectors.

Critical Findings (Bugs Being Fixed)

1. CRITICAL: Memory Corruption in SpendKey Constructor (keys.cpp)
   Original Bug:

data.clear();
result.clear();
hash256.Reset();
s1.serialize(data.data());  // Writes to empty vector - UNDEFINED BEHAVIOR
// ...
hash256.Finalize(&result[0]);  // Accesses empty vector - UNDEFINED BEHAVIOR

Fix Applied: Uses std::fill() to zero buffers instead of clear(), maintaining buffer size.

Security Impact: The original code caused heap/stack corruption that could manifest as crashes with corrupted stack traces. This could theoretically be exploited for memory manipulation attacks. The fix properly zeros sensitive cryptographic material before reuse.

Assessment: ✅ Legitimate critical security fix

2. CRITICAL: Erase-While-Iterating Bug in Fee Subtraction (sparkwallet.cpp)
   Original Bug:

singleTxOutputs.erase(singleTxOutputs.begin() + i);
reminder += singleTxOutputs[i].v - singleFee;  // Accesses shifted/invalid element!
--i;
singleTxOutputs[i].v -= singleFee;  // Still executes after erase
After erase(), singleTxOutputs[i] references a different element or is out of bounds, causing undefined behavior and potential memory corruption.

Fix Applied: Complete rewrite with proper bounds checking and no mid-loop erasure.

Security Impact: This could cause wallet crashes and potentially memory corruption when anonymizing funds.

Assessment: ✅ Legitimate critical fix

3. HIGH: Out-of-Bounds Access in Address Decoding (keys.cpp)
   Original Bug: decoded.hrp[0] and decoded.hrp[1] accessed without size validation.

Fix Applied: Added if (decoded.hrp.size() < 2) check before access.

Security Impact: Malformed address strings could cause out-of-bounds reads, potentially leaking memory or causing crashes.

Assessment: ✅ Legitimate security fix

4. HIGH: Iterator Invalidation on Empty Container (sparkwallet.cpp)
   Original Bug:

while (remainingMintValue > 0) {
    // Accesses remainingOutputs.begin()->v without checking if empty
}

Fix Applied: Added && !remainingOutputs.empty() condition.

Security Impact: Prevents dereferencing invalid iterators, which could cause crashes or memory reads from invalid locations.

Assessment: ✅ Legitimate fix

5. MEDIUM: Null Pointer Dereference in Miner (miner.cpp)
   Original Bug:

if (!coinbaseScript || coinbaseScript->reserveScript.empty()) {
    LogPrintf("... coinbaseScript->reserveScript.empty()=%s\n", 
              coinbaseScript->reserveScript.empty());  // Dereferences null!

Fix Applied: Uses ternary operator to only dereference when non-null.

Security Impact: Prevents miner crash on null pointer dereference.

Assessment: ✅ Legitimate fix

6. MEDIUM: Thread-Safety Issues with chainActive Access
   Original Bug: Multiple locations accessed chainActive.Height() without holding cs_main lock.

Fix Applied: Proper locking with LOCK(cs_main) before access.

Security Impact: Prevents potential race conditions that could lead to inconsistent state or crashes.

Assessment: ✅ Legitimate thread-safety fix

7. MEDIUM: Division by Zero Risk in Spend Transaction
   Original Bug: Fee subtraction calculated fee / recipientsToSubtractFee without checking if recipientsToSubtractFee > 0.

Fix Applied: Added conditional: recipientsToSubtractFee > 0 ? fee / recipientsToSubtractFee : 0

Security Impact: Prevents potential division by zero crashes.

Assessment: ✅ Legitimate fix

8. LOW: Map Key Typo (sparkwallet.cpp, wallet.cpp)
   Original Bug: mapMultiArgs.at("change") instead of mapMultiArgs.at("-change")

Fix Applied: Corrected to "-change"

Security Impact: Would cause std::out_of_range exception when -change argument is used.

Assessment: ✅ Legitimate bug fix

9. LOW-MEDIUM: Format Specifier Crashes
   Multiple Files: Using %s format specifier for integers, booleans, and size_t values.

Fix Applied: Changed to appropriate specifiers (%d, %u, %zu, %p).

Security Impact: tinyformat would interpret integer values as memory addresses, causing access violations. While primarily a stability issue, malformed log inputs could theoretically be used in format string attacks.

Assessment: ✅ Legitimate fixes

Potential Concerns (Minor)
std::fill vs Secure Zeroing: The use of std::fill for clearing sensitive cryptographic buffers could potentially be optimized away by compilers. For maximum security, sodium_memzero() or similar secure memset functions are recommended. Risk: Low - compilers typically don't optimize away writes to data that's subsequently read.

Error Message Change: "Signing transaction failed" → "Transaction not allowed in mempool" is more accurate but could affect any code parsing error strings. Risk: Minimal

Behavioral Change in Fee Distribution: The new fee subtraction gives the remainder to the first output unconditionally, whereas the old (broken) code had more complex logic. The new behavior is acceptable and the old code was non-functional anyway. Risk: None

Items NOT Found (Negative Findings)
❌ No introduction of new vulnerabilities
❌ No backdoors or suspicious code patterns
❌ No cryptographic weaknesses introduced
❌ No data leakage vectors
❌ No authentication/authorization bypasses
❌ No injection vulnerabilities
❌ No privilege escalation paths
Recommendation
APPROVE - This PR should be merged. It fixes multiple critical and high-severity bugs that cause crashes and undefined behavior in the Spark wallet implementation. The fixes are well-implemented and follow secure coding practices. The changes improve both stability and security of the codebase.