gui: allow to connect to an electrum server w/ a self signed certificate

[pythcoiner]

closes #1300
The issue about connecting to an electrum certificate using rustls have been fixed upstream but in order to beneficiate from it we have to update bdk_electrum and rust-bitvoin dependencies.
Meanwhile, this PR introduce a workaround: the initial issue is related to electrum-client use-rustls feature and use-openssl feature is not reexported by bdk_electrum but we can use electrum-client crate directly and use use-openssl feature by this way:

• use electrum-client directly w/ use-openssl
• add and option to opt-out of ssl domain validation
• let user change the validate_domain values in Settings menu.

Note: ssl://testnet.aranguren.org:51002 electrum server can be used to test this PR

[jp1ac4]

In the GUI, could we add a "Use SSL" checkbox (related #1291), which, if selected, would then show the "validate domain" checkbox? If SSL is not required, then I feel it would be better not to show the validate option if it's not applicable.

Related to this, I wonder if we should use an enum in ElectrumConfig to include validate_domain only for the SSL case, e.g. something like Tcp(url) or Ssl(url, validate_domain). I'm not sure validate_domain should be a mandatory field in the config file. Even in the SSL case, it could default to false if not present.

[nondiremanuel]

As we were discussing, let's have for now the validation checkbox (which I would phrase as "Do not validate SSL Domain (check this only if you want to use a self-signed certificate)") only if the user uses ssl in the address.

[pythcoiner]

the checkbox is now only displayed if the address is valid and contains "ssl://"

[pythcoiner]

Related to this, I wonder if we should use an enum in ElectrumConfig to include validate_domain only for the SSL case, e.g. something like Tcp(url) or Ssl(url, validate_domain). I'm not sure validate_domain should be a mandatory field in the config file. Even in the SSL case, it could default to false if not present.

i think this can be done in a follow-up if we decide to add a checkbox for enable/disable ssl instead prepending it to the address.

[jp1ac4]

i think this can be done in a follow-up if we decide to add a checkbox for enable/disable ssl instead prepending it to the address.

OK yep, let's save that for a follow-up. We could probably keep the value saved in the config file as "ssl://..." and just change how it's parsed into ElectrumConfig.

For this present PR, I think you'll need to make validate_domain an Option so that it works with existing config files. In the case of SSL, it can then default to false if not present.

[pythcoiner]

i think this can be done in a follow-up if we decide to add a checkbox for enable/disable ssl instead prepending it to the address.

OK yep, let's save that for a follow-up. We could probably keep the value saved in the config file as "ssl://..." and just change how it's parsed into ElectrumConfig.

For this present PR, I think you'll need to make validate_domain an Option so that it works with existing config files. In the case of SSL, it can then default to false if not present.

i think having a default as true can avoid footgun and make sense, as it's the default value of the bdk electrum client, see here

[jp1ac4]

i think having a default as true can avoid footgun and make sense, as it's the default value of the bdk electrum client, see here

Ah I see, and so this is also the value we've implicitly been using for SSL connections so far. I agree we can default to true then (also for the GUI checkbox).

[pythcoiner]

i think having a default as true can avoid footgun and make sense, as it's the default value of the bdk electrum client, see here

Ah I see, and so this is also the value we've implicitly been using for SSL connections so far. I agree we can default to true then (also for the GUI checkbox).

the checkbox follow the default, but checked mean "do NOT validate", so it's unchecked as default

[jp1ac4]

the checkbox follow the default, but checked mean "do NOT validate", so it's unchecked as default

That sounds good then. I think it just remains to make validate_domain an Option in the config or otherwise to change how it's deserialized so that a missing value is treated as true.

[pythcoiner]

That sounds good then. I think it just remains to make validate_domain an Option in the config or otherwise to change how it's deserialized so that a missing value is treated as true.

i've implemented default deserialization

[jp1ac4]

As validate_domain is part of the config, I feel that this should somehow also be part of ConfigFieldEdited. Perhaps you can use something like ConfigFieldEdited(electrum::ConfigFieldValue) where ConfigFieldValue is:

pub enum ConfigFieldValue {
    Address(String),
    ValidateDomain(bool),
}

[pythcoiner]

ValidDomainChanged(true) is shorter than
ConfigFieldEdited(electrum::ConfigFieldValue(electrum::ConfigFieldValue::ValidateDomain(bool)))
the event system of iced is already overwhelming imho, we should avoid "overwrapping" it only make the codebase less readable & more error prone

[jp1ac4]

Wouldn't it be just ConfigFieldEdited(electrum::ConfigFieldValue::ValidateDomain(bool))?

And by removing electrum:: it would be just ConfigFieldEdited(ConfigFieldValue::ValidateDomain(bool)).

It doesn't seem too bad, but we can leave it for now. If we have a separate message per field, then we should at some point change ConfigFieldEdited to AddressEdited.

[pythcoiner]

at this point of the release cycle i'd like to bring this PR quickly to a mergeable state or put it on hold: to avoid everyone testing under the christmas tree 😄

[jp1ac4]

That's OK, I meant it as something to consider in the future.

[jp1ac4]

It would be good to add some comments to https://github.com/wizardsardine/liana/blob/master/contrib/lianad_config_example.toml about the new field.

[pythcoiner]

It would be good to add some comments to https://github.com/wizardsardine/liana/blob/master/contrib/lianad_config_example.toml about the new field.

adressed, feel free if you have better wording

[jp1ac4]

Needs a rebase on master.

[pythcoiner]

Needs a rebase on master.

rebsed & addressed comments

[jp1ac4]

In the case that SSL is being used, should we show in the read-only settings view a line to indicate whether validate_domain is true or false?

[pythcoiner]

In the case that SSL is being used, should we show in the read-only settings view a line to indicate whether validate_domain is true or false?

no strong opinion, ping @nondiremanuel

[pythcoiner]

looks like CI failing on windows because missing openssl dependency...

[pythcoiner]

5e5e65e seems to fix windows CI

[nondiremanuel]

In the case that SSL is being used, should we show in the read-only settings view a line to indicate whether validate_domain is true or false?

no strong opinion, ping @nondiremanuel

It's not required at this stage imho.

[jp1ac4]

Tested ACK 5e5e65e.

A couple of nits and a question, but we can come back to those if need be.

[pythcoiner]

comments addressed