feat(digsex): wire the dig-sex economic layer into the node - #456
feat(digsex): wire the dig-sex economic layer into the node#456MichaelTaylor3d wants to merge 5 commits into
Conversation
Stub commit so the branch, PR and issue log exist before implementation. Co-Authored-By: Claude <noreply@anthropic.com>
…269) Wires `dig_sex::admission` (SPEC 8.5) into the mTLS peer surface. The meter was implemented, tested and gating nothing: the node did the work first and had no admission step at all. `PeerAdmission` holds one node-wide `AdmissionMeter` and is consulted at the top of `NodeResponder::handle_json_rpc` and `handle_availability`, ahead of the method allowlist and every dispatch, so a refused request costs a hex decode rather than a read, a decode or a DHT lookup. Two properties the shape enforces rather than documents: - The meter key is the mTLS-verified `peer_id`, decoded via the existing `hex64`. A session with no verified identity is REFUSED, never admitted unmetered and never metered under a placeholder -- a constant key collapses every requestor into one bucket, so one peer would exhaust the allowance for everybody. - `AdmissionGuard` releases on `Drop`, so allowance returns on every exit path including the error paths a hand-written release is forgotten on. Co-Authored-By: Claude <noreply@anthropic.com>
Wires `dig_sex::conduct` (SPEC 8.2A), which was implemented, tested and received not one observation from dig-node. `ConductState` keys `ConductRecord`s on `RoutedPeer` -- the mTLS-verified peer_id the ask router already ranks on -- so conduct can neither be attributed to nor escaped by a self-chosen identity, and is bounded by pool membership rather than a TTL. Reputation stays node-local and is never gossiped. The forwarded-ask loop now feeds it the outcome it already classifies, and `dialable()` filters the pool BEFORE `decide_forward` ranks it: ranking a peer this node has proven dishonest would still spend dials on it whenever the ranking favoured it. The threshold is "share > 0.0", which is not arbitrary -- `dial_share` returns exactly 0.0 for a proven fault and floors non-performance above zero. So the filter excludes precisely the verifiably faulty and can never evict a merely-slow peer, which is what stops induced distress being an eviction primitive. VACUITY, stated rather than implied: only `HonestAnswer` and `NonPerformance` are produced today. `ProvenLie` needs a per-peer verification verdict that `dig-download` owns and does not surface (module_transport.rs:1587), and `SelfContradiction` needs an announce/answer correlation the node does not keep. Both are follow-ups; neither is faked from a transport error, because branding an honest peer on unverifiable evidence is the conflation SPEC 8.2A exists to prevent. The exclusion path is therefore correct, tested, and dormant in production. Co-Authored-By: Claude <noreply@anthropic.com>
MINOR: two new capabilities, both backwards compatible. Inbound peer work is now admitted per authenticated identity before it is performed (#269), and peer conduct gates which peers a forwarded ask will dial (#268). No public API was removed or renamed and no wire format changed; an existing caller sees the same surface. Also drops the WIP marker the lane opened with. Co-Authored-By: Claude <noreply@anthropic.com>
…y the gates (#269) Adds the WIRING assertion the unit tests cannot make: `admission.rs`'s tests prove the meter behaves, not that anything calls it, and a meter nothing calls is the defect #269 describes. The new test drives `NodeResponder::handle_json_rpc` and asserts an unauthenticated session is refused with -32000 rather than the allowlist's -32601 -- which is how we know admission ran FIRST -- with an authenticated control proving the responder is not simply refusing everybody. `node_responder_returns_method_not_found_for_management_methods` now passes a 64-hex conn_key. That is the shape every production session supplies, since both listeners derive it from the verified client leaf and no production path reaches the responder caller-less. Without it the test would answer -32000 and stop exercising the allowlist at all; with it, the property under test is unchanged. Also drops an unused accessor and two redundant u32 conversions for clippy -D warnings. Co-Authored-By: Claude <noreply@anthropic.com>
|
IN PROGRESS — not the verdict. Gate on head Confirmed so far:
Still open in this review: the |
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
CHANGES-REQUIRED
Head reviewed: 825f329ab263b98497a0cc8d3271729f155ed9a4 (resolved from the remote, not from the dispatch brief). PR is DRAFT and was left DRAFT; nothing merged, nothing edited.
Two gating findings, both posted as inline threads. Everything else the lane claimed, I checked and it held — this is a genuinely careful PR and the vacuity disclosure is the right instinct. .gitnexus was not used: the registered dig-node index is stale and impact returns a false-safe zero, so the radius below came from grep + direct read of git objects, stated rather than implied.
Gating
peer.rs:1423—max_request_units(256, dig-sex default) refuses availability batches of 257-512 items that this crate's ownMAX_AVAILABILITY_ITEMS = 512documents and tests as served, truncated. Wire-visible narrowing; the existing 513-item test callsavailability_batchbelow the responder so the suite cannot see it; two doc claims become false. Do not fix by deleting the clamp.peer.rs:1351— the admission gate makesserve_peer_session's own doc (peer.rs:999-1001) false for the JSON-RPC and availability paths on apubAPI, and the PR body cites that same comment as evidence. Behaviour change for an external consumer, not stated in the SemVer section.
Both are cheap. Neither is a design error.
Verified — the lane's claims held
- dig-sex latest is 0.5.0.
index.crates.io/di/g-/dig-sexyields 0.3.0 / 0.4.0 / 0.5.0;Cargo.tomldeclares"0.5". #287's ticket body was stale and the close was justified on that point;dig_sex::acquisition::decidefor #270 is genuinely live atcapsule_store.rs:587. - #269's meter key cannot be influenced by an unauthenticated caller. Every path reaches
admitwithcaller.map(|c| c.peer_id)(peer.rs:1139/1151/1161/1172);callercomes only fromcaller_from_tls(peer.rs:3324-3332, SHA-256 of the verified client leaf SPKI) orrelayed_caller_contactoff the mTLS-verifiedPeerConnection.peer_id(peer.rs:2371). Never a wire field. Refusal is reachable, not theoretical, andAdmissionGuard: Dropreleases on every path including panic. - No lie is synthesised from a transport error. Grepping every
.rsin the crate at this head:ConductEvidence::appears in production at exactly two lines,download.rs:1975and:1978, bothHonestAnswer/NonPerformance.ProvenLieandSelfContradictionappear only inconduct.rstests. The conflation SPEC 8.2A exists to prevent was not committed. - Both revert-proofs hold, checked analytically rather than by rebuild (disk at 99%):
MIN_NON_PERFORMANCE_DIAL_SHARE = 0.1(dig-sexconduct.rs:116/194), so a> 0.5threshold drops theslowcontrol at 0.1 whilequiet(1.0) and theliarexclusion (0.0) are unchanged — exactly one of the eight fails, and it is the control half that catches it. Likewise a constant meter key fails only the two-peer test; the other four never key a second identity. - The rival is reconciled, as claimed —
download.rs:2788-2803documents the three-state hop budget,remaining -> None -> ForwardRefusal::UnreadableHopBudget(dig-node#281), with the redirect/forward asymmetry stated. My brief was stale on this. (The body cites2740-2751; the text is at ~2788-2803.) - Commit subjects: five commits, longest 87 chars. Commitlint's 100-char rule is satisfied.
Non-gating (posted for the record, not blocking)
- The vacuity disclosure is honest but understates itself. "Conduct half fires" is not quite the shape.
dial_sharehas exactly one consumer — the> 0.0filter indialable(conduct.rs:104-110) — and neitherHonestAnswernorNonPerformancecan ever produce0.0, becausedig_sex::dial_sharefloors non-performance at 0.1. So in productiondialableis identicallyroutableand conduct changes no dialling decision at all today; the observation half records, and the effect half is entirely inert. Worth saying that way in the body and on #268, because "half fires" reads as half the behaviour rather than none of it. - The two halves of this PR interact.
download.rs:1978scoresAskOutcome::RefusedasNonPerformance, and this same PR makes refusals more common by shedding load at admission. A node correctly shedding is penalised by its peers. Bounded — floored and decaying, never an exclusion — so not gating, but it is a feedback loop that did not exist before this commit. Refusal::MeterFullis unreachable under the node's defaults, and that is good news.releaseremoves a peer's entry at zero (dig-sexadmission.rs:175-181), soin_flight.len()is bounded byglobal(64) which is far belowmax_tracked_peers(1024). I went looking for a Sybil eviction primitive — fill the tracked map, deny honest peers — and there isn't one. What Sybils can reach isGlobalCeiling(8 identities xper_peer_share8 = 64), which refuses everybody transiently. That is SPEC 8.5's design and writes no durable exclusionary record, so it is not a finding against this PR, but it is the state an attacker aims for and is worth a ticket against the limits rather than the wiring.
The e2e gap: I do not gate on it
The lane flagged that no test drives a forwarded ask end-to-end and asserts conduct changed the peers dialled, and invited disagreement. I agree with the lane. Given the exclusion path is provably inert in production, such a test could today only assert dialable == routable — it would be a vacuous green, and a vacuous green defending a dormant path is worse than a stated gap. It becomes worth writing the moment dig-download surfaces per-peer attribution, and that is the right sequencing.
What I could not reach
- I did not rebuild or run the suite (disk ~28 GB at 99%; a full
-p dig-node-core --librun is 632s per the body). The revert-proofs and the test-count claim are therefore checked by reading, not by execution. The body's1037 passed; 0 filtered outis plausible and correctly guards against the matched-nothing trap, but I did not independently reproduce it. - I did not audit the dig-sex 0.5.0 crate beyond
admission.rsandconduct.rs.
| // ADMISSION (dig-sex SPEC 8.5, #269) — before the items are even read. `items.len()` is the | ||
| // attacker-chosen quantity this request asks for, so it is what gets clamped at the boundary | ||
| // (`AdmissionLimits::max_request_units`); clamping it deeper in would already have paid for it. | ||
| let requested_units = |
There was a problem hiding this comment.
GATING — two limits on the same caller-controlled quantity disagree by 2x, and the tighter one is newly in front.
requested_units = items.len() is clamped here against AdmissionLimits::max_request_units. The node constructs PeerAdmission::default() (lib.rs:4520, and 7 sibling constructors), so that value is dig-sex's default 256.
But this crate's own documented cap on the same quantity is 512: MAX_AVAILABILITY_ITEMS (lib.rs:211), and availability_batch deliberately truncates rather than refusing — let capped = &items[..items.len().min(MAX_AVAILABILITY_ITEMS)] (lib.rs:3946), with lib.rs:10273-10286 asserting a 513-item batch is answered with 512.
Consequence: an availability batch of 257-512 items — which this node's contract says it serves — is now refused outright with -32000 "request too large". That is a wire-visible narrowing on a public peer surface, and it is not mentioned in the PR body.
Three things make it worse than a tuning nit:
- The suite cannot see it.
lib.rs:10273callsavailability_batchdirectly, below the responder, so it never crosses the new gate and stays green. The guard is not exercised by the test that owns the property. - Two doc claims are now false.
peer.rs:855-857says the availability batch is the largest amplification vector "up toMAX_AVAILABILITY_ITEMSlookups per request", andlib.rs:3928says the batch is capped atMAX_AVAILABILITY_ITEMS. The real ceiling reaching the batch is 256. - The two constants are in different crates, so nothing detects the drift when either moves.
The fix must not be to delete the clamp — clamping the attacker-chosen quantity at the boundary is the correct shape and is the point of #269. Make the two agree deliberately: either construct PeerAdmission with max_request_units: MAX_AVAILABILITY_ITEMS so one constant governs, or lower MAX_AVAILABILITY_ITEMS to 256 and update lib.rs:3928, peer.rs:855, and the 10273 test. Whichever is chosen, add a test that drives the batch through the responder at 256/257 (both sides of the bound) so the two limits cannot drift apart again unobserved.
| // early `return`s below, which is why no path here needs to remember to. | ||
| let _admitted = match self | ||
| .node | ||
| .peer_admission() |
There was a problem hiding this comment.
GATING — this admission gate makes a public function's own doc false, and the PR body cites that doc as evidence.
serve_peer_session (peer.rs:995-1003) is pub, passes caller: None, and its comment at peer.rs:999-1001 states:
a caller-less session still serves the JSON-RPC/range/availability paths — only DHT routing-table population needs the caller.
After this commit that is no longer true. None yields an empty conn_key (peer.rs:1151, 1161), authenticated_peer returns None on a non-64-hex key, and both handle_json_rpc and handle_availability return -32000 "unauthenticated" before dispatch. A caller-less session now serves neither of the two paths that sentence promises; only stream_range / stream_module_range remain.
I agree with the refusal itself — the reasoning in admission.rs is right, and I independently confirmed both production listeners supply a verified caller (peer.rs:3304 via caller_from_tls → peer_id_from_leaf_cert_der; peer.rs:2371 via relayed_caller_contact off the mTLS-verified PeerConnection.peer_id). The defect is that the old rule's phrasing survived the change, on a pub item, and the PR body then leans on the same comment ("its own doc concedes the caller is supplied by the listener") as support — reading half of a sentence whose other half the diff just invalidated.
For an external consumer this is a silent behaviour change: code that compiled and served yesterday now returns a JSON-RPC error for every request, with the doc still saying it works. Required:
- Rewrite
peer.rs:999-1001to state the new rule: a caller-less session is refused on the JSON-RPC and availability paths, and this entry point is for listeners that supply a verified caller. - State the change in the PR body's SemVer section. It does not force a major on its own, but "no removed or renamed API and no wire change" is not the whole story — a retained API's behaviour changed, and that belongs in the release note rather than being discovered by a consumer.
- Sweep for the superseded wording rather than spot-checking:
peer.rs:843,852and863each carry "empty for a caller-less/test session" on the trait methods, which now reads as permitted where it is refused for two of the four.
DO NOT MERGE — DRAFT. The gate round has not run.
Wires two of the dig-sex economic-layer policies into the node. Both were implemented, tested and consuming nothing from dig-node.
Scope: three of the five batch tickets were already satisfied or are deferred
The batch named five. Measured against
origin/main@3e480ddbefore building:Cargo.toml:348pinsdig-sex = "0.2"; the file declaresdig-sex = "0.5"atcrates/dig-node-core/Cargo.toml:509.run_roundalready takesheld, populatesresidentfrom it, and is fed a real cache-derived set every round byNodeHeldCapsules::held_content_ids(tier0_live.rs:423). The acceptance bar is asserted as an outcome byan_already_held_candidate_is_not_refetched_while_an_unheld_one_still_is.dig_sex::acquisition::decideis called atseams/capsule/capsule_store.rs:587, with a control-plus-three-refusals test atlib.rs:14217. Left open for the gate to confirm; not rebuilt.dig-sex latest published is 0.5.0, not the 0.4 the tickets name (
index.crates.io/di/g-/dig-sex, with the requiredUser-Agent).Blast radius checked
.gitnexusis stale — the registered dig-node index is ~301 commits behind and would return a false-safeimpactedCount: 0. This radius was therefore established by grep + direct read, and that is stated rather than implied.NodeResponder::handle_json_rpc/handle_availability(peer.rs) — the two entry points gaining an admission gate. 38 call sites of the four responder methods across the crate; exactly one passes an empty conn_key.NodeContent(download.rs) — gainsconduct+conduct_epochfields and a filter betweenroutableanddecide. The singleask_routing.recordcall site is the only writer; conduct is fed from the same place.RoutedPeer(ask_routing.rs) — gainsHashso it can key the conduct map. Additive; no existing behaviour reads it.Node(lib.rs) — gainspeer_admission; all constructors updated.No public API was removed or renamed, and no wire format changed.
VACUITY — which wired clauses actually fire
This is stated explicitly because a conformance list that cannot separate satisfied from vacuously satisfied reports a capability the system does not have.
#269 admission — genuinely fires. Every inbound JSON-RPC and availability request on the peer surface passes the meter, and a refusal is reachable today.
#268 conduct — half fires.
HonestAnswerandNonPerformanceare produced on every forwarded ask.ProvenLieandSelfContradictionare produced nowhere, so the durable-exclusion path — the onedialable()implements — is correct, tested, and dormant in production.The reason is a real architectural boundary, not an oversight: per-chunk verification attribution lives inside
dig-download's engine againstchunk_hashes(module_transport.rs:1587says so in as many words), and the node's ownDownloadErrorsurface offers onlyTransport/Sink/NotDownloadable. A whole-capsule mismatch (module_reshare.rs:1858) is assembled from several holders and names no single peer. Synthesising aProvenLiefrom a transport error would brand an honest peer on unverifiable evidence — precisely the conflation SPEC 8.2A exists to prevent — so it was not done. Surfacing the per-peer verdict needs adig-downloadchange, release-first.Rival check
The batch brief flagged a rival: dig-node coercing an unreadable hop budget to the most permissive value while dig-sex refuses. That disagreement is already reconciled and is not a live finding.
download.rs:2746maps an unreadable forwarding budget toremaining → None, whichdig_sex::discovery::decide_forward(discovery.rs:170) turns intoForwardRefusal::UnreadableHopBudget— both sides now refuse (dig-node#281). The one asymmetry that remains is deliberate and documented: the redirect leg keeps the tolerant reading because it spends only this node's own DHT lookup, while the forwarded ask spends other nodes' bandwidth. Different cost, different tolerance — stated atdownload.rs:2740-2751.Tests
13 new, all passing with real counts (
running 8/running 5— not a filter that matched nothing).Conduct (8):
a_proven_lie_costs_dial_share_while_an_honest_peer_keeps_its_own,a_proven_lie_does_not_decay_with_elapsed_time,non_performance_decays_on_elapsed_time_without_the_peer_being_talked_to,sustained_non_performance_never_silences_a_peer_completely,an_unobserved_peer_starts_neutral_rather_than_penalised,retain_drops_peers_that_left_the_pool,a_proven_liar_leaves_the_dial_set_while_a_merely_slow_peer_stays_in_it,a_self_contradiction_is_durable_like_a_lie_not_transient_like_a_timeout.Admission (5): listed on #269.
Both proven load-bearing by injecting the real defect, committed first:
exhausting_one_peers_share_does_not_refuse_a_different_peerfails (4 passed, 1 failed). The single-peer half alone would have passed it, because a constant-keyed meter also refuses at the limit — it just refuses everybody.> 0.0→> 0.5(above the non-performance floor): onlya_proven_liar_leaves_the_dial_set_while_a_merely_slow_peer_stays_in_itfails (7 passed, 1 failed). The merely-slow control is the assertion that catches it, and it is the one that stops induced distress becoming an eviction primitive.§2.4b dependencies
Every
dig-*andchia-*declaration indig-node-corewas checked against the index and is already at the latest published version — dig-sex 0.5.0, dig-download 0.22.0, dig-dht 0.15.0, dig-nat 0.21.0, dig-peer 0.13.0, dig-peer-selector 0.11.0, dig-tls 0.4.0, dig-identity 0.7.1, dig-constants 0.13.0, dig-rpc-protocol 0.10.2, dig-store-cache 0.1.1, dig-message 0.7.0, dig-keystore 0.13.0, dig-ip 0.1.2, dig-pex 0.1.1; chia-* on the single 0.36 line. No bump was owed, so none was made.SemVer
0.189.0 → 0.190.0(dig-node-core0.64.0 → 0.65.0). MINOR — two new capabilities, backwards compatible, no removed or renamed API and no wire change. Version re-read from disk after the last fetch, not from the commit log.Gate evidence (head
825f329)cargo build -p dig-node-core— RC=0cargo clippy -p dig-node-core --all-targets -- -D warnings— RC=0, zero errorscargo fmt --check— RC=0cargo test -p dig-node-core --lib— 1037 passed; 0 failed; 0 ignored; 0 filtered out, 632s,CARGO_RC=0. Run unpiped to a file and the count read from the file: a filter matching nothing exits 0 and printsrunning 0 tests, and a piped$?is the last command's.0 filtered outis what makes this the whole suite.One regression found and fixed, and why the fix is the right one
Admission runs ahead of the peer allowlist, so
node_responder_returns_method_not_found_for_management_methods— which calledhandle_json_rpcwith an empty conn_key — started getting-32000instead of-32601.Before choosing, I checked whether any production path reaches the responder caller-less, because refusing would otherwise have broken live peer traffic. It does not: both real listeners supply a verified caller (
peer.rs:3306derives it from the client leaf viacaller_from_tls;peer.rs:2373builds a relayed caller). The publicserve_peer_sessiondoes passNone, but has no in-crate caller — only doc references — and its own doc concedes the caller is supplied by the listener.So the refusal is correct, and the test was updated to pass a 64-hex key: the property it guards (an authenticated peer is still merely "some peer_id", never an authorized admin — audit #179) is unchanged, and it now exercises the allowlist rather than stopping at admission.
A gap in my own evidence, stated rather than left implicit
The conduct→ask-loop wiring is proven by compilation and by
conduct.rs's unit tests, but there is no test driving a forwarded ask end-to-end and asserting a peer's conduct changed the peers it dialled. The admission half has that test (the_responder_refuses_an_unauthenticated_session_before_consulting_the_allowlist); the conduct half does not, because the forwarded-ask harness is substantially heavier. Given the exclusion path is dormant anyway (nothing produces a verifiable fault yet), I judged the end-to-end test worth more oncedig-downloadsurfaces attribution. Flagging it so the gate can disagree.