Skip to content

fix(jd_client): handle invalid tp_address gracefully instead of panicking#213

Open
jayvaliya wants to merge 1 commit into
dmnd-pool:masterfrom
jayvaliya:fix/tp-address-validation
Open

fix(jd_client): handle invalid tp_address gracefully instead of panicking#213
jayvaliya wants to merge 1 commit into
dmnd-pool:masterfrom
jayvaliya:fix/tp-address-validation

Conversation

@jayvaliya
Copy link
Copy Markdown

Closes #212

Description:


Summary

Fixes two gaps in error handling for --tp-address:

  1. config.rs — Added validate_tp_address() called at config load time, giving users an immediate clear error on startup (mirrors the existing validate_miner_name() pattern).

  2. jd_client/mod.rs — Replaced three panicking .expect() calls (lines 110–111, 208–209) with graceful match + return None, consistent with the 15 other error paths already in initialize_jd().


Changes

File What changed
src/config.rs Added validate_tp_address() + call after loading tp_address
src/jd_client/mod.rs Replaced .expect() with match + return None

@jayvaliya
Copy link
Copy Markdown
Author

@Priceless-P, could you please review when you have a moment?

Copy link
Copy Markdown
Contributor

@jbesraa jbesraa left a comment

Choose a reason for hiding this comment

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

thanks for looking into this. two things:

  1. maybe it is best to do SocketAddr::from_str instead of the split and then extract ip/addr, or even better - make the code use SocketAddr all along
  2. (bonus) when we extract the address we should validate the node is reachable

@jayvaliya jayvaliya force-pushed the fix/tp-address-validation branch from 39ac408 to e743923 Compare April 6, 2026 11:17
@jayvaliya
Copy link
Copy Markdown
Author

Hi @jbesraa, thank you for the earlier feedback.

I have addressed the requested changes:

  1. Switched tp_address handling to SocketAddr end-to-end.
  2. Removed manual split/parsing logic and now use direct SocketAddr flow in the JD path.
  3. Added a startup reachability check for the TP address (connect timeout).
  4. Rebased the branch on the latest upstream master and re-verified with cargo check.

Could you please review again when you have a moment?
Thank you.

@jayvaliya jayvaliya requested a review from jbesraa April 7, 2026 08:58
Copy link
Copy Markdown
Contributor

@jbesraa jbesraa left a comment

Choose a reason for hiding this comment

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

Thanks @jayvaliya
Just a added a couple of nits

Comment thread src/config.rs Outdated
.or_else(|| std::env::var("TP_ADDRESS").ok());
let tp_address = tp_address.map(|tp| {
let addr = tp.parse::<std::net::SocketAddr>().unwrap_or_else(|e| {
eprintln!("Invalid TP address '{tp}': {e}. Expected format: 'ip:port' (e.g., '127.0.0.1:8442')");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
eprintln!("Invalid TP address '{tp}': {e}. Expected format: 'ip:port' (e.g., '127.0.0.1:8442')");
error!("Invalid TP address '{tp}': {e}. Expected format: 'ip:port' (e.g., '127.0.0.1:8442')");

Comment thread src/config.rs Outdated
std::process::exit(1);
});
if let Err(e) = std::net::TcpStream::connect_timeout(&addr, std::time::Duration::from_secs(3)) {
eprintln!("Warning: TP address '{addr}' is not reachable: {e}");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
eprintln!("Warning: TP address '{addr}' is not reachable: {e}");
error!("Error: TP address '{addr}' is not reachable: {e}");

hmm dont we need to process:exit here as well?

@jayvaliya jayvaliya requested a review from jbesraa April 8, 2026 09:28
@jbesraa
Copy link
Copy Markdown
Contributor

jbesraa commented Apr 8, 2026

please also squash the changes into a single commit @jayvaliya

@jayvaliya
Copy link
Copy Markdown
Author

Thanks for the reminder, and sorry about the premature review request. I accidentally clicked it before applying your requested changes. I’ll make the updates and re-request review once they’re pushed.

@jbesraa
Copy link
Copy Markdown
Contributor

jbesraa commented Apr 8, 2026

BTW one thing to note here, if the user enters template provider address and it is invalid(or inaccessible), we NEED to exit and print an error. if the user does not enter TP address at all, we should allow it(in that case proxy is not doing JD)

@jayvaliya jayvaliya force-pushed the fix/tp-address-validation branch from e743923 to 059b547 Compare April 8, 2026 10:32
@jayvaliya
Copy link
Copy Markdown
Author

Hi @jbesraa, thank you again for your detailed feedback and patience.

I’ve addressed all requested changes, pushed the updates, and squashed the PR into a single commit. Please let me know if any further changes are needed.

I’d appreciate your review when convenient.

@jayvaliya jayvaliya force-pushed the fix/tp-address-validation branch from 059b547 to 83e3188 Compare April 9, 2026 08:18
@jayvaliya
Copy link
Copy Markdown
Author

Hi @jbesraa, I’ve pushed the latest updates and resolved the issues with all three workflow commands:

  • cargo fmt --all -- --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test --all-features

Everything should be working properly now. Happy to make further adjustments if needed.

@jayvaliya jayvaliya force-pushed the fix/tp-address-validation branch 2 times, most recently from 611a282 to 9b4787e Compare April 14, 2026 06:08
@jayvaliya
Copy link
Copy Markdown
Author

Hi @Priceless-P, @jbesraa just a gentle follow-up when you have a moment. I’ve addressed the feedback, rebased it on the latest master, and re-checked the workflow commands. I’d appreciate your review whenever you have time.

@jayvaliya jayvaliya force-pushed the fix/tp-address-validation branch from 9b4787e to 87f2dda Compare April 18, 2026 19:28
…king

Closes dmnd-pool#212

- Switch tp_address from String to SocketAddr end-to-end
- Validate TP address format at startup with clear error message
- Exit with error if TP address is invalid or unreachable
- Remove manual split/parsing logic in JD client
- Allow proxy to run without TP address (non-JD mode)
@jayvaliya jayvaliya force-pushed the fix/tp-address-validation branch from 87f2dda to d97e18e Compare April 20, 2026 09:52
@jayvaliya
Copy link
Copy Markdown
Author

Updated library_init.rs to match the tp_address type change

The CI was failing on cargo clippy and cargo test because Configuration::new() now expects Option<SocketAddr> instead of Option<String> (changed in config.rs), but the test was still passing Some(tp_sniffer_addr.to_string()).

Fixed by changing to Some(tp_sniffer_addr) since tp_sniffer_addr is already a SocketAddr.

- Some(tp_sniffer_addr.to_string()),
+ Some(tp_sniffer_addr),

This was the only remaining cause of the clippy/test failures — cargo fmt, cargo clippy, and cargo test all pass now.

This additional change to library_init.rs was needed to align the test with the Option<String>Option<SocketAddr> refactor. The test file was added in #215, so it wasn't part of the original PR diff.

@jayvaliya
Copy link
Copy Markdown
Author

Hi @jbesraa, everything’s good now, no conflicts and all checks are passing. Appreciate a review when you have a moment.

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.

Invalid --tp-address causes proxy to panic instead of failing gracefully

2 participants