fix(gossip): RejectV1 default for modern-only support (ADR-014) - #546
Conversation
0e4ffd0 to
6a26b23
Compare
| if plumtree_inner.signature_policy() != SignaturePolicy::RejectV1 { | ||
| return Err(NetworkError::NodeCreation( | ||
| "mandatory outer signature policy RejectV1 could not be installed".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
The mandatory policy check runs before with_health_oracle, but production constructs this manager with Some(oracle) and the new constructor test only covers None. If applying the oracle changes the policy, the final object can lose RejectV1 without detection. Verify the final post-oracle object and cover the production branch so this invariant remains protected.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gossip/pubsub.rs
Line: 491-495
Comment:
**Post-oracle policy unchecked**
The mandatory policy check runs before `with_health_oracle`, but production constructs this manager with `Some(oracle)` and the new constructor test only covers `None`. If applying the oracle changes the policy, the final object can lose `RejectV1` without detection. Verify the final post-oracle object and cover the production branch so this invariant remains protected.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.|
|
||
| /// Configure before any topic creation or inbound handling, including | ||
| /// standalone managers made with the synchronous public constructors. | ||
| async fn ensure_eager_ceiling(&self) { | ||
| self.eager_ceiling_initialized | ||
| .get_or_init(|| async { | ||
| let degree = if self.participation.forwards_passthrough() { | ||
| 0 | ||
| } else { | ||
| self.egress_config.leaf_max_eager_degree | ||
| }; | ||
| self.plumtree.set_eager_degree_ceiling(degree).await; | ||
| }) | ||
| .await; | ||
| } | ||
|
|
||
| /// Sample independently of HTTP reads, once per runtime peer-refresh tick. | ||
| pub(crate) fn sample_egress(&self) { | ||
| let stages = serde_json::to_value(self.plumtree.stage_stats()).unwrap_or_default(); | ||
| let mut config = self.egress_config.clone(); | ||
| config.participation = self.participation; | ||
| self.egress_meter | ||
| .lock() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner) | ||
| .sample( | ||
| Instant::now(), | ||
| &stages["outbound_by_topic"], | ||
| &self.subscribed_topic_keys(), | ||
| &config, | ||
| ); | ||
| } | ||
|
|
||
| /// Named subscription/outbound diagnostics. Names use stored IDs, including | ||
| /// raw DM inbox IDs; unknown or no-longer-subscribed rows stay unknown-hex. | ||
| pub fn egress_diagnostics(&self) -> serde_json::Value { | ||
| let names = self | ||
| .topic_id_by_name | ||
| .read() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| let mut subscribed = names | ||
| .iter() | ||
| .map(|(name, id)| (name.clone(), id.to_string())) | ||
| .collect::<Vec<_>>(); | ||
| subscribed.sort(); | ||
| let topics = subscribed | ||
| .iter() | ||
| .map(|(name, id)| serde_json::json!({"name": name, "topic_id_hex8": id})) | ||
| .collect::<Vec<_>>(); | ||
| let stages = serde_json::to_value(self.plumtree.stage_stats()).unwrap_or_default(); | ||
| let rows = stages["outbound_by_topic"] | ||
| .as_object() | ||
| .map(|rows| { | ||
| rows.iter() | ||
| .map(|(id, counters)| { | ||
| let matching = subscribed | ||
| .iter() | ||
| .filter(|(_, topic)| topic == id) | ||
| .map(|(name, _)| name.clone()) | ||
| .collect::<Vec<_>>(); | ||
| serde_json::json!({ | ||
| "topic_id_hex8": id, | ||
| "name": matching.first().map(String::as_str).unwrap_or("unknown-hex"), | ||
| "names": matching, | ||
| "outbound": counters, | ||
| }) | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| }) | ||
| .unwrap_or_default(); | ||
| let meter = self | ||
| .egress_meter | ||
| .lock() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| serde_json::json!({ | ||
| "subscribed_topics": topics, | ||
| "outbound_by_topic_named": rows, | ||
| "egress_budget": { | ||
| "leaf_max_eager_degree": self.egress_config.leaf_max_eager_degree, | ||
| "leaf_egress_soft_bytes_per_sec": self.egress_config.leaf_egress_soft_bytes_per_sec, | ||
| "leaf_egress_hard_bytes_per_sec": self.egress_config.leaf_egress_hard_bytes_per_sec, | ||
| "applies_to_leaf": !self.participation.forwards_passthrough(), | ||
| "sustained_cap_status": "experimental: pinned sg draft #51 ceiling; publication and full acceptance pending", | ||
| "byte_policy": "observe_only", | ||
| "window_secs": 60, | ||
| "sample_age_secs": meter.sampled_at.map(|at| at.elapsed().as_secs_f64()), | ||
| "subscribed_outbound_bytes_per_sec_60s": meter.rate, | ||
| "egress_budget_soft_exceeded": meter.soft_exceeded, | ||
| "egress_budget_hard_exceeded": meter.hard_exceeded, | ||
| "exceed_count_unit": "runtime samples above threshold (nominally 1s)", | ||
| "rate_semantics": "1s sampled subscribed-topic send-attempt bytes, all kinds including repair; 60s denominator including warm-up", | ||
| "per_topic_meter_limit": "sg may omit topics beyond its bounded meter; rate is observed bytes, not a hard bound", | ||
| "repair": { | ||
| "tracking_overflow": self.transport.repair_tracking_overflow.load(Ordering::Relaxed), | ||
| "iwant_matched_eager_attempt_msgs": self.transport.repair_msgs.load(Ordering::Relaxed), | ||
| "iwant_matched_eager_attempt_bytes": self.transport.repair_bytes.load(Ordering::Relaxed), | ||
| "semantics": "subset of eager counters matching authenticated v2 in-flight IWANT peer/topic/message IDs; includes coincident same-message forwards, not confirmed delivery; anti_entropy stays separate" | ||
| } | ||
| } | ||
| }) | ||
| /// Cumulative outer v1 (header-only-signed) receipts since startup. | ||
| /// | ||
| /// Counts rejected frames under RejectV1 as well — evidence of contact | ||
| /// with the sunset boundary, not acceptance. | ||
| #[must_use] | ||
| pub fn outer_v1_receipts(&self) -> u64 { | ||
| self.plumtree.v1_receipt_count() |
There was a problem hiding this comment.
Receipt counter undercounts traffic
This diagnostic is described as cumulative outer-V1 receipts since startup, but a Leaf node returns before PlumtreePubSub::handle_message for V1 frames on unsubscribed topics. Those received frames never reach v1_receipt_count, so the metric can understate contact with V1 peers and provide misleading convergence evidence. Count at the earlier admission boundary or narrow the field's documented meaning and name.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gossip/pubsub.rs
Line: 543-550
Comment:
**Receipt counter undercounts traffic**
This diagnostic is described as cumulative outer-V1 receipts since startup, but a Leaf node returns before `PlumtreePubSub::handle_message` for V1 frames on unsubscribed topics. Those received frames never reach `v1_receipt_count`, so the metric can understate contact with V1 peers and provide misleading convergence evidence. Count at the earlier admission boundary or narrow the field's documented meaning and name.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.…ss) (#548) * test(convergence): additive convergence-release-modern (ADR-014 harness) Add just convergence-release-modern (fail-closed if X0XD_LEGACY_BINARY set) and soak --modern-only that excludes stock mixed-version from PASS, labeling not_in_modern_predicate. Stock convergence-release unchanged. Hermetic env-refuse + classifier self-tests; receipt helper stub with RejectV1 TODO until #546. No fake 10/10; no tag. * fix(convergence): fail-closed modern_policy_admission under --expect-fixed HOLD #548: under --modern-only --expect-fixed, require live diagnostics readback proving outer_signature_policy=reject_v1 and grants disabled. AcceptV1 / grants-enabled → fail; missing/unreadable/unproven → incomplete_policy. Both fail and incomplete_policy block nonzero exit so the modern recipe cannot green without admission. Hermetic classifier negative controls in ModernOnlyPredicateTests; stock recipe unchanged.
b85f862 to
d119faf
Compare
Install SignaturePolicy::RejectV1 at PubSubManager construction, expose outer_signature_policy / outer_v1_receipts on diagnostics, and add focused regression coverage for constructors, V1 refuse+receipt, V2 positive/tamper, signed publish sealing, Leaf/Full refuse, and the diagnostics route.
set_signature_policy does not need &mut; clears Clippy unused_mut that failed Clippy Lint and API Coverage Guard on tip 8009bd5.
Recover the six reviewed ADR-014 tests dropped after tip 0e4ffd0 so CI again asserts RejectV1 constructors, V1 receipting, V2 accept/tamper, modern publish sealing, Leaf/Full refusal, and diagnostics receipts.
d119faf to
a730fe2
Compare
Summary
saorsa_gossip_pubsub::SignaturePolicy::RejectV1inPubSubManager::new_with_oracle(coversnew/new_with_oracle/new_with_participation) and fails closed withNetworkError::NodeCreationif the policy does not stick.outer_signature_policy(reject_v1/accept_v1) and cumulativeouter_v1_receiptsthrough Agent +GET /diagnostics/gossip.publish_with_fanoutwithSigningContextsealing V2, Leaf/Full refuse without grant API, and diagnostics route contract.0.41.3; no Cargo.toml dependency bump; no AcceptV1 restore path; no migration grant enablement; no docs/ADR/receipt template in this PR.Authority / separation
docs/release/modern-only-convergence-receipt.template.md)34058040463stay FAIL as stock-compat evidence; this PR does not flipjust convergence-release.Test plan
cargo fmt --allcargo clippy --all-features --all-targets -- -D warningscargo clippy --all-features --lib --bins -- -D warnings -D clippy::panic -D clippy::unwrap_used -D clippy::expect_usedcargo check --workspace --all-targetsadr014_*+gossip_route_reports_reject_v1_and_receipt_increment(6 passed)Greptile Summary
The PR installs the modern-only
RejectV1outer gossip signature policy, exposes its state and V1 receipt count through diagnostics, and adds focused constructor, receive, publish, participation, and route tests.Confidence Score: 4/5
The PR appears safe to merge after non-blocking improvements to production-path policy coverage and the accuracy of the V1 receipt diagnostic contract.
RejectV1 is installed and the primary receive behavior is tested, but the final oracle-enabled object is not directly verified and the advertised cumulative receipt metric omits V1 frames rejected by the earlier Leaf admission gate.
Files Needing Attention: src/gossip/pubsub.rs
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Incoming outer gossip frame] --> B{Leaf and unsubscribed passthrough?} B -->|Yes| C[Refuse before PlumTree handling] C --> D[V1 receipt counter unchanged] B -->|No| E[PlumTree handle_message] E --> F{Outer signature version} F -->|V1| G[RejectV1 refusal] G --> H[Increment V1 receipt counter] F -->|Valid sealed V2| I[Continue delivery]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: 0e4ffd0 | Re-trigger Greptile