QoL fix: TCP bind/fail before creation of client in socks5 - #6929
Draft
mfahampshire wants to merge 14 commits into
Draft
QoL fix: TCP bind/fail before creation of client in socks5 #6929mfahampshire wants to merge 14 commits into
mfahampshire wants to merge 14 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
mfahampshire
marked this pull request as draft
June 30, 2026 21:09
Contributor
📝 WalkthroughWalkthroughThe SOCKS5 client core now binds its TcpListener before starting the mixnet client and passes the listener explicitly into the server, removing internal binding logic. The nym-sdk introduces a NetworkRequester abstraction with country-based directory discovery, a new connect_with API, supporting error variants, and an updated example. ChangesSOCKS5 listener binding and network requester selection
Sequence Diagram(s)sequenceDiagram
participant Example
participant Socks5MixnetClient
participant NetworkRequester
participant Directory
participant ReqwestClient
Example->>NetworkRequester: any()
Example->>Socks5MixnetClient: connect_with(requester, bind)
Socks5MixnetClient->>NetworkRequester: resolve()
NetworkRequester->>Directory: query exit gateways
Directory-->>NetworkRequester: weighted candidates
NetworkRequester-->>Socks5MixnetClient: Recipient
Socks5MixnetClient->>Socks5MixnetClient: bind TcpListener, start mixnet client
Example->>ReqwestClient: GET https://nymtech.net via SOCKS5 proxy
ReqwestClient-->>Example: HTTP status
Example->>Socks5MixnetClient: disconnect()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 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/examples/socks5.rs (1)
17-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuote the address literals in the doc-comment examples.
exact(...)/connect_new(...)take string arguments (impl AsRef<str>), but the doc comment shows the Nym address as a bare unquoted token, which reads as invalid pseudo-code.✏️ Suggested fix
- // NetworkRequester::exact(Entztfv6Uaz2hpYHQJ6JKoaCTpDL5dja18SuQWVJAmmx.Cvhn9rBJw5Ay9wgHcbgCnVg89MPSV5s2muPV2YF1BXYu@Fo4f4SQLdoyoGkFae5TpVhRVoXCF8UiypLVGtGjujVPf)? -> a specific known requester - // If you already have the address, Socks5MixnetClient::connect_new(Entztfv6Uaz2hpYHQJ6JKoaCTpDL5dja18SuQWVJAmmx.Cvhn9rBJw5Ay9wgHcbgCnVg89MPSV5s2muPV2YF1BXYu@Fo4f4SQLdoyoGkFae5TpVhRVoXCF8UiypLVGtGjujVPf) is the one-line shorthand for the `exact` case. + // NetworkRequester::exact("Entztfv6Uaz2hpYHQJ6JKoaCTpDL5dja18SuQWVJAmmx.Cvhn9rBJw5Ay9wgHcbgCnVg89MPSV5s2muPV2YF1BXYu@Fo4f4SQLdoyoGkFae5TpVhRVoXCF8UiypLVGtGjujVPf")? -> a specific known requester + // If you already have the address, Socks5MixnetClient::connect_new("Entztfv6Uaz2hpYHQJ6JKoaCTpDL5dja18SuQWVJAmmx.Cvhn9rBJw5Ay9wgHcbgCnVg89MPSV5s2muPV2YF1BXYu@Fo4f4SQLdoyoGkFae5TpVhRVoXCF8UiypLVGtGjujVPf") is the one-line shorthand for the `exact` case.🤖 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/examples/socks5.rs` around lines 17 - 18, The doc-comment examples in socks5.rs show Nym addresses as bare tokens, but the referenced APIs expect string inputs. Update the examples in the comment near NetworkRequester::exact and Socks5MixnetClient::connect_new so the address literals are quoted like string arguments, keeping the examples aligned with the actual AsRef<str> signatures.
🤖 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_client.rs`:
- Around line 139-148: The `requester.resolve().await` call in
`socks5_client.rs` happens before the SOCKS5 bind is attempted, so discovery can
fail first and hide a deterministic bind error. Update the `connect` flow to
bind the selected or default address before calling `requester.resolve()` and
carry that bound listener/address through
`MixnetClientBuilder::new_ephemeral()`, `socks5_config(...)`, and
`connect_to_mixnet_via_socks5()`. Make sure the bind failure is returned
immediately when the port is occupied, while preserving the existing discovery
behavior afterward.
- Around line 275-280: Reject empty InCountries values inside
NetworkRequester::resolve so direct construction cannot bypass the guard and
fall back to Any. Update the match arm for Self::InCountries(countries) to
detect an empty list and return an error instead of calling discovery::discover,
while keeping the existing behavior for non-empty country lists and the
Exact/Any variants unchanged.
---
Nitpick comments:
In `@sdk/rust/nym-sdk/examples/socks5.rs`:
- Around line 17-18: The doc-comment examples in socks5.rs show Nym addresses as
bare tokens, but the referenced APIs expect string inputs. Update the examples
in the comment near NetworkRequester::exact and Socks5MixnetClient::connect_new
so the address literals are quoted like string arguments, keeping the examples
aligned with the actual AsRef<str> signatures.
🪄 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: 36014c8a-1552-4285-bdc7-24566c0e8b69
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
clients/socks5/src/main.rscommon/socks5-client-core/src/lib.rscommon/socks5-client-core/src/socks/server.rssdk/rust/nym-sdk/Cargo.tomlsdk/rust/nym-sdk/examples/socks5.rssdk/rust/nym-sdk/src/error.rssdk/rust/nym-sdk/src/mixnet.rssdk/rust/nym-sdk/src/mixnet/client.rssdk/rust/nym-sdk/src/mixnet/socks5_client.rs
Comment on lines
+139
to
+148
| let provider = requester.resolve().await?; | ||
| let mut socks5_config = Socks5::new(provider.to_string()); | ||
| if let Some(addr) = bind { | ||
| socks5_config.bind_address = addr; | ||
| } | ||
| MixnetClientBuilder::new_ephemeral() | ||
| .socks5_config(socks5_config) | ||
| .build()? | ||
| .connect_to_mixnet_via_socks5() | ||
| .await |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bind before requester discovery to keep bind failures deterministic.
requester.resolve().await can perform mainnet directory I/O before the SOCKS5 listener is bound later in connect_to_mixnet_via_socks5; if the port is occupied but discovery is slow or fails, callers get the wrong failure first. Bind the selected/default address before discovery and thread that listener into the SOCKS5 connect path.
🤖 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_client.rs` around lines 139 - 148, The
`requester.resolve().await` call in `socks5_client.rs` happens before the SOCKS5
bind is attempted, so discovery can fail first and hide a deterministic bind
error. Update the `connect` flow to bind the selected or default address before
calling `requester.resolve()` and carry that bound listener/address through
`MixnetClientBuilder::new_ephemeral()`, `socks5_config(...)`, and
`connect_to_mixnet_via_socks5()`. Make sure the bind failure is returned
immediately when the port is occupied, while preserving the existing discovery
behavior afterward.
Comment on lines
+275
to
+280
| pub async fn resolve(&self) -> Result<Recipient, Error> { | ||
| match self { | ||
| Self::Exact(addr) => Ok(**addr), | ||
| Self::Any => discovery::discover(&[]).await, | ||
| Self::InCountries(countries) => discovery::discover(countries).await, | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject empty InCountries during resolution too.
Because NetworkRequester is public, callers can construct NetworkRequester::InCountries(Vec::new()) directly and bypass in_countries(); resolve() then treats it as Any.
Proposed fix
match self {
Self::Exact(addr) => Ok(**addr),
Self::Any => discovery::discover(&[]).await,
+ Self::InCountries(countries) if countries.is_empty() => {
+ Err(Error::NoCountriesSpecified)
+ }
Self::InCountries(countries) => discovery::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), | |
| Self::Any => discovery::discover(&[]).await, | |
| Self::InCountries(countries) => discovery::discover(countries).await, | |
| } | |
| pub async fn resolve(&self) -> Result<Recipient, Error> { | |
| match self { | |
| Self::Exact(addr) => Ok(**addr), | |
| Self::Any => discovery::discover(&[]).await, | |
| Self::InCountries(countries) if countries.is_empty() => { | |
| Err(Error::NoCountriesSpecified) | |
| } | |
| Self::InCountries(countries) => discovery::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_client.rs` around lines 275 - 280, Reject
empty InCountries values inside NetworkRequester::resolve so direct construction
cannot bypass the guard and fall back to Any. Update the match arm for
Self::InCountries(countries) to detect an empty list and return an error instead
of calling discovery::discover, while keeping the existing behavior for
non-empty country lists and the Exact/Any variants unchanged.
benedettadavico
modified the milestones:
Amsterdam - (mainnet release 21st july),
Bydgoszcz - (mainnet release 4th august)
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
2 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.
Only review once #6920 is merged and this has been rebased against
developThis change is
Summary by CodeRabbit
New Features
Bug Fixes