fix(replication)!: tune audit timeout policies - #164
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tunes replication audit timeout policy by increasing the minimum audit-response deadline and by aligning prune-audit request timeouts with the existing dynamic audit_response_timeout(key_count) sizing logic used for audit challenges.
Changes:
- Increased the audit response timeout floor from 2s to 4s and updated timeout-scaling tests accordingly.
- Removed the dedicated prune-audit timeout path and sized prune-audit request deadlines via
audit_response_timeout(key_count).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/replication/pruning.rs |
Computes prune challenge key_count and uses config.audit_response_timeout(key_count) when sending prune audit requests. |
src/replication/config.rs |
Raises AUDIT_RESPONSE_FLOOR_SECS to 4s, removes prune-audit-specific timeout constant/field, and updates timeout math/test expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Slack multiplier on the honest-read estimate before | ||
| /// declaring an audit timed out. | ||
| pub audit_response_honest_multiplier: u64, | ||
| /// Single-key prune-audit response deadline. Has its own constant | ||
| /// because the relay-defence rationale that motivates the tight | ||
| /// commitment-bound budget does not apply to a single-key prune | ||
| /// challenge. | ||
| pub prune_audit_response_timeout: Duration, | ||
| /// Maximum duration a peer may claim bootstrap status. | ||
| pub bootstrap_claim_grace_period: Duration, |
| audit_tick_interval_min: AUDIT_TICK_INTERVAL_MIN, | ||
| audit_tick_interval_max: AUDIT_TICK_INTERVAL_MAX, | ||
| audit_response_floor: Duration::from_secs(AUDIT_RESPONSE_FLOOR_SECS), | ||
| audit_honest_read_bps: AUDIT_HONEST_READ_BPS, | ||
| audit_response_honest_multiplier: AUDIT_RESPONSE_HONEST_MULTIPLIER, | ||
| prune_audit_response_timeout: Duration::from_secs(PRUNE_AUDIT_RESPONSE_SECS), | ||
| bootstrap_claim_grace_period: BOOTSTRAP_CLAIM_GRACE_PERIOD, | ||
| prune_hysteresis_duration: PRUNE_HYSTERESIS_DURATION, |
dirvine
left a comment
There was a problem hiding this comment.
I can't approve this as a patch as-is.
The timeout behaviour itself looks sound: audit_response_timeout now gives 4.4s / 8s / 44s for 1 / 10 / 100 keys, and prune audits now derive their request timeout from the encoded challenge key count. Local checks passed:
cargo fmt --all -- --checkcargo test audit_response_timeout --all-features— 5 passedcargo test prune --all-features— 22 passed including the prune e2e filters
Blocker: ReplicationConfig is publicly re-exported from src/lib.rs, so removing the public prune_audit_response_timeout field breaks downstream struct-literal users while the PR says this is patch/no public API change. Either keep the field for compatibility (deprecated/ignored if necessary, still initialised in Default) or update the semver/release framing to acknowledge the breaking API change.
CI note: current head is dec0625bf4b6a13d2e3972cbba4d28d422d8d7ad; build/clippy/fmt/docs/security/no-logging test are green, but the OS test matrix was still pending when I checked.
BREAKING CHANGE: ReplicationConfig no longer exposes the public prune_audit_response_timeout field. Prune audits now derive their request timeout from audit_response_timeout(key_count).
dec0625 to
a35f8eb
Compare
dirvine
left a comment
There was a problem hiding this comment.
Re-reviewed current head a35f8eb977524b9d3a88a3b4ddb9063b5b80eaab after the API/semver note was updated.
The prior blocker is addressed: the public ReplicationConfig::prune_audit_response_timeout field removal is now explicitly marked as breaking in the title/body/semver notes.
Verified locally:
git diff --check origin/main...HEAD— cleancargo test audit_response_timeout --all-features— 5 passedcargo test prune --all-features— 18 unit + 4 e2e prune tests passed
Code path checked: prune challenges still encode one key today, pass the derived key count through to send_prune_audit_challenge, and use config.audit_response_timeout(key_count); no protocol/runtime blocker found.
CI note: build/clippy/fmt/docs/security/no-logging checks were green when reviewed; OS test matrix was still pending, so merge should still wait for required checks.
Summary
This PR adjusts replication audit timeout policy in two commits:
ReplicationConfig::prune_audit_response_timeoutfield. Prune audit requests now use the same dynamicaudit_response_timeout(key_count)helper used by responsible chunk audits.Details
Raise chunk audit timeout floor
The minimum audit response deadline was previously 2 seconds. This PR raises
AUDIT_RESPONSE_FLOOR_SECSto 4 seconds and updates the dependent timeout math/test expectations:audit_response_timeout(1)becomes 4.4s.audit_response_timeout(10)becomes 8s.audit_response_timeout(100)becomes 44s.This keeps the existing per-byte scaling behavior while giving the hashes-only audit envelope more baseline room for cross-continent RTT and scheduling jitter.
Dynamic prune audit timeouts
Prune audits previously had their own static
PRUNE_AUDIT_RESPONSE_SECS = 10constant and publicReplicationConfig::prune_audit_response_timeoutfield. This PR removes that separate timeout/config surface and derives the prune audit request deadline from the encoded challenge key count:Today prune audit challenges still encode one key, but the timeout now follows the same dynamic path as responsible chunk audits and will scale correctly if prune challenges become batched later.
Semver
Breaking.
ReplicationConfigis publicly re-exported fromsrc/lib.rs, and this PR removes the publicprune_audit_response_timeoutfield. Downstream struct-literal users must remove that field and allow prune audits to useaudit_response_timeout(key_count).The wire format is unchanged.
Testing
Ran: