Skip to content

fix: disable geo IP diversity checks in devnet#19

Merged
dirvine merged 2 commits intomainfrom
fix/devnet-disable-geo-diversity
Mar 9, 2026
Merged

fix: disable geo IP diversity checks in devnet#19
dirvine merged 2 commits intomainfrom
fix/devnet-disable-geo-diversity

Conversation

@mickvandijke
Copy link
Copy Markdown
Collaborator

Summary

  • Devnet nodes were using production IPDiversityConfig defaults (geo checks, ASN limits, subnet limits), causing unnecessary restrictions during local testing
  • Set IPDiversityConfig::permissive() on devnet node configs to disable all diversity enforcement

Test plan

  • Start a local devnet and verify nodes connect without diversity-related rejections
  • Confirm production node startup is unaffected (does not use devnet code path)

🤖 Generated with Claude Code

Devnet nodes were using production diversity defaults (geo checks,
ASN limits, subnet limits), which is unnecessary for local testing.
Set IPDiversityConfig::permissive() to disable all diversity
enforcement in devnet nodes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 9, 2026 11:42
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 9, 2026

Greptile Summary

This PR fixes a gap in the devnet code path where Devnet::start_node was not setting a diversity_config, causing devnet nodes to fall back on whatever default saorsa_core applies — likely the same strict production settings (geo checks, ASN/subnet limits) that make local multi-node setups fail to form a network.

The one-line fix mirrors what NodeBuilder already does for NetworkMode::Development in src/node.rs (line 230), and correctly scopes the change only to the saorsa-devnet binary's node-startup path.

Key points:

  • The change is logically sound and consistent in intent with the existing NetworkMode::Development handling in src/node.rs.
  • Production and testnet code paths (NodeBuilder::apply_network_mode_tuning) are entirely unaffected.
  • Minor: the new line uses saorsa_core::security::IPDiversityConfig (a submodule path) while the rest of the codebase imports IPDiversityConfig from the saorsa_core crate root with the alias CoreDiversityConfig. Aligning with the existing import convention would improve consistency.

Confidence Score: 5/5

  • Safe to merge — the change is tightly scoped to the devnet code path with no risk to production nodes.
  • Single-line addition in a devnet-only function (Devnet::start_node). The production NodeBuilder path is completely separate and already handles its own diversity_config per NetworkMode. The fix correctly sets permissive() diversity configuration, matching the existing pattern in node.rs for NetworkMode::Development. The only issue is a minor import path style inconsistency.
  • No files require special attention.

Important Files Changed

Filename Overview
src/devnet.rs One-line fix that sets IPDiversityConfig::permissive() on devnet node configs during start_node; logically correct and scoped only to the devnet code path. Minor style inconsistency in how the type is referenced (inline submodule path vs. crate-root import used elsewhere).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[saorsa-node binary\nNodeBuilder::build] -->|NetworkMode::Production| B[CoreDiversityConfig::default\ngeo checks + ASN + subnet limits]
    A -->|NetworkMode::Testnet| C[CoreDiversityConfig::testnet\ncustomizable geo / ASN]
    A -->|NetworkMode::Development| D[CoreDiversityConfig::permissive\nno checks]

    E[saorsa-devnet binary\nDevnet::start_node] -->|Before this PR| F[No diversity_config set\nfalls back to saorsa-core default\nstrict checks block local nodes]
    E -->|After this PR ✅| G[CoreDiversityConfig::permissive\nno diversity enforcement\nnodes connect freely]

    style F fill:#ffcccc,stroke:#cc0000
    style G fill:#ccffcc,stroke:#009900
    style D fill:#ccffcc,stroke:#009900
Loading

Last reviewed commit: 6cd0d5e

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates devnet node startup to disable IP diversity enforcement so local devnet nodes don’t get rejected due to production diversity defaults (geo/ASN/subnet limits).

Changes:

  • Set a permissive diversity_config on devnet CoreNodeConfig during node startup to relax diversity checks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot AI review requested due to automatic review settings March 9, 2026 11:44
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

src/devnet.rs Outdated
.bootstrap_peers
.clone_from(&node.bootstrap_addrs);
core_config.max_message_size = Some(crate::ant_protocol::MAX_WIRE_MESSAGE_SIZE);
core_config.diversity_config = Some(CoreDiversityConfig::permissive());
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CoreDiversityConfig isn’t imported in this module, so this line won’t compile. Import saorsa_core::IPDiversityConfig (aliased as CoreDiversityConfig, consistent with src/node.rs) or use the fully-qualified path here.

Copilot uses AI. Check for mistakes.
Simplify import statement by directly using IPDiversityConfig in devnet.rs.
@mickvandijke mickvandijke force-pushed the fix/devnet-disable-geo-diversity branch from 48bd6f2 to 230e9b4 Compare March 9, 2026 14:38
Copy link
Copy Markdown
Collaborator

@dirvine dirvine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mick. I checked this change and the devnet-only diversity override is scoped correctly.

@dirvine dirvine merged commit 3b199e9 into main Mar 9, 2026
11 checks passed
@dirvine dirvine deleted the fix/devnet-disable-geo-diversity branch March 9, 2026 17:01
mickvandijke added a commit that referenced this pull request Apr 1, 2026
Add unit and e2e tests covering the remaining Section 18 scenarios:

Unit tests (32 new):
- Quorum: #4 fail→abandoned, #16 timeout→inconclusive, #27 single-round
  dual-evidence, #28 dynamic threshold undersized, #33 batched per-key,
  #34 partial response unresolved, #42 quorum-derived paid-list auth
- Admission: #5 unauthorized peer, #7 out-of-range rejected
- Config: #18 invalid config rejected, #26 dynamic paid threshold
- Scheduling: #8 dedup safety, #8 replica/paid collapse
- Neighbor sync: #35 round-robin cooldown skip, #36 cycle completion,
  #38 snapshot stability mid-join, #39 unreachable removal + slot fill,
  #40 cooldown peer removed, #41 cycle termination guarantee,
  consecutive rounds, cycle preserves sync times
- Pruning: #50 hysteresis prevents premature delete, #51 timestamp reset
  on heal, #52 paid/record timestamps independent, #23 entry removal
- Audit: #19/#53 partial failure mixed responsibility, #54 all pass,
  #55 empty failure discard, #56 repair opportunity filter,
  response count validation, digest uses full record bytes
- Types: #13 bootstrap drain, repair opportunity edge cases,
  terminal state variants
- Bootstrap claims: #46 first-seen recorded, #49 cleared on normal

E2e tests (4 new):
- #2 fresh offer with empty PoP rejected
- #5/#37 neighbor sync request returns response
- #11 audit challenge multi-key (present + absent)
- Fetch not-found for non-existent key

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants