Track deferred cleanup items from PR #2125

[sanity]

Overview

Tracking deferred code cleanup items from PR #2125 review to ensure they don't get lost in follow-up work.

Background

During review of the transient connection handling changes, several code quality issues were intentionally left unchanged to minimize churn in the PR slice. This issue tracks those items for systematic follow-up.

Reference: #2125 (comment)

Deferred Items

1. Dead Code Annotations

File: crates/core/src/ring/connection_manager.rs

Remove unused #[allow(dead_code)] annotations on helper methods:

• get_open_connections()
• get_reserved_connections()

These were marked to avoid compilation warnings but may indicate methods that should be removed entirely or need better documentation of their purpose.

2. Redundant Pending Reservation Logic

File: crates/core/src/ring/connection_manager.rs

The should_accept() method inserts peers into pending_reservations before the acceptance decision completes. This creates potential duplicate tracking:

• Line ~184: Initial insertion during acceptance check
• Line ~244: Second insertion via record_pending_location()

Review if both insertions are necessary or if logic can be simplified.

3. Double Removal During Rejection

File: crates/core/src/ring/connection_manager.rs

In add_connection(), when a connection is rejected due to cap enforcement, the peer is removed from pending_reservations twice:

• Once in the rejection path
• Again during rollback cleanup

Consolidate removal logic to avoid redundant operations.

4. Logging Verbosity

File: crates/core/src/node/network_bridge/p2p_protoc.rs

Routing candidate selection logs at info! level cause log flooding during normal operation. Downgrade to debug! or trace! to reduce noise.

5. Public Key Display Format

File: crates/core/src/node/network_bridge/p2p_protoc.rs (and related)

SHA256 hash representation for public keys made logs unreadable. Reverted to shortened base58 encoding in this PR, but review if consistent display format should be used project-wide.

6. Test Harness Dependency

Related issue: #2082

Soak and cap regression tests rely on out-of-tree freenet-test-network crate. Resolve whether to:

• Inline necessary test utilities into freenet-core
• Move freenet-test-network into the main repo
• Keep as external dependency with explicit justification

See also: #2125 (comment)

Priority

These are code quality improvements, not critical bugs. Can be addressed incrementally in follow-up PRs.

Success Criteria

• All #[allow(dead_code)] removed or justified with comments
• Pending reservation logic simplified to single insertion point
• No redundant removals during rollback
• Routing logs produce reasonable output volume at default log level
• Consistent public key display format documented
• Test harness strategy documented and consensus reached

[github-actions]

🤖 Auto-labeled

Applied labels:

• T-tracking (98% confidence)
• E-medium (80% confidence)
• A-networking (90% confidence)
• A-developer-xp (90% confidence)

Reasoning: This issue is explicitly a meta tracking task for deferred cleanup items from PR #2125 rather than a single bug or new feature, so T-tracking is appropriate. The work spans multiple files and involves moderate refactors (simplifying reservation logic, consolidating removal, changing log levels, deciding public-key display format, and test harness strategy), which suggests a medium effort. A-networking applies because the changes touch connection_manager and routing candidate selection; A-developer-xp applies due to test harness dependency, logging, and general developer-facing changes; A-documentation applies because there is a need to document consistent public key display format and the test harness strategy. No P- (priority) label is set because the issue is not a bug or feature request and is described as low-risk, incremental work.

Previous labels: none

If these labels are incorrect, please update them. This helps improve the auto-labeling system.