Page MenuHomePhabricator

Replace getopts() with gArgs in the Seeder.
Needs ReviewPublic

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

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
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).

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 -?

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 -dnsthreads=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.

bitcoin-seeder -regtest

This should error out with the following message:

Seeder does not support use of regtest.

Diff Detail

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

Event Timeline

nakihito created this revision.Sun, Oct 27, 02:04
Owners added a reviewer: Restricted Owners Package.Sun, Oct 27, 02:04
Herald added a reviewer: Restricted Project. · View Herald TranscriptSun, Oct 27, 02:04
nakihito planned changes to this revision.Sun, Oct 27, 02:04
nakihito edited the test plan for this revision. (Show Details)Sun, Oct 27, 02:06
nakihito retitled this revision from Replace getopts() with gArgs. to Replace getopts() with gArgs in the Seeder..
nakihito added reviewers: Fabien, jasonbcox.
nakihito edited the summary of this revision. (Show Details)Sun, Oct 27, 02:09
nakihito requested review of this revision.Mon, Oct 28, 17:34
jasonbcox requested changes to this revision.Mon, Oct 28, 19:24
jasonbcox added inline comments.
src/seeder/main.cpp
69 ↗(On Diff #13721)

nit: this can be initialized all at once rather than appending immediately after init.

72 ↗(On Diff #13721)

Is there any reason to return to the caller when the program's execution has finished?

93 ↗(On Diff #13721)

The second clause is always true, so it can be removed.

465 ↗(On Diff #13721)

In case of an error, bitcoin-seeder will still exit with status 0.

This revision now requires changes to proceed.Mon, Oct 28, 19:24
nakihito updated this revision to Diff 13743.Tue, Oct 29, 17:24

Fixed nits and removed return from ParseCommandLine(). Function now exits upon error or end of -help options.

deadalnix requested changes to this revision.Wed, Oct 30, 16:07
deadalnix added inline comments.
src/seeder/main.cpp
63 ↗(On Diff #13743)

This function never exited before. It is generally not the best idea to abruptly exit anywhere.

looked into several of the existing application, and this isn't how they do it. What the benefit of using this new pattern?

93 ↗(On Diff #13743)

You did something recently involving this kind of patterns.

461 ↗(On Diff #13743)

It is somewhat strange that the args need to be setup here, when they are parsed somewhere else. This doesn't match how other application we have are doing it. Why the new pattern?

This revision now requires changes to proceed.Wed, Oct 30, 16:07
nakihito edited the test plan for this revision. (Show Details)Mon, Nov 4, 19:45
nakihito updated this revision to Diff 13955.Tue, Nov 5, 23:47

Removed exit() calls, replacing them with returns and a member variable for tracking whether the user used the help option. Also moved SetSeederArgs() to CDnsSeedOpts.

src/seeder/main.cpp
461 ↗(On Diff #13743)

You're correct. After thinking about it, I can see that I am skipping a step here. I've moved the function to the CDnsSeedOpts class.

jasonbcox requested changes to this revision.Thu, Nov 7, 19:26
jasonbcox added inline comments.
src/seeder/main.cpp
76 ↗(On Diff #13955)

Why introduce a member variable when the return value will do? This is indicative that the return type doesn't match the use case.

This revision now requires changes to proceed.Thu, Nov 7, 19:26
nakihito updated this revision to Diff 13984.Fri, Nov 8, 03:57

Added enum class and changed return value of ParseCommandLine() to make use of it.

deadalnix requested changes to this revision.Sun, Nov 10, 13:43
deadalnix added inline comments.
src/seeder/main.cpp
35 ↗(On Diff #13984)

How is RunSeeder even a ParseStatus ?

The structure is better now, but these names are clearly working against the comprehension of the code rather than the opposite.

37 ↗(On Diff #13984)

That seems to be a classic case of smurf naming. If the error wasn't about parsing, then that'd be a very good sign that the whole approach is cursed and should be reconsidered. And if it is, then why say it?

38 ↗(On Diff #13984)

This would probably benefit being scoped in CDnsSeedOpts because ParseStatus is not something that does make sense in isolation. What was parsed? Is it even a status to begin with? The parser having a status would seems to indicate is is still doing something and is in some state - it has a STATus. But in this case, the parsing process is finished.

148 ↗(On Diff #13984)

This is still not how it is done thorough the codebase. It is important because without doing it like the rest of the application expect, you will not be able to use the chain params, and will have to continue to duplicate infos such as net magic.

474 ↗(On Diff #13984)

EXIT_SUCCESS

477 ↗(On Diff #13984)

EXIT_FAILURE

This revision now requires changes to proceed.Sun, Nov 10, 13:43
nakihito updated this revision to Diff 14092.Wed, Nov 13, 00:22

Renamed enum class and moved it within the CSeederOpts class. Changed main() return values to use EXIT_SUCCESS and EXIT_FAILURE.

nakihito planned changes to this revision.Wed, Nov 13, 00:22
nakihito requested review of this revision.Wed, Nov 13, 18:11
nakihito added inline comments.
src/seeder/main.cpp
148 ↗(On Diff #13984)

This is exactly what is done by bitcoin-cli.cpp (line 58 and 149) and chainparamsbase.cpp (lines 23-24). Are you saying I should use SetupChainParamsBaseOptions()? The seeder doesn't really make any sense to run on regtest, so adding that to the help message (or even as an option in general) would only serve to cause confusion.

nakihito planned changes to this revision.Thu, Nov 14, 01:10
nakihito added inline comments.
src/seeder/main.cpp
148 ↗(On Diff #13984)

Sorry, I misunderstood how regtest worked. Got a better understanding of the problem after talking with Jason.

nakihito updated this revision to Diff 14114.Thu, Nov 14, 01:43

Came up with a better solution for the return value of ParseCommandLine(). Changed handling of -testnet option to mirror bitcoin-cli.cpp.

nakihito planned changes to this revision.Thu, Nov 14, 01:43
nakihito requested review of this revision.Thu, Nov 14, 01:44
deadalnix requested changes to this revision.Thu, Nov 14, 14:45
deadalnix added inline comments.
src/seeder/main.cpp
86 ↗(On Diff #14114)

SelectParams and then use the selected params.

This revision now requires changes to proceed.Thu, Nov 14, 14:45
nakihito updated this revision to Diff 14133.Thu, Nov 14, 20:55

ParseCommandLine uses SelectParams() now. Added error message for -regtest option.

nakihito planned changes to this revision.Thu, Nov 14, 20:55
nakihito edited the test plan for this revision. (Show Details)Thu, Nov 14, 20:57
nakihito requested review of this revision.Thu, Nov 14, 20:57
jasonbcox accepted this revision.Thu, Nov 14, 22:07

There's still more work to do to de-dup netMagic, etc from the rest of the seeder, but this is a start.