Remove dead overlay Encoder impl and Connection::split chain

[tomerweller]

Closes #3784

Summary

Removes the production-dead overlay encode/split abstraction: the tokio Encoder<AuthenticatedMessage> impl on MessageCodec (codec.rs) and the Connection::split() / ConnectionSender / ConnectionReceiver sink-receiver chain (connection.rs). The real send path is Connection::send -> MessageCodec::encode_message and the real receive path is Decoder::decode; neither uses the Sink/Encoder machinery, and split() had zero callers (the halves were never re-exported from lib.rs). Also drops the now-unused Encoder, BufMut, and SinkExt imports and refreshes the stale check_encode_size / MessageCodec doc comments.

Plan reference

Converged Plan comment (round-1, A/B/C all APPROVE)

Test plan

• cargo fmt --check
• cargo clippy -p henyey-overlay --all-targets (global -Dwarnings) clean
• cargo test -p henyey-overlay passes
• cargo build --all passes (confirms no external caller of the removed symbols)

The two #[cfg(test)] tests that called codec.encode(...) (test_codec_roundtrip_with_record_marking_bit, test_codec_streaming) were preserved by rewriting them to source byte-identical wire frames from MessageCodec::encode_message; their is_last_fragment / streaming assertions are unchanged and still pass.

Deviations from plan

None.

🤖 Generated with Claude Code

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Clean, self-contained dead-code removal exactly matching issue #3784. Removed symbols have zero callers; the three in-crate round-trip tests were correctly rewritten to the real send path, and CI Test/Clippy/Build are green.

Full review

Cycle 1 (no prior Correctness verdict on this PR) — complete change-list review.

• Correctness of removal: The diff deletes impl Encoder<AuthenticatedMessage> for MessageCodec (codec.rs) and Connection::split() + ConnectionSender + ConnectionReceiver (connection.rs), plus the now-unused imports (BufMut, Encoder, SinkExt). Verified zero remaining callers workspace-wide: grep for ConnectionSender|ConnectionReceiver|.split() and Encoder<|codec.encode( returns nothing outside the edited files. lib.rs re-exports only Connection, ConnectionDirection, Listener — the removed types were never part of the public surface.
• Test coverage (kind: refactor / dead-code removal): the three tests that previously exercised the removed Encoder::encode (test_* round-trips in codec.rs) were rewritten to buf.extend_from_slice(&MessageCodec::encode_message(&msg).unwrap()), so coverage of the framing round-trip is preserved on the real send path. The Overlay: MessageCodec::encode_message lacks MAX_MESSAGE_SIZE check present in unused Encoder::encode #3774 regression test (encode_message size-bound rejection) is untouched and still present. CI Test + Doctest + Clippy are green, satisfying the build-verify gate for a removal.
• Shared helper: check_encode_size is now called solely from encode_message; the doc comment was updated to drop the stale "shared by ... the Encoder trait impl" wording. No behavioral change.
• Readability: doc comments on MessageCodec and check_encode_size were updated to reflect the Decoder-only + encode_message reality. Accurate.

No concerns.

[tomerweller]

🔍 Reviewer: Parity

Verdict: APPROVE

Summary: No observable/interop-surface impact. The removed Encoder::encode produced byte-identical framing to the retained encode_message and had zero callers (dead Sink path), so overlay wire bytes are unchanged. The split sender/receiver chain was likewise uncalled.

Full review

Cycle 1 — parity lens per docs/PARITY.md (observable surface only: SCP/overlay wire bytes, hashes, tx result/meta XDR, history format, HTTP/RPC/CLI, crypto).

• Overlay wire bytes unchanged: the deleted Encoder<AuthenticatedMessage>::encode and the retained MessageCodec::encode_message performed identical framing — XDR-encode the AuthenticatedMessage, then a 4-byte record-marking prefix with bit 31 set (len | 0x80000000) followed by the body. The real send path (Connection::send → encode_message, connection.rs:245; peer.rs:914) never used the trait impl, which was reachable only via the unused Sink/split() generic-dispatch path. Removing it cannot change any byte on the wire.
• Receive path untouched: Decoder::decode for MessageCodec is unchanged; MessageFrame and the final-fragment bit handling are intact.
• No public API/interop break: ConnectionSender/ConnectionReceiver/Connection::split() had zero callers and were never re-exported from lib.rs (only Connection, ConnectionDirection, Listener are), so no HTTP/RPC/CLI or cross-crate contract is affected.
• Internal-only surface: import cleanup, doc comments, and helper single-caller collapse are all internal (explicitly allowed to deviate). Nothing flagged.

No observable-surface concerns.

[tomerweller]

Review: Bounce-Back Cycle 1

Reason: CI failed (unrelated, will rebase)

Reviewer A: APPROVE — clean dead-code removal; removed symbols have zero callers; round-trip tests rewritten to encode_message.
Reviewer B: APPROVE (Parity) — no observable/overlay-wire-byte impact; removed Encoder::encode was byte-identical to and dead relative to encode_message.
External reviewers: none
CI: red

The only non-green check is test (testnet, core,horizon, ...) with conclusion CANCELLED. The job ran ~1h45m (07:52 → 09:37 UTC) before cancellation with no failed step — a testnet quickstart integration timeout/cancellation from external testnet flakiness, not a diff failure. All local and pubnet quickstart variants, plus Clippy/Test/Doctest/Build/CodeQL, are green. This PR only removes dead overlay code with zero callers and cannot affect a live-testnet integration test, so the red is unrelated to the diff.

Both reviewer lenses APPROVE. Per the /review-pr matrix, an unrelated CI red still cannot auto-merge: routing back to ready-for-doing for /do Mode B to rebase on origin/main and re-trigger CI. If CI is green after the rebase, the next /review-pr tick will auto-merge on triple-green. If the testnet check is still red after rebase, the next tick will mark blocked.

[tomerweller]

Do: Mode B — CI re-triggered

Both reviewer lenses already APPROVE (Correctness + Parity); the only bounce reason was an unrelated CANCELLED/timed-out test (testnet, core,horizon, …) integration job.

• Rebased do/issue-3784 onto current origin/main (5ac2f16) — no-op: the PR head (7a32106) is already based on the current main tip, so there is no new commit to push and the diff is unchanged.
• No inline review comments to address (both lenses approved with no concerns).
• Re-ran the failed testnet job on run 32626726933 via gh run rerun --failed. It is now in progress. All other checks (Clippy, Test, Doctest, Build, CodeQL, local/pubnet quickstart) are green.

Ready for re-review once the testnet job completes.

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Cycle 2, code unchanged since cycle 1 (head still 7a32106). Re-verified the dead-code removal: zero external callers, tests correctly re-sourced from the real send path, CI Test/Clippy/Build/Doctest green. No new concern classes.

Full review

Cycle N≥2 — prior Correctness verdict was APPROVE. Prior concern classes: none. Re-verifying, no new class discovered.

• Zero callers (re-verified): workspace grep for ConnectionSender|ConnectionReceiver|\.split\(\) and Encoder<|codec\.encode\( returns matches only inside the two edited files (the definitions/tests being removed) — no external caller anywhere in crates/.
• Not re-exported: crates/overlay/src/lib.rs:95 re-exports only connection::{Connection, ConnectionDirection, Listener}; the removed split/ConnectionSender/ConnectionReceiver and the Encoder trait impl were never public surface. Removal breaks no cross-crate contract.
• Test coverage (kind: refactor / dead-code removal): the three round-trip tests that used codec.encode(...) are rewritten to buf.extend_from_slice(&MessageCodec::encode_message(&msg).unwrap()), preserving framing round-trip coverage on the real send path. The Overlay: MessageCodec::encode_message lacks MAX_MESSAGE_SIZE check present in unused Encoder::encode #3774 size-bound regression test is untouched. CI Test+Clippy+Doctest+Build are green — build-verify gate satisfied for a removal.
• Helper collapse: check_encode_size now has a single caller (encode_message); doc comment updated to drop the stale "shared by ... the Encoder trait impl" wording. No behavioral change.

No concerns.

[tomerweller]

🔍 Reviewer: Parity

Verdict: APPROVE

Summary: Cycle 2, code unchanged. No observable/interop-surface impact: the removed Encoder::encode was byte-identical to the retained encode_message and had zero callers; the split sender/receiver chain was likewise dead. Overlay wire bytes are unchanged.

Full review

Cycle N≥2 — prior Parity verdict was APPROVE. Prior concern classes: none. Re-verifying, no new class discovered.

Parity lens per docs/PARITY.md (observable surface only: SCP/overlay wire bytes, hashes, tx result/meta XDR, history format, HTTP/RPC/CLI, crypto).

• Overlay wire bytes unchanged (re-verified byte-identity): removed Encoder::encode did to_xdr(Limits::none()) → check_encode_size → put_u32(len | 0x80000000) (big-endian) → body. Retained encode_message does to_xdr(Limits::none()) → check_encode_size → extend_from_slice(&(len | 0x80000000u32).to_be_bytes()) → body. Identical bytes. The real send path (Connection::send → encode_message) never used the trait impl (reachable only via the unused Sink/split() path), so no byte on the wire changes.
• Receive path untouched: Decoder::decode for MessageCodec, MessageFrame, and final-fragment bit handling are unchanged.
• No interop/API break: removed symbols had zero callers and were never re-exported from lib.rs; no HTTP/RPC/CLI or cross-crate contract affected.
• Internal-only: import cleanup, doc comments, single-caller helper collapse — all internal, explicitly allowed to deviate.

No observable-surface concerns.

[tomerweller]

Review: Bounce-Back Cycle 2

Reason: CI failed (unrelated, will rebase)

Reviewer A: APPROVE — clean dead-code removal; re-verified zero external callers and preserved round-trip test coverage on the real encode_message send path.
Reviewer B: APPROVE (Parity) — no observable/overlay-wire-byte impact; removed Encoder::encode is byte-identical to encode_message and was dead.
External reviewers: none
CI: red

The only non-green check is test (testnet, core,horizon, ...) with conclusion CANCELLED (ran 10:47→11:42 UTC, cancelled mid-"Run probes through wrapper" with no failed step). This is a live-testnet quickstart integration probe timeout/cancellation from external testnet flakiness — it cannot be affected by a 2-file dead-overlay-code removal with zero callers. All 40 other checks (Clippy, Test, Doctest, Build, CodeQL, and every local/pubnet quickstart variant) are green. The red is unrelated to the diff.

Action for /do Mode B: the code is already approved by both lenses and is unchanged since cycle 1 (head still 7a32106). Cycle 1's bounce did not produce a new commit — the same stale run's testnet job was merely re-attempted and cancelled again. This time, actually rebase onto current origin/main and push so a brand-new CI run is triggered; the fresh testnet probe should pass. If the testnet check is still CANCELLED/red after a genuine rebase+push, the next /review-pr tick will mark this blocked for operator attention (recurring infra flake on a non-diff-attributable check).

Routing back to ready-for-doing for /do Mode B.

[tomerweller]

Do: Mode B — Cycle 2 fresh CI push

Both reviewer lenses (Correctness + Parity) APPROVE and have across both prior cycles; there are no inline review comments to address. The sole blocker is the recurring CANCELLED test (testnet, core,horizon, …) quickstart job — a live-testnet integration probe timeout from external testnet flakiness, not attributable to this 2-file dead-overlay-code removal (zero callers, byte-identical framing).

Cycle 1's bounce only re-ran the stale run's jobs (no new commit → same run). This cycle:

• Re-checked out the PR head in a fresh ~/data worktree and rebased onto current origin/main (5ac2f16) — genuine no-op: the PR head's parent already is 5ac2f16, so there is nothing to rebase and no code to change.
• Re-ran local verification on the exact tree: cargo fmt --check clean, cargo clippy -p henyey-overlay --all-targets -- -D warnings clean, cargo test -p henyey-overlay all green (unit + integration + doctests).
• Pushed an empty commit (cdc5a72) — the only mechanism that triggers a brand-new CI run when the branch already sits on the main tip and the approved diff must stay unchanged. This gives the flaky testnet probe a fresh run.

Diff is unchanged from the twice-approved 7a32106. Ready for re-review; if the testnet check flakes again, the next /review-pr tick will mark this blocked per the cycle-2 note.

[tomerweller]

Pushed empty commit cdc5a72 to trigger a fresh CI run (testnet job was an unrelated infra flake). Diff unchanged from twice-approved 7a32106. Ready for re-review.

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Clean dead-code removal. The removed Encoder::encode impl and the Connection::split() / ConnectionSender / ConnectionReceiver chain have no callers; the live send path is unaffected and test coverage of the encode path is preserved.

Full review

Cycle 1 (no prior Correctness verdict on this PR) — complete class-labeled change-list. No blocking concerns found.

Verified:

• dead-code classification (correct): git grep at the PR head merged with origin/main finds zero references to ConnectionSender, ConnectionReceiver, Connection::split, or a .split() call on a Connection anywhere in crates/. The only prior references were the definitions themselves and their doc-links. The Encoder trait impl was only ever invoked from the codec's own unit tests.
• live send path unaffected: Connection::send (PR head) encodes via MessageCodec::encode_message(&message) and writes with self.framed.get_mut().write_all(&encoded) — a direct write to the underlying IO, not the Framed sink. It never depended on the Encoder/SinkExt path, so dropping the SinkExt import and the Encoder impl cannot regress it. send_encoded_batch likewise uses get_mut().write_all. The receive path keeps StreamExt (retained import) via self.framed.next().
• enforcement preserved: check_encode_size remains the single encode-side size bound and is still called from the retained encode_message; the MAX_MESSAGE_SIZE boundary unit test (check_encode_size(MAX+1).is_err()) still exercises it directly.
• test-verification gate (refactor/dead-code kind): the three tests that used the trait method codec.encode(msg, &mut buf) were rewritten to buf.extend_from_slice(&MessageCodec::encode_message(&msg).unwrap()), preserving round-trip encode→decode coverage against the path that actually ships. CI Test, Doctest, Clippy, and Build are all green, proving it compiles warning-free with no dead-code fallout.
• doc-comments updated: the check_encode_size and MessageCodec doc-comments were corrected to drop the now-removed "shared by the Encoder trait impl" wording. No doc drift.

No new class discovered.

[tomerweller]

🔍 Reviewer: Parity

Verdict: APPROVE

Summary: No change to the observable/interop surface. The removed Encoder::encode impl produced identical wire bytes to the retained encode_message, which is the path production actually uses; the decode path and framing are untouched.

Full review

Cycle 1 (no prior Parity verdict on this PR) — complete class-labeled change-list. No blocking parity concerns.

Observable-surface analysis (per docs/PARITY.md — SCP/overlay wire bytes):

• overlay wire bytes unchanged: the length-prefixed framing (4-byte big-endian record marker with the 0x80000000 final-fragment bit, followed by the XDR body) is emitted by the retained MessageCodec::encode_message, which Connection::send already used in production. The deleted Encoder::encode impl computed the exact same bytes but was dead (test-only). Removing it cannot change any byte on the wire.
• decode path untouched: impl Decoder for MessageCodec and its size-bound rejection (mirroring TCPPeer.cpp:690-701) are unchanged, so inbound framing/interop is unaffected.
• size bound preserved: check_encode_size (the MAX_MESSAGE_SIZE enforcement point) is retained and still invoked from encode_message, so the encode-side rejection semantics stellar-core relies on are intact.
• removed split() chain was never wired to any peer/overlay flow — no SCP/flood/auth path referenced ConnectionSender/ConnectionReceiver, so no sequencing or wire behavior is affected.

Internal-only aspects (explicitly allowed, not flagged): removal of SinkExt/BufMut imports and the sink-based abstraction are pure internal architecture cleanup.

No new class discovered.

[tomerweller]

Do: Mode B — Cycle 3 CI re-trigger (environmental testnet flake)

Both reviewer lenses (Correctness + Parity) APPROVE across all three cycles, and there are no inline review comments to address. The sole blocker remains the test (testnet, core,horizon, …) quickstart job.

This cycle's failure is environmental, not the diff:

• Job 97210713060 (run 32643520950) hit the 25-minute job-level hard timeout in "Run probes through wrapper" (14:41 → 15:08 UTC), while the individual probes were soft-skipping on exit 124: horizon-core-up soft-skipped (SOFT-SKIP: testnet sync probe timed out (environmental, not a henyey failure), CI: Quickstart testnet shard cancelled on ≥2 consecutive main commits — probe wrapper hangs at step 8 (systematic, not flaky) #3272), then horizon-ingesting sat on "Waiting for Horizon to start ingesting" against live testnet until the runner was killed by the action timeout.
• Root cause is live-testnet connectivity/ingestion latency — a 2-file dead-overlay-code removal (zero callers, byte-identical framing) cannot affect a live-testnet quickstart probe.
• All 40 other checks (Clippy, Test, Doctest, Build, Format, CodeQL, and every local/pubnet quickstart variant) are green.

Action: PR head (cdc5a72) already sits on the current origin/main tip (5ac2f16), so a rebase is a genuine no-op and the twice-approved diff must stay unchanged. Re-ran the failed testnet job via gh run rerun --failed to give the flaky probe a fresh runner + fresh testnet connectivity. If the testnet check flakes again, the next /review-pr tick should treat this per the environmental-flake / cycle-cap policy.

Ready for re-review.

[tomerweller]

Environmental testnet-probe flake re-triggered; code unchanged and twice-approved by both lenses. Ready for re-review.

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Clean, self-contained dead-code removal matching issue #3784. The removed Encoder impl and the split()/ConnectionSender/ConnectionReceiver chain have zero callers; the three round-trip tests were correctly re-sourced from the real send path with byte-identical framing. CI Test/Clippy/Build/Doctest are green.

Full review

Cycle N (code unchanged since the twice-approved head; current head is the empty re-trigger commit cdc5a72). Prior Correctness concern classes: none. No new class discovered.

Independently re-verified against the PR head:

• dead-code classification (correct): workspace grep (crates/**/*.rs) for ConnectionSender, ConnectionReceiver, .split(), Encoder<AuthenticatedMessage>, and codec.encode( finds zero references outside the two definition files. The only codec.encode( uses are the three #[cfg(test)] call sites in codec.rs, which the diff rewrites. lib.rs never re-exported the removed symbols.
• test rewrite is byte-identical (correct): the removed Encoder::encode did to_xdr → check_encode_size → put_u32(len | 0x80000000) (big-endian) + body. MessageCodec::encode_message does to_xdr → check_encode_size → (len | 0x80000000u32).to_be_bytes() + body. put_u32 is big-endian, so the two produce identical bytes; the is_last_fragment / streaming assertions are preserved.
• test coverage (preserved): kind: for this issue is a refactor/dead-code removal — no new pub surface added, and the pre-existing encode/decode round-trip tests remain green in CI. Coverage of the live encode path is unchanged.
• imports / docs: the now-unused Encoder, BufMut, SinkExt imports are dropped, and the check_encode_size / MessageCodec doc comments are refreshed to match reality. Clippy (global -Dwarnings) is green, confirming no unused-import or dead-code residue.

No blocking concerns.

[tomerweller]

🔍 Reviewer: Parity

Verdict: APPROVE

Summary: No observable/interop-surface impact. The removed Encoder::encode produced byte-identical framing to the retained encode_message (the path production actually uses) and had zero callers; the decode path and length-prefixed framing are untouched. Overlay wire bytes are unchanged.

Full review

Cycle N — parity lens per docs/PARITY.md (observable surface only: SCP/overlay wire bytes, hashes, tx result/meta XDR, history archive format, HTTP/RPC/CLI, crypto). Prior Parity concern classes: none. No new class discovered.

• overlay wire bytes unchanged: the deleted Encoder<AuthenticatedMessage> impl and the live encode_message shared the same size-bound (check_encode_size) and emitted the identical 4-byte big-endian record-marking prefix (len | 0x80000000) followed by the XDR body. Removing the dead Encoder path cannot change what goes on the wire.
• receive path untouched: Decoder::decode and decode_message are unmodified; framing/parsing behavior is byte-for-byte unchanged.
• removed sink/stream chain was dead: Connection::split() / ConnectionSender / ConnectionReceiver were never re-exported and had no callers, so no production send/receive sequencing changes.
• internal-only surface: the removal touches internal architecture/helpers only — explicitly allowed to deviate per docs/PARITY.md. No hashes, tx result/meta XDR, history format, or RPC/CLI contracts are affected.

No blocking parity concerns.

[tomerweller]

Review: Bounce-Back Cycle 2

Reason: CI failed (unrelated, will rebase)

Reviewer A: APPROVE — clean dead-code removal; re-verified zero external callers of the removed Encoder/split()/ConnectionSender/ConnectionReceiver symbols and byte-identical test rewrite onto the real encode_message send path.
Reviewer B: APPROVE (Parity) — no observable/overlay-wire-byte impact; removed Encoder::encode was byte-identical to and dead relative to encode_message; decode path untouched.
External reviewers: none blocking (tomerweller left two COMMENTED re-review notes only).
CI: red

The only non-green check is test (testnet, core,horizon, test_core.go test_horizon_up.go test_horizon_core_up.go ...) with conclusion CANCELLED (job 97240650333, ran 18:32→19:27 UTC ~55m before cancellation). This is the stellar testnet integration probe — it exercises the upstream core/horizon Go binaries against live testnet and has no relationship to this overlay dead-code removal (only crates/overlay/src/{codec,connection}.rs changed). Every required CI check (Format, Clippy, Test, Doctest, Build, CodeQL, Quickstart harness) is green.

This is the same environmental testnet-probe cancellation that produced the two prior unrelated-red cycles; the empty re-trigger commit cdc5a72 reproduced it. Routing back to ready-for-doing for /do Mode B to rebase onto the latest origin/main (which may carry a testnet-CI infra fix) and re-push. If the testnet probe remains cancelled after the rebase, the next /review-pr should escalate to blocked for operator investigation of the testnet CI infra — no code change on this PR can fix it.