fix: ecdh shared key width

[HashEngineering]

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

Summary by CodeRabbit

Release Notes

• Tests

  • Added validation checks to ensure cryptographic key parameters meet expected length requirements.

• Chores

  • Improved internal key derivation mechanism for enhanced robustness and reliability.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR refines ECDH key derivation by standardizing the conversion of the shared secret to a fixed 32-byte unsigned array using BigIntegers utility, and adds test assertions validating key parameter length in key exchange operations.

Changes

Cohort / File(s)	Summary
ECDH Key Derivation Implementation core/src/main/java/org/bitcoinj/crypto/KeyCrypterECDH.java	Modified shared secret byte conversion to use BigIntegers.asUnsignedByteArray(32, sharedSecret) instead of toByteArray(), ensuring consistent 32-byte output for key derivation.
Cryptographic Tests core/src/test/java/org/bitcoinj/crypto/KeyCrypterAESCBCTest.java, core/src/test/java/org/bitcoinj/crypto/KeyCrypterECDHTest.java	Updated test implementations: switched KeyCrypterAESCBCTest from ECDH to AES-CBC testing, and added assertions verifying derived key parameter length is 32 bytes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 The bytes align, no longer stray,
Thirty-two digits hold their sway,
ECDH shares secrets neat and tight,
With unsigned arrays shining bright! ✨
Tests assert what once was left to chance,
Cryptography gets a second glance.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: ecdh shared key width' directly and clearly describes the main change: addressing ECDH shared key byte width to ensure 32-byte values with proper zero-byte handling.

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

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/ECDH-shared-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 and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

core/src/test/java/org/bitcoinj/crypto/KeyCrypterECDHTest.java (1)

99-99: Add one fixture for the zero-prefixed shared-secret case.

These length checks are useful, but they still don't pin the exact edge case this PR is fixing. Please add a vector where the derived shared secret starts with 0x00, so the regression is covered directly.

Also applies to: 120-120

🤖 Prompt for AI Agents

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

In `@core/src/test/java/org/bitcoinj/crypto/KeyCrypterECDHTest.java` at line 99,
Add a unit test fixture in KeyCrypterECDHTest that covers the zero-prefixed
shared-secret case: locate the test(s) that assert the derived key length
(references: aliceKeyParameter and the existing assertions around lines using
assertEquals(32, aliceKeyParameter.getKeyLength())) and add an additional
vector/fixture where the ECDH-derived shared secret begins with 0x00, then run
the same derivation path and assert the final key length and/or expected derived
bytes to ensure the zero-prefix is handled correctly; update both occurrences
(the current assert at aliceKeyParameter and the similar check around the other
assertion noted) so the test suite includes this regression vector.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@core/src/test/java/org/bitcoinj/crypto/KeyCrypterECDHTest.java`:
- Line 99: Add a unit test fixture in KeyCrypterECDHTest that covers the
zero-prefixed shared-secret case: locate the test(s) that assert the derived key
length (references: aliceKeyParameter and the existing assertions around lines
using assertEquals(32, aliceKeyParameter.getKeyLength())) and add an additional
vector/fixture where the ECDH-derived shared secret begins with 0x00, then run
the same derivation path and assert the final key length and/or expected derived
bytes to ensure the zero-prefix is handled correctly; update both occurrences
(the current assert at aliceKeyParameter and the similar check around the other
assertion noted) so the test suite includes this regression vector.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 97914dab-b289-4e9a-8b6e-b7c92b78ecc2

📥 Commits

Reviewing files that changed from the base of the PR and between 3777b7c and 2b5485c.

📒 Files selected for processing (3)

• core/src/main/java/org/bitcoinj/crypto/KeyCrypterECDH.java
• core/src/test/java/org/bitcoinj/crypto/KeyCrypterAESCBCTest.java
• core/src/test/java/org/bitcoinj/crypto/KeyCrypterECDHTest.java