Spark: Fix spend key buffer reuse

[reubenyap]

PR intention

Prevent the Windows access violation observed after clicking Anonymize All.

Spark deterministic spend-key construction cleared its hash input and output vectors, then wrote through storage outside the vectors' live element ranges. This undefined behavior could corrupt memory and surface later in unrelated code, which explains the misleading CNodeStats and std::string frames in the reported stack trace.

Code changes

• Keep both key-derivation buffers at their required sizes while they are reused.
• Reset sensitive intermediate material with memory_cleanse instead of shrinking the vectors with clear().
• Use vector::data() only while the buffers contain live elements.
• Add a regression test for deterministic s1 and s2 derivation from the same spend-key scalar.

Testing

• git diff --check
• Added spark_address_tests/spend_key_recovery
• Full Boost test execution is pending CI because this checkout has no configured build directory.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2b17dab7-3fc3-4ba3-823f-8503bf034b7d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• ✅ Review completed - (🔄 Check again to review again)

---

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.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/libspark/keys.cpp`:
- Around line 38-42: After the final s2.memberFromSeed call in the
key-derivation flow, explicitly cleanse both sensitive buffers, data and result,
before returning or allowing them to be destroyed. Use the existing project
zeroization utility and ensure cleansing occurs after the derived key has been
consumed.

🪄 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 Plus

Run ID: 58676a5f-435f-48a6-9641-d79201045b8a

📥 Commits

Reviewing files that changed from the base of the PR and between 7af906d and 3997075.

📒 Files selected for processing (2)

• src/libspark/keys.cpp
• src/libspark/test/address_test.cpp

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Cleanse the final key-derivation buffers before returning.

After Line 36, data contains serialized s1. After Line 41, result contains the final hash output. The std::vector destructors release storage but do not guarantee zeroization. Sensitive intermediate data can remain in freed heap memory.

Add cleansing after the final memberFromSeed call.

Proposed fix

     hash256.Finalize(result.data());
     this->s2.memberFromSeed(result.data());
+    memory_cleanse(data.data(), data.size());
+    memory_cleanse(result.data(), result.size());
 }

📝 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

	std::string prefix2 = "s2_generation";
	hash256.Write(reinterpret_cast<const unsigned char*>(prefix2.c_str()), prefix2.size());
	hash256.Write(data.data(), data.size());
	hash256.Finalize(&result[0]);
	this->s2.memberFromSeed(&result[0]);
	hash256.Finalize(result.data());
	this->s2.memberFromSeed(result.data());
	std::string prefix2 = "s2_generation";
	hash256.Write(reinterpret_cast<const unsigned char*>(prefix2.c_str()), prefix2.size());
	hash256.Write(data.data(), data.size());
	hash256.Finalize(result.data());
	this->s2.memberFromSeed(result.data());
	memory_cleanse(data.data(), data.size());
	memory_cleanse(result.data(), result.size());

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/libspark/keys.cpp` around lines 38 - 42, After the final
s2.memberFromSeed call in the key-derivation flow, explicitly cleanse both
sensitive buffers, data and result, before returning or allowing them to be
destroyed. Use the existing project zeroization utility and ensure cleansing
occurs after the derived key has been consumed.

[reubenyap]

Do not review this. Has serious issues.

[reubenyap]

Closing in favor of #1894. Keeping data sized here changes the s2 seed hash from SHA256d("s2_generation") to SHA256d("s2_generation" || ser(s1)), which changes every existing wallet's view keys and addresses — the same derivation break that #1767 introduced and #1784 reverted. #1894 fixes the out-of-range vector writes while keeping the hash inputs byte-for-byte identical, and adds a test that pins the deployed derivation so it can't silently drift again.

---

Generated by Claude Code