net_processing: make all Misbehaving increments = 100
Needs ReviewPublic
Actions

Authored by PiRK on Wed, Apr 30, 16:46.

Details

Reviewers

None

Group Reviewers

Restricted Project

Summary

This removes the need to actually track misbehavior score (see further commit), because any
Misbehaving node will immediately hit the discouragement threshold.

This is a partial backport of core#29575
https://github.com/bitcoin/bitcoin/pull/29575/commits/6457c311977bba3585648e32e3bd5754028aa292

Depends on D18031

Test Plan

ninja all check-all-extended

run an ibd with -assumevalid=0 -checkpoints=0`

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 33177
Build 65837: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 65836: arc lint + arc unit

Event Timeline

PiRK created this revision.Wed, Apr 30, 16:46

Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Apr 30, 16:46

PiRK requested review of this revision.Wed, Apr 30, 16:46

teamcity edited the summary of this revision. (Show Details)Wed, Apr 30, 16:46

@bot build-ibd-no-assumevalid-checkpoint

teamcity awarded a token.Wed, Apr 30, 16:47

PiRK added inline comments.Wed, Apr 30, 16:56

test/functional/abc_p2p_version_timestamp.py
68	we now disconnect peers with `VERSION` timestamps older than the genesis block. So we cannot assume that we will have time to detect the successful connection before we get disconnected because of misbehavior when the version message is processed. See change below in test_node.py
test/functional/p2p_timeouts.py
71–73	In our codebase we discourage sending any message before `version` or before `verack`. Keeping these pings makes the test fail intermittently (often) because disconnection can happen before the handshake timeout so the node never logs the expected messages below (expected_timeout_logs).
86	By keeping this one `ping` we can maintain the same log messages. Without it, we would lose the coverage for the message count before the handshake times out. We would need to replace: socket no message in first 3 seconds, 1 0 peer=1 with: socket no message in first 3 seconds, 0 0 peer=1 I didn't see any intermittent test failures by running it 100 times. So I suggest we keep it unless we start seeing test failures on CI

PiRK added a child revision: D18034: net_processing: remove Misbehavior score and increments.Wed, Apr 30, 16:58

Harbormaster completed remote builds in B33177: Diff 53791.Wed, Apr 30, 17:04

PiRK added inline comments.Wed, Apr 30, 17:15

src/test/net_tests.cpp
1296	used by the version message below. Anything lower than the genesis timestamp would cause a disconnection
test/functional/abc_p2p_version_timestamp.py
96	the previous two nodes got disconnected before this one was connected

I didn't see any intermittent test failures by running it 100 times. So I suggest we keep it unless we start seeing test failures on CI

It didn't take long before failing on CI: https://reviews.bitcoinabc.org/D18034#411094

fix the p2p_timeouts.py flakiness by dropping the ping test.

Harbormaster completed remote builds in B33181: Diff 53798.Wed, Apr 30, 17:39

I ran a full IBD + 12 hours of normal operation, got about 2100 connections during this time, and didn't see a single "Misbehavior" log

Fabien added a subscriber: Fabien.Thu, May 1, 11:04

Fabien added inline comments.

src/net_processing.cpp
2717 ↗	(On Diff #53798)	how does this behave if a block is avalanche rejected and a miner sends a block that connects to it (case of mining 2 blocks quickly, the first one ending up invalid) ?