Validate transferred key pairs and prepare 1.7.1

[moisesja]

Fixes #30

Summary

• validate key type plus public/private key lengths in TransferableKeyMaterial.FromKeyPair before creating the one-way transfer owner
• share validation helpers with FromRawKey so the sibling factory contracts cannot drift again
• add all-key-type regression coverage in both length directions, including empty material and the 64-byte libsodium Ed25519 secret representation
• prepare repository version metadata and changelog for the SemVer patch release NetCrypto 1.7.1
• clarify the FR-7b custody-boundary contract and document the intentional component-level ArgumentException.ParamName values
• remove the probabilistic signature[0] != 0x30 assertion from Secp256k1_IsAlwaysFixedWidthCompact (included at the owner's request): it rejected a valid compact signature ~1 run in 256 and is what turned this PR's Windows leg red; the deterministic contract — fixed 64-byte width plus the compact verify round-trip — remains

Verification

• Release build: 0 warnings, 0 errors
• main tests: 1,275 passed
• external-store tests: 21 passed
• focused transfer suite: 63 passed
• regression proof: removing the production guard made all 36 malformed-input cases fail at the intended assertion, including shorter and longer material for every key type; restoring it returned the suite to green
• flake proof for the removed assertion: across 5,000 freshly generated secp256k1 keys, 18 valid compact signatures began with 0x30 (expected ~19.5 at 1/256) and every one verified; a zero-change re-run of the failed Windows job on 23f8bad also went green
• all 11 samples completed successfully
• API coverage passed
• local pack produced NetCrypto.1.7.1.nupkg and .snupkg with version 1.7.1
• independent adversarial harness: 5,618/5,618 checks passed against assembly 1.7.1.0, including 5,000 dispose races; no findings or leaked exceptions

Scope

This PR prepares the build for 1.7.1. It does not merge, tag, create a GitHub release, or publish the NuGet package.

[moisesja]

Reviewed the diff, the FR-7b rule-13 contract, and KeyPair's borrow semantics. The core fix is correct and well-placed: the public key is validated on the defensive copy before the borrow, and the private-key length is checked inside WithPrivateKey — so malformed material is rejected while still borrowed, before the TransferableKeyMaterial constructor ever copies it into a transfer owner. Sharing ValidateKeyTypeAndPublicKey/ValidatePrivateKey between both factories is the right structural move to stop the siblings drifting again. CI is green on all four jobs. That said, three concrete issues before this merges:

1. The version bump contradicts your own SemVer claim — this should be 1.7.1, not 1.8.0. The changelog header says "this project adheres to Semantic Versioning," and the 1.8.0 entry contains exactly one section: Fixed. No added API (PublicAPI.Unshipped.txt is empty per your own task file), no new functionality. SemVer says that's a PATCH release. If the argument is "previously-accepted inputs now throw, so it's a behavior change" — that argument proves too much: by that logic it's MAJOR, not MINOR. Rejecting garbage that was never valid per the documented contract is a bug fix. Either re-cut this as 1.7.1 or state in the changelog entry why it's minor. Right now the version number is making a claim the diff doesn't support.

2. ArgumentException.ParamName from FromKeyPair names parameters that don't exist on FromKeyPair. The method's only parameter is keyPair, but the shared helpers throw with ParamName = "keyType", "publicKey", "privateKey" — and the new tests pin exactly that. A caller catching ArgumentException and inspecting ParamName gets a name that doesn't appear in the signature they called. I understand the trade (helper reuse, parity with FromRawKey, and the names do identify which part of the pair is bad), and the tests now make it a de facto contract — but the XML doc doesn't say so. At minimum, document in the <exception> tag that the parameter name identifies the offending component of the pair, so it reads as intentional rather than leaked helper internals.

3. The checked-in regression suite is thinner than the PR description implies. The description leans on the adversarial harness ("0, 1, expected±1, 2× expected, and 1 MB buffers") — but that harness lives in /tmp and is not in this PR. What's actually checked in probes only one direction per dimension: public key at expected−1, private key at expected+1, across the eight key types. Too-long public keys and too-short private keys are only covered for Ed25519 (the empty and 64-byte libsodium cases). The harness evidence dies with your scratch directory; the regression suite is what protects this forever. Adding the mirror cases (publicKeyLength + 1, privateKeyLength − 1) to the two existing [MemberData] theories is ~8 lines and closes the gap.

Also noting for the record: bundling release prep (version bump, changelog, compare links) into the fix PR is mild scope creep, but it's small and the "prepare, don't publish" boundary is clearly stated and respected, so no objection — it just makes point 1 load-bearing, since merging this commits the repo to the 1.8.0 number.

The verification story is otherwise genuinely strong — the revert-the-fix regression proof and the documented 20 intended failures is exactly the standard this repo asks for.

---

Generated by Claude Code

[moisesja]

Addressed all three review concerns in e496c6f:

1. Reclassified the release preparation as SemVer patch 1.7.1 across Directory.Build.props, CHANGELOG heading/links, task evidence, PR title/body, and package verification. I also recorded the durable classification rule in the PRD and lesson L16.
2. Updated FromKeyPair XML documentation to state intentionally that ArgumentException.ParamName identifies the offending keyPair component as keyType, publicKey, or privateKey.
3. Checked in distinct expected−1 and expected+1 public/private length cases for all eight key types. The focused suite is now 63 tests, and removing the production guard produces 36 intended malformed-input failures (only the independent null guard still passes).

Fresh verification on the restored exact artifact:

• Release build: 0 warnings / 0 errors
• main tests: 1,275 passed
• external-store tests: 21 passed
• API coverage: passed
• local package: NetCrypto.1.7.1.nupkg / .snupkg, nuspec version 1.7.1
• independent adversarial rerun: 5,618/5,618 checks, 5,000 dispose races with zero leaks, no findings; exact assembly version 1.7.1.0

[moisesja]

Re-reviewed at head 23f8bad. Verdict: approve (posted as a comment — GitHub refuses a formal approval from the PR author's own account). All three concerns from my first review are properly resolved — verified against the diff, not just the response comment:

1. SemVer — reclassified to patch 1.7.1 everywhere it appears (Directory.Build.props, changelog heading and compare links, PR title/body, milestone, pack evidence). Better than asked: the classification rule is now durable in netcrypto-prd.md §Per-release hygiene and lesson L16, so the next release doesn't re-litigate this. The L16 write-up correctly identifies why the "previously accepted call now throws" argument proves too much.
2. ParamName contract — the FromKeyPair <exception> doc now states explicitly that ParamName identifies the offending component (keyType/publicKey/privateKey) rather than the keyPair argument. It now reads as a deliberate contract.
3. Test coverage — WrongPublicKeyLengths/WrongPrivateKeyLengths theory sources now probe expected±1 in both directions for all eight key types, checked in rather than living in the /tmp harness; the regression proof was re-run (36 intended failures with the guard removed).

On the red Windows check: it is not this PR's failure, and here is exactly why. The failing test is AlgorithmEncodingTests.Secp256k1_IsAlwaysFixedWidthCompact, which this diff does not touch, and it is flaky by construction: line 84 asserts signature[0] != 0x30 on a 64-byte compact (r‖s) signature produced with a freshly generated random key. The first byte of r is effectively uniform, so ~1 in 256 runs a perfectly valid compact signature legitimately begins with 0x30 and the test fails — which is what happened here (the same test passed on this PR's previous head, and ubuntu/macos passed on this one). The HaveCount(64) assertion on the previous line already proves the signature isn't DER (a DER ECDSA sig is 70–72 bytes); the byte check adds nothing but a built-in failure rate. Re-run the Windows job to get green, and file a follow-up to drop the signature[0] assertion (or replace it with a length-only claim) — please don't fold that fix into this PR; it's out of scope for #30.

Bottom line: the fix is correct, the validation seam is properly placed inside the WithPrivateKey borrow, the version number now matches what the diff actually is, and the verification trail (revert-proof, both-direction coverage, re-run adversarial pass against the exact 1.7.1.0 assembly) is exemplary. Merge once the Windows job is re-run green.

---

Generated by Claude Code

[moisesja]

Pushed aa55d8f, folding the flaky-test fix into this PR at the owner's request (superseding my review note that suggested a separate follow-up). It removes the probabilistic signature[0] != 0x30 assertion from Secp256k1_IsAlwaysFixedWidthCompact — measured rejecting valid compact signatures 18 times in 5,000 runs (~1/256, every one verifying), and the cause of this PR's earlier Windows red. The deterministic contract stays: fixed 64-byte width plus the compact verify round-trip. Validated before push: Release build 0 warnings/0 errors, 1,232 + 21 tests green in BBS-absent mode. Lessons L17 and task evidence are included; the PR description is updated.

---

Generated by Claude Code