Page MenuHomePhabricator

Merge #12218: net: Move misbehaving logging to net logging category
ClosedPublic

Authored by Fabien on Sep 24 2019, 21:13.

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

Fixed missed reason -> message

Fabien requested changes to this revision.Sep 26 2019, 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.Sep 26 2019, 06:32

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.Sep 27 2019, 13:08

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

This revision now requires changes to proceed.Sep 27 2019, 13:08

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.Sep 27 2019, 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.Sep 27 2019, 19:57
Fabien edited reviewers, added: jasonbcox; removed: Fabien.

Rebase, check that all Misbehaving calls in net_processing.cpp have a message and no duplicated log.

This revision is now accepted and ready to land.Jan 5 2021, 00:48