Introduce Return Path to the protocol #6932

NumberFour8 · 2025-03-18T21:01:07Z

New features

Increase the maximum usable packet payload size

The maximum usable payload size has been increased to 799 bytes per packet.

Key IDs

Key identifiers (Key IDs) were introduced to shorten the size of the Sphinx header. An X25519 public key is now represented using a "Keybinding ID", which is computed as 32 MSB bits from a hash of packet key, chain key and block ID of the key binding announcement. This is a temporary solution which will be replaced by an actual ID governed by the Announcement SC, once modified.

For this reason, new DB migration has been introduced, that adds the published_at information, that holds the Block ID where an Account has been included on-chain.

The Key Id <-> OffchainPublicKey bi-directional mapping is required to construct a Sphinx header (forward or for SURB) and is covered by the database cache. However, note that this mapping is not strictly an async cache, since the values are required to be fetched inside strictly synchronous code.

SURB & Return path support

The main contribution of this PR is the addition of SURB (Single Use Reply Block) support and its corresponding ReplyOpener to the code. Packet creation and decoding has been significantly changed to allow:

sending packets with optional SURBs
retrieving of the SURBs by the packet's recipient
optional sending of packets only using a given SURB
retrieving of packets delivered via SURB using the associated ReplyOpener

sequenceDiagram
    autonumber
    participant Alice
    participant Bob

    Note left of Alice: Initial Setup
    Alice->>Alice: Generate Pseudonym, SURB, ReplyOpener
    Note left of Alice: Store Pseudonym & ReplyOpener

    Alice->>Bob: Send Request + SURB + Pseudonym
    Note right of Bob: Read Request, store SURB & Pseudonym

    
    Bob->>Bob: Encrypt Reply using SURB
    Bob->>Alice: Send Reply + Pseudonym
    

    Alice->>Alice: Find ReplyOpener via Pseudonym
    Alice->>Alice: Decrypt Data

Each node maintains a (non-permanent) list received SURBs (indexed by Pseudonym). This list is transitory, as each received SURB has a lifetime (currently set to 10 minutes) until it must be used.

Nodes also maintain a (non-permanent) list of ReplyOpeners (indexed by Pseudonym) for each sent SURB. This list has the same transitory behavior as the former one.

When sending a packet, each node can now decide on how to route the packet:

Use an explicitly given forward path (either user defined or from a path selection algorithm)
Or use a Pseudonym, for which a SURB is on the node's list

This will be subsequently used inside Session protocol, so that it actually uses the return path.

No-ACK packets

Special type of flag has been introduce into the Sphinx packet header. The no_ack flag signals to the packet recipient, that the acknowledgement should not be sent back to the sender. The no_ack flag can be set only for zero-hop packets and represents a basis for ACK and Heartbeat protocol to be functioning inside the HOPR protocol itself (packets in such protocols naturally cannot be acknowledged, as they'd result in an infinite loop).

This will be further utilized in #7073 .

Fixes & improvements

PRP and PRG algorithms have been replaced

Lioness PRP and AES-CTR PRG were replaced with a Chacha20 based stream cipher.
This gives an important performance boost on most modern CPUs.

`OffchainPublicKey` contains more precomputes

The OffchainPublicKey type, that represents a packet key, now carries more precomputed values, so that operations are faster. This goes at the expense of an increased size of the type. For this reason, CompactOffchainPublicKey has been introduced and should be used on places where no computation using the key is needed.
The types can be converted into one another, however the conversion from the compact representation is expensive and therefore must be done explicitly (there's on purpose no Into trait impl in this direction).

`serde` usage as a crate feature

Serialization and deserialization using serde has been made optional (and put behind the serde feature) in some crates:

hopr-crypto-packet
hopr-crypto-types
hopr-internal-types
hopr-network-types

Other

Blake2s has been replaced with Blake3
several types in hopr-crypto-types were simplified
added benchmarks for packet and return path code
some cryptographic dependencies have been updated
legacy Ticket aggregation types have been removed
randomness generators in the hopr-crypto-random crate have been improved

Further work

Introduce SURB support into Session including the SURB rebalance protocol
Add more unit tests for SURB management code in caches
Replace OffchainPublicKey with CompactOffchainPublicKey where appropriate

Refs #4496
Refs #5925

…o 799

…p with Pseudonym

coderabbitai

Actionable comments posted: 23

🔭 Outside diff range comments (6)

hoprd/keypair/src/key_pair.rs (2)
319-326: ⚠️ Potential issue

random_bytes requires a const generic – current call does not compile

random_bytes is defined as random_bytes::<N>(); the const generic cannot be inferred.
Compilation fails with “cannot infer constant”. Supply the length explicitly:
-                let packet_key: [u8; PACKET_KEY_LENGTH] = random_bytes();
+                let packet_key: [u8; PACKET_KEY_LENGTH] = random_bytes::<{ PACKET_KEY_LENGTH }>();
379-397: ⚠️ Potential issue

Same generic‑parameter omission for salt and iv

Both calls will fail to compile for the same reason as above.
-        let salt: [u8; HOPR_KEY_SIZE] = random_bytes();
+        let salt: [u8; HOPR_KEY_SIZE] = random_bytes::<{ HOPR_KEY_SIZE }>();

 ...

-        let iv: [u8; HOPR_IV_SIZE] = random_bytes();
+        let iv: [u8; HOPR_IV_SIZE] = random_bytes::<{ HOPR_IV_SIZE }>();
common/network-types/src/types.rs (1)

292-296: 🛠️ Refactor suggestion

Switch from PeerId to Address breaks callers silently

RoutingOptions::IntermediatePath now contains BoundedVec<Address>.
Callers that previously pushed PeerIds will compile again after they add .address() conversions, but the semantic meaning changes (nodes vs. accounts).

Make sure:

Address::from(peer_id) or equivalent helpers exist so the migration is mechanical.

Unit tests cover mis‑typed path elements – an Address accidentally created from a wrong network (e.g. Ethereum vs. Substrate) should fail fast.

Consider providing a TryFrom<Vec<PeerId>> for RoutingOptions impl with a deprecation notice to ease transition.
transport/api/src/lib.rs (1)
688-701: ⚠️ Potential issue

Tag‑range validation is incomplete & silently ignores the upper bound

send_message blocks tags < RESERVED_SESSION_TAG_UPPER_LIMIT but does not validate:

that the tag is ≥ RESERVED_SUBPROTOCOL_TAG_UPPER_LIMIT (the lower slice of the reserved range), and

that the tag fits into the valid u16 application range (e.g. < 0xFFFF).

If a caller supplies a tag in the gap between the two reserved ranges the packet will be sent yet may collide with future protocol‑level reservations.
- if application_tag < RESERVED_SESSION_TAG_UPPER_LIMIT {
+ if application_tag < RESERVED_SESSION_TAG_UPPER_LIMIT
+     || application_tag < RESERVED_SUBPROTOCOL_TAG_UPPER_LIMIT
+     || application_tag == u16::MAX     // example hard‑cap
+ {
common/internal-types/src/tickets.rs (1)
418-425: 🛠️ Refactor suggestion

Hard‑coding the on‑chain selector can easily drift out of sync

[252, 183, 121, 111] is the little‑endian representation of the current RedeemTicket function selector. Duplicating it here makes the hashing fragile – any future contract upgrade will require hunting down magic numbers across the codebase.

Consider exporting the selector from the ABI/contract binding crate and reference it:
- let hash_struct = Hash::create(&[&[252u8, 183u8, 121u8, 111u8], &[0u8; 28], ticket_hash.as_ref()]);
+ use hopr_contract_bindings::redeem_ticket::SELECTOR; // or similar
+ let hash_struct = Hash::create(&[SELECTOR.as_ref(), &[0u8; 28], ticket_hash.as_ref()]);
This guarantees compile‑time breakage when the selector changes and keeps business logic free of opaque constants.
db/sql/src/protocol.rs (1)
555-559: ⚠️ Potential issue

Possible underflow when computing current_path_pos - 1.

current_path_pos is a u8; if for any reason a value of 0 sneaks through, current_path_pos - 1 will underflow and wrap to 255, inflating the ticket value enormously.

Even though upstream code attempts to guard against this, add an explicit check to fail fast:
-                .mul(U256::from(current_path_pos - 1))
+                .mul(U256::from(
+                    current_path_pos.checked_sub(1).ok_or_else(|| {
+                        DbSqlError::LogicalError("path position must be ≥1".into())
+                    })?,
+                ))

♻️ Duplicate comments (2)

transport/protocol/Cargo.toml (1)

20-20: Bincode dependency added for improved serialization

The addition of bincode from the workspace is appropriate for the new serialization needs. Previous discussions indicate bincode was chosen over alternatives like cbor4ii for performance reasons based on benchmarks.

crypto/types/src/lib.rs (1)

43-43: Uncommented PeerId in prelude

The uncommented PeerId in the prelude suggests increased usage in the codebase, which aligns with the PR objective of introducing support for Single Use Reply Blocks (SURBs) and return paths.

As noted in previous discussions, while this creates a dependency on libp2p_identity in the crypto library, the conversion between PeerId and OffchainPublicKey is non-trivial and justifies this approach.

🧹 Nitpick comments (38)

transport/protocol/benches/protocol_throughput_emulated.rs (1)
107-107: Variable name could be clarified.

The variable name path is misleading since it now contains a ResolvedTransportRouting object rather than just a path. Consider renaming it to routing for clarity.
-                                let path = routing.clone();
+                                let routing_info = routing.clone();
transport/p2p/src/swarm.rs (2)
247-248: Minor style: avoid the fully‑qualified std::result::Result in the alias

Inside the alias you can use the prelude‑imported Result instead of the verbose path ‑ the surrounding code already brings Result into scope. It keeps the line shorter and consistent with the rest of the file.
-pub type TicketAggregationResponseType = ResponseChannel<std::result::Result<Ticket, String>>;
+pub type TicketAggregationResponseType = ResponseChannel<Result<Ticket, String>>;
398-400: Consider acknowledging send_response errors to the awaiting peer

The current branch only logs a failure from send_response. Down‑stream code (the requestor) is still waiting for a response and will eventually time out.
If feasible, sending an explicit Err(...) over the same channel (or at least finalising the stored TicketAggregationFinalizer) would shorten the failure detection path.
transport/protocol/src/ack/processor.rs (1)
31-40: Validation & DB call look good, but double‑check peer/key matching

OffchainPublicKey::try_from(peer)? assumes that the supplied peer string is always a valid off‑chain public key. If a malicious peer manages to spoof an ID not matching its signing key, ack.validate(&remote_pk) will incorrectly fail (or worse, succeed for the wrong identity).

Please make sure the transport layer guarantees this invariant, or add an explicit equality check between the embedded sender inside ack and remote_pk before persisting:
let remote_pk = OffchainPublicKey::try_from(peer)?;
let verified = ack.validate(&remote_pk)?;
ensure!(verified.sender() == remote_pk, ProtocolError::InvalidSignature);
self.db.handle_acknowledgement(verified).await
transport/protocol/src/ticket_aggregation/processor.rs (1)
341-349: tickets is already TransferableWinningTicket – extra mapping in tests is redundant

Down‑stream code (and tests) sometimes still call
acked_tickets.into_iter().map(TransferableWinningTicket::from).collect().

As TransferableWinningTicket::from(TransferableWinningTicket) is an
identity conversion, this creates an unnecessary clone:
- acked_tickets.into_iter().map(TransferableWinningTicket::from).collect()
+ acked_tickets
Removing it reduces allocations and simplifies the call‑site.
hopr/hopr-lib/tests/session_integration_tests.rs (1)
22-29: Avoid shadowing the data parameter and explicitly ignore unused arguments

The destructuring assignment re‑uses the identifier data, shadowing the original ApplicationData parameter.
Although legal, it can be confusing during maintenance and may hide subtle bugs if the original value is mistakenly expected later.
Additionally, the DestinationRouting argument is unused; prefixing it with an underscore makes this explicit.
-    async fn send_message(&self, data: ApplicationData, _: DestinationRouting) -> Result<(), TransportSessionError> {
-        let (_, data) = unwrap_chain_address(&data.plain_text)?;
+    async fn send_message(
+        &self,
+        app_data: ApplicationData,
+        _routing: DestinationRouting,
+    ) -> Result<(), TransportSessionError> {
+        let (_, payload) = unwrap_chain_address(&app_data.plain_text)?;
hoprd/keypair/src/key_pair.rs (1)
398-400: Propagate new_from_slices errors accurately

The error mapping loses the underlying cipher error details, making debugging harder.
Consider bubbling up the original error message:
-            .map_err(|_| KeyPairError::KeyDerivationError("invalid key or iv".into()))?;
+            .map_err(|e| KeyPairError::KeyDerivationError(format!("invalid key/iv: {e}")))?;
hopr/hopr-lib/src/lib.rs (2)

1091-1104: Out‑of‑date doc‑string & API drift

The rust‑doc above send_message still lists
destination, options, and an optional applicationTag, while the
signature now takes a single routing: DestinationRouting and a
mandatory Tag.

Please update the comment (and any generated OpenAPI / TS bindings) to
match the new parameters, otherwise downstream users will be confused.

815-819: Comment says “Sync key ids” but no code follows

The TODO‑style comment hints at a required operation (populating
CacheKeyMapper) yet the logic was removed in a previous refactor. If
the sync now happens elsewhere, drop or reword the comment; otherwise
the node will miss key‑id bindings after restart and SURB decoding will
fail.

transport/protocol/tests/common/mod.rs (1)

307-320: Error conflation in TestResolver PathAddressResolver impl

All DB errors are converted to PathError::InvalidPeer, losing the
original failure cause and making debugging harder (e.g. I/O or
serialization issues).

Prefer propagating the original error via a dedicated
PathError::ResolverError(String) (already exists) or mapping only the
Err(DbError::NotFound) case to InvalidPeer.

db/sql/src/cache.rs (2)

62-68: Ring buffer capacity is hard‑coded

AllocRingBuffer::new(10_000) reserves ~7 MB of memory per pseudonym.
On nodes with many pseudonyms this will balloon.

Expose the capacity via a config constant or make it proportional to
surbs_per_pseudonym.max_capacity().

71-84: Potential starvation with concurrent SURB consumers

pop_one dequeues with dequeue() which returns None when the buffer
is empty. If two async tasks race, one may starve the other indefinitely
because no waker/notifier is used.

Consider returning a Result<Option<_>> and letting the caller decide
whether to await more SURBs or add a Condvar/channel to signal pushes.
transport/protocol/src/lib.rs (1)
78-79: Update the import list only where it is actually required

ResolvedTransportRouting is imported at module level but – apart from the changed API stream – nothing else in this file references it directly.
If the type is used solely to type‑annotate the api stream (lines 161‑165) consider doing a scoped import there instead of keeping it in the top‑level prelude to avoid “unused import” warnings when this file is compiled behind feature flags/tests that do not build the msg/ack pipeline.
-use hopr_network_types::prelude::ResolvedTransportRouting;
+// keep top‑level list tidy; import in the only place it is needed
transport/p2p/src/lib.rs (3)
37-38: Prefer explicit imports over glob to avoid accidental name clashes

use hopr_internal_types::prelude::*; pulls in >200 symbols.
A future addition of a type named Error or Event can silently shadow the libp2p counterparts. Import only what you need:
-use hopr_internal_types::prelude::*;
+use hopr_internal_types::prelude::{
+    Ticket,                // core ticket type
+    TransferableWinningTicket,
+};
151-159: Timeout too small for on‑chain aggregation?

ticket_aggregation_timeout is now applied to the updated ticket types. Depending on chain latency, the default Duration you pass in tests (10 s?) might be insufficient for aggregation + signing. Consider exposing this as a config value rather than hard‑coding it in higher layers.

175-176: Event variant name still reflects old terminology

Variant TicketAggregation now carries Vec<TransferableWinningTicket>, but the enum field name hasn’t changed.
If you plan to introduce other aggregation kinds (e.g. “partial”), rename to something more specific (WinningTicketAggregation) before the next release – changing public enums later is a breaking change.
common/network-types/src/types.rs (3)
321-360: DestinationRouting – clarify pseudonym semantics

pseudonym: Option<HoprPseudonym> is optional, while documentation says “If not given, will be resolved as random”.
Nothing in this enum enforces the generation; the responsibility leaks to every resolver.

Two minor improvements:
-        pseudonym: Option<HoprPseudonym>,
+        pseudonym: HoprPseudonym,
and offer a helper with_random_pseudonym() to preserve ergonomics.

This guarantees the field is never None, simplifies pattern‑matching, and avoids accidental unwrap()s in downstream code.

370-393: Derive PartialEq (& possibly Eq) for easier testing & hashing

ResolvedTransportRouting is missing PartialEq/Eq derives, making it awkward to assert equality in unit‑tests or use it as a key in maps.
Unless ValidatedPath lacks these traits, add them:
-#[derive(Debug, Clone, strum::EnumIs)]
+#[derive(Debug, Clone, PartialEq, Eq, strum::EnumIs)]
If ValidatedPath isn’t Eq, derive at least PartialEq so tests can compare routes.

384-393: forward_only() generates a new pseudonym every call

Each call creates a brand‑new random pseudonym; two packets sent in succession via the same forward path but different pseudonyms cannot share SURBs.
If caller wants deterministic pseudonym per session, provide an overload that accepts an explicit pseudonym to avoid subtle bugs where reply routes cannot be matched.

Example:
pub fn forward_only_with_pseudonym(
    pseudonym: HoprPseudonym,
    forward_path: ValidatedPath
) -> Self { … }
crypto/types/src/primitives.rs (2)

47-51: Default yields an all‑zero key: discourage accidental use

Returning an IV+Key filled with zeros is convenient for tests, but dangerous in production: it silently produces an insecure, deterministic cipher state.

Two mitigations:

Add #[deprecated(note = "Default for IvKey is insecure; use IvKey::random()")] (or feature‑gate it for tests).

Remove the Default impl entirely and force callers to be explicit.

67-74: Debug prints the IV – this may leak sensitive nonce information

While the key is correctly redacted, the IV is still printed in logs. Depending on the threat model, exposing IVs might aid an attacker (e.g. in replay or state‑recovery attacks).

Option: follow the same policy as for the key and redact the IV as well, or guard the implementation behind a cfg(debug_assertions).
crypto/packet/src/types.rs (2)
28-36: Off‑by‑one guard could still crash on perfectly‑sized buffers

if surb_end >= data.len() rejects cases where surb_end == data.len(),
but the subsequent drain(0..=surb_end) indexes surb_end inclusive.
So the last valid index is data.len()-1; allowing surb_end == data.len()-1 is fine, while == data.len() would panic.
The existing guard is correct but rather subtle — a future refactor
might inadvertently switch to a half‑open range and re‑introduce an
OOB. Consider documenting the invariants or changing the guard to the
more idiomatic half‑open form to make the intent obvious.
-            let surb_end = num_surbs * SURB::<S, H>::SIZE;
-            if surb_end >= data.len() {
+            let surb_end = num_surbs
+                .checked_mul(SURB::<S, H>::SIZE)
+                .ok_or(GeneralError::ParseError("overflow".into()))?;
+            if surb_end + 1 > data.len() {
                 return Err(GeneralError::ParseError("HoprPacketMessage.num_surbs not valid".into()).into());
             }
63-72: Over‑allocates every packet by a full padding block

Vec::with_capacity(PaddedPayload::<P>::SIZE) reserves space for a
fully padded packet even though the vector is immediately handed over
to PaddedPayload::new_from_vec, which will allocate again if the
capacity is insufficient and otherwise just copies. Reserving the
exact length we are going to push avoids one allocation and reduces
memory footprint for small messages:
-        let mut ret = Vec::with_capacity(PaddedPayload::<P>::SIZE);
+        let expected_len = Self::HEADER_LEN + surbs.len() * SURB::<S, H>::SIZE + payload.len();
+        let mut ret = Vec::with_capacity(expected_len);
crypto/sphinx/src/packet.rs (1)
56-67: Minor: simplify and clarify in‑place padding logic

The current new_from_vec mutates the same buffer several times (resize → copy_within → fill), which is correct but a bit opaque.
A tiny refactor both clarifies intent and removes one pass over the slice:
-        msg.resize(Self::SIZE, Self::PADDING); // Reallocates only if capacity is not enough
-        msg.copy_within(0..len, Self::SIZE - len);
-        msg[0..Self::SIZE - len].fill(Self::PADDING);
-        msg[Self::SIZE - len - 1] = Self::PADDING_TAG;
+        // Resize once, then write the tag and data directly
+        msg.resize(Self::SIZE, Self::PADDING);
+        let tag_pos = Self::SIZE - len - 1;
+        msg[tag_pos] = Self::PADDING_TAG;
+        msg[tag_pos + 1..].copy_from_slice(&msg[..len]);
crypto/packet/benches/packet_benches.rs (1)

10-11: Bench sample size is unusually high

SAMPLE_SIZE = 100_000 yields very long benchmark runs (and CI time‑outs on slower runners).
Criterion’s default (or a few thousand) is usually enough for stable statistics. Consider lowering this unless you have a specific reason to keep it that high.
crypto/sphinx/src/routing.rs (1)
117-129: HeaderPrefix::new silently allows MAX_HOPS > 7

The method caps path_pos to 3 bits (0–7) but doesn’t check H::MAX_HOPS.
Although RoutingInfo::new asserts later, adding a guard here prevents accidental misuse of HeaderPrefix in isolation:
         // Due to size restriction, we do not allow greater than 7 hop paths.
-        if path_pos > 7 {
+        if path_pos > 7 || H::MAX_HOPS.get() > 7 {
             return Err(GeneralError::ParseError("HeaderPrefixByte".into()));
         }
logic/path/src/lib.rs (1)

257-314: Avoid sequential awaits inside the hop‑validation loop – resolve and channel‑check can be pipelined

ValidatedPath::new performs one await per hop (address <-> key resolution) followed by an immediate synchronous channel lookup.
For paths of non‑trivial length this results in strictly serial round‑trips against the resolver / database even though the operations are independent. In moderately long paths (e.g. 10 hops) the latency becomes O(hops × rt) instead of O(rt + hops).

A small refactor that collects all unresolved items first and then calls futures::try_join_all (or join_all + collect::<Result<…>>()) cuts the worst‑case latency dramatically and removes back‑pressure for concurrent callers.

transport/api/src/lib.rs (1)

702-711: Minor: routing resolution happens before early‑return size check

path_planner.resolve_routing is executed after the payload‑size guard, which is good.
Consider also short‑circuiting on the (cheap) tag‑range test before constructing ApplicationData to avoid an unnecessary allocation.

crypto/sphinx/src/shared_keys.rs (1)

99-107: Blinding factor validity is checked only after multiplying α

If b_k_checked is invalid the preceding alpha_prev = alpha_prev.mul(&b_k_checked); already committed the potentially invalid element.
Swap the order (validate first, then apply) to avoid transient invalid state or remove the mutation until the check passes.

hoprd/rest-api/src/session.rs (1)

257-259: HTTP 422 is not the best fit for configuration errors

into_protocol_session_config currently returns ApiErrorStatus::UnknownFailure, which is wrapped into a 422 Unprocessable Entity. If the failure is due to an invalid peer/path specification coming from the client, a 400 Bad Request would communicate the problem more accurately and avoids collapsing user input problems with genuine server exceptions.
crypto/sphinx/src/derivation.rs (2)
40-55: Doc‑comment drift: Blake2 → Blake3 and HKDF semantics

generate_key/generate_key_iv are now Blake3‑HKDF based, but the comments still talk about “Blake2s256” and RFC‑5869 extract/expand behaviour. This is misleading for future maintainers and automated doc tools.

Please update the rust‑docs accordingly:
-/// The function internally uses Blake2s256 based HKDF (see RFC 5869).
+/// Internally uses Blake3 in HKDF‑XOF mode.  The API is still RFC‑5869
+/// compatible (optional salt → Extract, followed by Expand).
(Same for the IV/key paragraph below.)

69-86: Small optimisation: avoid heap allocation in generate_key_iv

out is allocated on the heap even though its size is fully known at compile time
(Key::<T>::LEN + Iv::<T>::LEN, both ≤ 32 for the ciphers we use). Using a
stack‑allocated array reduces allocations in a hot crypto path:
- let mut out = vec![0u8; key.len() + iv.len()];
- output.fill(&mut out);
-
- let (v_iv, v_key) = out.split_at(iv.len());
- iv.copy_from_slice(v_iv);
- key.copy_from_slice(v_key);
+ let mut buf = [0u8; 64]; // supports AES‑256/ChaCha20; truncate otherwise
+ let needed = key.len() + iv.len();
+ output.fill(&mut buf[..needed]);
+ iv.copy_from_slice(&buf[..iv.len()]);
+ key.copy_from_slice(&buf[iv.len()..needed]);
This removes one allocation per key derivation without sacrificing clarity.
transport/api/src/helpers.rs (1)
150-162: Consider bounding the number of return‑path resolutions

max_surbs_with_message(size_hint) can yield a fairly large value for small
messages, and FuturesUnordered spawns that many concurrent calls to
resolve_path. On a busy node this may create significant DB/graph‑lock
pressure or even become a DoS vector.

If the protocol allows, cap the number of SURBs per packet (e.g. by a
configuration constant) before entering the loop:
-                    let num_possible_surbs = HoprPacket::max_surbs_with_message(size_hint);
+                    let max_surbs = std::cmp::min(
+                        HoprPacket::max_surbs_with_message(size_hint),
+                        crate::constants::MAX_SURBS_PER_PACKET, // new constant, e.g. 8
+                    );
This keeps resource usage predictable without changing the external API.
db/sql/src/protocol.rs (1)
288-294: Misleading error message – refer to the correct hop.

The formatter still says “previous hop” although this branch is executed while resolving the very first next hop for a brand–new zero‑hop/no‑ack packet. Leaving the wording unchanged makes log triage harder.
-                "failed to find chain key for packet key {} on previous hop",
+                "failed to find chain key for packet key {} on next hop",
common/internal-types/src/protocol.rs (1)
52-61: Avoid unwrap() in fallible deserialisation path.

Although the length check prevents a panic today, replacing unwrap() with a safe conversion makes the code future‑proof and easier to audit.
-            Ok(Self {
-                data: value.try_into().unwrap(),
+            Ok(Self {
+                data: value.try_into().expect("slice length is verified above"),
db/api/src/protocol.rs (1)
76-82: Document the no_ack flag in TransportPacketWithChainData::Final.

A short doc comment (or at least an inline comment) explaining that no_ack == true indicates a zero‑hop packet that must not be acknowledged will prevent accidental misuse by callers.
-        ack_key: HalfKey,
-        no_ack: bool,
+        ack_key: HalfKey,
+        /// `true` → sender requested that no ACK be generated (zero‑hop packets)
+        no_ack: bool,
crypto/packet/src/packet.rs (2)

70-74: TicketBuilder::leak() obscures ownership semantics

Turning a freshly‑signed Ticket into a “leaked” value suggests an intentional zeroize bypass or ownership discard.
If the intent is merely to convert a builder into an owned Ticket, prefer an explicit, side‑effect‑free method (e.g. finish() or into_ticket()); reserving the word leak for true memory‑leak semantics avoids confusion and audit friction.

257-279: Minor: double allocation when mapping return‑path keys

create_surb_for_path builds a temporary Vec<KeyIdent> only to pass it immediately to create_surb.
You can remove one allocation by collecting directly into a small array on the stack or by accepting an iterator in create_surb.

Not critical, but worth considering for tight loops or constrained environments.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dd1e15b and 2dff90b.

⛔ Files ignored due to path filters (2)

Cargo.lock is excluded by !**/*.lock
uv.lock is excluded by !**/*.lock

📒 Files selected for processing (107)

Cargo.toml (2 hunks)
chain/actions/src/node.rs (1 hunks)
chain/actions/src/redeem.rs (1 hunks)
chain/indexer/src/block.rs (1 hunks)
chain/indexer/src/handlers.rs (7 hunks)
common/internal-types/Cargo.toml (3 hunks)
common/internal-types/src/account.rs (5 hunks)
common/internal-types/src/channels.rs (3 hunks)
common/internal-types/src/errors.rs (1 hunks)
common/internal-types/src/legacy.rs (0 hunks)
common/internal-types/src/lib.rs (0 hunks)
common/internal-types/src/protocol.rs (8 hunks)
common/internal-types/src/tickets.rs (15 hunks)
common/network-types/Cargo.toml (3 hunks)
common/network-types/benches/session.rs (1 hunks)
common/network-types/src/session/mod.rs (0 hunks)
common/network-types/src/types.rs (3 hunks)
common/primitive-types/Cargo.toml (1 hunks)
common/primitive-types/src/lib.rs (1 hunks)
common/primitive-types/src/primitives.rs (3 hunks)
common/primitive-types/src/traits.rs (1 hunks)
crypto/packet/Cargo.toml (3 hunks)
crypto/packet/benches/packet_benches.rs (1 hunks)
crypto/packet/benches/packet_crypto.rs (0 hunks)
crypto/packet/benches/por_benches.rs (1 hunks)
crypto/packet/src/chain.rs (0 hunks)
crypto/packet/src/errors.rs (1 hunks)
crypto/packet/src/lib.rs (1 hunks)
crypto/packet/src/packet.rs (1 hunks)
crypto/packet/src/por.rs (4 hunks)
crypto/packet/src/types.rs (1 hunks)
crypto/random/Cargo.toml (1 hunks)
crypto/random/src/lib.rs (1 hunks)
crypto/sphinx/Cargo.toml (3 hunks)
crypto/sphinx/src/derivation.rs (3 hunks)
crypto/sphinx/src/ec_groups.rs (3 hunks)
crypto/sphinx/src/errors.rs (1 hunks)
crypto/sphinx/src/lib.rs (1 hunks)
crypto/sphinx/src/packet.rs (1 hunks)
crypto/sphinx/src/prg.rs (0 hunks)
crypto/sphinx/src/prp.rs (0 hunks)
crypto/sphinx/src/routing.rs (3 hunks)
crypto/sphinx/src/shared_keys.rs (6 hunks)
crypto/sphinx/src/surb.rs (1 hunks)
crypto/types/Cargo.toml (1 hunks)
crypto/types/src/errors.rs (1 hunks)
crypto/types/src/keypairs.rs (6 hunks)
crypto/types/src/lib.rs (3 hunks)
crypto/types/src/primitives.rs (1 hunks)
crypto/types/src/types.rs (26 hunks)
crypto/types/src/utils.rs (2 hunks)
crypto/types/src/vrf.rs (2 hunks)
db/api/Cargo.toml (2 hunks)
db/api/src/protocol.rs (5 hunks)
db/migration/src/lib.rs (3 hunks)
db/migration/src/m20250419_000022_account_add_published_block.rs (1 hunks)
db/sql/Cargo.toml (3 hunks)
db/sql/src/accounts.rs (16 hunks)
db/sql/src/cache.rs (7 hunks)
db/sql/src/db.rs (2 hunks)
db/sql/src/errors.rs (1 hunks)
db/sql/src/protocol.rs (5 hunks)
db/sql/src/resolver.rs (2 hunks)
db/sql/src/ticket_manager.rs (2 hunks)
db/sql/src/tickets.rs (3 hunks)
hopr/hopr-lib/Cargo.toml (1 hunks)
hopr/hopr-lib/src/lib.rs (8 hunks)
hopr/hopr-lib/tests/chain_integration_tests.rs (1 hunks)
hopr/hopr-lib/tests/session_integration_tests.rs (5 hunks)
hoprd/inbox/src/inbox.rs (2 hunks)
hoprd/keypair/Cargo.toml (0 hunks)
hoprd/keypair/src/key_pair.rs (7 hunks)
hoprd/rest-api/src/messages.rs (5 hunks)
hoprd/rest-api/src/session.rs (13 hunks)
logic/path/Cargo.toml (2 hunks)
logic/path/src/channel_graph.rs (6 hunks)
logic/path/src/errors.rs (2 hunks)
logic/path/src/lib.rs (1 hunks)
logic/path/src/path.rs (0 hunks)
logic/path/src/selectors/dfs.rs (8 hunks)
logic/path/src/selectors/mod.rs (1 hunks)
logic/strategy/src/aggregating.rs (1 hunks)
logic/strategy/src/auto_redeeming.rs (1 hunks)
logic/strategy/src/promiscuous.rs (1 hunks)
sdk/python/localcluster/cluster.py (1 hunks)
sdk/python/localcluster/node.py (2 hunks)
tests/test_integration.py (1 hunks)
transport/api/Cargo.toml (1 hunks)
transport/api/src/config.rs (2 hunks)
transport/api/src/helpers.rs (5 hunks)
transport/api/src/lib.rs (9 hunks)
transport/network/src/messaging.rs (3 hunks)
transport/p2p/src/lib.rs (5 hunks)
transport/p2p/src/swarm.rs (3 hunks)
transport/p2p/tests/p2p_transport_test.rs (2 hunks)
transport/protocol/Cargo.toml (2 hunks)
transport/protocol/benches/protocol_throughput_emulated.rs (3 hunks)
transport/protocol/src/ack/processor.rs (2 hunks)
transport/protocol/src/bloom.rs (2 hunks)
transport/protocol/src/errors.rs (1 hunks)
transport/protocol/src/lib.rs (4 hunks)
transport/protocol/src/msg/codec.rs (5 hunks)
transport/protocol/src/msg/packet.rs (4 hunks)
transport/protocol/src/msg/processor.rs (9 hunks)
transport/protocol/src/ticket_aggregation/processor.rs (3 hunks)
transport/protocol/tests/common/mod.rs (8 hunks)
transport/session/Cargo.toml (1 hunks)

⛔ Files not processed due to max files limit (5)

transport/session/src/initiation.rs
transport/session/src/lib.rs
transport/session/src/manager.rs
transport/session/src/traits.rs
transport/session/src/types.rs

💤 Files with no reviewable changes (9)

common/internal-types/src/lib.rs
hoprd/keypair/Cargo.toml
common/network-types/src/session/mod.rs
crypto/sphinx/src/prg.rs
crypto/packet/benches/packet_crypto.rs
logic/path/src/path.rs
crypto/packet/src/chain.rs
common/internal-types/src/legacy.rs
crypto/sphinx/src/prp.rs

🧰 Additional context used

🧠 Learnings (9)

transport/protocol/Cargo.toml (1)

Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6345
File: hopr-socks/hopr-socks-server/Cargo.toml:20-20
Timestamp: 2024-06-21T20:51:35.871Z
Learning: All code and packages in the `hoprnet` project should use the workspace level dependency of `reqwest`.

transport/session/Cargo.toml (2)

Learnt from: tolbrino
PR: hoprnet/hoprnet#6399
File: common/primitive-types/Cargo.toml:24-24
Timestamp: 2024-11-01T09:32:55.651Z
Learning: In the hoprnet project, dependency versions are defined in the workspace Cargo.toml, so it's not necessary to specify them in individual package Cargo.toml files.

Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6345
File: hopr-socks/hopr-socks-server/Cargo.toml:20-20
Timestamp: 2024-06-21T20:51:35.871Z
Learning: All code and packages in the `hoprnet` project should use the workspace level dependency of `reqwest`.

tests/test_integration.py (1)

Learnt from: NumberFour8
PR: hoprnet/hoprnet#6511
File: tests/test_integration.py:377-381
Timestamp: 2024-09-30T07:58:21.238Z
Learning: In `test_hoprd_should_fail_sending_a_message_that_is_too_large`, clarifying the maximum payload size for testing is considered out of scope.

common/network-types/Cargo.toml (1)

Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6345
File: hopr-socks/hopr-socks-server/Cargo.toml:20-20
Timestamp: 2024-06-21T20:51:35.871Z
Learning: All code and packages in the `hoprnet` project should use the workspace level dependency of `reqwest`.

common/internal-types/Cargo.toml (1)

Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6345
File: hopr-socks/hopr-socks-server/Cargo.toml:20-20
Timestamp: 2024-06-21T20:51:35.871Z
Learning: All code and packages in the `hoprnet` project should use the workspace level dependency of `reqwest`.

logic/path/src/selectors/dfs.rs (1)

Learnt from: NumberFour8
PR: hoprnet/hoprnet#6728
File: logic/path/src/selectors/dfs.rs:268-0
Timestamp: 2024-12-13T08:15:31.746Z
Learning: In `logic/path/src/selectors/dfs.rs`, the DFS path selector algorithm intentionally clones `current` to create new starting points during queue extension.

logic/path/Cargo.toml (1)

Learnt from: tolbrino
PR: hoprnet/hoprnet#6399
File: chain/api/Cargo.toml:50-53
Timestamp: 2024-11-01T15:26:53.058Z
Learning: In `chain/api/Cargo.toml`, it's acceptable to declare the same dependency `hopr-db-sql` in both `[dependencies]` and `[dev-dependencies]` with different features, especially when additional features are only needed for tests.

hopr/hopr-lib/src/lib.rs (1)

Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6313
File: hopr/hopr-lib/src/lib.rs:0-0
Timestamp: 2024-07-28T07:26:06.634Z
Learning: The types `IndexerTransportEvent`, `Network`, `PeerEligibility`, and `PeerOrigin` in the `hopr/hopr-lib/src/lib.rs` file are internal to the transport mechanism and do not require public documentation.

crypto/packet/benches/packet_benches.rs (1)

Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6619
File: crypto/packet/benches/packet_crypto.rs:74-120
Timestamp: 2024-11-11T18:29:26.659Z
Learning: In the `crypto/packet/benches/packet_crypto.rs` benchmarks, generalizing initialization code to reduce duplication is preferred, but alternative approaches are favored over the previously suggested fixture pattern.

🧬 Code Graph Analysis (6)

transport/protocol/benches/protocol_throughput_emulated.rs (4)

transport/api/src/lib.rs (6)

mpsc (389-389)

mpsc (392-392)

mpsc (419-419)

mpsc (547-547)

futures (280-280)

futures (478-478)

common/internal-types/src/protocol.rs (2)

random (78-80)

count (191-193)

crypto/packet/src/types.rs (1)

count (127-143)

transport/protocol/tests/common/mod.rs (7)

futures (74-78)

futures (193-193)

futures (195-195)

futures (196-196)

futures (198-198)

futures (203-203)

futures (204-204)

transport/network/src/messaging.rs (1)

crypto/random/src/lib.rs (1)

random_fill (75-77)

transport/protocol/src/ticket_aggregation/processor.rs (1)

db/sql/src/tickets.rs (7)

tickets (2417-2420)

tickets (2477-2480)

tickets (2502-2505)

tickets (2543-2546)

tickets (2698-2701)

tickets (2757-2760)

tickets (2799-2802)

transport/api/src/helpers.rs (3)

transport/api/src/lib.rs (3)

new (147-159)

new (211-261)

send_message (689-725)

crypto/packet/src/packet.rs (1)

max_surbs_with_message (319-321)

transport/session/src/traits.rs (1)

send_message (9-13)

hoprd/keypair/src/key_pair.rs (2)

crypto/types/src/keypairs.rs (7)

random_bytes (53-53)

random (26-26)

random (51-54)

random (120-126)

from_secret (29-29)

from_secret (56-61)

from_secret (128-135)

misc/platform/src/file.rs (2)

read_to_string (8-12)

write (41-53)

common/internal-types/src/tickets.rs (2)

crypto/packet/src/por.rs (2)

create (228-242)

create (247-259)

crypto/types/src/types.rs (1)

create (476-482)

⏰ Context from checks skipped due to timeout of 90000ms (7)

GitHub Check: hoprd / docker
GitHub Check: Docs / Rust docs
GitHub Check: zizmor
GitHub Check: tests-smoke-websocket
GitHub Check: tests-unit-nightly
GitHub Check: Linter
GitHub Check: tests-unit

🔇 Additional comments (180)

transport/session/Cargo.toml (2)

45-45: Dependency addition aligns with feature objectives

Adding the hopr-crypto-packet dependency is consistent with the PR's goal of introducing SURB support and return paths to the protocol.

48-48: Dependency order change noted

This is simply a reordering of the dependency list, which doesn't impact functionality.

common/network-types/benches/session.rs (2)

1-3: Refactored test utilities import path

Appropriate restructuring to import test utilities directly from their source location rather than through feature-gated exports. This aligns with the broader changes to remove the testing feature from the crate.

12-12: Updated import to use local module

This change correctly updates the import to use the newly defined local module, maintaining the same functionality while adapting to the restructured code organization.

transport/api/Cargo.toml (1)

50-50: Added cfg-if for improved conditional compilation

Adding the cfg-if dependency is a good practice for consolidating conditional compilation logic. This will make the config code more maintainable by replacing multiple #[cfg]-gated function definitions with a cleaner approach.

crypto/random/Cargo.toml (1)

3-3: Version bump for new Randomizable trait

The increment from 0.2.0 to 0.2.1 appropriately follows semantic versioning for the addition of the new Randomizable trait. This supports the PR's stated goal of improving randomness generators.

common/primitive-types/src/lib.rs (1)

6-6: Looks good - minor documentation improvement.

Simple documentation change that improves clarity.

common/primitive-types/Cargo.toml (1)

21-21: Good addition for serialization support.

Adding serde_bytes is appropriate for optimizing serialization of byte arrays, which aligns with the PR's introduction of Key IDs and SURB support that likely involve binary data handling.

logic/path/src/selectors/mod.rs (1)

11-11: Import path updated correctly.

The import path has been updated to reflect the relocation of path-related types from path.rs to lib.rs. This change aligns with the broader refactoring of path abstractions and validation logic in this PR.

logic/strategy/src/auto_redeeming.rs (1)

193-193: Added Randomizable trait import for test support.

The addition of the Randomizable trait import enhances cryptographically secure randomness support in tests. This aligns with similar changes across the codebase.

chain/actions/src/redeem.rs (1)

257-257: Approve: Inline Randomizable import for enhanced test randomness.
The addition of Randomizable alongside random_bytes ensures cryptographically secure random generation in tests, consistent with changes in other modules.

db/api/Cargo.toml (2)

3-3: Verify version bump coherence.
The package version was updated from 0.6.0 to 0.7.0. Ensure the changelog, documentation, and downstream consumers are updated to reflect this new version.

23-26: Review added workspace dependencies.
New dependencies hopr-crypto-packet, hopr-network-types, and hopr-path support expanded packet handling and routing abstractions. Verify these crates exist in the workspace and that dependent modules compile correctly.

sdk/python/localcluster/cluster.py (1)

20-20: Assess increased GLOBAL_TIMEOUT to 90 seconds.
The timeout was raised from 60s to 90s to accommodate longer network operations due to routing enhancements. Confirm this change is documented and does not mask performance regressions.

sdk/python/localcluster/node.py (2)

225-225: Fix comment grammar.
The updated comment "filter out peers that are not well-connected yet" improves clarity.

237-238: Log successful peer connection.
The new logging.info statement in the else block provides useful visibility when all required peers are connected.

transport/protocol/src/errors.rs (1)

20-21: Add NoSurb error variant.
The new NoSurb variant explicitly represents absence of a SURB for a given pseudonym, improving error granularity. Ensure callers handle this case and add unit tests covering scenarios where no SURB is available.

db/sql/Cargo.toml (2)

3-3: Version bump properly reflects the significant changes

The package version increase from 0.12.3 to 0.13.0 correctly indicates a breaking change, which aligns with the introduction of the Return Path functionality.

32-33: Dependency additions support SURB functionality

The new dependencies added (dashmap, ringbuffer) along with the moka feature update and new workspace dependencies (hopr-network-types, hopr-path) appropriately support the implementation of Single Use Reply Blocks (SURBs) and return paths:

dashmap provides a thread-safe hash map for concurrent access to SURBs

ringbuffer enables implementing the transient lists with automatic expiration

moka with "sync" feature supports synchronous caches for pseudonym openers

The new workspace dependencies contain the routing abstractions needed for the return path protocol

Also applies to: 37-40, 61-62

db/sql/src/ticket_manager.rs (2)

316-316: LGTM: New import for Randomizable

The addition of the Randomizable trait from hopr_crypto_random is appropriate.

347-347: Added required field for key binding ID implementation

The addition of the published_at field (set to 0) in the AccountEntry initialization supports the new key binding ID functionality described in the PR, which tracks on-chain inclusion information.

db/sql/src/errors.rs (1)

24-26: SURB error handling correctly implemented

The addition of NoSurbAvailable error variant is appropriate for handling the case when no Single-Use Reply Block is available for a given pseudonym. This error provides clear feedback to callers and supports the new SURB functionality described in the PR.

transport/protocol/Cargo.toml (1)

45-46: Enabling serde and adding network types

The changes to enable serde features on hopr-internal-types and add hopr-network-types are necessary to support the new routing abstractions and serialization requirements for the return path functionality.

transport/p2p/tests/p2p_transport_test.rs (1)

125-125: Import and constant update aligned with packet abstraction changes.

This change correctly updates the test's imports and constants to use the new HoprPacket abstraction from the prelude instead of the deprecated ChainPacketComponents enum. This aligns with the broader refactoring of packet handling in the codebase.

Also applies to: 167-167

Cargo.toml (1)

70-70: Dependencies updated to support refactoring goals.

The addition of cfg-if supports conditional compilation for feature flags, while upgrading generic-array to v1.2.0 without the zeroize feature aligns with the PR's goal of replacing AES-based implementations with ChaCha20 and making serialization optional.

Also applies to: 94-94

hopr/hopr-lib/tests/chain_integration_tests.rs (1)

97-105: Updated AccountEntry initialization with new published_at field.

This change correctly updates the test to use the new struct initialization pattern with the additional published_at field, which supports the new Key ID binding functionality mentioned in the PR objectives.

common/primitive-types/src/traits.rs (1)

82-84: Added BytesRepresentable implementation for fixed-size byte arrays.

This implementation enhances the type system's capabilities for handling fixed-size byte arrays, which is beneficial for the cryptographic operations and packet structures being refactored in this PR.

chain/indexer/src/block.rs (2)

883-888: New AccountEntry field correctly initialized.

The code now properly initializes the AccountEntry struct with the new published_at field set to 1, which aligns with the PR objective of adding published_at information to track on-chain inclusion.

893-898: Consistent initialization of AccountEntry with published_at field.

The AccountEntry struct initialization follows the same pattern with the published_at field set to 1, maintaining consistency with the previous instance.

hopr/hopr-lib/Cargo.toml (1)

80-80: Feature flag for serde correctly enabled.

The dependency on hopr-path now explicitly enables the serde feature flag, which aligns with the PR objective of making serde serialization optional via feature flags across multiple crates. This change supports the serialization capabilities needed for the new routing and path validation functionality.

transport/protocol/src/msg/codec.rs (3)

5-5: Updated import for HoprPacket.

The code now correctly imports HoprPacket from the prelude module, replacing the previous ChainPacketComponents import. This aligns with the PR objective of replacing legacy types with the new packet abstractions.

28-29: Updated packet size constant in decoding logic.

The code now uses HoprPacket::SIZE instead of ChainPacketComponents::SIZE for determining the minimum byte length required for decoding and for slicing the packet data from the buffer. This ensures consistency with the new packet abstraction.

49-49: Consistently updated packet size constants in tests.

All test code now uses HoprPacket::SIZE instead of the previous ChainPacketComponents::SIZE, ensuring test coverage remains valid with the new packet structure. This includes constants for payload sizes and assertions verifying byte lengths.

Also applies to: 56-56, 76-76, 97-97, 105-107
tests/test_integration.py (1)

182-182:
✅ Verification successful

Updated maximum payload size to align with new packet structure.

The maximum payload size has been increased from 500 to 800 bytes, matching the updated PAYLOAD_SIZE_INT (800 bytes) in the crypto/packet crate. This change ensures that the integration test correctly validates the payload size constraints of the new HoprPacket implementation.

🌐 Web query:
What is the typical maximum payload size for network packets in similar systems?
💡 Result:

The typical maximum payload size for network packets varies depending on the network type and its configuration. Here are some common maximum payload sizes for different systems:

Ethernet: Typically, Ethernet frames have a maximum payload size of 1500 bytes. Jumbo frames can support larger payloads, often up to around 9000 bytes, but this depends on the specific network setup and devices involved[2][4][7].

Internet IPv4 Path: IPv4 packets generally should not exceed an MTU of 576 bytes for reliability across the internet, though larger packets can be used if the path supports them[8].

TCP Connections: The maximum segment size (MSS) is often set around 1460 bytes to accommodate typical internet MTUs minus the TCP and IP headers[3].

PCI Express (PCIe): For PCIe packets, the payload size is typically limited to 256 bytes or less for most devices, but servers can support up to 512 bytes[6].

In general, larger packet sizes can increase efficiency but also increase the risk of packet loss or fragmentation. Networks often balance these factors based on their specific requirements and infrastructure.

Citations:

1: https://www.baeldung.com/cs/tcp-max-packet-size

2: https://labs.apnic.net/index.php/2024/10/04/the-size-of-packets/

3: https://www.cloudflare.com/learning/network-layer/what-is-mss/

4: https://jvns.ca/blog/2017/02/07/mtu/

5: https://blog.apnic.net/2024/10/08/the-size-of-the-packet/

6: https://www.mindshare.com/files/resources/PLX_Choosing_PCIe_Packet_Payload_Size.pdf

7: https://en.wikipedia.org/wiki/Maximum_transmission_unit

8: https://community.cisco.com/t5/switching/why-the-maximum-ip-packet-size-is-65535/td-p/1897269
Approve payload size update
The integration test now aligns with the PAYLOAD_SIZE_INT (800 bytes) in the crypto/packet crate and remains conservative relative to common MTU limits (e.g., 1500 bytes for Ethernet).

• tests/test_integration.py, line 182:
maximum_payload_size = 800
chain/actions/src/node.rs (1)

186-194: Updated AccountEntry initialization to include the new published_at field

The struct initialization now includes the required published_at field that was added to the AccountEntry struct. This field appears to track the block number when the account was published on-chain.

db/sql/src/tickets.rs (4)

1295-1297: Reordered imports for better organization

The hopr_crypto_random::Randomizable import has been moved above the standard library imports, providing a more logical grouping of dependencies.

1302-1305: Moved standard library imports

Standard library imports have been regrouped for better organization.

1331-1339: Added published_at field to AccountEntry initialization

The AccountEntry initialization in the test helper function now includes the new published_at field set to 0, which is consistent with the updated struct definition and aligns with the database schema changes.

3315-3316: Improved comment clarity

The comment has been updated to more accurately describe what's happening in the code, which is setting the winning probability to zero and signing the ticket again.

logic/strategy/src/promiscuous.rs (1)

752-757: Updated AccountEntry initialization to include the new published_at field

The AccountEntry initialization in the test helper function now uses struct literal syntax with the new published_at field set to 1. This is consistent with the updated struct definition across the codebase and aligns with the database schema changes.

logic/strategy/src/aggregating.rs (1)

437-442: Added new required published_at field to AccountEntry initialization

The initialization of AccountEntry has been updated to include the new published_at field, which is used for tracking when key bindings are published on-chain. This change is consistent with the addition of Key IDs and on-chain inclusion tracking introduced in this PR.

crypto/packet/src/errors.rs (1)

55-56: Added SphinxError for more detailed cryptographic error handling

A new error variant has been added to transparently wrap the Sphinx-specific cryptographic errors. This is a good addition that enables proper error propagation from the Sphinx cryptographic layer, supporting the new SURB functionality and return paths.

chain/indexer/src/handlers.rs (6)

139-144: Updated AccountEntry initialization with published_at field

The code now initializes AccountEntry with the new published_at field set to the current block number. This properly records when key bindings are published on-chain, enabling the Key ID functionality mentioned in the PR.

997-1001: AccountEntry initialization updated with published_at in test code

Test code has been updated to include the new published_at field in the AccountEntry initialization, maintaining consistency with the production code changes.

1027-1032: Added published_at field to AccountEntry in test code

Test code has been properly updated to include the new published_at field with a value of 1, ensuring the tests remain valid with the updated AccountEntry structure.

1087-1095: Updated AccountEntry initialization with published_at in announcement tests

AccountEntry initialization has been updated in the announcement tests to include the new published_at field, maintaining test validity with the enhanced account structure.

1148-1156: Added published_at field to AccountEntry in DNS announcement tests

The DNS announcement test cases have been updated to include the new published_at field in AccountEntry initialization, ensuring tests function correctly with the updated data structure.

1226-1231: Updated AccountEntry initialization in revocation tests

The account revocation test cases now include the new published_at field in the AccountEntry initialization, maintaining consistency across the test suite.

db/sql/src/resolver.rs (4)

102-111: Updated test code with new AccountEntry initialization

The resolver tests have been updated to initialize AccountEntry with the new published_at field, ensuring test compatibility with the enhanced account structure that supports Key IDs.

115-124: Added published_at field to second AccountEntry in resolver test

The second AccountEntry in the resolver test has been properly updated to include the published_at field, maintaining consistent test data.

171-180: Updated AccountEntry initialization in offchain key resolution test

The offchain key resolution test has been updated to include the new published_at field in the AccountEntry initialization, ensuring test validity with the enhanced data structure.

184-193: Added published_at field to second AccountEntry in key resolution test

The second AccountEntry in the key resolution test has been properly updated to include the published_at field, maintaining consistent test data structure throughout the test suite.

logic/path/src/errors.rs (1)

13-14: Good addition of specific error variant for unresolved peers

The new UnknownPeer error variant clearly distinguishes between malformed peer IDs (InvalidPeer) and peers that exist but cannot be resolved during path validation, improving error handling granularity.

common/network-types/Cargo.toml (3)

3-3: Appropriate version bump following semantic versioning

Incrementing to 0.6.0 is correct since adding new dependencies and expanding the feature set constitutes a potentially breaking change.

13-20: Well-structured serde feature expansion

The serde feature is properly expanded to include serialization support for the new dependencies, maintaining consistency across the codebase.

63-69: Dependencies properly aligned with PR objectives

The addition of hopr-crypto-packet and hopr-path dependencies supports the implementation of Single Use Reply Blocks (SURBs) and return paths as described in the PR objectives.

crypto/random/src/lib.rs (2)

87-87: Good simplification of generic constraint

Relaxing the constraint from L: ArrayLength<u8> to L: ArrayLength simplifies the signature without losing functionality since the return type GenericArray<u8, L> already specifies the element type.

93-97: Well-designed Randomizable trait

This trait provides a standardized way to generate cryptographically secure random instances, following Rust's best practices with a single, clear responsibility. The name "Randomizable" (rather than "Randomize") correctly describes the capability rather than an action.

db/sql/src/db.rs (1)

197-210: Good initialization of key_id_mapper cache

The initialization code properly populates the key_id_mapper cache from persisted database accounts, ensuring consistency between database state and in-memory caches at startup. The error handling is well-implemented, allowing initialization to continue even if some accounts can't be decoded.

This initialization supports the PR's objective of introducing Key IDs to shorten the Sphinx header size by maintaining a bidirectional mapping between Key IDs and OffchainPublicKeys.

common/internal-types/Cargo.toml (4)

3-3: Version bump indicates significant changes

The version bump from 0.6.2 to 0.7.0 appropriately reflects the substantial changes being made to the package's API, particularly the restructuring of serialization features.

16-18: Streamlined dependency management

The removal of explicit features from bloomfilter and addition of hex as a direct dependency is consistent with the project's dependency reorganization.

21-22: Optional serialization dependencies

Making serde and serde_bytes optional is a good practice that aligns with the PR objective of making serialization optional via feature flags.

41-46: Well-structured feature flag implementation

The new serde feature flag is well-structured, enabling all the necessary serde-related dependencies. This approach allows consumers to opt-in to serialization functionality only when needed, which can improve compilation times and reduce binary size.

db/migration/src/m20250419_000022_account_add_published_block.rs (1)

1-42: Well-structured database migration

This migration properly adds the PublishedAt column to the Account table, which according to the PR objectives, is meant to track on-chain inclusion. The implementation follows best practices:

Includes both up and down migration paths

Sets appropriate column type (unsigned integer)

Makes the column non-nullable with a sensible default value

Uses proper SeaORM migration patterns

This addition supports the PR's objective of computing a temporary "Keybinding ID" from packet key, chain key, and block ID.

db/migration/src/lib.rs (2)

24-24: Proper module import for the new migration

The new migration module is correctly imported.

69-69: Migration correctly registered in both sequences

The migration is properly added to both the Migrator and MigratorIndex sequences, ensuring the schema change is applied in both general and index-specific database contexts.

Also applies to: 96-96

crypto/types/src/lib.rs (3)

9-9: Improved module documentation

The updated documentation for the primitives module better clarifies its purpose as re-exporting low-level cryptographic primitives rather than implementing them directly.

22-30: New crypto_traits module centralizes cryptographic trait exports

The addition of a dedicated module for re-exporting cryptographic traits from external crates is a good architectural decision. This centralizes the imports and makes it clear which traits are available throughout the codebase.

This aligns with the PR objective of replacing various cryptographic implementations with more standardized ones, such as replacing Lioness PRP and AES-CTR PRG with a ChaCha20-based stream cipher.

34-34: Prelude updated for crypto_traits

The crypto_traits module is correctly added to the prelude, making the cryptographic traits readily available to consumers of this crate.

common/internal-types/src/channels.rs (3)

7-8: Good feature-gating of serialization dependencies.

Making the serialization derives conditional on the serde feature flag reduces dependency bloat when serialization is not needed, which is in line with best practices for Rust library design.

66-67: Appropriate feature-gating for ChannelEntry serialization.

Consistently applying the conditional serialization pattern to all serializable types improves maintainability.

170-170: Minor documentation improvement.

Changing from "Enumerates" to "Lists" is a small but welcome improvement in documentation clarity.

crypto/types/src/errors.rs (2)

7-8: Good optimization: using static string slices.

Changing from String to &'static str for error messages reduces heap allocations, improving performance and memory usage. The error message format is also clearer now.

10-11: Enhanced error context with function name.

Converting InvalidInputValue from a unit variant to a tuple variant with a function name parameter provides better context in error messages, making debugging easier.

hoprd/inbox/src/inbox.rs (3)

104-104: Updated tag handling for consistency.

The API change to wrap the application tag in Some() reflects the updated internal representation of tags.

110-110: Consistent treatment of application tags.

Wrapping the tag in Some() ensures consistency with the modified tag model throughout the codebase.

210-232: Updated application tag initialization in tests.

Test cases correctly reflect the change in the ApplicationData structure where application_tag is now a direct Tag rather than an Option<Tag>.

transport/protocol/src/bloom.rs (4)

15-18: Good standardization of serialization configuration.

Creating a shared constant for bincode configuration ensures consistency across serialization and deserialization operations.

23-25: Improved serialization approach.

Using the standard bincode serde implementation with a consistent configuration simplifies the code and improves maintainability.

46-48: Standardized serialization pattern.

Consistently using bincode::serde::encode_to_vec with the shared configuration improves code consistency and reliability.

50-50: Fixed typo in error log.

Corrected "erorr" to "error" in the log message, improving readability and professionalism.

transport/protocol/benches/protocol_throughput_emulated.rs (4)

7-12: Import updates align with new routing abstractions.

The addition of new imports from hopr_crypto_packet, hopr_crypto_random, and the changes to import style for the internal, network, and primitive types reflect the shift towards the new routing abstractions mentioned in the PR objectives.

19-19: Good use of the standardized payload size constant.

Using HoprPacket::PAYLOAD_SIZE instead of a hardcoded value improves maintainability by centralizing this important constant.

61-65: Updated message passing to use the new routing abstraction.

The channel type has been updated to use ResolvedTransportRouting which supports the new return path functionality described in the PR objectives.

97-101: Implemented the new forward path routing with SURB support.

This code implements the new routing abstraction with explicit support for both forward and return paths as described in the PR objectives. The empty vector for return_paths is appropriate for this benchmark where return paths aren't being tested.

crypto/packet/Cargo.toml (5)

3-3: Version bump reflects significant changes.

The version increment from 0.8.0 to 0.9.0 appropriately indicates substantial changes to the packet implementation, including the addition of SURB support.

16-21: Well-structured optional serialization feature.

The new serde feature cleanly implements the optional serialization support mentioned in the PR objectives, making it available only when needed while ensuring dependent crates also enable their serialization features.

25-35: Updated dependencies align with new functionality.

Adding strum and making serde-related dependencies optional aligns with the PR goals. The addition of hopr-path as a dependency reflects the central role of path handling in the updated packet structure.

40-44: Added new dev dependencies for testing.

The addition of bimap and lazy_static in dev dependencies suggests improved testing capabilities, likely related to SURB and return path testing.

55-60: Enhanced benchmarking structure.

Renaming and adding benchmarks improves organization and suggests more comprehensive performance testing, likely covering the new SURB and return path functionality mentioned in the PR objectives.

crypto/sphinx/Cargo.toml (3)

3-3: Version bump reflects significant protocol changes.

The version increment from 0.6.1 to 0.7.0 appropriately indicates substantial changes to the Sphinx protocol implementation, supporting the new SURB functionality.

15-23: Updated dependencies support new cryptographic approaches.

The addition of bimap, serde-related dependencies, and thiserror supports the new error handling and optional serialization features mentioned in the PR objectives. The updated cryptographic libraries likely relate to replacing Lioness PRP and AES-CTR with ChaCha20-based stream cipher for better performance.

42-49: Well-structured optional serialization feature.

The new serde feature cleanly implements optional serialization support, consistent with the approach in other crates and aligning with the PR objectives to make serialization optional via feature flags.

crypto/sphinx/src/errors.rs (1)

1-19: Well-designed error handling structure.

The new SphinxError enum provides a comprehensive and structured approach to error handling for the Sphinx protocol implementation. The use of thiserror ensures good error messages, and the #[from] attributes facilitate error propagation from lower-level error types. This structure will improve debugging and error handling throughout the protocol stack.

logic/path/src/selectors/dfs.rs (4)

14-14: Import change aligns with structural refactoring

The import path for ChannelPath has been updated to reflect its new location at the crate root instead of from the removed path module, which is consistent with the broader refactoring described in the PR summary.

277-277: API method change affects path construction

The method to create a ChannelPath has changed from new_valid() to from_iter(), which suggests a shift in validation responsibility. This aligns with the PR's goal of making path validation more robust, asynchronous, and centralized.

339-346: Path validation is now asynchronous

The check_path function has been properly refactored to leverage the new async validation model, creating a ValidatedPath from a ChainPath derived from the given ChannelPath.

This change is significant for the following reasons:

Validation is now performed asynchronously

It uses the new ValidatedPath type, a core type for representing resolved and validated routes

It leverages ChainPath for better type safety

595-596: Test assertions updated to reflect API changes

Tests have been correctly updated to use path.num_hops() instead of path.length() and to await the asynchronous check_path function.

These changes ensure that tests properly validate the new path validation logic.

Also applies to: 612-613, 629-630, 646-647

transport/api/src/config.rs (2)

104-126: Improved configuration with conditional compilation

The refactoring of default_multiaddr_transport using cfg_if is a significant improvement:

Consolidates multiple conditional function definitions into a single function

Handles environment variables (DAPPNODE, HOPRD_NAT) to intelligently determine transport type

Maintains backward compatibility while streamlining the codebase

This change enhances maintainability and readability without changing functionality.

312-322: Tests properly updated for feature-flagged behavior

The updated test implementations correctly use feature flags to test different compilation scenarios. This ensures that all transport configuration variations are tested appropriately based on:

Whether the transport-quic feature is enabled

Various environment variable combinations

Different NAT/network configurations

This comprehensive test coverage is crucial for verifying the new conditional transport logic.

Also applies to: 331-361

logic/path/Cargo.toml (4)

3-3: Appropriate version bump for significant changes

Incrementing from 0.7.0 to 0.8.0 appropriately reflects the significant changes in this crate, including new path validation abstractions and serialization features.

16-24: Well-structured feature flags for optional serialization

The new serde feature is well-organized, making serialization optional across multiple dependencies. This modular approach:

Reduces dependencies when serialization isn't needed

Provides a single feature flag to enable serialization across all relevant dependencies

Follows Rust best practices for feature gating optional functionality

The prometheus feature update to use direct dependency declaration (dep:lazy_static) is also cleaner than the previous approach.

29-36: Dependencies properly restructured and optimized

The dependency updates align with the PR objectives:

Optional dependencies are correctly marked as such

New dependencies support the enhanced path validation and routing abstractions

Workspace dependencies are used consistently for shared dependencies

These changes maintain a clean dependency graph while supporting the new features.

48-54: Enhanced testing capabilities with new dependencies

The addition of testing tools like bimap, parameterized, and mockall suggests improved test coverage and methodology, which is important given the significant refactoring in this PR.

crypto/types/Cargo.toml (3)

3-3: Version bump reflects significant cryptographic changes

The update to version 0.2.0 appropriately indicates the significant changes to cryptographic dependencies and feature structure.

8-21: Cryptographic library updates enhance security and performance

The cryptographic dependency changes align with the PR's objective to improve performance:

Addition of blake3 (replacing blake2) for faster hashing

New dependencies for ChaCha20-based stream cipher implementation (chacha20, poly1305)

Support for additional cryptographic primitives (aes, cipher, ctr)

These changes support the stated goal of replacing Lioness PRP and AES-CTR PRG with a ChaCha20-based stream cipher for better performance.

23-24: Well-implemented optional serialization support

Making serialization optional through a dedicated feature flag is an excellent practice for cryptographic libraries:

Reduces attack surface when serialization isn't needed

Makes dependencies explicit and optional

Provides a clean way to enable serialization across multiple dependencies

This approach aligns with the broader effort to modularize serialization support via feature flags.

Also applies to: 35-44

transport/p2p/src/swarm.rs (1)

290-313: Nice alignment with the new ticket types

Switching the request/response generics to Vec<TransferableWinningTicket> and Result<Ticket, String> removes the last dependency on the legacy ticket layer and compiles cleanly with the updated protocol behaviour.

transport/protocol/src/ticket_aggregation/processor.rs (1)

63-66: Good: Send variant now uses the canonical ticket type

Moving away from the legacy AcknowledgedTicket keeps the surface smaller and removes an unnecessary conversion step. 👍

transport/protocol/src/msg/packet.rs (4)

15-16: Field structure enhancement for Final packet acknowledgement handling

The change from a single ack: Acknowledgement field to separate ack_key: HalfKey and no_ack: bool fields in the IncomingPacket::Final variant aligns with the PR's goal of introducing SURB support. This split provides explicit control over whether acknowledgements should be sent back.

37-38: Consistent propagation of acknowledgement fields in conversion implementation

The conversion implementation for TransportPacketWithChainData correctly handles the new split acknowledgement fields, maintaining consistency with the structural changes in the packet types.

Also applies to: 43-44

100-101: Consistent field structure in TransportPacket::Final

The TransportPacket::Final variant has been updated in line with the changes to IncomingPacket::Final, ensuring consistency across the packet type hierarchy.

126-127: Proper field mapping in conversion implementation

The From<IncomingPacket> for TransportPacket implementation correctly maps the new acknowledgement-related fields, ensuring proper data transfer between packet types.

Also applies to: 132-133

crypto/types/src/vrf.rs (3)

26-27: Feature gating of serde::Serialize implementation

The serde::Serialize implementation is now conditionally compiled based on the serde feature flag, aligning with the PR objective of making serialization optional. This reduces dependencies when serialization is not needed.

37-57: Well-structured deserialization code with feature gating

Moving the deserialization visitor into a dedicated de module and applying the serde feature gate improves code organization while maintaining the optional serialization approach.

60-68: Consistent feature gating of Deserialize trait implementation

The serde::Deserialize implementation is properly feature-gated, maintaining consistency with the other serialization-related code. This provides clean conditional compilation when serialization is not needed.

crypto/types/src/keypairs.rs (6)

18-18: Removed ZeroizeOnDrop and simplified trait bounds

Removing the ZeroizeOnDrop trait bound from the Keypair trait and generalizing the SecretLen associated type improves flexibility and aligns with the PR's goal of simplifying types and updating cryptographic dependencies.

Also applies to: 20-20

44-45: Removed ZeroizeOnDrop from OffchainKeypair

The ZeroizeOnDrop derive has been removed from OffchainKeypair, consistent with the trait changes and the PR's objective of updating cryptographic dependencies.

58-58: Improved error handling with context

The error message in InvalidInputValue now includes a "bytes" context string, providing more specific information about the error source, which improves debugging.

87-87: Minor comment wording improvement

The comment wording has been refined to better explain the relationship between the secret keys and the Ed25519 peer IDs.

113-114: Removed ZeroizeOnDrop from ChainKeypair

Similar to OffchainKeypair, the ZeroizeOnDrop trait has been removed from ChainKeypair, ensuring consistency across the codebase's keypair implementations.

131-134: Consistent error handling and improved code formatting

The from_secret implementation for ChainKeypair now uses consistent error handling with the "bytes" context string and has improved code formatting for better readability.

common/internal-types/src/errors.rs (3)

14-14: Standardized error message formatting

Error messages for ArithmeticError and InvalidTicketRecipient have been updated to use lowercase formatting, improving consistency across error messages in the codebase.

Also applies to: 17-17

20-21: Added InvalidAcknowledgement error variant

The new InvalidAcknowledgement error variant supports the enhanced acknowledgement handling introduced with the packet structure changes. This aligns with the PR's focus on improving packet routing and acknowledgements.

23-24: Reordered LoopbackTicket error variant

The LoopbackTicket error variant has been repositioned in the enum to maintain logical grouping with the new InvalidAcknowledgement variant, as both relate to protocol validation.

hoprd/rest-api/src/messages.rs (5)

17-21: Cleaner import organization.

The imports have been reorganized with crate imports first, followed by external imports, making the code more maintainable.

140-143: Appropriate variable renaming from peer_id to peer_addr.

This change correctly reflects the shift from using PeerId to Address types, making the code more consistent with the updated routing abstractions.

170-171: Consistent use of addresses instead of peer IDs.

This change ensures that intermediate paths use addresses instead of peer IDs, aligning with the new routing abstractions.

204-211: Well-structured implementation of the new routing approach.

The code now uses the new DestinationRouting::Forward struct to encapsulate routing information, which is consistent with the PR's goal of adding support for return paths. The comment about API deprecation is also helpful.

307-314: Simplified application tag handling.

The error handling for missing application_tag has been removed, reflecting a change in the underlying ApplicationData structure where application_tag is now non-optional.

transport/network/src/messaging.rs (6)

1-2: Good use of crypto primitives.

The imports for random_fill and blake3_hash are appropriate for the new nonce derivation implementation.

7-8: Updated error import path.

The error import has been updated to use a more specific path, which improves code clarity.

10-11: Well-defined constant for nonce size.

Introducing a named constant PING_PONG_NONCE_SIZE improves code readability and maintainability.

13-24: Clean implementation of derive_ping_pong function.

This function efficiently implements nonce generation for ping/pong messages using cryptographic primitives. It either generates a random nonce or derives a response from a challenge using Blake3 hashing.

84-85: Updated nonce type in PingMessage.

The nonce field now uses the defined constant for its size, ensuring consistency throughout the code.

116-117: Updated constant reference.

The PING_MESSAGE_LEN constant now references the new PING_PONG_NONCE_SIZE constant, maintaining consistency.

common/primitive-types/src/primitives.rs (5)

507-509: New type KeyIdent for fixed-size key identifiers.

This generic struct is well-designed to represent fixed-size byte array identifiers for public keys, supporting the PR's goal of introducing Key IDs to shorten the Sphinx header size.

511-515: Good implementation of Display trait.

The Display implementation using hex strings makes the identifiers human-readable, which is helpful for debugging and logging.

517-539: Convenient conversions between numeric types and KeyIdent.

The implementations for converting between u32/u64 and KeyIdent<4>/KeyIdent<8> are valuable for interoperability and serialization.

541-547: Appropriate error handling in TryFrom implementation.

The implementation properly handles conversion errors when trying to create a KeyIdent from a byte slice of incorrect length.

549-563: Complete implementation of required traits.

The implementations for AsRef<[u8]>, Default, and BytesRepresentable ensure that KeyIdent integrates well with the rest of the codebase and follows established patterns.

crypto/packet/benches/por_benches.rs (5)

1-17: Well-structured benchmark module setup.

The module properly imports and sets up the necessary components for benchmarking the Proof-of-Relay functionality.

18-19: Appropriate sample size for benchmarks.

The large sample size (100,000) will provide statistically significant performance measurements.

20-39: Comprehensive benchmarking of PoR creation.

This benchmark thoroughly tests the performance of proof-of-relay generation across different hop counts (0-3), providing valuable insights into the efficiency of the implementation.

41-53: Important benchmark for PoR pre-verification.

This benchmark measures a critical operation in the PoR workflow, which is essential for understanding the performance characteristics of the protocol.

55-56: Standard Criterion benchmark setup.

The benchmark group and main function are set up correctly using the Criterion framework.

common/internal-types/src/account.rs (4)

7-8: Good implementation of conditional serialization.

The conditional serialization using cfg_attr is a clean approach that makes the serialization optional via a feature flag, which aligns with the PR objectives of making serialization optional in multiple crates.

36-37: Consistent use of conditional serialization.

Applying the same conditional serialization pattern to the AccountEntry struct, maintaining consistency across the codebase.

42-42: New field added to track on-chain inclusion.

The published_at field now stores the block number when an account was published on-chain, which matches the PR objective of adding "published_at information to track on-chain inclusion."

160-160: Test case validates key_id computation.

The test verifies that the key_id method returns the expected hex value, which is important for ensuring consistency in key identification.

crypto/sphinx/src/ec_groups.rs (10)

71-71: Improved error specificity.

Adding the "alpha" parameter to the InvalidInputValue error provides better context about what value was invalid, improving error reporting.

101-101: Consistent error handling improvement.

Similar improvement to error handling as in the previous implementation, maintaining consistency across different curve implementations.

115-116: Added proper trait derivation with conditional serialization.

Adding the essential traits (Clone, Copy, Debug, PartialEq, Eq) and conditional serialization support to Secp256k1Suite improves usability and consistency with the rest of the codebase.

124-124: Replaced PRP implementation with ChaCha20.

Added the PRP associated type using ChaCha20, aligning with the PR objective to replace "Lioness PRP and AES-CTR PRG with a ChaCha20-based stream cipher for better performance."

129-130: Consistent trait derivation for Ed25519Suite.

Applying the same trait derivations and conditional serialization as with the Secp256k1Suite, maintaining consistency across implementations.

138-138: Consistent PRP implementation for Ed25519Suite.

Same ChaCha20 implementation applied to Ed25519Suite, maintaining consistency across different cryptographic suites.

143-144: Consistent trait derivation for X25519Suite.

Applying the same trait derivations and conditional serialization as with other suites, maintaining consistency across implementations.

152-152: Consistent PRP implementation for X25519Suite.

Same ChaCha20 implementation applied to X25519Suite, ensuring consistent cryptographic primitives across all suite implementations.

168-172: Updated test to use new key extraction method.

The test was updated to use extract_key with a new parameter "test" and verifies against an updated expected hash value, reflecting the changes in the key derivation logic.

183-188: Unified key extraction testing.

The test now uses extract_key instead of expand_key, unifying the expected output between tests and reflecting the updated key derivation logic.

crypto/sphinx/src/lib.rs (3)

22-34: Improved module organization and access control.

The refactoring of module visibility from public to private (for derivation, ec_groups, routing, shared_keys) improves encapsulation, while adding new modules (errors, packet, surb) supports the new SURB functionality mentioned in the PR objectives.

36-44: Well-designed prelude for simplified imports.

The prelude module exports the key types and traits, making it easier for consumers to use the library with minimal imports. This is a good API design practice.

46-100: Comprehensive test utilities.

The new test module provides good scaffolding with WrappedBytes and TestSpec implementations to support testing of the new SURB and header functionality.

db/sql/src/accounts.rs (6)

149-150: Added published_at field in model_to_account_entry.

Correctly mapping the database field to the new published_at field in the AccountEntry struct.

228-228: Insert query updated for published_at field.

The insert query now includes the published_at field, ensuring it's properly stored in the database.

252-252: Key ID mapper synchronization.

The cache is properly updated with the new key ID binding after account insertion, which aligns with the PR objective of "a bi-directional mapping between Key IDs and OffchainPublicKeys is maintained synchronously in the database cache."

471-471: Test updated for published_at field.

Test cases have been properly updated to include the new published_at field in AccountEntry construction.

480-480: Test validates published_at value.

Good test coverage ensuring the published_at field is correctly handled and stored.

597-597: Tests use consistent published_at values.

All tests consistently initialize AccountEntry instances with the published_at field, using incrementing values (1, 2, 3) that provide good test coverage and match the field's purpose.
hopr/hopr-lib/src/lib.rs (1)

1102-1108:
❓ Verification inconclusive

Unchecked propagation of Tag validity

application_tag is passed straight through without validating that it
is higher than RESERVED_TAG_UPPER_LIMIT.
A malicious caller could inject a reserved tag and subvert protocol
logic.

Consider clamping / validating:
if application_tag <= RESERVED_TAG_UPPER_LIMIT {
    return Err(HoprLibError::GeneralError(
        format!("application tag {} is in reserved range", application_tag)
    ));
}
#!/bin/bash
set -e

echo "=== Searching for Tag definition or alias ==="
rg --ignore-case "^(pub )?(struct|enum|type) Tag" -n hopr/hopr-lib/src

echo
echo "=== Searching for Tag implementation (e.g., constructors) ==="
rg --ignore-case "impl Tag" -n hopr/hopr-lib/src

echo
echo "=== Searching for RESERVED_TAG_UPPER_LIMIT constant ==="
rg "RESERVED_TAG_UPPER_LIMIT" -n hopr/hopr-lib/src
#!/bin/bash
set -e

echo "=== Locate Tag definition ==="
rg -n "^(pub )?(struct|enum|type) Tag" hopr/hopr-lib/src

echo
echo "=== Find RESERVED_TAG_UPPER_LIMIT constant ==="
rg -n "RESERVED_TAG_UPPER_LIMIT" hopr/hopr-lib/src

echo
echo "=== Inspect send_message signature ==="
rg -n "send_message" hopr/hopr-lib/src
🏁 Script executed:
#!/bin/bash
# Verify Tag definition and validations
echo "=== Tag definition ==="
rg -n "^(pub )?(struct|enum|type)\s+Tag" hopr/hopr-lib/src || true

echo
echo "=== RESERVED_TAG_UPPER_LIMIT constant ==="
rg -n "RESERVED_TAG_UPPER_LIMIT" hopr/hopr-lib/src || true

echo
echo "=== send_message signature context ==="
rg -n "send_message" -n hopr/hopr-lib/src -A3 || true

echo
echo "=== Tag constructors or TryFrom implementations ==="
rg -n "Tag::new" -n hopr/hopr-lib/src || true
rg -n "impl\s+TryFrom<u8>\s+for\s+Tag" -n hopr/hopr-lib/src || true
Length of output: 1174
#!/bin/bash
set -e

echo "=== Imports of Tag in lib.rs ==="
rg -n "use .*Tag" hopr/hopr-lib/src/lib.rs || true

echo
echo "=== Listing constants files ==="
ls hopr/hopr-lib/src | grep -E "constants(\.rs|/)" || true

echo
if [[ -f hopr/hopr-lib/src/constants.rs ]]; then
  echo "=== Tag & RESERVED_TAG_UPPER_LIMIT in constants.rs ==="
  rg -n "Tag" hopr/hopr-lib/src/constants.rs || true
  rg -n "RESERVED_TAG_UPPER_LIMIT" hopr/hopr-lib/src/constants.rs || true
elif [[ -d hopr/hopr-lib/src/constants ]]; then
  echo "=== Tag & RESERVED_TAG_UPPER_LIMIT in constants/mod.rs ==="
  rg -n "Tag" hopr/hopr-lib/src/constants/mod.rs || true
  rg -n "RESERVED_TAG_UPPER_LIMIT" hopr/hopr-lib/src/constants/mod.rs || true
else
  echo "No constants.rs or constants/ directory found."
fi
#!/bin/bash
set -e

echo "=== Tag declaration ==="
rg -n "^(pub )?(struct|enum|type) Tag" hopr/hopr-lib/src

echo
echo "=== impl Tag or Tag methods ==="
rg -n "impl\s+Tag" hopr/hopr-lib/src || true
rg -n "fn\s+new\(\)\s*->\s*Tag" -n hopr/hopr-lib/src || true

echo
echo "=== From/TryFrom implementations for Tag ==="
rg -n "TryFrom<[^>]+> for Tag" hopr/hopr-lib/src || true
rg -n "From<[^>]+> for Tag" hopr/hopr-lib/src || true
Validate application_tag against reserved tags
Confirm whether the Tag type or its constructor already rejects values ≤ RESERVED_TAG_UPPER_LIMIT. Without this, a caller could inject a reserved tag and subvert protocol logic. If no such check exists, add one in send_message:
if application_tag <= constants::RESERVED_TAG_UPPER_LIMIT {
    return Err(HoprLibError::GeneralError(
        format!("application tag {} is in reserved range", application_tag),
    ));
}
• File: hopr/hopr-lib/src/lib.rs
• Location: inside pub async fn send_message (around lines 1102–1108)
transport/protocol/src/lib.rs (2)

270-276: 🛠️ Refactor suggestion

Potential unbounded growth of the outgoing‑message buffer

then_concurrent() is fed by an unbounded upstream (api.1) and itself forwards only successful packets (Some(v)) to the transport sink.
If PacketWrapping::send() keeps returning errors (e.g. no available SURBs) finalizer.finalize(Err(_)) is called, but nothing limits the rate of new attempts – the closure will spin and may eat CPU.

Consider back‑pressuring the api source or switching to a bounded channel with try_send/poll_ready to propagate congestion upwards:
-let (tx, rx) = futures::channel::mpsc::unbounded();
+let (tx, rx) = futures::channel::mpsc::channel(128); // tune capacity
⛔ Skipped due to learnings
Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6701
File: transport/api/src/lib.rs:361-361
Timestamp: 2024-12-05T15:16:42.369Z
Learning: In `transport/api/src/lib.rs`, unbounded channels like `msg_to_send_tx` and `msg_to_send_rx` are acceptable for internal use, as memory is not an issue, and external channels have been changed to bounded channels.
161-165:
❓ Verification inconclusive

Make callers verify the new tuple signature

The stream that feeds outbound packets now yields (ApplicationData, ResolvedTransportRouting, PacketSendFinalizer).
This is an API break: every caller constructing the stream must be updated, otherwise the compiler will complain but downstream integration tests might silently convert with map(|(_, p, _)| … ) in ad‑hoc places.

Please grep for previous to_send() invocations that still return (ApplicationData, TransportPath, …) and adjust them.

🏁 Script executed:
rg -A2 -B2 "to_send(" | grep TransportPath
Length of output: 111

To catch any lingering to_send() calls still using the old TransportPath tuple, let’s retry with an escaped parenthesis and fixed-string search:

🏁 Script executed:
#!/usr/bin/env bash
# Search for to_send( invocations and filter for TransportPath usage
rg -F 'to_send(' -A2 -B2 . | rg 'TransportPath'
Length of output: 47
Double‑check to_send() callers for the new tuple signature
Ran:
rg -F 'to_send(' -A2 -B2 . | rg 'TransportPath'
→ no matches
Please manually verify that every to_send() invocation now consumes
(ApplicationData, ResolvedTransportRouting, PacketSendFinalizer)
and update any remaining callers that still expect
(ApplicationData, TransportPath, …) to avoid ad‑hoc map(|(_, p, _)| …) fallbacks.
transport/p2p/src/lib.rs (1)

104-107: Double‑check CBOR (de)serialisation for the new ticket types

libp2p::request_response::cbor::Behaviour requires both Request and Response to implement serde traits.
TransferableWinningTicket almost certainly does, but std::result::Result<Ticket, String> only implements them when Ticket does.
If Ticket is behind serde feature‑gating, compile with the relevant feature set in CI to avoid a compile‑time failure when building binaries with default features.

crypto/sphinx/src/surb.rs (1)

115-146: Offset math re‑used from two different const families – please verify

into_boxed uses RoutingInfo::<H>::SIZE, whereas try_from switches to H::HEADER_LEN + H::TAG_SIZE when computing the offset for sender_key and additional_data_receiver.
If those two formulas ever diverge, deserialization will corrupt the parsed fields silently.

Please double‑check that
RoutingInfo::<H>::SIZE == H::HEADER_LEN + H::TAG_SIZE
is guaranteed by trait invariants, or unify both call‑sites to a single constant to avoid future regressions.

crypto/packet/src/lib.rs (1)

51-59: Missing size constants in HoprSphinxHeaderSpec could break trait invariants

SphinxHeaderSpec usually requires KEY_ID_SIZE and SURB_RECEIVER_DATA_SIZE as consts, yet HoprSphinxHeaderSpec only defines associated types.
If the trait provides no defaults, this will fail to compile; if defaults exist, they may be wrong for HOPR.

Please confirm the implementation is complete and update the trait impl accordingly.

crypto/sphinx/src/routing.rs (1)

299-307: Double‑check additional_data_relayer indexing

When idx > 0, the code uses
additional_data_relayer.get(inverted_idx) where inverted_idx = secrets.len() - idx - 1.

For a path of n hops the additional data slice normally has length n − 1 (no entry for the final hop).
Using inverted_idx instead of inverted_idx + 1 means the element for hop k is fetched from position k − 1, causing an off‑by‑one when additional_data_relayer.len() == secrets.len() − 1.
Please verify that this is intentional; if not, adjust the index.

logic/path/src/lib.rs (1)

293-299: Only immediate self‑loops are rejected – long routing loops remain possible

The ticket_issuer == ticket_receiver guard stops a direct A → A hop,
yet a path such as A → B → A passes this check and is explicitly allowed by the tests.
Although this might be intentional (e.g. for re‑entry paths), please double‑check that longer loops cannot provoke replay or fee‑leakage issues on routed packets.

common/internal-types/src/tickets.rs (1)

328-330: serde gating looks good

Nice use of cfg_attr(feature = "serde", derive(...)) – this keeps the core crate lightweight while still allowing consumers to opt‑in to serialisation.

db/sql/src/protocol.rs (1)

435-437: insert operation is fire‑and‑forget – confirm it is synchronous.

pseudonym_openers.insert(..) is invoked inside for_each without .await, whereas all other cache inserts in this file are async and awaited.
If pseudonym_openers was switched from an async map (e.g. dashmap::DashMap) to a plain sync container this is fine, otherwise the call silently discards the future and the opener will never be stored.

Please double‑check the type of pseudonym_openers and wrap the call in tokio::spawn/.await if it is still async.
Failing to persist the opener will break subsequent SURB replies.

crypto/packet/src/packet.rs (1)

135-142: ack_challenge still populated for NoAck packets

For the PacketRouting::NoAck branch the header sets no_ack: true, yet ack_challenge is retained.
Down‑stream components should never attempt to solve or store an ACK challenge for a no‑ack packet. Either:

Set ack_challenge to a sentinel (HalfKeyChallenge::default()) and document this, or

Change the type to Option<HalfKeyChallenge> and use None here.

Failing to do so risks logic that unintentionally queues impossible acknowledgements.

transport/p2p/src/swarm.rs

common/internal-types/src/account.rs

db/sql/src/accounts.rs

logic/path/src/channel_graph.rs

hopr/hopr-lib/src/lib.rs

crypto/types/src/types.rs

crypto/packet/src/por.rs

common/internal-types/src/protocol.rs

crypto/packet/src/packet.rs

coderabbitai

Actionable comments posted: 2

🔭 Outside diff range comments (1)

crypto/sphinx/src/shared_keys.rs (1)
83-90: 🛠️ Refactor suggestion

Reject empty peer list to avoid “empty‑path” corner cases early

SharedKeys::generate silently succeeds with peer_group_elements.is_empty(),
producing an alpha but zero shared secrets.
Down‑stream code (e.g. RoutingInfo::new) expects path.len() == secrets.len()
and will error later.

Add an explicit guard to fail fast:
 if peer_group_elements.is_empty() {
-    // falls through and returns secrets.len() == 0
-}
+    return Err(CryptoError::InvalidInputValue("peer_group_elements"));
+}
Failing early keeps the error source closer to the caller and prevents
half‑initialised SURBs/packets from being created.

♻️ Duplicate comments (1)

crypto/packet/src/packet.rs (1)

318-321: Arithmetic underflow correctly guarded

max_surbs_with_message now uses saturating_sub, preventing the wrap‑around
reported in an earlier review. 👍

🧹 Nitpick comments (5)

crypto/types/Cargo.toml (1)
8-14: Add zeroize feature to new cipher crates to avoid key‑material leakage

aes, chacha20, ctr, and poly1305 all keep key material in memory.
Because the zeroize feature is opt‑in, those keys will not be wiped on drop, which is a regression compared to the previous Blake2s → Blake3 switch where zeroization was preserved.
-aes = "0.8.4"
-chacha20 = "0.9.1"
-ctr = "0.9.2"
-poly1305 = { version = "0.8.0" }
+aes      = { version = "0.8.4", features = ["zeroize"] }
+chacha20 = { version = "0.9.1", features = ["zeroize"] }
+ctr      = { version = "0.9.2", features = ["zeroize"] }
+poly1305 = { version = "0.8.0", features = ["zeroize"] }
This keeps the “secrets‑never‑linger” guarantee consistent across the code‑base.
(If there is a strong reason not to zeroize, please document it explicitly.)

Also applies to: 31-31
crypto/sphinx/src/surb.rs (1)
46-66: Factor repeated offset maths into helpers for maintainability

Both into_boxed and TryFrom manually re‑calculate byte offsets with long
expressions. This is error‑prone (see previous bug) and hard to audit.

Consider:
const fn offsets(alpha: usize) -> (usize, usize, usize, usize) {
    let k = H::KEY_ID_SIZE.get();
    let a = alpha;
    let r = RoutingInfo::<H>::SIZE;
    let s = SecretKey16::LENGTH;
    (k, k + a, k + a + r, k + a + r + s)
}
and re‑using the tuple everywhere. Future field additions will then update a
single place.

Also applies to: 118-141
crypto/types/src/utils.rs (1)
4-4: Remove unused cipher::zeroize re‑export to avoid shadowing

use cipher::zeroize; is currently unused and shadows the external
zeroize crate used in #[derive(..., zeroize::ZeroizeOnDrop)]. While it
still compiles (cipher re‑exports the same module), the import is redundant and
may confuse future readers.
-use cipher::zeroize;
crypto/packet/src/packet.rs (2)
578-583: Potential panic: negative array index when node_pos == 0 in reply path

In the reply branch the fallback arm subtracts 1 from node_pos, yielding
PEERS[node_pos - 1].
If a malformed test (or future code change) feeds node_pos = 0 while the
packet is still in the Forwarded state this will underflow and panic in
debug builds or produce an out‑of‑bounds access in release builds.

Consider making the match explicit and defensive:
-(_, true) => PEERS[node_pos - 1].0.public().0.clone(),
+(_, true) if node_pos > 0 => PEERS[node_pos - 1].0.public().0.clone(),
+(_, true) => unreachable!("node_pos == 0 cannot be Forwarded in reply path"),
70-74: TicketBuilder::leak() may unintentionally bypass zeroisation

leak() transfers ownership into a 'static reference, preventing the ticket
bytes (and therefore the on‑chain signature) from being zeroised when dropped.
Unless the ticket must live for the entire program lifetime, prefer
into()/build() (or an explicit clone) so the sensitive material is still
wiped when the Ticket value goes out of scope.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2dff90b and 49a7166.

⛔ Files ignored due to path filters (1)

Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (7)

crypto/packet/src/packet.rs (1 hunks)
crypto/sphinx/src/derivation.rs (3 hunks)
crypto/sphinx/src/shared_keys.rs (6 hunks)
crypto/sphinx/src/surb.rs (1 hunks)
crypto/types/Cargo.toml (1 hunks)
crypto/types/src/utils.rs (2 hunks)
transport/protocol/src/msg/processor.rs (9 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

crypto/sphinx/src/derivation.rs
transport/protocol/src/msg/processor.rs

🧰 Additional context used

🧬 Code Graph Analysis (2)

crypto/sphinx/src/surb.rs (2)

crypto/sphinx/src/routing.rs (7)

clone (181-183)

try_from (161-167)

try_from (209-215)

new (117-129)

new (232-341)

keypairs (523-523)

default (195-197)

crypto/sphinx/src/shared_keys.rs (2)

random (14-14)

new_shared_keys (159-163)

crypto/packet/src/packet.rs (7)

crypto/sphinx/src/surb.rs (4)

fmt (104-112)

fmt (160-165)

try_from (118-145)

clone (88-96)

crypto/packet/src/por.rs (13)

fmt (63-69)

fmt (119-124)

derive_ack_key_share (21-23)

generate_proof_of_relay (186-221)

pre_verify (163-183)

new (33-39)

new (99-104)

try_from (81-86)

try_from (130-135)

hops (292-292)

as_ref (73-75)

as_ref (139-141)

chain_length (43-45)

crypto/sphinx/src/packet.rs (13)

fmt (254-260)

fmt (297-299)

new (41-52)

new (198-240)

new (361-367)

shared_keys (226-231)

forward_path (208-215)

try_from (497-506)

BiHashMap (702-705)

clone (264-270)

clone (304-309)

as_ref (106-108)

as_ref (489-491)

crypto/sphinx/src/routing.rs (11)

fmt (175-177)

new (117-129)

new (232-341)

try_from (161-167)

try_from (209-215)

from (153-155)

clone (181-183)

default (195-197)

is_reply (132-134)

hops (511-511)

as_ref (201-203)

crypto/sphinx/src/shared_keys.rs (2)

new_shared_keys (159-163)

random (14-14)

crypto/packet/src/types.rs (5)

from_parts (53-72)

data (36-36)

from (76-78)

from (82-84)

BiHashMap (121-124)

crypto/types/src/keypairs.rs (9)

from_secret (29-29)

from_secret (56-61)

from_secret (128-135)

public (35-35)

public (67-69)

public (141-143)

random (26-26)

random (51-54)

random (120-126)

⏰ Context from checks skipped due to timeout of 90000ms (5)

GitHub Check: hoprd / docker
GitHub Check: tests-smoke-websocket
GitHub Check: tests-unit-nightly
GitHub Check: tests-unit
GitHub Check: Linter

🔇 Additional comments (3)

crypto/sphinx/src/surb.rs (1)

69-80: Double‑check whether additional_data_receiver should be in the hash

get_hash() omits additional_data_receiver. Two SURBs that differ only in
this field will hash to the same value, which might weaken SURB‑dedup or replay
protections upstream.

Please verify the intended domain‑separation; if uniqueness is required, append
self.additional_data_receiver.as_ref() to the input list.
crypto/types/src/utils.rs (1)

59-67: Ensure DST length ≤ 255 in sample_secp256k1_field_element

The hash‑to‑curve draft constrains the domain separation tag to max 255 bytes.
"secp256k1_XMD:SHA3-256_SSWU_RO_" (29 B) + tag.len() could overflow this
limit if callers pass long strings.

Add a check (or truncate) to guarantee protocol compliance:
 if secret.len() >= SecretKey::LENGTH {
+    if 29 + tag.len() > 255 {
+        return Err(InvalidParameterSize { name: "tag", expected: 226 });
+    }
crypto/packet/src/packet.rs (1)

234-241: Display implementation matches enum variants correctly

Tuple‑like variants are matched with Self::Variant(_), fixing the previous
compile error.

crypto/sphinx/src/surb.rs

crypto/packet/src/packet.rs

coderabbitai

Actionable comments posted: 0

♻️ Duplicate comments (1)

db/sql/src/cache.rs (1)

186-215: Non-atomic double-map insertion could lead to inconsistent state

This implementation retrieves entries from both maps independently and then inserts values conditionally. If there's concurrent access between these operations, the maps could become inconsistent. This was previously flagged in an earlier review.

Consider using atomic operations by locking both maps simultaneously or implementing a single atomic update operation:

 pub fn update_key_id_binding(&self, account: &AccountEntry) -> Result<(), DbSqlError> {
     let id = account.key_id();
-
-    // Lock entries in the maps to avoid concurrent modifications
-    let id_entry = self.0.entry(id);
-    let key_entry = self.1.entry(account.public_key);
-
-    match (id_entry, key_entry) {
-        (Entry::Vacant(v_id), Entry::Vacant(v_key)) => {
-            v_id.insert(account.public_key);
-            v_key.insert(id);
-            tracing::debug!(%id, %account.public_key, "inserted key-id binding");
-            Ok(())
-        }
-        (Entry::Occupied(v_id), Entry::Occupied(v_key)) => {
-            // Check if the existing binding is consistent with the new one.
-            if v_id.get() != v_key.key() {
-                Err(DbSqlError::LogicalError(format!(
-                    "attempt to insert key {} with key-id {id} already exists for the key {}",
-                    v_key.key(),
-                    v_id.get()
-                )))
-            } else {
-                Ok(())
-            }
-        }
-        // This should never happen.
-        _ => Err(DbSqlError::LogicalError("inconsistent key-id binding".into())),
-    }
+    // Use a single atomic operation to update both maps
+    let key_to_id = &self.1;
+    let result = self.0.try_lock(|id_map| {
+        key_to_id.try_lock(|key_map| {
+            let id_entry = id_map.entry(id);
+            let key_entry = key_map.entry(account.public_key);
+            
+            match (id_entry, key_entry) {
+                (Entry::Vacant(v_id), Entry::Vacant(v_key)) => {
+                    v_id.insert(account.public_key);
+                    v_key.insert(id);
+                    tracing::debug!(%id, %account.public_key, "inserted key-id binding");
+                    Ok(())
+                }
+                (Entry::Occupied(v_id), Entry::Occupied(v_key)) => {
+                    // Check if the existing binding is consistent with the new one.
+                    if v_id.get() != v_key.key() {
+                        Err(DbSqlError::LogicalError(format!(
+                            "attempt to insert key {} with key-id {id} already exists for the key {}",
+                            v_key.key(),
+                            v_id.get()
+                        )))
+                    } else {
+                        Ok(())
+                    }
+                }
+                // This should never happen.
+                _ => Err(DbSqlError::LogicalError("inconsistent key-id binding".into())),
+            }
+        })
+    }).unwrap_or_else(|_| Err(DbSqlError::LogicalError("failed to lock key-id maps".into())))?;
+    
+    result
 }

🧹 Nitpick comments (4)

db/sql/src/cache.rs (4)

60-68: The RingBuffer capacity could be made configurable

The hardcoded capacity of 10,000 SURBs (~7MB) is quite large. Consider making this value configurable through settings or environment variables to allow adaptation to different deployment scenarios.

-        // TODO: make the RB capacity configurable in the future ?
-        // With the current packet size, this is roughly for 7 MB of data budget in SURBs
-        Self(Arc::new(Mutex::new(AllocRingBuffer::new(10_000))))
+        // Default capacity of 10,000 SURBs (roughly 7MB with current packet size)
+        let capacity = std::env::var("HOPR_SURB_BUFFER_CAPACITY")
+            .ok()
+            .and_then(|s| s.parse::<usize>().ok())
+            .unwrap_or(10_000);
+        Self(Arc::new(Mutex::new(AllocRingBuffer::new(capacity))))

167-168: Add more context for the key_id_mapper exclusion

The comment doesn't explain why the key_id_mapper shouldn't be invalidated. Adding a brief explanation would help future maintainers understand this design decision.

-        // NOTE: key_id_mapper intentionally not invalidated
+        // NOTE: key_id_mapper intentionally not invalidated because it contains 
+        // critical mappings that are expensive to rebuild and must persist
+        // across cache invalidation cycles

78-85: Add retry logic for temporary lock failures

The current implementation simply returns an error if the mutex lock fails. Adding retry logic would improve resilience in high-concurrency scenarios.

 pub fn pop_one(&self) -> Result<HoprSurb, DbSqlError> {
-    self.0
-        .lock()
-        .map_err(|_| DbSqlError::LogicalError("failed to lock surbs".into()))?
-        .dequeue()
-        .ok_or(DbSqlError::NoSurbAvailable)
+    const MAX_RETRIES: usize = 3;
+    let mut attempts = 0;
+    
+    while attempts < MAX_RETRIES {
+        match self.0.lock() {
+            Ok(mut guard) => {
+                return guard.dequeue().ok_or(DbSqlError::NoSurbAvailable);
+            }
+            Err(_) => {
+                attempts += 1;
+                if attempts >= MAX_RETRIES {
+                    return Err(DbSqlError::LogicalError("failed to lock surbs after multiple attempts".into()));
+                }
+                // Small backoff before retry
+                std::thread::sleep(std::time::Duration::from_millis(5));
+            }
+        }
+    }
+    
+    // Should never reach here due to the return in the loop
+    Err(DbSqlError::LogicalError("unexpected error in pop_one".into()))
 }

218-225: Consider adding tracing to the KeyIdMapper implementation

Adding debug-level tracing to the map_key_to_id and map_id_to_public methods would help diagnose issues in production when key mappings are missing.

 impl hopr_crypto_packet::KeyIdMapper<HoprSphinxSuite, HoprSphinxHeaderSpec> for CacheKeyMapper {
     fn map_key_to_id(&self, key: &OffchainPublicKey) -> Option<KeyIdent> {
-        self.1.get(key).map(|k| *k.value())
+        let result = self.1.get(key).map(|k| *k.value());
+        if result.is_none() {
+            tracing::debug!(%key, "key to id mapping not found");
+        }
+        result
     }

     fn map_id_to_public(&self, id: &KeyIdent) -> Option<OffchainPublicKey> {
-        self.0.get(id).map(|k| *k.value())
+        let result = self.0.get(id).map(|k| *k.value());
+        if result.is_none() {
+            tracing::debug!(%id, "id to key mapping not found");
+        }
+        result
     }
 }

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49a7166 and f6ba99f.

📒 Files selected for processing (2)

crypto/types/src/primitives.rs (1 hunks)
db/sql/src/cache.rs (7 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (8)

GitHub Check: hoprd / docker
GitHub Check: hopli / docker
GitHub Check: Docs / Rust docs
GitHub Check: tests-smoke-websocket
GitHub Check: tests-unit-nightly
GitHub Check: tests-unit
GitHub Check: zizmor
GitHub Check: Linter

🔇 Additional comments (6)

crypto/types/src/primitives.rs (6)

28-29: Security improvement: Secret material is now properly zeroized

The implementation now correctly uses #[derive(zeroize::ZeroizeOnDrop)] to ensure cryptographic key material is securely wiped from memory when the object is dropped. This addresses a potential security vulnerability where sensitive data could remain in memory and be accessible to attackers.

11-17: Streamlined cryptographic primitive exports

Good organization of re-exported cryptographic primitives. The code now uses Blake3 and ChaCha20 as mentioned in the PR objectives, replacing the older implementations for better performance.

26-113: Well-designed generic cryptographic abstraction

The IvKey<T> struct provides a clean, generic abstraction for cryptographic primitives that require both a key and an initialization vector. This design:

Enforces type safety through trait bounds

Reduces code duplication

Properly secures sensitive data

Provides a consistent interface for different cryptographic primitives

The implementation properly separates concerns, uses appropriate inlining for performance-critical methods, and includes thorough documentation.

68-75: Good security practice in Debug implementation

The Debug implementation correctly redacts sensitive key material while still providing useful information about the IV. This prevents accidental exposure of secrets in logs or debug output.

107-112: Valuable conversion utility

The into_init<V> method provides a clean way to convert between compatible cryptographic primitives while preserving the key and IV material. This promotes code reuse and flexibility.

89-91: Verify usage of mutable accessors

The mutable accessors for key and IV materials provide direct access to modify sensitive cryptographic parameters. While these methods may be necessary for some use cases, they could lead to security issues if not used carefully.

Ensure that any code using these mutable accessors properly validates any modifications to maintain cryptographic security.

Also applies to: 101-103

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (4)

crypto/packet/src/packet.rs (4)
102-107: Consider optimizing error message construction

The error message with hex representation is constructed unconditionally, even when the mapping succeeds, which is inefficient.
-                    next_hop: mapper.map_id_to_public(&surb.first_relayer).ok_or_else(|| {
-                        PacketConstructionError(format!(
-                            "failed to map key id {} to public key",
-                            surb.first_relayer.to_hex()
-                        ))
-                    })?,
+                    next_hop: mapper.map_id_to_public(&surb.first_relayer).ok_or_else(|| 
+                        PacketConstructionError(format!(
+                            "failed to map key id {} to public key",
+                            surb.first_relayer.to_hex()
+                        ))
+                    )?,
270-273: Similar error message construction issue in closure

This map operation has similar error message construction inefficiency as seen earlier in the file.
-                mapper
-                    .map_key_to_id(k)
-                    .ok_or_else(|| PacketConstructionError(format!("failed to map key {} to id", k.to_hex())))
+                mapper
+                    .map_key_to_id(k)
+                    .ok_or_else(|| PacketConstructionError(format!("failed to map key {} to id", k.to_hex())))
Note: While this looks the same, I'm highlighting that both locations have similar patterns. You might want to consider extracting this error creation logic if it appears multiple times.

117-145: Consider refactoring NoAck handling to reduce code duplication

The NoAck case shares significant logic with the ForwardPath case but with a fixed destination and no_ack: true. This could be refactored to reduce duplication.

You could consider extracting the common path handling logic between ForwardPath and NoAck into a separate function, passing the relevant parameters like no_ack flag and the path.

336-400: Consider more informative error message for invalid packet size

When a packet has an invalid size, the error message is quite generic. It would be more helpful to include both the expected and actual sizes.
-            Err(PacketDecodingError("packet has invalid size".into()))
+            Err(PacketDecodingError(format!(
+                "packet has invalid size: expected {} bytes, got {} bytes",
+                Self::SIZE,
+                data.len()
+            )))

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f6ba99f and 7968ccf.

📒 Files selected for processing (2)

crypto/packet/src/packet.rs (1 hunks)
crypto/sphinx/src/surb.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

crypto/sphinx/src/surb.rs

⏰ Context from checks skipped due to timeout of 90000ms (7)

GitHub Check: tests-smoke-websocket
GitHub Check: tests-unit
GitHub Check: tests-unit-nightly
GitHub Check: Linter
GitHub Check: hopli / docker
GitHub Check: hoprd / docker
GitHub Check: zizmor

🔇 Additional comments (5)

crypto/packet/src/packet.rs (5)

320-320: Good fix for the arithmetic underflow issue

The saturating_sub is correctly used here to prevent arithmetic underflow when message length exceeds payload size, addressing the issue from a previous review.

237-239: Correct fix for tuple-variant pattern matching

The Display implementation now correctly uses tuple variant matching syntax, fixing the compilation issue noted in a previous review.

404-873: Excellent test coverage for packet functionality

The test module provides comprehensive coverage of various packet scenarios including:

Forward-only packets with varying hop counts

Packets with SURBs

Full round-trip testing with forward and reply messages

SURB-only packets with replies

This thorough testing is excellent for ensuring the new return path functionality works correctly.

598-598: Function signature fixed for parameterized test

The function signature now correctly includes the hops parameter from the parameterized annotation, which fixes the compilation issue mentioned in a previous review.

73-73:
✅ Verification successful

Investigate memory implications of leak() method on ticket

The .leak() method is used at multiple places when building tickets. Depending on its implementation, this could potentially lead to memory leaks in long-running processes if it behaves similar to Box::leak() in standard Rust.

Also applies to: 98-98, 126-126

🏁 Script executed:
#!/bin/bash
# Search for the implementation of `.leak()` method on Ticket or TicketBuilder
rg 'impl.*(Ticket|TicketBuilder).*\{' -A 15 | rg -A 3 'fn\s+leak'
Length of output: 67

I’m not seeing any local leak() implementation yet. Let’s locate its declaration and inspect its usages in context:

🏁 Script executed:
#!/bin/bash
# 1) Find all definitions of leak():
rg -n 'fn leak' -A3 -B0

# 2) Show .leak() calls with surrounding lines in packet.rs:
rg -n '\.leak\(' -A3 -B3 crypto/packet/src/packet.rs
Length of output: 1525
No memory leak—.leak() returns the owned Ticket, it does not allocate permanently
The leak() method is defined in common/internal-types/src/tickets.rs as:
pub fn leak(self) -> Ticket {
    self.0
}
It simply moves out the inner Ticket value rather than calling Box::leak or leaking heap memory. You can safely leave the calls at lines 73, 98, 126, and 442 as is.

NumberFour8 added 30 commits March 6, 2025 19:07

Prepare basic types for RP

aa9f1b8

Add SURB stubs

67c5f9a

Refactor Sphinx routing

b298378

Refactor hopr-crypto-packet

87130ad

Update SURB description

1c1f04c

Integration of SURBs into packet format, part I

c21c4e8

Proper SURB parsing at the packet level

b5ed235

Removed additional reallocations in packet forwarding

e552c38

PRP abstraction

1c3d2c9

WIP on Chacha20 migration

27e9d00

Fix PRG

6f69c22

Refactor PRG

6fc1019

Remove PRP and PRG modules

a238294

Proper use of Poly1305 one-time authenticator in Sphinx header

618e575

Minor refactoring of crypto traits usages

8f65649

Further reduce codebase in hopr-crypto-types, update hoprd-keypair

173d52a

WIP

44f0cc0

Improve routing data representation, add basic SURB test

11258f6

Add MetaPacket test for RP

9d63201

Added working UTs for ChainPacketComponents, increased payload size t…

8a0cdec

…o 799

Code refactoring

d573e56

Serialization wip

23d3d77

Serialization wip

f4c763f

Moved MetaPacket into the Sphinx crate, replaced AdditionalDataLastHo…

6ae3c31

…p with Pseudonym

Code cleanup and docs

e157296

Smaller code cleanup

c91752a

Better check payload length

fc56c0f

Refactoring

f6cf101

Improve padding behavior

5550cad

Added more tests and refactoring

463dd93

coderabbitai bot reviewed Apr 22, 2025

View reviewed changes

coderabbitai bot mentioned this pull request Apr 22, 2025

Create a smart contract assigned Key ID #7080

Open

NumberFour8 added 2 commits April 22, 2025 14:53

Added feedback

033f971

Merge remote-tracking branch 'origin/lukas/rp' into lukas/rp

49a7166

coderabbitai bot reviewed Apr 22, 2025

View reviewed changes

crypto/sphinx/src/surb.rs Outdated Show resolved Hide resolved

crypto/packet/src/packet.rs Outdated Show resolved Hide resolved

Ensure CacheKeyMapper consistency

f6ba99f

coderabbitai bot reviewed Apr 22, 2025

View reviewed changes

Add more feedback

7968ccf

coderabbitai bot reviewed Apr 22, 2025

View reviewed changes

Fix chain_integration_tests.rs

3017021

Teebor-Choka approved these changes Apr 22, 2025

View reviewed changes

Update values in test_session.py

70c08a2

NumberFour8 merged commit f38eac3 into master Apr 23, 2025
28 of 31 checks passed

NumberFour8 deleted the lukas/rp branch April 23, 2025 09:03

tolbrino mentioned this pull request Apr 29, 2025

The common/internal-types package deserves optimizations #6452

Closed

NumberFour8 mentioned this pull request May 11, 2025

Remove database connection opening per-packet #6922

Closed

coderabbitai bot mentioned this pull request May 12, 2025

Introduce Return path and SURB balancing in Sessions #7088

Merged

coderabbitai bot mentioned this pull request Jun 3, 2025

refactor(hopr-lib): Reduce required on-chain logs in the indexer (revisited) #7213

Merged

This was referenced Jul 1, 2025

fix(ci): comply with pre-commit #7281

Merged

fix(hopr-lib): Connectability issues in the local cluster and production #7255

Merged

This was referenced Jul 15, 2025

Close release 3.0.0-rc.1 #7312

Merged

feat(hopr-lib): add corrupted state to channels #7296

Merged

This was referenced Jul 23, 2025

feat(chain): Add logs snapshot fetching #7341

Merged

feat: add number of surbs into the dissector #7357

Merged

coderabbitai bot mentioned this pull request Jul 31, 2025

fix: allow multi-layered reply openers #7363

Merged

This was referenced Aug 13, 2025

refactor: various optimizations to sessions & transport #7368

Merged

refactor: move Application protocol into its own crate, fix tests #7401

Merged

This was referenced Aug 26, 2025

feat: updated Lioness wide-block cipher implementation #7419

Merged

refactor: optimize acknowlegement processing and point decompression #7428

Merged

fix: use faster ECDSA/ECDH library for secp256k1 #7432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Introduce Return Path to the protocol #6932

Introduce Return Path to the protocol #6932

Uh oh!

NumberFour8 commented Mar 18, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Introduce Return Path to the protocol #6932

Introduce Return Path to the protocol #6932

Uh oh!

Conversation

NumberFour8 commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New features

Increase the maximum usable packet payload size

Key IDs

SURB & Return path support

No-ACK packets

Fixes & improvements

PRP and PRG algorithms have been replaced

OffchainPublicKey contains more precomputes

serde usage as a crate feature

Other

Further work

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NumberFour8 commented Mar 18, 2025 •

edited

Loading

`OffchainPublicKey` contains more precomputes

`serde` usage as a crate feature