Paths

Table of Contentst

Start using Logger for seeder logging
Changes PlannedPublic
Actions

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

Details

Reviewers

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

Diff Detail

Repository

rABC Bitcoin ABC

Branch

SeederSimpleLogs

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 11616
Build 22555: Default Diff Build & Tests	Link to the build
Build 22554: Build without Wallet	Link to the build
Build 22553: Build with clang-10	Link to the build
Build 22552: arc lint + arc unit

Event Timeline

nakihito created this revision.May 7 2020, 00:32

Owners added a reviewer: Restricted Owners Package.May 7 2020, 00:32

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 7 2020, 00:32

nakihito requested review of this revision.May 7 2020, 00:32

nakihito planned changes to this revision.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

nakihito edited the test plan for this revision. (Show Details)May 7 2020, 00:37

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10625: Diff 19774.May 7 2020, 00:42

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.

nakihito edited the summary of this revision. (Show Details)May 7 2020, 00:48

deadalnix added a subscriber: deadalnix.May 9 2020, 20:54

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.

nakihito added inline comments.May 11 2020, 17:39

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

nakihito planned changes to this revision.May 20 2020, 20:03

Added release notes and rebased.

Build Bitcoin ABC Diffs / Diff Testing (build-diff) started.

Build Bitcoin ABC Diffs / Diff Testing (build-werror) started.

Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) started.

Build Bitcoin ABC Diffs / Diff Testing (build-werror) passed.

Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) passed.

Build Bitcoin ABC Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10923: Diff 20413.May 21 2020, 00:56

deadalnix requested changes to this revision.May 25 2020, 17:06

deadalnix added inline comments.

src/seeder/main.cpp
544 ↗	(On Diff #20413)	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.