refactor: Move inbound eviction logic to its own translation unit #25500

pull dergoegge wants to merge 4 commits into bitcoin:master from dergoegge:2022-06-evictionman-1ofN changing 10 files +431 −350

dergoegge commented at 12:04 PM on June 29, 2022: member

This PR splits of the first couple commits from #25268 that move the inbound eviction logic from net.{h,cpp} to eviction.{h,cpp}.

Please look at #25268 for motivation and conceptual review.
fanquake added the label P2P on Jun 29, 2022
dergoegge force-pushed on Jun 29, 2022
DrahtBot commented at 3:10 PM on June 29, 2022: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #25493 (compat: document code in compat.h by fanquake)
- #25472 (build: Increase MS Visual Studio minimum version by hebasto)
- #25268 (refactor: Introduce EvictionManager by dergoegge)
- #24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)
- #17167 (Allow whitelisting outgoing connections by luke-jr)
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.
dergoegge force-pushed on Jun 30, 2022

in src/net_permissions.h:51 in bd02dcc0be outdated

  44 | @@ -45,6 +45,12 @@ static inline constexpr NetPermissionFlags operator|(NetPermissionFlags a, NetPe
  45 |      return static_cast<NetPermissionFlags>(static_cast<t>(a) | static_cast<t>(b));
  46 |  }
  47 |  
  48 | +static inline constexpr NetPermissionFlags operator&(NetPermissionFlags a, NetPermissionFlags b)
  49 | +{
  50 | +    using t = typename std::underlying_type<NetPermissionFlags>::type;
  51 | +    return static_cast<NetPermissionFlags>(static_cast<t>(a) & static_cast<t>(b));

MarcoFalke commented at 2:28 PM on July 1, 2022:

nit: Could write NetPermissionFlags{...} instead of static_cast?

dergoegge commented at 1:09 PM on July 4, 2022:

Removed this commit entirely and took john's suggestion: #25500 (review)

in src/Makefile.am:354 in 06d45ac8ac outdated
```
 347 | @@ -346,8 +348,10 @@ libbitcoin_node_a_SOURCES = \
 348 |    blockfilter.cpp \
 349 |    chain.cpp \
 350 |    consensus/tx_verify.cpp \
 351 | +  connection_types.cpp \
 352 |    dbwrapper.cpp \
 353 |    deploymentstatus.cpp \
 354 | +  eviction.cpp \
```
MarcoFalke commented at 2:29 PM on July 1, 2022:

nit: I wonder if the new files should be placed in node, as the lib is called libbitcoin_node_a_SOURCES

jnewbery commented at 7:47 AM on July 4, 2022:

Seems reasonable. Same for connection_types.cpp|h.

dergoegge commented at 1:09 PM on July 4, 2022:

Done.
in src/eviction.h:31 in 06d45ac8ac outdated
```
  26 | +    bool fBloomFilter;
  27 | +    uint64_t nKeyedNetGroup;
  28 | +    bool prefer_evict;
  29 | +    bool m_is_local;
  30 | +    Network m_network;
  31 | +    NetPermissionFlags m_perm_flags;
```
jnewbery commented at 7:42 AM on July 4, 2022:

Does the eviction logic need to know about all the permission flags? The logic only uses whether the NetPermissionFlags::NoBan bit is set. Perhaps this could be replaced with a m_noban bool?

dergoegge commented at 1:09 PM on July 4, 2022:

Done.
in src/eviction.h:32 in 06d45ac8ac outdated
```
  27 | +    uint64_t nKeyedNetGroup;
  28 | +    bool prefer_evict;
  29 | +    bool m_is_local;
  30 | +    Network m_network;
  31 | +    NetPermissionFlags m_perm_flags;
  32 | +    ConnectionType m_conn_type;
```
jnewbery commented at 7:44 AM on July 4, 2022:

Likewise, only one bit of information is actually needed by the eviction logic. Would it be better to replace this with an m_inbound boolean?

Perhaps the eviction logic will be changed in future to use more permissions and connection types, but it'll be easy enough to change these internal interfaces when that happens.

dergoegge commented at 1:12 PM on July 4, 2022:

Left this as is because the outbound eviction logic that gets moved to the eviction manager in #25500 does need the connection type.

Of course i could go with your suggestion here and add back the connection type in a later commit for the outbound logic. Let me know if you prefer that.

jnewbery commented at 9:25 AM on July 6, 2022:

That sounds fine to me.
jnewbery commented at 7:48 AM on July 4, 2022: member

Concept ACK
[net] Add NoBan status to NodeEvictionCandidate 42aa5d5b62
[net] Add connection type to NodeEvictionCandidate a3c2707039
dergoegge force-pushed on Jul 4, 2022
dergoegge force-pushed on Jul 4, 2022
in src/node/connection_types.cpp:1 in 2eb7256b64 outdated
```
   0 | @@ -0,0 +1,27 @@
   1 | +// Copyright (c) 2009-2010 Satoshi Nakamoto
```
jnewbery commented at 9:31 AM on July 6, 2022:

I'm pretty sure Satoshi didn't write any of this code (same for other files)

dergoegge commented at 4:19 PM on July 6, 2022:

Done.
jnewbery commented at 9:33 AM on July 6, 2022: member

utACK 2eb7256b643012b00f98997150ad753aa6d0a22b
theuni commented at 2:54 PM on July 6, 2022: member

utACK 2eb7256b643012b00f98997150ad753aa6d0a22b modulo @jnewbery's nits. Regardless of the larger effort, I think this stands on its own as a nice cleanup and small fuzzing improvement.
[net] Move ConnectionType to its own file c741d748d4
[net] Move eviction logic to its own file 0101d2bc3c
in src/net.h:1264 in 42aa5d5b62 outdated
```
1260 | @@ -1261,6 +1261,7 @@ struct NodeEvictionCandidate
1261 |      bool prefer_evict;
1262 |      bool m_is_local;
1263 |      Network m_network;
1264 | +    bool m_noban;
```
theuni commented at 3:41 PM on July 6, 2022:

At a high level, it's a little weird to add a flag to the eviction criteria that basically means "don't consider me for eviction". I assume this will prompt in future commits the reasonable question: should we bother adding nodes that can't be evicted to the eviction manager? (I don't know the answer to that question at this point, just pointing out that the discussion will come up later).

dergoegge commented at 4:19 PM on July 6, 2022:

Hm, good point. I will leave this unresolved for now so other reviewers can chime in.

theuni commented at 4:25 PM on July 6, 2022:

It's obviously correct here btw, I didn't mean to imply otherwise. Just a little weird :)
dergoegge force-pushed on Jul 6, 2022
jnewbery commented at 4:23 PM on July 6, 2022: member

utACK 0101d2bc3c3bcf698d6cc2a237a680fc52395987
theuni commented at 2:49 PM on July 7, 2022: member

utACK 0101d2bc3c3bcf698d6cc2a237a680fc52395987. I quickly verified with git --color-moved that the move-only changes are indeed move-only.
fanquake merged this on Jul 7, 2022
fanquake closed this on Jul 7, 2022

in src/net.h:12 in c741d748d4 outdated

   8 | @@ -9,6 +9,7 @@
   9 |  #include <chainparams.h>
  10 |  #include <common/bloom.h>
  11 |  #include <compat.h>
  12 | +#include <node/connection_types.h>

dhruv commented at 4:03 AM on July 12, 2022:

nit: Just noticed these are now out of the recommended include order

sidhujag referenced this in commit 20a7279e78 on Jul 13, 2022
bitcoin locked this on Jul 12, 2023

Contributors

Labels

Linked (view graph)

#25268 refactor: Introduce EvictionManager #27502 meta: Isolated fuzzing of net processing

refactor: Move inbound eviction logic to its own translation unit #25500

Conflicts