librqbit(dht): provide an option to set DhtConfig::listen_addr by chantra · Pull Request #591 · ikatson/rqbit

chantra · 2026-05-17T02:10:34Z

Summary: Currently, if not using PersistentDhtConfig, it is not possible to set the listen_addr of DhtConfig.

I feel this should rather be captured under a "dht_config: Option" field instead, but it would conflict with the existing pub dht_config: Option<PersistentDhtConfig> so this version does not attempt to change any of this.

Test Plan: cargo test

Summary: Currently, if not using `PersistentDhtConfig`, it is not possible to set the listen_addr of DhtConfig. I feel this should rather be captured under a "dht_config: Option<DhtConfig>" field instead, but it would conflict with the existing `pub dht_config: Option<PersistentDhtConfig>` so this version does not attempt to change any of this. Test Plan: `cargo test`

chantra · 2026-05-17T02:14:34Z

@ikatson your thoughts are welcome in how to make supporting "DhtConfig" with only one field. I can't think of a way to do it without breaking backward compatibility.

ikatson · 2026-05-19T07:04:19Z

It's fine to break, 9 isn't released anyway

chantra

ok, so I streamlined DhtConfig with the help of LLM. There is quite some changes in function signature which the commit description explains how to port forward.

The change in general LGTM, but I am saying that without having to care about backward compatibility, so happy to hear what you think.

Comment inline with some chunk of code worth highlighting.

One of the core change is that now a SocketAddr needs to be passed, not just a port, this comes with some slight behaviour change.

chantra · 2026-05-20T05:25:35Z

-                            if config.ipv4_only {
-                                // Force IPv4 if requested
-                                let port = r.addr.port();
-                                r.addr = SocketAddr::from(([0, 0, 0, 0], port));
-                            } else if r.addr.ip() == Ipv4Addr::UNSPECIFIED {


the behaviour here changes. My read is that before it would load up the SocketAddr from the persistence file.
If ipv4_only was set, it would force to Ipv4Addr::UNSPECIFIED. If ipv4_only was not set and Ipv4Addr::UNSPECIFIED was read from the file, it would upgrade to Ipv6Addr::UNSPECIFIED down below.

Now, there is no ipv4_only anymore, but if the r.addr() was not Ipv4Addr::UNSPECIFIED it would not get upgraded, otherwise, it would.

chantra · 2026-05-20T05:33:54Z

-            if let Some(port) = config.port {
-                if let Some(ref mut addr) = listen_addr {
-                    addr.set_port(port);
-                } else {
-                    listen_addr = Some(SocketAddr::from((Ipv6Addr::UNSPECIFIED, port)));
-                }
-            }


Then we arrive here.
If config.port was not set, we would reuse the listen_addr we read from the persistence file, not doing the v6 upgrade.
If config.port is set, we would update the port of the address we read from the file, or would bind to [::]:port

Now, we try to use the listen addr provided by the user, if None, we use what we read from the file.

I feel the outcome is quite similar, but provides more flexibility to the caller? Happy to align more with the original flow if this makes more sense.

chantra · 2026-05-20T05:35:41Z

    pub client_name_and_version: Option<String>,
 }

+impl Default for SessionOptions {


The Default impl is now needed because disable_dht and disable_dht_persistence are now equivalent to dht: Some(DhtSessionConfig::default()),

chantra · 2026-05-20T05:37:27Z

+    let dht = if opts.disable_dht {
+        None
+    } else {
+        let persistence = if opts.disable_dht_persistence {
+            None
+        } else {
+            Some(DhtPersistenceConfig::default())
+        };
+        Some(DhtSessionConfig {
+            bootstrap_addrs: dht_bootstrap_addrs,
+            listen_addr: None,
+            persistence,
+        })
+    };
+


Setting dht is now a bit more verbose than used to with independent fields, but maybe more explicit.

ikatson · 2026-05-26T13:26:34Z

I really like the refactor of multiple session options into one "dht" option!

There's some weirdness with addresses/ports/ipv4_only flag though. And if you are touching this, would be good to make this right at the same time.

It seems there should be some logic, possibly extracted into a function that handles all combinations of:

explicit listen address requested
ipv4_only set or not (this option on the session must be respected)
stored address in the routing table different from the requested one
suspected rqbit 8 upgrade

I never asked, but now thinking about how messy this logic is / would be - why would you need to set listen address in the first place? At least it was all simpler, although still messy, when the address was not configurable at all.

For context, the port+ipv4 was added in a [PR](https://github.com/ikatson/rqbit/pull/531]

ikatson

Left a comment for further discussion

chantra · 2026-05-26T17:35:01Z

I never asked, but now thinking about how messy this logic is / would be - why would you need to set listen address in the first place? At least it was all simpler, although still messy, when the address was not configurable at all.

Seems in the same vein as #531 . I have clients that run under docker and scheduled by nomad. The client is natted by docker and the port dynamically assigned at startup time. This allows me to start the DHT using a port that I know is opened so the client's DHT is accessible from outside.
In residential network, this is typically taken care of by upnp, but in this case this is not available.

Re ipv4-only. I would have to double-check, but it is not clear it was ever wired to DHT other than when explicitely building the session with a specific DhtPersistentConfig. I think this is the only invocation of DhtPersistentConfig. And the SessionOptions's ipv4_only is never wired in DhtPersistentConfig in

rqbit/crates/librqbit/src/session.rs

Line 572 in 61e189a

let pdht_config = opts.dht_config.take().unwrap_or_default();

ipv4_only set or not (this option on the session must be respected)

would providing an explicit SocketAddr solve this? Either one provides an IPv4/0.0.0.0 or a v6/[::]?

suspected rqbit 8 upgrade
In such case, the upgrade is to go from 0.0.0.0 -> [::], right?

stored address in the routing table different from the requested one

What is typically stored in that file, from looking at the code, it seems we don't care about the actual listen IP and default to using wildcard, upgrade it to [::] unless v4_only is set. Same here, would encoding the intent in a SocketAddr enough? Having it default to [::]:0?

ikatson · 2026-05-26T17:56:49Z

This allows me to start the DHT using a port that I know is opened so the client's DHT is accessible from outside.

But there was "port" exposed for that (not full listen addr) in the config. If you keep the change to just port + ipv4_only, then it seems it'll both fix your use-case, make the options saner, and keep existing (maybe not working as you say) ipv4_only feature which can be fixed as part of this PR.

So in short:

compute IpAddr from ipv4_only flag, ignore the stored one completely
use port in this order explicit port -> stored port -> new random port

chantra · 2026-05-26T18:14:05Z

But there was "port" exposed for that (not full listen addr) in the config.

yes, port was available, but only for persistent dht config, not plain dht config.

The benefit of listen_addr over just port is that you can define ip:port instead of *:port, which can be used to restrict how widely the port can be accessed. I don't have a strong need for specifying an IP in my case, so no strong opinions on that.

ikatson · 2026-05-26T18:25:24Z

Exactly, if no-one needs this let's keep it simple.
Bind device is there for limiting if needed.
Is there's an actual usecase ever where a single interface has multiple ips we can reconsider

ikatson · 2026-05-27T08:37:50Z

yes, port was available, but only for persistent dht config, not plain dht config.

Just to clarify, my suggestion is to fix it in this PR as you are changing the config options entirely

chantra · 2026-05-27T13:33:23Z

Sure, that's what I understood.

chantra · 2026-05-27T21:24:26Z

                    ) {
-                        Ok(mut r) => {
+                        Ok(r) => {
                            info!(filename=?config_filename, "loaded DHT routing table from");


is there any cases where this may not be ipv4/ipv6::unspecified? If not, then it seems it is much easier to decide on [::] or 0.0.0.0 based on the ipv4_only value.
The warn! is removed, I can add it back if needed, just wondering if this is worth the if/else gymnastic.

I think the only possible values are Ipv*Addr::UNSPECIFIED.

ok, this is now handled in dht_listen_addr based off of ipv4_only value.

chantra · 2026-05-27T21:25:52Z

-                    listen_addr = Some(SocketAddr::from((Ipv6Addr::UNSPECIFIED, port)));
-                }
-            }
+            let listen_addr = dht_listen_addr(port, stored_port, ipv4_only);


this takes care of ientifying if [::] or 0.0.0.0 should be used, as well as selecting the port to use based on the "explicit port -> stored port -> new random port" logic.

chantra · 2026-05-27T21:28:15Z

+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn no_port_no_stored_v6() {
+        let addr = dht_listen_addr(None, None, false);
+        assert_eq!(addr.ip(), IpAddr::from(Ipv6Addr::UNSPECIFIED));
+        assert_eq!(addr.port(), 0);
+    }
+
+    #[test]
+    fn no_port_no_stored_v4() {
+        let addr = dht_listen_addr(None, None, true);
+        assert_eq!(addr.ip(), IpAddr::from(Ipv4Addr::UNSPECIFIED));
+        assert_eq!(addr.port(), 0);
+    }
+
+    #[test]
+    fn explicit_port_v6() {
+        let addr = dht_listen_addr(Some(6881), None, false);
+        assert_eq!(addr.ip(), IpAddr::from(Ipv6Addr::UNSPECIFIED));
+        assert_eq!(addr.port(), 6881);
+    }
+
+    #[test]
+    fn explicit_port_v4() {
+        let addr = dht_listen_addr(Some(6881), None, true);
+        assert_eq!(addr.ip(), IpAddr::from(Ipv4Addr::UNSPECIFIED));
+        assert_eq!(addr.port(), 6881);
+    }
+
+    #[test]
+    fn stored_port_only_v6() {
+        let addr = dht_listen_addr(None, Some(12345), false);
+        assert_eq!(addr.ip(), IpAddr::from(Ipv6Addr::UNSPECIFIED));
+        assert_eq!(addr.port(), 12345);
+    }
+
+    #[test]
+    fn stored_port_only_v4() {
+        let addr = dht_listen_addr(None, Some(12345), true);
+        assert_eq!(addr.ip(), IpAddr::from(Ipv4Addr::UNSPECIFIED));
+        assert_eq!(addr.port(), 12345);
+    }
+
+    #[test]
+    fn explicit_overrides_stored_v6() {
+        let addr = dht_listen_addr(Some(6881), Some(12345), false);
+        assert_eq!(addr.ip(), IpAddr::from(Ipv6Addr::UNSPECIFIED));
+        assert_eq!(addr.port(), 6881);
+    }
+
+    #[test]
+    fn explicit_overrides_stored_v4() {
+        let addr = dht_listen_addr(Some(6881), Some(12345), true);
+        assert_eq!(addr.ip(), IpAddr::from(Ipv4Addr::UNSPECIFIED));
+        assert_eq!(addr.port(), 6881);
+    }
+}


probably a good candidate for table-driven tests. I have used rstest in the past for that. Lemme now if you are fine with the added dependency.

chantra · 2026-05-27T21:30:32Z

+                    PersistentDht::create(
+                        persistence_config,
+                        dht_config.port,
+                        opts.ipv4_only,


this was missing before, it is now wired.

chantra · 2026-05-27T21:32:01Z

+                    let listen_addr = dht_listen_addr(dht_config.port, None, opts.ipv4_only);
                    DhtBuilder::with_config(DhtConfig {
-                        bootstrap_addrs: opts.dht_bootstrap_addrs.clone(),
+                        bootstrap_addrs: dht_config.bootstrap_addrs,
                        cancellation_token: Some(token.child_token()),
                        bind_device: bind_device.as_ref(),
+                        listen_addr: Some(listen_addr),


hum, I don't think dht_listen_addr should leak here. I should move this within DhtBuilder probably, let me take another look.

actually, this is prior art. The plus side is that it would now obey ipv4_only.

ikatson · 2026-06-02T07:04:05Z

                    ) {
-                        Ok(mut r) => {
+                        Ok(r) => {
                            info!(filename=?config_filename, "loaded DHT routing table from");


I think the only possible values are Ipv*Addr::UNSPECIFIED.

ikatson · 2026-06-02T07:05:48Z

        ..Default::default()
    });

+    let dht_bootstrap_addrs = opts


would be nice to move this block under "else" so that it doesn't do anything if dht is disabled

done, this is the only change in the latest force-pushed version https://github.com/ikatson/rqbit/compare/46cf7b17eb5126010a47e7e5ea38da875d7acf13..a476b3cac94923153ca8d8ea0c1bcc93810846bc

Replace the scattered DHT-related fields in SessionOptions with a single `dht: Option<DhtSessionConfig>` field. This eliminates the `disable_dht`, `disable_dht_persistence`, `dht_config`, `dht_bootstrap_addrs`, and `dht_listen_addr` fields in favor of a structured config where `None` means DHT is disabled. Also renames `PersistentDhtConfig` to `DhtPersistenceConfig` and drops its `port` and `ipv4_only` fields. The DHT now exposes a simple `port: Option<u16>` instead of a full `SocketAddr`. The bind IP is derived from `SessionOptions::ipv4_only` (`0.0.0.0` if true, `[::]` otherwise); use `SessionOptions::bind_device_name` to scope the bind to a specific interface. Port resolution priority (extracted into the public `dht_listen_addr` helper for both the persistent and non-persistent paths): explicit `DhtSessionConfig::port` -> persisted port -> random The persisted IP from the routing-table file is now ignored entirely, which also makes the rqbit 8 -> 9 `0.0.0.0 -> [::]` upgrade implicit. This fixes a latent bug where `SessionOptions::ipv4_only` was never actually wired into DHT init. Migration guide: Before: SessionOptions { disable_dht: false, disable_dht_persistence: false, dht_config: Some(PersistentDhtConfig { config_filename: Some(path), ..Default::default() }), dht_bootstrap_addrs: Some(vec!["host:port".into()]), dht_listen_addr: Some(addr), ipv4_only: true, .. } After: SessionOptions { dht: Some(DhtSessionConfig { bootstrap_addrs: Some(vec!["host:port".into()]), port: Some(6881), persistence: Some(DhtPersistenceConfig { config_filename: Some(path), ..Default::default() }), }), ipv4_only: true, // now actually applied to DHT .. } To disable DHT entirely: SessionOptions { dht: None, .. } To disable only DHT persistence: SessionOptions { dht: Some(DhtSessionConfig { persistence: None, ..Default::default() }), .. } Test Plan: cargo test (incl. 8 new unit tests covering the explicit/stored/random x ipv4/ipv6 matrix for dht_listen_addr), cargo clippy --all-targets, cargo fmt --check all pass. Desktop app (rqbit-desktop) also compiles cleanly.

chantra

ok. addressed your comment. LMK if there is anything that still stands out.
Happy to rebase if needed.

chantra · 2026-06-02T16:20:03Z

                    ) {
-                        Ok(mut r) => {
+                        Ok(r) => {
                            info!(filename=?config_filename, "loaded DHT routing table from");


ok, this is now handled in dht_listen_addr based off of ipv4_only value.

chantra · 2026-06-02T16:40:50Z

        ..Default::default()
    });

+    let dht_bootstrap_addrs = opts


done, this is the only change in the latest force-pushed version https://github.com/ikatson/rqbit/compare/46cf7b17eb5126010a47e7e5ea38da875d7acf13..a476b3cac94923153ca8d8ea0c1bcc93810846bc

chantra commented May 20, 2026

View reviewed changes

ikatson requested changes May 26, 2026

View reviewed changes

chantra force-pushed the dht_config_port branch from d0b9285 to 46cf7b1 Compare May 27, 2026 21:20

chantra commented May 27, 2026

View reviewed changes

chantra requested a review from ikatson June 1, 2026 16:29

ikatson approved these changes Jun 2, 2026

View reviewed changes

chantra force-pushed the dht_config_port branch from 46cf7b1 to a476b3c Compare June 2, 2026 16:39

chantra commented Jun 2, 2026

View reviewed changes

Conversation

chantra commented May 17, 2026

Uh oh!

chantra commented May 17, 2026

Uh oh!

ikatson commented May 19, 2026

Uh oh!

chantra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikatson commented May 26, 2026

Uh oh!

ikatson left a comment

Choose a reason for hiding this comment

Uh oh!

chantra commented May 26, 2026

Uh oh!

ikatson commented May 26, 2026

Uh oh!

chantra commented May 26, 2026

Uh oh!

ikatson commented May 26, 2026

Uh oh!

ikatson commented May 27, 2026

Uh oh!

chantra commented May 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chantra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants