Skip to content

feat!: use unique ports for Electrum #523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Jul 2, 2025
Merged

feat!: use unique ports for Electrum #523

merged 6 commits into from
Jul 2, 2025

Conversation

@luisschwab
Copy link
Contributor

@luisschwab luisschwab commented Jun 15, 2025 •
edited
Loading

What is the purpose of this pull request?

  • Bug fix
  • Documentation update
  • New feature
  • Test
  • Other:

Which crates are being modified?

  • floresta-chain
  • floresta-cli
  • floresta-common
  • floresta-compact-filters
  • floresta-electrum
  • floresta-watch-only
  • floresta-wire
  • floresta
  • florestad
  • Other:

Description

Closes #522.

This PR introduces the use of different ports for the Electrum Server, conditional on bitcoin::Network and TLS status:

Network sans-TLS TLS
Bitcoin 50001 50002
Signet 60001 60002
Testnet4 40001 40002
Testnet3 30001 30002
Regtest 20001 20002

I also changed the naming from SSL to TLS (a big misnomer that still lingers).

Changelog

  • Refactor Electrum Server section.
  • Use TLS naming instead of SSL; SSL has been deprecated for over 10 years (since the POODLE attack, in 2015), and everything uses TLS now, but the misnaming still happens a lot.
  • Removed ssl_electrum_address: this argument is redundant, as we don't have two Electrum servers running.
  • Renamed ssl_key_path to tls_key_path.
  • Renamed ssl_cert_path to tls_cert_path.
  • Renamed no_ssl to enable_electrum_tls: in my opinion, the Electrum server shouldn't use TLS by default; it's more common to do TLS termination elsewhere.
  • Renamed gen_selfsigned_cert to generate_cert: more concise.
  • Adjusted tests accordingly.
  • Adjusted documentation accordingly.

@luisschwab
Copy link
Contributor Author

Why doesn't just test-functional-uv-fmt actually format the tests?

@luisschwab luisschwab force-pushed the feat/unique-electrum-ports branch 2 times, most recently from 9d0ec20 to cb75535 Compare June 16, 2025 00:00
jaoleal
jaoleal reviewed Jun 16, 2025
View reviewed changes
@jaoleal
Copy link
Collaborator

jaoleal commented Jun 16, 2025

Why doesn't just test-functional-uv-fmt actually format the tests?

black is in --check mode for some reason, would you mind ?

@Davidson-Souza Davidson-Souza added documentation Improvements or additions to documentation enhancement New feature or request chore Cleaning, refactoring, reducing complexity labels Jun 16, 2025
@luisschwab luisschwab force-pushed the feat/unique-electrum-ports branch from cb75535 to cc72c79 Compare June 16, 2025 22:14
jaoleal
jaoleal suggested changes Jun 17, 2025
View reviewed changes
@qlrd
Copy link
Contributor

qlrd commented Jun 17, 2025

Why doesn't just test-functional-uv-fmt actually format the tests?

Because some editors (specially neovim) could conflict. When you save after format, it could edit the things you already edited. So a check is more guaranteed and if you wanna a formate uv run black ./tests.

@luisschwab
Copy link
Contributor Author

Because some editors (specially neovim) could conflict. When you save after format, it could edit the things you already edited. So a check is more guaranteed and if you wanna a formate uv run black ./tests.

I don't agree, most projects have commands that format the code. This behavior should be on a test-functional-uv-check recipe.

@qlrd
Copy link
Contributor

qlrd commented Jun 17, 2025

Because some editors (specially neovim) could conflict. When you save after format, it could edit the things you already edited. So a check is more guaranteed and if you wanna a formate uv run black ./tests.

I don't agree, most projects have commands that format the code. This behavior should be on a test-functional-uv-check recipe.

fair enough, maybe in another PR?

@luisschwab
Copy link
Contributor Author

Sure, feel free to to do it if you want.

@Davidson-Souza
Copy link
Member

You've removed the option to run both clear/ssl at the same time. I like that feature, not sure it should be removed

@luisschwab
Copy link
Contributor Author

luisschwab commented Jun 17, 2025 •
edited
Loading

It does not make a lot of sense to me. What is your rationale?

For me it makes sense to either not enable TLS (and use it insecurely or terminate TLS elsewhere), or enable it and always use TLS.

@Davidson-Souza
Copy link
Member

Some wallets doesn't support TLS. I also use nc and jq to request some suff manually. But I also like TLS for my main wallets, specially if I'm accessing them remotely.

@luisschwab
Copy link
Contributor Author

So you mean always serving non-TLS and optionally serving TLS?

@Davidson-Souza
Copy link
Member

So you mean always serving non-TLS and optionally serving TLS?

Yes. That's the behavior we have now.

@luisschwab luisschwab force-pushed the feat/unique-electrum-ports branch 2 times, most recently from e28b263 to 0cb4983 Compare June 18, 2025 18:42
@luisschwab luisschwab requested a review from jaoleal June 18, 2025 18:53
JoseSK999
JoseSK999 suggested changes Jun 20, 2025
View reviewed changes
@luisschwab luisschwab mentioned this pull request Jun 24, 2025
Closed
@luisschwab luisschwab force-pushed the feat/unique-electrum-ports branch from 0cb4983 to 7c800e6 Compare June 25, 2025 04:43
@luisschwab
Copy link
Contributor Author

@JoseSK999 addressed your reviews.

Davidson-Souza
Davidson-Souza requested changes Jun 26, 2025
View reviewed changes
Copy link
Member

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

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

Can you drop e2456c7? It's already addressed in #540

JoseSK999
JoseSK999 suggested changes Jun 26, 2025
View reviewed changes
@luisschwab luisschwab force-pushed the feat/unique-electrum-ports branch from 7c800e6 to b1c6a87 Compare June 27, 2025 15:03
@luisschwab
Copy link
Contributor Author

Can you drop e2456c7? It's already addressed in #540

Dropped

JoseSK999
JoseSK999 suggested changes Jun 27, 2025
View reviewed changes
@luisschwab luisschwab force-pushed the feat/unique-electrum-ports branch 2 times, most recently from 053ff8b to af5475b Compare June 28, 2025 18:48
@JoseSK999
Copy link
Contributor

Can you rebase now?

@luisschwab
feat(florestad): use unique ports for the Electrum Server
3c518c3
@luisschwab
refactor!(florestad): clean up Electrum Server section and refactor C…
9994c4d 
…LI arguments

Clean up Electrum Server section.

Use `TLS` naming instead of `SSL`; SSL has been deprecated for 10 years (since the POODLE attack, in 2015), and everything uses TLS now, but the misnaming still happens a lot.

Renamed `no_ssl` to `enable_electrum_tls`.
Renamed `ssl_electrum_address` to `electrum_address_tls`.
Renamed `ssl_key_path` to `tls_key_path`.
Renamed `ssl_cert_path` to `tls_cert_path`.
Renamed `gen_selfsigned_cert` to `generate_cert`: more concise.
@luisschwab
chore(doc): update TLS section
798e80d
@luisschwab luisschwab force-pushed the feat/unique-electrum-ports branch from af5475b to 1fedd11 Compare June 30, 2025 19:49
@luisschwab
Copy link
Contributor Author

Can you rebase now?

You got it

JoseSK999
JoseSK999 reviewed Jul 1, 2025
View reviewed changes
@luisschwab luisschwab force-pushed the feat/unique-electrum-ports branch from 1fedd11 to c58cadd Compare July 1, 2025 19:59
JoseSK999
JoseSK999 reviewed Jul 1, 2025
View reviewed changes
@luisschwab luisschwab force-pushed the feat/unique-electrum-ports branch from c58cadd to af432ab Compare July 2, 2025 00:14
@luisschwab luisschwab force-pushed the feat/unique-electrum-ports branch from af432ab to e2b69c4 Compare July 2, 2025 00:16
JoseSK999
JoseSK999 approved these changes Jul 2, 2025
View reviewed changes
Copy link
Contributor

@JoseSK999 JoseSK999 left a comment

Choose a reason for hiding this comment

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

ACK e2b69c4

Davidson-Souza
Davidson-Souza approved these changes Jul 2, 2025
View reviewed changes
Copy link
Member

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

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

ACK e2b69c4

@Davidson-Souza Davidson-Souza merged commit d68edfd into vinteumorg:master Jul 2, 2025
10 checks passed
@qlrd qlrd mentioned this pull request Jul 2, 2025
Merged
15 tasks
@luisschwab luisschwab deleted the feat/unique-electrum-ports branch July 25, 2025 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Davidson-Souza Davidson-Souza Davidson-Souza approved these changes

@jaoleal jaoleal Awaiting requested review from jaoleal

+2 more reviewers

@qlrd qlrd qlrd left review comments

@JoseSK999 JoseSK999 JoseSK999 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

chore Cleaning, refactoring, reducing complexity documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] Different networks use the same electrum port

5 participants

@luisschwab @jaoleal @qlrd @Davidson-Souza @JoseSK999