Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Summary

Currently, the seeder will write its database files to the current
working directory. This is clumsy and can lead to starting the seeder
up without its database if it is started up from a different location.

This change will not only create a default directory for the seeder's
database files (and any future files it writes/reads) but also allow the
user to define where that data directory is.

Test Plan

ninja
./bitcoin-seeder --help

Verify help text for new option

mkdir dirtest
./bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org --datadir=dirtest

Let the seeder run for a while and then verify the files dnsseed.dat, dnsseed.dump, and dnsstats.log are written to the dirtest/seeder/ directory. Also verify the relative datadir warning shows up.

./bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org

Verify the files dnsseed.dat, dnsseed.dump, and dnsstats.log are written to the ~/.bitcoin/seeder/ directory.

Confirm the above works for testnet and that the files are written to <path>/seeder/testnet3/<file>.
Also confirm that the above works for regtest and that the files are written to <path>/seeder/regtest/<file>.

rm -r dirtest
./bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org --datadir=dirtest

Verify that the seeder errors out with the error "Specified data directory <path>/dirtest/ does not exist.".

mv ~/.bitcoin/ ~/.bubitcoin/
./bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org

Verify that the seeder operates as usual without error and that ~/.bitcoin/seeder/ directory is created and
eventually the files are written there.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

SeederDir

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 11100
Build 20672: Default Diff Build & Tests
Build 20671: Build without Wallet
Build 20670: Build with clang-10
Build 20669: arc lint + arc unit

Event Timeline

nakihito created this revision.May 4 2020, 22:40

Owners added a reviewer: Restricted Owners Package.May 4 2020, 22:40

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 4 2020, 22:40

nakihito requested review of this revision.May 4 2020, 22:40

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

nakihito retitled this revision from Add -seederdir option to allow selection of seeder's data directory to Add -seederdir option to allow specification of seeder's data directory.May 4 2020, 22:41

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10582: Diff 19425.May 4 2020, 22:53

This is introducing dependencies ont he seeder in the base libraries, this makes no sense whatsoever.

src/util/system.h
92 ↗	(On Diff #19425)	No.

This revision now requires changes to proceed.May 5 2020, 02:54

nakihito edited the test plan for this revision. (Show Details)May 5 2020, 19:23

Moved seeder specific functions out of regular node code.

nakihito edited the test plan for this revision. (Show Details)May 5 2020, 20:20

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10598: Diff 19706.May 5 2020, 20:38

nakihito added a child revision: D5987: Start using Logger for seeder logging.May 7 2020, 00:32

It seems like there is already a machinery to select the folder in which business happen, why does this needs to be all duplicated with a different name?

src/seeder/main.cpp
99 ↗	(On Diff #19706)	Isn't this an error?

This revision now requires changes to proceed.May 7 2020, 14:40

nakihito edited the test plan for this revision. (Show Details)May 7 2020, 19:23

Error outputs to std::cerr and changed GetSeederDir() to call GetDefaultDataDir() directly when -seederdir is not set.

In D5956#146767, @deadalnix wrote:

It seems like there is already a machinery to select the folder in which business happen, why does this needs to be all duplicated with a different name?

I'm assuming you are talking about the -datadir related functions in util/system.cpp, those functions do not allow the seeder to create its own subdirectory. If I were to use those then all of the seeder files would be mixed in with the regular node files.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10648: Diff 19817.May 7 2020, 19:35

I'm assuming you are talking about the -datadir related functions in util/system.cpp, those functions do not allow the seeder to create its own subdirectory. If I were to use those then all of the seeder files would be mixed in with the regular node files.

You can use a subdirectory if you think that it is important. But ultimately, this makes no sense because I can run the seeder with one datadir and the node with another if I want to.

Removed added files/functions. Option uses pre-existing -datadir related code.

nakihito planned changes to this revision.May 20 2020, 00:40

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-werror) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-without-wallet) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-werror) passed.

Build Bitcoin-ABC / Diffs / Diff Testing (build-without-wallet) passed.

nakihito edited the test plan for this revision. (Show Details)May 20 2020, 00:46

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10874: Diff 20318.May 20 2020, 00:49

Adjusted help text.

Build Bitcoin-ABC / Diffs / Diff Testing (build-werror) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-without-wallet) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-werror) passed.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-without-wallet) passed.

Harbormaster completed remote builds in B10902: Diff 20372.May 20 2020, 18:29

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

deadalnix requested changes to this revision.May 25 2020, 16:23

deadalnix added inline comments.

src/seeder/main.cpp
99 ↗	(On Diff #20372)	The diff still mention seederdir.
100 ↗	(On Diff #20372)	Wouldn't it be preferable to always append a seedersubfolder to the datadir?

This revision now requires changes to proceed.May 25 2020, 16:23

nakihito retitled this revision from Add -seederdir option to allow specification of seeder's data directory to Add -datadir option to allow specification of seeder's data directory.May 26 2020, 17:38

nakihito edited the test plan for this revision. (Show Details)

Added some output that specifies the default directory and the directory in use on start up.

src/seeder/main.cpp
100 ↗	(On Diff #20372)	I do not think so. The help text says that the option specifies the directory that is read from and/or written to. Right now, the option is flexible in that a user can specify any directory and the seeder will read and write from there. This gives users more control if, for example, someone wants to keep the seeder directory separate from the regular node directory. If a user specifies `-datadir=~/.seeder/` for example, it is expected that reads from and writes to the specified directory `~/.seeder/` and not `~/.seeder/seeder/`. This also more closely matches the behavior of the node software's `-datadir` option.