[HashEngineering]

Issue being fixed or feature implemented

For about 1 DashPay contact pair in 512, dashj encrypts and decrypts contact requests with a non-standard ECDH key. dashj-to-dashj still works, but libsecp256k1, iOS (DashSync) and the Rust Platform SDK can't decrypt those requests, and the SDK can't build the contact's sending account.

Cause: Secp256k1ECDHAgreement.calculateAgreement computes the standard libsecp256k1 value, SHA256((y&1|2) ‖ x), but returns it as new BigInteger(Sha256Hash.hash(...)), which reads the hash as a signed number. Since #295, KeyCrypterECDH.deriveKey turns it back into bytes with BigIntegers.asUnsignedByteArray(32, ...).

• When the hash starts with 0xFF followed by a byte ≥ 0x80, the minimal two's-complement encoding drops the leading 0xFF, and the padding puts 0x00 in its place: ff80… → 0080…, ffff80… → 000080….
• Before fix: ecdh shared key width #295, those pairs (and the leading-00 ones) got a 31-byte key and failed outright. fix: ecdh shared key width #295 fixed the leading-00 case but turned the leading-FF case into a silently different key.

Confirmed on mainnet. Two wallets whose requests are both dated after #295 were affected in both directions: dashj decrypted them fine, the Rust SDK did not. An SDK build that retries with dashj's altered key decrypted the request on the first try ("Contact xpub decrypted with the dashj-legacy ECDH key") and built the account. The SDK side of that fix is in HashEngineering/platform fix/dashj-legacy-ecdh-key.

What was done?

• Secp256k1ECDHAgreement:
  • New byte[] calculateSharedSecret(CipherParameters) returns the 32 SHA256 bytes directly.
  • calculateAgreement keeps its old BigInteger result for BasicAgreement compatibility. It is now @Deprecated, and its javadoc explains the problem.
• KeyCrypterECDH.deriveKey now uses the 32 bytes directly as the AES key. This is the standard key, the same as libsecp256k1, iOS and the SDK, and it is used for all encryption.
• New, for reading data that older dashj encrypted:
  • deriveLegacyKey(ECKey, ECKey): the fix: ecdh shared key width #295 key, rebuilt exactly as asUnsignedByteArray(32, new BigInteger(hash)).
  • deriveCandidateKeys(ECKey, ECKey): the standard key first, plus the legacy key only when it differs (about 1 pair in 512).
  • decryptWithFallback(EncryptedData, ECKey, ECKey) / decryptWithFallback(EncryptedData, List<KeyParameter>): try the standard key, then the legacy key. If both fail, the standard key's error is thrown.
• Caveat, in the javadoc: AES-CBC with a wrong key passes the PKCS7 check about 1 time in 256. Callers that can validate the plaintext should loop over deriveCandidateKeys and keep the first result that parses. For example, the decrypted xpub should deserialize.

Nothing else in dashj uses these classes. The consumer is dash-sdk-kotlin (BlockchainIdentity.decryptExtendedPublicKey, plus the account-label and contactInfo decrypts). It needs a follow-up to decrypt with deriveCandidateKeys; encryption needs no change there.

How Has This Been Tested?

./gradlew :dashj-core:test --tests 'org.bitcoinj.crypto.*': 147 passed, 3 skipped. In KeyCrypterECDHTest:

• An affected pair, found by search: Alice is the existing test key 7337920e…aa01, Bob is SHA256("dashj-ecdh-test-498") = 4a42b025…6bce.
  • The standard key is ffc083b9…7e74, equal to the raw SHA256, checked against an independent hash of the compressed shared point. I also verified it with a separate Python secp256k1.
  • The legacy key is 00c083b9….
  • There are 2 candidate keys.
  • Data encrypted with the legacy key fails with the standard key (fixed IV) and decrypts through the fallback.
  • Data encrypted with the standard key decrypts, and fails with the legacy key.
• Unaffected pairs: leading ff+<80, 00+<80, 00+≥80, the original pair and the known-answer pair. The legacy key equals the standard one, there is a single candidate, and no second attempt is made.
• Shape checks: ff80→0080, ffff80→000080, ffff7f→00ff7f. ff7f, fe80, 8000, 0080, 007f, 0000, 7fff and all-zero stay unchanged.
• The existing known-answer tests still pass.

Breaking Changes

KeyCrypterECDH.deriveKey returns a different key for the affected ~1/512 pairs: the standard key instead of #295's. So for those pairs, data encrypted by this dashj can't be decrypted by an older dashj, which only knows the legacy key. iOS and the Rust SDK can now read it, in both directions. Data encrypted by an older dashj stays readable through deriveCandidateKeys / decryptWithFallback. For every other pair, the key is unchanged.

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

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • ECDH key derivation now uses the standard 32-byte shared secret.
  • Decryption can try both standard and legacy-derived keys, helping open encrypted data created with older key derivation behavior.
• Bug Fixes
  • Corrected key derivation for shared secrets with certain leading byte values, while preserving support for affected legacy data.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5d6d26e2-e8da-4927-89b7-e828208f9d4f

📥 Commits

Reviewing files that changed from the base of the PR and between 0732897 and 3398e22.

📒 Files selected for processing (3)

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

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

ECDH key derivation now uses the raw 32-byte shared secret. New APIs derive legacy keys, list distinct key candidates, and decrypt by trying candidate keys in order.

Changes: ECDH Key Compatibility

Layer / File(s)	Summary
Raw shared-secret key derivation core/src/main/java/org/bitcoinj/crypto/Secp256k1ECDHAgreement.java, core/src/main/java/org/bitcoinj/crypto/KeyCrypterECDH.java	calculateSharedSecret returns the SHA-256 ECDH secret as bytes. Standard key derivation wraps those bytes directly. The calculateAgreement method is deprecated and delegates to the byte-returning method.
Legacy and candidate-key derivation core/src/main/java/org/bitcoinj/crypto/KeyCrypterECDH.java, core/src/test/java/org/bitcoinj/crypto/KeyCrypterECDHTest.java	Legacy derivation reproduces the prior signed-BigInteger conversion. Candidate derivation returns the standard key first and adds the legacy key only when it differs. Tests cover affected and unchanged shared-secret shapes.
Fallback decryption core/src/main/java/org/bitcoinj/crypto/KeyCrypterECDH.java, core/src/test/java/org/bitcoinj/crypto/KeyCrypterECDHTest.java	Fallback decryption tries keys in order and returns the first plaintext that passes AES-CBC padding. Tests cover standard and legacy ciphertext and failed decryption.

Priority: ➖ Normal

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant KeyCrypterECDH
  participant Secp256k1ECDHAgreement
  participant AESCBC
  Caller->>KeyCrypterECDH: decryptWithFallback(data, secretKey, publicKey)
  KeyCrypterECDH->>Secp256k1ECDHAgreement: calculateSharedSecret(publicKey)
  Secp256k1ECDHAgreement-->>KeyCrypterECDH: 32-byte shared secret
  KeyCrypterECDH->>KeyCrypterECDH: derive standard and distinct legacy keys
  loop Keys in order
    KeyCrypterECDH->>AESCBC: decrypt with candidate key
    AESCBC-->>KeyCrypterECDH: plaintext or padding error
  end
  KeyCrypterECDH-->>Caller: first plaintext that passes padding, or first decryption error

Loading

Merge Risk: ⚪ Minimal · up to 3398e

No actionable merge-blocking issue remains. Callers decrypting multiple items can derive candidate keys once and reuse them.

Pre-merge checks · 4 · 1

Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 48.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks (4 passed)

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: using the standard libsecp256k1 ECDH key for DashPay encryption.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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