Page MenuHomePhabricator

Remove TestNode()
ClosedPublic

Authored by nakihito on Apr 22 2020, 00:07.

Details

Reviewers
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGING0a7fb85228f5: Remove TestNode()
rABC0a7fb85228f5: Remove TestNode()
Summary

TestNode() already takes in the majority of CServiceResult and only has one call site.
In addition, having it located in seeder/bitcoin.* when the call site is in seeder/main.cpp
make banning logic more complex and difficult to follow.

Test Plan

Edit chainparams.cpp such that the only seed for TestNet is localhost.

./bitcoind -whitelist=127.0.0.1 --datadir=testdir -listen=1 -connect=0 -debug=net -datadir=testdir
./bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org --testnet --wipeban --wipeignore

Confirm seeder successfully connects with the local node and then connects to the peers obtained by the local node.
Output should start as:

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

Then eventually become something like:

1/581 available (3 tried in 4s, 546 new, 32 active), 0 banned; 0 DNS requests, 0 db queries

Diff Detail

Repository
rABC Bitcoin ABC
Branch
RemoveTestNodeReturn
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 10358
Build 18537: Default Diff Build & Tests
Build 18536: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Apr 22 2020, 00:07
jasonbcox requested changes to this revision.Apr 22 2020, 20:53
jasonbcox added a subscriber: jasonbcox.

It looks like the old code was trying to keep a separation between the DB results and the seeder implementation. Your patch now entangles the two. Neither approach appears to be correct, so that begs the question: Why does TestNode() exist outside of main.cpp? or why does it exist at all when it's only call site is ThreadCrawler?

Unless I'm missing something, you'll likely find that consolidating the code to the one call site will simplify this patch and make the code much easier to understand.

Removed TestNode() completely.

nakihito retitled this revision from Fully pass in CServiceResult to TestNode() and make it void to Remove TestNode().
nakihito edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Apr 22 2020, 21:38
This revision was automatically updated to reflect the committed changes.