Skip to content

Conversation

@Smartdevs17
Copy link

Related Issue: #312

py-libp2p generated a new random peer identity on every restart.
As a result, nodes could not maintain a stable Peer ID across sessions, which made long-lived peers, reputation, and reproducible networking setups difficult.


How was it fixed?

This PR introduces opt-in peer identity persistence, allowing users to explicitly control how a peer identity is created and reused, while keeping the default behavior unchanged.

By default, py-libp2p still generates a new random identity on each host creation.
Users who need a stable Peer ID can now provide or persist their identity explicitly.


Usage Options

Option 1: Provide Your Own KeyPair

You can directly supply a key_pair when creating a host.
As long as the same key pair is reused, the Peer ID will remain stable.

from libp2p import new_host
from libp2p.crypto.ed25519 import create_new_key_pair

# Create or load your keypair
key_pair = create_new_key_pair()

# Create a host with a stable Peer ID
host = new_host(key_pair=key_pair)
print(f"Peer ID: {host.get_id()}")

Option 2: Save and Load Identity from Disk

Helper utilities are provided to persist and reload peer identities across restarts.

from libp2p import new_host
from libp2p.identity_utils import save_identity, load_identity, identity_exists
from libp2p.crypto.ed25519 import create_new_key_pair

identity_file = "my_peer_identity.key"

# First run: create and save identity
if not identity_exists(identity_file):
    key_pair = create_new_key_pair()
    save_identity(key_pair, identity_file)
    print("Created new identity")
else:
    # Subsequent runs: load existing identity
    key_pair = load_identity(identity_file)
    print("Loaded existing identity")

# Create host with persistent identity
host = new_host(key_pair=key_pair)
print(f"Peer ID: {host.get_id()}")  # Same ID every time

Option 3: Deterministic Identity from Seed

For reproducible setups (e.g. testing), a deterministic identity can be derived from a 32-byte seed.

from libp2p import new_host
from libp2p.identity_utils import create_identity_from_seed
import hashlib

# Derive a 32-byte seed from any string
seed = hashlib.sha256(b"my-unique-seed-phrase").digest()

# Same seed always produces the same identity
key_pair = create_identity_from_seed(seed)
host = new_host(key_pair=key_pair)
print(f"Peer ID: {host.get_id()}")  # Always the same for this seed

Security Notes

  • Identity files contain private keys and must be kept secure.
  • The save_identity() helper sets restrictive file permissions (0600) where supported.
  • Deterministic identities should only be used when appropriate (e.g. testing or controlled environments).

Backward Compatibility

  • Default behavior remains unchanged.
  • Users who do not explicitly provide an identity will still receive a randomly generated Peer ID on each run.

Testing

  • Added tests to ensure:

    • Reusing the same identity results in the same Peer ID
    • Default behavior still produces different Peer IDs across restarts

- Add docstring to generate_peer_id_from() explaining deterministic peer ID generation
- Add inline comments in new_swarm() explaining identity persistence options
- Add comprehensive docstring to ID.from_pubkey() with examples
- Document that same keypair always produces same peer ID

This commit adds documentation only - no functional changes.
Relates to libp2p#312
- Add identity_utils.py module with helper functions
- save_identity(): Save keypair to disk with secure permissions
- load_identity(): Load keypair from disk
- create_identity_from_seed(): Generate deterministic identity from 32-byte seed
- identity_exists(): Check if identity file exists
- Export helper functions from main libp2p module

These utilities enable opt-in identity persistence without changing
default behavior. Users can now easily save/load identities or create
deterministic identities from seeds.

Relates to libp2p#312
- test_same_keypair_produces_same_peer_id: Verify deterministic peer ID generation
- test_no_keypair_produces_different_peer_ids: Verify default random behavior
- test_identity_from_seed_is_deterministic: Verify seed-based identity
- test_different_seeds_produce_different_identities: Verify seed uniqueness
- test_seed_must_be_32_bytes: Verify seed validation
- test_save_and_load_identity: Verify file persistence
- test_save/load_identity_with_string_path: Verify path type flexibility
- test_load_nonexistent_identity_raises_error: Verify error handling
- test_identity_exists: Verify file existence check
- test_host_with_saved_identity: Integration test for saved identity
- test_host_with_seed_identity: Integration test for seed-based identity
- test_default_host_generates_random_identity: Verify backward compatibility

All 14 tests pass successfully.

Relates to libp2p#312
@Smartdevs17
Copy link
Author

@seetadev Sir I have raised this PR.

- Apply ruff formatting fixes (trailing whitespace, etc.)
- Fix long line in identity_utils.py (E501)
- Ensure codebase passes ruff check
@Smartdevs17 Smartdevs17 force-pushed the feat/persist-peer-identity-312 branch from 7ba2584 to da05feb Compare February 6, 2026 07:49
@Smartdevs17
Copy link
Author

@seetadev please can you look at this pr. Thank you.

@seetadev
Copy link
Contributor

seetadev commented Feb 7, 2026

@Smartdevs17 : Thanks so much for sharing—really appreciate it.

CC’ing @lla-dane, @acul71, @yashksaini-coder and @Winter-Soren, who’ve worked closely with me in this area and can help with a peer review.

Please feel free to reach out to them on Discord.

@acul71
Copy link
Contributor

acul71 commented Feb 8, 2026

Hello @Smartdevs17 thanks for this PR.
good intuitions but this PR need work. See detailed review here:

AI PR Review — PR #1185: Feat/persist peer identity 312

Reviewer: AI (claude-4.6-opus)
Date: 2026-02-08
PR: #1185
Author: @Smartdevs17
Branch: feat/persist-peer-identity-312main


1. Summary of Changes

Type: New Feature

Related Issue: #312 — Provide utilities to persist network identities between runs of a node

Problem: py-libp2p generated a new random peer identity (keypair) on every restart, making it impossible to maintain a stable Peer ID across sessions. This affected long-lived peers, reputation systems, and reproducible networking setups.

Solution: This PR introduces opt-in peer identity persistence through a new libp2p/identity_utils.py module providing:

  • save_identity() — Save a keypair to disk as raw bytes with 0o600 permissions
  • load_identity() — Load a keypair from disk (Ed25519 only)
  • create_identity_from_seed() — Deterministic keypair generation from a 32-byte seed
  • identity_exists() — Check if an identity file exists at a given path

Files Changed (4):

File Status Description
libp2p/__init__.py Modified Imports new identity utilities, adds docstrings and comments
libp2p/identity_utils.py Added New module with save/load/seed identity helpers
libp2p/peer/id.py Modified Added docstring to ID.from_pubkey() with doctest examples
tests/core/test_identity_persistence.py Added 14 tests covering the new functionality

Breaking Changes: None. Default behavior is unchanged.


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.
  • The branch is fully up to date with main.

Merge Conflict Analysis

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

3. Strengths

  1. Clean, opt-in design: The default behavior is completely unchanged. Users who want identity persistence must explicitly opt in, which is the right approach.

  2. Good test coverage: 14 tests covering save/load, seed-based generation, path types (string and Path), error cases (invalid seed length, nonexistent file), and integration tests with new_host().

  3. Clear PR description: The PR body includes excellent documentation with three usage options and code examples. Security notes and backward compatibility are addressed.

  4. Proper seed validation: The create_identity_from_seed() function validates the seed is exactly 32 bytes with a helpful error message suggesting hashlib.sha256().

  5. File permission handling: The save_identity() function sets 0o600 permissions with a graceful fallback for filesystems that don't support chmod.

  6. Good docstrings: All new functions have complete docstrings with Args, Returns, Raises, and Example sections.


4. Issues Found

Critical

4.1 Doctest Failure Breaks Documentation Build

  • File: libp2p/peer/id.py

  • Line(s): 101–110

  • Issue: The docstring example in ID.from_pubkey() references ID without importing it in the doctest namespace, causing 2 doctest failures:

    NameError: name 'ID' is not defined. Did you mean: 'id'?
    

    The make linux-docs command fails with exit code 2 because of this.

  • Suggestion: Either add >>> from libp2p.peer.id import ID before the ID.from_pubkey(...) lines in the doctest, or use the .. doctest:: directive with :options: +SKIP to skip the example. The simplest fix:

    Example:
        >>> from libp2p.crypto.ed25519 import create_new_key_pair
        >>> from libp2p.peer.id import ID
        >>> kp1 = create_new_key_pair()
        >>> kp2 = create_new_key_pair()
        >>> # Same keypair produces same peer ID
        >>> ID.from_pubkey(kp1.public_key) == ID.from_pubkey(kp1.public_key)
        True
        >>> # Different keypairs produce different peer IDs
        >>> ID.from_pubkey(kp1.public_key) == ID.from_pubkey(kp2.public_key)
        False

4.2 Trailing Whitespace in Docstrings

  • File: libp2p/__init__.py
  • Line(s): 274, 278 (in generate_peer_id_from docstring)
  • Issue: The make lint first pass found trailing whitespace in the docstring added to generate_peer_id_from(). Pre-commit auto-fixed it, but this means the PR as submitted would fail CI linting.
  • Suggestion: The author should run pre-commit run --all-files before pushing to ensure the PR passes lint on the first try.

Major

4.3 Must Use Existing Protobuf Serialization Instead of Raw Bytes

  • File: libp2p/identity_utils.py (new)

  • Issue: The PR serializes private keys as raw bytes (key_pair.private_key.to_bytes()) with no type tagging. However, py-libp2p already has the exact same protobuf-based serialization infrastructure that go-libp2p uses — and this PR completely ignores it.

    go-libp2p's approach (core/crypto/key.go):

    MarshalPrivateKey wraps {KeyType, raw_bytes} in a protobuf message:

    // MarshalPrivateKey converts a key object into its protobuf serialized form.
    func MarshalPrivateKey(k PrivKey) ([]byte, error) {
        data, err := k.Raw()
        if err != nil {
            return nil, err
        }
        return proto.Marshal(&pb.PrivateKey{
            Type: k.Type().Enum(),
            Data: data,
        })
    }

    UnmarshalPrivateKey reads the type tag and dispatches to the correct deserializer:

    func UnmarshalPrivateKey(data []byte) (PrivKey, error) {
        pmes := new(pb.PrivateKey)
        err := proto.Unmarshal(data, pmes)
        if err != nil {
            return nil, err
        }
        um, ok := PrivKeyUnmarshallers[pmes.GetType()]
        if !ok {
            return nil, ErrBadKeyType
        }
        return um(pmes.GetData())
    }

    The protobuf schema (core/crypto/pb/crypto.proto):

    enum KeyType {
      RSA = 0;
      Ed25519 = 1;
      Secp256k1 = 2;
      ECDSA = 3;
    }
    
    message PrivateKey {
      required KeyType Type = 1;
      required bytes Data = 2;
    }

    py-libp2p already has the identical infrastructure:

    The same protobuf schema exists at libp2p/crypto/pb/crypto.proto:

    enum KeyType {
      RSA = 0;
      Ed25519 = 1;
      Secp256k1 = 2;
      ECDSA = 3;
      ECC_P256 = 4;
      X25519 = 5;
    }
    
    message PrivateKey {
      required KeyType key_type = 1;
      required bytes data = 2;
    }

    The PrivateKey base class already has serialize() — equivalent to Go's MarshalPrivateKey:

    # libp2p/crypto/keys.py lines 89-98
    def _serialize_to_protobuf(self) -> crypto_pb2.PrivateKey:
        """Return the protobuf representation of this Key."""
        key_type = self.get_type().value
        data = self.to_bytes()
        protobuf_key = crypto_pb2.PrivateKey(key_type=key_type, data=data)
        return protobuf_key
    
    def serialize(self) -> bytes:
        """Return the canonical serialization of this Key."""
        return self._serialize_to_protobuf().SerializeToString()

    And serialization.py has deserialize_private_key() — equivalent to Go's UnmarshalPrivateKey:

    # libp2p/crypto/serialization.py lines 44-52
    def deserialize_private_key(data: bytes) -> PrivateKey:
        f = PrivateKey.deserialize_from_protobuf(data)
        try:
            deserializer = key_type_to_private_key_deserializer[f.key_type]
        except KeyError as e:
            raise MissingDeserializerError(
                {"key_type": f.key_type, "key": "private_key"}
            ) from e
        return deserializer(f.data)

    This creates three problems with the current PR:

    1. No key type tagging — raw bytes cannot identify the key type on load
    2. Hardcoded Ed25519load_identity() always calls Ed25519PrivateKey.from_bytes(), silently failing for other key types
    3. Duplication — the existing save_keypair()/load_keypair() in libp2p/__init__.py (PEM format) is a third, incompatible persistence path
  • Suggestion: Replace raw byte serialization with the existing protobuf infrastructure. The fix is minimal — change two lines in identity_utils.py:

    # save_identity — use protobuf serialization (type-tagged, like Go)
    def save_identity(key_pair: KeyPair, filepath: str | Path) -> None:
        filepath = Path(filepath)
        data = key_pair.private_key.serialize()  # protobuf with KeyType tag
        fd = os.open(str(filepath), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
        try:
            os.write(fd, data)
        finally:
            os.close(fd)
    
    # load_identity — auto-detect key type from protobuf tag
    def load_identity(filepath: str | Path) -> KeyPair:
        filepath = Path(filepath)
        data = filepath.read_bytes()
        from libp2p.crypto.serialization import deserialize_private_key
        private_key = deserialize_private_key(data)  # handles any key type
        public_key = private_key.get_public_key()
        return KeyPair(private_key, public_key)

    This approach:

    • Supports all key types (Ed25519, RSA, Secp256k1, ECDSA) automatically
    • Uses the canonical libp2p serialization format (same as Go)
    • Sets file permissions atomically (no TOCTOU race)
    • Eliminates the need for the existing save_keypair/load_keypair functions

4.5 save_identity Has a TOCTOU Race on File Permissions

  • File: libp2p/identity_utils.py

  • Line(s): 49–62

  • Issue: The file is first written with filepath.write_bytes() (using default permissions, typically 0o644 — world-readable), then permissions are changed to 0o600. Between the write and the chmod, the private key is briefly readable by other users on the system.

  • Suggestion: Use os.open() with os.O_WRONLY | os.O_CREAT | os.O_TRUNC and mode 0o600, then os.fdopen() and write to it. This atomically creates the file with restrictive permissions:

    import os
    fd = os.open(str(filepath), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
    try:
        os.write(fd, private_key_bytes)
    finally:
        os.close(fd)

4.6 Missing Relationship to Issue #312's "Key Pair Provider" Design

  • File: Overall design

  • Issue: The original issue Provide utilities to persist network identities between runs of a node #312 by @ralexstokes proposed a "key pair provider" pattern with type Callable[[], KeyPair] for maximum flexibility. A maintainer (@mhchia) also suggested building on KeyStore/PeerStore abstractions (similar to Go's FSKeystore). The current PR takes a simpler utility-function approach instead.

    While the simpler approach is pragmatic and works, it doesn't address the broader design questions raised in the issue (multiple key management, keystore abstraction, auto-save).

  • Suggestion: This is acceptable as a first step, but the PR description or a follow-up issue should acknowledge the remaining design work from Provide utilities to persist network identities between runs of a node #312 that this doesn't cover (keystore abstraction, multi-key management).

Minor

4.7 identity_exists Is a Trivial Wrapper

  • File: libp2p/identity_utils.py
  • Line(s): 138–156
  • Issue: identity_exists() is just return Path(filepath).exists(). It provides no additional validation (e.g., checking the file is readable, has valid key data, or is the correct size).
  • Suggestion: Either:
    1. Add basic validation (e.g., check file size is 32 or 64 bytes for Ed25519), or
    2. Consider removing it and documenting that users can use Path.exists() directly.

4.8 Module-Level Docstring Examples Will Fail as Doctests

  • File: libp2p/identity_utils.py
  • Line(s): 8–17
  • Issue: The module docstring contains >>> doctest examples for save_identity and load_identity, but these would fail if run as doctests because they create files in the current directory without cleanup.
  • Suggestion: Use .. code-block:: python instead of >>> doctest format, or use tempfile in the examples.

4.9 Inconsistent Docstring Style

  • File: libp2p/identity_utils.py
  • Issue: The new module uses Google-style docstrings (Args:, Returns:, Raises:), while the rest of the py-libp2p codebase predominantly uses ReST/Sphinx style (:param:, :return:, :raises:). The docstrings in libp2p/__init__.py and libp2p/peer/id.py modified by this same PR use :param: style.
  • Suggestion: Use :param: / :return: / :raises: style consistently with the rest of the codebase.

4.10 Overly Verbose Inline Comments

  • File: libp2p/__init__.py
  • Line(s): 340–356
  • Issue: The added block comments in new_swarm() explaining the "Identity Generation Flow" (7 lines of comments) are overly verbose for well-named, straightforward code. The if key_pair is None: key_pair = generate_new_ed25519_identity() pattern is self-explanatory.
  • Suggestion: Reduce to a brief one-line comment or remove entirely, relying on the function docstrings instead.

5. Security Review

5.1 Private Key Briefly World-Readable (TOCTOU)

  • Risk: Private key file is created with default permissions (~0o644) before being chmod'd to 0o600.
  • Impact: Low (requires a local attacker with timing, but defense-in-depth matters for key material)
  • Mitigation: Use os.open() with explicit mode to create the file with restricted permissions atomically. (See Issue 4.5)

5.2 Raw Private Key Storage Without Encryption

  • Risk: The save_identity() function saves private keys as unencrypted raw bytes. Anyone with read access to the file gets the full private key.
  • Impact: Medium (standard for libp2p implementations — go-libp2p also stores unencrypted — but worth noting)
  • Mitigation: Use protobuf serialization for this PR (see Issue 4.3). In a follow-up PR, add optional password-based encryption using the cryptography library (already a py-libp2p dependency). See Appendix A below for a complete design proposal.

5.3 No Key Type Tagging in Serialized Format

  • Risk: The saved file contains raw bytes with no header indicating the key type. If the library later adds support for other key types (e.g., secp256k1), loading a mismatched key type could produce incorrect identity without obvious failure.
  • Impact: Low (currently only Ed25519 is supported, but becomes an issue with multi-key support)
  • Mitigation: Use the existing protobuf serialization (see Issue 4.3) which includes the KeyType enum — this resolves the issue completely.

6. Documentation and Examples

Positives

  • The PR description is well-written with clear usage examples for all three options.
  • All new functions have complete docstrings.
  • The ID.from_pubkey() docstring addition is valuable for explaining the deterministic nature of peer IDs.

Issues

  • Doctest failure: The example in ID.from_pubkey() fails the Sphinx doctest build (see Issue 4.1). This is a blocker for CI.
  • No README update: The PR adds significant new user-facing functionality but doesn't update the README or any documentation files to mention identity persistence.
  • No getting_started or tutorial update: Identity persistence is a commonly needed feature; it would benefit from a section in the getting started guide.
  • Inconsistent docstring style: Google-style in identity_utils.py vs. ReST-style in the rest of the codebase (see Issue 4.9).

7. Newsfragment Requirement

  • Severity: CRITICAL / BLOCKER
  • Issue: No newsfragment file exists for issue Provide utilities to persist network identities between runs of a node #312.
  • Impact: PR cannot be approved without a valid newsfragment file. This is a mandatory requirement.
  • Suggestion:
    • Add newsfragments/312.feature.rst with content such as:
      Added opt-in peer identity persistence utilities (``save_identity``, ``load_identity``, ``create_identity_from_seed``) allowing nodes to maintain a stable Peer ID across restarts.
    • The file must end with a newline character.
  • Action Required: PR approval is blocked until a newsfragment file is added.

8. Tests and Validation

Test Results

Check Result Notes
Lint (make lint) ⚠️ Pass (after auto-fix) Trailing whitespace in libp2p/__init__.py docstring was auto-fixed by pre-commit hooks. First pass failed.
Type Check (make typecheck) ✅ Pass mypy and pyrefly both passed cleanly.
Tests (make test) ✅ Pass 1933 passed, 16 skipped, 25 warnings in 92.84s
Docs (make linux-docs) FAIL Doctest failures in libp2p.peer.rst — 2 failures due to missing ID import in doctest example. Exit code 2.

Test Coverage Analysis

The 14 new tests in tests/core/test_identity_persistence.py cover:

  • ✅ Same keypair → same peer ID (determinism)
  • ✅ Different keypairs → different peer IDs (uniqueness)
  • ✅ Seed-based identity determinism
  • ✅ Different seeds → different identities
  • ✅ Seed length validation (too short, too long)
  • ✅ Save and load round-trip
  • ✅ String and Path type acceptance
  • ✅ Nonexistent file raises FileNotFoundError
  • identity_exists() helper
  • ✅ Integration with new_host() (saved identity, seed identity, default random)

Missing Test Cases

  • ❌ No test for saving a non-Ed25519 key (e.g., RSA) — should verify it either works or raises a clear error
  • ❌ No test for loading a corrupt/invalid key file
  • ❌ No test for loading a file with wrong size (e.g., 0 bytes, 100 bytes)
  • ❌ No test for file permission verification (that chmod 0o600 is applied)
  • ❌ No test for overwriting an existing identity file

Warnings (from test output)

25 warnings were reported during the test run. These are pre-existing warnings unrelated to this PR (primarily trio and pytest-asyncio deprecation warnings).


9. Recommendations for Improvement

For This PR (Required)

  1. Fix the doctest failure (BLOCKER): Add from libp2p.peer.id import ID to the doctest example in ID.from_pubkey().

  2. Add the newsfragment (BLOCKER): Create newsfragments/312.feature.rst with a user-facing description.

  3. Use protobuf serialization (CRITICAL): Replace raw byte serialization with PrivateKey.serialize() / deserialize_private_key() — the existing py-libp2p infrastructure that matches go-libp2p's MarshalPrivateKey/UnmarshalPrivateKey. See Issue 4.3 for the exact code changes. This resolves Issues 4.3, 4.4, 5.2, and 5.3 in one change.

  4. Fix the TOCTOU file permission issue: Use os.open() with mode 0o600 instead of write-then-chmod.

  5. Fix trailing whitespace: Run pre-commit run --all-files before submitting.

  6. Standardize docstring style: Use :param: / :return: / :raises: format to match the codebase convention.

  7. Add missing test cases: Corrupt files, wrong key types (now testable with protobuf), permission checks, overwrite behavior.

For a Follow-Up PR (Recommended)

  1. Add optional password-based encryption for key files. See Appendix A for a complete design proposal with code, including a password_provider: Callable pattern for flexible secret management across deployment contexts (interactive, environment variable, OS keyring, cloud KMS).

  2. Consolidate with existing save_keypair/load_keypair: Once protobuf serialization is in place, deprecate or remove the PEM-based save_keypair()/load_keypair() in libp2p/__init__.py to avoid having two incompatible persistence paths.

  3. Acknowledge remaining Provide utilities to persist network identities between runs of a node #312 design work: The "key pair provider" pattern (Callable[[], KeyPair]) and KeyStore/PeerStore abstraction from Provide utilities to persist network identities between runs of a node #312 are not yet implemented. Open a follow-up issue to track this.


10. Questions for the Author

  1. Why not use the existing protobuf serialization? py-libp2p already has PrivateKey.serialize() and deserialize_private_key() in libp2p/crypto/keys.py and libp2p/crypto/serialization.py — the exact equivalent of go-libp2p's MarshalPrivateKey/UnmarshalPrivateKey. These handle key type tagging and support all key types automatically. Why was raw byte serialization chosen instead?

  2. Relationship to existing save_keypair/load_keypair: The codebase already has save_keypair() and load_keypair() in libp2p/__init__.py that use PEM format. How do you envision these coexisting with the new save_identity()/load_identity() which use a third, incompatible format (raw bytes)? Should one approach be consolidated?

  3. Key pair provider pattern: The original issue Provide utilities to persist network identities between runs of a node #312 proposed a Callable[[], KeyPair] provider pattern for maximum flexibility. Was this considered? The utility-function approach works but is less composable.

  4. Why not build on PeerStore/KeyStore? A maintainer suggested in Provide utilities to persist network identities between runs of a node #312 building on KeyStore/PeerStore abstractions (similar to Go's datastore-backed keybook at p2p/host/peerstore/pstoreds/keybook.go). Was this approach considered and rejected for simplicity?


11. Overall Assessment

Criterion Rating
Quality Rating Needs Work
Security Impact Low
Merge Readiness Needs fixes
Confidence High

Summary

The PR addresses a genuine need (issue #312) with a clean, opt-in design that preserves backward compatibility. The test coverage is good and the code is well-documented. However, there are two blockers that must be resolved before merge:

  1. Missing newsfragment (newsfragments/312.feature.rst)
  2. Doctest failure breaking the documentation build

Additionally, the PR introduces functional duplication with the existing save_keypair/load_keypair functions and has a minor file permission TOCTOU issue that should be addressed given we're handling private key material.

The PR is a solid first step toward identity persistence, but needs the blockers fixed and the duplication question addressed before it can be merged.


Appendix A: Follow-Up PR Design — Password-Encrypted Key Files with Provider Pattern

Status: Recommended for a follow-up PR after the protobuf serialization fix lands.
Neither go-libp2p nor py-libp2p currently support password-encrypted key files.
This would make py-libp2p the first libp2p implementation with built-in key encryption.

Encrypted File Format

┌──────────────────────────────────────────────────────┐
│  Encrypted Key File Format                           │
├──────────────────────────────────────────────────────┤
│  magic: b"LIBP2P-KEY\x01"  (11 bytes, version 1)    │
│  salt:  16 random bytes     (for scrypt KDF)         │
│  nonce: 12 bytes            (for AES-256-GCM)        │
│  ciphertext: AES-256-GCM(                            │
│      key = scrypt(password, salt),                   │
│      plaintext = protobuf_serialized_key             │
│  )                                                   │
└──────────────────────────────────────────────────────┘

Unencrypted files remain plain protobuf (no magic header), so load_identity() auto-detects the format.

Implementation

Using the cryptography library (already a py-libp2p dependency):

import os
import getpass
from pathlib import Path
from typing import Callable

from cryptography.hazmat.primitives.ciphers.aead import AESGCM
from cryptography.hazmat.primitives.kdf.scrypt import Scrypt

from libp2p.crypto.keys import KeyPair
from libp2p.crypto.serialization import deserialize_private_key

_MAGIC = b"LIBP2P-KEY\x01"


def save_identity(
    key_pair: KeyPair,
    filepath: str | Path,
    password: str | None = None,
) -> None:
    """
    Save a keypair to disk using protobuf serialization.

    If a password is provided, the key is encrypted with AES-256-GCM
    using a scrypt-derived key. Otherwise, the key is saved as plain
    protobuf (matching go-libp2p's MarshalPrivateKey format).

    :param key_pair: The KeyPair to save
    :param filepath: Path where the key will be saved
    :param password: Optional password for encryption
    """
    filepath = Path(filepath)

    # Serialize using protobuf (type-tagged, like Go)
    data = key_pair.private_key.serialize()

    if password is not None and password != "":
        salt = os.urandom(16)
        nonce = os.urandom(12)
        kdf = Scrypt(salt=salt, length=32, n=2**18, r=8, p=1)
        aes_key = kdf.derive(password.encode("utf-8"))
        ciphertext = AESGCM(aes_key).encrypt(nonce, data, None)
        data = _MAGIC + salt + nonce + ciphertext

    # Atomic file creation with restrictive permissions
    fd = os.open(str(filepath), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
    try:
        os.write(fd, data)
    finally:
        os.close(fd)


def load_identity(
    filepath: str | Path,
    password: str | None = None,
    password_provider: Callable[[], str] | None = None,
) -> KeyPair:
    """
    Load a keypair from disk.

    Auto-detects encrypted vs. plain protobuf format. If encrypted,
    resolves the password from (in order):
      1. Explicit ``password`` parameter
      2. ``password_provider()`` callback
      3. ``LIBP2P_KEY_PASSPHRASE`` environment variable
      4. Interactive prompt via ``getpass``

    :param filepath: Path to the saved key file
    :param password: Optional password for decryption
    :param password_provider: Optional callback that returns the password
    :return: KeyPair loaded from the file
    :raises FileNotFoundError: If the file doesn't exist
    :raises ValueError: If the file is encrypted and no password is available,
                        or if the password is incorrect
    """
    filepath = Path(filepath)
    data = filepath.read_bytes()

    # Auto-detect encrypted format
    if data.startswith(_MAGIC):
        # Resolve password from tiered sources
        if password is None and password_provider is not None:
            password = password_provider()
        if password is None:
            password = os.environ.get("LIBP2P_KEY_PASSPHRASE")
        if password is None:
            password = getpass.getpass(
                f"Enter passphrase for {filepath.name}: "
            )
        if password is None or password == "":
            raise ValueError(
                "Key file is encrypted but no password was provided"
            )

        salt = data[11:27]
        nonce = data[27:39]
        ciphertext = data[39:]

        kdf = Scrypt(salt=salt, length=32, n=2**18, r=8, p=1)
        aes_key = kdf.derive(password.encode("utf-8"))

        try:
            data = AESGCM(aes_key).decrypt(nonce, ciphertext, None)
        except Exception as e:
            raise ValueError("Incorrect password or corrupted key file") from e

    # Deserialize protobuf (auto-detects key type)
    private_key = deserialize_private_key(data)
    public_key = private_key.get_public_key()
    return KeyPair(private_key, public_key)

Password Provider Pattern — Usage Examples

The password_provider: Callable[[], str] parameter enables flexible secret management across all deployment contexts:

# 1. Interactive desktop application — prompt the user
key_pair = load_identity("peer.key")
# → Falls through to getpass.getpass() automatically

# 2. Server with environment variable
#    LIBP2P_KEY_PASSPHRASE=mysecret python my_node.py
key_pair = load_identity("peer.key")
# → Reads from $LIBP2P_KEY_PASSPHRASE automatically

# 3. Kubernetes / Docker — mounted secret file
key_pair = load_identity(
    "peer.key",
    password_provider=lambda: Path("/run/secrets/libp2p_pw").read_text().strip()
)

# 4. OS Keyring (macOS Keychain, GNOME Keyring, Windows Credential Manager)
import keyring
key_pair = load_identity(
    "peer.key",
    password_provider=lambda: keyring.get_password("py-libp2p", "peer.key")
)

# 5. HashiCorp Vault / AWS Secrets Manager
key_pair = load_identity(
    "peer.key",
    password_provider=lambda: vault_client.read("secret/libp2p")["password"]
)

# 6. GUI application
key_pair = load_identity(
    "peer.key",
    password_provider=lambda: gui_password_dialog("Enter key passphrase")
)

# 7. No encryption (default, matches current go-libp2p behavior)
save_identity(key_pair, "peer.key")  # no password → plain protobuf
key_pair = load_identity("peer.key")  # loads directly, no prompt

Password Resolution Order

┌─────────────────────────────────────────────────────────┐
│  load_identity("peer.key", ...) called                  │
└───────────────┬─────────────────────────────────────────┘
                │
                ▼
  ┌──── Is the file encrypted? (check magic bytes) ────┐
  │                                                     │
  No                                                   Yes
  │                                                     │
  ▼                                                     ▼
  Deserialize protobuf              ┌── Resolve password ──┐
  directly (any key type)           │                      │
                                    │  1. password= param  │
                                    │  2. password_provider │
                                    │  3. $LIBP2P_KEY_...  │
                                    │  4. getpass() prompt  │
                                    │                      │
                                    └──────────┬───────────┘
                                               │
                                               ▼
                                    Decrypt → Deserialize protobuf

Why This Design

  1. Library provides the mechanism, application chooses the policy. The password_provider callback lets each deployment context plug in its own secret source without the library needing to know about Vault, Kubernetes, keyrings, etc.

  2. Backward compatible. Unencrypted files (plain protobuf) load without any password interaction. The encryption is fully opt-in.

  3. Auto-detection. The magic header (LIBP2P-KEY\x01) distinguishes encrypted files from plain protobuf, so a single load_identity() call handles both.

  4. Graceful fallback chain. Servers use env vars, desktops get an interactive prompt, containers use mounted secrets — all through the same API.

  5. py-libp2p would lead. go-libp2p relies solely on file permissions (0400) with no encryption support. This would make py-libp2p the first libp2p implementation with built-in password-based key encryption.

@yashksaini-coder
Copy link
Contributor

Few differences that I found during analysis, which I have resolved @Smartdevs17


Key Differences

1. Documentation Build Status — RESOLVED

Analysis: The doctest failure identified in make linux-docs has been fixed. The ID.from_pubkey() docstring example now works correctly.


2. Trailing Whitespace — RESOLVED

Analysis: The previous code was pushed without proper linting applied, so there were Code quality issues which now have been cleaned up.


3. 📊 Test Execution Metrics — MINOR CHANGES

Metric Previous Review Current Review Change
Tests Passed 1933 1933 No change
Tests Skipped 16 16 No change
Warnings 25 11 Reduced by 14
Execution Time 92.84s 155.12s ⚠️ Increased by 62s (67% slower)

Analysis: Test warnings have been reduced (improvement), but execution time has increased significantly (possibly due to additional test infrastructure or different test environment).


❌ Critical Issues

🚨 BLOCKER 1: Missing Newsfragment

Aspect Status
Issue No newsfragments/312.feature.rst file exists
Previous Review Identified as CRITICAL/BLOCKER (Issue 7)
Current Review Still CRITICAL/BLOCKER (Issue 4.1)
Status UNRESOLVED

🚨 BLOCKER 2: Raw Byte Serialization vs. Protobuf

Aspect Previous Review Current Review Status
Issue Must use existing protobuf serialization Same issue identified UNRESOLVED
Reference Issue 4.3 (Major) Issue 4.2 (Critical) Elevated to Critical
Code Change Required ~2 lines (identical in both reviews) Same fix suggested Not implemented
Related Issues Resolves 4.3, 4.4, 5.2, 5.3 Resolves 4.2, 4.3, 5.1, 5.3 Multiple issues still open

Security Assessment — Consistency

These areas need some looking into it, you have not taken the following points into account, avoiding these can lead to security problems.

Security Issue Impact Status
TOCTOU Race Low ❌ Unresolved
No Encryption Medium ❌ Unresolved
No Key Type Tag Low ❌ Unresolved
No Path Validation Low New issue

Documentation Issues — Consistency

Documentation Issue Status
No README Update ❌ Unresolved
No Tutorial/Getting Started ❌ Unresolved
Inconsistent Docstring Style ❌ Unresolved

The PR has made incremental progress (fixed doctest, cleaned up lint), but still needs work a lot.

public_key = private_key.get_public_key()

return KeyPair(private_key, public_key)

Copy link
Contributor

Choose a reason for hiding this comment

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

No Private Key Validation on Load

Issue:

  1. No validation of file integrity (checksum, length check, version header)
  2. No error handling for corrupted/truncated files
  3. Directly passes raw bytes to Ed25519PrivateKey.from_bytes()

Impact: Corrupted identity files could:

  • Produce invalid keys silently
  • Cause subtle cryptographic failures
  • Be difficult to debug

Recommendation: Add validation:

def load_identity(filepath: str | Path) -> KeyPair:
    filepath = Path(filepath)
    private_key_bytes = filepath.read_bytes()
    
    # Ed25519 private keys are exactly 32 bytes
    if len(private_key_bytes) != 32:
        raise ValueError(f"Invalid private key file: expected 32 bytes, got {len(private_key_bytes)}")
    
    # Reconstruct and validate
    private_key = Ed25519PrivateKey.from_bytes(private_key_bytes)
    
    # Verify key is valid by attempting to get public key
    try:
        public_key = private_key.get_public_key()
        # Additional validation: verify we can serialize/deserialize roundtrip
        _ = public_key.serialize()
    except Exception as e:
        raise ValueError(f"Invalid private key in file: {e}")
    
    return KeyPair(private_key, public_key)

>>> save_identity(key_pair, "my_peer_identity.key")

"""
filepath = Path(filepath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: Between write_bytes() (line 55) and chmod() (line 59), the file exists with default permissions (often 0644). In a multi-user system, another process could read the private key during this window.

Recommendation: Create file with restrictive permissions atomically:

import os

def save_identity(key_pair: KeyPair, filepath: str | Path) -> None:
    filepath = Path(filepath)
    private_key_bytes = key_pair.private_key.to_bytes()
    
    # Create file with 0600 permissions atomically
    fd = os.open(filepath, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600)
    try:
        os.write(fd, private_key_bytes)
    finally:
        os.close(fd)


# Write to file with restrictive permissions (owner read/write only)
filepath.write_bytes(private_key_bytes)

Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: If the parent directory doesn't exist, filepath.write_bytes() raises FileNotFoundError.

Recommendation: Create parent directories:

filepath = Path(filepath)
filepath.parent.mkdir(parents=True, exist_ok=True)
private_key_bytes = key_pair.private_key.to_bytes()
filepath.write_bytes(private_key_bytes)

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing Test: File Permissions Not Verified

Issue: No test verifies that save_identity() actually sets file permissions to 0600.

Recommendation: Add a test:

def test_save_identity_sets_restrictive_permissions():
    """Verify that saved identity files have restrictive permissions."""
    with tempfile.TemporaryDirectory() as tmpdir:
        filepath = Path(tmpdir) / "test_identity.key"
        key_pair = create_new_key_pair()
        save_identity(key_pair, filepath)
        
        # Check file permissions (Unix-like systems)
        import os
        import stat
        if os.name != 'nt':  # Skip on Windows
            mode = filepath.stat().st_mode
            # Should be 0600 (owner read/write only)
            assert stat.S_IMODE(mode) == 0o600, 
                f"Expected permissions 0600, got {oct(stat.S_IMODE(mode))}"

Missing Test: Corrupted File Not Tested

Location: Test file - Missing test

Issue: No test for loading corrupted/truncated identity files.

Recommendation: Add a test:

def test_load_corrupted_identity_raises_error():
    """Verify that loading a corrupted identity file raises ValueError."""
    with tempfile.TemporaryDirectory() as tmpdir:
        filepath = Path(tmpdir) / "corrupted.key"
        # Write invalid data
        filepath.write_bytes(b"not a valid private key")
        
        with pytest.raises(ValueError):
            load_identity(filepath)

Copy link
Contributor

@yashksaini-coder yashksaini-coder left a comment

Choose a reason for hiding this comment

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

The PR needs to fix these bugs

  1. Add private key validation on load (length check, format validation)

  2. Fix race condition in file permissions (use atomic create with 0600)

  3. Add filepath.parent.mkdir(parents=True, exist_ok=True) before writing

  4. Fix docstring example (line comparing identical objects)

  5. Consider adding format version header for future algorithm support

  6. Add file permissions verification test

  7. Add corrupted file handling test

Smartdevs17 and others added 3 commits February 9, 2026 19:52
…ersistence

Security Fixes:
- Fix race condition in save_identity() by creating file atomically with 0600 permissions
- Add validation on load_identity() to detect corrupted/invalid key files
- Create parent directories automatically to prevent FileNotFoundError

Compatibility Fixes:
- Use protobuf serialization (serialize/deserialize_private_key) for interoperability
- Support Ed25519, RSA, and Secp256k1 keys automatically via protobuf

Documentation:
- Fix doctest in ID.from_pubkey() by adding missing import
- Add newsfragment for identity persistence feature

Addresses review feedback from PR libp2p#1185
Added 7 new test cases addressing PR review feedback:

Security Tests:
- test_save_identity_sets_restrictive_permissions: Verify 0600 file permissions
- test_load_corrupted_identity_raises_error: Validate error handling for invalid data
- test_load_truncated_file_raises_error: Handle interrupted writes
- test_load_empty_file_raises_error: Reject empty files

Functionality Tests:
- test_save_and_load_rsa_identity: Verify RSA key support via protobuf
- test_overwrite_existing_identity: Ensure file overwrites work correctly
- test_save_identity_creates_parent_directories: Auto-create nested paths

All tests pass. Addresses review feedback from yashksaini-coder and main reviewer.
@acul71
Copy link
Contributor

acul71 commented Feb 11, 2026

Hi @Smartdevs17.
Thanks for addressing my and @yashksaini-coder requests
Please run lint/typechecks (make pr) before submitting commits.
Some question remained unanswered .
Also don't just implement the requested fixes of @yashksaini-coder in code but answer to his questions in conversation.

Full review here:

AI PR Review — PR #1185: Feat/persist peer identity 312

Reviewer: AI (Cursor / py-libp2p PR Review Prompt)
Date: 2026-02-11
PR: #1185
Author: @Smartdevs17
Branch: feat/persist-peer-identity-312main


1. Summary of Changes

Type: New feature

Related Issue: #312 — Provide utilities to persist network identities between runs of a node

Problem: py-libp2p generated a new random peer identity on every restart, so nodes could not keep a stable Peer ID across sessions. That made long-lived peers, reputation, and reproducible setups harder.

Solution: This PR adds opt-in peer identity persistence via a new libp2p/identity_utils.py module:

  • save_identity(key_pair, filepath) — Saves a keypair to disk using libp2p protobuf serialization (type-tagged; supports Ed25519, RSA, Secp256k1). Uses atomic create with 0o600 and creates parent directories if needed.
  • load_identity(filepath) — Loads a keypair from disk via existing deserialize_private_key() (multi-key-type support). Validates with roundtrip serialization.
  • create_identity_from_seed(seed) — Deterministic Ed25519 keypair from a 32-byte seed (validates length; suggests hashlib.sha256() in the error message).
  • identity_exists(filepath) — Checks if an identity file exists.

libp2p/__init__.py imports these helpers and documents the identity flow in new_swarm / generate_peer_id_from. Default behavior is unchanged: no key_pair ⇒ new random Ed25519 each time.

Files changed:

File Status Description
libp2p/__init__.py Modified Identity utils imports; docstrings/comments for identity flow; generate_peer_id_from and new_swarm unchanged in behavior
libp2p/identity_utils.py Added Save/load/seed/exists helpers; protobuf serialization; atomic 0600; parent mkdir
libp2p/peer/id.py Modified Docstring and doctest for ID.from_pubkey() (with ID import in example)
newsfragments/312.feature.rst Added User-facing feature note; ends with newline
tests/core/test_identity_persistence.py Added Core persistence, seed, path types, errors, integration with new_host
tests/core/test_identity_persistence_enhanced.py Added Permissions, corrupted/truncated/empty file, overwrite, parent dirs

Breaking changes / deprecations: None.


2. Branch Sync Status and Merge Conflicts

Branch sync status

  • Status: ℹ️ Ahead of origin/main
  • Details: Branch is 0 commits behind and 10 commits ahead of origin/main (from branch_sync_status.txt: 0 10).

Merge conflict analysis

  • Conflicts detected:No conflicts
  • Details: Test merge (git merge --no-commit --no-ff origin/main) completed successfully; log shows "Already up to date." The PR branch can be merged cleanly into origin/main.

3. Strengths

  1. Opt-in design — Default remains one random identity per host creation; persistence is explicit (key_pair or save/load/seed). Aligns with issue Provide utilities to persist network identities between runs of a node #312 and avoids breaking existing users.

  2. Correct use of existing crypto stack — Save/load use key_pair.private_key.serialize() and deserialize_private_key(), so format is protobuf, type-tagged, and supports Ed25519, RSA, and Secp256k1. No custom raw-byte format.

  3. Security-conscious file handling — Atomic create with os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600 avoids TOCTOU between write and chmod. Parent directories created with filepath.parent.mkdir(parents=True, exist_ok=True).

  4. Strong test coverage — Two test modules: core persistence, seed determinism, path types (str/Path), missing/corrupted/truncated/empty files, permissions, overwrite, parent-dir creation, and integration with new_host() (saved identity, seed identity, default random). Both sync and async (@pytest.mark.trio) paths covered.

  5. Clear PR description — Three usage options (own KeyPair, save/load from disk, deterministic seed) with code snippets, security notes, and backward-compatibility statement.

  6. Docstrings and doctestidentity_utils and generate_peer_id_from documented; ID.from_pubkey() has an example that imports ID and runs in doctest (docs build and doctests pass).

  7. Seed validationcreate_identity_from_seed enforces 32-byte seed and suggests hashlib.sha256() in the error message.


4. Issues Found

Critical

None.

Major

None.

Minor

  • File: tests/core/test_identity_persistence_enhanced.py (and pre-commit run)

  • Line(s): Multiple (trailing whitespace, import order, line length)

  • Issue: On first run, make lint failed: “trim trailing whitespace” and “ruff” modified this file (trailing whitespace, Path import order, long assertion lines). Pre-commit fixed them and a second run passed.

  • Suggestion: Run pre-commit run --all-files (or equivalent) before pushing so CI lint passes on first try.

  • File: tests/core/test_identity_persistence_enhanced.py

  • Line(s): Test name test_save_and_load_rsa_identity

  • Issue: The test checks overwriting an identity file with a second identity and loading it; both keypairs are from create_new_key_pair() (Ed25519). The name suggests RSA.

  • Suggestion: Rename to something like test_save_overwrites_existing_identity or keep the name but add a short comment that the test is about overwrite behavior, not key type.


5. Security Review

  • Private key storage: Identity files contain private keys. PR states they must be kept secure; save_identity uses 0600 and atomic create. No encryption (same as many libp2p setups); optional encryption could be a follow-up.
  • Serialization: Protobuf with type tag; no custom format. Load path uses existing deserialize_private_key() and validates via public key and roundtrip serialize.
  • Input validation: Seed length enforced; load path turns deserialization/validation failures into ValueError with a clear message.
  • Path handling: Uses Path and standard I/O; no shell or unsafe path injection observed.
  • Sensitive data in logs/exceptions: Error messages refer to file path and “Invalid or corrupted”; no key material logged.

No further security issues identified for this change set.


6. Documentation and Examples

  • Docstrings: identity_utils and generate_peer_id_from have Args/Returns/Raises/Example where appropriate. ID.from_pubkey() docstring includes a runnable doctest with ID imported.
  • PR description: Usage options, code snippets, and security/backward-compatibility notes are clear.
  • Newsfragment: 312.feature.rst is present and user-focused; towncrier draft includes it under Features.
  • API surface: identity_utils is imported in libp2p/__init__.py and is part of the public API implied by the PR. Consider adding libp2p.identity_utils to the docs API index or a short “Identity persistence” section if the project documents modules there.

7. Newsfragment Requirement

  • Issue reference: ✅ PR references issue Provide utilities to persist network identities between runs of a node #312 (Related Issue in description).
  • Newsfragment file:newsfragments/312.feature.rst exists.
  • Naming:312.feature.rst (single issue, feature).
  • Content: ✅ Short ReST, user-facing (“Add opt-in peer identity persistence utilities…”).
  • Newline: ✅ File ends with newline (od -c shows .\n).
  • Type:.feature.rst matches new functionality.

No blocker; requirement satisfied.


8. Tests and Validation

Lint (make lint)

  • First run: Failed. “trim trailing whitespace” and “ruff” modified tests/core/test_identity_persistence_enhanced.py (trailing whitespace, import order, line length). Pre-commit re-ran and then all hooks passed.
  • Second run (after auto-fix): All checks passed (yaml, toml, end of files, trailing whitespace, pyupgrade, ruff, ruff format, mdformat, mypy, pyrefly, rst check).
  • Conclusion: Lint passes after applying pre-commit; author should run pre-commit before push to avoid CI failure.

Type checking (make typecheck)

  • Result: Passed (mypy and pyrefly).
  • Output: No type errors or warnings reported in typecheck_output.log.

Test execution (make test)

  • Result: All tests passed.
  • Summary: 1939 passed, 16 skipped, 25 warnings, ~92.56s.
  • Warnings: e.g. PytestUnknownMarkWarning: Unknown pytest.mark.integration in test_proxy.py, and RuntimeWarning: coroutine 'AsyncMockMixin._execute_mock_call' was never awaited in test_muxer_multistream.py. These are pre-existing and unrelated to this PR.
  • Identity tests: No failures in test_identity_persistence*.

Documentation build (make linux-docs)

  • Result: Build and doctests succeeded.
  • Doctests: e.g. libp2p (4), libp2p.peer (9), libp2p.transport.websocket (105); 118 total, 0 failures.
  • Newsfragment: python ./newsfragments/validate_files.py and towncrier build --draft ran successfully; 312.feature appears in the draft under Features.

9. Recommendations for Improvement

  1. Pre-commit before push — Run pre-commit run --all-files (or project equivalent) so lint passes on first CI run.
  2. Test name — Rename or clarify test_save_and_load_rsa_identity so it doesn’t imply RSA keys (see Minor issues).
  3. Docs — Optionally add libp2p.identity_utils (or an “Identity persistence” section) to the documented API if the project maintains that list.

10. Questions for the Author

  1. Would you consider adding a one-line note in the main README or “Getting started” pointing to identity persistence (e.g. “For a stable Peer ID across restarts, see libp2p.identity_utils and the examples in the PR”)?
  2. For the overwrite test, was the name test_save_and_load_rsa_identity intended to leave room for a future RSA-specific test, or should it be renamed to reflect overwrite behavior?

11. Status of Previous Maintainer and Reviewer Requests

Cross-check against the current branch: acul71 (maintainer) and yashksaini-coder (reviewer) had requested specific fixes in earlier comments. Below is whether those requests are fulfilled in this PR.

acul71 (maintainer — from embedded AI review)

Request Status
Fix doctest (add ID import in ID.from_pubkey()) ✅ Fulfilled
Add newsfragment 312.feature.rst ✅ Fulfilled
Use protobuf serialization (not raw bytes) ✅ Fulfilled
Fix TOCTOU: atomic create with 0600 ✅ Fulfilled
Add missing tests (corrupt file, permissions, overwrite) ✅ Fulfilled
Run pre-commit so lint passes on first run ⚠️ Not yet: first lint still fails on test_identity_persistence_enhanced.py; author should run pre-commit run --all-files and commit
Standardize docstrings to ReST (:param:, :return:) in identity_utils ❌ Not done: module still uses Google style (Args/Returns/Raises)
Acknowledge #312 design (key pair provider / keystore) in PR or follow-up ❌ Not done (optional)

Summary: All critical/blocker items are fulfilled. Remaining: pre-commit + optional docstring style and #312 acknowledgment.

yashksaini-coder (reviewer — “The PR needs to fix these bugs”)

# Request Fulfilled?
1 Private key validation on load (length/format) ✅ Yes — deserialize_private_key + get_public_key + roundtrip
2 Fix race condition: atomic create with 0600 ✅ Yes
3 filepath.parent.mkdir(parents=True, exist_ok=True) ✅ Yes
4 Fix docstring example (comparing identical objects) ✅ Yes — doctest compares distinct ID instances correctly
5 Consider format version header ❌ Not done (optional)
6 File permissions verification test ✅ Yes — test_save_identity_sets_restrictive_permissions
7 Corrupted file handling test ✅ Yes — corrupted, truncated, empty file tests

Summary: All relevant items (1–4, 6, 7) are fulfilled. Item 5 (version header) is optional.


12. Overall Assessment

  • Quality rating: Good — Clear feature, correct use of protobuf and existing APIs, solid tests and docs; only minor lint/style fixes needed before merge.
  • Security impact: Low — Private keys on disk with 0600 and atomic create; no encryption (documented); no new high-risk patterns.
  • Merge readiness: Needs small fixes — Run pre-commit (and optionally rename the one test) so lint is green and naming is accurate; then ready to merge.
  • Confidence: High — Checked branch sync, merge, lint, typecheck, tests, and docs; behavior and design match the stated goal and issue Provide utilities to persist network identities between runs of a node #312.

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.

4 participants