Added validate_domain in electrum option by Rahamath-unnisa · Pull Request #223 · bitcoindevkit/bdk-cli

Rahamath-unnisa · 2025-10-20T15:12:27Z

Description:
This PR adds the validate_domain option to the Electrum client in bdk-cli.
This resolves #134 .
Changes included:

Added --validate-domain flag in command.rs.

Updated Electrum client handling in handlers.rs and utils.rs to respect the flag.

Verified functionality locally using:

RUST_LOG=debug,rusqlite=info,rustls=info cargo run --features electrum -- wallet --client-type electrum --database-type sqlite --url ssl://electrum.blockstream.info:60002 --validate-domain true --ext-descriptor "wpkh(.../*)" sync

Motivation:
Allows users to enable or disable domain validation for Electrum servers, useful for self-hosted/custom servers while preserving security by default.

Checklist:

Tested locally with Electrum client

No unrelated changes included

va-an · 2025-10-21T08:53:54Z

@Rahamath-unnisa pls mention in PR description that this resolves #134.

110CodingP · 2025-10-21T09:46:49Z

src/commands.rs


-#![allow(clippy::large_enum_variant)]
+
+#[allow(clippy::large_enum_variant)]


ig this is not required.

110CodingP · 2025-10-21T10:20:39Z

src/handlers.rs

 //!
 //! This module describes all the command handling logic used by bdk-cli.
-
+ use crate::debug;


ig this is not needed either.

110CodingP · 2025-10-21T10:22:35Z

src/commands.rs

    },
    /// Syncs with the chosen blockchain server.
-    Sync,
-    /// Broadcasts a transaction to the network. Takes either a raw transaction or a PSBT to extract.


This doc comment should not be removed.

110CodingP · 2025-10-21T10:30:29Z

src/handlers.rs

                        .populate_tx_cache(wallet.tx_graph().full_txs().map(|tx_node| tx_node.tx));

-                    let update = client.sync(request, batch_size, false)?;
+                    let update = client.sync(request, batch_size, validate_domain)?;


ig this is not the right place to pass validate_domain looking at the defn here. I think we need to change how the client is built using this.

Rahamath-unnisa · 2025-10-23T11:02:02Z

Hi @110CodingP,
I have made the requested cleanups in commands.rs and handlers.rs per your review comments
All changes are pushed to the validate_domain-config branch.
Please let me know if anything else is needed. Thanks!

tvpeter

Hi @Rahamath-unnisa,
Thank you for the eagerness to contribute, but please understand the project fully before attempting to do so as it is easier to make progress that way.
Thank you.

tvpeter · 2025-11-03T10:05:16Z

Cargo.toml

 shlex = {  version = "1.3.0", optional = true }
 tracing = "0.1.41"
 tracing-subscriber = "0.3.20"
+electrum-client = "0.24.0"


bdk_electrum is built on this crate, so no need pulling it here.

tvpeter · 2025-11-03T10:06:01Z

src/commands.rs

    #[cfg(feature = "electrum")]
    #[arg(env = "ELECTRUM_BATCH_SIZE", short = 'b', long, default_value = "10")]
    pub batch_size: usize,
+    ///Electrum validate domain option.


this comment is not needed

tvpeter · 2025-11-03T10:06:37Z

src/commands.rs

+    ///Electrum validate domain option.
+    #[cfg(feature = "electrum")]
+    #[arg(env="VALIDATE_DOMAIN",long = "validate-domain", action = clap::ArgAction::Set, default_value_t = true)]
+    pub validate_domain: bool,


a user do not need to specifically indicate whether the domain should be validated

sorry if I misunderstood but doesn't the issue want us to provide validate_domain as an option? Because otherwise config.validate_domain is true by default anyways.

va-an

Could you rebase and squash the commits into one, please?

Rahamath-unnisa · 2026-03-28T12:28:12Z

Hi @tvpeter, @110CodingP, @va-an,

I’ve updated the PR based on your feedback:

Removed CLI option for validate_domain
Enabled validation internally
Cleaned up code and dependencies
Rebased and resolved conflicts

Please review again. Thanks!

va-an

Make sure you've done the rebase correctly.
Also, I'll repeat my request to squash into a single commit.

va-an · 2026-03-28T21:03:56Z

Cargo.toml

What's the idea behind these changes?
Moreover, because of these changes, Cargo.toml has become invalid:

-> % just pre-push error: failed to parse manifest at `/home/vaan/src/bitcoindevkit/bdk-cli-tmp/Cargo.toml` Caused by: feature `_payjoin-dependencies` includes `payjoin` which is neither a dependency nor another feature error: Recipe `pre-push` failed on line 29 with exit code 101

va-an · 2026-03-28T21:05:53Z

src/handlers.rs

+<<<<<<< HEAD
+<<<<<<< HEAD
+=======


Suggested change

<<<<<<< HEAD

<<<<<<< HEAD

=======

va-an · 2026-03-28T21:06:10Z

src/handlers.rs

+>>>>>>> 7931dbd (Added validate_domain in electrum option)
+=======
+>>>>>>> a2f5954 (Clean up commands.rs and handlers.rs per review comments)


Suggested change

>>>>>>> 7931dbd (Added validate_domain in electrum option)

=======

>>>>>>> a2f5954 (Clean up commands.rs and handlers.rs per review comments)

va-an · 2026-03-28T21:06:27Z

src/utils.rs

+use std::fmt::Display;
+use std::path::{Path, PathBuf};
+use std::str::FromStr;
+>>>>>>> 80e132f (Added validate-domain option config in utils.rs and made changes required to that.)


Suggested change

>>>>>>> 80e132f (Added validate-domain option config in utils.rs and made changes required to that.)

va-an

Also, please fill out the PR template provided in this repo - it includes checklists that help catch issues like, you know, the code not actually compiling.

Rahamath-unnisa · 2026-03-29T15:04:31Z

Hi @va-an,

Thanks for your feedback.

I’ve now:

Squashed all changes into a single commit
Fixed Cargo.toml by restoring upstream version
Removed leftover conflict markers and cleaned up code

Please review again. Thanks!

tvpeter

hi @Rahamath-unnisa, thank you for continuing to work on this issue. Below are some of the observations I have on this PR:

I have noticed that the primary change in this PR is changing the electrum_client::Client::new(url) to electrum_client::Client::from_config(url, config) where the config is still default.
Looking at the electrum_client code, there seems to be no functional difference between the two approaches, as the new internally returns the from_config with the same defaults.
Secondly, my initial thoughts about the issue was that, maybe it was meant to handle the parsing and validation of the supplied url string. If this was the intent of the issue, I think it is best it were moved to the electrum_client codebase.
lastly, as I commented earlier, adding another flag will rather complicate the user experience by having too many flags to accompany each (sub)command.

Therefore, I am of the opinion that, this PR should either be closed or moved to the electrum_client codebase. I will wait to hear others' opinions before deciding the best course of action.
Thank you so much for your effort.

va-an · 2026-03-31T18:12:44Z

@tvpeter I would prefer to close this.

The code still doesn't compile, the PR template wasn't used despite being asked, and most of the changes are unrelated to the issue.

If the use case is still relevant, it can be revisited as a fresh PR.

github-project-automation bot added this to BDK-CLI Oct 20, 2025

110CodingP reviewed Oct 21, 2025

View reviewed changes

110CodingP suggested changes Oct 21, 2025

View reviewed changes

Rahamath-unnisa force-pushed the validate_domain-config branch from d18f668 to 31a3fbf Compare October 30, 2025 13:25

Rahamath-unnisa requested a review from 110CodingP October 30, 2025 13:25

tvpeter requested changes Nov 3, 2025

View reviewed changes

Rahamath-unnisa requested a review from tvpeter November 4, 2025 16:56

va-an suggested changes Feb 22, 2026

View reviewed changes

tvpeter assigned Rahamath-unnisa Feb 23, 2026

Rahamath-unnisa force-pushed the validate_domain-config branch from 818a744 to c697136 Compare March 28, 2026 12:26

va-an suggested changes Mar 28, 2026

View reviewed changes

fix(electrum): always enable domain validation internally

546948e

Rahamath-unnisa force-pushed the validate_domain-config branch from 1fc13e4 to 546948e Compare March 29, 2026 15:02

tvpeter reviewed Mar 31, 2026

View reviewed changes


		#![allow(clippy::large_enum_variant)]

		#[allow(clippy::large_enum_variant)]

	>>>>>>> 7931dbd (Added validate_domain in electrum option)
	=======
	>>>>>>> a2f5954 (Clean up commands.rs and handlers.rs per review comments)

Conversation

Rahamath-unnisa commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

va-an commented Oct 21, 2025

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!

Rahamath-unnisa commented Oct 23, 2025

Uh oh!

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

va-an left a comment

Choose a reason for hiding this comment

Uh oh!

Rahamath-unnisa commented Mar 28, 2026

Uh oh!

va-an 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!

va-an left a comment

Choose a reason for hiding this comment

Uh oh!

Rahamath-unnisa commented Mar 29, 2026

Uh oh!

tvpeter left a comment

Choose a reason for hiding this comment

Uh oh!

va-an commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Rahamath-unnisa commented Oct 20, 2025 •

edited

Loading