Add CLI and env var config support, split `bitcoind_rpc_addr` into host/port #69

moisesPompilio · 2025-08-21T12:59:53Z

Added support for configuring the node via CLI arguments and environment variables, using clap (as already done in ldk-server-cli).

Configuration is now loaded from three sources:

Config file (most complete set of options)
Environment variables (essential options only)
CLI arguments (essential options only)

Environment variables and CLI arguments override values defined in the config file when provided.

In addition, the bitcoind_rpc_addr field was split into two variables for better clarity:

bitcoind_rpc_host
bitcoind_rpc_port

Tests were added for the new configuration loading logic, and the README was updated with usage instructions.

close #42 and #66

ldk-reviews-bot · 2025-08-21T12:59:55Z

👋 Thanks for assigning @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Anyitechs · 2025-08-22T13:42:14Z

Thank you for your work on this!

I'm still going through the changes, but I have an early high-level question. Is there a reason or trade-off why the clap crate is preferred over the config crate for this?

My initial thought was that a crate like config might be a better fit here, since it's specifically designed for layered configuration and helps separate config loading from CLI argument parsing.

Just want to make sure I understand the design decision.

moisesPompilio · 2025-08-23T01:51:36Z

I chose to use clap because it was already a dependency in ldk-server-cli and it provides built-in support for both CLI arguments and environment variables. This made it convenient to extend ldk-server without introducing a new dependency, so clap seemed like a good fit for this use case.

I hadn’t considered using a separate configuration crate like config for this, so I didn’t research that option.

ldk-reviews-bot · 2025-08-23T13:11:02Z

🔔 1st Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-08-25T13:11:45Z

🔔 2nd Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz

Thanks for tackling these issues! Looks like CI is failing, though.

jkczyz · 2025-08-25T23:32:22Z

ldk-server/src/util/config.rs

+		None
+	};
+
+	macro_rules! pick {


TBH, the call sites may be more readable if everything was explicitly written out. There isn't much happening here, so I don't think it would be too verbose.

Got it, I removed the picks macro and wrote the calls explicitly.

jkczyz · 2025-08-25T23:39:33Z

ldk-server/src/util/config.rs

-	rpc_address: String,
-	rpc_user: String,
-	rpc_password: String,
+	rpc_host: Option<String>,
+	rpc_port: Option<u16>,
+	rpc_user: Option<String>,
+	rpc_password: Option<String>,


Could you make a separate commit for splitting the RPC address?

Yes, sorry, I forgot to split it into two commits.

jkczyz · 2025-08-25T23:44:18Z

ldk-server/src/util/config.rs

+
+	macro_rules! pick {
+		($cli:expr, $toml:expr, $err_msg:expr) => {
+			$cli.or($toml).ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, $err_msg))


jkczyz · 2025-08-26T00:18:02Z

ldk-server/src/util/config.rs

+		};
+	}
+
+	fn missing_field_msg(field: &str) -> String {


Could this return the io::Error instead? Then the unrolled macro would be less verbose.

Yes, I removed the macro and made the code explicit. The function now directly returns io::Error

moisesPompilio · 2025-09-01T19:55:20Z

Looks like CI is failing, though.

CI was failing because tests used the same file names. I fixed it by giving each test a unique file name.

Anyitechs

Thanks again for your work on this! The changes look good overall, but I left a few comments for consideration.

Anyitechs · 2025-09-02T02:10:10Z

ldk-server/src/util/config.rs

-			format!("Failed to read config file '{}': {}", config_path.as_ref().display(), e),
-		)
+#[derive(Parser, Debug)]
+#[command(version, about = "LDK Node Configuration", long_about = None)]


nit: would it be appropriate to use 'LDK Server Configuration' here instead of ‘LDK Node Configuration’?

Anyitechs · 2025-09-02T02:24:06Z

ldk-server/src/util/config.rs

+				format!("Config file contains invalid TOML format: {}", e),
+			)
+		})?)
+	} else {


Do we really need the else {} block here? Since we're primarily interested in the Some() case, and considering the events-rabbitmq and experimental-lsps2-support features are already being handled in load_config_feature(), this block might be redundant.

The else {} is needed to handle the None case and also manage features when they're passed. I replaced if with match to make the options clearer.

Anyitechs · 2025-09-02T02:38:22Z

ldk-server/src/util/config.rs

+	storage_dir_path: Option<String>,
+}
+
+pub fn load_config(args_config: &ArgsConfig) -> io::Result<Config> {


The load_config() function seems to be doing quite a lot at the moment. It might be beneficial to implement TryFrom<TomlConfig> for Config and move all the logic after the if let Some() block into the try_from() method, similar to how it was structured before.

I see your point.TryFrom<TomlConfig>made sense when the goal was a direct conversion from TOML to Config. Now that load_config() merges values from both TomlConfig and ArgsConfig, the logic is more about combining sources than just converting one type. In this case, keeping it in load_config() seems more consistent.

jkczyz

Sorry about the delay. Some travel got the best of my time.

jkczyz · 2025-09-08T16:56:48Z

ldk-server/src/main.rs

-	let bitcoind_rpc_addr = config_file.bitcoind_rpc_addr;
-
 	builder.set_chain_source_bitcoind_rpc(
-		bitcoind_rpc_addr.ip().to_string(),
-		bitcoind_rpc_addr.port(),
+		config_file.bitcoind_rpc_addr.ip().to_string(),
+		config_file.bitcoind_rpc_addr.port(),


Does this belong in the second commit?

Yeah, done!

jkczyz · 2025-09-08T17:01:05Z

ldk-server/src/util/config.rs

-	node: NodeConfig,
-	storage: StorageConfig,
-	bitcoind: BitcoindConfig,
+	node: Option<NodeConfig>,
+	storage: Option<StorageConfig>,
+	bitcoind: Option<BitcoindConfig>,


I don't have a good enough grasp of this change to understand why these need to be Options now. Same elsewhere. Could you explain?

Previously these configs were only provided via file. I made them Option so they’re not required in the file anymore, since they can now also be passed via CLI or env. This way, the user can decide to provide some settings only through CLI/env or file, while they’re still required at node startup.

Ah, I see. So long Config isn't using Options then this should be ok. (cc: @tnull in case he has any opinons on the matter).

I think it might make sense to rename these structs, since they’re only used for the TOML file. Adding a suffix or prefix to make that explicit could make the code clearer.

jkczyz · 2025-09-08T17:06:12Z

ldk-server/src/util/config.rs

+	let network = match args_config.node_network.or(node.and_then(|n| n.network)) {
+		Some(n) => n,
+		None => return Err(missing_field_err("network")),
+	};


Instead of matching like this, let's use ok_or_else()? and map when needed. Likewise elsewhere.

jkczyz · 2025-09-08T17:06:51Z

ldk-server/src/util/config.rs

+	let listening_addr_str = match args_config
+		.node_listening_address
+		.as_deref()
+		.or(node.and_then(|n| n.listening_address.as_deref()))
+	{
+		Some(addr) => addr,
+		None => return Err(missing_field_err("node_listening_address")),
+	};
+	let listening_addr = SocketAddress::from_str(listening_addr_str).map_err(|e| {
+		io::Error::new(io::ErrorKind::InvalidInput, format!("Invalid listening address: {}", e))
 	})?;


I think we can combine these using and_then?

jkczyz · 2025-09-08T19:24:29Z

ldk-server/src/util/config.rs

-	node: NodeConfig,
-	storage: StorageConfig,
-	bitcoind: BitcoindConfig,
+	node: Option<NodeConfig>,
+	storage: Option<StorageConfig>,
+	bitcoind: Option<BitcoindConfig>,


Ah, I see. So long Config isn't using Options then this should be ok. (cc: @tnull in case he has any opinons on the matter).

jkczyz · 2025-09-08T19:27:05Z

ldk-server/src/util/config.rs

+				toml_config = remove_config_line(&toml_config, &format!("{} =", $field));
+				fs::write(storage_path.join(config_file_name), &toml_config).unwrap();
+				let result = load_config(&args_config);
+				println!("Rsult: {:?}", result);


s/Rsult/Result

But why print in tests?

You're right, the print isn't needed. I had added it temporarily for debugging while checking values, but I've removed it now.

- Added `clap` dependency to `ldk-server` to support passing essential node config via CLI arguments and environment variables. - Implemented layered config loading: config file (full set of options) + environment variables + CLI arguments. Env vars and CLI args override values from the config file when present. - Added tests for the new config loading logic. - Updated README with usage instructions and explanation of config precedence. Close lightningdevkit#42

…pc_port` This change replaces the single RPC address field with separate host and port fields, allowing hostname support and improving compatibility with containerized environments. Closes lightningdevkit#66 juntar com o segundo commit

ldk-reviews-bot requested a review from jkczyz August 21, 2025 13:10

jkczyz reviewed Aug 26, 2025

View reviewed changes

moisesPompilio force-pushed the issue-42 branch 2 times, most recently from d05dec0 to 96a5676 Compare September 1, 2025 19:20

Anyitechs reviewed Sep 2, 2025

View reviewed changes

moisesPompilio force-pushed the issue-42 branch from 96a5676 to 7cf7e98 Compare September 2, 2025 14:00

moisesPompilio requested a review from jkczyz September 8, 2025 12:42

jkczyz reviewed Sep 8, 2025

View reviewed changes

moisesPompilio force-pushed the issue-42 branch from 7cf7e98 to a93ee9f Compare September 8, 2025 18:26

jkczyz reviewed Sep 9, 2025

View reviewed changes

moisesPompilio added 2 commits September 9, 2025 11:59

moisesPompilio force-pushed the issue-42 branch from a93ee9f to 43d1a52 Compare September 9, 2025 16:33

Add CLI and env var config support, split bitcoind_rpc_addr into host/port #69

Are you sure you want to change the base?

Add CLI and env var config support, split bitcoind_rpc_addr into host/port #69

Uh oh!

Conversation

moisesPompilio commented Aug 21, 2025

Uh oh!

ldk-reviews-bot commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Anyitechs commented Aug 22, 2025

Uh oh!

moisesPompilio commented Aug 23, 2025

Uh oh!

ldk-reviews-bot commented Aug 23, 2025

Uh oh!

ldk-reviews-bot commented Aug 25, 2025

Uh oh!

jkczyz 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!

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!

moisesPompilio commented Sep 1, 2025

Uh oh!

Anyitechs 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Add CLI and env var config support, split `bitcoind_rpc_addr` into host/port #69

Add CLI and env var config support, split `bitcoind_rpc_addr` into host/port #69

ldk-reviews-bot commented Aug 21, 2025 •

edited

Loading

jkczyz left a comment •

edited

Loading