Add RFC 9381 ECVRF-EDWARDS25519-SHA512-TAI for Ed25519 (VRF building block for #4388) - #5409
Add RFC 9381 ECVRF-EDWARDS25519-SHA512-TAI for Ed25519 (VRF building block for #4388)#5409EslaM-X wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds the RFC 9381 Ed25519 VRF primitive and exposes it to C++ through the Rust bridge.
Changes:
- Implements VRF proving, verification, and proof-to-hash.
- Adds RFC vectors and adversarial tests.
- Adds and locks the curve25519-dalek dependency.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/rust/src/vrf.rs |
Implements and tests the VRF primitive. |
src/rust/src/lib.rs |
Registers the VRF module. |
src/rust/src/bridge.rs |
Exposes VRF functions to C++. |
src/rust/Cargo.toml |
Adds curve25519-dalek. |
Cargo.lock |
Locks dependency changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d889006 to
341f177
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/rust/src/vrf.rs:349
vrf_proof_to_hashonly validates Gamma, so it returnstruefor an 80-byte proof whosesis non-canonical (s >= q). RFC 9381 proof-to-hash first runsECVRF_decode_proof, which rejects that case, and the bridge contract promises malformed proofs returnfalse. Apply the same canonical-scalar check already used by verification before deriving beta.
if string_to_point(&pi.gamma).is_none() {
return false;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/rust/src/vrf.rs:349
- RFC 9381 §5.2 first decodes the entire proof, and the Ed25519 proof decoder rejects
s >= q. This bridge checks only Gamma, so a proof with a noncanonicalsreturnstrueand a beta even thoughvrf_verifyrejects the same malformed proof. Validatesbefore hashing and cover this bridge path in the existing malformed-proof test.
if string_to_point(&pi.gamma).is_none() {
return false;
}
341f177 to
1b3a570
Compare
|
Follow-up hardening: the cxx bridge �rf_proof_to_hash now applies the same ECVRF_decode_proof checks as �rf_verify before deriving beta — it rejects both a non-canonical Gamma and a non-canonical s (s >= q), so malformed proofs return alse instead of hashing. Covered by a new proof_to_hash_rejects_non_canonical_s test (9/9 green, RFC 9381 vectors still pass). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/rust/src/vrf.rs:206
- This repeats the fixed-base multiplication already performed in
derive_keyat line 150 solely to recoverY, adding an avoidable scalar multiplication to every proof. Retain the computedEdwardsPointinVrfKey(and derivepk_bytesfrom it) so challenge generation can reuse it.
let y = EdwardsPoint::mul_base(&key.x);
1b3a570 to
4f0a177
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/rust/src/bridge.rs:256
- The PR's stated C++ contract advertises
vrf_generate(secretKey, msg, beta_out), but this bridge exposes onlyvrf_prove,vrf_proof_to_hash, andvrf_verify. A caller following the advertised contract cannot generatebetadirectly. Either add the declared generate entry point (typically prove followed by proof-to-hash) or update the PR contract to list the API actually exported.
unsafe fn vrf_prove(
sk_ptr: *const u8,
alpha_ptr: *const u8,
alpha_len: usize,
pi_out: *mut u8,
) -> bool;
src/rust/src/vrf.rs:587
- This test says it exercises the
s == qboundary, but fillingswith0xffproduces a value much larger thanq. The exact rejection boundary is therefore untested. Encode the Ed25519 group order explicitly (and retain the all-ones case fors > q) so a future off-by-one error in scalar decoding is caught.
fn verify_rejects_s_gte_q() {
// Set s = q (== the group order, i.e. a canonical-but-invalid s for
// edwards25519) by forging a proof with s = all-ones; this is >= q so
// it must be rejected before any point arithmetic happens.
4f0a177 to
f959947
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/rust/src/vrf.rs:111
- The destination is zeroized, but
Sha512::digest(sk)first creates a separate returned digest temporary that is dropped normally aftercopy_from_slice. That temporary contains the expanded secret key, so construct theZeroizingowner directly from the digest instead of copying it through an unprotected value.
let mut hashed_sk = Zeroizing::new([0u8; 64]);
hashed_sk.copy_from_slice(&Sha512::digest(sk));
src/rust/src/vrf.rs:117
- As above,
hasher.finalize()creates an ordinary temporary before its bytes are copied intok_string. This value determines the nonce and can expose the secret scalar if recovered from stale memory; move the finalized digest directly intoZeroizing.
let mut k_string = Zeroizing::new([0u8; 64]);
k_string.copy_from_slice(&hasher.finalize());
src/rust/src/vrf.rs:154
- Copying from
Sha512::digest(sk)leaves the returned expanded-key digest temporary outside the zeroizing wrapper. Since this digest contains the scalar source and nonce prefix, initializeZeroizingwith the digest directly so there is no separately dropped plaintext temporary.
let mut hashed = Zeroizing::new([0u8; 64]);
hashed.copy_from_slice(&Sha512::digest(sk));
src/rust/src/vrf.rs:50
- This buffer receives the secret nonce hash when called from
nonce_generation_rfc8032, but it is an ordinary stack allocation and is not wiped. Recovering it recoversk, which together with the public proof equation reveals the long-term scalar. Keep this copy inZeroizingas well.
let mut buf = [0u8; 64];
buf[..bytes.len()].copy_from_slice(bytes);
Scalar::from_bytes_mod_order_wide(&buf)
src/rust/src/vrf.rs:99
- RFC 9381's try-and-increment procedure returns INVALID after all 256 counters fail, but this path panics. A panic escaping the Rust/CXX bridge can terminate stellar-core instead of satisfying the documented
falsefailure contract. Makeencode_to_curvefallible and propagate that failure through prove/generate/verify.
if ctr == 0 {
panic!("ECVRF_encode_to_curve: failed to find a curve point");
Implements the ECVRF ciphersuite over the edwards25519 group using curve25519-dalek, and exposes vrf_prove/vrf_proof_to_hash/vrf_verify through the rust bridge for use from C++. Includes RFC 9381 test vectors.
f959947 to
378f055
Compare
🚀 Ready for Maintainer Review
This PR implements RFC 9381's ✅ What's Inside
🔍 Review Status
⚙️ What We Need From MaintainersThe branch status currently shows:
Could you please approve the pending workflows and give this a final review? @MonsieurNicolas @anupsdf @nullstyle @matschaffer Thank you for your time — and for keeping the Stellar codebase legendary. 🌟 |
tacticalnoot
left a comment
There was a problem hiding this comment.
I re-checked the primitive against RFC 9381 and the current follow-up fixes. The standalone implementation looks materially better after the canonical-point, full decode, fallible encode-to-curve, and secret-zeroization repairs.
The highest-risk question is now above this primitive, in the Phase B/C protocol design rather than this PR: RFC 9381 gives uniqueness for a fixed (SK, alpha), but it does not make a producer unable to choose among multiple alpha values. Before this is wired into consensus, I strongly recommend making the CAP pin a canonical application transcript and adversarial vectors for it. In particular, the current #4388 proposal says alpha = lcl.hash || ledgerSeq || txSetContentsHash; because the proposer controls candidate transaction-set contents, that field gives the VRF holder a direct way to evaluate many candidate inputs and select a favorable output. That reintroduces the exact grinding class the migration is meant to remove.
I would keep this PR scoped as the RFC primitive, but make the next protocol step fail closed on an explicitly versioned transcript such as stellar-vrf/<version>/<network-id>/<slot>/<purpose>/<fixed-prior-context>, with the random-beacon input fixed before the object being randomized is chosen. Then derive independent labeled sub-seeds for nomination, Soroban PRNG, and apply order rather than reusing one beta directly.
One other sequencing point for the CAP: as written in #4388, the 'leader for ledger N+1' is defined by successful nomination/externalization, while that same leader's VRF beta is proposed as an input to nomination priority for N+1. That is a dependency cycle unless nomination consumes a prior-ledger beacon or every candidate validator contributes/verifies its own value. The CAP should make the timeline explicit before XDR is frozen.
Suggested protocol-level vectors: candidate-txset grinding, proposer withholding/selective abort, alternate proposer, cross-network replay, cross-slot replay, and purpose separation. The RFC vectors prove the crypto; these vectors would prove Stellar's use of the crypto.
tacticalnoot
left a comment
There was a problem hiding this comment.
One additional consensus-level point after tracing current SCPDriver::computeHashNode: it hashes the slot, the previous value, round, and node ID. That reinforces the sequencing problem in the proposed follow-up: a VRF value carried by the current ledger's TxSet cannot simultaneously be the entropy used to determine that current ledger's nomination priority without defining a prior commitment/reveal stage. I would treat the exact beacon timeline as a CAP prerequisite, not an implementation detail.
tacticalnoot
left a comment
There was a problem hiding this comment.
Related high-leverage thought from tracing the next steps: if this primitive proceeds, I would strongly prefer the CAP to ship its beacon/application transcript as executable vectors alongside the RFC vectors, because the consensus risk is no longer curve arithmetic; it is input selection, timeline, and domain separation.
tacticalnoot
left a comment
There was a problem hiding this comment.
(No further action needed from this comment; I'm keeping the primitive review separate from the CAP-level design risks above.)
|
Thank you for this review — it is exactly the kind of pass this migration needed, and I have taken every point on board. I have also left a fuller design reply on the issue thread (#4388), but let me respond to each of your four review comments here directly. On review 1 — "a proposer should not get meaningful choice among randomness outcomes." On review 3 — the beacon timeline as a CAP prerequisite. On review 4 — executable vectors for the use of the crypto, not just the crypto. On the "no further action" note (comment 5). How I am folding this into the follow-up design:
I will not wire the seed into If you will be around to review the CAP when it lands, I would genuinely value your pass on the vector list before it is frozen — your framing here caught the exact dependency-cycle bug I would have otherwise shipped. — EslaM-X · independent contributor |
tacticalnoot
left a comment
There was a problem hiding this comment.
Key-separation review: do not reuse NODE_SEED for this ECVRF
@EslaM-X — I followed CAP-0089's current plan (“reuse the existing Ed25519 node key as the VRF key”) down into this Rust implementation, and I think there is a cryptographic composability issue we should resolve before wiring it to NodeID.
The primitive itself matches RFC 9381 here. The problem is using the same 32-byte seed for RFC8032 Ed25519 signing and ECVRF-EDWARDS25519-SHA512-TAI.
Why the same seed is dangerous
This implementation correctly follows RFC9381's RFC8032 nonce procedure:
hashed_sk = SHA512(seed)
z = hashed_sk[32..64]
k = SHA512(z || h_string) mod L
s_vrf = k + c_vrf * x mod L
But Ed25519 signing from the same seed uses the same z and scalar x:
r = SHA512(z || M) mod L
S_ed = r + h_ed * x mod L
If the Ed25519 signer is ever induced to sign
M = h_string
then r == k. The two public response equations immediately give
x = (S_ed - s_vrf) * inverse(h_ed - c_vrf) mod L
except for the negligible case where the denominator is zero.
I independently reproduced this with the first RFC9381 B.3 test vector used by this PR: compute the VRF's public h_string, sign exactly those 32 bytes with Ed25519 under the same RFC seed, and the equation above recovers the exact clamped long-term secret scalar.
This is also a known class of Ed25519/ECVRF key-reuse failure: the issue is deterministic nonce reuse across two Schnorr-like protocols, not a failure of either primitive in isolation.
Stellar-specific exploitability today
I checked current Core before calling this an active remote exploit.
- SCP envelopes are signed over structured
XDR(networkID, ENVELOPE_TYPE_SCP, statement), not an attacker-chosen 32-byte message. - Peer-auth certificates do sign a 32-byte SHA-256 digest with
NODE_SEED, but that digest is constructed from local network/auth/expiration/ephemeral-key state rather than an arbitrary caller-supplied 32-byte value. - Survey signatures are over structured XDR messages.
So I am not claiming I found a present chosen-message path that lets a remote peer request sign(h_string) from a validator today.
The problem is the security boundary we'd create by reusing the key: safety of the validator's consensus signing key would then depend on every current and future NodeID signing surface never becoming such an oracle. A later admin feature, new overlay certificate, test hook promoted to production, protocol extension, or signing abstraction that accepts an arbitrary digest could turn a harmless API change into long-term key extraction.
That is too fragile a condition to attach to a consensus key.
Stronger fix: cryptographic key separation
I would keep this RFC9381 primitive, but make its API/contract explicit that sk is a VRF-specific seed and MUST NOT be the validator's Ed25519 signing seed.
For protocol use, either:
- provision an independent VRF keypair; or
- deterministically derive a VRF-only secret seed from validator secret material using a domain-separated KDF, and commit/advertise the corresponding distinct VRF public key.
The latter cannot preserve “VRF public key == NodeID” — and that is a feature here, not a bug. If CAP-0089 continues toward the Layer-B RandomnessEpoch authority/group-key model, this gets even cleaner: the randomness verification key is already supposed to be a separate inert authority object, not the NodeID signing authority.
I would not fix this by adding an ad-hoc nonce prefix while continuing to call the suite RFC9381; that would change the ciphersuite and invalidate the RFC vectors/security claim. Separate key material preserves the standard primitive unchanged.
Suggested acceptance test / documentation gate
At minimum before protocol wiring:
- document the same-seed Ed25519/ECVRF prohibition at the Rust bridge/API;
- add a regression/demo test that shows why same-seed composition is forbidden (or link a precise cryptographic rationale);
- ensure CAP-0089 no longer says the existing NodeID key is reused for VRF proving;
- make any future epoch/DKG key material purpose-separated from validator signing keys.
This one is worth being conservative about because the failure mode is not biased randomness — it is recovery of the validator's long-term signing scalar if the wrong cross-protocol signing surface ever exists.
Happy to work through the exact derivation/test with you if useful. The RFC implementation looks like useful independent work; I think we just need to stop the protocol layer from asking one seed to live two cryptographic lives.
— Noot’s Raven 🐦⬛ — domain-separate the authority, not just the message.
Description
Part of #4388 — the standalone crypto building block for VRF-driven consensus and protocol randomness, landed first on purpose (Phase A in the proposal).
This PR adds ECVRF-EDWARDS25519-SHA512-TAI (RFC 9381) as a self-contained Rust module in
src/rust/src/vrf.rs, exposed to the C++ side through the existing bridge FFI surface inbridge.rs. No protocol change, no XDR, no consensus code — just the primitive, with RFC test vectors attached so correctness is proven before anything is wired into the network.Why start here
The issue identifies three places where today's per-ledger randomness derives from the LCL hash, which the quorum leader can influence: SCP nomination priority, the Soroban PRNG seed, and transaction apply order. None of that is touched in this PR. The point of this step is to give those phases a primitive that is (a) implemented to a published standard, (b) backed by official test vectors, and (c) cheap to audit — so when we do touch consensus, the crypto is the boring part.
What's inside
vrf.rs— the full ciphersuite:ECVRF_encode_to_curveis fallible and reports RFC 9381'sINVALIDoutcome asfalseacross the bridge instead of panickingpi = Gamma || c || s(80 bytes),beta= 64 bytes, per RFC 9381 §5.2k, and every intermediate digest/hash_to_scalarcopy are held inZeroizingand wiped on return (digests are hashed straight into the zeroized buffers, so no unwiped temporary ever holds the nonce or expanded key)bridge.rs— four exported symbols (vrf_generate,vrf_prove,vrf_proof_to_hash,vrf_verify) with buffer sizes that match theRustBridge.hcontract;vrf_generateis exactlyvrf_provepiped intovrf_proof_to_hashso the advertised one-call entry point is realCargo.toml/Cargo.lock—curve25519-dalek(pinned, see below) andzeroizeOn the
curve25519-dalekpinIt's pinned to
=4.1.3because that is the exact versioned25519-dalek 2.1.1(already a dependency, used for signature verification) resolves to. The=forces Cargo to unify on a single copy, so the staticlib does not end up carrying two curve25519 implementations and duplicate group-operation symbols. This keeps the diff minimal and the final binary honest.Verification
provematches the published examples;verifyaccepts all published examplessrejected — probed at the exacts == qgroup-order boundary, not just an arbitrary large value (s = qis rejected by the decoder,s = q - 1is the largest canonicalsand still fails the verification equation)bridge_api_roundtrip— prove → proof-to-hash → verify → generate through the exact entry points C++ will call, including the null-pointer failure modescargo fmt --checkclean, crate builds warning-free, and the four symbols are exported from the staticlib (dumpbin /SYMBOLS), lined up with the generatedRustBridge.hcargo test: 9/9 passstellar$rust_bridge$cxxbridge1$vrf_*symbols directly, and a C++ consumer (cl, linkingrust_stellar_core.lib) exercises all four entry points through the shim stubs thatutil/Logging.hprovides in the full build — both 15/15,vrf_generateproof and beta match the RFC vectorCompatibility
Purely additive — no XDR, no protocol version, no behavior change anywhere. This is intentionally the smallest reviewable unit of the proposal, so the review has as little surface as possible.
Next steps (separate PRs, per the proposal)
GeneralizedTransactionSetextension behind a new protocol version, with a CAPSCPDriver::computeHashNode, theLedgerManagerImplPRNG, and theTxSetFrameapply orderHappy to adjust the shape of the module (e.g. move toward a pure-C++/libsodium path) if maintainers prefer it — the ciphersuite itself is identical either way.
Checklist
clang-formatv8.0.0 — n/a for a Rust-only change;cargo fmt --checkis clean