feat(holdings): real-time opcode-222 announce/retract with verified inbound ingest (#1429) - #115
Conversation
29550fb to
8b6db9b
Compare
…nbound ingest Closes #1429. Adds the real-time, authenticated half of the content-replication flywheel. The durable half already existed (DHT provider PUT on capsule gain, find_providers on read, #1576/#1586); it converges only as fast as the last PUT reached, and a departed holder lingers for its record's TTL. This closes that gap. Egress: every inventory reconcile now derives BOTH effects from one delta -- the DHT records (with an ACTIVE retract_own_provider on loss, replacing the passive withdraw_provider that left this node answering find_providers for content it can no longer serve) and a signed opcode-222 HoldingsAnnounce for exactly the ids that changed. Signed by the node's own NodeCert leaf, since the wire derives provider_peer_id from SHA-256(provider_spki). Oversized reconciles split across frames with advancing seq rather than truncate, so a bulk cache-clear cannot drop retracts. Ingress: verify -> reject self-attributed -> reject non-advancing seq -> two token buckets -> ingest_verified_provider / remove_provider_record. The provider identity is always the verified signer, structurally: accept() takes no caller-supplied provider id, so a retract cannot de-list an honest holder. The receive path performs zero egress, so an inbound announce cannot be amplified. Co-Authored-By: Claude <noreply@anthropic.com>
8b6db9b to
eb4d89d
Compare
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Recorded as a COMMENT review: GitHub returns 422 on REQUEST_CHANGES for a self-authored PR (the loop commits under the same identity). The verdict is nonetheless CHANGES-REQUIRED - the four blocking inline threads below must be addressed and resolved before merge.
CHANGES-REQUIRED
Strong PR: the SPEC section is genuinely normative, the real-wire tests are real (live GossipService pair over loopback mTLS, real broadcast/decode, real DhtService, real find_providers — not a shared helper or a sink()), the truthful-control discipline is held, and the two bugs you credit to the wire test are genuinely fixed. Suite reproduced green on my side: 16 + 2 holdings tests, 448 lib tests, cargo fmt --check clean.
Your central claim: VERIFIED
The durable flywheel does already close on 0.63.0. I traced the whole chain on origin/main independently:
download.rs:1008 spawn_capsule_reshare → module_reshare.rs:454 spawn_capsule_warm → warm() → module_reshare.rs:398 promote_into_cache → module_reshare.rs:401 announce_inventory() → peer.rs:849 DhtInventoryAnnouncer → dht.rs:466 refresh_inventory → sync_inventory → announce_provider, and dht.rs:472 locate_providers → find_providers. Production wiring confirmed at peer.rs:2274 wire_capsule_reshare (not test-only). The second hook (capsule_store.rs:230 refresh_dht_inventory) covers the gap-fill / explicit-cache paths. So read → cache → announce → find_providers mechanically closes today, gated on ReadOrigin::Local + backfill_on_miss_enabled(). Your reframe of #1429 as speed and authentication is correct, and I confirm the two smaller corrections too: peer.rs:568 list_inventory already serves dig.listInventory, and lib.rs:1145 evict_if_needed operates on responses_dir() — the response-window cache — so capsule LRU genuinely does not exist and hooking retract to the inventory-reconcile chokepoint is the right seam.
But three of your seven guards do not hold over their class
The guard-deletion probe was run against inputs that are all same-case, so it proved the guards load-bearing without proving them sound. verify_holdings_announce decodes provider_peer_id with hex::decode (dig-gossip holdings_announce.rs:353), which is case-insensitive, and the signing message is computed over the decoded bytes — so an attacker can uppercase the hex of a captured announce and it still verifies with the original signature, no re-signing. Both G2 and G3 then compare/key on the raw wire string.
I confirmed all three empirically with a throwaway probe against this head (since deleted, not pushed):
PROBE A uppercase provider_peer_id STILL VERIFIES -> pass (the enabler)
PROBE B replayed older ADD, hex uppercased -> Ok(ingested: 1)
retracted record RESURRECTED as 10e5e64c... (canonical lowercase!)
PROBE C our OWN announce, hex uppercased -> Ok(ingested: 1) G2 bypassed
PROBE D 50 REJECTED announces -> providers tracked = 51 unbounded
PROBE B is the sharp one: ProviderRecord::new (dig-dht record.rs:303) canonicalises to lowercase, so the resurrected record lands under the victim's real peer id — the exact outcome a_replayed_older_seq_cannot_resurrect_a_retracted_record exists to forbid. That test passes only because its fixture never varies the case.
Details inline. Four blocking findings, four non-gating (which I will resolve myself so they cannot bar merge).
Not blocking / verified
- SemVer MINOR is defensible —
dig-node-coreis not on crates.io (confirmed via the API), both consumers are in-workspace and migrated, opcode 222 is additive. Both manifests agree. PutOutcomefix confirmed atholdings.rs:224-229; the wall-clock / pinned-NOWsplit is real and documented at the fixture (holdings_wire.rs:37-51) rather than accidental.- Fixture sizes are taken from
HOLDINGS_MAX_CHANGESand2x+1, not round numbers. Good. - §2.5:
holdings.rsreads cleanly and is genuinely cohesive (egress / ingress / composition), guard clauses throughout, doc-comments on every public item, WHY-comments that earn their place. No complaint. - No regression to 0.63.0's framing fix — full 448-test lib suite green.
What I did not examine
I did not run cargo llvm-cov (the 86.26% / 83.91% / 44.69% figures are unverified by me), did not review the Cargo.lock delta beyond the version lines, and did not exercise run_holdings_ingest or the degraded bring-up branches (see finding 4).
…ation-free rejections Co-Authored-By: Claude <noreply@anthropic.com>
…verified Co-Authored-By: Claude <noreply@anthropic.com>
…ed record Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…test additions Co-Authored-By: Claude <noreply@anthropic.com>
…bility A node that cannot mint a P-256 holdings signer takes the `holdings: None` branch, which the module documents as staying discoverable through the durable DHT provider records alone. Both halves sit behind ONE `if let (Some(..), false)` condition, so that promise was compile-checked only: a mistake letting the missing signer short-circuit the reconcile too would make an unsigning node invisible to find_providers - a discovery outage, not a freshness one. Asserts the SURVIVING half positively (the record really is in the provider store) in both the gain and loss directions, because asserting only that nothing was flooded passes identically whether or not the reconcile ran. Refs #1429
There was a problem hiding this comment.
Re-gate (correctness + test-vacuity leg) — VERDICT: CHANGES-REQUIRED (one blocking finding; posted as a comment review because request-changes on the loop identity is a 422)
I re-executed the lane's probe table in a throwaway worktree rather than reading the diff. All four claimed rows reproduce, and I added a fifth probe the lane did not claim (wrong-victim eviction), which also reds — so the_provider_map_evicts_the_least_recently_seen_at_its_capacity pins placement AND victim, not just a count.
| Probe | Mutation | Observed |
|---|---|---|
| 1 | dht.rs:437 retract_own_provider -> withdraw_provider |
RED — losing_a_capsule_stops_this_node_being_returned_as_a_provider_at_once at holdings_wire.rs:499 on the stated assertion (record still returned by find_providers), plus a_node_that_cannot_sign_still_reconciles_its_records_without_flooding at :621 |
| 2 | delete evict_lru(&mut self.providers, ...) |
RED — the_provider_map_evicts_the_least_recently_seen_at_its_capacity, left: 4 right: 3 |
| 2b (mine) | evict_lru min_by -> max_by (evict the WRONG victim) |
RED — at :1192, "the LEAST-recently-seen provider is the one evicted" — the test discriminates victim selection, not only cardinality |
| 3 | flood omits the retracted ids (SUBSET) | RED — a_reconcile_floods_exactly_the_deltas_it_moved at :573, removes {} vs expected 2 |
| 3b | lost ids flooded as adds (SUPERSET) | RED — same test at :569, adds 4 vs expected 2 |
Both prior blocking findings are therefore genuinely discharged.
Independent guard probes (not claimed by the lane)
| Guard | Mutation | Observed |
|---|---|---|
Gate 3 self-attribution (holdings.rs) |
condition -> false |
RED (2 tests) |
Gate 4 freshness abs_diff |
short-circuited | RED (3 tests) |
| Gate 2 canonicalization | provider_hex = raw wire spelling |
RED (3 tests) |
| Gate 5 seq watermark | seq <= highest -> false && |
RED (4 tests) |
bounded_dht_addresses .take(MAX_ADDRESSES_PER_RECORD) |
deleted | RED (unit test) |
evict_lru(&mut self.senders, ...) |
deleted | GREEN — 32/32 still pass |
The last row is the one blocking finding: the sibling of the exact defect round 1 was sent back for. Detail inline.
Everything else clears: version bump 0.63.0 -> 0.64.0 / dig-node-core 0.21.0 -> 0.22.0 is correct minor for additive capability; SPEC.md §19.3a is normative and matches the code; the reject path IS now exercised (rejected_announcements_allocate_no_tracking_state, and the stale frame embedded mid-fixture in the capacity test); §2.5 readable-code is well above bar — the rustdoc states the WHY for each guard and each doc claim I spot-checked is true of the code. dig-constants: nothing here redefines a cross-repo value locally (HOLDINGS_MAX_CHANGES, MAX_ADDRESSES_PER_RECORD, the opcode-222 framing all come from dig-gossip/dig-dht); the receiver-policy numbers are documented in SPEC.md, which is their right home.
SPEC.md 19.3a states the ingress is "capacity-bounded (1,024 senders, 8,192 providers) with least-recently-seen eviction". The provider half was proven; the sender half was an untested claim -- deleting evict_lru(&mut self.senders, ..) left the whole suite green. The bound is load-bearing: a peer_id is SHA-256(NodeCert SPKI), so it is self-minted, and no path other than eviction ever removes a sender entry. Connect -> announce -> disconnect churn therefore offers an unbounded key space. The new test mirrors the provider-side fixture disciplines. It drives a lowered tracked_senders cap, exhausts each sender's delta budget, and reads eviction through its CONSEQUENCE -- a sender entry IS that sender's budget, so losing the entry lets an exhausted sender relay again. The same relay is asserted refused before the bound is crossed (making the later admission attributable), and a RETAINED sender is probed before the evicted one. It fails under both nearest-wrong implementations: deleting the eviction call, and evicting the wrong victim (min_by -> max_by). Two doc corrections alongside: MAX_TRACKED_SENDERS is restated as a hard bound on unbounded growth rather than "headroom" over a connected pool, and ADVERTISED_TTL_SECS now names dig_dht::DhtConfig::provider_ttl as the symbol it must track. Refs #1429 Co-Authored-By: Claude <noreply@anthropic.com>
Closes DIG-Network/dig_ecosystem#1429. Refs #1394, #1423, #1424, #1428, #1425.
What this is, and what it is NOT
The flywheel's durable half already shipped and I did not rebuild it:
seams/dig_peer/dht.rsalready reconciles DHT provider records on every inventory change (#1586) andmodule_reshare.rsalready pulls a whole capsule after a remote read and announces (#1576), soread -> cache -> announce -> find_providersmechanically closes today. What it lacks is speed and authentication: a new holder is only as discoverable as the last PUT reached, and a departed holder lingers for its record's TTL.This PR adds the real-time, authenticated half — opcode-222
HoldingsAnnounceegress/ingress plus an ACTIVE retract.Ticket corrections
status:blocked; it was not. Both blockers had landed. dig-dht 0.7 (already the pinned dep) carries all four #1424 APIs (ingest_verified_provider,remove_provider_record,retract_own_provider,holders_of), and the pinned dig-gossip revb065e70(v0.16.0) already containssrc/service/holdings_announce.rsbyte-identical to itsorigin/main. No lower-level crate needed to ship and no dep was bumped — no release-first cascade.dig.listInventory(peer.rs::list_inventory). Not rebuilt.Node::evict_if_neededis the response-window cache, not the capsule cache; capsule loss today iscache_remove_cachedor an inventory shrink. Retract is therefore hooked to the real capsule-loss signal (the single inventory-reconcile chokepoint), so it picks up LRU eviction for free when #1423 lands. Restart-durability of theseqcounter is deliberately left to #1477 (noted in SPEC and in the code); the loop closes without it.Blast radius checked
gitnexus is disabled per the standing CLAUDE.md override, so this was done with ripgrep plus direct code reads.
dht::sync_inventoryInventoryDeltainstead of(usize, usize); loss usesretract_own_providernotwithdraw_providerDhtHandle::refresh_inventory. No test referenced it.DhtHandle::refresh_inventoryreconcile_inventorypeer.rsDhtInventoryAnnouncerplus the inventory-refresher hook (both migrated)bring_up_dht(handle, Option<HoldingsBroadcaster>)DhtInventoryAnnouncerholdingsfield; body routes through the sharedreconcile_and_floodOne behaviour change worth reviewer attention (MEDIUM, intentional): capsule loss now ACTIVELY retracts the local provider record instead of letting it lapse via TTL. Rationale: a passively-withdrawn record keeps answering
find_providerswith this node for content it can no longer serve, costing every reader a wasted dial. This is the local half of #1423's atomic evict-and-retract. Wholedig-node-coresuite green (448 unit plus all integration), workspacecargo check --all-targetsclean.Security answers
What does an announce cost the announcer vs. a victim? The announcer pays an ECDSA-P256 signature and a flood. A receiver pays
verify_holdings_announce(one SHA-256 plus one P-256 verify) plus bounded local map work. The ingress performs ZERO egress — it never re-broadcasts, dials, probes or fetches, so there is no path by which one cheap inbound message makes other peers do more work than the sender did. Dissemination is dig-gossip's existing Plumtree flood with its own seen-set dedup, not a per-message re-send added here. This is deliberately unlike the #1670 shape that turned one unauthenticated record into 3x egress.Can a peer announce holdings it does not have, and what does a reader waste? Yes, and this is bounded by cost asymmetry rather than prevented — the honest design. A false claim buys at most one failed dial or a
dig.getAvailabilityanswering "no", after which the peer-selector deranks the liar. No merkle- or chain-verification decision ever rests on an announcement; the record is a hint, and the read path's anchor gate is unchanged. The liar paid a signature plus a flood for one wasted dial.Can retract de-list an honest holder? No, and it holds on the wire, not just in a struct:
HoldingsDelta::Removecarries only acontent_key— there is no per-delta peer field to abuse. The only provider id the receive path can produce isannounce.provider_peer_id, whichverify_holdings_announceproves equalsSHA-256(provider_spki)and which signed the batch.HoldingsIngress::accepttakes no caller-supplied provider id at all, so this is enforced by the type signature rather than by a check that could be moved or dropped. Pinned bya_retract_cannot_delist_another_peers_record, whose fixture keeps a truthful CONTROL holder of the same content key that must survive — a single-holder fixture would pass under a "wipe the whole key" implementation and prove nothing — and by the real-wirea_signed_retract_crosses_the_real_wire_and_spares_the_other_holder.Rate/quantity bounds, as numbers, at one chokepoint (
IngressState::admit, the only place an announcement is charged):4 x HOLDINGS_MAX_CHANGES)The two buckets are keyed differently on purpose, and each covers the other's weakness. The provider bucket is semantically right but its key space is attacker-minted, so its map must be capacity-bounded — and an attacker who overflows it can evict its own bucket and refill, making it bypassable alone. The sender bucket is coarse but its key space is the connected pool, which cannot be inflated from off-network, so it is the unbypassable backstop. A rejected announcement charges neither bucket, or the limiter itself becomes the denial of service (one bad signature per honest announce would silence a neighbour).
I caught and fixed one vacuous bound during design: my first draft set the delta cap to
10 x MAX_CHANGES, which is exactly what the announce bucket already permits — the guard would have executed and constrained nothing.Guard-deletion probe — every guard proved load-bearing
Each guard was deleted individually (committed first, restored from a file copy) and the suite re-run:
verify_holdings_announcea_forged_signature...,an_announce_claiming_another_peers_id...,rejected_announcements_do_not_consume_the_budget)our_own_announce_replayed_back_at_us_is_ignored)seqa_replayed_older_seq_cannot_resurrect_a_retracted_record)the_per_provider_announce_budget_binds_at_its_stated_bound)the_per_sender_delta_budget_binds_independently_of_the_provider_budget)a_retract_cannot_delist_another_peers_record,a_replayed_older_seq...)rejected_announcements_do_not_consume_the_budgetplus 2)Also probed the real-wire test itself: signing with a cert other than the one the gossip handshake presents makes
a_signed_announcement_crosses_the_real_wire_and_becomes_discoverablefail on the peer_id assertion — so it genuinely binds announce identity to handshake identity rather than checking self-consistency.cargo-mutantswas not run (not installed).Two real bugs the real-wire test caught that the unit tests could not
DhtServicereads the SYSTEM clock to clamp and expire provider records. The first draft threaded a pinned pastNOWthrough it, so every ingested record was already expired andfind_providersreturned[]. A pinned-NOWwire test would have "proved" the flywheel closed while exercising only the expired path. The wire test now uses real wall-clock with the reason documented at the fixture; the pure-policy suite keeps its pinnedNOWbecause its API is clock-injected.PutOutcome.HoldingsSink::ingestdiscarded dig-dht's admission outcome, soApplied::ingestedcounted attempts. It now returnsbooland counts onlyPutOutcome::Accepted— which is what surfaced bug 1 at all.Tests
tests/holdings_ingress.rs— 16 adversarial tests. Fixtures built against the nearest WRONG implementation, with a truthful control actor in every censorship/flood test, an explicit pinnedNOW, sizes taken from the protocol limits (HOLDINGS_MAX_CHANGES,2x+1), and every numeric bound pinned from both sides (at-bound passes, one-over fails).tests/holdings_wire.rs— 2 REAL-WIRE tests: two livedig-gossipservices over loopback mTLS, each presenting its own persisteddig_tls::NodeCert; real signature by the TLS leaf; realframe_holdings_announce-> transmit ->holdings_announce_payload-> verify -> realDhtService-> realfind_providers/holders_of.dig-node-coresuite: 448 unit plus all integration green.cargo fmt --checkclean,cargo clippyzero warnings.Version bump — MINOR
dig-node0.63.0 -> 0.64.0,dig-node-core0.21.0 -> 0.22.0. New backwards-compatible capability; no wire break (opcode 222 is additive, and a peer that does not send it is simply not real-time-discoverable, falling back to the durable DHT records).dig-node-core's minor covers the changedpubsignature ofsync_inventory— the crate is an internal library shared by the two host shells, and both shells are in this workspace and migrated here.SPEC.md19.3a records the normative contract (egress MUSTs, the five ingress gates, the attribution invariant, the no-amplification property, and the bounded false-claim cost).