Unify the msg and ack protocol inside a single payload structure
#7073
Conversation
📝 Walkthrough## Walkthrough
This change set removes the ticket aggregation protocol and its related infrastructure from the transport layer, P2P network behavior, and protocol configuration. It introduces two new workspace crates: `hopr-transport-packet` and `hopr-transport-ticket-aggregation`, the latter being a placeholder for future reimplementation. The packet processing flow is refactored to unify message and acknowledgment handling, removing separate acknowledgment protocols and codecs. Packet and protocol abstractions are simplified, with explicit replay detection handled via a Bloom filter. Several public APIs, trait signatures, and error types are updated to reflect these changes. Integration and unit tests are updated or skipped to accommodate the absence of session-based ticket aggregation.
## Changes
| File(s) / Path(s) | Change Summary |
|--------------------------------------------------------------------------------------------------------|---------------|
| Cargo.toml, logic/strategy/Cargo.toml, transport/api/Cargo.toml, transport/protocol/Cargo.toml | Added new workspace members and dependencies: `hopr-transport-packet` and `hopr-transport-ticket-aggregation`. Updated feature groups and dev-dependencies accordingly. |
| transport/packet/.gitignore, transport/packet/Cargo.toml, transport/packet/src/lib.rs | Added new crate `hopr-transport-packet` with packet primitives, fixed-size payload enforcement, and basic packet structures. |
| transport/ticket-aggregation/.gitignore, transport/ticket-aggregation/Cargo.toml, transport/ticket-aggregation/src/lib.rs | Added new crate `hopr-transport-ticket-aggregation` as a placeholder. Introduced new error type `TicketAggregationError`. |
| hoprd/hoprd/example_cfg.yaml, transport/protocol/src/config.rs | Removed ticket aggregation protocol configuration from protocol config and example config. |
| transport/protocol/src/ticket_aggregation/config.rs, transport/protocol/src/ticket_aggregation/mod.rs | Deleted ticket aggregation protocol config and module re-exports. |
| transport/protocol/src/ack/codec.rs, transport/protocol/src/ack/mod.rs, transport/protocol/src/ack/processor.rs | Deleted acknowledgment protocol codec, module, and processor. |
| transport/protocol/src/msg/codec.rs, transport/protocol/src/msg/mod.rs, transport/protocol/src/msg/packet.rs | Deleted message protocol codec, module, and packet abstractions. |
| transport/protocol/src/processor.rs, transport/protocol/src/codec.rs | Refactored packet processor: simplified interfaces, removed explicit replay detection, introduced `FixedLengthCodec`. |
| transport/protocol/src/lib.rs, transport/protocol/benches/protocol_throughput_emulated.rs, transport/protocol/tests/common/mod.rs | Refactored protocol entrypoint: unified message and acknowledgment handling, removed explicit ack protocol, updated test and bench setup accordingly. |
| transport/api/src/errors.rs, transport/api/src/lib.rs, transport/api/src/proxy.rs, transport/api/src/helpers.rs | Updated error handling for ticket aggregation, refactored proxy and aggregator traits to return new error types, removed session-based ticket aggregation logic. |
| db/api/src/protocol.rs, db/sql/src/protocol.rs | Refactored packet and acknowledgment handling: removed `AckResult`, unified packet representations, updated trait signatures, improved metrics. |
| transport/p2p/src/behavior/mod.rs, transport/p2p/src/behavior/ticket_aggregation.rs, transport/p2p/src/constants.rs, transport/p2p/src/lib.rs, transport/p2p/src/swarm.rs | Removed ticket aggregation behavior, protocol constants, and all related event handling from the P2P network stack. |
| transport/protocol/src/bloom.rs | Added async `is_tag_replay` method to Bloom filter wrapper for replay detection. |
| crypto/packet/src/packet.rs | Changed `ack_key` in `HoprIncomingPacket` to `Option<HalfKey>`, removed `no_ack` field, updated logic and tests accordingly. |
| tests/test_integration.py, tests/test_redeeming.py, tests/test_win_prob.py | Skipped tests related to ticket aggregation, as it is not implemented as a session protocol. |
| logic/strategy/src/aggregating.rs, logic/strategy/src/strategy.rs | Updated import paths for ticket aggregation trait/types to new crate. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant App as Application
participant Protocol as Protocol Layer
participant DB as Database
participant P2P as P2P Network
App->>Protocol: Send application data
Protocol->>DB: Packet wrapping (send)
DB-->>Protocol: OutgoingPacket
Protocol->>P2P: Send packet (wire)
P2P-->>Protocol: Receive packet (wire)
Protocol->>DB: Packet unwrapping (recv)
DB-->>Protocol: Option<IncomingPacket>
alt Final packet
Protocol->>P2P: Send acknowledgment (wire)
end
Protocol-->>App: Deliver application data (if final)Possibly related PRs
Suggested labels
Suggested reviewers
|
413853f to
d59ecf0
Compare
d59ecf0 to
9dd3813
Compare
0db9447 to
075480b
Compare
32274ca to
6296371
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
transport/p2p/src/lib.rs (1)
154-156:⚠️ Potential issueLeft-over
TicketAggregationevent is now unreachable and will rot.The ticket-aggregation behaviour was removed from
HoprNetworkBehavior, yet the event variant and theFrom<…>conversion are still present.
With no sub-behaviour producing this event, the code is:
- Dead / unreachable (may trigger dead-code warnings).
- Misleading to future maintainers (suggests the feature is still available).
- A potential compile-error magnet if the referenced types are ever deleted or renamed.
- TicketAggregation( - libp2p::request_response::Event<Vec<TransferableWinningTicket>, std::result::Result<Ticket, String>>, - ), + // (ticket aggregation removed – variant intentionally deleted)and drop the matching
impl From … { … }.If the plan is to re-add ticket aggregation later, keep the git history instead of zombie code.
Also applies to: 191-199
transport/protocol/src/processor.rs (1)
150-155:⚠️ Potential issueMissing definition of
DEFAULT_OUTGOING_TICKET_WIN_PROB.
determine_actual_outgoing_win_probreferencesDEFAULT_OUTGOING_TICKET_WIN_PROB, but the constant is not declared in this module nor imported. This will not compile.Either import the constant from its new home:
use hopr_internal_types::constants::DEFAULT_OUTGOING_TICKET_WIN_PROB;or re-define it locally (same pattern as
DEFAULT_PRICE_PER_PACKET).transport/p2p/src/swarm.rs (1)
75-87:⚠️ Potential issueBuilder fails to compile due to superfluous
map_erron a non-Result value
swarmis aSwarmBuilder, not aResult.
The extra.map_err(..)?therefore makes the whole function fail to compile.- Ok(swarm - .map_err(|e| crate::errors::P2PError::Libp2p(e.to_string()))? - .with_behaviour(|_key| { + Ok(swarm + .with_behaviour(|_key| { HoprNetworkBehavior::new( me_peerid, network_update_input, indexer_update_input, heartbeat_requests, protocol_cfg.heartbeat.timeout, ) })(The later
.map_err(..)?afterwith_behaviouris still needed because that method does return aResult).
Without this fix the crate will not build.
🧹 Nitpick comments (19)
transport/protocol/src/bloom.rs (1)
4-4: Missingtracing::traceimportThe code uses the
tracelevel in thetracing::instrumentattribute, but thetraceimport is missing from the tracing imports.-use tracing::{debug, error, info}; +use tracing::{debug, error, info, trace};crypto/packet/src/packet.rs (1)
385-390: Simplified acknowledgment handling logic.The code now explicitly calculates
should_acknowledgefrom!no_ackand then uses Rust'sthen_some()method to conditionally set theack_keyfield. This improves readability and follows Rust idioms.Consider making the intent even clearer by using this more direct approach:
- let should_acknowledge = !no_ack; - Ok(Self::Final( - HoprIncomingPacket { - packet_tag, - ack_key: (should_acknowledge).then_some(derive_ack_key_share(&derived_secret)), + Ok(Self::Final( + HoprIncomingPacket { + packet_tag, + ack_key: (!no_ack).then_some(derive_ack_key_share(&derived_secret)),transport/protocol/src/codec.rs (1)
3-38: Clean, generic fixed-length codec implementation.The
FixedLengthCodecimplementation is well-designed with the generic const parameter for size, making it reusable for different packet sizes. The tracing is appropriate for debugging.However, the error message "Payload too small" on line 29 could be misleading when the payload is actually too large.
- "Payload too small", + format!("Payload size mismatch: expected {}, got {}", HoprPacket::PAYLOAD_SIZE, data.len()),transport/api/src/proxy.rs (1)
40-62: Clear communication of unimplemented functionality.The implementation now properly returns an error with a clear message that ticket aggregation is not yet supported. This is better than silently failing or providing partial functionality.
Consider removing the commented-out code in a future cleanup PR, as it adds noise to the current implementation. If it's meant as a reference for future implementation, consider moving it to documentation or a design document.
transport/packet/src/lib.rs (1)
94-95: Address TODO comment.There's a TODO comment about possibly removing the SIZE constant. Since this PR introduces significant changes to the packet structure, it would be better to make a decision about this constant now rather than leaving it as a TODO.
Make a decision about whether to keep or remove this constant based on its current usage in the codebase.
transport/ticket-aggregation/Cargo.toml (1)
1-17: Mark placeholder crate as non-publishable.Because this crate is “inert” and contains no functional code yet, publishing it to crates.io could confuse external users.
[package] name = "hopr-transport-ticket-aggregation" version = "0.1.0" +# Prevent accidental publishing of an empty crate +publish = falseAdding
publish = false(and optionally a short README explaining the placeholder status) makes the intent explicit.transport/protocol/src/processor.rs (1)
6-8: Duplicate import & shadowing ofPacketError.
PacketErroris imported twice: once as the type and once inside the grouped import that also pulls in theTransportErrorvariant. The first import becomes unused and will triggerdead_code/unused_importwarnings.-use hopr_crypto_packet::errors::PacketError; -use hopr_crypto_packet::errors::{PacketError::TransportError, Result}; +use hopr_crypto_packet::errors::{PacketError, PacketError::TransportError, Result};This keeps a single, coherent import line.
transport/p2p/src/swarm.rs (1)
408-430: Dead code: oldInputsenum and ticket-aggregation aliases can be deletedThese definitions are no longer referenced after the ACK/ticket-aggregation removal. Keeping them:
- increases compile time (dead-code lint),
- confuses readers about the current design,
- risks accidental re-use of soon-to-be-removed types.
-/// Composition of all inputs allowing to produce a single stream of -/// input events passed into the swarm processing logic. -#[derive(Debug)] -pub enum Inputs { - Message((PeerId, Box<[u8]>)), - Acknowledgement((PeerId, Acknowledgement)), -} - -impl From<(PeerId, Acknowledgement)> for Inputs { … } -impl From<(PeerId, Box<[u8]>)> for Inputs { … } - -pub type TicketAggregationRequestType = OutboundRequestId; -pub type TicketAggregationResponseType = ResponseChannel<std::result::Result<Ticket, String>>;If you still need the type aliases elsewhere, move them to a dedicated (and clearly named) module instead.
transport/api/src/lib.rs (2)
607-613: Hard-coded 15 s aggregation timeout makes the value un-tuneable
ticket_aggregator()bakes the timeout into the code:std::time::Duration::from_secs(15)Please expose this through
HoprTransportConfig(e.g.cfg.protocol.ticket_agg_timeout_secs) so operators can adjust it for different network conditions or unit tests.- std::time::Duration::from_secs(15), + std::time::Duration::from_secs(self.cfg.protocol.ticket_agg_timeout_secs),
737-749: Stray commented code & unconditional error — clarify intentThe block is now:
// old implementation … /// … Err(TicketAggregationError::TransportError( "Ticket aggregation not supported as a session protocol yet".to_string(), ).into())
- The large commented section clutters the file – please remove it or open an issue that references the historic code.
- If ticket aggregation is temporarily disabled, gate the call behind a feature flag or TODO so downstream callers can feature-detect instead of always failing at runtime.
transport/protocol/tests/common/mod.rs (1)
230-268: Potential indefinite await in relay emulationInside
emulate_channel_communicationeach relayer awaits:components[i].1.next().awaitIf fewer than
countpackets arrive (e.g. due to earlier test failure) the test hangs forever.
Add a timeout to fail fast and surface the real issue:-let (dest, payload) = components[i].1.next().await +let (dest, payload) = components[i] + .1 + .next() + .timeout(std::time::Duration::from_secs(5)) + .await + .expect("channel stalled")This prevents CI timeouts and gives clearer feedback.
transport/protocol/src/lib.rs (3)
7-11: Removeackfrom the public-facing docsThe introductory comment still advertises an
ackprotocol, yet acknowledgements are now embedded in the unified “mix” flow. Leaving this line in misleads integrators and future maintainers.-//! - `ack`
132-139: Rename the API — it no longer runs an “ack” protocol
run_msg_ack_protocolsuggests two distinct protocols are spun up. After the merge, only the mix payload is transported. Renaming avoids confusion and aligns with the de-duped design.-pub async fn run_msg_ack_protocol<Db>( +pub async fn run_mix_protocol<Db>((remember to update call-sites & exports).
160-166: Dead metric initialisation code can be removedThe commented-out
METRIC_*ACKSinitialisers were left behind. Keeping them increases noise and hints at half-finished work.- // lazy_static::initialize(&METRIC_RECEIVED_ACKS); - // lazy_static::initialize(&METRIC_SENT_ACKS); - // lazy_static::initialize(&METRIC_TICKETS_COUNT);db/api/src/protocol.rs (2)
50-68: Consider marking the large enums/structs as#[non_exhaustive]
IncomingPacketandOutgoingPacketare public API guarantees. Given the protocol is still evolving (e.g. ticket aggregation resurrection), adding a#[non_exhaustive]attribute future-proofs against breaking changes.
69-74:ack_challengeshould be optional for “no-ack” packets
to_send_no_ackalways fabricates anOutgoingPacketcarrying anack_challenge, even though the transport layer will never await it. Making the fieldOption<HalfKeyChallenge>clarifies intent and avoids misleading downstream metrics that count “sent acks”.db/sql/src/protocol.rs (3)
252-254: Metric label loses informationEvery valid acknowledgement increments
METRIC_RECEIVED_ACKSwith the label"true".
Distinguishing"win" | "loss" | "sender"(or similar) would give operators better insight without extra counters.
565-566: Typo in log field name
erorrwill appear in logs and makes searching harder.-warn!(?rejected_ticket, %rejected_value, erorr = ?error, "failure to validate during forwarding"); +warn!(?rejected_ticket, %rejected_value, error = ?error, "failure to validate during forwarding");
273-279: Underscore prefix hides a used bindingThe binding
_channelis later referenced, defeating the intent of a leading underscore.
Usingchannel_id(without underscore) silences the “unused variable” lint cleanly.-ResolvedAcknowledgement::RelayingLoss(_channel) => { +ResolvedAcknowledgement::RelayingLoss(channel_id) => { ... - crate::tickets::METRIC_HOPR_TICKETS_INCOMING_STATISTICS - .increment(&[&_channel.to_string(), "losing_count"], 1.0f64); + crate::tickets::METRIC_HOPR_TICKETS_INCOMING_STATISTICS + .increment(&[&channel_id.to_string(), "losing_count"], 1.0f64);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
Cargo.toml(2 hunks)crypto/packet/src/packet.rs(8 hunks)db/api/src/protocol.rs(5 hunks)db/sql/src/protocol.rs(13 hunks)hoprd/hoprd/example_cfg.yaml(0 hunks)logic/strategy/Cargo.toml(2 hunks)logic/strategy/src/aggregating.rs(2 hunks)logic/strategy/src/strategy.rs(1 hunks)tests/test_integration.py(2 hunks)tests/test_redeeming.py(1 hunks)tests/test_win_prob.py(1 hunks)transport/api/Cargo.toml(5 hunks)transport/api/src/errors.rs(1 hunks)transport/api/src/helpers.rs(1 hunks)transport/api/src/lib.rs(5 hunks)transport/api/src/proxy.rs(2 hunks)transport/p2p/src/behavior/mod.rs(0 hunks)transport/p2p/src/behavior/ticket_aggregation.rs(0 hunks)transport/p2p/src/constants.rs(0 hunks)transport/p2p/src/lib.rs(2 hunks)transport/p2p/src/swarm.rs(6 hunks)transport/p2p/tests/p2p_transport_test.rs(3 hunks)transport/packet/.gitignore(1 hunks)transport/packet/Cargo.toml(1 hunks)transport/packet/src/lib.rs(1 hunks)transport/protocol/Cargo.toml(1 hunks)transport/protocol/benches/protocol_throughput_emulated.rs(2 hunks)transport/protocol/src/ack/codec.rs(0 hunks)transport/protocol/src/ack/mod.rs(0 hunks)transport/protocol/src/ack/processor.rs(0 hunks)transport/protocol/src/bloom.rs(2 hunks)transport/protocol/src/codec.rs(1 hunks)transport/protocol/src/config.rs(0 hunks)transport/protocol/src/lib.rs(9 hunks)transport/protocol/src/msg/codec.rs(0 hunks)transport/protocol/src/msg/mod.rs(0 hunks)transport/protocol/src/msg/packet.rs(0 hunks)transport/protocol/src/processor.rs(9 hunks)transport/protocol/src/ticket_aggregation/config.rs(0 hunks)transport/protocol/src/ticket_aggregation/mod.rs(0 hunks)transport/protocol/tests/common/mod.rs(4 hunks)transport/ticket-aggregation/.gitignore(1 hunks)transport/ticket-aggregation/Cargo.toml(1 hunks)transport/ticket-aggregation/src/lib.rs(4 hunks)
💤 Files with no reviewable changes (13)
- transport/protocol/src/config.rs
- hoprd/hoprd/example_cfg.yaml
- transport/p2p/src/constants.rs
- transport/p2p/src/behavior/mod.rs
- transport/protocol/src/ticket_aggregation/config.rs
- transport/protocol/src/ack/mod.rs
- transport/protocol/src/ack/processor.rs
- transport/protocol/src/msg/codec.rs
- transport/protocol/src/msg/mod.rs
- transport/protocol/src/ticket_aggregation/mod.rs
- transport/protocol/src/msg/packet.rs
- transport/protocol/src/ack/codec.rs
- transport/p2p/src/behavior/ticket_aggregation.rs
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6765
File: transport/api/src/lib.rs:602-605
Timestamp: 2025-01-15T14:52:38.363Z
Learning: In the HOPR transport layer, the `TicketAggregatorProxy` is intentionally kept separate from the main transport interface to allow for potential extraction to upper interfaces in the future. This design decision facilitates architectural flexibility and should be maintained.
transport/protocol/Cargo.toml (1)
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.
logic/strategy/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/api/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/api/src/proxy.rs (1)
Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6765
File: transport/api/src/lib.rs:602-605
Timestamp: 2025-01-15T14:52:38.363Z
Learning: In the HOPR transport layer, the `TicketAggregatorProxy` is intentionally kept separate from the main transport interface to allow for potential extraction to upper interfaces in the future. This design decision facilitates architectural flexibility and should be maintained.
transport/api/src/lib.rs (1)
Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6765
File: transport/api/src/lib.rs:602-605
Timestamp: 2025-01-15T14:52:38.363Z
Learning: In the HOPR transport layer, the `TicketAggregatorProxy` is intentionally kept separate from the main transport interface to allow for potential extraction to upper interfaces in the future. This design decision facilitates architectural flexibility and should be maintained.
🧬 Code Graph Analysis (4)
crypto/packet/src/packet.rs (1)
crypto/packet/src/por.rs (1)
derive_ack_key_share(21-23)
db/sql/src/protocol.rs (4)
common/internal-types/src/protocol.rs (1)
new(68-75)hopr/hopr-lib/src/lib.rs (2)
new(331-333)new(371-522)db/api/src/protocol.rs (2)
handle_acknowledgement(18-18)to_send_no_ack(27-27)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)
db/api/src/protocol.rs (1)
db/sql/src/protocol.rs (4)
handle_acknowledgement(243-288)get_network_winning_probability(290-295)get_network_ticket_price(297-302)to_send_no_ack(305-355)
transport/protocol/src/lib.rs (4)
transport/protocol/src/codec.rs (3)
FixedLengthCodec(49-49)FixedLengthCodec(68-68)FixedLengthCodec(89-89)transport/protocol/src/bloom.rs (1)
new(20-37)transport/protocol/src/processor.rs (2)
new(122-124)new(224-226)common/internal-types/src/protocol.rs (2)
random(78-80)from_bytes(297-310)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: Docs / Rust docs
- GitHub Check: zizmor
- GitHub Check: tests-smoke-websocket
- GitHub Check: tests-smart-contracts
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-unit
- GitHub Check: Linter
🔇 Additional comments (47)
transport/api/src/helpers.rs (1)
17-17: Update import path to unified processor module
The import ofMsgSenderandSendMsgInputhas been updated to reflect the new unifiedprocessormodule inhopr_transport_protocol. This aligns with the refactoring that merges themsgandackprotocols. No further changes to usage or behavior are required here.logic/strategy/src/strategy.rs (1)
31-31: SwitchTicketAggregatorTraitto the new aggregation crate
The import path forTicketAggregatorTraitnow points tohopr_transport_ticket_aggregation. This correctly modularizes ticket aggregation into its own workspace crate. All trait usages inMultiStrategy::newwill now reference the extracted crate.logic/strategy/src/aggregating.rs (2)
41-41: ImportTicketAggregatorTraitfrom the dedicated aggregation crate
The aggregating strategy now depends onhopr_transport_ticket_aggregation::TicketAggregatorTraitinstead of the old protocol crate. This keeps the logic intact while honoring the new crate boundary.
314-316: Update test imports to use new aggregation crate
Tests now referenceAwaitingAggregator,TicketAggregationInteraction, andTicketAggregationProcessedfromhopr_transport_ticket_aggregation, matching the refactored module layout. Test functionality is preserved.transport/protocol/Cargo.toml (1)
52-52: Add workspace dependency for the packet crate
Introducinghopr-transport-packet = { workspace = true }brings the new transport-packet workspace member into this crate. Dependency versions are managed at the workspace level, so no version specifier is needed here.transport/packet/.gitignore (1)
1-6: Ignore generated and backup artifacts for the new packet crate
This.gitignoreprevents committing build artifacts (/target), editor backups (*.rs.bk), lock files, and temporary logs withintransport/packet. It’s a standard setup for a new workspace member.transport/ticket-aggregation/.gitignore (1)
1-6: LGTM - Standard Rust project gitignore patterns.The gitignore file includes common patterns for Rust projects, properly excluding build artifacts, backup files, package files, and log files.
transport/api/src/errors.rs (1)
39-41: Appropriate error integration for the new ticket aggregation crate.This addition correctly follows the established error handling pattern in the project, using the
#[from]attribute for automatic error conversion and providing a clear error message format.logic/strategy/Cargo.toml (4)
20-20: LGTM - Consistent feature flag integration.The ticket-aggregation dependency is properly added to the runtime-async-std feature group.
26-27: LGTM - Consistent feature flag integration.The ticket-aggregation dependency is properly added to the runtime-tokio feature group.
35-35: LGTM - Consistent feature flag integration.The ticket-aggregation dependency is properly added to the prometheus feature group.
64-64: LGTM - Proper workspace dependency addition.The hopr-transport-ticket-aggregation is correctly added as a workspace dependency, following the project's dependency management pattern.
tests/test_win_prob.py (1)
64-64: Appropriate test skipping during refactoring.The test is correctly marked to be skipped with a clear reason: "ticket aggregation is not implemented as a session protocol yet." This aligns with the PR's objective of unifying the message and acknowledgment protocols.
transport/packet/Cargo.toml (1)
1-22: Looks good - appropriate package configuration for new moduleThe new Cargo.toml file is well-structured with proper metadata, dependencies, and configuration for the new
hopr-transport-packetcrate. This addition aligns with the PR's goal of unifying message and acknowledgment protocols.tests/test_redeeming.py (1)
121-123: Appropriate test skip with clear reasonSkipping this test is consistent with the PR objective of unifying message and acknowledgment protocols. The skip reason clearly documents that ticket aggregation functionality is not currently implemented as a session protocol.
transport/protocol/src/bloom.rs (1)
39-46: Well-documented replay detection methodThe new
is_tag_replaymethod is a good addition that properly encapsulates the replay detection logic. The documentation clearly communicates the false positive rate limitation of using a Bloom filter.transport/protocol/benches/protocol_throughput_emulated.rs (4)
13-13: Updated import path aligns with protocol unificationThe import path change reflects the consolidation of message and acknowledgment processing into a unified processor module, which is consistent with the PR objectives.
63-68: PacketInteractionConfig simplified by removing chain_keypairThe configuration no longer includes the
chain_keypairfield, which aligns with the refactoring of the packet interaction configuration to support the unified protocol structure.
69-76: Protocol initialization simplifiedThe call to
run_msg_ack_protocolhas been simplified by removing the acknowledgement channels tuple argument, which is consistent with the unification of message and acknowledgment handling.
80-80: Updated path construction for mock path resolutionThe code now correctly passes clones of public keys directly, rather than using
.into()conversions, which is consistent with updated key handling elsewhere in the codebase.transport/api/Cargo.toml (5)
22-22: Added new dependency to runtime-async-std feature.Adding
hopr-transport-ticket-aggregationto the runtime feature sets is consistent with how other transport components are organized in this project.
32-32: Added new dependency to runtime-tokio feature.The parallel addition to tokio runtime features maintains consistency with the async-std version.
45-45: Added new dependency to prometheus feature.Adding the prometheus feature for the new ticket-aggregation module keeps instrumentation consistent across all transport components.
86-86: Added new workspace dependency.The direct dependency on
hopr-transport-ticket-aggregationis properly declared as a workspace dependency.
95-97: Added dev dependency with appropriate features.The dev dependency includes runtime-async-std for testing, which aligns with the project's testing approach.
Cargo.toml (4)
42-43: Added new workspace member for transport packet module.Adding
transport/packetas a workspace member is appropriate for the unification of message and acknowledgment protocols.
46-47: Added placeholder workspace member for ticket aggregation.Adding
transport/ticket-aggregationas a workspace member sets up the structure for future implementation.
242-243: Added workspace dependency for transport packet.The workspace dependency declaration for
hopr-transport-packetis correctly set up.
245-246: Added placeholder dependency with clear documentation.The workspace dependency for ticket-aggregation includes a helpful comment explaining its current status as a "dead package waiting for reimplementation as a session protocol," which provides valuable context for developers.
crypto/packet/src/packet.rs (2)
174-177: Improved design by making ack_key optional instead of using a separate flag.Replacing the
no_ack: boolfield with an optionalack_key: Option<HalfKey>is a cleaner approach. This change centralizes the acknowledgment presence logic into a single field, making the code more maintainable and reducing potential inconsistencies.
618-619: Updated test assertions for ack_key presence.All test assertions have been correctly updated to check for the presence of
ack_keyinstead of the absence of theno_ackflag, maintaining consistency with the implementation changes.Also applies to: 656-657, 796-797, 842-843
tests/test_integration.py (2)
251-252: Skipped test for ticket aggregation functionality.The test
test_hoprd_should_fail_sending_a_message_when_the_channel_is_out_of_fundingis correctly marked to be skipped as ticket aggregation is not implemented as a session protocol yet. The skip reason is clear and appropriate.
348-349: Skipped test for automatic ticket aggregation and redeeming.The test
test_hoprd_default_strategy_automatic_ticket_aggregation_and_redeemingis correctly marked to be skipped for the same reason. This is consistent with the PR's objective of deferring ticket aggregation functionality.transport/ticket-aggregation/src/lib.rs (4)
23-39: Well-structured error handling approach.The new
TicketAggregationErrorenum withthiserroris a good choice for domain-specific error handling, providing clear error messages and proper error categorization. The separation from the previous protocol errors aligns well with separating ticket aggregation into its own crate.
167-170: Appropriate error mapping in consume_and_wait.The transition from previous error types to the new
TicketAggregationErroris handled correctly here, maintaining the same error semantics while using the new type system.
253-260: Good error handling in process method.The error mapping in the
processmethod correctly uses the new error types while preserving the original error conditions and messages.
356-356: Minor variable name improvement.Changing from
_to_destimproves code readability by providing context for what this variable represents, while still indicating it's unused with the leading underscore.transport/protocol/src/codec.rs (1)
40-106: Comprehensive test coverage.The tests thoroughly cover the key codec behaviors:
- Happy path (encode/decode roundtrip)
- Handling insufficient bytes
- Handling excess bytes
This ensures the codec will work reliably in all expected scenarios.
transport/api/src/proxy.rs (2)
6-6: Appropriate import update.Correctly updated the import to use the new dedicated ticket aggregation crate, following the architectural change to separate these concerns.
13-18: Reasonable use of #[allow(dead_code)].Adding these attributes is appropriate since these fields are temporarily unused while ticket aggregation is disabled as a session protocol. This preserves the structure for future implementation while avoiding compiler warnings.
transport/packet/src/lib.rs (4)
10-47: Robust DataBytes implementation with proper validation.The
DataBytesstruct correctly enforces the fixed payload size requirement through itsTryFromimplementations. This ensures that invalid data is caught early with appropriate error messages.
49-96: Well-designed Payload enum with appropriate trait implementations.The
Payloadenum properly represents both message and acknowledgment payloads. The trait implementations (Debug, AsRef, From/TryFrom) are comprehensive and make the type easy to use throughout the codebase.
98-105: Simple and appropriate packet structure.The
HeaderandPacketstructures are straightforward and provide a clear separation of concerns between routing information (header) and payload data.
107-129: Good test coverage for DataBytes validation.The tests properly verify the three key scenarios for DataBytes:
- Rejecting too short payloads
- Rejecting too long payloads
- Accepting correctly sized payloads
This ensures the fundamental size validation works correctly.
transport/p2p/tests/p2p_transport_test.rs (1)
72-74: Verify protocol constant & codec after unification.The test still uses the pre-refactor constant
CURRENT_HOPR_MSG_PROTOCOL.
After merging msg and ack into a single packet protocol, the constant may have been renamed or removed. A stale constant will fail to compile once CI turns strict on unused items.let msg_proto_control = swarm.build_protocol_control(hopr_transport_protocol::CURRENT_HOPR_MSG_PROTOCOL); let msg_codec = hopr_transport_protocol::HoprBinaryCodec {};Please confirm the constant exists and represents the new unified protocol, or switch to the updated name (e.g.
CURRENT_HOPR_PACKET_PROTOCOL).transport/protocol/tests/common/mod.rs (1)
143-146: Channel alias now omits ACK stream – check remaining call-sites
WireChannelswas reduced to two elements, but several helper functions (e.g.peer_setup_for) still construct/expect ACK-related logic. Ensure all usages were updated; otherwise tests may silently discard ACK traffic.transport/protocol/src/lib.rs (1)
286-295: Bloom-filter look-up may race without recording fresh tags
is_tag_replayreturns a boolean but it’s not obvious whether it ALSO inserts unseen tags.
If insertion is not atomic inside that method, the filter must be updated here to prevent re-processing on concurrent tasks.Please confirm (or amend) that
is_tag_replay()stores the tag when it returnsfalse.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
transport/protocol/src/lib.rs (1)
255-258:⚠️ Potential issueFix potential panic in peer conversion
The code uses
unwrap()when converting aPeerIdtoOffchainPublicKey, which could cause the application to crash if the conversion fails.Replace the unwrap with proper error handling:
-let peer: OffchainPublicKey = peer.try_into().unwrap(); // TODO: should not unwrap !!! +let peer: OffchainPublicKey = match peer.try_into() { + Ok(p) => p, + Err(e) => { + tracing::warn!(%peer, %e, "Dropping packet – cannot convert peer id"); + return None; + } +};
🧹 Nitpick comments (2)
transport/protocol/src/lib.rs (2)
7-10: Update documentation to reflect protocol unificationThe documentation lists protocols that appear inconsistent with the unification changes. Line 7 has been updated to only include "mix" protocol, but lines 8-10 still list "ack", "heartbeat", and "ticket_aggregation" as separate protocols.
Update the documentation to accurately reflect the current protocol structure after unification:
-//! - `mix` -//! - `ack` -//! - `heartbeat` -//! - `ticket_aggregation` +//! - `mix` (unified protocol handling both messages and acknowledgments) +//! - `heartbeat`
316-319: Optimize acknowledgment creationThere are multiple instances of creating acknowledgments with unnecessary copying.
Consider optimizing the acknowledgment data copying:
-db.to_send_no_ack(Box::from_iter(ack.as_ref().iter().copied()), previous_hop) // TODO: Optimize this copy +db.to_send_no_ack(Box::from(ack.as_ref()), previous_hop)This creates the Box directly from the acknowledgment reference, avoiding the need to iterate and copy each byte individually.
Also applies to: 352-354
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
transport/protocol/src/lib.rs(9 hunks)
🔇 Additional comments (8)
transport/protocol/src/lib.rs (8)
47-48: LGTM - New unified protocol structure looks goodThe refactored code introduces a new processor module and simplified codec structure, which aligns well with the PR objective of unifying the message and acknowledgment protocols.
Also applies to: 59-60, 81-87
163-165: LGTM - Appropriate metric adjustmentsCommenting out the acknowledgment and ticket metrics is consistent with the protocol unification approach.
192-193: LGTM - Simplified packet processor initializationThe packet processor initialization no longer requires a bloom filter parameter, which aligns with the unified approach to packet processing.
258-273: LGTM - Sending fallback acknowledgment on packet processing failureThe implementation sends a random signed acknowledgment back to the sender when packet processing fails, which provides useful feedback.
280-299: LGTM - Effective replay detection with bloom filterThe code properly handles replay detection using the bloom filter, incrementing metrics and dropping replayed packets.
309-334: LGTM - Clear handling of final packetsThe code for processing final packets correctly extracts the plaintext and sends an acknowledgment back to the previous hop.
335-368: LGTM - Efficient packet forwarding logicThe forwarding logic handles both packet forwarding and acknowledgment sending appropriately.
371-377: LGTM - Proper application data decodingThe code correctly filters and decodes application data, with appropriate error logging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
transport/protocol/src/lib.rs (1)
163-165: Commented out metric initializationsThese metrics initializations are commented out but not removed. Consider either removing them completely if they're no longer needed or properly reinitializing them if they'll be used in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crypto/packet/Cargo.toml(1 hunks)db/api/Cargo.toml(1 hunks)db/sql/Cargo.toml(1 hunks)db/sql/src/protocol.rs(13 hunks)logic/strategy/Cargo.toml(3 hunks)transport/api/Cargo.toml(6 hunks)transport/p2p/Cargo.toml(1 hunks)transport/protocol/Cargo.toml(2 hunks)transport/protocol/src/lib.rs(9 hunks)
✅ Files skipped from review due to trivial changes (4)
- db/sql/Cargo.toml
- crypto/packet/Cargo.toml
- transport/p2p/Cargo.toml
- db/api/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- transport/protocol/Cargo.toml
- logic/strategy/Cargo.toml
- transport/api/Cargo.toml
- db/sql/src/protocol.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6765
File: transport/api/src/lib.rs:602-605
Timestamp: 2025-01-15T14:52:38.363Z
Learning: In the HOPR transport layer, the `TicketAggregatorProxy` is intentionally kept separate from the main transport interface to allow for potential extraction to upper interfaces in the future. This design decision facilitates architectural flexibility and should be maintained.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: Docs / Rust docs
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-unit
- GitHub Check: tests-smoke-websocket
- GitHub Check: Linter
- GitHub Check: tests-smart-contracts
- GitHub Check: zizmor
- GitHub Check: Cargo Audit
🔇 Additional comments (10)
transport/protocol/src/lib.rs (10)
7-7: Documentation updated to reflect protocol unificationThe protocol name change from "msg" to "mix" in the documentation correctly reflects the unification of message and acknowledgment protocols into a single payload structure as described in the PR objectives.
59-60: Module restructuring aligns with protocol unificationThe change from separate message and acknowledgment modules to a unified "processor" module aligns well with the PR objective of unifying these protocols at the libp2p level.
84-87: Well-defined packet size constants and codec typesThe addition of constants for packet size and defining a fixed-length codec type alias improves code clarity and maintainability. The protocol name constant also helps with protocol versioning.
254-260: Improved error handling for PeerId conversionThis change properly handles the error case when converting PeerId to OffchainPublicKey instead of using unwrap(), which could cause panics. This is a good improvement that addresses a previous review comment.
262-280: Enhanced error feedback with random signed acknowledgementSending a random signed acknowledgement when packet processing fails provides valuable feedback to the sender, improving the protocol's robustness. The error handling is comprehensive with appropriate logging.
286-308: Explicit replay detection with bloom filterThe implementation now includes explicit replay detection using the bloom filter, which helps prevent replay attacks. The metrics for replayed packets are appropriately incremented when replays are detected.
317-341: Unified packet handling for final packetsThe handling of final packets now includes proper acknowledgment creation and sending, with comprehensive error handling. This implementation aligns with the goal of unifying message and acknowledgment protocols.
342-377: Comprehensive forwarded packet handlingThe forwarding logic includes both sending the packet to the next hop and sending an acknowledgment back to the previous hop, with robust error handling throughout. This implementation supports the unified protocol approach well.
378-384: Improved application data decoding with error handlingThe application data decoding process now includes proper error handling with detailed logging, which will make debugging issues easier.
192-193:✅ Verification successful
Packet processor instantiation without bloom filter
The packet processor is now instantiated without the bloom filter argument, which suggests the bloom filter handling has been moved internally. Ensure this aligns with the implementation in the processor module.
🏁 Script executed:
#!/bin/bash # Check if the packet processor implementation handles bloom filter internally grep -A 20 "struct PacketProcessor" transport/protocol/src/processor.rs || echo "PacketProcessor struct not found"Length of output: 698
🏁 Script executed:
#!/bin/bash # Search for PacketProcessor::new signature grep -R "impl<Db> PacketProcessor" -n transport/protocol/src/processor.rs -A 20 # Specifically locate the constructor rg "fn new" -n transport/protocol/src/processor.rs # Search for bloom usage in the transport protocol module rg "bloom" -n transport/protocol/src # Locate PacketInteractionConfig definition rg "struct PacketInteractionConfig" -n transport/protocol/srcLength of output: 2314
🏁 Script executed:
#!/bin/bash # Print PacketInteractionConfig definition and its fields sed -n '250,300p' transport/protocol/src/processor.rs # Locate where packet_cfg is constructed in the main library grep -R -n "packet_cfg" transport/protocol/src/lib.rs -A 20Length of output: 4653
Bloom filter integration confirmed in
lib.rs; no changes neededThe
PacketProcessor::new(db, cfg)constructor correctly only takes the database andPacketInteractionConfig. Bloom filter setup and persistence are handled upstream intransport/protocol/src/lib.rsvia theWrappedTagBloomFilter, so no bloom-filter argument is required in the processor module.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
db/sql/src/db.rs(1 hunks)db/sql/src/protocol.rs(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- db/sql/src/protocol.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6765
File: transport/api/src/lib.rs:602-605
Timestamp: 2025-01-15T14:52:38.363Z
Learning: In the HOPR transport layer, the `TicketAggregatorProxy` is intentionally kept separate from the main transport interface to allow for potential extraction to upper interfaces in the future. This design decision facilitates architectural flexibility and should be maintained.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: tests-smoke-websocket
- GitHub Check: tests-unit
- GitHub Check: tests-smart-contracts
- GitHub Check: tests-unit-nightly
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: Docs / Rust docs
- GitHub Check: Linter
- GitHub Check: zizmor
- GitHub Check: Cargo Audit
|
The github runner for the unit tests has disk I/O errors, tested locally for unit tests - all passing. |
The protocols have so far lived as separate protocols on the libp2p level. In order to improve privacy, they can be joined inside a single payload structure to externally appear only as sending messages.
This pull request includes several changes to the
Cargo.toml,crypto/packet/src/packet.rs,db/api/src/protocol.rs, anddb/sql/src/protocol.rsfiles, focusing on improving the handling of acknowledgments, restructuring data models, and adding metrics for better observability. The most significant changes include replacing theno_ackfield with anOption<HalfKey>forack_key, refactoring acknowledgment handling, and introducing new Prometheus metrics.Changes to acknowledgment handling:
no_ackboolean field inHoprIncomingPacketwith anOption<HalfKey>forack_key, allowingNonewhen no acknowledgment is required. Updated the logic inHoprPacketto deriveack_keyonly when necessary. (crypto/packet/src/packet.rs, [1] [2]no_ackfield from theTransportPacketWithChainDataenum and replaced it with the newOutgoingPacketstruct, simplifying the outgoing packet model. (db/api/src/protocol.rs, [1] [2]Option<HalfKey>logic forack_keyand removed assertions related tono_ack. (crypto/packet/src/packet.rs, [1] [2] [3] [4] [5] [6]Refactoring acknowledgment processing:
handle_acknowledgementmethod inHoprDbProtocolOperationsby removing theAckResultenum and usingResult<()>instead. This change makes the acknowledgment handling pipeline more streamlined. (db/api/src/protocol.rs, [1] [2]db/sql/src/protocol.rsto align with the updated interface and added Prometheus metrics for received acknowledgments. (db/sql/src/protocol.rs, [1] [2] [3]Metrics and observability:
METRIC_RECEIVED_ACKS,METRIC_SENT_ACKS, andMETRIC_TICKETS_COUNT, to track acknowledgment and ticket-related events. (db/sql/src/protocol.rs, [1] [2]db/sql/src/protocol.rs, [1] [2]Dependency and workspace updates:
transport/packetandtransport/ticket-aggregationtoCargo.toml, and included their dependencies. (Cargo.toml, [1] [2]Notes
Relates to #5662