Skip to content

Remove ringbuffer usage for node-to-node traffic - #8446

Merged
Amaury Chamayou (achamayou) merged 3 commits into
mainfrom
agents/node-to-node-ringbuffer-removal
Oct 7, 2026
Merged

Amaury Chamayou (achamayou) merged 3 commits into
mainfrom
agents/node-to-node-ringbuffer-removal

Conversation

@eddyashton

@eddyashton Eddy Ashton (eddyashton) commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Motivation

Stacked on #8445 as the next step of the ringbuffer-removal plan (#8405 has landed). Node-to-node traffic still crosses the host-enclave ringbuffer as four messages (associate_node_address, node_outbound, node_inbound, close_node_outbound). This PR replaces them with typed interfaces.

Ready for review. The agreed design uses the existing shared task system without a critical-task class or dedicated consensus executor. Repeated green CI runs since that simplification, including the latest head, support proceeding with this design.

AdminMessage::tick remains on the ringbuffer; removing it and the remaining ringbuffer infrastructure is follow-up work.

Implementation summary

  • Add a typed, thread-safe AbstractNodeTransport / NodeInboundHandler (src/node/node_transport.h), implemented by the host's NodeConnections. Channels, the channel manager, NodeState and Enclave take the transport instead of ringbuffer writer factories. Libuv-loop constraints stay inside the host transport.
  • Outbound operations still go through the ledger lane, so AppendEntries frames carry exactly (prev_idx, idx] after earlier appends. Channels submit under their lock, preserving per-peer key-exchange and nonce order. The wire format is unchanged.
  • Inbound frames are copied into owned buffers and run, with node ticks and stop notices, on one ordinary OrderedTasks lane (NodeIngress) on the main job board. These operations remain FIFO and mutually exclusive regardless of which worker executes them.
  • The dispatch thread stays ingress-only, as after Raise worker_threads default to 1, make dispatch thread ingress-only #8404: it submits ticks and stop notices to NodeIngress and executes no tasks. There are no task-system changes in this PR.
  • Remove the four message declarations, serialisers and dispatcher handlers, and the enclave's now-unused writer factories.
  • The host drains the ledger lane before job-board shutdown, then explicitly closes node sockets because the enclave retains the transport.

Safety and compatibility

  • Accepted scheduling change: node ingress previously ran directly on the dispatch thread, independently of worker tasks. It now queues as an ordinary task and needs a free worker. Blocking tasks occupying every worker can delay consensus messages and ticks and cause elections. FIFO, mutual exclusion and protocol validation are preserved; no consensus safety rules are changed. CI coverage is reassuring, not proof that worker-saturation stalls are impossible. A separate consensus thread/job board is a possible follow-up if operational evidence warrants it, not part of this PR.
  • A frame declaring a size above memory.max_msg_size now closes that connection before buffering, rather than throwing in the libuv callback and terminating the node. The inbound backlog remains unbounded, as before.
  • Malformed peer input is still dropped per message; framing errors close the connection.
  • No wire-format, ledger-format, public API or configuration change. Mixed-version peers continue to use the same protocol.
  • Heartbeats still queue behind ledger IO; reordering outbound Raft traffic is out of scope. Forwarding is deprecated and its removal is separate work.

Validation: focused unit tests cover ingress ownership/order/shutdown, malformed input, channel key exchange and nonce ordering, connection lifecycle and AppendEntries attachment. Local host/app builds and formatting/include/ASCII checks pass. On head 1df49ba61, all regular CI buckets, long Debug/Release tests, Long Shuffled/LTS/Snmalloc, ASAN and TSAN pass. A fresh independent adversarial review found no significant issues. No changelog entry is included, as agreed for this internal change.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

It changes consensus ingress scheduling, cross-thread transport ordering, and shutdown behavior, warranting final human review.

Review effort: Balanced
Findings: None

What changed in this PR

Replaces node-to-node ringbuffer messages with a typed, thread-safe transport while preserving framing and ordering.

Changes:

  • Introduces AbstractNodeTransport and host implementation.
  • Serializes inbound traffic, ticks, and stop notices through NodeIngress.
  • Adds transport ordering, framing, lifecycle, and concurrency tests.

Custom instructions used

  • .github/copilot-instructions.md
  • .github/instructions/reviewing.instructions.md
  • .github/skills/testing/SKILL.md
File Description
src/​node/​test/​node_inbound_message.cpp Tests ingress ordering, ownership, shutdown, and serialization.
src/​node/​test/​channels.cpp Migrates channel tests to typed transports and adds ordering coverage.
src/​node/​node_types.h Removes node ringbuffer message declarations.
src/​node/​node_transport.h Defines typed transport interfaces.
src/​node/​node_to_node_channel_manager.h Routes channel management through the transport.
src/​node/​node_state.h Injects the transport and accepts typed inbound frames.
src/​node/​node_inbound_message.h Adds the serialized NodeIngress task lane.
src/​node/​channels.h Sends channel traffic directly through the transport.
src/​host/​test/​node_connections.cpp Expands framing, ordering, malformed-input, and shutdown tests.
src/​host/​run.cpp Creates and shuts down the host transport.
src/​host/​node_connections.h Implements typed transport, framing, limits, and lifecycle handling.
src/​host/​ledger_subsystem.h Documents transport use of mutation ordering.
src/​enclave/​main.cpp Passes the transport into enclave creation.
src/​enclave/​entry_points.h Extends the enclave creation interface.
src/​enclave/​enclave.h Connects transport ingress to node processing.
doc/​architecture/​threading.rst Documents shared-worker ingress scheduling.
doc/​architecture/​tcp_internals.rst Updates node transport architecture documentation.
CMakeLists.txt Links ingress tests with the task library.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@eddyashton
Eddy Ashton (eddyashton) force-pushed the agents/node-to-node-ringbuffer-removal branch from 1df49ba to 5346afd Compare October 7, 2026 14:24
Base automatically changed from agents/periodic-task-implementation-ccf to main October 7, 2026 14:39
Replace the associate_node_address, node_outbound, node_inbound and
close_node_outbound ringbuffer messages with a typed, thread-safe node
transport implemented by the host's NodeConnections. Outbound sends stay
ordered behind earlier ledger mutations, preserving AppendEntries entry
attachment and per-peer nonce order. Inbound frames are copied into owned
buffers and run, with ticks and stop notices, on one critical OrderedTasks
lane.

Add a critical task class to JobBoard: every worker runs critical tasks
first, and the enclave dispatch thread now runs only critical tasks, so
blocking general tasks cannot delay consensus. Frames declaring a size
above memory.max_msg_size close the connection instead of terminating the
node. The host closes node sockets explicitly at shutdown, since the
enclave retains the transport.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Drop TaskClass::Critical and the dispatch thread's critical-task loop. The
node ingress lane is now an ordinary OrderedTasks on the main job board,
and the dispatch thread is ingress-only again, as after #8404.

Inbound node messages, ticks and stop notices remain FIFO and mutually
exclusive on their lane, but now queue behind other ready tasks and need a
free worker. Blocking general tasks occupying every worker can therefore
delay consensus. Measure this in CI before considering a dedicated
consensus executor.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@eddyashton
Eddy Ashton (eddyashton) force-pushed the agents/node-to-node-ringbuffer-removal branch from 5346afd to f07d8d9 Compare October 7, 2026 14:39
@achamayou
Amaury Chamayou (achamayou) merged commit d2aa5fd into main Oct 7, 2026
12 checks passed
@achamayou
Amaury Chamayou (achamayou) deleted the agents/node-to-node-ringbuffer-removal branch October 7, 2026 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-long-test Run Long Test job

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants