fix(payment): enforce single-node proof verification#107
Conversation
SemVer: patch
There was a problem hiding this comment.
Pull request overview
Tightens single-node payment proof verification in PaymentVerifier by ensuring the proof’s quoted peers match the verifier’s local close-group view and by strengthening the on-chain validation (median-priced selection, recipient-prefix binding, and under/overpayment handling) against PaymentVault.completedPayments.
Changes:
- Adds local close-group membership enforcement for all quoted peers in single-node proofs.
- Replaces the previous
SingleNodePayment::verify(...)path with explicit on-chaincompletedPayments(quoteHash)checks that handle median-price ties and validate rewards-address prefix + paid amount. - Extends unit tests to cover close-group set validation and completed-payment amount/recipient-prefix checks.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SemVer: patch
SemVer: patch
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/payment/verifier.rs:501
- The CPU-heavy ML-DSA signature verification runs before the single-node close-group check (which also fails fast if no P2PNode is attached). To reduce DoS surface and avoid wasting CPU when the proof will be rejected due to close-group / attachment constraints, consider running
validate_single_node_close_group(...)(or at least thep2p_nodeattachment + local peer/rewards checks) before spawning the blocking signature verification task.
// Verify quote signatures (CPU-bound, run off async runtime)
let peer_quotes = payment.peer_quotes.clone();
tokio::task::spawn_blocking(move || {
for (encoded_peer_id, quote) in &peer_quotes {
if !verify_quote_signature(quote) {
grumbach
left a comment
There was a problem hiding this comment.
LGTM, approving. This is the right shape — covers the six invariants you listed at the top and binds the on-chain recipient prefix to the paid quote's own rewards address (not this node's), which fixes the model regression my reverted PR introduced.
A few notes, none merge-blocking:
1. Price floor (the one we discussed). Dropping (b) leaves an economic underpricing hole: an attacker who controls ≥5 nodes in a victim's local close-group view can sign 7 quotes at 1 atto, pay 3 atto to one of their own nodes, and store data on the network for ~gas-only. The close-group / 5-of-7 check forces them to own real RT-resident peers but not to price honestly. Agreed the 4× divisor I picked was a guess — but a baseline-pegged floor (e.g. quote.price >= calculate_price(0) = BASELINE) costs us nothing on legit traffic and shuts the 1-atto path. Worth a follow-up at minimum.
2. Replication ingestion path. validate_local_quoted_peer rejects any proof where this node isn't in the quoted set — correct for fresh PUTs. But if a node becomes newly responsible after an RT shift, the original quote set won't include it. Worth tracing handle_fresh_offer / replication ingestion to confirm it short-circuits via the verified-cache (or via a different path) rather than re-running verify_evm_payment, otherwise legitimate re-replication breaks. If you've already checked that, ignore.
3. 5-of-7 close-group threshold. Same shape as the merkle 13/16; the 2-peer skew tolerance is defensible but reduces to "attacker needs ≥5 RT-close real peers" — same Sybil cost class as the #99 residual. A code comment with the calibration rationale (or a pointer to a test you ran) would help future reviewers not re-derive it.
4. Test coverage of validate_single_node_close_group. The pure helper check_single_node_close_group_match is well-tested; the wiring (P2PNode-required, error path) doesn't have a unit test. Minor.
Ship it.
Hey @grumbach , I agree that the price check is a good idea, but I would like the change to be its own PR as a follow-up because we can then also remove the requirement of sending 7 quotes with payments. Instead we can let clients just send the 1 paid quote with the upload. The node will then just verify if the quote issuer is in its current closest group and if the price is within margin of its own calculated price. If we compare the quote to the local node's price calculation, then there is no need for all the other quotes to be included at all. If we also include closest group data in quote responses, then clients will be able to "smart" pick the best quote that will be accepted by a majority of peers in the close group for a chunk. |
Description
This PR tightens the final single-node payment verification path used before a new PUT is cached as paid. The verifier still enforces the existing quote count, distinct encoded peer IDs, content-address binding, quote freshness, ML-DSA public-key-to-peer-ID binding, and quote signature checks. After that, it now requires an attached
P2PNode, verifies that this node's peer ID is one of the quoted peers, verifies that the local peer's own quote uses the configured local rewards address, and checks the quoted peer set againstfind_closest_nodes_local_with_self(xorname, CLOSE_GROUP_SIZE). The close-group check is intentionally local and self-inclusive, so it avoids a request-handler network DHT lookup while requiring at least 5 of 7 quoted peers to match and tolerating up to two missing peers for normal routing-table skew.For the on-chain part, the verifier no longer delegates to the generic
SingleNodePayment::verify()amount-only check. It reconstructs the sorted single-node payment, checks the selected median-priced quote first, falls back to other median-price ties if needed, and validates each candidate withPaymentVaultV2.completedPayments(quote.hash()). A candidate only passes if the recorded amount is at least3 * quote.priceand the stored 16-byte recipient prefix matches the quote rewards address. Full 20-byte recipient equality is still not possible fromcompletedPaymentswithout a contract/ABI change or event-based reconstruction.SemVer: patch
Tests
cargo fmt --all -- --checkcargo test payment::verifiercargo clippy --all-targets --all-features -- -D warnings