Skip to content

electrum: add validate_domain to ElectrumBlockchainConfig#805

Merged
notmandatory merged 1 commit intobitcoindevkit:masterfrom
icota:electrum-config-validate-domain
Dec 24, 2022
Merged

electrum: add validate_domain to ElectrumBlockchainConfig#805
notmandatory merged 1 commit intobitcoindevkit:masterfrom
icota:electrum-config-validate-domain

Conversation

@icota
Copy link
Copy Markdown
Contributor

@icota icota commented Nov 25, 2022

Description

The purpose of the PR is to be able to configure both stop_gap and validate_domain. Perhaps there are nicer ways.

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Bugfixes:

  • This pull request breaks the existing API
  • I'm linking the issue being fixed by this PR

Issue in #804

Copy link
Copy Markdown
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

This currently fails multiple tests in which the new field is missing from ElectrumBlockchainConfig.

@icota
Copy link
Copy Markdown
Contributor Author

icota commented Nov 28, 2022

@tnull this PR is more of a conversation starter since I'm not sure breaking the API would be acceptable anyway. I can close it and we can discuss in #804

@notmandatory
Copy link
Copy Markdown
Member

I think it's OK to implement this as a breaking change as you've done, but you also need to update the related tests that create a new ElectrumBlockchainConfig, setting your new parameter to false.

@icota icota force-pushed the electrum-config-validate-domain branch from 9dc7321 to f2d7b80 Compare November 29, 2022 13:58
@afilini
Copy link
Copy Markdown
Member

afilini commented Dec 7, 2022

Concept ACK, but the default should be true, right? I guess it makes more sense to opt-out of validating a domain, the default should be to do it

@tnull
Copy link
Copy Markdown
Contributor

tnull commented Dec 7, 2022

Concept ACK, but the default should be true, right? I guess it makes more sense to opt-out of validating a domain, the default should be to do it

I had the same thought, but I believe the default returned by ConfigBuilder::new() is actually true:
https://github.com/bitcoindevkit/rust-electrum-client/blob/129081999ce96f1ed33877a2aecad4e2550764f3/src/config.rs#L119-L128

However, I agree that it should probably also be set to true in the doc example given.

Also, needs a rebase now to make CI happy.

@BitcoinQnA
Copy link
Copy Markdown

Concept ACK, but the default should be true, right? I guess it makes more sense to opt-out of validating a domain, the default should be to do it

@afilini forgive my naivety here, but how would this default affect BDK users connecting to their own node at home? The default for this is over Tor, which carries no domain/cert.

@notmandatory
Copy link
Copy Markdown
Member

You all are right the default for validate_domain in the examples should be true since this is what the electrum client Config already defaults to.

@icota this PR also needs to be rebased to fix a recent CI issue. Thanks!

@notmandatory notmandatory added the new feature New feature or request label Dec 16, 2022
@icota icota force-pushed the electrum-config-validate-domain branch from f2d7b80 to 2451c00 Compare December 20, 2022 09:45
Copy link
Copy Markdown
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 2451c00

@notmandatory notmandatory merged commit 8d4cc39 into bitcoindevkit:master Dec 24, 2022
@notmandatory notmandatory mentioned this pull request Dec 26, 2022
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants