Add unit testing of node eviction logic (
p2p, tests) Dec 9, 2020
The PR branch HEAD was cadd93f at the time of this review club meeting.
When all inbound connection slots are occupied and a new inbound connection
is received, the node attempts to evict an existing inbound connection in
order to open up a slot.
However, any eviction logic is susceptible to
previous review club meetings on PR 16702 and PR
17428 for more background on eclipse attacks). An attacker may try to
make lots of new inbound connections to us, and if we always evict another
honest node, we may eventually be eclipsed.
src/net.cpp contains logic to increase the
cost of such attacks. Among other things, in order to take over all of the
inbound slots for a node, the attacker would need to:
Obtain IP addresses in various net groups
Be amongst the lowest latency peers (geographic cost)
Provide us with transactions before other peers
Provide us with blocks before other peers
The scarcity of tests for the eviction logic was first noticed in
#16660 and addressed with
functional tests in #16756
( review club).
SelectNodeToEvict() to increase
testability and adds unit tests for
Why is it important to allow for evictions? What are the risks in rejecting
new inbounds if slots are full?
What are the various ways
AttemptToEvictConnection() tries to increase the
cost of an eclipse attack?
remove peers being disconnected from
instead of waiting for them to disconnect (as that would open up a slot)?
What is a net group? Why can Bitcoin Core group peers by
ASNs instead of /16 subnets since
V0.20 ( issue,
Can an attacker predict which net groups are most likely to offer them
protection from eviction?
Why is eviction guaranteed if we have at least 29 eviction candidates?
Why is non-eviction guaranteed if we have no more than 20 eviction
candidates? Is 20 the highest number of nodes that guarantees non-eviction?
How does the new unit test leverage this information about guaranteed
eviction and non-eviction?
What is the purpose of the
here? Meeting Log
1 18:00 <dhruvm> #startmeeting
5 18:00 <michaelfolkson> (Joke)
8 18:00 <dhruvm> Hello everyone! Welcome to the PR Review Club for #20477: test/net: Add unit testing of node eviction logic.
9 18:00 <samuel-pedraza> hi
12 18:00 <dhruvm> Please say hi so everyone knows who is in attendance.
14 18:00 <joelklabo> haha, michaelfolkson
17 18:00 <troygiorshev> hi!
18 18:00 <joelklabo> jumped the gun
21 18:00 <prayank> hello world
24 18:00 <dhruvm> Anyone here for the first time?
26 18:00 <samuel-pedraza> yes, hi!
27 18:00 <murtyjones> I am
30 18:00 <dhruvm> welcome, samuel-pedraza!
32 18:01 <dhruvm> nice to meet you, murtyjones, kouloumos
33 18:01 <robot-dreams> hi
36 18:01 <dhruvm> A reminder of meeting convention: When you have a question, don't ask to ask, just ask.
37 18:01 <dhruvm> Did everyone get a chance to review the PR? How about a quick y/n from everyone
42 18:02 <samuel-pedraza> y
44 18:02 <pinheadmz> n :-( lkurking
51 18:02 <glozow> n but somewhat familiar
53 18:02 <emzy> y (only tested it)
55 18:02 <dhruvm> Awesome, let's get started
56 18:02 <dhruvm> Q: Why is it important to allow for evictions? What are the risks in rejecting new inbounds if slots are full?
59 18:03 <joelklabo> could allow an eclipse attacker to replace all connections with theirs?
60 18:03 <thomasb06> we risk to be connected to only malcious nodes if we keep only old ones
61 18:03 <stacie> Allowing evictions opens up the opportunity to get better peers (i.e. ones w/ less latency, IP diversity for net groups, ones that provide transactions and blocks faster, etc.)
62 18:03 <mango> Nodes with certain properties are prioritized (lower latency, better relay services)
63 18:03 <amiti> an attacker can execute a connection starvation attack - just take up all the slots on the network, and then there's no way for a new nodes to connect to the honest network
64 18:04 <Murch> dhruvm: you could get stuck in a subcluster that loses connection to the main network
65 18:04 <dhruvm> That's right, joelklabo, amiti!
66 18:04 <dhruvm> If honest peers in the network don't have available inbound slots, a new node may not be able to create any outbound connections into the honest network. This might make it easy for a dishonest node to attack the new node - all they have to do is be available.
67 18:04 <dhruvm> Good point on diversity stacie.
68 18:05 <Murch> (if nobody ever replaces their connections)
69 18:05 <dhruvm> Murch: network partitions are more probably if we don't evict for new inbounds, yes.
70 18:05 <dhruvm> s/probably/probable
71 18:05 <troygiorshev> Murch: +1, a static topology would be much easier to attack imo
72 18:05 <dhruvm> Q: What are the various ways AttemptToEvictConnection() tries to increase the cost of an eclipse attack?
73 18:05 <glozow> what a nice combo of individual node security + overall network health
74 18:05 <michaelfolkson> For the network (and new peers) it is obviously a problem. Less of a problem for you as a node I think (unless you are literally not finding out blocks, transactions anymore)
75 18:06 <Murch> glozow: +1
76 18:06 <thomasb06> There are ten: nTimeConnected, nMinPingUsecTime, ..., m_is_local
77 18:06 <sipa> the primary defense against eclipse attacks is our outbound peer selection, as those are far less under attacker control than inbound
78 18:07 <Murch> dhruvm: we keep nodes that gave us blocks recently, protect nodes from different internet regions, keep the ones that we have been connected to the longest, keep peers with low latency, there was more I'm forgetting
79 18:07 ⚡ album sneaks in late into the back
80 18:08 <dhruvm> sipa: that makes sense, but aren't inbounds the primary way to poison addrman and wait for a restart (of course anchors help)?
81 18:08 <jonatack> recently gave us new txns and blocks
82 18:08 <dhruvm> thomasb06: Murch: jonatack: correct
83 18:08 <dhruvm> AttemptToEvictConnection protects some connections from eviction if they are in certain net groups, have had low latency ping times, or have provided us with valid transactions or blocks we did not know about. So to protect their own prior connection from eviction (when they make a new inbound), an attacker has to change real world parameters, and do useful work.
84 18:08 <thomasb06> (oof)
85 18:09 <sipa> dhruvm: right, it's a second order protection
86 18:09 <dhruvm> Q: Why does AttemptToEvictConnection remove peers being disconnected from eviction candidates instead of waiting for them to disconnect (as that would open up a slot)?
87 18:09 <murtyjones> does that mean that blocksonly nodes are more likely to be evicted?
88 18:10 <sipa> dhruvm: inbound variety helps, but given that not even all nodes permit incoming connections, it's hard to say variety in it is essential for eclipse protection
89 18:10 <thomasb06> An attack would be easier
91 18:11 <jonatack> some trivia, the original motivation behind the -netinfo dashboard was specifically to observe some of these inbound eviction criteria
92 18:11 <jonatack> murtyjones: block-relay-only connections are a type of outbound (not inbound) connection
93 18:11 <troygiorshev> jonatack: cool, thanks!
95 18:11 <stacie> At the end of SelectNodeToEvict(), it picks the net group with the most connections. Could the peer waiting to disconnect alter this metric?
96 18:12 <murtyjones> dhruvm jonatack thank you
97 18:13 <dhruvm> stacie: that's a really good point, i hadn't thought about that. regardless, if a slot if being opened up anyway, why not wait?
98 18:13 <dhruvm> I actually don't know the know the answer to this one. I suspect it is because there would be no way to know how many new inbounds were waiting on a disconnection in progress (given current code). But it can probably be changed?
99 18:13 <Murch> stacie: An attacker could add more connections of a specific net group with a vpn in that net group, but then that would also increase the chance of the attacker's nodes to get removed?
100 18:14 <joelklabo> can you stay in the waiting to disconnect state for a long time?
101 18:14 <Murch> Or do you mean, a node would change their netgroup to remain connected?
102 18:14 <mango> There is guaranteed eviction when the set of candidates is high enough (29?), so disconnect keeps the set reasonable and avoids unncessary evictions later.
103 18:14 <amiti> yeah, I don't know either, but it seems more reasonable to have too many available slots vs having too many connections competing for the limited slots
104 18:14 <Murch> stacie: I don't think the node would know it'll get disconnected until we disconnect, would it?
105 18:15 <thomasb06> stacie: when the node reconnects, it becomes a good candidate to many criteria maybe
106 18:15 <dhruvm> amiti: yeah that makes sense. SInce ThreadSocketHandler and the logic in AttemptToEvictConnection are not synchronized, better to have more available slots than too few
107 18:15 <sipa> stacie, Murch: an overall assumption is that access to multiple netgroups is more expensive than multiple connections from one netgroup
108 18:17 <dhruvm> Speaking of net groups
109 18:17 <dhruvm> Q: What is a net group? Why can Bitcoin Core group peers by ASNs instead of /16 subnets since V0.20?
110 18:17 <Murch> Right, and since we never tell about all of our connections, the attacker wouldn't know which of our peers' netgroups has the highest representation
111 18:17 <michaelfolkson> As an attacker you don't know much about the node's peers (other than what you can deduce from addr gossiping) unless you hack their node obviously
112 18:17 <Murch> joelklabo: Good question, can someone enlighten us?
113 18:17 <sipa> joelklabo: milliseconds, i think
114 18:17 <sipa> just until the net handling loop passes that node
115 18:17 <dhruvm> Murch: joelklabo: ThreadSocketHandler takes care of the disconnections. It should be pretty fast.
116 18:17 <joelklabo> Ah, not that long
117 18:18 <sipa> mango: we only evict when we're running out of incoming connection slots, not before
118 18:18 <Murch> stacie: perhaps the attacker could learn what netgroup we have a lot of nodes from when they get disconnected and focus on connecting from other netgroups thereafter
119 18:19 <michaelfolkson> Murch: But you don't know why you've been disconnected
120 18:19 <michaelfolkson> You're flailing in the dark
121 18:19 <thomasb06> (lost)
122 18:19 <michaelfolkson> I guess you could try things and see what causes disconnection
123 18:20 <schmidty> dhruvm: -asmap config option from 0.20.0
124 18:20 <Murch> michaelfolkson: You do know that you weren't in one of the protected categories, though
125 18:20 <michaelfolkson> Murch: Right
126 18:20 <dhruvm> schmidty: that's right
127 18:21 <dhruvm> Can anyone tell us why ASNs are better net groups copared to /16 subnets?
128 18:21 <Murch> dhruvm: The idea is that the ASNs split the network up better than the subnets
129 18:21 <dhruvm> Murch: How so?
130 18:21 <pinheadmz> I think IP distirbution has gotten mixed up between service providers.
131 18:22 <pinheadmz> so you cant assume that 1.x.x.x and 2.x.x.x are in different parts of the world anymore
132 18:22 <dhruvm> pinheadmz: exactly!
134 18:22 <dhruvm> ASNs are dynamic groups of IP address range assignments representing real world entities like ISPs, companies, etc. Grouping by ASNs is superior to /16 subnets because large entities would span multiple /16 subnets, lowering the cost for an attacker to obtain addresses in different groups.
135 18:22 <pinheadmz> now its possible both of those netgroups are owned by amazon,
136 18:22 <michaelfolkson> Murch: In this way a honest node is going to behave the same way as a malicious node. Both are trying to stop being disconnected. I guess a malicious node will make more effort and expend more resources to maintain a connection
137 18:23 <sipa> michaelfolkson: or just create more connections and see which live
138 18:23 <sipa> michaelfolkson: or even better, decide that because of these protections, it"s not worth trying to attack
139 18:23 <stacie> Murch I was thinking something along those lines, trying different net groups to see which ones are successful. But I was more trying to find out how including a disconnecting node in the node eviction candidates could possibly alter any of the logic in SelectNodeToEvict(). Wasn't really going anywhere more specific than that :) Thanks for digging into it deeper! A lot of interesting questions that I don't quite know
140 18:23 <stacie> the answer to :)
141 18:23 <dhruvm> Ok, next question is related to an ongoing thread.
142 18:23 <dhruvm> Q: Can an attacker predict which net groups are most likely to offer them protection from eviction?
143 18:24 <emzy> Apple for example has 220.127.116.11/8
144 18:27 <Murch> stacie: Only stumbling around in the dark here myself! Networking stuff is not really my forte. ;)
145 18:27 <troygiorshev> dhruvm: well the code comments say no :D I'm still thinking of a good justification though...
146 18:27 <dhruvm> troygiorshev: :)
147 18:27 <Murch> dhruvm: Not really, because we never broadcast more than a subset of our peer connections
148 18:27 <dhruvm> Murch: That's right
149 18:27 <dhruvm> An attacker cannot predict which net groups would shelter them from eviction because they'd need to know the net groups of all our peers. We never reveal more than 23% of our addrman to any single peer, and we definitely don't reveal a list of our active peers to anyone (that would open up other attack vectors).
150 18:27 <michaelfolkson> dhruvm: If the attacker knows which net groups are less populated on the network due to gossiping. Or are they evenly distributed somehow?
151 18:27 <troygiorshev> Does someone have a quick explanation of KeyedNetGroup? Is the ordering exploitable?
152 18:27 <dhruvm> michaelfolkson: I think CompareNetGroupKeyed sorts the peers by net group, not by count in the net group
153 18:28 <Murch> michaelfolkson: That would sort of require the attacker again to do useful work, by populating underrepresented groups. :D
154 18:28 <jonatack> murtyjones: thinking about your question, ISTM it would be the opposite: CompareNodeBlockRelayOnlyTime() is used for protecting potential block-relay-ony peers from eviction
155 18:28 <dhruvm> michaelfolkson: so in terms of the protection from eviction due to net groups, the cardinality of the net group doesn't matter
156 18:29 <prayank> I have a noob question. Everything that we are discussing related to this PR is same for nodes with and without tor or anything different in the eviction process? Or something that may work differently for Tor nodes?
157 18:29 <michaelfolkson> Murch: If the underrepresented group is entirely populated by malicious parties that is a problem :)
158 18:29 <Murch> michaelfolkson: huh, why? :)
159 18:30 <jonatack> prayank: that's a relevant question
160 18:30 <troygiorshev> (I've answered my own question. The third last line of net.cpp assigns netgroups, and it does so randomly. An attacker can not know which netgroups are prioritized, at least not easily)
162 18:30 <Murch> they may get a first connection more easily, but what do they then do? They presumably could have gotten an inbound connection either way
163 18:30 <sipa> prayank: yes and no; tor connections don't have a meaningful "from IP" or equivalent to clasify them by, so it's much easier to e.g. exhaust incoming tor slots
165 18:31 <michaelfolkson> Murch: The whole point of this is to make attacks harder. You make it easier for the attacker if they know to join an underrepresented group
166 18:31 <prayank> Interesting. Thanks
167 18:31 <jonatack> recently the code was changed to protect tor peers, which by their longer ping times were being disadvantaged by our criteria
168 18:31 <Murch> michaelfolkson: That only helps them to get one additional node not evicted though. Unless I'm missing something
169 18:32 <sipa> jonatack: right, but anything netgroup related doesn't apply to tor connections
170 18:32 <dhruvm> Ok, let's move on to the heart of the PR.
171 18:32 <dhruvm> Q: Why is eviction guaranteed if we have at least 29 eviction candidates?
172 18:33 <sipa> dhruvm: i'm confused by that, we should only be evicting when we're out of connection slots
173 18:33 <Murch> Whoever answers this one should respond to nehan's comment. :)
174 18:33 <sipa> (but haven't looked at that code in a while, or your PR)
175 18:33 <nehan> yeah, i'm confused. this seems empirical more than guaranteed? i'm very curious where those numbers came from!
176 18:34 <dhruvm> sipa: you're right. but this unit test is for a lower level function. the test for slots being full is executed by the caller.
177 18:34 <nehan> and how someone updating the eviction logic can figure out when/how to update the test...
178 18:34 <schmidty> troygiorshev: so the DeterministicRandomizer prevents an attacker from knowing which net group will be preserved it looks like
179 18:34 <sipa> dhruvm: ah, of course
180 18:34 <dhruvm> sipa: SelectNodeToEvict assumes you want to evict
181 18:34 <jnewbery> perhaps the questions should be rephrased: "if our maximum number of inbound slots is more than 29, and they're all full, why is it guaranteed that we'll evict an inbound connection if AttemptToEvictConnection() is called?"
182 18:34 <sipa> schmidty: yup
183 18:35 <jnewbery> schmidty: right. The deterministic randomizer mixes in a (private) salt, so an attacker shouldn't be able to know how the netgroups are ordered
184 18:36 <Murch> So, across the categories for protecting nodes from eviction, the total sum of protected nodes sums up to 28
186 18:36 <troygiorshev> schmidty: yep thanks! It does seem like this could leak though. It's good that we only protect 4 peers on this metric.
187 18:37 <nehan> Murch: ok thanks! I thought it might be something like that. I think it would still be good to point to where the number came from in the test
188 18:37 <Murch> so, when we're full and have 29 or more nodes, there will at least be one unprotected and thus eligible for eviction. We do prefer the new connection, so an eviction is guaranteed
189 18:37 <dhruvm> nehan: it was derived empirically but we can also see why this is the case by reviewing eviction logic in SelectNodeToEvict
190 18:37 <stacie> 29 lines up with the protections, but I agree on the rephrasing of the question/referencing where it came from in the test - 4 by net group, 8 by lowest ping time, 4 by most recently sent novel tx, 8 for non-relay-tx peers that most recently sent novel blocks, 4 for nodes that sent novel blocks
191 18:38 <nehan> ok now explain the 20 :) why isn't it 8?
192 18:38 <MarcoFalke> 4+8+4+0+4=20
193 18:38 <dhruvm> Ok, so for 29:
194 18:38 <dhruvm> The code in net.cpp ( https://github.com/bitcoin/bitcoin/blob/fabecce/src/net.cpp#L954-L968) protects at most 28 peers from eviction (4 by net group, 8 by lowest ping time, 4 by last time of novel tx, up to 8 non-tx-relay peers by last novel block time, and 4 more peers by last novel block time). So any additional peers are guaranteed to be candidates for eviction.
195 18:38 <schmidty> 4+8+4+8+4 = 28?
196 18:39 <dhruvm> schmidty: correct!
197 18:39 <Murch> Four nodes from highest keyed netgroup values, eight nodes with minimum bping, four nodes from novel transactions, eight non-tx-relay peers that sent us blocks last, up to 4 from localhost
198 18:39 <dhruvm> nehan has already moved on to the more interesting question
199 18:39 <dhruvm> Q: Why is non-eviction guaranteed if we have no more than 20 eviction candidates? Is 20 the highest number of nodes that guarantees non-eviction?
200 18:39 <nehan> MarcoFalke: couldn't some of those nodes overlap?
201 18:39 <jonatack> see test/functional/p2p_eviction.py for the 20
202 18:39 <stacie> nehan subtract the 8 in case there aren't any non-relay-tx peers? err.. that's my best guess
203 18:39 <Murch> Okay, I may be misreading that on localhost
204 18:39 <jonatack> lines 40-41
205 18:40 <Murch> dhruvm probably did more research than me ^^
206 18:40 <nehan> jonatack: aha! thank you!
207 18:40 <jonatack> nehan: :)
208 18:40 <dhruvm> stacie: you're close. why would the randomly generated peers not qualify for the non-tx-relay protection?
209 18:40 <MarcoFalke> nehan: nodes are removed from the list, so they can't overlap
210 18:41 <nehan> MarcoFalke: ah right, EraseLastKElements, thanks
211 18:42 <Murch> nehan: I think it's 8 from non-tx-relay peers and 4 _more_ by last novel block.
212 18:42 <Murch> Each category removes eviction candidates, any new category will pick from the remaining
213 18:43 <jonatack> Murch: seems so
214 18:43 <jonatack> since #19670
215 18:43 <dhruvm> Right. So it is indeed about the 8 non-tx-relay peers
216 18:43 <dhruvm> The protection at [net.cpp::961]( https://github.com/bitcoin/bitcoin/blob/fabecce/src/net.cpp#L961) for up to 8 non-tx-relay peers may, or may not apply to the randomly generated eviction candidates since `!n.fRelayTxes && n.fRelevantServices` will evaluate to `randbool`. So we cannot count them in the _guaranteed_ non-eviction?
217 18:43 <Murch> so, it's only non-tx-relay peers, because we are not guaranteed to have any
218 18:43 <dhruvm> guaranteed non-eviction is only on 4(CompareNetGroupKeyed) + 8(ReverseCompareNodeMinPingTime) + 4(CompareNodeTXTime) + 4(CompareNodeBlockTime) = 20.
219 18:43 <Murch> oh, stacie is way ahead of me
220 18:44 <dhruvm> If we have 21-28 peers, we may or may not get an eviciton based on what `!n.fRelayTxes && n.fRelevantServices` evaluates to
221 18:45 <dhruvm> Q: How does the new unit test leverage this information about guaranteed eviction and non-eviction?
222 18:47 <murtyjones> looks to me like depending on the number of nodes in the test, it verifies that the correct number are evicted based on the guarantees
223 18:47 <stacie> dhruvm that makes sense. Thanks!
225 18:48 <dhruvm> murtyjones: correct!
226 18:48 <dhruvm> although only at most one is ever evicted
227 18:48 <schmidty> Test case makes sure there is always an eviction at (29) GUARANTEED_EVICTION_AT_N_CANDIDATES_OR_ABOVE or above. Likewise no evictions at (20) GUARANTEED_NON_EVICTION_AT_N_CANDIDATES_OR_BELOW or below.
228 18:48 <dhruvm> The tests generate randomized eviction candidates and since non-eviction is guaranteed at <= 20 peers and eviction at >= 29 peers, the characteristics of the randomized peers are irrelevant, except the ones specifically overriden by each test case.
229 18:48 <dhruvm> schmidty: you got it
231 18:49 <Murch> The test counts up 10 times from zero to 199 nodes and checks that for each number of nodes under 21 there is no eviction and above 28 there is.
232 18:49 <dhruvm> Murch: exactly
233 18:50 <MarcoFalke> I don't get the 'continue' in the test either
234 18:50 <dhruvm> MarcoFalke: that bit is definitely awkward. I failed to offer a simpler formulation though.
235 18:51 <dhruvm> MarcoFalke: the continue is trying to cover for the 21-28 case where we don't have eviction guarantees
236 18:51 <jonatack> I wonder if this test wouldn't deserve its own file, e.g. eviction_tests.cpp. It's bound to grow over time as well.
237 18:52 <Murch> dhruvm: no it happens for everything below 29
238 18:52 <MarcoFalke> dhruvm: The checks that follow are for non-evicition, not for eviction
239 18:52 <nehan> dhruvm: why wouldn't you want to do some checks to make sure the right things are protected when there are between 21 and 28 nodes?
240 18:52 <dhruvm> Murch: MarcoFalke: Yes, you are correct. Sorry.
241 18:53 <dhruvm> nehan: that's a good question. we should probably be checking for that.
242 18:53 <Murch> I guess it only checks whether the protected groups are full, because the count in the categories is not stable below 29
243 18:54 <dhruvm> nehan: instead of checking for does eviction happen, the test should be "if eviction happens, the correct one does"
244 18:54 <dhruvm> This PR was a good introduction to some modern idiomatic C++ concepts for me. Let's talk about move semantics and lambdas.
246 18:54 <nehan> Murch: I don't understand what you just said. it definitely exits out via the continue if 20 < number_of_nodes < 29?
247 18:55 <jnewbery> nehan: I think you're right that you could remove the else continue and the test would run fine
248 18:55 <murtyjones> i interpret std::move as moving the node to evict from vEvictionCandidates to node_id_to_evict
249 18:55 <MarcoFalke> dhruvm: std::move makes a static_cast to the "double-ampersand type"
250 18:56 <murtyjones> or rather, maybe moving depending on the result of SelectNodeToEvict
251 18:56 <jonatack> and avoids a copy
252 18:56 <Murch> nehan: Sorry, I meant that it only processes the BOOST_CHECKs above 28 because before then the categories wouldn't necessarily be full
253 18:56 <dhruvm> MarcoFalke: jonatack: correct!
254 18:56 <dhruvm> std::move casts the parameters into an rvalue reference and invokes a move constructor. This (1) transfers the ownership of the vector to `SelectNodeToEvict`, (2) avoids a copy of the vector and (3) makes it clear to future contributors that they should not re-use the vector as the behavior will be unspecified.
256 18:56 <michaelfolkson> Formally should this be considered a functional test rather than a unit test? I know it doesn't matter but unsure on where the line is between unit and functional other than using Boost/functional test framework
258 18:56 <jnewbery> because we're testing what's protected, not what's evicted
259 18:56 <jonatack> "Write std::move() only when you need to explicitly move an object to another scope"
260 18:56 <Murch> It may be more complicated to compare the total counts if e.g. sometimes there are non-block-relay nodes and sometimes there aren't
261 18:57 <dhruvm> Last question.
262 18:57 <MarcoFalke> michaelfolkson: I'd imagine this is hard to setup as functional test
264 18:57 <murtyjones> gotta run but thank you dhruvm this was very informative!
265 18:57 <troygiorshev> michaelfolkson: tbh here it's "if python then functional, if c++ then unit"
266 18:58 <michaelfolkson> troygiorshev: Is that the Core philosophy? :) I wasn't aware. Whether the author wants to write C++ or Python?
267 18:58 <troygiorshev> michaelfolkson: that said, because we're calling IsEvicted directly, it's a unit test
268 18:58 <sipa> michaelfolkson: it's testing a specific function that's not not exposed, so i'd say it's a unit test regardless of your criteria
269 18:58 <jnewbery> std::move doesn't move anything, it simply casts to rvalue. I'd recommend Effective Modern C++ by Scott Meyers for a really good explanation of move sematics
270 18:58 <Murch> jnewbery: I don't understand why the test would work as written when the protected categories wouldn't be full
271 18:59 <michaelfolkson> Interesting, thanks troygiorshev sipa
272 18:59 <dhruvm> On the lambdas question: After setting up random eviction candidates, `candidate_setup_fn` is used to give some candidates specific attributes, like net groups, etc.
273 19:00 <troygiorshev> michaelfolkson: if you write your test in python, then you'll have to spin up an entire bitcoin node to interact with through the public api. On the other hand, in C++ here we can call non-exposed methods directly
274 19:00 <jnewbery> I really like the way the unit test is constructed. Passing a lambda to manipulate the eviction candidates is neat
275 19:00 <dhruvm> Thank you all for coming. I had a ton of fun reviewing the PR and hosting today. Will definitely be doing it again. I know jnewbery is looking for more hosts. If you have the time and inclination, I highly recommend hosting, it's a great way to learn!
276 19:00 <sipa> thanks, dhruvm!
277 19:00 <glozow> thank you dhruvm!!!
278 19:00 <dhruvm> #endmeeting