feat(key-wallet): impl Zeroize for ExtendedPrivKey

[lklimek]

Why this PR exists

• Problem: ExtendedPrivKey holds a secp256k1::SecretKey (+ ChainCode) but has no Zeroize impl, so its private scalar can't be wiped through the standard zeroize machinery. Downstream code (e.g. dash-platform's mnemonic-resolver FFI signers) must scrub the scalar by hand with SecretKey::non_secure_erase() on every code path.
• What breaks without it: with no Zeroize, callers can't wrap the key in zeroize::Zeroizing, so the private scalar leaks un-wiped whenever a ?-early-return or a panic unwinds before the manual non_secure_erase() is reached.
• Blocking relationship: unblocks downstream adoption of Zeroizing<ExtendedPrivKey>, which removes the manual, per-path scrubbing entirely.

What was done

Added a hand-written impl Zeroize for ExtendedPrivKey in key-wallet/src/bip32.rs that wipes the whole value (mirroring how RootExtendedPrivKey zeroes all of its own fields):

impl zeroize::Zeroize for ExtendedPrivKey {
    fn zeroize(&mut self) {
        // Secret key material.
        self.private_key.non_secure_erase();
        self.chain_code.zeroize();
        // Derivation metadata — cleared too so the whole value is wiped.
        self.depth.zeroize();
        self.parent_fingerprint.0.zeroize();
        self.child_number = ChildNumber::Normal { index: 0 };
        self.network = Network::Mainnet; // repr(u8)=0 discriminant, the "zero" value
    }
}

• Hand-written, not #[derive(Zeroize)]: secp256k1::SecretKey exposes non_secure_erase() rather than a Zeroize trait impl, so a derive does not compile.
• No Drop impl: ExtendedPrivKey is Copy, and Copy + Drop is forbidden. Callers get drop-time wiping via zeroize::Zeroizing<ExtendedPrivKey>.
• network reset to Network::Mainnet — the #[repr(u8)] 0 discriminant, i.e. the natural zero value. No change to dashcore::Network (kept confined to key-wallet, single crate).

Testing

• cargo check -p key-wallet — passes.
• cargo fmt — clean.

Breaking changes

None — additive Zeroize impl; ExtendedPrivKey is unchanged in layout and remains Copy.

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

Summary by CodeRabbit

• Bug Fixes
  • Improved protection for sensitive wallet keys by ensuring private key material is automatically cleared from memory when no longer needed.
  • Updated key derivation flows to work with borrowed keys, reducing unnecessary key handling and helping avoid accidental reuse.
  • Refined wallet and account validation coverage so key-related behavior is verified more reliably.

[coderabbitai]

📝 Walkthrough

Walkthrough

ExtendedPrivKey now zeroizes secret and derivation data on drop, no longer implements Copy, and dependent example and test code was updated to borrow or clone key values explicitly.

Changes

Extended key lifecycle updates

Layer / File(s)	Summary
Zeroize on drop key-wallet/src/bip32.rs	ExtendedPrivKey drops Copy, imports zeroize::Zeroize, adds a manual zeroize implementation for private key material and derivation metadata, and calls zeroize() from Drop.
Borrowed key call sites and tests dash/examples/taproot-psbt.rs, key-wallet/src/tests/account_tests.rs, key-wallet/src/tests/wallet_tests.rs	The taproot PSBT helper takes &ExtendedPrivKey, the example passes borrowed keys, and tests cache or clone key material before later assertions.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• dashpay/rust-dashcore#780: Also changes extended private-key handling to add secret wiping with Zeroize and Drop.

Suggested labels: ready-for-review

Suggested reviewers: xdustinface

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

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

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately names the main change: adding a Zeroize implementation for ExtendedPrivKey.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/zeroize-extended-priv-key

---

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.84%. Comparing base (78d1002) to head (a8c57fe).

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #833      +/-   ##
==========================================
+ Coverage   72.82%   72.84%   +0.02%     
==========================================
  Files         323      323              
  Lines       72438    72451      +13     
==========================================
+ Hits        52755    52780      +25     
+ Misses      19683    19671      -12     

Flag	Coverage Δ
core	76.94% <ø> (ø)
ffi	45.31% <ø> (+0.01%)	⬆️
rpc	20.00% <ø> (ø)
spv	90.29% <ø> (+0.06%)	⬆️
wallet	71.79% <100.00%> (+0.01%)	⬆️

Files with missing lines	Coverage Δ
key-wallet/src/bip32.rs	80.70% <100.00%> (+0.15%)	⬆️

... and 5 files with indirect coverage changes

[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 `@key-wallet/src/bip32.rs`:
- Around line 363-367: Add a focused unit test for ExtendedPrivKey::zeroize()
that exercises the new key-erasure behavior and verifies both the private key
material and chain_code are scrubbed after the call. Place the test alongside
the ExtendedPrivKey implementation in bip32.rs, and assert the fields are no
longer retaining their original values after zeroization to guard against
regressions.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9bc079e7-62d3-41b0-97c5-56ff77866456

📥 Commits

Reviewing files that changed from the base of the PR and between 78d1002 and 87e7904.

📒 Files selected for processing (1)

• key-wallet/src/bip32.rs

[lklimek]

@coderabbitai review all

[coderabbitai]

@lklimek Sure, I'll review all the changes in this PR again.

✅ Action performed

Full review finished.

[ZocoLini]

Is there a reason to not implement Drop trait and call the heroize implementation inside??

[lklimek]

Is there a reason to not implement Drop trait and call the heroize implementation inside??

error[E0184]: the trait Copy cannot be implemented for this type; the type has a destructor

https://doc.rust-lang.org/error_codes/E0184.html

[ZocoLini]

I see, I think it is worth it to implement Drop so we ensure the structure memory is always zeroed. I just checked the Copy derive and its only being used in one place, what about removing the Copy property, using clone() for that one place, and implementing Drop??

[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 `@key-wallet/src/bip32.rs`:
- Around line 364-381: The ExtendedPrivKey zeroization implementation is relying
on SecretKey::non_secure_erase(), which does not satisfy the stronger wiping
guarantee implied by the Zeroize impl and Drop path. Update the
ExtendedPrivKey::zeroize method to use a zeroize-backed secret key handling
approach for the private_key field, or remove the public Zeroize promise if that
guarantee cannot be met; keep the rest of the field clearing in place.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 85b71ab4-76eb-45e1-b1a4-317db67729a8

📥 Commits

Reviewing files that changed from the base of the PR and between 87e7904 and a8c57fe.

📒 Files selected for processing (4)

• dash/examples/taproot-psbt.rs
• key-wallet/src/bip32.rs
• key-wallet/src/tests/account_tests.rs
• key-wallet/src/tests/wallet_tests.rs

[lklimek]

I see, I think it is worth it to implement Drop so we ensure the structure memory is always zeroed. I just checked the Copy derive and its only being used in one place, what about removing the Copy property, using clone() for that one place, and implementing Drop??

done