feat(peerup): consolidate crypto, add distributed consensus and DHT operations#23
feat(peerup): consolidate crypto, add distributed consensus and DHT operations#23veryCrunchy wants to merge 1 commit intomainfrom
Conversation
veryCrunchy
commented
Feb 21, 2026
veryCrunchy
commented
- Move crypto into peerup crate: Ed25519 signing, X25519 ECDH, XChaCha20-Poly1305 AEAD
- Add distributed consensus, DHT storage, metadata, sync, visibility modules
- Add trust module for peer trust scoring
- Add Kademlia DHT put/get/provide operations (node/core/dht.rs)
- Refactor network event conversions into dedicated modules (kad, mdns, relay, request_response)
- Remove flat conv files (kad_conv, mdns_conv, relay_conv)
- Fix P2P resilience: PeerUPEvent::Noop for unhandled events
- Update gossipsub, node_methods, events to support new architecture
…perations - Move crypto into peerup crate: Ed25519 signing, X25519 ECDH, XChaCha20-Poly1305 AEAD - Add distributed consensus, DHT storage, metadata, sync, visibility modules - Add trust module for peer trust scoring - Add Kademlia DHT put/get/provide operations (node/core/dht.rs) - Refactor network event conversions into dedicated modules (kad, mdns, relay, request_response) - Remove flat conv files (kad_conv, mdns_conv, relay_conv) - Fix P2P resilience: PeerUPEvent::Noop for unhandled events - Update gossipsub, node_methods, events to support new architecture
There was a problem hiding this comment.
Pull request overview
This pull request consolidates cryptographic operations into the peerup crate and adds comprehensive distributed peer-to-peer functionality including consensus, DHT operations, and peer data synchronization. The changes represent a significant architectural evolution from a simple P2P library to a full-featured distributed system framework.
Changes:
- Moved crypto functionality (Ed25519 signing/verification, X25519 ECDH, XChaCha20-Poly1305 AEAD) into peerup crate
- Added distributed consensus protocol with rate limiting and Byzantine fault tolerance via signature verification
- Added DHT operations for public monitor discovery, metadata storage, and peer coordination
- Introduced peer data storage, sync manager, and visibility types for managing distributed state
- Refactored network event conversions into dedicated modules and introduced
PeerUPEvent::Noopfor unhandled events - Added trust module placeholder for future peer reputation functionality
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/peerup/src/crypto/keys.rs | Ed25519 keypair with X25519 derivation for encryption |
| crates/peerup/src/crypto/signing.rs | Ed25519 signing operations for bytes and JSON |
| crates/peerup/src/crypto/verification.rs | Ed25519 signature verification |
| crates/peerup/src/crypto/encryption.rs | X25519 + XChaCha20-Poly1305 encryption for private data |
| crates/peerup/src/crypto/signatures.rs | Signed result verification for P2P distribution |
| crates/peerup/src/crypto/mod.rs | Crypto module exports |
| crates/peerup/src/distributed/mod.rs | Distributed module structure and configuration |
| crates/peerup/src/distributed/visibility.rs | Monitor visibility types and orchestration scheduling |
| crates/peerup/src/distributed/sync.rs | Sync manager for peer data synchronization |
| crates/peerup/src/distributed/storage.rs | Peer data storage abstraction and in-memory implementation |
| crates/peerup/src/distributed/metadata.rs | DHT-stored metadata for rate limits and trust scores |
| crates/peerup/src/distributed/messages.rs | Message types for peer data queries and sync |
| crates/peerup/src/distributed/dht.rs | DHT-based public monitor discovery |
| crates/peerup/src/distributed/consensus.rs | Consensus protocol with rate limiting and signature verification |
| crates/peerup/src/trust/mod.rs | Placeholder module for trust management |
| crates/peerup/src/node/core/dht.rs | Kademlia DHT put/get/provide operations |
| crates/peerup/src/node/core/gossipsub.rs | GossipSub topic subscription and publishing methods |
| crates/peerup/src/node/core/node_methods.rs | Bootstrap peer handling improvements |
| crates/peerup/src/node/core/mod.rs | Added DHT module export |
| crates/peerup/src/node/events.rs | Updated event handler for probe requests |
| crates/peerup/src/node/config/types.rs | Added distributed peer data configuration options |
| crates/peerup/src/network/events.rs | Added DHT record events and Noop variant |
| crates/peerup/src/network/conversions/kad.rs | Kademlia event conversion with DHT record handling |
| crates/peerup/src/network/conversions/mdns.rs | mDNS event conversion using Noop for empty lists |
| crates/peerup/src/network/conversions/relay.rs | Relay event conversion using Noop for unhandled events |
| crates/peerup/src/network/conversions/request_response.rs | Request/response event conversion using Noop |
| crates/peerup/src/network/kad_conv.rs | Removed flat conversion file |
| crates/peerup/src/network/mdns_conv.rs | Removed flat conversion file |
| crates/peerup/src/network/relay_conv.rs | Removed flat conversion file |
| crates/peerup/src/lib.rs | Updated library documentation and exports |
| crates/peerup/Cargo.toml | Added crypto dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //! Admin trust management for decentralized authentication. | ||
| //! | ||
| //! This module provides: | ||
| //! - HTTPS bootstrap from trusted sources | ||
| //! - Admin key caching in DHT | ||
| //! - Trust chain management | ||
| //! - Revocation handling | ||
|
|
||
| // This module will be expanded to hold trust-related operations | ||
| // Currently serves as architectural placeholder for security separation |
There was a problem hiding this comment.
The trust module is added but only contains a placeholder comment. While architectural placeholders can be useful, this module is exported from lib.rs and appears in the public API, but provides no actual functionality. This could confuse users of the crate.
Either implement basic trust functionality, or remove the module from the public exports until it's actually implemented. If keeping it, consider adding a clear #[doc(hidden)] or noting in the module documentation that it's a placeholder for future development.
| PeerUPEvent::ProbeRequestReceived { peer, request, channel } => { | ||
| info!("Received probe request from {}: {:?}", peer, request); | ||
| handle_probe_request(peer, request, channel); | ||
| // We cannot send a response here because we don't have swarm access. | ||
| // Log the request and drop the channel — the peer will see an inbound | ||
| // failure. Production consumers should handle this variant in their | ||
| // own swarm event loop where they have mutable access to the behaviour. | ||
| warn!( | ||
| "ProbeRequestReceived from {} ({:?}) — dropping channel \ | ||
| (handle in your own event loop to respond)", | ||
| peer, request | ||
| ); | ||
| drop(channel); |
There was a problem hiding this comment.
The comment and warning about dropping the response channel is helpful, but the approach of logging at warn level and explicitly dropping the channel in a general-purpose event handler is problematic. If this handler is called in a context where the channel should be handled (like in a service), the warning will fire unnecessarily and the peer will see an inbound failure.
Consider either removing this handler entirely (since the comment says consumers should handle this in their own event loop), or at least log at debug level instead of warn to avoid alarming users when this is expected behavior.
| // Ensure we're subscribed to the topic (required for publishing) | ||
| if !self.swarm.behaviour().gossipsub.topics().any(|t| t.to_string() == topic_name) { | ||
| self.swarm | ||
| .behaviour_mut() | ||
| .gossipsub | ||
| .subscribe(&topic) | ||
| .map_err(|e| anyhow::anyhow!("Failed to subscribe to topic {}: {}", topic_name, e))?; | ||
| tracing::debug!("Auto-subscribed to topic {}", topic_name); | ||
| } |
There was a problem hiding this comment.
The publish_to_topic method auto-subscribes to a topic if not already subscribed (line 64-71). While convenient, this behavior is surprising and could lead to unintended subscriptions. In GossipSub, subscribing to a topic means the node will receive all messages on that topic, which has bandwidth and processing implications.
Consider documenting this auto-subscription behavior prominently in the method's documentation, or requiring explicit subscription before publishing. The generic subscribe_to_topic method exists for this purpose.
| // Check that with peer_count, we don't exceed max checks/hour | ||
| let divisor = schedule.interval_seconds * peer_count as u64; | ||
| if divisor == 0 { | ||
| return Err("Invalid schedule: interval cannot be zero".to_string()); | ||
| } | ||
| let checks_per_peer_per_hour = 3600 / divisor; | ||
|
|
||
| if checks_per_peer_per_hour > self.max_checks_per_peer_per_hour { | ||
| return Err(format!( | ||
| "Schedule would cause {} checks/peer/hour (max: {})", | ||
| checks_per_peer_per_hour, self.max_checks_per_peer_per_hour | ||
| )); | ||
| } |
There was a problem hiding this comment.
The rate limit validation has a potential division by zero vulnerability. When peer_count is 1 and schedule.interval_seconds is less than 3600, the calculation 3600 / divisor could produce incorrect results. More critically, if the multiplication on line 122 overflows, divisor could be 0 despite the check on line 123.
Consider checking for overflow explicitly or using checked arithmetic. Also, the logic appears incorrect: with interval_seconds = 60 and peer_count = 1, the divisor is 60 * 1 = 60, and 3600 / 60 = 60 checks per hour per peer, which is correct. However, the comment on line 121 suggests this is meant to prevent exceeding max checks, but the calculation doesn't account for the fact that each peer performs one check per interval_seconds, not per interval_seconds * peer_count.
| match query_fn(peer_id.clone(), request).await { | ||
| Ok(response) => { | ||
| // Store each data item | ||
| let mut synced_until = 0; |
There was a problem hiding this comment.
The indentation of let mut synced_until = 0; is inconsistent with the surrounding code. This line should be indented to align with the other statements in the loop body. While this doesn't affect functionality, it impacts code readability and maintainability.
| let mut synced_until = 0; | |
| let mut synced_until = 0; |
| // Check if already voted | ||
| if self.pending_votes.iter().any(|v| v.voter_peer_id == vote.voter_peer_id) { | ||
| return Err("Peer already voted".to_string()); | ||
| } |
There was a problem hiding this comment.
The duplicate vote check on line 172 only compares voter_peer_id strings, but there's no verification that this ID matches the public key that signed the vote. A malicious peer could claim to be another peer by setting voter_peer_id to match a different peer's ID, and the signature check wouldn't catch this unless the application explicitly binds peer IDs to public keys.
Consider either: (1) requiring that voter_peer_id matches the hex-encoded public key used for signing (similar to SignedResult.verify_peer_id()), or (2) documenting clearly that the application layer must maintain and verify peer ID to public key mappings.
| /// Verify the signature on this data | ||
| pub fn verify_signature(&self) -> bool { | ||
| // TODO: Implement Ed25519 signature verification | ||
| // For now, return true (implement in storage layer) | ||
| true | ||
| } |
There was a problem hiding this comment.
The verify_signature method in PeerData is not implemented and always returns true. This is a critical security vulnerability as it means peer data signatures are never actually verified before being stored. The sync manager relies on this verification at line 94 of sync.rs to prevent storing malicious or tampered data.
The signature and public_key fields are stored as Strings but should be verified using the crypto module's verify_signature function. Implement actual Ed25519 signature verification here using the crypto utilities available in the crate.
| GetRecord(Err(_e)) => PeerUPEvent::DhtGetRecordErr { key: Vec::new() }, | ||
| PutRecord(Ok(ok)) => { | ||
| let key = ok.key.as_ref().to_vec(); | ||
| PeerUPEvent::DhtPutRecordOk { key } | ||
| } | ||
| PutRecord(Err(e)) => { | ||
| PeerUPEvent::DhtPutRecordErr { | ||
| key: Vec::new(), |
There was a problem hiding this comment.
In the DHT get/put error events, the key field is set to an empty Vec::new(). This loses important debugging information about which key failed. The original key should be preserved and included in the error event so that consumers can properly handle and log failures for specific keys.
Consider capturing the key from the error result or the original query context and including it in the error events.
| GetRecord(Err(_e)) => PeerUPEvent::DhtGetRecordErr { key: Vec::new() }, | |
| PutRecord(Ok(ok)) => { | |
| let key = ok.key.as_ref().to_vec(); | |
| PeerUPEvent::DhtPutRecordOk { key } | |
| } | |
| PutRecord(Err(e)) => { | |
| PeerUPEvent::DhtPutRecordErr { | |
| key: Vec::new(), | |
| GetRecord(Err(e)) => { | |
| let key = e.key.as_ref().to_vec(); | |
| PeerUPEvent::DhtGetRecordErr { key } | |
| } | |
| PutRecord(Ok(ok)) => { | |
| let key = ok.key.as_ref().to_vec(); | |
| PeerUPEvent::DhtPutRecordOk { key } | |
| } | |
| PutRecord(Err(e)) => { | |
| let key = e.key.as_ref().to_vec(); | |
| PeerUPEvent::DhtPutRecordErr { | |
| key, |
| pub fn dht_key(domain: &str) -> RecordKey { | ||
| RecordKey::new(&format!("/uppe/public-monitor/{}", domain)) | ||
| } |
There was a problem hiding this comment.
The DHT key format uses "uppe" as a hardcoded namespace in line 24, but the code is in the generic peerup crate which claims to be application-agnostic. The metadata module correctly uses a configurable namespace parameter, but this DHT module hardcodes "uppe".
Consider making the namespace configurable (e.g., passed to PublicMonitorDHT::new()) to maintain the generic nature of the peerup crate, or document that this module is specifically for the Uppe application.
| /// Serialize registry to DHT record | ||
| pub fn to_record(&self) -> Result<Record, Box<dyn std::error::Error>> { | ||
| let value = serde_json::to_vec(&self)?; | ||
| // Key will be set by caller based on specific domain | ||
| Ok(Record { | ||
| key: RecordKey::new(&[]), // Placeholder | ||
| value, | ||
| publisher: None, | ||
| expires: None, | ||
| }) | ||
| } |
There was a problem hiding this comment.
The to_record method creates a Record with an empty placeholder key (RecordKey::new(&[])), which is confusing and error-prone. The comment states "Key will be set by caller", but Record keys are immutable once created. This suggests the method is not actually useful as written.
Either remove this method, or have it accept a domain parameter and create the proper key directly. The current implementation could lead to bugs if someone tries to use the returned Record without realizing the key is invalid.
