feat(network): define NetworkTransport trait + TcpTransport (7/n)

[shreyan-gupta]

• NetworkTransport trait — 5-method dyn-safe trait:
  • send_message(tier, peer_id, msg), broadcast_message(tier, msg)
  • connect_to_peer(clock, peer_info, tier) -> ConnectHandle (async, default no-op)
  • disconnect_peer(peer_id, ban_reason) (default no-op), shutdown() (default no-op)
• transport_info() is added in PR 9
• TcpTransport — production impl wrapping existing Pool-based code
• ConnectHandle — implements Future<Output = Result<(), ConnectError>>
• PeerManagerActor now holds Arc<dyn NetworkTransport> alongside Arc<NetworkState>

[codecov]

Codecov Report

❌ Patch coverage is 67.54386% with 74 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.46%. Comparing base (a9a453b) to head (864d458).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
...ain/network/src/peer_manager/peer_manager_actor.rs	37.73%	33 Missing ⚠️
...hain/network/src/peer_manager/network_transport.rs	4.76%	20 Missing ⚠️
...hain/network/src/peer_manager/network_state/mod.rs	84.00%	11 Missing and 1 partial ⚠️
chain/network/src/peer_manager/tcp_transport.rs	57.89%	8 Missing ⚠️
.../network/src/peer_manager/network_state/routing.rs	94.73%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #15575      +/-   ##
==========================================
- Coverage   69.48%   69.46%   -0.03%     
==========================================
  Files         940      942       +2     
  Lines      214797   214963     +166     
  Branches   214797   214963     +166     
==========================================
+ Hits       149262   149322      +60     
- Misses      59610    59720     +110     
+ Partials     5925     5921       -4     

Flag	Coverage Δ
pytests-nightly	1.14% <0.00%> (-0.01%)	⬇️
unittests	68.91% <67.54%> (-0.02%)	⬇️
unittests-nightly	68.99% <67.10%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pugachAG]

given that only T2 broadcast is supported I suggest we just remove tier argument

[pugachAG]

does it make sense to introduce transport as a field in NetworkState instead of passing to its functions? seems like it feels here better than in PeerActor

[shreyan-gupta]

I had tried that but I think it's not possible due to circular dependencies and ownership issues. I can take a deeper look.

[shreyan-gupta]

The transport that NetworkState methods receive comes from PeerManagerActor. PMA constructs it via TcpTransport::new(state.clone()) — so NetworkState is built first, then TcpTransport wraps an Arc of it (it needs Arc<NetworkState> to spawn PeerActors). PMA holds both and passes transport into the methods that need it.

Storing transport on NetworkState would close a ref cycle (NetworkState → transport → state) and leak both. We could break the cycle with Weak<dyn NetworkTransport> on NetworkState (via Arc::new_cyclic), but every call would then need self.transport.upgrade().expect(...).<method>() — extra boilerplate for an upgrade that can only fail during teardown.

I'd leave transport as a parameter, but happy to revisit if you have a structuring in mind that I'm not seeing.

🤖 Response from Claude Code on behalf of Shreyan

[pugachAG]

that makes sense, but in this case I find it weird that NetworkState both need NetworkTransport instance and also itself implements it.

[shreyan-gupta]

Let me get back to this problem after all the refactoring. I did have some ideas of introducing an interface for the upward communication from transport layer to the network graph layer (network state) and this can fall as a part of that.

But that said, better to unblock as is for now.

[Copilot]

Pull request overview

Introduces a transport abstraction layer in chain/network by defining a dyn-safe NetworkTransport trait and wiring message delivery paths to use it, with an initial production implementation (TcpTransport) wrapping the existing pool-based networking.

Changes:

• Add NetworkTransport trait and ConnectHandle future type.
• Implement TcpTransport as the production transport using existing NetworkState tier pools.
• Thread Arc<dyn NetworkTransport> through PeerManagerActor, PeerActor, and NetworkState message/broadcast APIs; update affected tests/helpers.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
chain/network/src/peer_manager/tests/tier1.rs	Update tier1 tests to pass a NetworkTransport when sending messages.
chain/network/src/peer_manager/testonly.rs	Update test-only ping helper to pass a NetworkTransport.
chain/network/src/peer_manager/tcp_transport.rs	Add production TcpTransport implementing NetworkTransport.
chain/network/src/peer_manager/peer_manager_actor.rs	Store Arc<dyn NetworkTransport> in PMA and use it for shutdown + routing sends.
chain/network/src/peer_manager/network_transport.rs	Define NetworkTransport, ConnectHandle, and ConnectError.
chain/network/src/peer_manager/network_state/routing.rs	Route routing-table broadcasts and account announcements via NetworkTransport.
chain/network/src/peer_manager/network_state/mod.rs	Update message-send APIs to take &dyn NetworkTransport / Arc<dyn ...>; pass transport through connect/disconnect flows.
chain/network/src/peer_manager/mod.rs	Export new network_transport and tcp_transport modules.
chain/network/src/peer/testonly.rs	Update peer test harness to provide a NetworkTransport to PeerActor::spawn.
chain/network/src/peer/peer_actor.rs	Add transport field to PeerActor and thread transport through edge/routing/account flows.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

fix_local_edges creates a new TcpTransport inside NetworkState to satisfy the add_edges(..., transport) API. This couples NetworkState to the production transport and will break the goal of having NetworkState be transport-agnostic (e.g. for a future TestLoop transport). Consider changing fix_local_edges to accept/capture an Arc<dyn NetworkTransport> from the caller (PeerManagerActor) instead of constructing TcpTransport here, and remove the TcpTransport dependency from network_state.

[Copilot]

A TcpTransport instance is created for the listener loop here, but another TcpTransport::new(state.clone()) is created later when constructing PeerManagerActor. Since the PR description notes that Pools will move into TcpTransport in a follow-up, having multiple transport instances risks splitting/duplicating transport-owned state. Prefer creating a single Arc<dyn NetworkTransport> once and cloning it into both the listener loop and the actor struct.

[Copilot]

spawn_and_handshake constructs a fresh TcpTransport internally, which hard-codes the production transport and also increases the number of TcpTransport instances created per connection attempt. If TcpTransport becomes stateful (as planned when Pools move into it), this pattern can cause subtle divergence. Consider taking Arc<dyn NetworkTransport> as a parameter (or otherwise sourcing it from the caller) so this helper can work with non-TCP transports and reuse the shared transport instance.

[Copilot]

TcpTransport exposes its state field as pub, which makes it easy for other modules to bypass the NetworkTransport abstraction and reach directly into NetworkState/Pools. Since this type is meant to encapsulate transport behavior, consider making the field private (and exposing only the trait methods), to keep the abstraction boundary intact.

Suggested change

	pub state: Arc<NetworkState>,
	state: Arc<NetworkState>,