p2p: clean up Misbehaving() #19583

jnewbery commented at 3:45 PM on July 24, 2020: member

This PR makes a few minor clean-ups to Misbehaving() in preparation to move it out of the cs_main lock.

There are very minor logging changes but otherwise no functional changes.

fanquake added the label P2P on Jul 24, 2020

jnewbery commented at 3:47 PM on July 24, 2020: member

This is the next PR in the #19398 sequence to move application data into net_processing.

in src/net_processing.cpp:1070 in 2b5057de16 outdated

1064 |  {
1065 | -    if (howmuch == 0)
1066 | -        return;
1067 | +    assert(howmuch > 0);
1068 |  
1069 | -    CNodeState *state = State(pnode);

Saibato commented at 3:50 PM on July 24, 2020:

assert ?,== DoS ?

laanwj added this to the "Blockers" column in a project

in src/net_processing.cpp:3734 in 2b5057de16 outdated

3733 |          state.m_should_discourage = false;
3734 |      } // cs_main
3735 |  
3736 |      if (pnode.HasPermission(PF_NOBAN)) {
3737 | -        // Peer has the NOBAN permission flag - log but don't disconnect
3738 | +        // We never disconnect or discourage peers for bad behavior if they have the NOBAN permission flag

jonatack commented at 12:50 PM on July 25, 2020:

dd9b305 whitespace linter needs appeasing (rm space at EOL)

        // We never disconnect or discourage peers for bad behavior if they have the NOBAN permission flag

jonatack commented at 12:52 PM on July 25, 2020:

am reviewing anyway, so can wait on this until the review, if you like

jnewbery commented at 2:51 PM on July 25, 2020:

Fixed

in src/net_processing.cpp:1847 in 2b5057de16 outdated

1843 | @@ -1848,7 +1844,7 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateMan
1844 |              UpdateBlockAvailability(pfrom.GetId(), headers.back().GetHash());
1845 |  
1846 |              if (nodestate->nUnconnectingHeaders % MAX_UNCONNECTING_HEADERS == 0) {
1847 | -                Misbehaving(pfrom.GetId(), 20);
1848 | +                Misbehaving(pfrom.GetId(), 20, strprintf("%d unconnecting headers", nodestate->nUnconnectingHeaders));

jonatack commented at 1:11 PM on July 25, 2020:

2d4e694 the log message might be more clear for users if s/unconnecting/non-connecting/...yes, in the codebase it is called unconnecting...but nicely enough, anyone grepping for "non-connecting" will see this line as the only result and link it immediately to nUnconnectingHeaders

jonatack commented at 1:25 PM on July 25, 2020:

(Maybe just me, but "unconnecting" strikes me as confusing naming; it suggests disconnection when we are really talking about invalid or non-connecting header messages, unless I am misreading the code in ProcessHeadersMessage() in net_processing.cpp and CBlockIndex* LookupBlockIndex in validation.cpp.)

jnewbery commented at 2:51 PM on July 25, 2020:

Agree that unconnecting sounds more like a verb than an adjective. I've changed this to non-connecting.

jonatack commented at 1:17 PM on July 25, 2020: member

Pre-linter-appeasement ACK 2b5057de161e34 with one logging nit.

I think the last two commits touching Misbehaving() can be squashed; the changes are light and it's easier to review them together.

[net processing] Fixup MaybeDiscourageAndDisconnect() style

Based on review comments from Marco Falke and Jon Atack.

634144a1c2

[net processing] Always supply debug message to Misbehaving()

Misbehaving() could optionally take a debug string for printing to the
log file. Make this mandatory and always provide the string.

A couple of additional minor changes:

- remove the unnecessary forward declaration of Misbehaving()
- don't include the nodeid or newline in the passed debug message.
Misbehaving() adds these itself.

d15b3afb4c

[net processing] Tidy up Misbehaving()

- Make const things const.
- Replace conditional return with assert.
- Don't log the peer's IP address.
- Log the name Misbehaving directly instead of relying on __func__.

a8865f8b72

jnewbery force-pushed on Jul 25, 2020

jnewbery commented at 2:53 PM on July 25, 2020: member

Thanks for the quick review @jonatack

I think the last two commits touching Misbehaving() can be squashed;

Done

fjahr approved

fjahr commented at 3:36 PM on July 25, 2020: member

Code review ACK a8865f8b7215a975fd3dd9d97d7f791ac93ea65d

jonatack commented at 4:06 PM on July 25, 2020: member

ACK a8865f8

in src/net_processing.cpp:1064 in a8865f8b72

1058 | @@ -1062,23 +1059,22 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans)
1059 |   * Increment peer's misbehavior score. If the new value >= DISCOURAGEMENT_THRESHOLD, mark the node
1060 |   * to be discouraged, meaning the peer might be disconnected and added to the discouragement filter.
1061 |   */
1062 | -void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1063 | +void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1064 |  {
1065 | -    if (howmuch == 0)
1066 | -        return;
1067 | +    assert(howmuch > 0);

promag commented at 10:24 AM on July 26, 2020:

I've verified all callers, this is is correct.

Saibato commented at 6:07 PM on July 26, 2020:

I've verified all callers, this is is correct.

While this is true, if you assert on a const that gives the cpu regs a lot of control power, imho.

sipa commented at 6:08 PM on July 26, 2020:

@saibato I have no clue what you're trying to say here.

Saibato commented at 6:12 PM on July 26, 2020:

we changed howmuch to const int , and then we assert, i saw such things optimized to regs. and the cpu is not always in your control and here we are at a net sanity check that should not be hookable.imho.

sipa commented at 6:50 PM on July 26, 2020:

@saibato Please stop spewing gibberish.

troygiorshev commented at 6:29 PM on July 27, 2020:

Bugs me that I can't find a way to check this at compile time (like static_assert), being that we're always giving this parameter an integer literal :/

sipa commented at 7:45 PM on July 27, 2020:

If you make it a templated function, you can pass in howmuch as a template parameter, enabling static_asserts on it. That may be a bit overkill, though.

jnewbery commented at 9:27 PM on July 27, 2020:

That may be a bit overkill, though.

And also would cause bloat in the built executable.

It's easy enough to grep all instances of Misbehaving. We don't need to automate everything :)

promag commented at 10:27 AM on July 26, 2020: member

Code review ACK a8865f8b7215a975fd3dd9d97d7f791ac93ea65d.

Dunno if this breaks some existing log parser, but it's an easy fix, unless it needs the peer IP.

jnewbery commented at 1:33 PM on July 26, 2020: member

Thanks for the review @promag!

Dunno if this breaks some existing log parser, but it's an easy fix, unless it needs the peer IP.

We shouldn't be logging the peer IP inside net processing, and the I believe the only other place we do is during the version handshake. See #19472 (review).

hebasto commented at 5:48 PM on July 27, 2020: member

Concept ACK.

troygiorshev commented at 6:42 PM on July 27, 2020: contributor

tACK a8865f8b7215a975fd3dd9d97d7f791ac93ea65d

fanquake renamed this:
~~Clean up Misbehaving()~~
p2p: clean up Misbehaving()
on Jul 28, 2020

fanquake merged this on Jul 28, 2020

fanquake closed this on Jul 28, 2020

fanquake removed this from the "Blockers" column in a project

jnewbery deleted the branch on Jul 28, 2020

sidhujag referenced this in commit a7a9b4a9a8 on Jul 28, 2020

deadalnix referenced this in commit 772ce26165 on Jan 6, 2021

DrahtBot locked this on Feb 15, 2022