Page MenuHomePhabricator

Update Seeder to use ArgsManager
AbandonedPublic

Authored by nakihito on Oct 7 2019, 23:14.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

Seeder code is very old and was still using tools like getopt() to
read options. This change attempts to bring the seeder code more in
line with similar files (like bitcoind.cpp) in the code base and modernize the tools it
uses. To accomplish this, the CDnsSeedOpts class has been removed as its purpose
is made almost completely obsolete by ArgsManager.

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).

Depends on D4222

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

These should all output the seeder help message

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

This should start the seeder with the following lines:

Supporting whitelisted filters: 0x1,0x5
Using testnet.
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.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
UpdateSeederHelp
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7758
Build 13555: Bitcoin ABC Buildbot (legacy)
Build 13554: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Oct 7 2019, 23:14

Fixed typo wipte -> wipe.

nakihito edited the summary of this revision. (Show Details)
nakihito requested review of this revision.Oct 8 2019, 00:08
deadalnix requested changes to this revision.Oct 8 2019, 15:16

There was a nice separation between argument processing and application initialization before, now there isn't. This clearly makes the code worse.

src/seeder/main.cpp
377 ↗(On Diff #13394)

It is worth noting that this isn't present in other utilities we have, and yet, this parameter works. There must be something at play.

419 ↗(On Diff #13394)

Is that necessary? It doesn't look like there is a test plan for this.

546 ↗(On Diff #13394)

...

599 ↗(On Diff #13394)

Clearly, AppInit isn't doing simply initialization. In addition, why is that necessary?

This revision now requires changes to proceed.Oct 8 2019, 15:16

Ha, this also breaks single letter parameters names, without release notes or any documentation, because why wouldn't it?

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

Completely removed CDnsSeedOpts class and some redundant variables. Also added release notes and edited README file to reflect changes relevant to users.

Removed AppInit() and moved functionality to main().

deadalnix requested changes to this revision.Oct 14 2019, 17:10

This is almost impossible to figure out if this patch missed something or not as it combine logic changes with refactoring.

It is introducing an inordinate amount of duplicated knowledge (how places needs to be changed is the default nuber of thread for dnsthreads needs to be updated for instance?).

You need to refactor the code in small steps in order to get to a place where moving to gArgs is a trivial change - however big it may be.

src/seeder/main.cpp
168

There was a nice function validating the parameters before doing anything. Now the validation code and the initialization logic are all mixed up. This is a clear regression.

540

threads is not a size_t, so why is the look counter a size_t ?

This revision now requires changes to proceed.Oct 14 2019, 17:10