[LP-DATA] Mix simulator, pipeline traits#6712
Open
simonwicky wants to merge 11 commits intodevelopfrom
Open
Conversation
simonwicky
requested review from
jstuczyn and
mfahampshire
and removed request for
jstuczyn
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughAdds a new generic Lewes Protocol pipeline crate ( ChangesWorkspace & Serialization
LP Data Library (common/nym-lp-data)
Simulator Implementation (nym-mix-sim)
Sequence DiagramsequenceDiagram
participant User
participant Driver as MixSimDriver
participant Client as BaseClient
participant Node as BaseNode
participant Net as UDP_Network
User->>Driver: run / tick(timestamp)
Driver->>Client: tick(timestamp) — app incoming
Client->>Client: recv_from_app() / process(payload)
Client->>Client: enqueue AddressedTimedData(timestamp)
Driver->>Node: tick_incoming()
Node->>Net: non-blocking recv_packet()
Node->>Node: buffer packets_to_process
Driver->>Node: tick_processing(timestamp)
Node->>Node: processing_node.process(packet)
Node->>Node: schedule outputs (timestamp +/- delay)
Driver->>Client: tick(timestamp) — outgoing
Client->>Client: send queued packets with pkt.timestamp <= timestamp
Client->>Net: send_to_node(first_hop, bytes)
Driver->>Node: tick_outgoing(timestamp)
Node->>Net: send_to_node(next_hop, bytes)
Driver->>Client: tick(timestamp) — mix incoming
Client->>Net: recv_from_mix() non-blocking
Client->>Client: unwrap(packet, timestamp) -> Maybe plaintext
Client->>Client: log received messages
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/nym-lp-data/src/clients/traits.rs`:
- Around line 143-156: In chunk_size(), replace the unchecked arithmetic with
checked operations: compute let base =
self.frame_size().checked_mul(self.nb_frames()) and if None return 0 (or a safe
default); then if input_options.routing_security() apply base =
base.and_then(|b| b.checked_sub(<Self as
RoutingSecurity<_,_,_>>::OVERHEAD_SIZE)) and if input_options.reliability()
apply base = base.and_then(|b| b.checked_sub(<Self as
Reliability<_,_,_>>::OVERHEAD_SIZE)); finally return base.unwrap_or(0). Update
the chunk_size function to use these checked_mul/checked_sub calls and the
referenced symbols (chunk_size, frame_size, nb_frames,
RoutingSecurity::OVERHEAD_SIZE, Reliability::OVERHEAD_SIZE,
input_options.routing_security, input_options.reliability) so over/underflow
cannot produce wrapped sizes.
In `@common/nym-lp-data/src/common/traits.rs`:
- Around line 126-130: The frame_size() implementation can underflow when
packet_size() is smaller than the combined OVERHEAD_SIZEs; update the frame_size
function to perform checked subtraction (e.g., use checked_sub on packet_size
then checked_sub again with the other OVERHEAD_SIZE) and fail early with a clear
error (panic/expect or return a Result) instead of letting wrapping produce a
huge usize; reference the function frame_size and the constants <Self as
Transport<Ts, Pkt, NdId>>::OVERHEAD_SIZE and <Self as Framing<Ts, Opts,
NdId>>::OVERHEAD_SIZE when constructing the error message so it’s clear which
overheads caused the underflow.
In `@nym-mix-sim/README.md`:
- Around line 36-38: Update the three fenced code blocks that show commands to
include a language identifier; specifically change the fence opening for the
blocks containing "cargo run --bin nym-mix-sim -- init-topology [OPTIONS]",
"cargo run --bin nym-mix-sim -- run [OPTIONS]", and "cargo run --bin mix-client
-- --src <ID> --dst <ID> [--topology <PATH>]" so they start with ```bash instead
of just ```, ensuring MD040 is satisfied.
In `@nym-mix-sim/src/client/mod.rs`:
- Around line 229-240: The loop over inputs calls processing_client.process(...)
for each (dst, payload), which re-triggers timer-driven stages inside
ClientWrappingPipeline::process() and causes duplicate reliable_encode/obfuscate
behavior; instead, batch all app datagrams for the current tick into a single
call that represents the tick entry point. Concretely: accumulate/collect all
payloads in inputs and call processing_client.process(...) once with a combined
or iterator-aware API (or add a new process_tick(...) on
processing_client/ClientWrappingPipeline that runs the per-tick
reliable_encode(None,..)/obfuscate(None,..) and then feeds all app messages),
then extend outgoing_queue with that single return; ensure you no longer call
processing_client.process for each (dst, payload) in the for loop.
In `@nym-mix-sim/src/client/simple.rs`:
- Around line 64-70: The constructor SimpleClient::new must validate the
simple-mode hop assumption (that next-hop node id 0 exists) and error early if
absent: before creating SimpleProcessingClient or calling
BaseClient::with_pipeline, check the supplied TopologyClient/Directory/topology
for a node with id 0 (the pipeline’s hardcoded next hop) and return an
anyhow::Error if not found; apply the same guard to the other SimpleClient
constructor variant that builds the pipeline (the block creating
SimpleProcessingClient / calling BaseClient::with_pipeline) so sends don’t
silently fail when node 0 is missing.
In `@nym-mix-sim/src/driver/mod.rs`:
- Around line 124-135: The run(...) method accepts display_state but doesn't
forward it to run_automatic(...) causing automatic mode ticks to always use
false; update run_automatic to accept a display_state: bool parameter and change
the call in run(...) to self.run_automatic(start_tick, tick_duration_ms,
display_state). Also update run_automatic's internal uses (and any helper
functions it calls that decide whether to display state) to use the passed
display_state instead of the hardcoded false; repeat the same change for the
other run/run_automatic call site referenced (the second occurrence) so both
invocation paths propagate display_state correctly.
In `@nym-mix-sim/src/driver/sphinx.rs`:
- Around line 29-36: Reject empty topologies early in the constructors: in pub
fn new (and the other constructor that currently accepts
topology.nodes.is_empty()), add a guard after deserializing Topology that checks
if topology.nodes.is_empty() and return an Err(anyhow::anyhow!("Topology
contains no nodes") ) (or equivalent anyhow::Context) so construction fails fast
with a clear message instead of allowing later panics during route sampling;
reference the Topology value and the constructors pub fn new and the other
constructor handling Topology to locate where to add the check.
In `@nym-mix-sim/src/main.rs`:
- Around line 100-101: The range expression nodes..nodes + clients used to build
client_list can overflow (u8) and produce an invalid/empty range; replace the
plain addition with a checked or saturating addition before constructing the
range (e.g., use nodes.checked_add(clients).unwrap_or_else(...) or cast to a
larger integer type and validate) so you either abort with a clear error or
clamp the upper bound; update the client_list construction to use the
validated/safe upper bound and keep references to the same variables
(client_list, nodes, clients) so the rest of the code remains unchanged.
In `@nym-mix-sim/src/node/simple.rs`:
- Around line 95-110: mix() currently forwards to self.id + 1 which assumes
sequential IDs; instead consult the topology/directory to choose the next hop so
you don't forward to a non-existent NodeId. Change the routing in the mix
function (the code constructing PipelinePayload via PipelinePayload::new) to
lookup the next hop using the Directory/topology API (or a configured routing
table) given self.id and fail/return an error or drop/metrics if no next hop
exists; ensure you reference SimpleMessage, SimpleInputOptions and NodeId when
replacing the hard-coded self.id + 1.
In `@nym-mix-sim/src/node/sphinx.rs`:
- Around line 116-120: Replace the panic-causing unwrap on
SphinxPacket::from_bytes(&payload.data) with proper error handling: call
SphinxPacket::from_bytes and match on the Result, logging the deserialization
error (include payload metadata if available) and aborting processing for that
SimMixPacket instead of panicking; ensure the logging uses the node's logger
context and simply returns/continues (drops the packet) on Err, and only
proceeds with the successfully parsed SphinxPacket on Ok so downstream code no
longer assumes from_bytes cannot fail.
In `@nym-mix-sim/src/packet/simple.rs`:
- Around line 162-171: SimpleFrame::try_from_bytes currently only checks length
and accepts any bytes with the correct minimal length; update it to verify that
the leading bytes match the expected magic header (Self::HEADER) before slicing
the payload. In the SimpleFrame::try_from_bytes function, compare
bytes[..Self::HEADER.len()] to Self::HEADER and return an
Err(anyhow::anyhow!(...)) when they differ (with a concise message like "Invalid
SimpleFrame header"), otherwise proceed to extract data =
bytes[Self::HEADER.len()..].to_vec() and return Ok(SimpleFrame { data }).
In `@nym-mix-sim/src/packet/sphinx.rs`:
- Around line 170-173: try_recover_first_hop_packet currently indexes b[0] and
will panic on empty input; guard against truncated SURB buffers by validating
the slice length before indexing. In try_recover_first_hop_packet, check that b
is non-empty (e.g., via b.is_empty() or b.get(0).ok_or(...)) and return an
anyhow::Result error when the buffer is empty or otherwise too short, then
proceed to construct NodeId from the first byte and call
SimMixPacket::try_from_bytes(&b[1..]) as before; ensure the error message
clearly indicates a malformed/truncated SURB buffer.
- Around line 196-201: The is_surb_ack function currently only checks for the
marker prefix length and can return true for payloads shorter than the full
16-byte ACK header; update is_surb_ack to first verify data.len() >= 16 (the
full ACK header length used later by SphinxClientUnwrapping when it accesses
plaintext[8..16]) and only then compare the marker bytes (Self::MARKER) against
the corresponding prefix; ensure you reference is_surb_ack and Self::MARKER and
guard against truncated payloads so the later plaintext[8..16] slice cannot
panic.
In `@nym-mix-sim/src/topology/directory.rs`:
- Around line 51-52: The Directory::client method currently accepts a NodeId but
the clients map is keyed by ClientId, causing a type-domain mismatch; change the
method signature to take a ClientId (e.g., pub fn client(&self, id: ClientId) ->
Option<&SocketAddr>) and update the internal lookup to use self.clients.get(&id)
(and rename the parameter if needed). After changing the signature, update all
callers to pass a ClientId instead of a NodeId so the API remains type-safe and
consistent with the clients field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6729ba99-6209-4d43-ab4a-0326679ed82a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
Cargo.tomlcommon/crypto/src/asymmetric/x25519/serde_helpers.rscommon/nym-lp-data/Cargo.tomlcommon/nym-lp-data/README.mdcommon/nym-lp-data/src/clients/driver.rscommon/nym-lp-data/src/clients/helpers.rscommon/nym-lp-data/src/clients/mod.rscommon/nym-lp-data/src/clients/traits.rscommon/nym-lp-data/src/clients/types.rscommon/nym-lp-data/src/common/helpers.rscommon/nym-lp-data/src/common/mod.rscommon/nym-lp-data/src/common/traits.rscommon/nym-lp-data/src/lib.rscommon/nym-lp-data/src/mixnodes/mod.rscommon/nym-lp-data/src/mixnodes/traits.rscommon/nym-lp-data/tests/integration/common/mod.rscommon/nym-lp-data/tests/integration/main.rscommon/nym-lp/src/packet/header.rsnym-mix-sim/Cargo.tomlnym-mix-sim/README.mdnym-mix-sim/src/bin/mix_client.rsnym-mix-sim/src/client/mod.rsnym-mix-sim/src/client/simple.rsnym-mix-sim/src/client/sphinx/mod.rsnym-mix-sim/src/client/sphinx/poisson_cover_traffic.rsnym-mix-sim/src/client/sphinx/surb_acks.rsnym-mix-sim/src/driver/mod.rsnym-mix-sim/src/driver/simple.rsnym-mix-sim/src/driver/sphinx.rsnym-mix-sim/src/lib.rsnym-mix-sim/src/main.rsnym-mix-sim/src/node/mod.rsnym-mix-sim/src/node/simple.rsnym-mix-sim/src/node/sphinx.rsnym-mix-sim/src/packet/mod.rsnym-mix-sim/src/packet/simple.rsnym-mix-sim/src/packet/sphinx.rsnym-mix-sim/src/topology/directory.rsnym-mix-sim/src/topology/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
nym-mix-sim/src/topology/directory.rs (1)
64-68: ⚡ Quick winAvoid per-call allocation in
random_next_hopto match the lock-free/allocation-free design note.Line 67 builds a fresh
Vec<NodeId>vianode_ids()on every call. Sampling directly fromself.nodes.keys()avoids that allocation on hot routing paths.Suggested patch
pub fn random_next_hop(&self, rng: &mut impl rand::Rng) -> NodeId { // SAFETY: The directory always contains at least one node in a valid simulation. #[allow(clippy::unwrap_used)] - *self.node_ids().choose(rng).unwrap() + *self.nodes.keys().choose(rng).unwrap() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nym-mix-sim/src/topology/directory.rs` around lines 64 - 68, The method random_next_hop currently calls node_ids() which allocates a Vec<NodeId> per call; change it to sample directly from the directory's map keys to avoid allocations by using the IteratorRandom helper on self.nodes.keys() (e.g. import rand::seq::IteratorRandom and call self.nodes.keys().choose(rng)), dereference the chosen &NodeId to return NodeId, and keep the existing SAFETY/unwrap assertion (or use expect with the same message) to preserve the guarantee that the directory is non-empty; this removes the per-call allocation from node_ids() while keeping semantics for random_next_hop.common/nym-lp-data/src/clients/traits.rs (1)
184-213: ⚡ Quick winLift the per-tick idempotency contract into the trait docs.
The "this should be a no-op since it already has been called with the same timestamp" property is load-bearing: any
Reliability/Obfuscationimpl that re-emits state on a repeat call with the sametimestampwill produce duplicate retransmissions or duplicate cover packets per tick. Right now this contract lives only in inline comments insideprocess; please document it onReliability::reliable_encodeandObfuscation::obfuscateso implementors can't get it wrong.Alternatively, restructure
processto call each stage exactly once per tick (passingOption<chunk>per item) so the contract isn't needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/nym-lp-data/src/clients/traits.rs` around lines 184 - 213, Document the per-tick idempotency contract directly on the trait methods: add clear doc comments to Reliability::reliable_encode and Obfuscation::obfuscate stating that each method must be idempotent per timestamp (calling with the same timestamp more than once must be a no-op and must not re-emit previously produced chunks), explain the meaning/role of the Option<Chunk> parameter (None is used to trigger any per-tick maintenance like catching retransmissions or emitting cover traffic), and note callers may call the method both with Some(chunk) and later with None for the same timestamp so implementations must guard against duplicate output; alternatively, if you prefer behavior instead of docs, refactor process to ensure each stage (reliable_encode and obfuscate) is invoked exactly once per tick per stage (passing Option<Chunk>) so no idempotency contract is required.common/nym-lp-data/src/lib.rs (1)
70-89: 💤 Low valueOptional: relax
FnMuttoFnOncefor*_transformclosures.
data_transformandts_transforminvokeopexactly once, soFnOnceis the more accurate (and looser) bound; this lets callers pass non-FnMutmoveclosures that consume captured state.Proposed change
- pub fn data_transform<F, Nd>(self, mut op: F) -> TimedData<Ts, Nd> + pub fn data_transform<F, Nd>(self, op: F) -> TimedData<Ts, Nd> where - F: FnMut(D) -> Nd, + F: FnOnce(D) -> Nd, { @@ - pub fn ts_transform<F>(self, mut op: F) -> Self + pub fn ts_transform<F>(self, op: F) -> Self where - F: FnMut(Ts) -> Ts, + F: FnOnce(Ts) -> Ts, {(and analogous changes on
PipelineData::data_transform/ts_transform)Also applies to: 128-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/nym-lp-data/src/lib.rs` around lines 70 - 89, The transform methods (TimedData::data_transform and TimedData::ts_transform, and the analogous PipelineData::data_transform/ts_transform) use a FnMut bound and a mut parameter even though the closure is invoked exactly once; change the generic bounds from F: FnMut(...) -> ... to F: FnOnce(...) -> ... and remove the unnecessary mut on the op parameter (e.g., `mut op: F` -> `op: F`) so single-use, consuming move closures are accepted; update both TimedData and PipelineData variants accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/nym-lp-data/src/common/traits.rs`:
- Around line 124-126: Fix the typo in the comment: change "accomodate" to
"accommodate" in the comment block that begins "// IMPORTANT NOTE : This fn can
be not constant..." in common/nym-lp-data/src/common/traits.rs so the comment
reads "must be able to accommodate the different overhead."
In `@nym-mix-sim/src/topology/directory.rs`:
- Around line 86-97: The From<&Topology> implementation for Directory must
enforce that the topology has at least one node to avoid later unwrap() panics
in random_next_hop/random_route; at the start of fn from(value: &Topology) ->
Self add an explicit check like assert!(!value.nodes.is_empty(), "Topology must
contain at least one node when constructing Directory") (or panic! with the same
clear message) so invalid Topology values fail fast, then proceed to create
Directory via Directory::default(), populate directory.nodes and
directory.clients as before.
---
Nitpick comments:
In `@common/nym-lp-data/src/clients/traits.rs`:
- Around line 184-213: Document the per-tick idempotency contract directly on
the trait methods: add clear doc comments to Reliability::reliable_encode and
Obfuscation::obfuscate stating that each method must be idempotent per timestamp
(calling with the same timestamp more than once must be a no-op and must not
re-emit previously produced chunks), explain the meaning/role of the
Option<Chunk> parameter (None is used to trigger any per-tick maintenance like
catching retransmissions or emitting cover traffic), and note callers may call
the method both with Some(chunk) and later with None for the same timestamp so
implementations must guard against duplicate output; alternatively, if you
prefer behavior instead of docs, refactor process to ensure each stage
(reliable_encode and obfuscate) is invoked exactly once per tick per stage
(passing Option<Chunk>) so no idempotency contract is required.
In `@common/nym-lp-data/src/lib.rs`:
- Around line 70-89: The transform methods (TimedData::data_transform and
TimedData::ts_transform, and the analogous
PipelineData::data_transform/ts_transform) use a FnMut bound and a mut parameter
even though the closure is invoked exactly once; change the generic bounds from
F: FnMut(...) -> ... to F: FnOnce(...) -> ... and remove the unnecessary mut on
the op parameter (e.g., `mut op: F` -> `op: F`) so single-use, consuming move
closures are accepted; update both TimedData and PipelineData variants
accordingly.
In `@nym-mix-sim/src/topology/directory.rs`:
- Around line 64-68: The method random_next_hop currently calls node_ids() which
allocates a Vec<NodeId> per call; change it to sample directly from the
directory's map keys to avoid allocations by using the IteratorRandom helper on
self.nodes.keys() (e.g. import rand::seq::IteratorRandom and call
self.nodes.keys().choose(rng)), dereference the chosen &NodeId to return NodeId,
and keep the existing SAFETY/unwrap assertion (or use expect with the same
message) to preserve the guarantee that the directory is non-empty; this removes
the per-call allocation from node_ids() while keeping semantics for
random_next_hop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a40dff51-5d4a-4e4d-80f5-d273feef3dd3
📒 Files selected for processing (5)
common/nym-lp-data/src/clients/traits.rscommon/nym-lp-data/src/common/traits.rscommon/nym-lp-data/src/lib.rsnym-mix-sim/README.mdnym-mix-sim/src/topology/directory.rs
Comment on lines
+124
to
+126
| // IMPORTANT NOTE : This fn can be not constant to allow e.g. flexible MTU | ||
| // However, every possible value must be able to accomodate the different overhead. | ||
| // If it doesn't, the pipeline becomes unusable |
There was a problem hiding this comment.
Typo: "accomodate" → "accommodate".
Proposed fix
- // However, every possible value must be able to accomodate the different overhead.
+ // However, every possible value must be able to accommodate the different overhead.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // IMPORTANT NOTE : This fn can be not constant to allow e.g. flexible MTU | |
| // However, every possible value must be able to accomodate the different overhead. | |
| // If it doesn't, the pipeline becomes unusable | |
| // IMPORTANT NOTE : This fn can be not constant to allow e.g. flexible MTU | |
| // However, every possible value must be able to accommodate the different overhead. | |
| // If it doesn't, the pipeline becomes unusable |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@common/nym-lp-data/src/common/traits.rs` around lines 124 - 126, Fix the typo
in the comment: change "accomodate" to "accommodate" in the comment block that
begins "// IMPORTANT NOTE : This fn can be not constant..." in
common/nym-lp-data/src/common/traits.rs so the comment reads "must be able to
accommodate the different overhead."
Comment on lines
+86
to
+97
| fn from(value: &Topology) -> Self { | ||
| let mut directory = Directory::default(); | ||
| for node in &value.nodes { | ||
| directory.nodes.insert(node.node_id, node.into()); | ||
| } | ||
| for client in &value.clients { | ||
| directory | ||
| .clients | ||
| .insert(client.client_id, client.mixnet_address); | ||
| } | ||
| directory | ||
| } |
There was a problem hiding this comment.
Enforce non-empty topology invariant at directory construction.
random_next_hop/random_route rely on unwrap() and will panic when nodes is empty. Add an explicit invariant check in From<&Topology> so invalid topology fails fast with a clear message (Line 86 onward), instead of crashing later during routing.
Suggested patch
impl From<&Topology> for Directory {
@@
fn from(value: &Topology) -> Self {
+ assert!(
+ !value.nodes.is_empty(),
+ "topology must contain at least one node"
+ );
let mut directory = Directory::default();
for node in &value.nodes {
directory.nodes.insert(node.node_id, node.into());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn from(value: &Topology) -> Self { | |
| let mut directory = Directory::default(); | |
| for node in &value.nodes { | |
| directory.nodes.insert(node.node_id, node.into()); | |
| } | |
| for client in &value.clients { | |
| directory | |
| .clients | |
| .insert(client.client_id, client.mixnet_address); | |
| } | |
| directory | |
| } | |
| fn from(value: &Topology) -> Self { | |
| assert!( | |
| !value.nodes.is_empty(), | |
| "topology must contain at least one node" | |
| ); | |
| let mut directory = Directory::default(); | |
| for node in &value.nodes { | |
| directory.nodes.insert(node.node_id, node.into()); | |
| } | |
| for client in &value.clients { | |
| directory | |
| .clients | |
| .insert(client.client_id, client.mixnet_address); | |
| } | |
| directory | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nym-mix-sim/src/topology/directory.rs` around lines 86 - 97, The
From<&Topology> implementation for Directory must enforce that the topology has
at least one node to avoid later unwrap() panics in
random_next_hop/random_route; at the start of fn from(value: &Topology) -> Self
add an explicit check like assert!(!value.nodes.is_empty(), "Topology must
contain at least one node when constructing Directory") (or panic! with the same
clear message) so invalid Topology values fail fast, then proceed to create
Directory via Directory::default(), populate directory.nodes and
directory.clients as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds two new workspace crates that together form an abstract pipeline framework for the upcoming Lewes Protocol (LP) plus a runnable simulator that consumes it.
common/nym-lp-data— a trait-only "vocabulary" crate that defines the shape of client and mix-node packet-processing pipelines. No concrete cryptography, transport, or networking code.nym-mix-sim— a discrete-time mixnet simulator running on localhost UDP. Doubles as the reference implementation of every trait innym-lp-data.nym-lp-data— the vocabularyThree modules:
common— wire-layer traits shared by clients and nodes (Framing/Transportand their unwrap counterparts) plus the supertraitsWireWrappingPipeline/WireUnwrappingPipeline.common/helpers.rsaddsNoOpWireWrapper/NoOpWireUnwrappermarkers for packet types that are already self-contained on the wire (e.g. a Sphinx packet).clients— the six-stage outbound pipelineChunking → Reliability → Obfuscation → RoutingSecurity → Framing → Transport, the supertraitClientWrappingPipelinethat ties them together with a defaultprocess(), an inbound counterpart, no-op marker traits per stage, aPipelinecomposition struct, and a tick-drivenClientWrappingPipelineDriver.mixnodes—MixnodeProcessingPipeline(unwrap →mix()→ re-wrap).A few generic wrappers thread per-packet state through every stage:
TimedData,AddressedTimedData, andPipelineData(which adds per-messageInputOptions).A couple of design points worth flagging
process()is called every tick, with or without input.ReliabilityandObfuscationare explicitly invoked twice per tick (once with the real input, once withNone) so retransmissions and cover-traffic loops can fire on idle ticks without the caller knowing what's in flight.InputOptionsare per-message, not per-pipeline. They toggle whether reliability / obfuscation / routing-security run for a given payload and carry the next-hop id, which is what lets a single pipeline interleave real and cover packets with different routing rules.Frametype is an associated item cross-constrained by the wire-pipeline supertraits, so the parameter list stays short at the cost of forcing implementors to spell out the cross-constraint when composing. Open to feedback.A small synthetic integration test wires a mock pipeline against the
nym-lppacket types as a self-contained example of every trait being implemented.nym-mix-sim— the simulatorDiscrete-time mixnet simulator that runs a configurable number of mix nodes and clients on localhost UDP. Time advances in ticks; each tick runs the client phase, then drains sockets, mixes buffered packets, and dispatches outgoing ones.
Two binaries:
nym-mix-sim(generatestopology.jsonand runs the loop) andmix-client(stdin → app socket of a running client).Three driver flavours, controlled by
--driver:simplesphinxInstantdiscrete-sphinx(default)--manualpauses for ENTER between ticks and pretty-prints each node's buffer state per phase, which is useful for stepping a Sphinx packet hop-by-hop. The Sphinx driver implements 3-hop onion encryption, two-loop Poisson cover traffic, a SURB-ACK reliability layer, and fragment reconstruction — exercising every trait innym-lp-data.See
nym-mix-sim/README.mdandcommon/nym-lp-data/README.mdfor full docs and quick-start examples.Small workspace touches
Cargo.toml: register both new crates and exposenym-lp-dataas a workspace dependency.common/crypto: newbs58_x25519_private_keyserde helper used bytopology.jsonto round-trip per-node Sphinx private keys.common/nym-lp: exposeLpHeader::SIZEas apub constso the integration test can size frames against it.This change is
Summary by CodeRabbit
New Features
Documentation
Tests