Page MenuHomePhabricator

Merge #12218: net: Move misbehaving logging to net logging category
Needs RevisionPublic

Authored by jasonbcox on Tue, Sep 24, 21:13.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
Summary

d3a185a net: Move misbehaving logging to net logging category (Wladimir J. van der Laan)

Pull request description:

This moves the error messages for misbehavior (when available) into the line that reports the misbehavior, as well as moves the logging to the `net` category.

This is a continuation of #11583 and avoids serious-looking errors due to misbehaving peers. As it is impossible to correlate the `peer=X` numbers to specific incoming connections now without enabling the `net` category, it doesn't really help to see these messages by default.

To do this, Misbehaving() gains an optional `message` argument.

E.g. change:

    2018-01-18 16:02:27 Misbehaving: x.x.x.x:62174 peer=164603 (80 -> 100) BAN THRESHOLD EXCEEDED
    2018-01-18 16:02:27 ERROR: non-continuous headers sequence

to

    2018-01-18 16:02:27 Misbehaving: x.x.x.x:62174 peer=164603 (80 -> 100) BAN THRESHOLD EXCEEDED: non-continuous headers sequence

When there is a category for "important" net messages (see #12219 ), we should move it there.

Tree-SHA512: 51c97e9a649bf5409f2fd4625fa1243a036e9c9de6037bb064244207408c2e0eb025e3af80866df673cdc006b8f35dc4078d074033f0d4c6a73bbb03949a269f

Backport of Core PR12218
https://github.com/bitcoin/bitcoin/pull/12218/files

Test Plan
make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
pr12218
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7604
Build 13248: Bitcoin ABC Teamcity Staging
Build 13247: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Tue, Sep 24, 21:13
jasonbcox added inline comments.Tue, Sep 24, 21:20
src/net_processing.cpp
1559 ↗(On Diff #13125)

Reviewer note: This and similar reason messages were introduced in D380, but do not appear to be utilized in tests or otherwise. I removed them in favor of Core's messages.

1657 ↗(On Diff #13125)

Reviewer note: Where appropriate, I retained the state.GetRejectReason() (introduced in D380) as it adds value for debugging.

src/net_processing.h
129 ↗(On Diff #13125)

Reviewer note: D380 introduced reason. I've changed it to message to match Core and reduce future backporting friction.

jasonbcox updated this revision to Diff 13126.Tue, Sep 24, 21:21

Fixed missed reason -> message

Fabien requested changes to this revision.Thu, Sep 26, 06:32
Fabien added a subscriber: Fabien.

Not sure about the pfrom to pfrom->GetId() reversal, that seems to make the call more verbose for no good reason. The pfrom form is the one used constantly in net_processing.cpp.

src/net_processing.cpp
2179 ↗(On Diff #13126)

Nit, keep the mention to oversize to make to message more explicit about the issue.

2256 ↗(On Diff #13126)

Dito.

2321 ↗(On Diff #13126)

Dito.

2792 ↗(On Diff #13126)

I know core does the same, but this construct looks pretty bad:

  • The peer info ends up duplicated with Misbehaving
  • The string is simply duplicated with the exact same content, leading to twice the maintenance
2907 ↗(On Diff #13126)

Dito

3134 ↗(On Diff #13126)

Dito, too many headers is a more explicit error message.

This revision now requires changes to proceed.Thu, Sep 26, 06:32
jasonbcox updated this revision to Diff 13165.Fri, Sep 27, 00:41

Addressed feedback:

  • Reverted back to using *CNode rather than NodeId for calls to Misbehaving()
  • Removed superfluous peer numbers: Peer %d did something... -> Peer did something...
  • Added back descriptive error codes.

Also removed some duplicate newlines at the end of error messages. These are unnecessary because Misbehaving() appends them for us.

deadalnix requested changes to this revision.Fri, Sep 27, 13:08

There are several extra calls to Misbehaving from our codebase that require updating.

This revision now requires changes to proceed.Fri, Sep 27, 13:08
jasonbcox requested review of this revision.Fri, Sep 27, 17:24

There are several extra calls to Misbehaving from our codebase that require updating.

The only other calls I see are not in scope of this diff. I've applied a separate change so that they use the overload, however: D4172

deadalnix requested changes to this revision.Fri, Sep 27, 19:57

I'm not talking about tests. Glancing quickly at net_processing.cpp, I found cases that would benefit from the same type of refactoring as done here that are not part of the orginal PR because the corresponding code doesn't exist in Core.

This revision now requires changes to proceed.Fri, Sep 27, 19:57