[net] De-duplicate connection eviction logic #11524

tjps · 2017-10-19T08:03:34Z

While reviewing the safeguards against deliberate node isolation on the network by malicious actors, I found a good de-duplication candidate.

I think this form is much more legible (the type of cutoffs notwithstanding). ReverseCompareNodeTimeConnected is not included in the list since the cutoff size is a function of the remaining number of nodes in the candidate eviction set.

practicalswift · 2017-10-19T10:08:33Z

Concept ACK. Nice cleanup!

promag

Concept ACK.

I would prefer to abstract the loop body into a function, and call it 4 times with the given arguments.

It should be easier to test and IMO understand.

ryanofsky

utACK b52635e0b9db58095c21de1af5f5a0c74a85b63c. Change is an an improvement over existing code, but this can be simplified more. There is no need for std::function indirection, and no need for looping. All you need here is a plain generic function:

//! Remove last n elements from container after sorting, and return true if it is non-empty.
template<typename Container, typename Compare>
bool EraseLastN(Container& container, int n, Compare compare);

and

if (!(EraseLastN(vEvictionCandidates, 4, CompareNetGroupKeyed) ||
    !(EraseLastN(vEvictionCandidates, 8, ReverseCompareNodeMinPingTime) ||
    !(EraseLastN(vEvictionCandidates, 4, CompareNodeTXTime) ||
    !(EraseLastN(vEvictionCandidates, 4, CompareNodeBlockTime)) {
    return false
};

You might also want to use std::partial_sort instead of std::sort in the new code since a full sort isn't necessary.

sipa · 2017-10-21T14:16:32Z

Concept ACK; I prefer @ryanofsky's suggestion as well.

meshcollider · 2017-10-22T10:07:50Z

Concept ACK, nice cleanup.
I too prefer @ryanofsky's version plus your comments :)

tjps · 2017-10-25T00:56:04Z

@ryanofsky - I was unaware of std::partial_sort, and while it seems perfectly suited to this use case, all the benchmarks I found comparing it to std::sort seemed to indicate it was slower. I'll leave it using std::sort for now.

Also since removeLastKElements is a no-op on an empty vector, I left it as a void return type and skipped the chained conditional return checks. I think this makes the code more readable.

tjps · 2017-10-26T18:47:57Z

Updated diff to include a little DRY/readability improvement at the end of the same function.

ryanofsky

utACK 0bd38149bde46779b5d0823344c1187372062d13. Still looks good. Feel free to ignore new nitpicky comments.

ryanofsky · 2017-10-31T18:26:33Z

src/net.cpp

+
+//! Sort an array by the specified comparator, then remove the last K elements.
+template<typename T>
+static void removeLastKElements(std::vector<T> &elements, bool(*comparator)(const T&, const T&), size_t k)


Maybe don't require comparator to be function pointer. It'd be nice to allow comparator to be a lambda or function class:

template<typename T, typename Compare> static void removeLastKElements(std::vector<T> &elements, Compare comp, size_t k) { std::sort(elements.begin(), elements.end(), comp); ... }

That's a good point, should have pulled that from your example. Old habits die hard.

ryanofsky · 2017-10-31T18:30:16Z

src/net.cpp

+static void removeLastKElements(std::vector<T> &elements, bool(*comparator)(const T&, const T&), size_t k)
+{
+    std::sort(elements.begin(), elements.end(), comparator);
+    size_t cutSize = std::min(k, elements.size());


Naming convention in new code would call this cut_size. I would probably just inline this variable, though. Also I think it's a little weird how the code is using "remove" "cut" and "erase" interchangeably. I don't see why you wouldn't want to be consistent and just say "erase" everywhere.

I like k to refer to the K in the function name. I agree that the naming should be more consistent, switching to 'erase' across the board.

TheBlueMatt · 2017-11-06T16:43:03Z

src/net.cpp

+{
+    std::sort(elements.begin(), elements.end(), comparator);
+    size_t eraseSize = std::min(k, elements.size());
+    elements.erase(elements.end() - eraseSize, elements.end());


Technically this introduces undefined behavior - we no longer return early if we're gonna call erase(end()), which is undefined.

Technically this introduces undefined behavior - we no longer return early if we're gonna call erase(end()), which is undefined.

This seems to be fine according to http://en.cppreference.com/w/cpp/container/vector/erase: "The iterator first does not need to be dereferenceable if first==last: erasing an empty range is a no-op."

Oops, indeed, somehow I missed that line.

TheBlueMatt · 2017-11-06T17:29:24Z

utACK efa57568236cd7b3a80419e09e9babd546cbbda3

sipa · 2017-11-07T18:43:13Z

utACK efa57568236cd7b3a80419e09e9babd546cbbda3, but can you follow the function naming convention (EraseLastKElements)?

tjps · 2017-11-07T23:34:36Z

@sipa whoops, of course. Updated the function name.

sipa · 2017-11-07T23:36:39Z

re-utACK 5ce7cb9, only change is the function name.

laanwj · 2017-11-08T07:46:48Z

utACK 5ce7cb9

5ce7cb9 [net] De-duplicate connection eviction logic (Thomas Snider) Pull request description: While reviewing the safeguards against deliberate node isolation on the network by malicious actors, I found a good de-duplication candidate. I think this form is much more legible (the type of `cutoffs` notwithstanding). ReverseCompareNodeTimeConnected is not included in the list since the cutoff size is a function of the remaining number of nodes in the candidate eviction set. Tree-SHA512: ed17999fa9250dcf8448329219324477117e4ecd2d41dedd72ad253e44630eef50b3232c420f1862ebbfb9b8c94efbba1a235b519e39ff5946865c7d69a75280

maflcko · 2017-11-08T20:07:38Z

post merge utACK 5ce7cb9

5ce7cb9 [net] De-duplicate connection eviction logic (Thomas Snider) Pull request description: While reviewing the safeguards against deliberate node isolation on the network by malicious actors, I found a good de-duplication candidate. I think this form is much more legible (the type of `cutoffs` notwithstanding). ReverseCompareNodeTimeConnected is not included in the list since the cutoff size is a function of the remaining number of nodes in the candidate eviction set. Tree-SHA512: ed17999fa9250dcf8448329219324477117e4ecd2d41dedd72ad253e44630eef50b3232c420f1862ebbfb9b8c94efbba1a235b519e39ff5946865c7d69a75280

fanquake added P2P Refactoring labels Oct 19, 2017

promag reviewed Oct 19, 2017

View reviewed changes

ryanofsky reviewed Oct 20, 2017

View reviewed changes

ryanofsky reviewed Oct 31, 2017

View reviewed changes

TheBlueMatt reviewed Nov 6, 2017

View reviewed changes

[net] De-duplicate connection eviction logic

5ce7cb9

laanwj merged commit 5ce7cb9 into bitcoin:master Nov 8, 2017

tjps deleted the tjps_eviction branch November 17, 2017 00:55

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[net] De-duplicate connection eviction logic #11524

[net] De-duplicate connection eviction logic #11524

tjps commented Oct 19, 2017

practicalswift commented Oct 19, 2017

promag left a comment

ryanofsky left a comment

sipa commented Oct 21, 2017

meshcollider commented Oct 22, 2017

tjps commented Oct 25, 2017

tjps commented Oct 26, 2017

ryanofsky left a comment

ryanofsky Oct 31, 2017

tjps Nov 1, 2017

ryanofsky Oct 31, 2017

tjps Nov 1, 2017

TheBlueMatt Nov 6, 2017

ryanofsky Nov 6, 2017

TheBlueMatt Nov 6, 2017

TheBlueMatt commented Nov 6, 2017

sipa commented Nov 7, 2017

tjps commented Nov 7, 2017

sipa commented Nov 7, 2017

laanwj commented Nov 8, 2017

maflcko commented Nov 8, 2017

[net] De-duplicate connection eviction logic #11524

[net] De-duplicate connection eviction logic #11524

Conversation

tjps commented Oct 19, 2017

practicalswift commented Oct 19, 2017

promag left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

sipa commented Oct 21, 2017

meshcollider commented Oct 22, 2017

tjps commented Oct 25, 2017

tjps commented Oct 26, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Oct 31, 2017

Choose a reason for hiding this comment

tjps Nov 1, 2017

Choose a reason for hiding this comment

ryanofsky Oct 31, 2017

Choose a reason for hiding this comment

tjps Nov 1, 2017

Choose a reason for hiding this comment

TheBlueMatt Nov 6, 2017

Choose a reason for hiding this comment

ryanofsky Nov 6, 2017

Choose a reason for hiding this comment

TheBlueMatt Nov 6, 2017

Choose a reason for hiding this comment

TheBlueMatt commented Nov 6, 2017

sipa commented Nov 7, 2017

tjps commented Nov 7, 2017

sipa commented Nov 7, 2017

laanwj commented Nov 8, 2017

maflcko commented Nov 8, 2017