Skip to content

Verify recovery byte for signature#136

Merged
b3y0urs3lf merged 2 commits into
mainfrom
sig-recovery-verify
Jul 14, 2026
Merged

Verify recovery byte for signature#136
b3y0urs3lf merged 2 commits into
mainfrom
sig-recovery-verify

Conversation

@martti007

Copy link
Copy Markdown
Collaborator

No description provided.

@martti007
martti007 requested a review from MastaP July 11, 2026 13:12

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors signature handling to use the structured Signature class instead of raw Uint8Array byte arrays across several files, including UnicitySeal, UnicitySealQuorumSignaturesVerificationRule, and SignaturePredicateVerifier. It also introduces new verification methods in SigningService and adds unit tests for SignaturePredicateVerifier. The review feedback identifies two important issues in SigningService: first, verifyWithPublicKey should explicitly check for a null recovered public key to avoid potential runtime errors or TypeScript compilation issues; second, the instance verify method should delegate to verifyWithPublicKey rather than verify to ensure the recovery byte is properly validated, preventing signature malleability.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/crypto/secp256k1/SigningService.ts
Comment thread src/crypto/secp256k1/SigningService.ts
@b3y0urs3lf

b3y0urs3lf commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

SDK QA sign-off — verified end-to-end

Fix closes both Finding A (unlockScript recovery byte, working-notes/finding-a-recovery-byte-brief.md) and Finding B candidate (BFT root-node sig recovery byte in
UnicitySealQuorumSignaturesVerificationRule) via the same one-shot change: SigningService.verifyWithPublicKey(hash, Signature, publicKey) now recovers the pubkey from (hash, sig, recovery) and cross-checks it against the expected publicKey. Tampered recovery byte ⇒ recovered pubkey ≠ expected ⇒ verify returns false.

Empirical confirmation (2026-07-14)

C5 bit-flip fuzz (tests/functional/FuzzRound5Test.ts):
seed=42 iter=1000 decodeFail=216 cleanFail=784 silentOK=0
Down from 1 → 0 silent-OK. Test un-skipped and repurposed as regression guard.

Direct PoC (tests/functional/UnlockScriptTamperReproTest.ts) — recovery byte flipped 0 → 1 on real inclusion proof:

Environment baseline tampered
Hermetic (TestAggregatorClient) OK NOT_AUTHENTICATED
Live (subscription-service aggregator) OK NOT_AUTHENTICATED

Unit-level tamper test (tests/unit/crypto/secp256k1/SigningServiceTest.ts T4):

it('rejects a signature whose recovery byte has been flipped (Finding A fix)', async () => {
  // ... flip recovery byte, expect verifyWithPublicKey → false ...
});

Regression check — zero SDK regressions

  • 108/108 jest tests passing
  • 15/15 hermetic BDD @v6a-hash-parity passing
  • V7 live @ N=100 mints — 100/100 root byte-match (PR #134 Use big endian bit ordering and byte array paths in smt #135 BE bit ordering cross-implementation contract intact)
  • Full BDD suite: 428/437 passing — 9 remaining failures are all aggregator infrastructure (bftShardId=0 has no leader) + 1 V11 shard-URL config mismatch, ZERO SDK-side

API migration note (downstream)

 SigningService.verifyWithPublicKey signature changed from (hash, sigBytes: Uint8Array, publicKey) to (hash, signature: Signature, publicKey). Existing SDK callers migrated in this PR:
 - SignaturePredicateVerifier — passes full Signature object (previously sig.bytes)
 - UnicitySealQuorumSignaturesVerificationRule:68 — typed Signature parameter (previously signature.slice(0, -1))

Any external consumer that calls verifyWithPublicKey with raw bytes will need Signature.decode(bytes) first. Cheap fix, but flag for release notes.

Verdict

Ready to merge. Fix is minimal, correct, addresses two independent findings in one code path, adds proper regression coverage, and passes hermetic + live end-to-end verification.

@b3y0urs3lf
b3y0urs3lf merged commit f4cc905 into main Jul 14, 2026
1 check passed
@b3y0urs3lf
b3y0urs3lf deleted the sig-recovery-verify branch July 14, 2026 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants