Skip to content
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.

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

[GUI] Installer - Add Electrum node option #1241

Merged
merged 15 commits into from
Sep 6, 2024

Conversation

jp1ac4
Copy link
Collaborator

@jp1ac4 jp1ac4 commented Aug 28, 2024

This is for #1223.

For now, it's possible to edit the node's settings but not to change node type.

Remaining tasks:

@jp1ac4 jp1ac4 self-assigned this Aug 28, 2024
@nondiremanuel nondiremanuel added this to the v7 - Liana milestone Aug 28, 2024
@pythcoiner pythcoiner mentioned this pull request Aug 29, 2024
7 tasks
@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Aug 29, 2024

I made a further change to the behaviour while checking the connection. It's safer to prevent the user editing config values until the connection check completes to ensure the values don't change in the meanwhile, which could mean the connection status applies to the wrong values.

I think this was already an issue with the current installer but I guess connection checks are usually quicker with a local bitcoind and so it was less likely to cause problems.

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Aug 29, 2024

I updated the liana dependency commit.

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Aug 29, 2024

@nondiremanuel I made minor wording changes to the following page to mention Electrum:

image

Then on the next page, the user can choose the node type:

image

Perhaps I should remove "full" from the title of this page.

@nondiremanuel
Copy link
Collaborator

For the first page, I was proposing to take the opportunity to make the text simpler and more understandable (at least in my view):

  • I already have a node
    Select this option if you already have a bitcoin node running locally or remotely. Liana will connect to it.
  • I want Liana to automatically install a Bitcoin node on my device
    Liana will install a pruned (i.e. "compressed") node on your computer. You won't need to do anything except having some disk space available (~25GB required) and waiting for the initial synchronization with the network (it can take some days according to your internet connection speed).
    [the two parenthesis with the estimation space and time possibly should be shown only if the network chosen is mainnet]

If there is no consensus with this, we can also leave it as you did (i.e. adding the Electrum mentions) and discuss the general wording for another time.

The second page was exactly what I had in mind. Thanks!

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Aug 29, 2024

I've updated the wording as suggested with some minor changes:

  • I felt "truncated" might be more accurate than "compressed", but probably pruned already conveys this so didn't include that part.
  • I added "on mainnet" after ~25 GB. We don't currently have the network selection available in the view. It should be simple to add as a follow-up commit if required and only display the additional info for mainnet.
  • I used "depending on" instead of "according to" as I think it's more common this way but either should be fine.

image

I removed "full" from the title:

image

@pythcoiner
Copy link
Collaborator

pythcoiner commented Aug 30, 2024

30 GB looks more correct to me (signet chain included but it's only 2.2GB):

~/.liana/bitcoind ❯ sudo du datadir -sh                                                                    
29G     datadir

@pythcoiner
Copy link
Collaborator

image
can we add 127.0.0.1:50001 as placeholder here?
even if it's not likely the electrum server is on localhost, it indicate the user it should specify the port, and indicate 50001 is the default port.

@pythcoiner
Copy link
Collaborator

pythcoiner commented Aug 30, 2024

i'm wondering about one thing here, did the address still only accept ip addresses?
i think the default config on umbrel is to give access via umbrel.local instead of an ip as (i guess) almost user do not configure a static ip for their node.
cf this comment

gui/src/app/state/settings/bitcoind.rs Outdated Show resolved Hide resolved
gui/src/installer/step/node/electrum.rs Outdated Show resolved Hide resolved
gui/src/installer/step/node/electrum.rs Outdated Show resolved Hide resolved
gui/src/installer/step/node/mod.rs Outdated Show resolved Hide resolved
gui/src/installer/step/node/mod.rs Outdated Show resolved Hide resolved
gui/src/installer/view.rs Outdated Show resolved Hide resolved
gui/src/installer/step/node/mod.rs Outdated Show resolved Hide resolved
gui/src/installer/step/node/mod.rs Outdated Show resolved Hide resolved
gui/src/installer/view.rs Outdated Show resolved Hide resolved
@pythcoiner
Copy link
Collaborator

i'm wondering about one thing here, did the address still only accept ip addresses? i think the default config on umbrel is to give access via umbrel.local instead of an ip as (i guess) almost user do not configure a static ip for their node. cf this comment

tested on linux adding 192.168.1.21 electrum.local to /etc/hosts:
image

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Aug 30, 2024

I've made the suggested changes.

I now need to rebase on master as there's a conflict.

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Aug 30, 2024

tested on linux adding 192.168.1.21 electrum.local to /etc/hosts:

Thanks for testing that. Yep, currently the Liana config file accepts any string, which is the same as the underlying Electrum client. Whether we allow any string or not might change as part of #1222.

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Aug 30, 2024

Rebased on master.

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Aug 30, 2024

Sorry, I made a further change to the Electrum address validation.

@pythcoiner
Copy link
Collaborator

testing w/ several remote servers:
image
image
image

seems ok w/ both ssl/tcp

@pythcoiner
Copy link
Collaborator

Here we should rename "Bitcoin Core" into "Electrum server":
image

@pythcoiner
Copy link
Collaborator

Here we should also remove the rescan feature if electrum, but we should keep the block height, i often use it for know if liana is sync w/ the backend:

image

@pythcoiner
Copy link
Collaborator

I'm actually trying electrum (on an other machine on my local network) on regtest w/ 550 coins:
image

for every sync (so every 30 seconds) electrs needs around 10 seconds to answer the gui request.

if i click on Receive menu during this 10 seconds i got not address util all the requests done:

image

i wonder about 2 things:

  • do we make some call to te electrum server for generate address? or it's the iced "event loop" that became overloaded?

  • why we still make so much requests to electrum server after we are sync?

@pythcoiner
Copy link
Collaborator

pythcoiner commented Sep 3, 2024

trying to connect a mainnet wallet on regtest electrum fail as expected:
image
maybe we can try to detect network by genesis hash at the previous step (check connection click) and display a more explicit message to the user?
(i don't think this should be tackle in this PR but rather in a follow up)

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Sep 3, 2024

Updated liana dependency commit hash.

@pythcoiner
Copy link
Collaborator

Maybe we can add (in a follow up) the name/type of the electrum server (by sending a server.banner request

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Sep 4, 2024

Updated liana dependency commit hash.
Changed DaemonControl to mutable following changes to start_rescan in #1222.
Allow to start rescan also for Electrum.

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Sep 5, 2024

Added a commit to enable changing from one node type to another in the settings.

Note that in its current form, it'll be possible for someone who's using managed bitcoind to switch to Electrum in the settings. In this case, the GUI will not try to start the managed bitcoind the next time they launch the GUI. I could also prevent such a user from switching node type (indeed, they cannot edit the bitcoind settings currently).

I also made a fix to a previous commit so that we take into account whether a rescan is in progress in order to determine whether the Electrum settings can be edited.

@jp1ac4 jp1ac4 marked this pull request as ready for review September 5, 2024 15:03
@pythcoiner
Copy link
Collaborator

ACK 819eb92

@darosior darosior merged commit 55958c2 into wizardsardine:master Sep 6, 2024
30 of 31 checks passed
darosior added a commit that referenced this pull request Sep 6, 2024
0f48db7 electrum: check connectivity before creating client (Michael Mallan)

Pull request description:

  This is to resolve #1276.

  Tested that this change stops GUI freezing in #1241 when using invalid address in Electrum settings.

ACKs for top commit:
  darosior:
    utACK 0f48db7

Tree-SHA512: b176ceafda887c4ef97110c6a87807787136da6fd67fb8a2af0b63c102a7d0145f46df5d75b61a10b45cf4cbc91ad27535de7abaf13409e259d1c75881abcef3
@jp1ac4 jp1ac4 deleted the gui-node-management branch September 6, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants