Make invalid network magic cause Misbehaving points
AbandonedPublic
Actions

Authored by OverlordQ on Jan 11 2018, 22:00.

Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Summary

Clients repeatdly connecting with invalid magic will eventually get banned

Test Plan

make check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

arcpatch-D934

Lint

Lint Skipped

Unit

No Test Coverage

Build Status

Buildable 1586
Build 1586: arc lint + arc unit

Event Timeline

OverlordQ created this revision.Jan 11 2018, 22:00

Owners added a reviewer: Restricted Owners Package.Jan 11 2018, 22:00

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 11 2018, 22:00

Harbormaster completed remote builds in B1577: Diff 2457.Jan 11 2018, 22:00

Thanks for the patch, this is very much needed. However,

lackoftest

This revision now requires changes to proceed.Jan 12 2018, 00:14

Also I'd probably put something a bit higher for penalty. It's gonna take forever before a node is banned that way.

I'll add in some tests.

I figured on a lower amount JIC there was a misconfigured pre-magic switch Cash node out there to not inadvertently get them blacklisted also.

Changes:

Increase penalty
Add functional test

Harbormaster completed remote builds in B1579: Diff 2458.Jan 12 2018, 02:21

Still some work to be done, just wanted to get the WIP updated.

Fix test and banning

Arcanist was throwing an error if I didn't skip linting.

[2018-01-12 06:31:50] EXCEPTION: (Exception) Two objects (of classes "ClangFormatLinter" and "FnClangFormatLinter", descendants of ancestor class "ArcanistLinter") returned the same key from "getLinterConfigurationName()" ("clang-format"), but each object in this class map must be identified by a unique key. at [<phutil>/src/symbols/PhutilClassMapQuery.php:268]
arcanist(custom=1), arcanist-abc(head=arcpatch-D934, ref.master=16cbb9e86e3f, ref.arcpatch-D934=874389cf9672), fn-phabricator-linters(), phutil()
  #0 PhutilClassMapQuery::loadMap() called at [<phutil>/src/symbols/PhutilClassMapQuery.php:179]
  #1 PhutilClassMapQuery::execute() called at [<arcanist>/src/lint/engine/ArcanistConfigurationDrivenLintEngine.php:143]
  #2 ArcanistConfigurationDrivenLintEngine::loadAvailableLinters() called at [<arcanist>/src/lint/engine/ArcanistConfigurationDrivenLintEngine.php:32]
  #3 ArcanistConfigurationDrivenLintEngine::buildLinters() called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:166]
  #4 ArcanistLintEngine::run() called at [<arcanist>/src/workflow/ArcanistLintWorkflow.php:334]
  #5 ArcanistLintWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]

This diff contains only part of your changes. Presumably you have several commits and submit only the topmost one. If that's the case you use git rebase -i master and mark all the commit but the first one as f and then run arc diff again.

Also, from what I can see, this looks pretty damn good.

Correct diff

Harbormaster completed remote builds in B1586: Diff 2467.Jan 12 2018, 19:36

deadalnix added inline comments.Jan 13 2018, 02:45

test/functional/p2p-badmagic.py
144	This is banning the node and check it is banned. This should be checking that the node gets banned when using the wrong magic a few times. To do so, 127.0.0.0/24 should be removed from the whitelist of the node.

OverlordQ added inline comments.Jan 13 2018, 03:29

test/functional/p2p-badmagic.py
144	I didn't see a way around not whitelisting localhost due to the CNetAddr::IsLocal check in the CheckIfBanned function, and I didn't feel like I should fiddle in there also.

deadalnix added inline comments.Jan 13 2018, 18:49

test/functional/p2p-badmagic.py
144	Looking into it, it seems indeed difficult. Maybe that's something we want to fix, but it's a bit outside the scope of this diff. Maybe one thing that can be done here is to check the banscore of the faulty node ?

OverlordQ added inline comments.Jan 13 2018, 21:28

test/functional/p2p-badmagic.py
144	So when it sends bad magic it gets disconnected immediately so all the associated meta such has hostid/ip/banscore/etc gets dropped as well. So you can't use getpeerinfo to grab the banscore as only connected nodes appear there. So without refactoring I'm not sure there's a easy way to accurately test this in the python test suite.