Page MenuHomePhabricator

Add options to seeder to determine initial seed list at start up
Changes PlannedPublic

Authored by nakihito on May 18 2020, 22:47.

Details

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

-connect and -addseed can be used to customize the initial list of seeds upon
the seeder's start up. Specifically, -connect can be used to tell the seeder whether
or not to use the default list of seeds. -addnode can be used to append an ip to the
initial list of seeds (after processing the results of -connect).

Test Plan
ninja
./bitcoin-seeder --help

Verify new options appear and their text

./bitcoind
./bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org -wipeban -wipeignore
./bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org -wipeban -wipeignore -connect=1

Verify behavior is the same as pre-patch for both, something like below at start:

0/123 available (0 tried in 0s, 107 new, 16 active), 0 banned; 0 DNS requests, 0 db queries

And then eventually something like below after running for a minute or two:

2/2392 available (43 tried in 29s, 1234 new, 1115 active), 0 banned; 0 DNS requests, 0 db queries
./bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org -wipeban -wipeignore -connect=0 -addnode=localhost
./bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org -wipeban -wipeignore -connect=localhost

Verify the initial output of the seeder window is similar to as follows:

0/1 available (1 tried in 6s, 0 new, 0 active), 0 banned; 0 DNS requests, 0 db queries

Let the seeder continue to run. The seeder's output should eventually become similar to:

0/304 available (1 tried in 2s, 287 new, 16 active), 0 banned; 0 DNS requests, 0 db queries

Event Timeline

Owners added a reviewer: Restricted Owners Package.May 18 2020, 22:47

Increased the usability of new option.

nakihito retitled this revision from Add a -debugseedlist option to seeder to limit initial seed list to Add options to seeder to determine initial seed list at start up.
nakihito edited the summary of this revision. (Show Details)
nakihito edited the test plan for this revision. (Show Details)
jasonbcox requested changes to this revision.May 20 2020, 17:35
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/seeder/main.cpp
160

This help text is not useful for users for the following reasons:

  • Users may not know what chainparams are. There's clearly a default seed list somewhere, but who cares?
  • Users don't care how this works under the hood. Why tell them that it clears a list they don't care about if -connect=0 ?
  • The -addseed option is extra, so adding some of its help text in the middle is confusing. Just tack on See also: -addseed to the end.
This revision now requires changes to proceed.May 20 2020, 17:35
jasonbcox requested changes to this revision.May 20 2020, 21:15

My original feedback still hasn't been entirely incorporated. Look at the name (-connect) and the description you've given it (essentially: "do some things related to seed lists but say absolutely nothing about connecting"). This is incredibly confusing for a user that has no idea how a seeder works. They know nothing about any default seeds.

This revision now requires changes to proceed.May 20 2020, 21:15
deadalnix added inline comments.
src/seeder/main.cpp
160 ↗(On Diff #20393)

What is it for? What problem does this solves?

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

While it is stated nowhere, I can imagine what the purpose of -addseed might be. I have no idea what problem does the connect option solves - not even consistency because names have been changed from bitcoind. That is by far the biggest concern I have with this as one can come up with random things to do that solves no problem essentially forever, creating a mountain of useless code in the process, and usually a few bugs in the process.

In light of D6176 , there is clearly a problem because the addseed nodes are supposed to be added manually, not automatically.

src/seeder/main.cpp
485 ↗(On Diff #20393)

bitcoind uses "-addnode", why is this using "-addseed"?

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