Docs: Create default bitcoin.conf file on startup

leishman commented at 2:28 am on July 26, 2018: contributor

Fixes #10746. If no -conf flag is passed and no bitcoin.conf file exists in the datadir, then a bitcoin.conf template will be created in the datadir. This conf file will have no configurations set, only explanatory comments. The default bitcoin.conf template was copied from contrib/debian/examples/bitcoin.conf.

The lack of a default config file historically causes a lot of confusion for new users because bitcoind will print Using config file $DATADIR/bitcoin.conf on startup even if no file exists.

Curious if error handling is being done properly here or if file permissions need to be considered. I don’t think a failure to create this file should terminate the process.

leishman force-pushed on Jul 26, 2018

DrahtBot commented at 4:10 am on July 26, 2018: member

#14074 (Use std::unordered_set instead of set in blockfilter interface by jimpo)
#14057 ([Logging] Only log “Using config file PATH_TO_bitcoin.conf” message on startup if conf file exists by leishman)
#13746 (-masterdatadir for datadir bootstrapping by kallewoof)
#13621 (Check for datadir after the config files were read by Flowdalic)

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.

in src/bitcoin.conf:1 in a517c47665 outdated

0@@ -0,0 +1,147 @@
1+R"(##

ken2812221 commented at 4:17 am on July 26, 2018:

What’s The R and ( for?

in src/bitcoin.conf:147 in a517c47665 outdated

142+
143+# Start Bitcoin minimized
144+#min=1
145+
146+# Minimize to the system tray
147+#minimizetotray=1)"

ken2812221 commented at 4:17 am on July 26, 2018:

What’s the ) for ?

in src/bitcoind.cpp:29 in a517c47665 outdated

22@@ -23,6 +23,12 @@
23 
24 #include <stdio.h>
25 
26+// default bitcoin.conf file template
27+const char* g_default_conf_file =
28+#include <bitcoin.conf>
29+    ;

ken2812221 commented at 4:19 am on July 26, 2018:

I don’t think this is a good way to do this. I’d prefer rename to bitcoin_conf.h and move const char ... into that file.

leishman commented at 4:43 am on July 26, 2018:

The R() is for being able to #include the file. But I like your idea better. I will update. Thanks for the suggestions.

leishman commented at 4:53 am on July 26, 2018:

Also sorry for not explaining further. R() is for multiline string literal: https://stackoverflow.com/a/20508617/2302781

ken2812221 changes_requested

ken2812221 commented at 4:19 am on July 26, 2018: contributor

You should add the filenameto src/Makefile.am https://github.com/bitcoin/bitcoin/blob/29b4ee64695f79511912ae127e66e0fe82f7a0d1/src/Makefile.am#L92

leishman force-pushed on Jul 26, 2018

Docs: Create default bitcoin.conf file on startup

Fixes #10746. If no -conf flag is passed and no bitcoin.conf file exists in the
datadir, then a bitcoin.conf template will be created in the datadir. This conf
file will have no configurations set, only explanatory comments. The default
bitcoin.conf template was copied from contrib/debian/examples/bitcoin.conf.

08def4b624

leishman force-pushed on Jul 26, 2018

leishman commented at 5:47 am on July 26, 2018: contributor

@ken2812221 updated!

jonasschnelli commented at 9:59 am on July 26, 2018: contributor

I’m conceptually unsure if the daemon binary should take care of the configuration management (if even supporting a template make sense). This seems to be better address on the package manager level.

leishman commented at 4:37 pm on July 26, 2018: contributor

I can understand that point of view, however I think that many users are accustomed to config files being generated automatically for them from a binary. When people see that the datadir has been generated they are confused about why there is no config file in there. At the very least we should prevent the daemon output from saying that it is using a conf file that doesn’t exist.

On a higher level, I’m hoping to start contributing more to Bitcoin Core and I’d like to start with some changes that improve usability for new users. It can be quite intimidating for people.

sipa commented at 0:40 am on July 27, 2018: member

If bitcoind is able to create a datadir, at least I think there should be no problem with it creating a default config file in that directory at the same time.

move file creation to same function that creates datadir 77f10e6462

revert changes to bitcoind.cpp 345425d08b

leishman commented at 7:03 am on July 27, 2018: contributor

I’ve updated the PR to create the template config file immediately after creation of the datadir.

laanwj commented at 11:20 am on July 30, 2018: member

I’m conceptually unsure if the daemon binary should take care of the configuration management (if even supporting a template make sense). This seems to be better address on the package manager level.

I tend to agree. Daemons should not write their own configuration file. We can package a default configuration file, but I don’t think bitcoin.conf should be created. (in many cases this is not even possible as the configuration is in a read-only location) (so NACK from me, see also my argumentation in #10746)

laanwj commented at 11:54 am on July 30, 2018: member

If bitcoind is able to create a datadir, at least I think there should be no problem with it creating a default config file in that directory at the same time.

What about a compromise: always create a bitcoin.conf.example in the data directory. The user can decide to use it by copying it to bitcoin.conf (whatever its location) and editing it, or simply ignore it.

leishman commented at 1:19 pm on July 30, 2018: contributor

What about a compromise: always create a bitcoin.conf.example in the data directory. The user can decide to use it by copying it to bitcoin.conf (whatever its location) and editing it, or simply ignore it.

I like this. I also think we should disable the startup logging message saying a conf file is being used, unless it is actually true. If there is no conf file, then we can print a message instructing the user how/where to create one. What do you think?

create bitcoin.conf.example and only log on startup if the conf file actually exists d88246a370

leishman commented at 1:22 am on August 1, 2018: contributor

I’ve updated things so that a bitcoin.conf.example file is created and we only tell the user a conf file is being used if this file actually exists at the time of startup

in src/conf_file.h:33 in d88246a370 outdated

28+
29+##############################################################
30+##            Quick Primer on addnode vs connect            ##
31+##  Let's say for instance you use addnode=4.2.2.4          ##
32+##  addnode will connect you to and tell you about the      ##
33+##    nodes connected to 4.2.2.4.  In addition it will tell ##

practicalswift commented at 8:02 am on August 2, 2018:

Please left-indent this entire block and increase the total line length to say 78 chars. That will make it easier to read.

leishman commented at 12:41 pm on August 2, 2018:

@practicalswift I copied this from contrib/debian/examples/bitcoin.conf. I’m happy to reformat the file and make these changes, but do you think they should be made in both places?

in src/conf_file.h:49 in d88246a370 outdated

44+##                                                          ##
45+##  If you run multiple nodes on a LAN, there's no need for ##
46+##  all of them to open lots of connections.  Instead       ##
47+##  'connect' them all to one node that is port forwarded   ##
48+##  and has lots of connections.                            ##
49+##       Thanks goes to [Noodle] on Freenode.               ##

practicalswift commented at 8:03 am on August 2, 2018:

Please remove the greeting :-)

in src/conf_file.h:84 in d88246a370 outdated

79+# when the server and client are run as the same user.
80+#
81+# If not, you must set rpcuser and rpcpassword to secure the JSON-RPC api. The first
82+# method(DEPRECATED) is to set this pair for the server and client:
83+#rpcuser=Ulysseys
84+#rpcpassword=YourSuperGreatPasswordNumber_DO_NOT_USE_THIS_OR_YOU_WILL_GET_ROBBED_385593

practicalswift commented at 8:05 am on August 2, 2018:

Perhaps the 385593 part could be longer and randomized?

fanquake commented at 8:21 am on August 2, 2018:

part could be longer and randomized?

What do you mean by this? This is an example password that we’d never want anyone to use, and actually comes from the sample bitcoin.conf on the wiki.

practicalswift commented at 8:30 am on August 2, 2018:

@fanquake Yes I know but randomizing would make it slighly less painful for newcomers to make the mistake of uncommenting as-is. Perhaps that is overkill though.

leishman commented at 12:45 pm on August 2, 2018:

I think it’s overkill. If anything we should remove the term ‘Number’ from the example pw so people don’t think it must be limited to a numeric value.

practicalswift commented at 12:50 pm on August 2, 2018:

OK, makes sense!

leishman commented at 12:57 pm on August 2, 2018: contributor

I think it may be more appropriate to make more of an overhaul to the default conf file in a separate PR, since that would involve a change here, a change to contrib/debian/examples/bitcoin.conf and a change to the wiki at a minimum.

widen the block explaining addnode vs connect and remove 'number' from the example pw e39da69c35

in src/init.cpp:1250 in e39da69c35 outdated

1243@@ -1244,7 +1244,12 @@ bool AppInitMain()
1244         LogPrintf("Startup time: %s\n", FormatISO8601DateTime(GetTime()));
1245     LogPrintf("Default data directory %s\n", GetDefaultDataDir().string());
1246     LogPrintf("Using data directory %s\n", GetDataDir().string());
1247-    LogPrintf("Using config file %s\n", GetConfigFile(gArgs.GetArg("-conf", BITCOIN_CONF_FILENAME)).string());
1248+
1249+    // Only log conf file usage message if conf file actually exists
1250+    fs::path config_file_path = GetConfigFile(gArgs.GetArg("-conf", BITCOIN_CONF_FILENAME));
1251+    if(fs::exists(config_file_path))

practicalswift commented at 1:01 pm on August 2, 2018:

Nit: Missing space before (

leishman commented at 1:14 pm on August 2, 2018:

fixed

formatting fix d8018d811c

in src/util.cpp:801 in e39da69c35 outdated

798     return path;
799 }
800 
801+void SetupDatadir(fs::path path)
802+{
803+    // create "wallets" subdirectory

practicalswift commented at 1:03 pm on August 2, 2018:

Nit: Redundant comment :-) Next line says the same thing.

leishman commented at 1:16 pm on August 2, 2018:

I like the explicit comment here personally. It was there before this PR as part of a larger comment and I wanted to keep it around

Sjors commented at 1:51 pm on August 2, 2018: member

One problem with bitcoin.conf.example is that QT won’t see it here:

Perhaps QT could copy the example file over if no bitcoin.conf exists and the user clicks on that button?

Is there a way to just copy the file from contrib during the build process, rather than adding a source code file? The documentation is quite useful in scenarios (e.g. package managers), but can’t be easily reused from conf_file.h.

leishman commented at 1:59 pm on August 2, 2018: contributor

I wanted to avoid adding any dependency from contrib, but I’m not sure the best practice here. Perhaps we could just remove the file in contrib and refer to the new file in src?

Also I think this PR should be separate from changing any conf behaviors with QT. It seems like a somewhat separate issue for the most part. What is the current behavior when clicking that button if no conf file exists?

Sjors commented at 2:49 pm on August 2, 2018: member

@leishman I just noticed there’s also a bitcoin.conf in share/examples, so maybe use (and improve) that one?

If no file exists, the button opens a blank new file (unsaved).

leishman commented at 8:57 pm on August 2, 2018: contributor

@Sjors While I agree that it would be nice to have one location for a reference bitcoin.conf file, I don’t like the idea of referencing a file from outside of the src directory for something like this. It just seems messy. It’s also unclear to me how to include this template file into the compiled binary if it is not defined as a c++ string in a .h or .cpp file. Is this an easy thing to do? I can’t find any clean solutions other than what I have.

Sjors commented at 3:01 pm on August 3, 2018: member

It doesn’t need to be part of the compiled binary though, just part of the distribution packages. Best practices for that vary per OS and I’m not really familiar with them. Maybe you can just pick just one OS to start? The Windows installer (https://github.com/bitcoin/bitcoin/blob/master/Makefile.am#L72) seems to get some stuff from /shared.

leishman commented at 6:07 pm on August 9, 2018: contributor

@Sjors but what if a user is just downloading the bitcoind binary and running it? This binary creates all of the other content in ~/.bitcoin, why shouldn’t it create the sample conf file as well?

Sjors commented at 6:54 pm on August 9, 2018: member

Afaik you can’t download that binary separately from the “official” source, and the release signatures don’t cover individual files either: https://bitcoincore.org/en/download/

leishman commented at 7:10 pm on August 9, 2018: contributor

They are hosted here: https://bitcoincore.org/bin/bitcoin-core-0.16.2/

And when installing the bitcoind package on ubuntu: sudo apt-get install bitcoind this does not setup the ~/.bitcoin directory for you. This directory is created upon execution of the bitcoind binary. So it’s unclear to me where a static file, like the default conf, could possibly be included. What am I missing?

sipa commented at 7:28 pm on August 9, 2018: member

It’s also not possible. When you do a system wide installation of bitcoind it doesn’t know which user(s) are going to run it.

Sjors commented at 7:29 pm on August 9, 2018: member

@leishman these are packages with multiple files, which you can add files to by tweaking the deploy scripts. Similarly, it should be possible for the ubuntu install script to put this template file somewhere, since it also manages put the icon in /usr/share.

leishman commented at 8:12 pm on August 9, 2018: contributor

@sipa so do you think the approach I have taken in this PR is reasonable?

Sjors commented at 11:11 am on August 10, 2018: member

To clarify my position: I prefer a more elegant solution, but I’m OK with doing that later.

I do however recommend adding a functional test to check if this file is indeed created.

leishman commented at 11:13 pm on August 11, 2018: contributor

@Sjors ok I’ll add in that test

leishman commented at 7:20 pm on August 17, 2018: contributor

@Sjors After digging into the testing framework it looks like testing for this behavior is actually quite tricky. The current functionality only creates a bitcoin.conf.example file if the default datadir is used and doesn’t already exist. In the testing framework, a custom test-specific datadir is always used in order to not pollute the global filesystem of the testing machine.

Sjors commented at 4:50 pm on August 20, 2018: member

@jnewbery is there already a test which checks that the datadir is created when necessary? If so, perhaps that test can be modified to also check the presence of bitcoin.conf.example?

jnewbery commented at 8:46 pm on August 24, 2018: member

is there already a test which checks that the datadir is created when necessary?

I don’t think so. See initialize_datadir() in util.py and the places it’s called. There are various places where the files in the datadir or the datadir itself is removed. See here for example: https://github.com/bitcoin/bitcoin/blob/55c18a45305f9e89a726f8cf82a7b16a2ab7f955/test/functional/feature_blocksdir.py#L22

leishman commented at 9:30 pm on August 24, 2018: contributor

Yeah, currently the test framework always puts the datadirs in a custom path deep in the system /tmp directory. This prevents polluting the $HOME path of the machine running the tests. But because of this, my change is basically untestable, unless we change the way the test framework works or hack something ugly.

Testing default datadir creation could be a testing feature we add in a later PR.

leishman commented at 10:38 pm on August 24, 2018: contributor

Pulling out logging update into a separate PR: https://github.com/bitcoin/bitcoin/pull/14057

jnewbery commented at 1:32 pm on August 27, 2018: member

The current functionality only creates a bitcoin.conf.example file if the default datadir is used and doesn’t already exist.

… my change is basically untestable

What do you think about changing the conditional here: https://github.com/bitcoin/bitcoin/pull/13761/files#diff-b7702a084eb00fb47f9800fd68271951R791 to check whether the directory is empty instead of whether the directory exists (ie create the wallets subdirectory and example conf file when a non-existent or empty datadir is used).

That makes more sense to me, and would be testable in the existing test framework.

ken2812221 referenced this in commit fbfa2e46ff on Sep 10, 2018

DrahtBot added the label Needs rebase on Sep 10, 2018

DrahtBot commented at 2:15 pm on September 10, 2018: member

DrahtBot commented at 7:23 pm on December 12, 2018: member

DrahtBot added the label Up for grabs on Dec 12, 2018

DrahtBot closed this on Dec 12, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

PastaPastaPasta referenced this in commit f9e48bdde0 on Jun 27, 2021

PastaPastaPasta referenced this in commit 776d44fac0 on Jun 28, 2021

PastaPastaPasta referenced this in commit 4ea255593d on Jun 29, 2021

PastaPastaPasta referenced this in commit b654ea2332 on Jul 1, 2021

Munkybooty referenced this in commit 47fe958a7d on Jul 1, 2021

MarcoFalke locked this on Dec 16, 2021

fanquake removed the label Up for grabs on Jan 2, 2022

Docs: Create default bitcoin.conf file on startup #13761