Page MenuHomePhabricator

Integrate gArgs and chainparams into the Seeder
ClosedPublic

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

Details

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 Passed
Unit
No Test Coverage
Build Status
Buildable 8234
Build 14494: Default Diff Build & Tests
Build 14493: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Removed fTestNet and fixed regtest default port and height.

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.

deadalnix requested changes to this revision.Nov 20 2019, 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.Nov 20 2019, 21:39

Completely integrated chainparams into seeder.

nakihito retitled this revision from Replace getopts() with gArgs in the Seeder. to Integrate gArgs and chainparams into the Seeder.Nov 20 2019, 23:41
nakihito edited the summary of this revision. (Show Details)

Removed an unnecessary header file.

deadalnix requested changes to this revision.Nov 21 2019, 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.Nov 21 2019, 15:59

Restored netMagic and rebased off D4417.

Clarified some wording in release notes and fixed formatting.

src/seeder/CMakeLists.txt
20 ↗(On Diff #14334)

Discussed offline with Fabien as an alternative to D4476.

deadalnix requested changes to this revision.Nov 23 2019, 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.Nov 23 2019, 01:42

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

This revision is now accepted and ready to land.Nov 27 2019, 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 requested review of this revision.Dec 2 2019, 21:38
deadalnix requested changes to this revision.Dec 2 2019, 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.Dec 2 2019, 23:42

seeder-net -> seeder-netprocessing.

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 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.Dec 4 2019, 17:09