Skip to content

create bitcoin.conf on first run with template #23931

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
Closed
wants to merge 4 commits into from
Closed

create bitcoin.conf on first run with template #23931

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 1, 2022 •
edited by ghost
Loading

Fixes #10746 and #19641

What?

Create bitcoin.conf on first run with a template added as comments. Can add/remove things based on feedback.

Why?

bitcoind can already create bitcoin.conf if a user runs below command on first startup:

bitcoind -startupnotify="touch path/bitcoin.conf"

bitcoin-qt can already create blank bitcoin.conf if a user can find this option after starting Bitcoin Core for the first time:

image

This pull request improves UX when using bitcoind.

How?

Used most of the code from #13761 (will add Alexander Leishman as co-author if I could get the email address)

Template is changed and I have used bitcoin.conf (privacy) template from https://github.com/jlopp/bitcoin-core-config-generator/

@ghost
Copy link
Author

ghost commented Jan 1, 2022

I tried compiling and running this locally. Weird this had no errors when compiling on Fedora but there was no bitcoin.conf created on running bitcoind.

I will try testing on other OS tomorrow.

@meshcollider
Copy link
Contributor

will add Alexander Leishman as co-author if I could get the email address

You can fetch his branch from the PR by running
git fetch upstream pull/13761/head
And then using git show to look at one of the commits and see the email address used.

benthecarman
benthecarman reviewed Jan 3, 2022
View reviewed changes
benthecarman
benthecarman reviewed Jan 3, 2022
View reviewed changes
@ghost
Copy link
Author

ghost commented Jan 4, 2022

Thanks @meshcollider I was able to find the email address and added as co-author.

Changes in last commit:

  1. Add Alexander Leishman as co-author

  2. Add signet section

  3. Change copyright year

  4. Change everything after #endif to comments based on errors in CI

Tried testing and it works as expected. Config file was created on first run. It works when there is no data directory present in default location which was not the case when I tried earlier in #23931 (comment)

@ghost ghost marked this pull request as ready for review January 7, 2022 01:56
@ghost
Copy link
Author

ghost commented Jan 7, 2022

I have marked the pull request ready for review. Will figure out a way to add test if it is required.

@Rspigler
Copy link
Contributor

Rspigler commented Jan 7, 2022

This approach doesn't seem as good to me as the other proposed changes. Why only show a subset of config options as comments? Doesn't really matter if it is a template provided by a third party.

@ghost
Copy link
Author

ghost commented Jan 7, 2022 •
edited by ghost
Loading

Why only show a subset of config options as comments? Doesn't really matter if it is a template provided by a third party.

We will have to decide and mention only some options in template. It's not possible to add all options. For other options users can do research. This file can also be left blank but one template will help users understand basic options.

As mentioned in OP options can be added/removed based on feedback.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 7, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24266 (util: Avoid buggy std::filesystem:::create_directories() call by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot mentioned this pull request Jan 8, 2022
Merged
@Rspigler
Copy link
Contributor

It's not possible to add all options

I don't see why that's true. Look at this PR for example #22235

@ghost
Copy link
Author

ghost commented Jan 11, 2022 •
edited by ghost
Loading

Sorry I don't agree with that approach to solve the problem. I would prefer a template in bitcoin.conf with basic things that a new user could understand and file created on first run.

luke-jr
luke-jr suggested changes Jan 12, 2022
View reviewed changes
niVelion
niVelion reviewed Jan 12, 2022
View reviewed changes
src/conf_file.h
# Add a node IP address to connect to and attempt to keep the connection open. This option can be set multiple times.
#addnode=randomchars.onion
# Bind to given address and always listen on it. (default: 0.0.0.0). Use [host]:port notation for IPv6. Append =onion to tag any incoming connections to that address and port as incoming Tor connections
#bind=127.0.0.1:8333
Copy link

@niVelion niVelion Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Could you explain the binding suggested here?

EDIT: I was referring to L16 #bind=127.0.0.1:8333, on re-reading perhaps it's my bad assumption that this was a suggestion.

Copy link
Author

@ghost ghost Jan 12, 2022 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

I was referring to L16 #bind=127.0.0.1:8333

Its an example for using bind

Copy link

@niVelion niVelion Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You mention the default binding is 0.0.0.0; which I understand to mean the server will listen on all network interfaces.

And we know listening on 127.0.0.1 would instruct the server not to listen on any interface; not to receive remote connections, but even a technical user might not realize this.

Maybe swapping the default with the example would reduce user error?

Suggested change
#bind=127.0.0.1:8333
# Bind to given address and always listen on it. Use [host]:port notation for IPv6. Eg 192.249.249.1:10333.
# Append =onion to tag any incoming connections to that address and port as incoming Tor connections
#bind=0.0.0.0

@ghost ghost mentioned this pull request Jan 25, 2022
Closed
@DrahtBot DrahtBot mentioned this pull request Feb 5, 2022
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 8, 2022

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Build system Needs rebase Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create a default bitcoin.conf in the correct location.
6 participants
@meshcollider @Rspigler @DrahtBot @luke-jr @benthecarman @niVelion