feat: close group views in quotes and payment verification#48
feat: close group views in quotes and payment verification#48mickvandijke wants to merge 4 commits intomainfrom
Conversation
…yment Nodes now return their local close group view (up to CLOSE_GROUP_SIZE peer IDs) in ChunkQuoteResponse::Success, enabling clients to verify close-group quorum before paying. On the payment verification side, nodes now check that the other peers in a payment proof are known close group members for the content address. The new validate_close_group_membership step requires at least CLOSE_GROUP_MAJORITY proof peers to appear in the node's local close group view (self + DHT peers). This prevents malicious clients from including arbitrary attacker nodes in payment proofs. Key changes: - ChunkQuoteResponse::Success gains a close_group field (Vec<[u8; 32]>) - AntProtocol holds Option<Arc<P2PNode>> for local DHT lookups - handle_quote and handle_put query the local routing table - PaymentVerifier.verify_payment accepts local_close_group for membership checks - PaymentVerifierConfig gains local_peer_id for building the full close group set Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… DHT lookups Devnet and e2e test nodes were constructed with p2p_node=None and never received it later, silently skipping close-group validation. Switch AntProtocol.p2p_node to OnceLock with a set_p2p_node() setter so the P2P node can be injected after construction. Wire it up in both devnet::start_node and testnet::start_node. Also extracts the duplicated DHT lookup into AntProtocol::local_close_group(), fixes the step-numbering comment in handle_put (5→6 for the store step), and clarifies docs around find_closest_nodes_local semantics (excludes self, CLOSE_GROUP_SIZE count rationale). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the permissive majority check (3/5 recognized) with strict validation that rejects the proof if ANY peer is not in the verifying node's current close group. This prevents malicious clients from including attacker-controlled nodes in payment proofs. Also promotes invalid ML-DSA pub_key derivation from a silent debug log to a hard error — a bad pub_key in a proof is suspicious, not benign. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR strengthens payment-proof validation by tying proofs to the locally known close group for a content address, and exposes each node’s local close-group view in quote responses so clients can verify quorum membership before paying.
Changes:
- Add
close_group: Vec<[u8; 32]>toChunkQuoteResponse::Successand populate it via local DHT lookups. - Extend payment verification to validate that peers referenced by a payment proof are members of the locally known close group (plus self).
- Plumb local peer identity / P2P node access into the storage protocol handler (constructor arg + post-construction injection for devnet/e2e).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/testnet.rs | Adds local_peer_id to payment config and injects P2P node into the protocol handler in e2e setup. |
| tests/e2e/data_types/chunk.rs | Updates test payment config (local_peer_id) and AntProtocol::new(..., None) signature change. |
| src/storage/mod.rs | Updates public docs/example for new AntProtocol::new(..., None) signature. |
| src/storage/handler.rs | Adds OnceLock<Arc<P2PNode>>, local close-group lookup, async quote handling, and passes close-group into payment verification + quote responses. |
| src/payment/verifier.rs | Adds local_peer_id to config, threads close-group into verification, and implements close-group membership validation for proof peers. |
| src/node.rs | Wraps P2P node in Arc earlier, passes it into AntProtocol, and populates local_peer_id. |
| src/devnet.rs | Adds local_peer_id and injects P2P node into protocol handler for devnet. |
| src/ant_protocol/chunk.rs | Extends ChunkQuoteResponse::Success with close_group. |
| Cargo.lock | Updates lockfile metadata for saorsa-core crate source/checksum. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Success { | ||
| /// Serialized `PaymentQuote`. | ||
| quote: Vec<u8>, | ||
| /// `true` when the chunk already exists on this node (skip payment). | ||
| already_stored: bool, | ||
| /// Up to `CLOSE_GROUP_SIZE` peer IDs (raw 32-byte BLAKE3 hashes) this | ||
| /// node considers closest to the content address, **excluding itself** | ||
| /// (the local node is filtered out by the DHT query). Clients combine | ||
| /// these views from multiple nodes to verify close-group quorum before | ||
| /// paying. | ||
| close_group: Vec<[u8; 32]>, | ||
| }, |
There was a problem hiding this comment.
Adding close_group to ChunkQuoteResponse::Success changes the postcard-encoded wire format for this response variant. With CHUNK_PROTOCOL_ID still at /v1 (and PROTOCOL_VERSION unchanged), older clients/nodes will fail to deserialize these responses. Consider bumping the protocol ID/version (e.g., /v2) or introducing a backwards-compatible encoding strategy (e.g., a new response variant) so mixed-version networks can interoperate.
| /// Verify that **every** peer in the payment proof is a known close group member. | ||
| /// | ||
| /// Builds the known set from the current DHT close group plus this node | ||
| /// itself, then checks that each proof peer (derived via `BLAKE3(pub_key)`) | ||
| /// appears in that set. Rejects the proof if ANY peer is unrecognized. | ||
| /// | ||
| /// Skipped when `local_close_group` is empty (unit tests without DHT). | ||
| fn validate_close_group_membership( | ||
| &self, | ||
| payment: &ProofOfPayment, | ||
| local_close_group: &[[u8; 32]], | ||
| ) -> Result<()> { | ||
| if local_close_group.is_empty() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Build the known peer set: current DHT close group + this node. | ||
| let mut known_peers: HashSet<[u8; 32]> = local_close_group.iter().copied().collect(); | ||
| known_peers.insert(self.config.local_peer_id); | ||
|
|
||
| // Every proof peer must be in the known set. | ||
| for (_encoded_peer_id, quote) in &payment.peer_quotes { | ||
| let peer_id = peer_id_from_public_key_bytes("e.pub_key).map_err(|e| { | ||
| Error::Payment(format!("Invalid ML-DSA pub_key in proof quote: {e}")) | ||
| })?; | ||
|
|
||
| if !known_peers.contains(peer_id.as_bytes()) { | ||
| return Err(Error::Payment(format!( | ||
| "Proof peer {} is not in the current close group", | ||
| peer_id.to_hex() | ||
| ))); | ||
| } |
There was a problem hiding this comment.
This membership check currently rejects the proof if any peer quote is not in the locally known close-group set, but the PR description says nodes should reject proofs with too many unknown peers. If some divergence is expected between nodes’ close-group views, consider allowing a bounded number of unknown peers (e.g., enforce a threshold like <= CLOSE_GROUP_SIZE - CLOSE_GROUP_MAJORITY) rather than failing on the first unknown.
| Self::validate_quote_structure(payment)?; | ||
| Self::validate_quote_content(payment, xorname)?; | ||
| Self::validate_quote_timestamps(payment)?; | ||
| Self::validate_peer_bindings(payment)?; | ||
| self.validate_local_recipient(payment)?; | ||
| self.validate_close_group_membership(payment, local_close_group)?; | ||
|
|
There was a problem hiding this comment.
New close-group membership enforcement is introduced here, but there are no unit tests asserting (1) a proof is accepted when all peers are in local_close_group + local_peer_id, and (2) a proof is rejected when at least one peer is outside that set. Adding targeted tests would help prevent regressions in this security-critical validation.
| let lock = OnceLock::new(); | ||
| if let Some(node) = p2p_node { | ||
| // Fresh OnceLock — set cannot fail. | ||
| let _ = lock.set(node); |
There was a problem hiding this comment.
OnceLock::set cannot fail here (fresh lock), but the let _ = ... silently discards the result. Using expect (or debug_assert!(lock.set(node).is_ok())) would make the invariant explicit and avoid hiding future changes where this could fail.
| let _ = lock.set(node); | |
| lock.set(node) | |
| .expect("p2p_node OnceLock was just created; set() must succeed"); |
src/devnet.rs
Outdated
|
|
||
| if let (Some(ref p2p), Some(ref protocol)) = (&node.p2p_node, &node.ant_protocol) { | ||
| // Inject P2P node into protocol handler for close-group lookups. | ||
| let _ = protocol.set_p2p_node(Arc::clone(p2p)); |
There was a problem hiding this comment.
set_p2p_node can return Err if the node was already injected; the result is currently ignored. Consider handling this explicitly (e.g., log/warn on Err or use expect if it should be impossible) to avoid silently running without the intended close-group lookup wiring.
| let _ = protocol.set_p2p_node(Arc::clone(p2p)); | |
| if let Err(e) = protocol.set_p2p_node(Arc::clone(p2p)) { | |
| warn!( | |
| "Failed to inject P2P node into protocol for devnet node {index}: {e}" | |
| ); | |
| } |
tests/e2e/testnet.rs
Outdated
| // Start protocol handler that routes incoming P2P messages to AntProtocol | ||
| if let (Some(ref p2p), Some(ref protocol)) = (&node.p2p_node, &node.ant_protocol) { | ||
| // Inject P2P node into protocol handler for close-group lookups. | ||
| let _ = protocol.set_p2p_node(Arc::clone(p2p)); |
There was a problem hiding this comment.
set_p2p_node can return Err if the node was already injected; the result is currently ignored. Consider handling this explicitly (e.g., log/warn on Err or use expect if it should be impossible) so testnet setup fails loudly if the protocol handler isn't wired for close-group lookups.
| let _ = protocol.set_p2p_node(Arc::clone(p2p)); | |
| protocol | |
| .set_p2p_node(Arc::clone(p2p)) | |
| .map_err(|e| { | |
| TestnetError::Startup(format!( | |
| "Failed to inject P2P node into protocol handler for node {}: {e}", | |
| node.index | |
| )) | |
| })?; |
…node errors Address Copilot review feedback: add 4 unit tests covering the accept, reject, skip, and local-peer-known paths of validate_close_group_membership, and handle set_p2p_node errors explicitly in devnet (warn) and testnet (propagate as TestnetError::Startup). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implements the remaining untested scenarios from REPLICATION_DESIGN.md Section 18, bringing coverage from 47/56 to 56/56: - #20: paid-list local hit bypasses presence quorum (quorum.rs) - #22: paid-list rejection below threshold (quorum.rs) - #29: audit start gate during bootstrap (audit.rs) - #30: audit peer selection from sampled keys (audit.rs) - #31: audit periodic cadence with jitter bounds (config.rs) - #32: dynamic challenge size equals PeerKeySet (audit.rs) - #47: bootstrap claim grace period in audit path (audit.rs) - #48: bootstrap claim abuse after grace period (paid_list.rs) - #53: audit partial per-key failure with mixed responsibility (audit.rs) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
ChunkQuoteResponse::Success, enabling clients to verify quorum before payinghandle_put), nodes validate that all peers in the proof are current close group members — rejecting proofs containing any unknown peerAntProtocolusesOnceLock<Arc<P2PNode>>with aset_p2p_node()setter so close-group validation works in devnet and testnet (wherep2p_nodeis injected after construction)AntProtocol::local_close_group()Motivation
Data is only permanently stored and replicated if accepted by a majority of close group nodes. Without this change, nodes blindly accept any payment proof that includes them, allowing malicious clients to include arbitrary attacker nodes in proofs. The client also had no way to verify it was paying the right set of nodes before spending tokens.
Changes
src/payment/verifier.rspub_keyderivation from a silent debug log to a hard errorsrc/storage/handler.rsAntProtocol::local_close_group()handle_put(5→6 for the store step)find_closest_nodes_localsemantics (excludes self,CLOSE_GROUP_SIZEcount rationale)src/ant_protocol/chunk.rsAntProtocol.p2p_nodetoOnceLock<Arc<P2PNode>>withset_p2p_node()settersrc/devnet.rs&tests/e2e/testnet.rsset_p2p_node()after node construction so close-group validation is active in devnet and e2e testsTest plan
cargo test --lib)--all-targets --all-features -- -D warnings🤖 Generated with Claude Code