docs(payment): document storer-side bad-binding rejection + add regression tests#90
Open
grumbach wants to merge 5 commits intoWithAutonomi:mainfrom
Open
docs(payment): document storer-side bad-binding rejection + add regression tests#90grumbach wants to merge 5 commits intoWithAutonomi:mainfrom
grumbach wants to merge 5 commits intoWithAutonomi:mainfrom
Conversation
When `validate_peer_bindings` rejects a quote because its `pub_key`
does not BLAKE3-hash to the claimed `PeerId`, report a strong
negative trust event so this storer's own AdaptiveDHT swaps that
peer out of the routing table on the next admission cycle.
This mirrors the client-side downscore in
`ant-client/ant-core/src/data/client/quote.rs` so both ends of the
wire apply the same penalty for the same evidence (a verifiable
cryptographic mismatch). Storer-side eviction means cleaner close-K
answers feed back to clients on the next lookup, which is the
second-order win on top of the per-client routing-table fix.
Changes in `src/payment/verifier.rs`:
- `validate_peer_bindings` becomes `&self` async so it can call
`self.report_bad_binding(encoded_peer_id).await` for both the
malformed-pub_key and the BLAKE3-mismatch error paths;
- new `report_bad_binding` helper resolves the encoded peer ID to
a `PeerId` and forwards to
`P2PNode::report_application_failure(peer, 5.0)`. No-op when
`P2PNode` isn't attached (only happens in unit-test builds that
do not exercise merkle verification).
The `5.0` weight is sized to drop a peer from neutral 0.5 to ~0.26
in a single event, well below the production swap-out threshold
(`saorsa_core::adaptive::DEFAULT_SWAP_THRESHOLD = 0.35`). saorsa-core
clamps consumer weights at `MAX_CONSUMER_WEIGHT = 5.0` so this is
the strongest legal signal — appropriate for a verifiable
cryptographic mismatch. Mirrors the client-side
`BAD_BINDING_TRUST_WEIGHT` constant.
Adds two tests:
- validate_peer_bindings_rejects_and_runs_report_path (C1)
A `ProofOfPayment` with one bad-binding quote is rejected with
the expected `Error::Payment` and the report path runs
cleanly when no `P2PNode` is attached.
- validate_peer_bindings_passes_through_when_all_quotes_clean (C2)
A `ProofOfPayment` whose every quote has a self-consistent
pub_key/peer_id pair passes the validator with no error.
The trust-event emission itself is exercised by the integration
tests that run real `P2PNode` instances; the storer-side path is
structurally identical to the client-side reporter wiring tested
in ant-client.
Depends on: saorsa-labs/saorsa-core#XXX
(P2PNode::report_application_failure entry point)
See notes/plan-1-bad-node-eviction.md for the full design and the
2026-05-06 production failure that motivates this work.
Open
3 tasks
3 tasks
There was a problem hiding this comment.
Pull request overview
This PR extends the storer-side payment proof validation to emit a strong negative trust signal when a quote’s pub_key does not BLAKE3-hash to the claimed PeerId, mirroring similar client-side behavior, with the goal of evicting misbound peers from the storer’s AdaptiveDHT routing table.
Changes:
- Make
validate_peer_bindingsasync and call a newreport_bad_bindinghelper on detected bad bindings. - Add
report_bad_bindingto translateEncodedPeerId→PeerIdand forward a weighted penalty toP2PNode::report_application_failure. - Add unit tests covering acceptance/rejection behavior and exercising the no-op report path when no
P2PNodeis attached.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+602
to
+608
| let expected_peer_id = match peer_id_from_public_key_bytes("e.pub_key) { | ||
| Ok(p) => p, | ||
| Err(e) => { | ||
| self.report_bad_binding(encoded_peer_id).await; | ||
| return Err(Error::Payment(format!( | ||
| "Invalid ML-DSA public key in quote: {e}" | ||
| ))); |
Collaborator
Author
There was a problem hiding this comment.
Good catch, you are right. Resolved in fad7f67: the storer-side downscore is removed entirely. The EncodedPeerId field is set by the uploader and is not in the signed payload (the signature only covers (content, timestamp, price, rewards_address) per PaymentQuote::bytes_for_sig), so attribution from this code path is not safe. Trust-based eviction of bad-binding peers now lives only on the client side (#77), where the responding peer is grounded in the QUIC connection identity. The storer keeps the structural rejection but no longer fires a trust event.
Comment on lines
612
to
616
| if expected_peer_id.as_bytes() != encoded_peer_id.as_bytes() { | ||
| let expected_hex = expected_peer_id.to_hex(); | ||
| let actual_hex = hex::encode(encoded_peer_id.as_bytes()); | ||
| self.report_bad_binding(encoded_peer_id).await; | ||
| return Err(Error::Payment(format!( |
Collaborator
Author
There was a problem hiding this comment.
Same fix as above — resolved in fad7f67 by removing the storer-side downscore entirely. The eviction logic stays client-side where the peer identity is bound to the QUIC connection rather than to attacker-controlled proof bytes.
…fix) Address Copilot review on PR WithAutonomi#90: the previous storer-side downscore was a trust-poisoning vector. The `(EncodedPeerId, PaymentQuote)` tuple inside `ProofOfPayment` is assembled by the payment uploader, and the quote signature only covers `(content, timestamp, price, rewards_address)` (see `PaymentQuote::bytes_for_sig` in evmlib). The `pub_key` field and the `EncodedPeerId` are NOT part of the signed payload, so a malicious uploader could pair a victim's `peer_id` with a bogus or unrelated quote to trigger a trust penalty against the victim while providing no cryptographic evidence of the victim misbehaving. The storer can still **reject** the proof structurally (which is harmless to bystanders) but cannot safely attribute the fault to any specific peer. This commit: - Removes `report_bad_binding` and the `BAD_BINDING_TRUST_WEIGHT` constant. - Reverts `validate_peer_bindings` to its original sync free-fn signature; the call site goes back to `Self::validate_peer_bindings`. - Replaces the C1 test (which asserted the report path runs) with a regression test that simply confirms bad-binding proofs are still rejected. - Adds an extensive doc comment on `validate_peer_bindings` explaining the trust-poisoning constraint so future work doesn't reintroduce the same hole without authenticated peer-id binding on the wire first. Trust-based eviction of bad-binding peers therefore lives only on the client side (`ant-client/ant-core/src/data/client/quote.rs`), where the responding peer's identity is grounded in the QUIC connection from `find_closest_peers` rather than in attacker- controlled proof bytes. Net effect: this PR's storer-side scope shrinks to a documentation update + the existing rejection logic. No behaviour change to the production verifier.
grumbach
changed the title
Strict CI clippy (--all-targets --all-features) caught: - clippy::panic in test code: replaced `panic!` with `assert!(matches!(...))` + a follow-up message check. - clippy::doc_markdown: `pub_key/peer_id` in C2 doc comment is now backticked. No behaviour change.
The new bad-binding regression tests use panic!() in match arms to flag unexpected outcomes — same shape as the existing tests on PR WithAutonomi#89. The workspace clippy config has -D clippy::panic, so the test module needs the explicit allow alongside the existing clippy::expect_used. Fixes the Clippy CI failure on PR WithAutonomi#90.
Comment on lines
+2649
to
+2653
| // The crossed-key shape: the EncodedPeerId is random rather than | ||
| // BLAKE3(pub_key), mirroring the production failure pattern (an | ||
| // operator running two co-located identities with crossed keys). | ||
| let bad_peer_id = EncodedPeerId::new(rand::random()); | ||
| evmlib::ProofOfPayment { |
Comment on lines
+2708
to
+2712
| // The well-bound shape: PeerId derives from BLAKE3(pub_key). | ||
| let derived = peer_id_from_public_key_bytes(&pub_key_bytes).expect("derive"); | ||
| let mut bytes = [0u8; 32]; | ||
| bytes.copy_from_slice(derived.as_bytes()); | ||
| peer_quotes.push((EncodedPeerId::new(bytes), quote)); |
Resolves a textual conflict in src/payment/verifier.rs tests module: both branches appended new test blocks at the end of the module, the C1/C2 bad-binding rejection tests from this PR and the closeness window regression tests from main. Splice both in; no semantic overlap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
validate_peer_bindingsand adds two regression tests (C1: bad-binding proofs are rejected; C2: clean proofs pass).(EncodedPeerId, PaymentQuote)tuple is assembled by the payment uploader, and the quote signature covers(content, timestamp, price, rewards_address)only — neither thepub_keyfield nor theEncodedPeerIdis part of the signed payload. A malicious uploader could pair a victim's peer-id with a bogus quote to trigger a trust penalty against the victim with no cryptographic attribution.ant-client/ant-core/src/data/client/quote.rs, in PR feat(quote): downscore peers that supply bad-bound quotes ant-client#77), where the responding peer's identity is grounded in the QUIC connection fromfind_closest_peersrather than in attacker-controlled proof bytes.What's in this PR now
validate_peer_bindingskeeps its original sync free-fn signature; behaviour is unchanged frommain.validate_peer_bindingsexplaining the trust-poisoning constraint so future work doesn't reintroduce the same hole without authenticated peer-id binding on the wire first.validate_peer_bindings_rejects_bad_binding_proofs(C1) andvalidate_peer_bindings_passes_through_when_all_quotes_clean(C2) — regression tests for the existing rejection logic.Test plan
cargo test --lib payment::verifier::tests::validate_peer_bindings(2/2)cargo clippy --lib --all-features -- -D warningscargo fmt --all -- --checkCross-links
Future work
If we ever want a safe storer-side downscore here, the prerequisite is authenticating the
EncodedPeerIdfield — either by including it inPaymentQuote::bytes_for_sigor by signing the whole(EncodedPeerId, PaymentQuote)tuple separately. That would let the storer attribute bad bindings cryptographically and downscore without trust-poisoning risk. Out of scope for this PR.