Page MenuHomePhabricator

Integrate gArgs and chainparams into the Seeder
ClosedPublic

Authored by nakihito on Oct 27 2019, 02:04.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC5b53e81f29d6: Integrate gArgs and chainparams into the Seeder
Summary

Replaces getopts() with gArgs.
Because ArgsManager does not seem to support both long and short
options and because it also reserves the -h option for help message
requests, the seeder's options have all been changed to long-form. This
is also more inline with our other code (for example, see bitcoind.cpp).

This patch also integrates chainparams with the seeder.

Release notes have been added and the Seeder's README file have
been edited to reflect this these changes.

Test Plan
make
ninja
bitcoin-seeder -help
bitcoin-seeder -h
bitcoin-seeder -?
bitcoin-seeder -h --help-debug

These should all output the seeder help message. The last line should print the same but also include the -regtest option help text.

./bitcoin-seeder -host=seeder.bitframe.org -ns=dev.bitframe.org -port=5364 -mbox=info@bitframe.org -testnet -filter=1,5 -wipeignore -wipeban -threads=95 -dnsthreads=5

This should start the seeder with the following lines:

Supporting whitelisted filters: 0x1,0x5
Using test.
Loading dnsseed.dat...Ban list wiped...Ignore list wiped...done
Starting 5 DNS threads for seeder.bitframe.org on dev.bitframe.org (port 5364)........done
Starting seeder...done
Starting 95 crawler threads...done
...

The third line above will only output if there is already a dnsseed.dat file in the directory the seeder is run from.
The second line from the top should reflect the selected chainparams:
main by default
test for testnet
regtest for regtest

bitcoin-seeder -jire=5

This should error out with the following message:

Error parsing command line arguments: Invalid parameter -jire

Diff Detail

Repository
rABC Bitcoin ABC
Branch
UpdateSeederHelp-4
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nakihito planned changes to this revision.Wed, Nov 20, 18:48
nakihito updated this revision to Diff 14265.Wed, Nov 20, 20:02

Removed fTestNet and fixed regtest default port and height.

nakihito added inline comments.Wed, Nov 20, 20:04
src/seeder/bitcoin.h
25 ↗(On Diff #14265)

This function will be removed in a future patch, but these changes are necessary now for proper regtest support.

src/seeder/db.h
37 ↗(On Diff #14265)

This function will be removed in a future patch, but these changes are necessary now for proper regtest support.

nakihito edited the test plan for this revision. (Show Details)Wed, Nov 20, 20:05
deadalnix requested changes to this revision.Wed, Nov 20, 21:39

Use the goddamn ChanParams. You broke the net magic and introduce many branches you don't need to introduce.

This revision now requires changes to proceed.Wed, Nov 20, 21:39
nakihito updated this revision to Diff 14281.Wed, Nov 20, 23:36

Completely integrated chainparams into seeder.

nakihito planned changes to this revision.Wed, Nov 20, 23:36
nakihito retitled this revision from Replace getopts() with gArgs in the Seeder. to Integrate gArgs and chainparams into the Seeder.Wed, Nov 20, 23:41
nakihito edited the summary of this revision. (Show Details)
nakihito updated this revision to Diff 14282.Wed, Nov 20, 23:43

Removed an unnecessary header file.

deadalnix requested changes to this revision.Thu, Nov 21, 15:59
deadalnix added inline comments.
src/seeder/bitcoin.cpp
174 ↗(On Diff #14282)

You assume you are getting a reference to the same object here, which you have no idea you do.

src/seeder/main.cpp
440 ↗(On Diff #14282)

You cna use a C++11 range loop and avoid creating a copy of the whole seed list for every thread.

545 ↗(On Diff #14282)

I don't quite understand why you went around modifying everything in the codebase when you could have just assigned the expected value here, modify GetDefaultPort, just like you did GetRequiredHeight, and be done with it. Now the patch is blocked for things that are unrelated to what this is actually doing.

This revision now requires changes to proceed.Thu, Nov 21, 15:59
nakihito updated this revision to Diff 14333.Fri, Nov 22, 19:42

Restored netMagic and rebased off D4417.

nakihito planned changes to this revision.Fri, Nov 22, 19:42
nakihito updated this revision to Diff 14334.Fri, Nov 22, 19:51

Clarified some wording in release notes and fixed formatting.

nakihito added inline comments.Fri, Nov 22, 20:22
src/seeder/CMakeLists.txt
20 ↗(On Diff #14334)

Discussed offline with Fabien as an alternative to D4476.

deadalnix requested changes to this revision.Sat, Nov 23, 01:42
deadalnix added inline comments.
src/seeder/main.cpp
19 ↗(On Diff #14334)

Please don't add boost dependencies.

99 ↗(On Diff #14334)

The problem was that you used string overload rather than char ones. You did two backport that fix these over the whole codebase so you should know.

This revision now requires changes to proceed.Sat, Nov 23, 01:42
nakihito updated this revision to Diff 14398.Mon, Nov 25, 19:07

Search for a char rather than a string and reverted boost dependency.

nakihito planned changes to this revision.Mon, Nov 25, 19:07
nakihito requested review of this revision.Mon, Nov 25, 22:09
deadalnix accepted this revision.Wed, Nov 27, 16:44
This revision is now accepted and ready to land.Wed, Nov 27, 16:44

Note that you find yourself blocked because you made this depend on a test, however, the test doesn't seems to be testing any of that code.

nakihito updated this revision to Diff 14580.Mon, Dec 2, 19:40

Removed dependency.

nakihito edited the summary of this revision. (Show Details)Mon, Dec 2, 19:40
nakihito requested review of this revision.Mon, Dec 2, 21:38
deadalnix requested changes to this revision.Mon, Dec 2, 23:42
deadalnix added inline comments.
src/seeder/CMakeLists.txt
7 ↗(On Diff #14580)

I'm not sure what's net about it, but why not.

src/seeder/main.cpp
100 ↗(On Diff #14580)

This creates copies of strings. In addition aoti64 doesn't take std::string as input, so it seems completely unnecessary. Is the even correct? When is the string destructed?

This revision now requires changes to proceed.Mon, Dec 2, 23:42
nakihito updated this revision to Diff 14586.Tue, Dec 3, 00:54

seeder-net -> seeder-netprocessing.

nakihito added inline comments.Tue, Dec 3, 00:55
src/seeder/main.cpp
100 ↗(On Diff #14580)

This creates temporary copies of substrings of flagString. These strings are then destructed at the end of the loop iteration. Also, atoi64() does have a version that takes std::string as input (see util/strencodings.cpp line 641).

deadalnix accepted this revision.Wed, Dec 4, 17:09
deadalnix added inline comments.
src/seeder/CMakeLists.txt
9 ↗(On Diff #14586)

I really do not think that the db is related to the processing of the network in anyway, but so be it.

src/seeder/main.cpp
100 ↗(On Diff #14580)

It's clearly not destructed at the end of the loop iteration. I looked at the spec and it looks like you are in the green, though clearly 100% due to luck.

The temporary creation and destruction clearly are unnecessary work, but considering this isn't critical and this has gone through way too many iterations, so be it.

This revision is now accepted and ready to land.Wed, Dec 4, 17:09
This revision was automatically updated to reflect the committed changes.