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

autodetect from bitcoin.conf file with network set #2037

Merged
merged 7 commits into from
Jan 11, 2023

Conversation

Kexkey
Copy link
Contributor

@Kexkey Kexkey commented Jan 5, 2023

Hi Specter team!

When using a bitcoin.conf file with remote Bitcoin node configurations, Specter wouldn't auto-detect the right connection configs when not mainnet and more than only mainnet configs. For some reason, it was ignoring the network when set in the config file. For example, if bitcoin.conf file was:

regtest=1
rpcconnect=bitcoin
main.rpcport=8332
test.rpcport=18332
regtest.rpcport=18443
rpcuser=bitcoin
rpcpassword=CHANGEME

Specter kept using port 8332, which is the default port in its config. It was reading the configurations for all networks in the config file even when a network was specified, like in the example above. Also, when rotating between the different network configs trying to connect to the node, it was exiting on the first failure instead of continue trying with the other network configs.

Maybe I misunderstood the rationale behind ignoring the network config and keeping trying for every network config found in the config file, sorry if that's the case. Ignoring the network config makes it impossible to run several nodes on the same machine on different ports and use the auto-detect feature. For example, if I run mainnet, testnet and regtest nodes on the same bitcoin host, current algorithm would only connect to the first one responding. I couldn't switch node by simply replacing regtest=1 by testnet=1 in the bitcoin.conf file, for instance.

This PR only changes rpc.py. What it does:

Let me know if I'm confused :) or if you see improvements or a better way to do this. I tried to minimize the changes for this PR.

@netlify
Copy link

netlify bot commented Jan 5, 2023

Deploy Preview for specter-desktop-docs canceled.

Name Link
🔨 Latest commit 8908482
🔍 Latest deploy log https://app.netlify.com/sites/specter-desktop-docs/deploys/63be87db4ec137000a0d288a

@moneymanolis
Copy link
Collaborator

@Kexkey what do you mean with "bitcoin.conf file with remote Bitcoin node configurations"? Specter can only read the bitcoin.conf if this file is on the same machine as Specter is running on.

In any case, have a look at this PR in the big UI revamp: 1db98cf

@k9ert
Copy link
Collaborator

k9ert commented Jan 10, 2023

I've added a test and sounds reasonable to me. @moneymanolis can we merge as the change does imho not interfere with 1db98cf ?!

@moneymanolis
Copy link
Collaborator

I've added a test and sounds reasonable to me. @moneymanolis can we merge as the change does imho not interfere with 1db98cf ?!

Little change only in get_rpcconfig, true.

But, tests are red.

@Kexkey
Copy link
Contributor Author

Kexkey commented Jan 10, 2023

Oh sorry I should have added tests, thanks @k9ert for that.

@moneymanolis what I meant was "...when using a local bitcoin.conf file to auto-setup Specter to use a remote bitcoin node..."

I checked the commit you linked above but I'm not familiar enough with the code to tell you if the revamp fixes what this PR does.

Thanks for taking care of this PR! I updated Specter to v1.14.2 for the next release of Cyphernode. If this PR goes through to the next Specter release in time, it will be part of the final release.

You guys are doing great work! Thank you!

@k9ert
Copy link
Collaborator

k9ert commented Jan 11, 2023

Really happy to hear that specter is doing well in cyphernode.
I'm confident to do a release hopefully tomorrow. Pretty sure we'll get this in here as well.
As you have an electrum on your stack, you might want to experiment with the new electrum connection which we implemented via the "spectrum-extension". It does work well for me and it's much more convenient: No more rescanning hassle. Much faster performance. Importing dozens of devices/wallets in minutes.
We created some hints here: https://github.com/cryptoadvance/specter-desktop/releases/tag/v1.14.1
Yes, it's alpha formally but i think in practice it's more beta than alpha. With the UI revamp, it'll be production ready.

@k9ert k9ert merged commit 2a03ec1 into cryptoadvance:master Jan 11, 2023
k9ert added a commit that referenced this pull request Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants