Repository navigation
feat(rotate): ElGamal key-rotation proofs (9 of 9) - #142
Merged
tesseract-ripple merged 8 commits intoOct 2, 2026
Merged
Conversation
- Add test_homomorphic_add_point_at_infinity: verifies secp256k1_elgamal_add returns 0 when result C1 is point at infinity - Add test_homomorphic_add_post_mergeinbox_attack: simulates post-MergeInbox inbox locking attack, verifies malicious Send is rejected at library level
- Extract "EncZero" domain tag into CANONICAL_ZERO_DOMAIN macro with a comment pointing to generate_canonical_encrypted_zero in src/elgamal.c, and derive hash_input length from sizeof() so both stay in sync - Zero-init result_c1/result_c2 before the failing elgamal_add calls and assert they are unmodified on failure (regression for issue XRPLF#115) - Remove the unconditional "If 1: attack succeeds" printf that printed confusingly even when the test passed; move the explanation into the block comment above the call
Adds the first of the ElGamal key rotation proofs from the key-rotation proof
specification. Five of the nine variants are the same relation with different
slots filled, so they share one implementation:
exists (b, sk, r) such that
pk_old = sk*G, C2 - sk*C1 = b*G, D1 = r*G, D2 = b*G + r*pk_target
mpt_rotate_core_{prove,verify} implements it; the public API stays
one-function-per-proof via thin wrappers that supply the slots and the domain
tag. This deliberately diverges from the one-file-per-proof layout of the
existing compact proofs: nine bespoke modules would mean nine copies of one
Fiat-Shamir transcript to keep correct, and the transcript is where
consensus-critical mistakes live.
Wired up so far: pi_mh (holder self-migration of the issuer mirror) and
pi_recbal (issuer-completed balance recovery). Both 4 scalars, 128 bytes.
Challenge is SHA-256 then reduce32, matching the other compact proofs. Domain
tags are prefix-free: the single-mirror variants carry an _ONLY suffix because
..._MIRROR_HOLDER would otherwise be a prefix of ..._MIRROR_HOLDER_AUDITOR_ONLY
and tags are hashed with strlen and no length prefix.
Balance zero is legal throughout and is covered by tests; no public b*G enters
any transcript, so the clawback point-at-infinity substitution is not needed.
Tests: round-trip at zero, typical and UINT64_MAX balances; every response
scalar tampered; wrong and absent context_id; each of six statement points
substituted; mismatched balance; and cross-variant substitution between pi_mh
and pi_recbal. Full suite 12/12.
pi_rec proves possession of sk_H' for a newly registered RecoveryKey, ahead of
issuer-completed recovery via pi_recbal. A holder who has lost sk_H can produce
no balance-related witness, so the proof establishes key control and nothing
else.
Its relation is identical to that of Convert-time key registration:
exists sk in Z_q such that pk = sk*G
so rather than fork a near-copy of proof_pok_sk.c, that module is generalised
over its domain separation tag. mpt_pok_sk_{prove,verify}_tagged carry the
implementation; the two public APIs are thin wrappers supplying
CMPT_POK_SK_REGISTER and CMPT_KEY_ROTATION_HOLDER_RECOVERY respectively. The tag
also personalises the nonce derivation.
The separation is the point, so it is tested in both directions: a registration
proof must not verify as a RecoveryKey proof, or an attacker who observed a
Convert could install a RecoveryKey on that account.
The registration transcript is already shipped and consensus-critical, so
test_pok_sk now pins a proof generated before this refactor and checks the
current verifier still accepts it. This is a legacy-vector test, not a
known-answer test: generate_deterministic_nonces salts with fresh entropy, so
proofs are not byte-reproducible and only verification can be pinned.
Recovery mode's transaction context digest carries an empty TxSpecific field
(there is no balance state to bind to); NULL and 32 zero bytes remain distinct
and both are covered.
Tests: round-trip with and without context_id, tampered e and s, wrong key,
wrong context, cross-tag substitution both ways. Full suite 13/13, clean under
ASan and UBSan.
…r rotation Three more of the nine key-rotation proofs. pi_ma and pi_mha are plain wrappers around the shared re-encryption core (proof_rotate_core.h), discharging the two auditor-only migration flows the review found missing from the original seven-variant spec: R_ma (issuer-anchored, covers both auditor rotation and late registration) and R_mha (holder-anchored). Domain tags follow the existing _ONLY prefix-free convention. pi_hr is not a core instantiation: it implements the review's fold-in fix for Finding f:inbox, extending the original five-conjunct holder-rotation relation with three more conjuncts binding the new CBIN ciphertext, so MergeInbox can no longer fold in an unproven inbox value after rotation. Six nonces, seven-scalar/224B wire format, bespoke transcript in proof_rotate_holder_rotate.c. 14 new negative-path assertions across the two test files, including a mismatched-inbox-balance case exercising the f:inbox fix directly.
tesseract-ripple
force-pushed
the
feature/key-rotation-proofs
branch
from
September 12, 2026 03:51
8c9c231 to
b4bf623
Compare
Fourth plain wrapper around the shared re-encryption core. Unblocked by XLS-99 landing the ledger-state-read fix for Finding f:pki: pk_I and the current issuer mirror are now resolved entirely from ledger state (IssuerMirrorEncryptionKey, falling back to InitialIssuerEncryptionKey), never a submitter-chosen transaction field, so the forgery the finding described no longer applies to this proof's statement. 7 of 9 key-rotation proofs now implemented. Remaining: the issuer-anchored and holder-anchored "both mirrors" AND-composed variants, which are not yet rigorously specified anywhere (the merged XLS-99 companion proof spec calls the holder-anchored one "outline only") and are left for follow-up rather than devised under time pressure.
tesseract-ripple
changed the title
tesseract-ripple
marked this pull request as ready for review
September 12, 2026 07:15
Both-issuer and both-holder: the two remaining "both mirrors" migration
flows, each decrypting one anchor and re-encrypting simultaneously to a
new issuer mirror and a new auditor mirror under one shared randomness
value. New shared core (proof_rotate_both_core.{c,h}) since this is a
distinct 5-reconstruction-equation relation, not an instantiation of the
existing 4-equation core.
This is the review's fix for Finding f:mhaud: the holder-anchored variant
was originally prose-only, with the shared-randomness equality check
(E1' == F1') stated as something validators "may" perform rather than a
mandatory step -- so nothing in the sigma algebra bound the auditor
ciphertext's first component, and a mismatched one went undetected. The
fix promotes both variants to a full relation with that equality
enforced unconditionally by the verifier, independent of the sigma proof
acceptance.
test_rotate_mirror_both.c exercises this directly: a proof that is
otherwise honest but carries a mismatched auditor first component must
be rejected, for both variants.
9 of 9 key-rotation proofs now implemented. 16/16 tests passing.
tesseract-ripple
changed the title
mrtcnk
approved these changes
Sep 28, 2026
mrtcnk
left a comment
mrtcnk
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for this, it's a clean, well-structured PR. I reviewed all of it against the key-rotation proof spec: both cores, π_hr, the proof_pok_sk refactor, all nine wrappers, and the four new test files. I found no correctness bugs: for all nine proofs, the commitments, reconstruction equations, challenge input order, wire order, domain tags, scalar range checks and constant-time handling match the spec.
Two minor observations, neither blocking merge:
- secp256k1_rotate_recovery_key_prove(ctx, proof, sk, pk, …) and secp256k1_mpt_pok_sk_prove(ctx, proof, pk, sk, …) take sk and pk in opposite orders.
- proof_rotate_core.c and proof_rotate_both_core.c repeat a lot of code. Factoring the shared parts into common static helpers would reduce duplication. Fine as a follow-up, or not at all.
…ry_key/pok_sk Address review note on PR XRPLF#142: secp256k1_rotate_recovery_key_prove takes (sk, pk) like the other rotate provers, while the pre-existing secp256k1_mpt_pok_sk_prove takes (pk, sk). Changing either would be churn (ABI break vs. inconsistency within the rotate family), so document the difference in both headers.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| MPT_ARG_CHECK(proof_out != NULL); | ||
| MPT_ARG_CHECK(pk != NULL); | ||
| MPT_ARG_CHECK(sk != NULL); | ||
| MPT_ARG_CHECK(domain != NULL); |
There was a problem hiding this comment.
⚪ Severity: LOW
domain_len is not validated (only domain != NULL). An empty tag (domain_len==0) weakens domain separation and risks cross-protocol proof acceptance if misused. Other rotate* cores enforce domain_len > 0; this API should similarly reject empty domains.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Add input validation to reject empty domain tags by enforcing domain_len > 0. Specifically, immediately after MPT_ARG_CHECK(domain != NULL); insert MPT_ARG_CHECK(domain_len > 0); in both mpt_pok_sk_prove_tagged and mpt_pok_sk_verify_tagged. This ensures the domain is always incorporated into the transcript and deterministic nonce derivation, preserving domain separation and aligning with the rotate cores.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements proofs for confidential MPT key rotation, per M. Cenk,
Compact Zero-Knowledge Proofs for ElGamal Key Rotation in Confidential
MPTs on the XRPL (RippleX Research, Aug. 2026), and the accompanying
security review,
T. Dore, Review of the Compact Zero-Knowledge Proofs for ElGamal Key
Rotation in Confidential MPTs (RippleX Research, Aug. 2026) — both
internal Ripple design documents.
All 9 key-rotation proofs implemented, all sharing one of two common
parameterized cores (
proof_rotate_core.{c,h},proof_rotate_both_core.{c,h})except where noted:
module (
proof_pok_skgeneralized over its domain tag; publicsecp256k1_mpt_pok_sk_*API unchanged)rotation and late registration)
landing the ledger-state-read fix for the review's
Finding f:pki
("the issuer-mirror proofs bind the balance to a submitter-chosen
key";
IssuerMirrorEncryptionKey/InitialIssuerEncryptionKeyreplace the submitter-chosen previous-key field)
bespoke 6-nonce/7-scalar transcript implementing the review's fold-in
fix for
Finding f:inbox
("the holder key-rotation proof does not
constrain the new inbox ciphertext"), which binds the rotated inbox
ciphertext into the transcript so
MergeInboxcan no longer fold inan unproven value post-rotation.
once" flows, each decrypting one anchor and re-encrypting simultaneously
to a new issuer mirror and a new auditor mirror under one shared
randomness value. New shared core since this is a distinct
5-reconstruction-equation relation. This is the review's fix for
Finding f:mhaud
("the holder-anchored AND-composed variant is
prose-only, and its one soundness-critical [equality check] is not
enforced"): the holder-anchored variant was originally prose-only, with
the shared-randomness equality check (
E1' == F1') stated as somethingvalidators "may" perform rather than mandatory — so nothing in the sigma
algebra bound the auditor ciphertext's first component. Both variants
here enforce that equality unconditionally in the verifier, and
test_rotate_mirror_both.cexercises the exact mismatched-F1' casedirectly.
16/16 tests passing, pre-commit clean.