Fix Ed25519 key generation interoperability

[GautamBytes]

What was wrong?

Issue #921

Previously, Ed25519PrivateKey.new() could probabilistically generate keys that were not valid points on the Ed25519 curve. While py-libp2p accepted these keys, strict implementations (such as Rust's curve25519-dalek, used in Zcash and other networks) rejected them. This caused persistent interoperability failures where valid-looking Python nodes could not connect to Rust nodes.

How was it fixed?

The fix ensures that all new keys generated by this library are valid curve points, complying with strict validation rules (like ZIP-215).

1. Updated Ed25519PrivateKey.new in libp2p/crypto/ed25519.py:

   • Added a validation check using crypto_core_ed25519_is_valid_point from nacl.bindings.
   • For random key generation: Implemented a loop that retries generation (up to 100 times) if an invalid key is produced, ensuring only valid keys are returned.
   • For seeded key generation: Added a validation check to raise a ValueError if the provided seed results in an invalid curve point.

2. Added Tests:

   • Created tests/core/crypto/test_ed25519_comprehensive.py with specific test cases to verify that both randomly generated and seeded keys are always valid curve points.

Note: This change specifically targets new key generation. It intentionally does not modify from_bytes (loading keys) to ensure backward compatibility for existing nodes that may possess technically invalid but previously functioning keys.

[GautamBytes]

@seetadev its ready for review!

[seetadev]

@GautamBytes : Thanks a lot, Gautam — this is a solid and very well-documented fix 👏

The root cause explanation around probabilistic generation of invalid Ed25519 points and the resulting Python ↔ Rust interoperability failures is very clear, and the approach you’ve taken makes sense:

Using crypto_core_ed25519_is_valid_point to enforce strict curve validity aligns well with ZIP-215–style validation.

The retry loop for random key generation is a pragmatic solution that preserves ergonomics while guaranteeing correctness.

Explicitly failing on invalid seeded keys is the right tradeoff.

I also appreciate the conscious decision not to modify from_bytes to avoid breaking existing nodes — that backward-compatibility note is important and well thought through.

The comprehensive tests covering both random and seeded generation are a big plus 👍

I’ll invite @acul71 and @pacrob for additional review as well, given the crypto + interoperability implications.

One small request before merge:
could you please add a newsfragment for this change?

Once that’s in and we get the remaining reviews, we should be in a good position to move this forward. Great work on this — thanks for tackling a subtle but impactful issue! 🚀

[GautamBytes]

@seetadev added the newsfragments

[acul71]

@seetadev @GautamBytes
Thanks for this PR.
LGTM.
Can be merged

Why PrivateKeyImpl can generate "invalid" keys

This comes from different validation rules for Ed25519 public keys, not a bug in PyNaCl.

The core issue

1. Ed25519 key generation:

   • PrivateKeyImpl.generate() (PyNaCl's SigningKey.generate()) follows the standard Ed25519 spec (RFC 8032).
   • It generates a private scalar and derives a public key point on the Ed25519 curve.
   • The public key is a 32-byte encoding of a point on the curve.

2. Validation differences:

   • Standard Ed25519: accepts any point that decodes and is on the curve.
   • ZIP-215 (used by Zcash, Rust curve25519-dalek): stricter checks, including:
     • The point must be in the prime-order subgroup (not the small subgroup).
     • Additional coordinate checks to avoid certain edge cases.

3. Why PyNaCl doesn't enforce ZIP-215:

   • PyNaCl follows RFC 8032, which allows points that ZIP-215 rejects.
   • These points are mathematically valid for Ed25519, but strict implementations reject them for interoperability and security reasons.

What happens in practice

When PrivateKeyImpl.generate() creates a key:

• It produces a valid Ed25519 key per RFC 8032.
• Some of these keys fail ZIP-215 validation.
• Strict implementations (e.g., Rust curve25519-dalek) reject them.
• This causes interoperability failures.

The fix

The PR adds a check using crypto_core_ed25519_is_valid_point() (from libsodium), which implements ZIP-215-style validation. This ensures:

• All generated keys pass strict validation.
• Interoperability with strict implementations.
• Keys remain valid for standard Ed25519 operations.

Why it's probabilistic

The retry loop exists because:

• Most keys pass ZIP-215 validation.
• A small fraction fail (low probability).
• The loop retries until a valid key is found.
• 100 attempts is sufficient in practice.

In summary: PyNaCl generates valid Ed25519 keys per RFC 8032, but some fail stricter ZIP-215 checks. The PR adds validation to ensure interoperability with strict implementations.

AI Pull Request Review: PR #1134 - Fix Ed25519 key generation interoperability

Review Date: 2026-01-27
PR Number: 1134
PR Title: Fix Ed25519 key generation interoperability
Author: GautamBytes
Reviewer: AI Assistant (using py-libp2p PR Review Prompt)

---

1. Summary of Changes

This PR addresses Issue #921 which reported interoperability failures between py-libp2p and Rust-based libp2p implementations (using curve25519-dalek). The problem was that Ed25519PrivateKey.new() could probabilistically generate keys that were not valid points on the Ed25519 curve. While py-libp2p accepted these keys, strict implementations rejected them, causing persistent connection failures.

Changes Made:

1. Modified libp2p/crypto/ed25519.py:

   • Added import for crypto_core_ed25519_is_valid_point from nacl.bindings
   • Updated Ed25519PrivateKey.new() method:
     • For random key generation: Implemented a retry loop (up to 100 attempts) that regenerates keys until a valid curve point is found
     • For seeded key generation: Added validation that raises ValueError if the seed produces an invalid curve point
   • This ensures all newly generated keys comply with strict validation rules (ZIP-215)

2. Enhanced tests/core/crypto/test_ed25519_comprehensive.py:

   • Added test_generated_keys_are_valid_curve_points() test method
   • Tests both random generation (50 iterations) and seeded generation with various seed patterns
   • Verifies that all generated keys pass crypto_core_ed25519_is_valid_point() validation

3. Added newsfragment newsfragments/921.bugfix.rst:

   • Documents the bugfix for issue Interoperability of ed25519 keys #921
   • Follows proper ReST formatting with newline termination

Key Design Decision:

The PR intentionally does not modify from_bytes() (key loading) to maintain backward compatibility with existing nodes that may have technically invalid but previously functioning keys. This is a sensible approach that fixes the problem for new keys while avoiding breaking changes for existing deployments.

---

2. Branch Sync Status and Merge Conflicts

Branch Sync Status

Status: ✅ Ahead of origin/main

• Details: Branch is 0 commits behind and 5 commits ahead of origin/main
• Interpretation: The PR branch contains 5 commits that are not yet in main. This is expected for a feature branch and indicates the PR is ready for review.

Merge Conflict Analysis

Conflicts Detected: ✅ No conflicts

✅ No merge conflicts detected. The PR branch can be merged cleanly into origin/main.

The test merge completed successfully with no conflicting files or markers. The branch is ready for merge from a conflict perspective.

---

3. Strengths

1. Correct Problem Identification: The PR correctly identifies and addresses the root cause of interoperability issues with strict Ed25519 implementations.

2. Appropriate Use of Library Functions: The implementation uses crypto_core_ed25519_is_valid_point from nacl.bindings, which is the recommended approach suggested in issue Interoperability of ed25519 keys #921 comments. This function performs the same validation as libsodium's internal checks.

3. Backward Compatibility: The decision to only validate new key generation (not from_bytes()) maintains backward compatibility with existing deployments, which is crucial for a production library.

4. Comprehensive Testing: The new test covers both random and seeded key generation scenarios with multiple seed patterns, providing good probabilistic coverage.

5. Clear Error Messages: The error messages are descriptive and help developers understand what went wrong:

   • "Provided seed generates a public key that is not a valid Ed25519 curve point."
   • "Failed to generate a valid Ed25519 key after 100 attempts."

6. Reasonable Retry Logic: The 100-attempt limit for random key generation is reasonable. The probability of failing 100 times in a row is astronomically low (approximately 2^-100 for a 50% failure rate, which is much higher than the actual failure rate).

7. Proper Newsfragment: The newsfragment correctly references issue Interoperability of ed25519 keys #921, uses the .bugfix.rst type, and follows ReST formatting conventions.

8. Issue Reference: The PR properly references issue Interoperability of ed25519 keys #921 in both the description and the newsfragment filename.

---

4. Issues Found

Critical

None.

Major

None.

Minor

1. File: libp2p/crypto/ed25519.py
   Line(s): 78-87
   Issue: The retry loop for random key generation could theoretically fail after 100 attempts, though this is extremely unlikely.
   Suggestion: Consider adding a comment explaining why 100 attempts is sufficient, or document the statistical probability. However, this is a very minor concern given the astronomical unlikelihood of such a failure.

2. File: tests/core/crypto/test_ed25519_comprehensive.py
   Line(s): 277-283
   Issue: The test uses 50 iterations for random generation testing, which is reasonable but could be documented as a probabilistic test.
   Suggestion: The test is already well-documented in its docstring, so this is acceptable as-is.

---

5. Security Review

Security Impact: ✅ Positive - This PR improves security and interoperability.

Security Analysis:

1. Key Validation: The addition of curve point validation ensures that all generated keys are cryptographically valid, preventing potential security issues from invalid keys.

2. Interoperability: By ensuring keys are valid according to strict standards (ZIP-215), the PR prevents interoperability failures that could lead to:

   • Connection failures between Python and Rust nodes
   • Potential security vulnerabilities from inconsistent key validation
   • Network partitioning issues

3. No Security Risks Introduced:

   • The retry mechanism does not introduce timing attacks (the number of retries is not exposed)
   • The validation function is from a trusted cryptographic library (PyNaCl/libsodium)
   • Error messages do not leak sensitive information

4. Backward Compatibility Consideration: The decision to not validate keys loaded via from_bytes() maintains backward compatibility but could theoretically allow invalid keys to persist. However, this is a reasonable trade-off for maintaining existing functionality.

Overall Security Assessment: This PR improves security posture by ensuring all newly generated keys meet strict cryptographic standards, enhancing interoperability and preventing potential security issues from invalid keys.

---

6. Documentation and Examples

Documentation Quality:

1. Code Documentation:

   • The module docstring in ed25519.py is clear and explains the purpose
   • The test docstring clearly explains what is being tested and why

2. PR Description:

   • The PR description is comprehensive and explains both the problem and the solution
   • It clearly documents the design decision to not modify from_bytes()

3. Newsfragment:

   • The newsfragment is user-facing and describes the fix in terms of user impact
   • Properly formatted and ends with a newline

Suggestions:

• The code itself is self-documenting, but a brief comment in the retry loop explaining the statistical unlikelihood of failure could be helpful for future maintainers.

---

7. Newsfragment Requirement

✅ Newsfragment is present and correctly formatted.

Verification:

• File: newsfragments/921.bugfix.rst ✅ Present
• Filename Format: 921.bugfix.rst ✅ Correct (issue number + type)
• Type: .bugfix.rst ✅ Appropriate for a bug fix
• Content: User-facing description ✅ Present
• Formatting: ReST format with newline ✅ Correct
• Issue Reference: PR references issue Interoperability of ed25519 keys #921 ✅ Present

Issue Requirement:

✅ PR properly references issue #921 in:

• PR description: "Issue Interoperability of ed25519 keys #921"
• Newsfragment filename: 921.bugfix.rst
• PR body explains the issue clearly

Status: All newsfragment requirements are met. No blockers.

---

8. Tests and Validation

Linting (make lint)

✅ All linting checks passed.

Output Summary:

• All pre-commit hooks passed:
  • YAML check ✅
  • TOML check ✅
  • End of files ✅
  • Trailing whitespace ✅
  • pyupgrade ✅
  • ruff (legacy alias) ✅
  • ruff format ✅
  • mdformat ✅
  • mypy ✅
  • pyrefly ✅
  • RST files check ✅

Exit Code: 0 (Success)

Issues Found: None. All code quality checks passed.

---

Type Checking (make typecheck)

✅ All type checking passed.

Output Summary:

• mypy with all dev dependencies: ✅ Passed
• pyrefly typecheck locally: ✅ Passed

Exit Code: 0 (Success)

Issues Found: None. All type annotations are correct and type checking passes.

---

Test Execution (make test)

✅ All tests passed.

Test Summary:

• Total Tests: 1920 items
• Passed: 1905 ✅
• Failed: 0
• Skipped: 15 (expected skips)
• Errored: 0
• Warnings: 25 (deprecation and resource warnings, not related to this PR)

Execution Time: 92.94s (1:32)

Key Test Results:

• test_generated_keys_are_valid_curve_points ✅ Passed
• All existing Ed25519 tests ✅ Passed
• All other test suites ✅ Passed

Exit Code: 0 (Success)

Issues Found: None. The new test passes and all existing tests continue to pass, indicating no regressions.

---

Documentation Build (make linux-docs)

✅ Documentation build succeeded.

Output Summary:

• Sphinx build: ✅ Success
• All 81 source files processed
• HTML generation: ✅ Success
• Doctest execution: ✅ 109 tests, 0 failures
• Newsfragment validation: ✅ Passed
• Towncrier draft build: ✅ Success (shows issue Interoperability of ed25519 keys #921 in release notes)

Exit Code: 0 (Success)

Issues Found: None. Documentation builds successfully and the newsfragment is properly integrated into the release notes.

---

9. Recommendations for Improvement

1. Add Comment to Retry Logic (Optional):

   • Consider adding a brief comment explaining why 100 attempts is sufficient, referencing the statistical unlikelihood of failure. This would help future maintainers understand the design decision.

2. Consider Logging (Optional):

   • If retries are needed (which is extremely rare), consider adding debug-level logging to track when retries occur. This could help with monitoring and debugging in production environments.

3. Documentation Enhancement (Optional):

   • The PR description and code are already clear, but a brief note in the module docstring about ZIP-215 compliance could be helpful for users who need to understand interoperability requirements.

Note: All of these are minor, optional improvements. The PR is already in excellent shape.

---

10. Questions for the Author

1. Retry Limit Justification: Was the 100-attempt limit chosen based on statistical analysis, or is it a conservative upper bound? (This is more for documentation purposes than a concern.)

2. Performance Impact: Have you measured the performance impact of the validation check? While it should be negligible, it would be good to confirm.

3. Interoperability Testing: Have you tested this fix against actual Rust libp2p nodes to confirm interoperability? (The issue description suggests this was the original problem.)

4. Future Consideration: Is there a plan to eventually validate keys loaded via from_bytes() in a future version, or will backward compatibility always take precedence?

---

11. Overall Assessment

Quality Rating: Excellent ✅

This PR demonstrates:

• Clear understanding of the problem
• Appropriate use of cryptographic libraries
• Good testing coverage
• Proper documentation
• Backward compatibility considerations
• Clean code that passes all quality checks

Security Impact: Positive ✅

The PR improves security by ensuring all newly generated keys meet strict cryptographic standards, enhancing interoperability and preventing potential security issues.

Merge Readiness: Ready ✅

• ✅ All tests pass
• ✅ All linting checks pass
• ✅ All type checks pass
• ✅ Documentation builds successfully
• ✅ No merge conflicts
• ✅ Proper newsfragment present
• ✅ Issue properly referenced
• ✅ Code quality is excellent

Confidence: High ✅

The implementation is sound, well-tested, and addresses the reported issue effectively. The code follows best practices and maintains backward compatibility while fixing the interoperability problem.

---

Final Recommendation

✅ APPROVE - This PR is ready to merge.

The fix correctly addresses the interoperability issue described in #921, uses the recommended validation approach from the issue comments, maintains backward compatibility, and includes comprehensive tests. All quality checks pass, and the code is well-written and documented.

Suggested Action: Merge after addressing any optional improvements if desired, but the PR is already in excellent shape and ready for production.

---

Review Completed: 2026-01-27
Reviewer: AI Assistant (following py-libp2p PR Review Prompt guidelines)

[acul71]

Great. I don't like 100 hard coded instead of a constant config var, but it's ok. Thank you!