net: Add unit testing of node eviction logic #20477

practicalswift · 2020-11-24T12:47:07Z

Add unit testing of node eviction logic.

Closes #19966.

DrahtBot · 2020-11-24T19:48:42Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

net: fix GetListenPort() to derive the proper port #20196 (net: fix GetListenPort() to derive the proper port by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jonatack · 2020-11-24T22:16:30Z

Thanks for working on adding testing here. I'll try to review this soon.

src/net.cpp

laanwj · 2020-12-03T12:12:56Z

Concept ACK. Thanks for adding tests.

I sometimes wonder if it would make sense to move "semi-internal" things that are only exposed externally for unit testing, like in this case "NodeEvictionCandidate" and "SelectNodeToEvict", to a separate set of headers. But not here anyhow.

dhruv

utACK 2c62fb6. Thank you, @practicalswift for making the code more testable. Also for exposure to idiomatic modern C++. I will checkout the branch and run soon.

src/test/net_tests.cpp

src/net.cpp

src/test/net_tests.cpp

maflcko · 2020-12-05T05:35:57Z

tsan failue can be ignored or fixed by a rebase

practicalswift · 2020-12-05T10:05:21Z

@MarcoFalke Thanks! Rebased!

src/net.cpp

jnewbery

Very strong concept ACK. This area of the code is definitely undertested. The structure of the unit tests (passing in a setup lambda function to set the relevant parameters in the eviction candidates) is really nice.

src/test/net_tests.cpp

maflcko · 2020-12-07T11:51:21Z

needs rebase

practicalswift · 2020-12-07T12:27:21Z

I understand why you'd want to pass by ref, but there's no benefit to passing by rvalue. This code suggests to me that SelectNodeToEvict() is going to keep ownership of vEvictionCandidates and you're using move semantics to avoid a copy, but that's not actually happening here.

I've now reverted to the original version (alternative 1 below), since your suggestion and sipa's suggestion are mutually exclusive :)

I'll let others chime in regarding the choice between the three alternatives:

Alternative 1. Optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>)
Alternative 2. Optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&)
Alternative 3. Optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&&)

Trying to summarize the pros/cons:

Alternative 1. Simple and no gotchas. Does an extraneous copy.
Alternative 2. No extraneous copy. Possibly surprising that a function named SelectNodeToEvict modifies its input.
Alternative 3. No extraneous copy. Risk of accidental use-after-moved-from in the future. Possibly confusing (see net: Add unit testing of node eviction logic #20477 (comment)).

Perhaps the members of the review club could chime in. Some questions for the review club: Do you agree with the pros/cons of each alternative? Which alternative do you find easier to review? Which alternative do you prefer? Is it possible to quantify the real-world impact of the extraneous copy in a theoretical worst-case scenario?

jonatack · 2020-12-11T16:13:14Z

Will review properly as soon as my laptop is done with the gitian builds for 3 RCs...until then, it's basically unusable.

practicalswift · 2020-12-15T15:25:56Z

Updated to address @dhruv's feedback. Should hopefully be ready for final review. Please re-review :)

dhruv · 2020-12-15T19:42:56Z

ACK 1c9b235

maflcko · 2020-12-16T10:04:58Z

cr ACK 6767d63 🤠

Show signature and timestamp

Signature:

Timestamp of file with hash 23398d20d71a2405d911d0dc99e4fc62b592150f49fa6c17afd6d588c418522c -

maflcko · 2020-12-16T10:06:53Z

cr ACK 1c9b235 👲

Show signature and timestamp

Signature:

Timestamp of file with hash 3a47dabfa2f12ebc4fbd136dcd7771912bfcd7ed7ce86ba72f473c364fba70a0 -

maflcko · 2020-12-16T10:07:49Z

I ACKed both versions. Let me know which one to merge.

jnewbery · 2020-12-16T10:11:56Z

code review ACK 1c9b235

Either are fine to merge.

jonatack · 2020-12-16T10:36:18Z

wait for meeee 😀

src/net.cpp

src/test/net_tests.cpp

jonatack · 2020-12-16T11:30:09Z

ACK 1c9b235 modulo a few suggestions

…didates.

practicalswift · 2020-12-16T12:01:18Z

@jonatack Thanks for reviewing! Feedback addressed: please re-review :)

jonatack · 2020-12-16T12:16:28Z

ACK fee8823

Thanks for adding this nicely done coverage! Looking forward to more.

maflcko · 2020-12-16T12:30:37Z

ACK fee8823 🐼

Show signature and timestamp

Signature:

Timestamp of file with hash fb125172e7abd1e8e484ae4f527e5e86e05737d1b1e006310dca8dbb38203afc -

…eviction protection test coverage 0cca08a Add unit test coverage for our onion peer eviction protection (Jon Atack) caa21f5 Protect onion+localhost peers in ProtectEvictionCandidatesByRatio() (Jon Atack) 8f1a53e Use EraseLastKElements() throughout SelectNodeToEvict() (Jon Atack) 8b1e156 Add m_inbound_onion to AttemptToEvictConnection() (Jon Atack) 72e30e8 Add unit tests for ProtectEvictionCandidatesByRatio() (Jon Atack) ca63b53 Use std::unordered_set instead of std::vector in IsEvicted() (Jon Atack) 41f84d5 Move peer eviction tests to a separate test file (Jon Atack) f126cbd Extract ProtectEvictionCandidatesByRatio from SelectNodeToEvict (Jon Atack) Pull request description: Now that #19991 and #20210 have been merged, we can determine inbound onion peers using `CNode::m_inbound_onion` and add it to the localhost peers protection in `AttemptToEvictConnection`, which was added in #19670 to address issue #19500. Update 28 February 2021: I've updated this to follow gmaxwell's suggestion in #20197 (comment). This branch now protects up to 1/4 onion peers (connected via our tor control service), if any, sorted by longest uptime. If any (or all) onion slots remain after that operation, they are then allocated to protect localhost peers, or a minimum of 2 localhost peers in the case that no onion slots remain and 2 or more onion peers were protected, sorted as before by longest uptime. This patch also adds test coverage for the longest uptime, localhost, and onion peer eviction protection logic to build on the welcome initial unit testing of #20477. Suggest reviewing the commits that move code with `colorMoved = dimmed-zebra` and `colorMovedWs = allow-indentation-change`. Closes #11537. ACKs for top commit: laanwj: Code review ACK 0cca08a vasild: ACK 0cca08a Tree-SHA512: 2f5a63f942acaae7882920fc61f0185dcd51da85e5b736df9d1fc72343726dd17da740e02f30fa5dc5eb3b2d8345707aed96031bec143d48a2497a610aa19abd

Summary: ``` Onion peers are disadvantaged under our eviction criteria, so prevent eventual eviction of them in the presence of contention for inbound slots by reserving some slots for localhost peers (sorted by longest uptime). Block-relay-only connections exist as a protection against eclipse attacks, by creating a path for block propagation that may be unknown to adversaries. Protect against inbound peer connection slot attacks from disconnecting such peers by attempting to protect up to 8 peers that are not relaying transactions but appear to be full-nodes, sorted by recency of last delivered block. ``` Backport of [[bitcoin/bitcoin#19670 | core#19670]]. Note that tests will be added when porting this PR: bitcoin/bitcoin#20477 Ref T1611. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1611 Differential Revision: https://reviews.bitcoinabc.org/D9676

Summary: Backport of [[bitcoin/bitcoin#20477 | core#20477]]. Depends on D9683. Ref T1611. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1611 Differential Revision: https://reviews.bitcoinabc.org/D9684

practicalswift force-pushed the unit-test-node-eviction-logic branch from a989adf to 9b6f891 Compare November 24, 2020 12:48

fanquake added P2P Tests labels Nov 24, 2020

practicalswift force-pushed the unit-test-node-eviction-logic branch 3 times, most recently from cb12388 to 2c62fb6 Compare November 24, 2020 15:28

DrahtBot mentioned this pull request Nov 24, 2020

net: fix GetListenPort() to derive the proper port #20196

Merged

DrahtBot mentioned this pull request Nov 24, 2020

fuzz: Add fuzzing harness for node eviction logic #19972

Merged

practicalswift mentioned this pull request Nov 25, 2020

Add unit tests for eviction logic #19966

Closed

laanwj reviewed Dec 3, 2020

View reviewed changes

src/net.cpp Show resolved Hide resolved

dhruv reviewed Dec 4, 2020

View reviewed changes

src/test/net_tests.cpp Outdated Show resolved Hide resolved

src/test/net_tests.cpp Outdated Show resolved Hide resolved

src/net.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Dec 4, 2020

net: Assume that SetCommonVersion is called at most once per peer #20138

Merged

jnewbery reviewed Dec 4, 2020

View reviewed changes

jnewbery added the Review club label Dec 4, 2020

practicalswift force-pushed the unit-test-node-eviction-logic branch from 2c62fb6 to 8da8e88 Compare December 4, 2020 22:40

practicalswift force-pushed the unit-test-node-eviction-logic branch from 8da8e88 to 071b4cd Compare December 5, 2020 10:05

jnewbery reviewed Dec 7, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Dec 7, 2020

View reviewed changes

src/test/net_tests.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the unit-test-node-eviction-logic branch from 071b4cd to 67f3759 Compare December 7, 2020 11:47

maflcko added the Needs rebase label Dec 7, 2020

practicalswift force-pushed the unit-test-node-eviction-logic branch 2 times, most recently from 4d4639d to ccc8162 Compare December 7, 2020 12:03

maflcko changed the title ~~test/net: Add unit testing of node eviction logic~~ net: Add unit testing of node eviction logic Dec 7, 2020

practicalswift force-pushed the unit-test-node-eviction-logic branch from a9cfb77 to 1c9b235 Compare December 15, 2020 15:32

jonatack reviewed Dec 16, 2020

View reviewed changes

src/net.cpp Show resolved Hide resolved

jonatack reviewed Dec 16, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

jonatack reviewed Dec 16, 2020

View reviewed changes

src/test/net_tests.cpp Outdated Show resolved Hide resolved

jonatack reviewed Dec 16, 2020

View reviewed changes

src/test/net_tests.cpp Outdated Show resolved Hide resolved

practicalswift added 3 commits December 16, 2020 12:00

net: Move eviction node selection logic to SelectNodeToEvict(...)

ed73f8c

test: Add unit testing of node eviction logic

685c428

Assert eviction at >= 29 candidates. Assert non-eviction at <= 20 can…

fee8823

…didates.

practicalswift force-pushed the unit-test-node-eviction-logic branch from 1c9b235 to fee8823 Compare December 16, 2020 12:00

maflcko merged commit b440c33 into bitcoin:master Dec 16, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 17, 2020

Merge bitcoin#20477: net: Add unit testing of node eviction logic

24939ef

practicalswift mentioned this pull request Dec 23, 2020

p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197

Merged

jnewbery mentioned this pull request Jan 12, 2021

Newsletters: add 131 (2020-01-13) bitcoinops/bitcoinops.github.io#503

Merged

6 tasks

practicalswift deleted the unit-test-node-eviction-logic branch April 10, 2021 19:43

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: Add unit testing of node eviction logic #20477

net: Add unit testing of node eviction logic #20477

practicalswift commented Nov 24, 2020 •

edited

Loading

DrahtBot commented Nov 24, 2020 •

edited

Loading

jonatack commented Nov 24, 2020

laanwj commented Dec 3, 2020 •

edited

Loading

dhruv left a comment •

edited

Loading

maflcko commented Dec 5, 2020

practicalswift commented Dec 5, 2020

jnewbery left a comment

maflcko commented Dec 7, 2020

practicalswift commented Dec 7, 2020 •

edited

Loading

jonatack commented Dec 11, 2020 •

edited

Loading

practicalswift commented Dec 15, 2020

dhruv commented Dec 15, 2020

maflcko commented Dec 16, 2020

maflcko commented Dec 16, 2020

maflcko commented Dec 16, 2020

jnewbery commented Dec 16, 2020

jonatack commented Dec 16, 2020

jonatack commented Dec 16, 2020

practicalswift commented Dec 16, 2020

jonatack commented Dec 16, 2020

maflcko commented Dec 16, 2020

net: Add unit testing of node eviction logic #20477

net: Add unit testing of node eviction logic #20477

Conversation

practicalswift commented Nov 24, 2020 • edited Loading

DrahtBot commented Nov 24, 2020 • edited Loading

Conflicts

jonatack commented Nov 24, 2020

laanwj commented Dec 3, 2020 • edited Loading

dhruv left a comment • edited Loading

Choose a reason for hiding this comment

maflcko commented Dec 5, 2020

practicalswift commented Dec 5, 2020

jnewbery left a comment

Choose a reason for hiding this comment

maflcko commented Dec 7, 2020

practicalswift commented Dec 7, 2020 • edited Loading

jonatack commented Dec 11, 2020 • edited Loading

practicalswift commented Dec 15, 2020

dhruv commented Dec 15, 2020

maflcko commented Dec 16, 2020

maflcko commented Dec 16, 2020

maflcko commented Dec 16, 2020

jnewbery commented Dec 16, 2020

jonatack commented Dec 16, 2020

jonatack commented Dec 16, 2020

practicalswift commented Dec 16, 2020

jonatack commented Dec 16, 2020

maflcko commented Dec 16, 2020

practicalswift commented Nov 24, 2020 •

edited

Loading

DrahtBot commented Nov 24, 2020 •

edited

Loading

laanwj commented Dec 3, 2020 •

edited

Loading

dhruv left a comment •

edited

Loading

practicalswift commented Dec 7, 2020 •

edited

Loading

jonatack commented Dec 11, 2020 •

edited

Loading