Page MenuHomePhabricator

Start using Logger for seeder logging
Changes PlannedPublic

Authored by nakihito on May 7 2020, 00:32.

Details

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

Currently, the seeder uses tfm::format() to print directly to console.
Switching to using LogPrint() will allow the seeder to log some of its
output to a .log file that can be read without worrying about having
to catch a specific message on screen.

This change only allows some output to be printed to file to avoid having
the log file grow unexpectedly large after running the seeder for long
periods of time.

Depends on D5956

Test Plan
ninja
mkdir dirtest
./bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org --seederdir=dirtest

Verify there is no dirtest/seeder/seederdebug.log and that the seeder prints all of its output to console as before patch (excluding the failed connection warning).

./bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org --seederdir=dirtest -logtofile

Verify the existance and contents of dirtest/seeder/seederdebug.log looks like:

2020-05-07T00:27:01Z Bitcoin ABC Seeder version v0.21.6-0d30fb8d0
2020-05-07T00:27:01Z Supporting whitelisted filters: 0x1,0x5,0x11,0x15
2020-05-07T00:27:01Z Starting 4 DNS threads for localhost on blah.bitframe.org (port 8888)...done
2020-05-07T00:27:01Z Starting seeder...done
2020-05-07T00:27:01Z Starting 96 crawler threads...done
2020-05-07T00:27:46Z Cannot create socket for pudcfw4jhhtvi4hz.onion:8333: unsupported network
2020-05-07T00:27:49Z Cannot create socket for 6p2pmunkvwmkk3gs.onion:8333: unsupported network
...

./bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org --seederdir=dirtest -noprinttoconsole

Verify that the seeder outputs the following to console:

Warning: relative seederdir option 'dirtest' specified, which will be interpreted relative to the current working directory '/home/nico/bitcoin-abc/build'. This is fragile, because if the seeder is started in the future from a different location, it will be unable to locate the current data files.
Using main.
[20-05-06 17:36:30] 62/4864 available (361 tried in 544s, 3473 new, 1030 active), 0 banned; 0 DNS requests, 0 db queries

Event Timeline

Owners added a reviewer: Restricted Owners Package.May 7 2020, 00:32
nakihito requested review of this revision.May 7 2020, 00:32
nakihito planned changes to this revision.
nakihito requested review of this revision.May 7 2020, 00:46
nakihito added inline comments.
src/seeder/bitcoin.cpp
263 ↗(On Diff #19774)

This change prevents the seeder from flooding the log file and/or console with unsuccessful socket connection warnings causing the log file's size to explode.

deadalnix added inline comments.
src/seeder/bitcoin.cpp
263 ↗(On Diff #19774)

Specifying what the parameter actually means - and if we actually want to change the behavior - rather than changing the behavior randomly because the log output is large certainly would inspire more confidence.

263 ↗(On Diff #19774)

Note that if the previous behavior was wrong, then fixing it is warranted regardless of change in logging. If it wasn't wrong, then this is introducing a bug.

src/seeder/bitcoin.cpp
263 ↗(On Diff #19774)

The parameter simply determines the log category connection failure message falls under (see https://reviews.bitcoinabc.org/source/bitcoin-abc-staging/browse/master/src/netbase.cpp$583-586 and https://reviews.bitcoinabc.org/source/bitcoin-abc-staging/browse/master/src/netbase.cpp$528. Because the seeder did not use the Logger class before, the parameter had no affect on behavior.

deadalnix requested changes to this revision.May 18 2020, 18:52
deadalnix added inline comments.
src/seeder/bitcoin.cpp
263 ↗(On Diff #19774)

No, the parameter specify if this is a manual connection or not and it happens that manual connections are logged differently.

Is it correct to qualify these connection are manual or should they not be manual? If the value passed is wrong, then this needs to be fixed, regardless of the impact on logging.

This is the whole point of using abstractions. If you need to dig into the other function to know this is changes only logging, then this is wrong, but even worse, if modifying this function require to be aware of every callers then things are bound to break.

This revision now requires changes to proceed.May 18 2020, 18:52

Added release notes and rebased.

deadalnix requested changes to this revision.May 25 2020, 17:06
deadalnix added inline comments.
src/seeder/main.cpp
544

You are effectively duplicating InitLogging for what appear to be no good reason. In fact the code in this patch is already inconsistent with the original. Options have different names and do different things.

This revision now requires changes to proceed.May 25 2020, 17:06

Changed initialization code and new options to more closely resemble InitLogging().