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

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

The default seems to be DataDirectoryGroupReadable=0 regardless of the OS: https://2019.www.torproject.org/docs/tor-manual.html.en#DataDirectoryGroupReadable. So would it be better to move that DataDirectoryGroupReadable 1 suggestion a few lines earlier, next to CookieAuthFileGroupReadable 1 and remove the sentence On some systems ...?

I confirm that on FreeBSD, by default the tor data directory is not group readable:

drwx------  7 _tor  _tor  13 Dec 28 12:24 /var/db/tor/

But even if it was, that would not suffice for bitcoind to be able to read the cookie file, since it runs as the bitcoin user which does not belong to the _tor group. I guess an extra step of adding bitcoin to the _tor group would be required. @jessebarton, do you have it running with cookie auth on FreeBSD? Did you add bitcoin to the _tor group?

@vasild Correct I had to add bitcoin to the _tor group. I am running cookie auth.

I think it makes sense to move that suggestion up a few lines sense its required on all systems and not just some.

Since the DataDirectoryGroupReadable 1 suggestion said only some systems in the doc when I initially set it up it wasn't working so I had to go back and add it. It would save a step for people setting this up on other systems if it just says its required for all.

I think it makes sense to move that suggestion up a few lines sense its required on all systems and not just some.

@jessebarton did you want to follow up with the changes here?

also cc @murrayn re FreeBSD.

I think it makes sense to move that suggestion up a few lines sense its required on all systems and not just some.

It does apply to FreeBSD, but it is not required on all systems.

It does apply to FreeBSD, but it is not required on all systems.

On which systems it is not required? How does it work on such a system?

Maybe some distro patched the tor daemon itself to make DataDirectoryGroupReadable=1 the default, or ship with a pre-set config file that contains DataDirectoryGroupReadable=1? Or if not, then maybe they run the bitcoind process with the same user as the tor daemon?

@jessebarton want to followup here?

The Tor project has this set (Default: 0) freebsd takes the same Default. Makes sense to me to follow what the Tor docs show rather than each individual project.

The changes in the first commit seem to be removed in the second commit? If so, please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

This was my first time doing a squash commit. If it doesn't look right I can fix it. Appreciate the help.

If it doesn't look right I can fix it. Appreciate the help.

@jessebarton There are currently three commits here, when it should be (squashed to) one.

I usually resort to git rebase -i. With the current history (a877011f64):

git rebase -i HEAD~3

an editor will open that contains this:

pick 0a6f9b4440 doc: Updating tor.md doc to include mention of FreeBSD requiring the DataDirectoryGroupReadable setting
pick 3c531ed814 doc: update DataDirectoryGroupReadable 1 in tor.md
pick a877011f64 doc: update DataDirectoryGroupReadable 1 in tor.md

change the last two lines to begin with f and f -C, like this:

pick 0a6f9b4440 doc: Updating tor.md doc to include mention of FreeBSD requiring the DataDirectoryGroupReadable setting
f 3c531ed814 doc: update DataDirectoryGroupReadable 1 in tor.md
f -C a877011f64 doc: update DataDirectoryGroupReadable 1 in tor.md

save and quit the editor.

Verify the result: git log to see that there is just one commit on top of master. git show to check that it has this commit message:

    doc: update DataDirectoryGroupReadable 1 in tor.md
    
    Move DataDirectoryGroupReadable 1 up a few lines to more clearly
    communicate that it is required for the filesystem group to read the
    DataDirectory.
    
    Per the Tor documentation
    https://2019.www.torproject.org/docs/tor-manual.html.en#DataDirectoryGroupReadable
    "If this option is set to 0, don’t allow the filesystem group to read
    the DataDirectory. If the option is set to 1, make the DataDirectory
    readable by the default GID. (Default: 0)"

and this diff:

@@ -89,25 +89,19 @@ some or all of the following settings in `/etc/tor/torrc`, generally commented
 out by default (if not, add them):
 
 ```
 ControlPort 9051
 CookieAuthentication 1
 CookieAuthFileGroupReadable 1
+DataDirectoryGroupReadable 1
 ```
 
 Add or uncomment those, save, and restart Tor (usually `systemctl restart tor`
 or `sudo systemctl restart tor` on most systemd-based systems, including recent
 Debian and Ubuntu, or just restart the computer).
 
-On some systems (such as Arch Linux), you may also need to add the following
-line:
-
-```
-DataDirectoryGroupReadable 1
-```
-
 ### Authentication
 
 Connecting to Tor's control socket API requires one of two authentication

@vasild Thanks that helped a ton!

Looks like I have it down to one now.