Max/socks5 autodiscovery#6920
Open
mfahampshire wants to merge 6 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds country-aware SOCKS5 auto-discovery in ChangesSOCKS5 Auto-Discovery Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
sdk/rust/nym-sdk/src/mixnet/socks5_discovery.rs (1)
74-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
get_best_network_requester*is a weighted random pick, not “best”.These helpers can legitimately return a lower-performance requester because
choose_weightedis probabilistic. Please rename them before this lands in the public API, or make the summary/docs lead with “weighted random selection” instead of “best”.Also applies to: 81-88, 127-129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/rust/nym-sdk/src/mixnet/socks5_discovery.rs` around lines 74 - 79, The public helpers named get_best_network_requester, get_best_network_requester_in, and get_best_network_requester_from are described as selecting the “best” requester, but their behavior uses choose_weighted and is only a weighted random selection. Update the API to either rename these functions and their related docs/comments to reflect weighted random selection, or revise the summaries/docstrings to avoid claiming “best” and instead clearly describe the probabilistic selection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@documentation/docs/pages/developers/concepts/exit-security.mdx`:
- Around line 65-66: The guidance in the Exit Security callout should not
recommend `preferredIpr` to SOCKS5 users, since that knob is IPR-specific and
not available for Network Requester flows. Update the wording in the
exit-security callout so it applies only to IPR-backed clients, or add the
SOCKS5 equivalent using `Socks5MixnetClient::connect_new(...)` and the
`discover().country(...)` / `.countries(...)` filters. Keep the recommendation
aligned with the client type mentioned in the surrounding docs.
In `@sdk/rust/nym-sdk/src/mixnet/socks5_discovery.rs`:
- Around line 54-68: The requester discovery logic in `socks5_discovery.rs` is
silently skipping invalid entries, which hides metadata issues and makes pool
shrinkage hard to diagnose. Update the `NetworkRequesterWithPerformance`
collection flow in this loop to emit warnings when `all_nodes.get(...)` misses a
described node and when `nr_info.address.parse()` fails, including the exit
identity and offending address in the log. Keep the existing filtering behavior,
but ensure malformed or missing requester records are observable instead of
being dropped without context.
---
Nitpick comments:
In `@sdk/rust/nym-sdk/src/mixnet/socks5_discovery.rs`:
- Around line 74-79: The public helpers named get_best_network_requester,
get_best_network_requester_in, and get_best_network_requester_from are described
as selecting the “best” requester, but their behavior uses choose_weighted and
is only a weighted random selection. Update the API to either rename these
functions and their related docs/comments to reflect weighted random selection,
or revise the summaries/docstrings to avoid claiming “best” and instead clearly
describe the probabilistic selection behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e1036cf-b1f1-46cf-8133-5943768464be
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
documentation/docs/components/outputs/api-scraping-outputs/circulating-supply.jsondocumentation/docs/components/outputs/api-scraping-outputs/nodes-count.jsondocumentation/docs/components/outputs/api-scraping-outputs/nyx-outputs/circulating-supply.mddocumentation/docs/components/outputs/api-scraping-outputs/nyx-outputs/epoch-reward-budget.mddocumentation/docs/components/outputs/api-scraping-outputs/nyx-outputs/stake-saturation.mddocumentation/docs/components/outputs/api-scraping-outputs/nyx-outputs/staking-target.mddocumentation/docs/components/outputs/api-scraping-outputs/nyx-outputs/staking_supply.mddocumentation/docs/components/outputs/api-scraping-outputs/nyx-outputs/token-table.mddocumentation/docs/components/outputs/api-scraping-outputs/reward-params.jsondocumentation/docs/components/outputs/api-scraping-outputs/time-now.mddocumentation/docs/components/outputs/command-outputs/nym-api-help.mddocumentation/docs/components/outputs/command-outputs/nym-node-help.mddocumentation/docs/components/outputs/command-outputs/nym-node-run-help.mddocumentation/docs/components/outputs/command-outputs/nymvisor-help.mddocumentation/docs/pages/developers/_meta.jsondocumentation/docs/pages/developers/clients/socks5.mdxdocumentation/docs/pages/developers/concepts/exit-security.mdxdocumentation/docs/pages/developers/index.mdxdocumentation/docs/pages/developers/rust.mdxdocumentation/docs/pages/developers/rust/_meta.jsondocumentation/docs/pages/developers/rust/socks5.mdxsdk/rust/nym-sdk/Cargo.tomlsdk/rust/nym-sdk/examples/socks5_autodiscover.rssdk/rust/nym-sdk/src/error.rssdk/rust/nym-sdk/src/mixnet.rssdk/rust/nym-sdk/src/mixnet/socks5_client.rssdk/rust/nym-sdk/src/mixnet/socks5_discovery.rs
serinko
approved these changes
Jun 29, 2026
serinko
left a comment
serinko
left a comment
Contributor
There was a problem hiding this comment.
I have a same comment over a few places, I leave it up to you how to handle that, just let me know to sync up with the Operators Guide
simonwicky
reviewed
Jun 29, 2026
| use nym_task::ShutdownTracker; | ||
| use tokio::sync::RwLockReadGuard; | ||
|
|
||
| use celes::Country; |
Contributor
There was a problem hiding this comment.
nittest nit : How about grouping the imports? One group nym stuff, on crate stuff and one with the rest. We're so close
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/rust/nym-sdk/src/mixnet/socks5_discovery.rs`:
- Around line 93-98: The resolve logic in Recipient/NetworkRequester currently
treats InCountries with an empty list the same as Any, which lets callers bypass
the country restriction. Update resolve() so the InCountries branch enforces the
non-empty invariant directly, either by rejecting empty vectors with an error or
by delegating to the same validation used by in_countries(), while keeping Exact
and Any behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cc8c203-89cb-41b2-bc6e-55502430d64e
📒 Files selected for processing (6)
documentation/docs/pages/developers/rust/socks5.mdxsdk/rust/nym-sdk/examples/socks5_autodiscover.rssdk/rust/nym-sdk/src/error.rssdk/rust/nym-sdk/src/mixnet.rssdk/rust/nym-sdk/src/mixnet/socks5_client.rssdk/rust/nym-sdk/src/mixnet/socks5_discovery.rs
✅ Files skipped from review due to trivial changes (1)
- documentation/docs/pages/developers/rust/socks5.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- sdk/rust/nym-sdk/examples/socks5_autodiscover.rs
- sdk/rust/nym-sdk/src/error.rs
Comment on lines
+93
to
+98
| pub async fn resolve(&self) -> Result<Recipient, Error> { | ||
| match self { | ||
| Self::Exact(addr) => Ok((**addr).clone()), | ||
| Self::Any => discover(&[]).await, | ||
| Self::InCountries(countries) => discover(countries).await, | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce the non-empty country invariant in resolve() too.
Lines 96-97 currently turn NetworkRequester::InCountries(vec![]) into an any-country discovery. Because the enum variants are public, callers can bypass in_countries() and silently lose the country restriction this PR is supposed to guarantee.
Proposed fix
pub async fn resolve(&self) -> Result<Recipient, Error> {
match self {
Self::Exact(addr) => Ok((**addr).clone()),
Self::Any => discover(&[]).await,
+ Self::InCountries(countries) if countries.is_empty() => {
+ Err(Error::NoCountriesSpecified)
+ }
Self::InCountries(countries) => discover(countries).await,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub async fn resolve(&self) -> Result<Recipient, Error> { | |
| match self { | |
| Self::Exact(addr) => Ok((**addr).clone()), | |
| Self::Any => discover(&[]).await, | |
| Self::InCountries(countries) => discover(countries).await, | |
| } | |
| pub async fn resolve(&self) -> Result<Recipient, Error> { | |
| match self { | |
| Self::Exact(addr) => Ok((**addr).clone()), | |
| Self::Any => discover(&[]).await, | |
| Self::InCountries(countries) if countries.is_empty() => { | |
| Err(Error::NoCountriesSpecified) | |
| } | |
| Self::InCountries(countries) => discover(countries).await, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/rust/nym-sdk/src/mixnet/socks5_discovery.rs` around lines 93 - 98, The
resolve logic in Recipient/NetworkRequester currently treats InCountries with an
empty list the same as Any, which lets callers bypass the country restriction.
Update resolve() so the InCountries branch enforces the non-empty invariant
directly, either by rejecting empty vectors with an error or by delegating to
the same validation used by in_countries(), while keeping Exact and Any behavior
unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The name of this branch is misleading - the socks5 -> socks changes touch a lot of stuff, so I'm going to do that on a different branch. This branch just covers the addition of autodiscovery + country-based selection, + docs
Summary by CodeRabbit