From 084505dea003e7a5a8da46685a2693b7a9686dc0 Mon Sep 17 00:00:00 2001 From: grumbach Date: Tue, 25 Aug 2026 13:23:52 +0900 Subject: [PATCH 1/3] feat(replication): stop penalising a peer for not holding a close-group chunk The chunk store can only grow. LMDB returns a deleted page to its own free list and never to the filesystem, so a node that deletes chunks frees no disk. Moving the fleet onto a store that does return space means a node short of disk will have to give up some chunks while it moves the rest across. It cannot avoid being seen doing that, and it cannot stop the consequence, because the penalty is the auditor's decision, not the audited node's. So the auditors stop one release ahead of the migration, and this is that release. What is withheld is deliberately narrow: only the accusation "you did not have a chunk you were supposed to be holding". That covers the responsible-chunk audit, the fresh-replication possession check, the prune audit, a sole-source replica hint whose sender then denies possession, and the fetch paths where a peer that answered Present could not serve the bytes. A node giving up chunks produces every one of those, so withholding some and not others would stop only some of its accusers. The commitment-bound subtree audit is untouched and still penalises. That is not a compromise, it is what makes the rest work: a migrating node reduces its signed commitment precisely so its peers hold it to the smaller claim, and suspending that enforcement would make the reduction meaningless. A sole-source hint the close group rejects outright is also still punished, because that is a claim about a key that does not exist rather than about the sender's own storage. Audits of both kinds keep running and keep recording. Only the trust event is withheld, and the record they leave is how we will know when it is safe to switch the penalty back on, which is a later release rather than a compiled-in expiry so the date can move on evidence. The switch is a build constant, not a configuration field: a node writes its effective configuration back to disk, so shipping it as a setting would bake this release's value into every operator's file and the next release would change nothing. It is initialised from that constant rather than defaulting to "penalise", so a construction path that never applies the policy behaves like this release instead of the previous one. Known cost, accepted: between this release and the one that restores the penalty, a peer that publishes no commitment at all can answer Present, fail to serve, and pay nothing for it. It is bounded by the restore and visible in the audit record. See ADR-0012. --- ...e-based-chunk-store-and-lmdb-retirement.md | 398 ++++++++++++++++++ src/node.rs | 6 + src/replication/config.rs | 173 ++++++++ src/replication/mod.rs | 132 ++++-- src/replication/possession.rs | 30 +- src/replication/pruning.rs | 13 +- tests/e2e/replication.rs | 25 +- 7 files changed, 718 insertions(+), 59 deletions(-) create mode 100644 docs/adr/ADR-0012-file-based-chunk-store-and-lmdb-retirement.md diff --git a/docs/adr/ADR-0012-file-based-chunk-store-and-lmdb-retirement.md b/docs/adr/ADR-0012-file-based-chunk-store-and-lmdb-retirement.md new file mode 100644 index 00000000..7dcd5077 --- /dev/null +++ b/docs/adr/ADR-0012-file-based-chunk-store-and-lmdb-retirement.md @@ -0,0 +1,398 @@ +# ADR-0012: One File Per Chunk, and Retiring LMDB Without Losing Data + +- **Status:** Proposed +- **Date:** 2026-08-25 +- **Decision owners:** Anselme Gaeremynck +- **Reviewers:** David Irvine, Chris O'Neil, Mick van der Most van Spijk +- **Supersedes:** none +- **Superseded by:** none +- **Related:** ADR-0002 (gossip-triggered subtree audit), ADR-0003 (possession checks), + ADR-0004 (commitment-bound quote pricing), ADR-0007 (Windows LMDB map headroom cap, + retired by this decision) + +## Context + +The node stores chunks in LMDB. LMDB returns a deleted page to its own free list and never +to the filesystem, so **deleting chunks does not free disk**. In one week the fleet deleted +2.29 million chunks and got back zero bytes. Operators read that as a bug and are tempted +to wipe node directories to reclaim space, which costs the network real replicas. + +There is no partial way out. Compaction needs free space equal to the live data, which is +exactly the condition a full node does not meet, and it does not get us off LMDB anyway. +Punching holes in `data.mdb` is Linux-only, needs LMDB internals to identify free pages, +and reads back as zeros. Disk comes back exactly once: when `chunks.mdb` is removed whole. + + peak disk during migration = allocated chunks.mdb (unchanged) + files written so far + +So a local migration is possible if and only if `free >= live payload`. On production +volumes today (55 volumes at 492 GiB, free median 25.8 GiB, p10 9.8 GiB, about 12 nodes +per volume, about 38 GiB of LMDB per node of which about 24 GiB is live) migrating one +node costs 24 GiB and returns 38 GiB. One at a time the host gains about 14 GiB per node +and the queue accelerates. All twelve at once need 288 GiB and all twelve stall. + +The chunk workload is the easiest possible case for a filesystem: content-addressed, +immutable, write once, read many, delete whole, and **4 MiB**, confirmed by the team +rather than assumed. That size is what makes one file per chunk the right shape; see the +Storj and borgbackup note under Validation for what would change the answer. + +## Decision Drivers + +- Deleting a chunk must return its blocks to the filesystem, on a full disk, with no free + space required and no compaction to schedule. +- No chunk may lose its last replica, including during a fleet rollback, a skipped + upgrade, or a crash halfway through the migration. +- Mass audit failures are as damaging as data loss. Nothing here may cause them. +- It has to work for every operator, not for our fleet. Most node operators are not us and + cannot be told to attach a second volume. +- No opt-in. Whatever we ship is what every node does by default. + +## Considered Options + +1. **Stay on LMDB and compact.** Needs free space equal to the live data, which is the + condition we are trying to escape, and leaves us on LMDB. +2. **Append-only packs** (borg segments, Storj hashstore). Reintroduces compaction, a free + list, and a cross-file index. That is LMDB's disease with a different allocator. +3. **Fixed-size slots** (Sia `hostd`, Swarm sharky). Cheaper than log packing, and Sia's + sector size is exactly our 4 MiB. But a freed slot returns space to the *store*, never + to the *filesystem*: the volume file never shrinks. It is the right design once a node + has a declared capacity, and the wrong one while our whole complaint is that disks stay + full as chunk counts drop. +4. **One file per chunk, sharded on the address prefix.** Broken for us, see below. +5. **One file per chunk, sharded on the address suffix.** Chosen. + +## Decision + +### The store + +One immutable file per chunk: + +```text +{root}/chunks/layout.json versioned layout marker +{root}/chunks//<64-hex> xy = the LAST two hex characters of the address +``` + +**Suffix, never prefix.** A node holds keys it is among the `CLOSE_GROUP_SIZE` closest to, +so its holdings share roughly `log2(N / 7)` leading bits with its own node ID, and that +shared prefix grows as the network grows. Distinct directories a single node would actually +use, sharding on the first hex characters: + +| nodes | shared bits | 2 hex | 3 hex | 4 hex | +|---:|---:|---:|---:|---:| +| 1,000 | 7.2 | 1.8 | 29 | 459 | +| 10,000 | 10.5 | 1 | 2.9 | 46 | +| 100,000 | 13.8 | 1 | 1 | 4.6 | +| 1,000,000 | 17.1 | 1 | 1 | 1 | + +At today's ~800 nodes a two-hex prefix is already down to about two directories. Prefix +sharding does not degrade, it fails, and it fails later for the nodes that grow into it. +Close-group membership constrains the leading bits and places no constraint at all on the +trailing ones, and the address is a BLAKE3 output, so the last byte is uniform by +construction at every network size. IPFS shipped the same fix for a different reason: its +prefixes were constant because of the CID encoding, not because of clustering, and the +flatfs `_README` still says *"Previously, we used prefixes, we now use the next-to-last two +characters."* The generalisation is the part worth keeping: **shard on bits you can prove +are uniform, not on bits that happen to be uniform today.** + +**256 shards, one level.** 23 files per directory at today's ~6,000 chunks per node, 977 at +a 1 TiB node, 39,000 at 10 TiB, for 1 MiB of directory inodes. 4,096 shards only starts to +pay past several million chunks and costs sixteen times the directory overhead for every +node that is not that large. + +**Lowercase hex filenames, full 64 characters.** NTFS and default APFS fold case, so under +base64url or base58 two distinct keys can share one case-folded filename, which is a silent +overwrite. No hex string can spell `CON`, `NUL`, `AUX`, `COM1` or `LPT1`, because none of +those letters is in `0-9a-f`. Keeping the whole key in the name means a `find` over the tree +recovers the store even if the directory layer is lost. + +**The scheme is recorded in `layout.json` at creation.** Nobody in this survey shipped an +in-place re-sharder and all of them paid for it: IPFS says export and re-import, Storj ran a +multi-year satellite-controlled backend migration, borg rewrites only on the next +compaction. One small file is the difference between changing the default later and never +being able to. + +### The index + +**The filesystem is the sole authority.** The key set is a `BTreeSet` rebuilt at +every open by reading directory entries, names only: no `stat`, no content read. A `stat` +per entry costs about ten times the enumeration on Linux and macOS and fifty to sixty times +on Windows, and buys nothing, because the filename is the key. + +No sidecar database, because a persistent index **cannot remove reconciliation**. Commit +the index first and a crash leaves a phantom key; rename the file first and a crash leaves +an unindexed file. Repairing either means looking at the filesystem anyway, so the +filesystem may as well be the authority, and then nothing can drift. Ceph FileStore's +tracker #17177 is the cautionary tale: a crash between `unlink` and the LevelDB flush +orphaned omap keys that were silently reattached to a different object later. + +`BTreeSet` rather than a hash set for three reasons: `all_keys()` must be sorted (the +commitment builder truncates the responsible subset with `take(cap)` *before* the Merkle +tree sorts it, so an unstable order would make the published commitment depend on iteration +luck), it never spikes memory while growing, and bulk-building it from a sorted vector packs +every node to capacity where repeated insertion converges on 68% fill for the same keys. + +**One process per data directory, enforced.** LMDB was genuinely multi-process safe. This +store is not: two of them keep independent in-memory indices, so both would report the same +write as newly stored and each would keep serving keys the other had deleted. A node whose +store is already held by another process refuses to start and says so. + +**Every in-memory mutation mirrors a filesystem operation that has already completed**, and +never anticipates one. Bitcask's issue #114 is what the opposite order looks like: an index +rebuilt at startup and then mutated in place drifted to 2,400 keys pointing at fewer than +100 files. + +### Durability + +Write: reserve capacity, create a temp in the **destination** directory, write, flush the +file, rename, flush the shard directory, then admit the key. The publish is an +intra-directory rename, so it is atomic on every filesystem we support and only that one +directory needs flushing. The final name can never appear on partial content, because the +name is the hash. Delete: unlink, flush the shard directory, then drop the key. + +Per platform, honestly: + +| | rename atomic | fsync(temp) + rename durable | directory fsync | +|---|---|---|---| +| ext4 | yes | **no**, `auto_da_alloc` only orders data before the rename's commit | yes, required | +| XFS | yes | not by that sequence | yes | +| btrfs | yes | **uncertain**, ALICE found reordering | yes | +| APFS | yes | `sync_all` already uses `F_FULLFSYNC` on Apple targets | returns 0, effect undocumented | +| NTFS | **not documented as atomic** | unknown | **no documented way** | + +On Windows a node cannot make the rename durable through the standard library at all. The +content is content-addressed and re-replicable, so the position we take is: accept it, +detect a missing or corrupt file on read, repair from the network, and **refuse to delete +the legacy environment on Windows** unless an operator explicitly overrides after +power-loss testing. + +### Retiring LMDB + +Three releases, because slashing is the *auditor's* decision. A node that has to give up +chunks cannot stop its auditors from penalising it, so the auditors have to stop first. + +| Release | Penalise a peer for not holding a close-group chunk? | Delete `chunks.mdb`? | +|---|---|---| +| **First**: stop one penalty | no | no | +| **Second**: migrate | no | yes | +| **Third**: restore it | yes | yes | + +What the first release withholds is deliberately narrow: only the penalty for **not holding a close-group +chunk you were supposed to be holding**. The commitment-bound subtree audit still +penalises, in every release. That is not a compromise, it is what makes the rest work: a +node reduces its commitment precisely so its peers hold it to the smaller claim, and +suspending that enforcement would make the reduction meaningless. Audits of both kinds run +and record throughout. + +Both are **build constants with environment overrides, never serialised config**. A node +writes its effective configuration back to disk, so shipping them as ordinary fields would +bake the first release's values into every operator's file and the next would change nothing. + +Per node, in order: + +1. **Open both stores.** Reads are the union, writes go to files. New chunks are also + written to LMDB **first** while it exists: a chunk uploaded during the bridge to holders + that all revert to a pre-migration build would otherwise be gone from every one of them, + and that is client data, not a replica. +2. **Copy closest first**, throttled, stopping at a slack floor above the disk reserve. +3. **Settle.** The node commits only to its file-backed keys from here, while still serving + everything it ever committed to. Serving reads the union; the commitment reads the + file-backed set. A node is at worst over-honest. Nothing is deleted at this step: it + only narrows the claim, so the close group can learn the new one before anything goes. +4. **Verify.** Every chunk both stores hold is re-hashed and recopied from LMDB on + mismatch. A filename is not proof the bytes behind it are good, and the startup scan + reads names only. +5. **Retire.** Once the retirement delay has elapsed, at least two commitment rebuilds have + been published, and no key the node is giving up is still answerable under a retained + commitment slot: rename `chunks.mdb` aside, flush the parent, record the node as + file-only, and only then delete it. The rename is what makes the state change atomic, + because `remove_dir_all` is not: a failure partway through leaves a directory that can + no longer be opened as an environment, and recording completion on top of that would + have the node claim it had finished over a half-deleted store. **This is where the disk + comes back.** Every gate is rechecked inside the destructive step itself, in the same + critical section that proves no other task holds the store, because the verification + pass alone can run for hours and a write whose file half failed adds a key in the + meantime. +6. **Refetch** the shortfall through ordinary replication, with the freed space to do it in. + +The delete gate is the pruner's existing retention contract +(`ResponderCommitmentState::is_held`, `GOSSIP_ANSWERABILITY_TTL` three hours). No new +protocol. + +**Nothing is given up without proof it exists elsewhere.** Only nodes that cannot fit +their payload give up anything at all, and such a node must clear three gates, in this +order, before a byte is deleted: + +1. **It is not near the front of the group for the chunk.** Only the last two positions of + the *admission group* (`storage_admission_width`, the close group plus its margin) are + eligible, which is the width the pruner treats as strictly in-range and refuses to + delete inside. A one-off migration must not be more willing to drop a chunk than the + thing that runs every day. +2. **Its close group has received the reduced commitment.** The node narrows what it claims + first, and only once peers have demonstrably received that narrower claim, proven by + them answering a neighbour sync that carried it, may anything be deleted. Until then + they audit it against the set it used to hold, and a wave of audit failures is as + damaging as losing the chunks. +3. **Other nodes have proven they hold the chunk, and are currently publishing a claim.** + All but one of its current close group must answer a cryptographic possession challenge + over a nonce they have never seen. This is the pruner's own evidence, reused + deliberately, and it is deliberately not the cheap `VerificationRequest`: that carries a + self-reported `present: bool`, and a node that has silently lost a chunk still answers + yes. A peer only counts if this node has also heard a commitment from it recently, which + excludes a peer sitting between a retired commitment and its next rotation. That gap is + exactly what a node in the middle of its own migration looks like, and counting it would + let two migrating nodes each conclude the other was covering the chunk. + +Rank alone would not do. Being far from a chunk says something about who *should* hold it, +not about who *does*, and in a fleet-wide migration the nodes that should hold it are +exactly the ones that may also be short of space. Without gate 3 the safety property is +merely statistical: every holder could be short at once and each drop the same chunk, and a +per-volume lock cannot see that, because it serialises one volume and this is a +network-wide question. + +A node that cannot clear these gates keeps both stores, does not free its disk, and tells +the operator to add storage. That is the correct answer, not a smaller replica count. + +Gates 2 and 3 are re-checked immediately before the environment is removed, not once when +the node settled hours earlier. The group moves, and two paths can put a key back into the +legacy-only set in between: a file that failed verification and is now being served from +the legacy copy, and a write whose file half failed. + +**Two of a close group at a time, not seven.** The gates above are per chunk, and they are +safe, but on their own they deadlock: if every holder migrates at once, none can prove to +the others that a copy survives and the whole group sits waiting. So each node derives a +migration wave from a hash of its own ID, and a group of seven is split into four waves. +Wave `w` opens `w * wave_hours` after the build first starts. It needs no coordination and +no protocol change, which matters because a node cannot usefully ask its close group "are +you migrating?" and would not trust the answer by the time it arrived. + +It is a stagger, not a guarantee: seven IDs hashed into four waves will not always land two, +two, two, one. What makes it safe rather than merely tidy is that it composes with the +possession gate. A node whose turn has come still cannot give a chunk up until its +neighbours prove they hold it, so an unlucky wave waits instead of over-shedding. Only nodes +that have to give something up wait for a wave; a node with room copies and retires +immediately, because it is never unable to serve. + +Separately, a host-wide advisory lock serialises migrations sharing a volume, held from the +first copy through retirement, so a node cannot release it and let eleven others start +before it has returned a byte. The two limits answer different questions: the lock is about +one machine's disk, the wave is about one chunk's replicas. + +## Consequences + +### Positive + +- `unlink` returns blocks immediately. No free list, no compaction, no free space required + to reclaim space. This is the entire point. +- `exists()` and `current_chunks()` become in-memory lookups with no syscall, cheaper than + the LMDB reads they replace. +- `all_keys()` gains a stable ascending order, which the commitment builder needs and the + pruning cursor wants. +- A fresh node never opens a memory map at all. `storage.db_size_gb` and ADR-0007's Windows + map headroom cap die with LMDB. +- The store is self-describing: the filename is the hash, so an operator can verify a chunk + with `b3sum`, and a scrambled directory layer is recoverable with `find`. + +### Negative / Trade-offs + +- **There is no rollback once a node has deleted its LMDB.** The staged rollout is the only + control: a small leading batch, ours, and a wide window. +- **The window between the first and third releases is publicly known, and in it nobody is + penalised for failing to hold a close-group chunk.** The cheapest way to exploit it is + precise and worth writing down: a modified peer that never gossips a commitment at all is + credited as a legacy node, can answer `Present`, and can then return `NotFound` or fail a + possession check with no trust cost. It pays only for an identity and the traffic. One + such identity removes one of seven replicas; control of all seven positions removes the + chunk's availability. The commitment-bound audit is untouched, so this only works for a + peer that publishes no commitment at all, which is itself visible. The mitigation is not + a code change, it is not letting the third release slip. + It is bounded, because the third release evicts afterwards, and audits keep recording so we + can see it happening, but it is a real invitation for the duration. +- `exists()` is now an index lookup rather than a read of the backing store, so something + outside the node deleting files is not noticed until the next read of that key. The read + path self-heals, and a `stat` per call on the node's hottest path is not worth it. +- One inode and one directory entry per chunk. At 4 MiB per object that is 0.05% overhead + and block rounding for a full chunk is exactly zero, but it is real. +- Windows retirement is off by default, so Windows nodes keep both stores until we can test + power loss on NTFS. +- The paid list is still LMDB. It is a fixed 256 MiB map that contributes nothing to the + disk problem, but it is why `heed` cannot be dropped yet. +- **Narrowing the commitment cuts the quoted price.** Price is quadratic in the committed + key count, so a node that has just proved it is short of disk advertises a cheaper quote + than its close-group peers and then refuses the store on capacity. A wasted round trip + rather than a mispayment. The fix belongs to the quote path and is a separate decision. +- **A cancelled awaiter drops the per-key lock while its blocking write runs on.** The two + consequences are bounded: a pruned chunk can be re-created, which the pruner deletes + again, and a cancelled write can leave an orphan in the legacy store, which retirement + removes and whose client was never acknowledged. + +### Neutral / Operational + +- Startup cost is the directory scan: 122 ms warm and 1.55 s cold at 250,000 files across + 256 shards on APFS, of which the index build is 2 to 11 ms. No fast-start snapshot in v1. + If one is ever added, validate it with the Merkle root of the sorted key set (which + ADR-0004 already computes) rather than a checksum, because a checksum passes for an + operator who restores yesterday's data directory and leaves yesterday's snapshot. +- APFS enumeration degrades with churn, not just size: a million files went from about 72 + to about 306 microseconds per entry over twenty cycles of 5% replacement. A long-lived + macOS node will get slower to start in a way a fresh benchmark never shows. +- NTFS 8.3 short-name generation is worse for us than for most, because a node's filenames + genuinely share a long prefix. Microsoft advises disabling it above 300,000 files per + directory. + +## Validation + +**Already proved, locally:** publish is exactly-once under sixteen concurrent writers of one +address; the index rebuilds from the filesystem across restarts with a stable order; a file +in the wrong shard, an uppercase name, and a non-hex name are all refused; an interrupted +write is swept; a corrupt file is removed and repaired from the legacy copy; a missing file +drops out of the index so replication repairs it; the copier is resumable and cannot +resurrect a pruned chunk; retirement is refused while any gate is unmet and removes the +environment when they are all met; the release switches never round-trip through a config +file. + +**Fleet gates, which cannot be closed from a workstation:** + +- Forced power loss on ext4, XFS, btrfs, APFS and NTFS showing old-or-new, with antivirus + and 8.3 generation enabled on the NTFS run. Windows retirement stays off until this passes. +- Startup scan, RSS and inode use at 100k, 1M and 10M keys on each filesystem. +- The first release gates on no audit-timeout regression on the quiet responsible lane and on + disk growth + matching prediction. +- The second gates on a soak of the first, plus a verified retirement returning the + predicted space. +- The third gates on migration-complete lines across the fleet, refetch backlogs drained, and the + recorded audit failure rate back to its pre-migration baseline. The first release's + observability is + what makes that decidable. +- **How often a short-of-disk node can actually clear the possession gate.** A node whose + close group is also short of space will not clear it, will not free its disk, and will + tell its operator to add storage. That is the intended answer, but the fleet needs to + show how large that population is before the second release, because it decides whether the + migration + completes on its own or needs operator action at scale. +- **Chunk size is 4 MiB, confirmed.** This was the open question that gated the whole + design and it is now answered. Storj and borgbackup both ran one file per object at scale + and reversed to packing, and both did so for *small* objects: Storj's pieces are *"often + smaller than a hard drive sector"* and over 60% of borg's chunks are under 8 KiB. Nobody + has reversed this decision for large objects. The tripwire remains: if the network ever + starts storing a large share of small records, this ADR should be revisited, and the + inode exposure below comes with it. + +**Review trigger:** if the network ever adopts a declared node capacity, fixed-slot packing +becomes the better store design and this decision should be reopened. + +## Implementation slices + +This ADR is landed by two pull requests, in this order: + +1. **Stop penalising a node for not holding a close-group chunk.** One switch, one helper, + six call sites. It must ship a release ahead of the migration, because the penalty is + the auditor's decision and a node cannot stop its peers applying it. The commitment-bound + subtree audit keeps penalising throughout. +2. **The file store and the migration.** Everything else in this document. + +A third release flips the switch from (1) back, gated on fleet evidence rather than a date, +which is why it is a release and not an expiry constant compiled into the first one. + +## Notes for AI-assisted work + +Drafted with AI assistance. Not to be marked Accepted without human review. diff --git a/src/node.rs b/src/node.rs index 65b66b4f..f98f4dee 100644 --- a/src/node.rs +++ b/src/node.rs @@ -97,6 +97,12 @@ impl NodeBuilder { // Ensure root directory exists std::fs::create_dir_all(&self.config.root_dir)?; + // One release-level decision, applied before anything can audit: while the fleet + // moves off the legacy chunk store, a peer is not penalised for failing to hold a + // chunk it was supposed to be holding. It is still penalised for failing a + // commitment-bound audit. Audits of both kinds run and record throughout. + crate::replication::config::apply_close_group_storage_penalty_policy(); + // Create shutdown token let shutdown = CancellationToken::new(); diff --git a/src/replication/config.rs b/src/replication/config.rs index 66c8e0bd..55bc7b60 100644 --- a/src/replication/config.rs +++ b/src/replication/config.rs @@ -15,6 +15,11 @@ use std::time::Duration; use rand::Rng; use crate::ant_protocol::CLOSE_GROUP_SIZE; +use crate::logging::{debug, info, warn}; +use saorsa_core::identity::PeerId; +use saorsa_core::{P2PNode, TrustEvent}; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::Arc; // --------------------------------------------------------------------------- // Static constants (compile-time reference profile) @@ -668,6 +673,145 @@ pub(crate) const CAPACITY_BLOCKED_RETRY: Duration = /// Trust event weight for confirmed audit failures. pub const AUDIT_FAILURE_TRUST_WEIGHT: f64 = 5.0; +/// Whether this build penalises a peer for not holding a chunk it was supposed to hold. +/// +/// **`true` while the fleet moves off the legacy LMDB chunk store; back to `false` once it +/// has.** Flipping it is a one-line change in one release. +/// +/// Deliberately narrow. It covers exactly one accusation: "you did not have a chunk you +/// were supposed to be holding". It does **not** cover the commitment-bound subtree audit, +/// where a peer published a signed claim to hold specific keys and could not answer for +/// them. That contract stays enforced in every release. +/// +/// The reason it has to exist at all is that the penalty is the *auditor's* decision. A +/// node that has to give up chunks, because it cannot fit them while it moves them out of +/// a store that never returns disk, cannot stop its peers penalising it for that. So the +/// peers stop first, one release ahead, and the node moves in the next one. +/// +/// A build constant rather than a config field on purpose: a node writes its effective +/// configuration back to disk, so shipping this as an ordinary setting would bake this +/// release's value into every operator's file and the next release would change nothing. +pub const RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY: bool = true; + +/// Environment override for [`RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY`], for a canary. +pub const SUSPEND_CLOSE_GROUP_STORAGE_PENALTY_ENV: &str = "ANT_SUSPEND_UNHELD_CHUNK_PENALTY"; + +/// The live switch. +/// +/// Initialised **from the release constant**, not to `false`. That matters: a code path +/// that never applies the policy then behaves like this release rather than the previous +/// one. Defaulting the other way meant any constructor that skipped the startup call would +/// keep penalising nodes for the very thing this release exists to stop penalising, and +/// `ReplicationEngine::new` is public and is constructed directly by test harnesses. +/// +/// Process-wide rather than threaded through a parameter because it is exactly that: one +/// release-level decision that every affected site has to obey identically, and those +/// sites are spread across call graphs that share no configuration object. +static CLOSE_GROUP_STORAGE_PENALTY_SUSPENDED: AtomicBool = + AtomicBool::new(RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY); + +/// Apply this release's decision. Called once, before anything can audit. +pub fn apply_close_group_storage_penalty_policy() { + let Ok(raw) = std::env::var(SUSPEND_CLOSE_GROUP_STORAGE_PENALTY_ENV) else { + apply_and_announce(RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY); + return; + }; + let suspended = match raw.trim().to_ascii_lowercase().as_str() { + "1" | "true" | "yes" | "on" => true, + "0" | "false" | "no" | "off" => false, + other => { + warn!( + "{SUSPEND_CLOSE_GROUP_STORAGE_PENALTY_ENV}={other} is not a boolean; \ + using the build default {RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY}" + ); + RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY + } + }; + apply_and_announce(suspended); +} + +/// Set the switch and say so, once, where an operator will see it. +/// +/// Both states are logged. An operator reading "penalties are suspended" and an operator +/// reading nothing at all cannot tell the second from a missing log line, and the state +/// that most needs to be visible is the one that disagrees with what the release intended. +fn apply_and_announce(suspended: bool) { + set_close_group_storage_penalty_suspended(suspended); + if suspended != RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY { + warn!( + close_group_storage_penalty_suspended = suspended, + "{SUSPEND_CLOSE_GROUP_STORAGE_PENALTY_ENV} overrides this build: the penalty \ + for not holding a close-group chunk is {}, where the release intends {}. \ + Clear that variable unless this node is a deliberate canary.", + if suspended { "SUSPENDED" } else { "APPLIED" }, + if RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY { + "SUSPENDED" + } else { + "APPLIED" + } + ); + } + if suspended { + info!( + close_group_storage_penalty_suspended = true, + "This release does NOT penalise a peer for failing to hold a close-group \ + chunk. Commitment-bound audits still penalise. Audits run and record \ + throughout." + ); + } else { + info!( + close_group_storage_penalty_suspended = false, + "This release penalises a peer for failing to hold a close-group chunk." + ); + } +} + +/// Set whether failing to hold a close-group chunk penalises. +/// +/// Startup applies the release policy through this. Tests that mean to exercise the +/// penalty itself set it explicitly, so what they assert is not an accident of whichever +/// release they happen to be compiled against. +pub fn set_close_group_storage_penalty_suspended(suspended: bool) { + CLOSE_GROUP_STORAGE_PENALTY_SUSPENDED.store(suspended, Ordering::Relaxed); +} + +/// Whether failing to hold a close-group chunk currently penalises. +#[must_use] +pub fn close_group_storage_penalty_suspended() -> bool { + CLOSE_GROUP_STORAGE_PENALTY_SUSPENDED.load(Ordering::Relaxed) +} + +/// Penalise `peer` at `weight` for not holding a chunk it was supposed to be holding, +/// unless this release withholds that particular penalty. +/// +/// Covers the responsible-chunk audit, the fresh-replication possession check, the prune +/// audit, and the fetch paths where a peer that answered `Present` could not then serve +/// the bytes. A node short of the disk to hold its chunks produces every one of those, so +/// leaving any of them out would stop some of its accusers and not others. +/// +/// Only the penalty is withheld. The caller has already logged the failure with its type, +/// class and key, and that record is what tells us when it is safe to switch the penalty +/// back on. +pub async fn penalise_unheld_close_group_chunk( + p2p_node: &Arc, + peer: &PeerId, + audit_type: &str, + weight: f64, +) { + if close_group_storage_penalty_suspended() { + debug!( + audit_type, + peer = %peer, + "Recorded but not penalised: this release withholds the penalty for not \ + holding a close-group chunk. Commitment-bound audits still penalise." + ); + return; + } + p2p_node + .report_trust_event(peer, TrustEvent::ApplicationFailure(weight)) + .await; +} + /// Probability of launching a subtree audit when a peer's *changed* commitment /// is ingested via gossip (ADR-0002). Keeps audits occasional surprise exams. pub const AUDIT_ON_GOSSIP_PROBABILITY: f64 = 0.2; @@ -1258,6 +1402,7 @@ fn random_duration_in_range(min: Duration, max: Duration) -> Duration { #[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] mod tests { use super::*; + use serial_test::serial; #[test] fn defaults_pass_validation() { @@ -1290,6 +1435,34 @@ mod tests { assert!((AUDIT_FAILURE_TRUST_WEIGHT - 5.0).abs() <= f64::EPSILON); } + /// One test rather than several, because the switch is process-wide: separate tests + /// would race each other under the default parallel runner. + #[test] + #[serial] + fn the_unheld_chunk_penalty_switch_follows_the_release_it_is_compiled_into() { + // A build that never applies the policy still behaves like THIS release, not the + // previous one. `ReplicationEngine::new` is public and is constructed directly by + // test harnesses, so defaulting the other way would leave those engines penalising + // exactly what the release exists to stop penalising. + assert_eq!( + close_group_storage_penalty_suspended(), + RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY + ); + + set_close_group_storage_penalty_suspended(true); + assert!(close_group_storage_penalty_suspended()); + set_close_group_storage_penalty_suspended(false); + assert!(!close_group_storage_penalty_suspended()); + + // And applying the release policy lands on whatever this build ships, without + // asserting the constant itself, which the follow-up release flips on purpose. + apply_and_announce(RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY); + assert_eq!( + close_group_storage_penalty_suspended(), + RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY + ); + } + #[test] fn core_replication_id_stays_v2_and_subtree_rides_its_own_id() { // Core replication, including all digest audit lanes, stays on v2. diff --git a/src/replication/mod.rs b/src/replication/mod.rs index 0b25e38c..ff3b29cb 100644 --- a/src/replication/mod.rs +++ b/src/replication/mod.rs @@ -5815,7 +5815,10 @@ async fn dispatch_fresh_offer( responder_class = "fresh_offer", source = %source, key = %hex::encode(key), - "Fresh offer refused at admission — this node will be penalised for the resulting absence: {failure}" + penalty_suspended = config::close_group_storage_penalty_suspended(), + "Fresh offer refused at admission; the resulting absence is recorded \ + against this node, and penalised unless the release withholds it: \ + {failure}" ); // Release the key explicitly rather than on drop, so the next offer // opens a fresh entry rather than queueing behind a handler that was @@ -8171,7 +8174,7 @@ async fn run_verification_cycle(ctx: VerificationCycleContext<'_>) { } // Step 5: Update queues with the evaluated outcomes. - let mut bad_singleton_hints: HashMap = HashMap::new(); + let mut bad_singleton_hints: HashMap<(PeerId, SingletonHintFault), usize> = HashMap::new(); let mut q = queues.write().await; for (key, outcome) in evaluated { let replica_hint_sources = q @@ -8232,20 +8235,38 @@ async fn run_verification_cycle(ctx: VerificationCycleContext<'_>) { } drop(q); - for (peer, bad_hint_count) in bad_singleton_hints { + for ((peer, fault), bad_hint_count) in bad_singleton_hints { let reports = bad_hint_count.min(MAX_BAD_HINT_TRUST_REPORTS_PER_PEER_PER_CYCLE); warn!( "Peer {peer} submitted {bad_hint_count} rejected or self-contradicting \ - sole-source replica hints; \ + sole-source replica hints ({fault:?}); \ reporting {reports} bounded trust failure(s)" ); for _ in 0..reports { - p2p_node - .report_trust_event( - &peer, - TrustEvent::ApplicationFailure(REPLICATION_TRUST_WEIGHT), - ) - .await; + match fault { + // A claim about a key that does not exist. Punishable whatever the + // sender's disk is doing. + SingletonHintFault::RejectedByCloseGroup => { + p2p_node + .report_trust_event( + &peer, + TrustEvent::ApplicationFailure(REPLICATION_TRUST_WEIGHT), + ) + .await; + } + // "I advertised it and no longer have it." That is the one statement a + // node short of disk cannot avoid making while it moves its chunks, so + // it goes through the release switch. + SingletonHintFault::DeniedPossession => { + config::penalise_unheld_close_group_chunk( + p2p_node, + &peer, + "replica_hint_denied_possession", + REPLICATION_TRUST_WEIGHT, + ) + .await; + } + } } } } @@ -8297,25 +8318,43 @@ fn add_replica_hint_sources(sources: &mut Vec, replica_hint_sources: &Ha } } +/// Why a sole-source replica hint is punishable. +/// +/// The two cases look alike and are not. A hint the close group rejects outright is a +/// claim about a key that does not exist, which is a bad hint however the sender's disk is +/// doing. A sender that advertised a key and then answers `Absent` for it is making a +/// statement about its own storage, and that is the one thing a node short of disk cannot +/// avoid saying while it moves its chunks out of a store that will not give the space back. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +enum SingletonHintFault { + /// The close group says the key does not exist. + RejectedByCloseGroup, + /// The sender advertised the key and then denied holding it. + DeniedPossession, +} + /// Return the sole replica advertiser when either the close group definitively -/// rejects the key or the advertiser explicitly denies possessing it. +/// rejects the key or the advertiser explicitly denies possessing it, and say which. /// Paid-only advertisements, corroborated replica hints, and inconclusive /// rounds without that direct contradiction are deliberately non-penalizing. fn punishable_singleton_replica_hint_source( replica_hint_sources: &HashSet, outcome: &KeyVerificationOutcome, evidence: &crate::replication::types::KeyVerificationEvidence, -) -> Option { +) -> Option<(PeerId, SingletonHintFault)> { // A paid-only advertiser leaves this set empty, so the sole-source lane is // reserved for peers that actually claimed possession. if replica_hint_sources.len() != 1 { return None; } let source = *replica_hint_sources.iter().next()?; - let rejected_by_close_group = matches!(outcome, KeyVerificationOutcome::QuorumFailed); - let denied_possession = evidence.presence.get(&source) == Some(&PresenceEvidence::Absent); - - (rejected_by_close_group || denied_possession).then_some(source) + if matches!(outcome, KeyVerificationOutcome::QuorumFailed) { + return Some((source, SingletonHintFault::RejectedByCloseGroup)); + } + if evidence.presence.get(&source) == Some(&PresenceEvidence::Absent) { + return Some((source, SingletonHintFault::DeniedPossession)); + } + None } /// Post-verification bootstrap bookkeeping: remove terminal keys from the @@ -8793,12 +8832,16 @@ async fn execute_single_fetch( "Fetch: verified source {source} returned NotFound for {}", hex::encode(key) ); - p2p_node - .report_trust_event( - &source, - TrustEvent::ApplicationFailure(REPLICATION_TRUST_WEIGHT), - ) - .await; + // A node short of the disk to hold its chunks answers exactly this + // way, once per fetching peer per key, so this is one of the lanes + // the release has to withhold. + config::penalise_unheld_close_group_chunk( + &p2p_node, + &source, + "fetch_not_found", + REPLICATION_TRUST_WEIGHT, + ) + .await; FetchOutcome { key, result: FetchResult::SourceFailed, @@ -8812,12 +8855,16 @@ async fn execute_single_fetch( "Fetch: peer {source} returned error for {}: {reason}", hex::encode(key) ); - p2p_node - .report_trust_event( - &source, - TrustEvent::ApplicationFailure(REPLICATION_TRUST_WEIGHT), - ) - .await; + // A node short of the disk to hold its chunks answers exactly this + // way, once per fetching peer per key, so this is one of the lanes + // the release has to withhold. + config::penalise_unheld_close_group_chunk( + &p2p_node, + &source, + "fetch_error", + REPLICATION_TRUST_WEIGHT, + ) + .await; FetchOutcome { key, result: FetchResult::SourceFailed, @@ -8905,6 +8952,10 @@ async fn handle_subtree_failed_audit( let mut provers_guard = recent_provers.write().await; apply_audit_failure_credit_revocation(&mut provers_guard, challenged_peer, reason); } + // Deliberately NOT routed through the release switch. This is the commitment-bound + // subtree audit: the peer published a signed claim to hold these keys and could not + // answer for them. That contract is enforced in every release, including the one that + // withholds the penalty for merely not holding a close-group chunk. p2p_node .report_trust_event( challenged_peer, @@ -9097,12 +9148,13 @@ async fn handle_audit_result( } else { debug!("Audit timeout for {challenged_peer}; retaining active bootstrap claim"); } - p2p_node - .report_trust_event( - challenged_peer, - TrustEvent::ApplicationFailure(config::AUDIT_FAILURE_TRUST_WEIGHT), - ) - .await; + config::penalise_unheld_close_group_chunk( + p2p_node, + challenged_peer, + crate::replication::audit_metrics::AuditType::ResponsibleChunk.as_str(), + config::AUDIT_FAILURE_TRUST_WEIGHT, + ) + .await; } } AuditTickResult::BootstrapClaim { peer } => { @@ -10364,7 +10416,9 @@ mod tests { assert_eq!( punishable_singleton_replica_hint_source(&HashSet::from([source]), &failed, &evidence), - Some(source) + Some((source, SingletonHintFault::RejectedByCloseGroup)), + "a close-group rejection outranks the denial: the key does not exist, which is \ + a bad hint however the sender's own disk is doing" ); assert_eq!( punishable_singleton_replica_hint_source( @@ -10387,7 +10441,7 @@ mod tests { .insert(source, PresenceEvidence::Unresolved); assert_eq!( punishable_singleton_replica_hint_source(&HashSet::from([source]), &failed, &evidence), - Some(source), + Some((source, SingletonHintFault::RejectedByCloseGroup)), "definitive close-group rejection is punishable without direct contradiction" ); assert_eq!( @@ -10409,8 +10463,10 @@ mod tests { }, &evidence, ), - Some(source), - "an explicit denial is punishable regardless of the overall outcome" + Some((source, SingletonHintFault::DeniedPossession)), + "an explicit denial is punishable regardless of the overall outcome, and is \ + classified separately because it is a statement about the sender's own \ + storage rather than about the key" ); } diff --git a/src/replication/possession.rs b/src/replication/possession.rs index 72c4e969..b01f9678 100644 --- a/src/replication/possession.rs +++ b/src/replication/possession.rs @@ -225,15 +225,16 @@ async fn report_possession_confirmed_failure( peer = %peer, key = %key_hex, trust_weight = AUDIT_FAILURE_TRUST_WEIGHT, - "Possession check: {peer} failed to prove possession for {key_hex} ({}); penalising at audit severity", + "Possession check: {peer} failed to prove possession for {key_hex} ({}); recorded at audit severity", failure_reason.as_str() ); - p2p_node - .report_trust_event( - peer, - TrustEvent::ApplicationFailure(AUDIT_FAILURE_TRUST_WEIGHT), - ) - .await; + crate::replication::config::penalise_unheld_close_group_chunk( + p2p_node, + peer, + AuditType::Possession.as_str(), + AUDIT_FAILURE_TRUST_WEIGHT, + ) + .await; } async fn report_possession_audit_failure( @@ -248,15 +249,16 @@ async fn report_possession_audit_failure( peer = %peer, key = %key_hex, trust_weight = AUDIT_FAILURE_TRUST_WEIGHT, - "Possession check: {peer} {} for {key_hex}; penalising at audit severity", + "Possession check: {peer} {} for {key_hex}; recorded at audit severity", failure_class.as_str() ); - p2p_node - .report_trust_event( - peer, - TrustEvent::ApplicationFailure(AUDIT_FAILURE_TRUST_WEIGHT), - ) - .await; + crate::replication::config::penalise_unheld_close_group_chunk( + p2p_node, + peer, + AuditType::Possession.as_str(), + AUDIT_FAILURE_TRUST_WEIGHT, + ) + .await; } async fn handle_possession_bootstrap_claim( diff --git a/src/replication/pruning.rs b/src/replication/pruning.rs index 10acee66..f1ad98d2 100644 --- a/src/replication/pruning.rs +++ b/src/replication/pruning.rs @@ -1980,12 +1980,13 @@ async fn report_prune_audit_failure_once( "Prune audit failure: peer={peer}, audit_failure_class={audit_failure_class}, key={}", hex::encode(key) ); - p2p_node - .report_trust_event( - peer, - saorsa_core::TrustEvent::ApplicationFailure(AUDIT_FAILURE_TRUST_WEIGHT), - ) - .await; + crate::replication::config::penalise_unheld_close_group_chunk( + p2p_node, + peer, + AuditType::Prune.as_str(), + AUDIT_FAILURE_TRUST_WEIGHT, + ) + .await; true } diff --git a/tests/e2e/replication.rs b/tests/e2e/replication.rs index 841da90a..3d359c54 100644 --- a/tests/e2e/replication.rs +++ b/tests/e2e/replication.rs @@ -266,7 +266,7 @@ async fn test_fresh_replication_propagates_to_close_group() { /// eviction acts on), via `P2PNode::peer_trust`. #[tokio::test] #[serial] -async fn possession_check_penalises_absent_peer_only() { +async fn possession_check_penalises_absent_peer_only_and_obeys_the_release_switch() { let harness = TestHarness::setup_small().await.expect("setup"); harness.warmup_dht().await.expect("warmup"); @@ -324,6 +324,10 @@ async fn possession_check_penalises_absent_peer_only() { "precondition: C must hold the chunk" ); + // Switched on explicitly, so this half keeps testing the possession mechanism rather + // than whichever release it happens to be compiled against. + ant_node::replication::config::set_close_group_storage_penalty_suspended(false); + let trust_b_before = p2p_a.peer_trust(&peer_b); let trust_c_before = p2p_a.peer_trust(&peer_c); @@ -345,6 +349,25 @@ async fn possession_check_penalises_absent_peer_only() { "present peer C must not be penalised: {trust_c_before} -> {trust_c_after}" ); + // And the other half of the contract, on the same harness. The release that moves + // nodes off the legacy chunk store withholds exactly this penalty: a node short of + // disk cannot avoid answering "absent" while it moves its chunks out of a store that + // never returns space, and it cannot stop its peers penalising it for that, because + // the penalty is the auditor's decision. So the auditors stop one release ahead. + ant_node::replication::config::set_close_group_storage_penalty_suspended(true); + let trust_b_suspended_before = p2p_a.peer_trust(&peer_b); + engine_a + .run_possession_check_now(address, vec![peer_b, peer_c]) + .await; + let trust_b_suspended_after = p2p_a.peer_trust(&peer_b); + ant_node::replication::config::set_close_group_storage_penalty_suspended(false); + + assert!( + trust_b_suspended_after >= trust_b_suspended_before - f64::EPSILON, + "an absent peer must not be penalised while the release withholds that penalty: \ + {trust_b_suspended_before} -> {trust_b_suspended_after}" + ); + harness.teardown().await.expect("teardown"); } From 0edc96f152dcce8f006181be04dff9386079c8df Mon Sep 17 00:00:00 2001 From: grumbach Date: Tue, 25 Aug 2026 13:42:23 +0900 Subject: [PATCH 2/3] fix(replication): make the audit-type label available without the logging feature `AuditType::as_str` was gated on the `logging` feature because every caller was inside a log macro, which compiles to nothing when that feature is off. The penalty helper takes the label as an ordinary argument, and arguments are evaluated whether or not the macro that consumes them survives, so a `--no-default-features` build stopped compiling. Ungated rather than worked around at the call sites: it is a `const fn` over a three-variant enum returning a string literal, so it costs nothing in a build that never logs, and passing hand-written literals instead would let the structured-log labels drift from the enum they are meant to name. --- src/replication/audit_metrics.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/replication/audit_metrics.rs b/src/replication/audit_metrics.rs index 5646a889..e1f2cf85 100644 --- a/src/replication/audit_metrics.rs +++ b/src/replication/audit_metrics.rs @@ -407,9 +407,12 @@ static DIGEST_DISPATCH_LATENCY_COUNT: AtomicU64 = AtomicU64::new(0); static DIGEST_DISPATCH_LATENCY_TOTAL_MS: AtomicU64 = AtomicU64::new(0); static DIGEST_DISPATCH_LATENCY_MAX_MS: AtomicU64 = AtomicU64::new(0); -#[cfg(feature = "logging")] impl AuditType { /// Stable structured-log label. + /// + /// Not gated on the `logging` feature: it is passed as an ordinary argument to the + /// penalty helper, which evaluates its arguments whether or not the log macro that + /// consumes them compiles to anything. #[must_use] pub const fn as_str(self) -> &'static str { match self { From ac68e5915c03150ff6931dd9f08b59424740fac1 Mon Sep 17 00:00:00 2001 From: grumbach Date: Tue, 25 Aug 2026 20:37:16 +0900 Subject: [PATCH 3/3] fix(replication): keep charging a responder whose own storage read failed Review found that `FetchResponse::Error` was routed through the suspended lane, and it should not be. Its only producer is the responder's storage read returning an error: an I/O fault, an exhausted descriptor table, or a chunk whose bytes no longer hash to their address. A peer that simply does not hold the chunk answers `NotFound`, which is a separate variant and stays suspended. Nothing about a node giving chunks up produces an error answer, so withholding the penalty for one hid real faults for no benefit. The response mapping and the charging decision are now two small functions used by the real paths, so the meaning a responder puts on the wire and the charge a fetcher applies cannot drift apart. Tests pin both: a key the node does not hold reads as a plain miss and is answered `NotFound`, a failed read is answered `Error`, and the two answers are classified as different faults. This brings the count back to the six call sites the ADR describes, and the ADR now says explicitly that a failed responder read is not one of them. --- ...e-based-chunk-store-and-lmdb-retirement.md | 5 +- src/replication/mod.rs | 234 ++++++++++++++---- 2 files changed, 183 insertions(+), 56 deletions(-) diff --git a/docs/adr/ADR-0012-file-based-chunk-store-and-lmdb-retirement.md b/docs/adr/ADR-0012-file-based-chunk-store-and-lmdb-retirement.md index 7dcd5077..c3103c09 100644 --- a/docs/adr/ADR-0012-file-based-chunk-store-and-lmdb-retirement.md +++ b/docs/adr/ADR-0012-file-based-chunk-store-and-lmdb-retirement.md @@ -177,7 +177,10 @@ chunks cannot stop its auditors from penalising it, so the auditors have to stop What the first release withholds is deliberately narrow: only the penalty for **not holding a close-group chunk you were supposed to be holding**. The commitment-bound subtree audit still -penalises, in every release. That is not a compromise, it is what makes the rest work: a +penalises, in every release. So does a responder whose own storage fails: a fetch answered +with an error means the read faulted or the bytes no longer hash to their address, which is +never what a node giving chunks up looks like, and a node that does not hold the chunk says +so with `NotFound` instead. That is not a compromise, it is what makes the rest work: a node reduces its commitment precisely so its peers hold it to the smaller claim, and suspending that enforcement would make the reduction meaningless. Audits of both kinds run and record throughout. diff --git a/src/replication/mod.rs b/src/replication/mod.rs index ff3b29cb..30792ba0 100644 --- a/src/replication/mod.rs +++ b/src/replication/mod.rs @@ -6972,6 +6972,86 @@ fn request_is_stale(received_at: Instant, timeout: Duration) -> bool { received_at.elapsed() >= timeout } +/// How a fetch responder's answer is charged against its reputation. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum FetchFault { + /// The peer does not hold a chunk it was expected to hold. + /// + /// This is the lane the release withholds, because a node part-way through moving + /// off the legacy store answers exactly this way about chunks it has legitimately + /// given up. + UnheldChunk, + /// The peer's own storage failed, or served bytes that no longer hash to their + /// address. + /// + /// Never withheld. `FetchResponse::Error` has one producer, and it is the responder's + /// storage read returning an error: an I/O fault, an exhausted descriptor table, or a + /// failed integrity check. A peer that merely does not hold the chunk answers + /// `NotFound` instead, so nothing about the migration produces this. + ResponderFault, +} + +/// Classify a fetch response that did not carry the chunk. +/// +/// `Success` yields `None`. Every other answer is a fault of one kind or the other, and +/// which kind decides whether this release charges for it. +fn fetch_fault_for(response: &protocol::FetchResponse) -> Option { + match response { + protocol::FetchResponse::Success { .. } => None, + protocol::FetchResponse::NotFound { .. } => Some(FetchFault::UnheldChunk), + protocol::FetchResponse::Error { .. } => Some(FetchFault::ResponderFault), + } +} + +/// Charge a fetch fault to the responder. +/// +/// The only place the two kinds are treated differently. An unheld chunk goes through the +/// release switch, which is currently withholding it; a responder fault is charged +/// directly and is not affected by the switch at all. +async fn charge_fetch_fault( + p2p_node: &Arc, + source: &PeerId, + fault: FetchFault, + lane: &'static str, +) { + match fault { + FetchFault::UnheldChunk => { + config::penalise_unheld_close_group_chunk( + p2p_node, + source, + lane, + REPLICATION_TRUST_WEIGHT, + ) + .await; + } + FetchFault::ResponderFault => { + p2p_node + .report_trust_event( + source, + TrustEvent::ApplicationFailure(REPLICATION_TRUST_WEIGHT), + ) + .await; + } + } +} + +/// Turn the responder's storage read into the answer it sends back. +/// +/// The whole distinction the fetch lanes rest on is made here. A key this node does not +/// hold reads as `Ok(None)` and is answered `NotFound`. A read that fails, from an I/O +/// fault, an exhausted descriptor table, or a failed integrity check, is answered `Error`. +/// Nothing about a node giving chunks up produces the second. +fn fetch_response_for(key: XorName, read: Result>>) -> protocol::FetchResponse { + match read { + Ok(Some(data)) => protocol::FetchResponse::Success { key, data }, + Ok(None) => protocol::FetchResponse::NotFound { key }, + Err(e) => protocol::FetchResponse::Error { + key, + reason: format!("{e}"), + }, + } +} + async fn handle_fetch_request( source: &PeerId, request: &protocol::FetchRequest, @@ -6980,17 +7060,7 @@ async fn handle_fetch_request( request_id: u64, rr_message_id: Option<&str>, ) -> Result<()> { - let response = match storage.get(&request.key).await { - Ok(Some(data)) => protocol::FetchResponse::Success { - key: request.key, - data, - }, - Ok(None) => protocol::FetchResponse::NotFound { key: request.key }, - Err(e) => protocol::FetchResponse::Error { - key: request.key, - reason: format!("{e}"), - }, - }; + let response = fetch_response_for(request.key, storage.get(&request.key).await); send_replication_response( source, @@ -8820,51 +8890,33 @@ async fn execute_single_fetch( result: FetchResult::Stored, } } - ReplicationMessageBody::FetchResponse(protocol::FetchResponse::NotFound { - .. - }) => { - // This peer was selected as a fetch source because it - // recently answered `Present` during verification. A - // subsequent NotFound is evidence of a stale/false claim - // or chunk wiping, so penalize lightly and try another - // verified source. - warn!( - "Fetch: verified source {source} returned NotFound for {}", - hex::encode(key) - ); - // A node short of the disk to hold its chunks answers exactly this - // way, once per fetching peer per key, so this is one of the lanes - // the release has to withhold. - config::penalise_unheld_close_group_chunk( - &p2p_node, - &source, - "fetch_not_found", - REPLICATION_TRUST_WEIGHT, - ) - .await; - FetchOutcome { - key, - result: FetchResult::SourceFailed, + ReplicationMessageBody::FetchResponse( + ref response @ (protocol::FetchResponse::NotFound { .. } + | protocol::FetchResponse::Error { .. }), + ) => { + // This peer was selected as a fetch source because it recently + // answered `Present` during verification, so either answer is + // evidence of something. Which one decides what it is charged: a peer + // that does not hold the chunk is the lane this release withholds, a + // peer whose own read failed is not. + if let protocol::FetchResponse::Error { reason, .. } = response { + warn!( + "Fetch: peer {source} returned error for {}: {reason}", + hex::encode(key) + ); + } else { + warn!( + "Fetch: verified source {source} returned NotFound for {}", + hex::encode(key) + ); + } + if let Some(fault) = fetch_fault_for(response) { + let lane = match fault { + FetchFault::UnheldChunk => "fetch_not_found", + FetchFault::ResponderFault => "fetch_error", + }; + charge_fetch_fault(&p2p_node, &source, fault, lane).await; } - } - ReplicationMessageBody::FetchResponse(protocol::FetchResponse::Error { - reason, - .. - }) => { - warn!( - "Fetch: peer {source} returned error for {}: {reason}", - hex::encode(key) - ); - // A node short of the disk to hold its chunks answers exactly this - // way, once per fetching peer per key, so this is one of the lanes - // the release has to withhold. - config::penalise_unheld_close_group_chunk( - &p2p_node, - &source, - "fetch_error", - REPLICATION_TRUST_WEIGHT, - ) - .await; FetchOutcome { key, result: FetchResult::SourceFailed, @@ -9955,6 +10007,78 @@ async fn rebuild_and_rotate_commitment( #[cfg(test)] #[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] mod tests { + + /// The two fetch failures mean different things and must be charged differently. + /// + /// `NotFound` is a peer saying it does not hold the chunk, which is what a node + /// part-way through the migration says about chunks it has legitimately given up, so + /// it is the lane this release withholds. `Error` has a single producer, the + /// responder's own storage read failing, and that is never about the migration. + #[test] + fn a_missing_chunk_and_a_failed_read_are_different_faults() { + let key = [7u8; 32]; + assert_eq!( + fetch_fault_for(&protocol::FetchResponse::NotFound { key }), + Some(FetchFault::UnheldChunk) + ); + assert_eq!( + fetch_fault_for(&protocol::FetchResponse::Error { + key, + reason: "read failed".to_string(), + }), + Some(FetchFault::ResponderFault) + ); + assert_eq!( + fetch_fault_for(&protocol::FetchResponse::Success { + key, + data: vec![1, 2, 3], + }), + None + ); + } + + /// The responder's answer says which fault it is, so the mapping from a storage read + /// to a response is what the classification above rests on. + /// + /// A key the peer does not hold reads as `Ok(None)`. A read that fails, whether from + /// an I/O fault or a failed integrity check, reads as `Err`. Nothing in the migration + /// turns the first into the second. + #[tokio::test] + async fn a_missing_key_reads_as_a_plain_miss_and_a_failed_read_as_a_fault() { + let dir = tempfile::tempdir().expect("temp dir"); + let storage = LmdbStorage::new(crate::storage::LmdbStorageConfig { + root_dir: dir.path().to_path_buf(), + verify_on_read: true, + max_map_size: 0, + disk_reserve: 0, + }) + .await + .expect("open store"); + + let absent = [9u8; 32]; + assert!( + matches!(storage.get(&absent).await, Ok(None)), + "a chunk this node does not hold must read as a plain miss, not a fault" + ); + + // And the answer each read produces. A miss is `NotFound`, which is the withheld + // lane; a failed read is `Error`, which is not. + assert!(matches!( + fetch_response_for(absent, Ok(None)), + protocol::FetchResponse::NotFound { .. } + )); + assert!(matches!( + fetch_response_for(absent, Ok(Some(vec![1, 2, 3]))), + protocol::FetchResponse::Success { .. } + )); + assert!(matches!( + fetch_response_for( + absent, + Err(crate::error::Error::Storage("read failed".into())) + ), + protocol::FetchResponse::Error { .. } + )); + } use super::*; use super::{ apply_audit_failure_credit_revocation, audit_failure_clears_bootstrap_claim,