Overlay: remove dead Encoder::encode / Connection::split() sender-receiver chain

[tomerweller]

Summary

Follow-up from #3774 / PR #3783. While fixing the missing MAX_MESSAGE_SIZE check on the real send path (MessageCodec::encode_message), /plan and both critics confirmed that an entire parallel encode/framing abstraction in crates/overlay/src/codec.rs and crates/overlay/src/connection.rs is dead code in production:

• The Encoder<AuthenticatedMessage>::encode trait impl (codec.rs) — reachable only via a Sink-trait generic-dispatch path.
• That Sink path runs through Connection::split() / ConnectionSender / ConnectionReceiver (connection.rs), which have zero callers anywhere in the workspace and are not re-exported from lib.rs.

The real send path is Connection::send → MessageCodec::encode_message, and the real receive path is Decoder::decode; neither goes through the split abstraction.

Suggested next step

Confirm (again, at cleanup time) that Connection::split(), ConnectionSender, ConnectionReceiver, and the Encoder::encode trait impl have no remaining callers, then remove the whole chain. After #3783 merges, the size-bound helper (check_encode_size) is called from both encode_message and Encoder::encode; removing Encoder::encode leaves encode_message as its sole caller, which is fine.

Context

Not urgent — pure dead-code removal, no observable-surface impact. Kept out of #3783 to keep that bug fix focused on the missing check.

[tomerweller]

Triage Report

Verdict: ACCEPT

Type: enhancement
Kind: refactor
Severity: n/a
Crate: crate:overlay
Size: S

Summary: Remove dead code in the overlay codec/connection abstraction—the Connection::split() method, ConnectionSender, ConnectionReceiver, and the Encoder<AuthenticatedMessage>::encode trait impl are unreachable in production. Clean up by deleting these unused paths, confirming no callers remain in the workspace.

Test obligation: Existing tests must keep passing; net behavioral diff = 0 (dead-code removal only). No new tests required.

Label adjustments: Removed bug, added enhancement (this is code quality work, not a bug fix).

[tomerweller]

📝 Plan Draft (Round 1)

Summary: Delete the production-dead overlay encode/split abstraction — the Encoder<AuthenticatedMessage> codec impl and the Connection::split() / ConnectionSender / ConnectionReceiver sink-receiver chain — and clean up the imports/tests they leave behind.

Files to modify:

• crates/overlay/src/codec.rs — remove the impl Encoder<AuthenticatedMessage> for MessageCodec block (lines ~271–292); drop Encoder from use tokio_util::codec::{Decoder, Encoder} and BufMut from use bytes::{Buf, BufMut, BytesMut}; refresh the check_encode_size doc comment (which currently claims it is "shared by … and the Encoder trait impl") to name encode_message as its sole caller; rewrite the two tests that call codec.encode(...).
• crates/overlay/src/connection.rs — remove Connection::split() (lines ~374–389), struct ConnectionSender + its impl (~392–428), struct ConnectionReceiver + its impl (~430–462); drop SinkExt from use futures::{SinkExt, StreamExt} (keep StreamExt, still used by Connection::recv).

Approach:
The real send path is Connection::send → MessageCodec::encode_message (a plain Vec<u8> builder that calls check_encode_size), and the real receive path is Framed::next → Decoder::decode. Neither touches the tokio Encoder/Sink machinery: send/send_encoded_batch write via self.framed.get_mut().write_all(...), and recv uses self.framed.next() (Stream/Decoder only). The Encoder impl is reachable in production only through Framed's Sink, which is only ever exercised by Connection::split() → ConnectionSender, and split() has no callers. So the entire chain is dead.

Removal is a straight deletion (not a visibility narrowing). Because Connection::send/recv/send_encoded_batch do not depend on MessageCodec: Encoder, dropping the Encoder impl does not affect Framed<BoxedIo, MessageCodec> usage. After deletion, three imports become unused and must be dropped in the same change to stay -Dwarnings-clean: Encoder and BufMut in codec.rs (BufMut::put_u32/reserve were only used by the removed encode; Buf/BytesMut stay for Decoder), and SinkExt in connection.rs (only used by ConnectionSender::send). SplitSink/SplitStream are referenced only via fully-qualified paths inside the removed structs, so no import change is needed for them.

Two #[cfg(test)] tests in codec.rs call the removed codec.encode(...): test_codec_roundtrip_with_record_marking_bit and test_codec_streaming. Both are genuine roundtrip/streaming coverage and must be preserved, not dropped — rewrite each to produce wire bytes via MessageCodec::encode_message(&msg) and feed them into the BytesMut (buf.extend_from_slice(&encoded)) before calling codec.decode(...). This keeps the decode-side assertions (final-fragment bit, streaming completeness) intact while sourcing bytes from the surviving real encoder.

Because this is a delete + import cleanup under a global -Dwarnings (.cargo/config.toml), /do must verify reachability with a scratch build, not a grep: in a worktree under ~/data with CARGO_TARGET_DIR under ~/data, run cargo build -p henyey-overlay --all-targets and cargo test -p henyey-overlay, plus a workspace cargo build --all to confirm no external caller (there are none: ConnectionSender/ConnectionReceiver are not re-exported from lib.rs, which exports only Connection, ConnectionDirection, Listener).

Test plan:

• New coverage: none required — this is a pure removal of dead code; no new public surface is introduced.
• Existing tests preserved (must keep passing in cargo test -p henyey-overlay):
  • crates/overlay/src/codec.rs::test_codec_roundtrip_with_record_marking_bit — rewritten to encode via encode_message; still asserts the decoded MessageFrame.is_last_fragment is true for both an authenticated and a Hello message.
  • crates/overlay/src/codec.rs::test_codec_streaming — rewritten to encode via encode_message; still asserts a full frame decodes to Some.
  • crates/overlay/src/codec.rs::test_encode_message_rejects_oversized_message, test_check_encode_size_rejects_oversized, test_encode_decode, test_record_marking_bit_set_for_*, test_decode_accepts_continuation_bit_clear, test_codec_partial_read, decoder size-limit tests — untouched, confirm the real encode/decode path is unaffected.
  • crates/overlay/src/connection.rs::test_connection_pool*, test_connection_direction — untouched.

Parity considerations:
n/a — does not change the observable surface. The removed Encoder::encode produced byte-identical framing to the surviving encode_message (same len | 0x80000000 prefix + XDR body); it was never on any production wire path. No hashes, result/meta XDR, SCP/overlay wire bytes, archive format, HTTP/RPC/CLI, or crypto output changes. No PARITY_STATUS.md update required (internal helper removal).

Risks:

• Import over-pruning: dropping an import that some other item still needs would break the build — mitigated by the mandatory scratch cargo build -p henyey-overlay --all-targets under -Dwarnings.
• Missing a codec.encode( call site outside the two identified tests — grep confirms only lines 511/524/604 in codec.rs; the workspace build is the backstop.
• Test rewrite must not weaken assertions (keep the is_last_fragment and streaming checks) — the surviving encoder emits identical bytes, so the assertions carry over unchanged.

[tomerweller]

🔍 Critic A (correctness) — Round 1

Verdict: APPROVE

Key concerns:

• Dead-code claims independently confirmed: ConnectionSender/ConnectionReceiver/Connection::split() have zero non-definition callers and are not re-exported from lib.rs (only Connection, ConnectionDirection, Listener are); codec.encode(...) is called only at codec.rs:511/524/604, all inside #[cfg(test)]. Nothing production-reachable is lost.
• Build-verify gate satisfied: the plan DELETES rather than blind-narrows to pub(crate), and mandates a scratch cargo build -p henyey-overlay --all-targets + workspace build under -Dwarnings — correct, since a grep cannot prove reachability and the three unused imports (Encoder, BufMut, SinkExt) would each be hard errors if mis-pruned.
• Import analysis is precise: Buf/BytesMut stay (used by Decoder::decode via advance/split_to), only BufMut (put_u32) leaves; StreamExt stays (used by recv), only SinkExt leaves. Matches source.
• Test obligation for kind: refactor is met: no regression test required, but the two encode-using tests are correctly preserved-by-rewrite rather than deleted, keeping the is_last_fragment/streaming assertions.

Full review

The plan targets the exact symbols named in triage and the issue. I verified each:

1. self.framed.get_mut().write_all(&encoded) in Connection::send (and send_encoded_batch) bypasses the Sink/Encoder path entirely; recv uses self.framed.next() (Stream → Decoder only). So MessageCodec: Encoder is not a bound required by any surviving Framed usage. Removing the impl cannot break send/recv/send_encoded_batch. Correct.

2. Encoder::encode and encode_message emit byte-identical frames (len | 0x80000000 prefix + to_xdr body), so the two rewritten tests will observe identical decode behavior — the assertions carry over without weakening. encode_message returns [len(4)|body] as Vec<u8>; buf.extend_from_slice(&encoded) then codec.decode(&mut buf) is the right substitution.

3. The check_encode_size doc-comment update is a necessary correctness/hygiene item (it currently names the Encoder impl as a co-caller); the plan calls it out.

No correctness blocker. The only residual risk is mechanical (import over-pruning / a missed call site), and it is fully covered by the mandated scratch build. APPROVE.

[tomerweller]

🔍 Critic B (parity) — Round 1

Verdict: APPROVE

Key concerns:

• The change does not touch the observable/interop surface defined in docs/PARITY.md. The removed Encoder::encode was never on a production wire path (the real send path is encode_message, which stays), so no overlay wire bytes change.
• encode_message — the surviving encoder — is untouched: same 4-byte record-marking prefix (len | 0x80000000) + XDR body, still matching stellar-core TCPPeer's framing and getIncomingMsgLength() masking semantics documented in the module.
• No ledger/bucket hashes, transaction result/meta XDR, SCP envelopes, history archive format, HTTP/RPC/CLI contracts, or crypto outputs are affected.
• No PARITY_STATUS.md update needed: the deleted items are internal helper abstractions with no parity mapping.

Full review

Parity here is only implicated via the overlay wire framing. The framing behavior lives in MessageCodec::encode_message / Decoder::decode, both of which the plan explicitly leaves intact. The Encoder trait impl produced byte-identical output to encode_message but was only reachable through the unused Connection::split() sink path, so its removal is invisible on the wire. This is precisely the "internal architecture / helper utilities" category that docs/PARITY.md says MAY be changed freely. Nothing to flag. APPROVE.

[tomerweller]

🔍 Critic C (scope) — Round 1

Verdict: APPROVE

Key concerns:

• Scope is "just right": one atomic concern (remove the dead encode/split abstraction), one crate (henyey-overlay), two files. Net change is a small deletion (~70 lines removed across the Encoder impl + split() + two structs/impls) plus three one-line import edits and two localized test rewrites — far under the ~500-line ceiling.
• Not too narrow: the plan removes the whole dead chain identified in Overlay: MessageCodec::encode_message lacks MAX_MESSAGE_SIZE check present in unused Encoder::encode #3774/Enforce MAX_MESSAGE_SIZE on the overlay send path #3783 (impl + split() + ConnectionSender + ConnectionReceiver + their now-unused imports), not just one symbol, so it addresses the root cleanup rather than band-aiding a fragment.
• Not too broad: it deliberately leaves check_encode_size in place (single caller is fine, per the issue) rather than inlining it — avoids scope creep. It does not attempt unrelated overlay cleanups.
• No follow-up sub-issues needed; this is a single reviewable PR.

Full review

The issue is explicitly a self-contained dead-code removal follow-up. The plan matches that scope exactly and resists two temptations: (1) inlining check_encode_size into encode_message (would be a gratuitous extra diff — correctly deferred), and (2) touching the surrounding Connection send/recv paths. Keeping the two encode-using tests alive by rewriting rather than deleting is the right scope call — it preserves coverage without expanding surface. Single crate, single concern, single PR. APPROVE.

[tomerweller]

✅ Converged Plan

Summary: Delete the production-dead overlay encode/split abstraction — the Encoder<AuthenticatedMessage> codec impl and the Connection::split() / ConnectionSender / ConnectionReceiver sink-receiver chain — and clean up the imports and tests they leave behind.

Files to modify:

• crates/overlay/src/codec.rs — remove the impl Encoder<AuthenticatedMessage> for MessageCodec block (~lines 271–292); drop Encoder from use tokio_util::codec::{Decoder, Encoder}; drop BufMut from use bytes::{Buf, BufMut, BytesMut} (keep Buf+BytesMut for Decoder); refresh the check_encode_size doc comment so it names encode_message as the sole caller (it currently says "shared by … and the Encoder trait impl"); rewrite the two #[cfg(test)] tests that call codec.encode(...).
• crates/overlay/src/connection.rs — remove Connection::split() (~lines 374–389), struct ConnectionSender + impl (~392–428), struct ConnectionReceiver + impl (~430–462); drop SinkExt from use futures::{SinkExt, StreamExt} (keep StreamExt, still used by Connection::recv). SplitSink/SplitStream are fully-qualified inside the removed structs, so no separate import edit.

Approach:
The real send path is Connection::send → MessageCodec::encode_message (a Vec<u8> builder calling check_encode_size), written via self.framed.get_mut().write_all(...); the real receive path is Framed::next → Decoder::decode. Neither uses the tokio Encoder/Sink machinery, which is reachable in production only through Connection::split() → ConnectionSender, and split() has no callers. So the whole chain is dead and can be deleted outright (not narrowed).

Because send/recv/send_encoded_batch do not require MessageCodec: Encoder, dropping the Encoder impl does not affect Framed<BoxedIo, MessageCodec> usage. Deletion leaves three imports unused, which must be removed in the same change to stay -Dwarnings-clean: Encoder and BufMut in codec.rs, and SinkExt in connection.rs.

Two in-module tests call the removed codec.encode(...) — test_codec_roundtrip_with_record_marking_bit and test_codec_streaming. Preserve (do not delete) them: rewrite each to produce wire bytes via MessageCodec::encode_message(&msg) and feed them into the BytesMut (buf.extend_from_slice(&encoded)) before codec.decode(...). The surviving encoder emits byte-identical frames, so the is_last_fragment / streaming assertions carry over unchanged.

Kind: refactor (from triage)

Test plan:

• Regression tests: none (not a bug-fix; net behavioral diff = 0).
• New coverage: none — pure dead-code removal, no new public surface.
• Existing tests preserved (must pass in cargo test -p henyey-overlay):
  • codec.rs::test_codec_roundtrip_with_record_marking_bit — rewritten to encode via encode_message; still asserts decoded is_last_fragment == true for an authenticated and a Hello message.
  • codec.rs::test_codec_streaming — rewritten to encode via encode_message; still asserts a full frame decodes to Some.
  • codec.rs::test_encode_message_rejects_oversized_message, test_check_encode_size_rejects_oversized, test_encode_decode, test_record_marking_bit_set_for_*, test_decode_accepts_continuation_bit_clear, test_codec_partial_read, decoder size-limit tests — untouched.
  • connection.rs::test_connection_pool*, test_connection_direction — untouched.
• Build-verify gate (required for /do): apply the deletion + import edits in a scratch worktree under ~/data with CARGO_TARGET_DIR under ~/data, then run cargo build -p henyey-overlay --all-targets, cargo test -p henyey-overlay, and cargo build --all — all under the repo's global -Dwarnings. A grep alone is insufficient to prove the imports/symbols are dead. (Grep already confirms no external callers and no lib.rs re-export of ConnectionSender/ConnectionReceiver.)

Parity considerations:
n/a — does not change the observable surface. The removed Encoder::encode produced byte-identical framing to encode_message (same len | 0x80000000 prefix + XDR body) and was never on a production wire path. No ledger/bucket hashes, result/meta XDR, SCP/overlay wire bytes, archive format, HTTP/RPC/CLI, or crypto outputs change. No PARITY_STATUS.md update required.

Minor items to consider during implementation:

• Do not weaken the two rewritten tests' assertions (keep the is_last_fragment and streaming checks).
• Do not inline check_encode_size — a single caller is fine (per the issue); just fix its stale doc comment.

Convergence: Round 1, verdicts: A=APPROVE, B=APPROVE, C=APPROVE.