Skip to content

Commit 4963130

Browse files
mickvandijkeclaude
andcommitted
fix(replication): make the fresh-offer proof cap a lifetime budget
MAX_FRESH_OFFER_ATTEMPTS_PER_KEY gated on `pending.len()`, the queue's instantaneous depth. The handler pops a proof before verifying it, so every pop returned a slot that a fresh source could refill. Since the per-source set only bars repeats, a stream of distinct peers kept one entry alive indefinitely: unbounded sequential payment verifications — EVM and DHT work — while holding an admission permit and one of only four fresh-offer worker slots. Four such keys idle the whole pool. The staleness shed is no backstop; it runs once before the loop, not inside it. Count admissions instead, and never decrement. A popped proof has spent its slot rather than returned it, so a key costs at most CLOSE_GROUP_MAJORITY verifications no matter how many peers offer it. This also bounds the entry's source set, which previously grew one PeerId per sybil for the life of the entry. Regression test drives pop-then-admit with a fresh source each time and asserts the lifetime count holds; it accepted 16 proofs against a budget of 4 before the fix. Reported by AI review of PR #165, independently reproduced here before fixing. Verified with the commands CI runs, including the no-default-features build and test that the previous commit tripped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 8d66ee0 commit 4963130

2 files changed

Lines changed: 79 additions & 8 deletions

File tree

docs/adr/ADR-0005-replication-repair-hardening.md

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -215,9 +215,14 @@ inline verification/LMDB path exists on the serial loop (`mod.rs`, `fresh.rs`).
215215
first arrival the only arrival: one bad proof lost the record even with a valid
216216
proof queued behind it. Rotation also makes each proof independently
217217
attributable, which is what makes the structural penalties below safe to apply.
218-
- The queue holds at most `MAX_FRESH_OFFER_ATTEMPTS_PER_KEY` =
219-
`CLOSE_GROUP_MAJORITY` proofs, **one per source peer**, so a single peer cannot
220-
fill it and each queued proof costs at most one extra verification. The
218+
- An entry admits at most `MAX_FRESH_OFFER_ATTEMPTS_PER_KEY` =
219+
`CLOSE_GROUP_MAJORITY` proofs **over its lifetime**, one per source peer. The
220+
budget counts admissions and is never decremented, which is the load-bearing
221+
detail: the handler pops a proof before verifying it, so a cap on the queue's
222+
instantaneous length would let a fresh source refill each popped slot and run
223+
unbounded sequential on-chain verifications while holding one of the four
224+
worker slots. Counted as a lifetime budget, a key costs at most that many
225+
verifications no matter how many peers offer it. The
221226
ceiling is now **64 MiB of payload + 32 MiB of proofs** (16 keys × 4 proofs ×
222227
`MAX_PAYMENT_PROOF_SIZE_BYTES`), which is why the proof-size bounds moved from
223228
the verifier onto the serial loop: the verifier only sees a proof on a worker,

src/replication/mod.rs

Lines changed: 71 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1019,9 +1019,16 @@ const FRESH_OFFER_MAX_OUTSTANDING: usize = 16;
10191019
///
10201020
/// Sizing the queue to the number of legitimate senders is what lets a failing
10211021
/// proof fall through to the next one instead of costing the node the record.
1022-
/// It also bounds the work an attacker can buy: each queued proof is at most one
1023-
/// extra on-chain verification, and one attempt per source means a single peer
1024-
/// occupies a single slot.
1022+
///
1023+
/// This is a **lifetime budget per entry**, not a queue depth: it counts proofs
1024+
/// admitted, and a proof the handler has popped has spent its slot rather than
1025+
/// returned it. That distinction is what bounds the work an attacker can buy —
1026+
/// at most this many on-chain verifications per key, after which the entry
1027+
/// refuses everyone until it closes. Gating on the queue's instantaneous length
1028+
/// instead would let a stream of distinct sources refill each popped slot and
1029+
/// run unbounded sequential verifications while holding one of the four worker
1030+
/// slots. One attempt per source additionally means a single peer can only ever
1031+
/// consume one of these.
10251032
const MAX_FRESH_OFFER_ATTEMPTS_PER_KEY: usize = crate::ant_protocol::CLOSE_GROUP_MAJORITY;
10261033

10271034
/// Fresh-offer slots kept reachable by sources that hold none.
@@ -1215,6 +1222,15 @@ struct FreshOfferEntry {
12151222
pending: VecDeque<FreshOfferAttempt>,
12161223
/// Sources already represented, so one peer cannot fill the queue alone.
12171224
sources: HashSet<PeerId>,
1225+
/// Proofs admitted over this entry's whole life, **never decremented**.
1226+
///
1227+
/// The budget has to be spent by admissions rather than held by the queue.
1228+
/// The handler pops a proof before verifying it, so gating on
1229+
/// `pending.len()` would let a fresh source refill the slot that pop just
1230+
/// freed, and a stream of distinct sources could then run unbounded
1231+
/// sequential payment verifications while holding one of only four worker
1232+
/// slots. Counting admissions makes the cap a lifetime budget instead.
1233+
admitted: usize,
12181234
}
12191235

12201236
/// How an arriving fresh offer joined the work already in flight for its key.
@@ -1293,9 +1309,12 @@ impl FreshOfferEntryGuard {
12931309
if entry.sources.contains(&source) {
12941310
return FreshOfferAdmission::DuplicateSource;
12951311
}
1296-
if entry.pending.len() >= MAX_FRESH_OFFER_ATTEMPTS_PER_KEY {
1312+
// Lifetime budget, not queue depth: a proof the handler has already
1313+
// popped has spent its slot, not returned it.
1314+
if entry.admitted >= MAX_FRESH_OFFER_ATTEMPTS_PER_KEY {
12971315
return FreshOfferAdmission::Full;
12981316
}
1317+
entry.admitted = entry.admitted.saturating_add(1);
12991318
entry.sources.insert(source);
13001319
entry.pending.push_back(attempt);
13011320
return FreshOfferAdmission::Joined;
@@ -1305,7 +1324,14 @@ impl FreshOfferEntryGuard {
13051324
sources.insert(source);
13061325
let mut pending = VecDeque::new();
13071326
pending.push_back(attempt);
1308-
in_flight_map.insert(key, FreshOfferEntry { pending, sources });
1327+
in_flight_map.insert(
1328+
key,
1329+
FreshOfferEntry {
1330+
pending,
1331+
sources,
1332+
admitted: 1,
1333+
},
1334+
);
13091335
drop(in_flight_map);
13101336
FreshOfferAdmission::Opened(Box::new(Self {
13111337
in_flight: Arc::clone(in_flight),
@@ -9239,6 +9265,46 @@ mod tests {
92399265
);
92409266
}
92419267

9268+
/// The cap has to be a lifetime budget, not a queue depth.
9269+
///
9270+
/// The handler pops a proof before verifying it, so gating admission on the
9271+
/// *instantaneous* queue length lets a fresh source refill the slot that pop
9272+
/// just freed. Each refill is another on-chain payment verification, run
9273+
/// sequentially while the entry holds its admission permit and a worker
9274+
/// slot, so a stream of distinct sources could keep one key's handler — and
9275+
/// one of only four workers — busy indefinitely.
9276+
#[test]
9277+
fn refilling_a_popped_slot_cannot_extend_a_keys_attempt_budget() {
9278+
let in_flight: FreshOfferInFlight = Arc::new(Mutex::new(HashMap::new()));
9279+
let data = vec![0x3Du8; 32];
9280+
9281+
let FreshOfferAdmission::Opened(mut entry) =
9282+
admit_test_offer(&in_flight, test_fresh_offer(data.clone(), 0), test_peer(0))
9283+
else {
9284+
panic!("the first offer should open the entry")
9285+
};
9286+
9287+
// Drive pop-then-admit with a fresh source every time, exactly as a
9288+
// sybil stream would. The budget must be spent by admissions, not
9289+
// returned by pops.
9290+
let mut admitted = 1;
9291+
for i in 1..u8::try_from(MAX_FRESH_OFFER_ATTEMPTS_PER_KEY * 4).unwrap_or(u8::MAX) {
9292+
let _ = entry.next_attempt();
9293+
if matches!(
9294+
admit_test_offer(&in_flight, test_fresh_offer(data.clone(), 0), test_peer(i)),
9295+
FreshOfferAdmission::Joined
9296+
) {
9297+
admitted += 1;
9298+
}
9299+
}
9300+
9301+
assert!(
9302+
admitted <= MAX_FRESH_OFFER_ATTEMPTS_PER_KEY,
9303+
"a key accepted {admitted} proofs over its lifetime, but the budget is \
9304+
{MAX_FRESH_OFFER_ATTEMPTS_PER_KEY}: popping a proof must not return its slot"
9305+
);
9306+
}
9307+
92429308
#[test]
92439309
fn a_fresh_offer_entry_queues_no_more_proofs_than_the_close_group_can_send() {
92449310
let in_flight: FreshOfferInFlight = Arc::new(Mutex::new(HashMap::new()));

0 commit comments

Comments
 (0)