Add unit testing of node eviction logic (p2p, tests)

https://github.com/bitcoin/bitcoin/pulls/20477

Host: dhruv  -  PR author: practicalswift

The PR branch HEAD was cadd93f at the time of this review club meeting.

Notes

  • 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 eclipse attacks (see 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.

  • AttemptToEvictConnection in 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).

  • This PR, #20477 refactors some of AttemptToEvictConnection() into SelectNodeToEvict() to increase testability and adds unit tests for SelectNodeToEvict().

Questions

  1. Why is it important to allow for evictions? What are the risks in rejecting new inbounds if slots are full?

  2. What are the various ways AttemptToEvictConnection() tries to increase the cost of an eclipse attack?

  3. Why does AttemptToEvictConnection remove peers being disconnected from eviction candidates instead of waiting for them to disconnect (as that would open up a slot)?

  4. What is a net group? Why can Bitcoin Core group peers by ASNs instead of /16 subnets since V0.20 (issue, PR)?

  5. Can an attacker predict which net groups are most likely to offer them protection from eviction?

  6. Why is eviction guaranteed if we have at least 29 eviction candidates?

  7. 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?

  8. How does the new unit test leverage this information about guaranteed eviction and non-eviction?

  9. What does std::move do here?

  10. What is the purpose of the candidate_setup_fn lambda here?

Meeting Log

  118:00 <dhruvm> #startmeeting
  218:00 <emzy> hi
  318:00 <elle> hi :)
  418:00 <hieblmi> hi
  518:00 <michaelfolkson> (Joke)
  618:00 <pinheadmz> hi
  718:00 <willcl_ark> hi
  818:00 <dhruvm> Hello everyone! Welcome to the PR Review Club for #20477: test/net: Add unit testing of node eviction logic.
  918:00 <samuel-pedraza> hi
 1018:00 <nehan> hi
 1118:00 <mango> hi
 1218:00 <dhruvm> Please say hi so everyone knows who is in attendance.
 1318:00 <andozw> hiiii
 1418:00 <joelklabo> haha, michaelfolkson
 1518:00 <schmidty> hi
 1618:00 <murtyjones> hi
 1718:00 <troygiorshev> hi!
 1818:00 <joelklabo> jumped the gun
 1918:00 <jnewbery> hi!
 2018:00 <jonatack> hi
 2118:00 <prayank> hello world
 2218:00 <glozow> hi
 2318:00 <kouloumos> hi!
 2418:00 <dhruvm> Anyone here for the first time?
 2518:00 <stacie> hi
 2618:00 <samuel-pedraza> yes, hi!
 2718:00 <murtyjones> I am
 2818:00 <thomasb06> hi
 2918:00 <kouloumos> yes
 3018:00 <dhruvm> welcome, samuel-pedraza!
 3118:01 <amiti> hi !
 3218:01 <dhruvm> nice to meet you, murtyjones, kouloumos
 3318:01 <robot-dreams> hi
 3418:01 <Murch> ih
 3518:01 <Murch> hi
 3618:01 <dhruvm> A reminder of meeting convention: When you have a question, don't ask to ask, just ask.
 3718:01 <dhruvm> Did everyone get a chance to review the PR? How about a quick y/n from everyone
 3818:01 <jnewbery> y
 3918:01 <mango> y
 4018:01 <nehan> y
 4118:01 <stacie> y
 4218:02 <samuel-pedraza> y
 4318:02 <troygiorshev> y
 4418:02 <pinheadmz> n :-( lkurking
 4518:02 <thomasb06> y
 4618:02 <murtyjones> n
 4718:02 <kouloumos> n
 4818:02 <jonatack> y
 4918:02 <cangr> n
 5018:02 <joelklabo> y
 5118:02 <glozow> n but somewhat familiar
 5218:02 <prayank> n
 5318:02 <emzy> y (only tested it)
 5418:02 <Murch> y
 5518:02 <dhruvm> Awesome, let's get started
 5618:02 <dhruvm> Q: Why is it important to allow for evictions? What are the risks in rejecting new inbounds if slots are full?
 5718:02 <sipa> n
 5818:02 <effexzi> Hi
 5918:03 <joelklabo> could allow an eclipse attacker to replace all connections with theirs?
 6018:03 <thomasb06> we risk to be connected to only malcious nodes if we keep only old ones
 6118: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.)
 6218:03 <mango> Nodes with certain properties are prioritized (lower latency, better relay services)
 6318: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
 6418:04 <Murch> dhruvm: you could get stuck in a subcluster that loses connection to the main network
 6518:04 <dhruvm> That's right, joelklabo, amiti!
 6618: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.
 6718:04 <dhruvm> Good point on diversity stacie.
 6818:05 <Murch> (if nobody ever replaces their connections)
 6918:05 <dhruvm> Murch: network partitions are more probably if we don't evict for new inbounds, yes.
 7018:05 <dhruvm> s/probably/probable
 7118:05 <troygiorshev> Murch: +1, a static topology would be much easier to attack imo
 7218:05 <dhruvm> Q: What are the various ways AttemptToEvictConnection() tries to increase the cost of an eclipse attack?
 7318:05 <glozow> what a nice combo of individual node security + overall network health
 7418: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)
 7518:06 <Murch> glozow: +1
 7618:06 <thomasb06> There are ten: nTimeConnected, nMinPingUsecTime, ..., m_is_local
 7718:06 <sipa> the primary defense against eclipse attacks is our outbound peer selection, as those are far less under attacker control than inbound
 7818: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
 7918:07 ⚡ album sneaks in late into the back
 8018: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)?
 8118:08 <jonatack> recently gave us new txns and blocks
 8218:08 <dhruvm> thomasb06: Murch: jonatack: correct
 8318: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.
 8418:08 <thomasb06> (oof)
 8518:09 <sipa> dhruvm: right, it's a second order protection
 8618: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)?
 8718:09 <murtyjones> does that mean that blocksonly nodes are more likely to be evicted?
 8818: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
 8918:10 <thomasb06> An attack would be easier
 9018:10 <dhruvm> The new questions refers to code here: https://github.com/bitcoin/bitcoin/blob/f35e4d90/src/net.cpp#L932
 9118:11 <jonatack> some trivia, the original motivation behind the -netinfo dashboard was specifically to observe some of these inbound eviction criteria
 9218:11 <jonatack> murtyjones: block-relay-only connections are a type of outbound (not inbound) connection
 9318:11 <troygiorshev> jonatack: cool, thanks!
 9418:11 <dhruvm> murtyjones: https://github.com/bitcoin/bitcoin/blob/fabecce/src/net.cpp#L961
 9518: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?
 9618:12 <murtyjones> dhruvm jonatack thank you
 9718: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?
 9818: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?
 9918: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?
10018:14 <joelklabo> can you stay in the waiting to disconnect state for a long time?
10118:14 <Murch> Or do you mean, a node would change their netgroup to remain connected?
10218: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.
10318: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
10418:14 <Murch> stacie: I don't think the node would know it'll get disconnected until we disconnect, would it?
10518:15 <thomasb06> stacie: when the node reconnects, it becomes a good candidate to many criteria maybe
10618: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
10718:15 <sipa> stacie, Murch: an overall assumption is that access to multiple netgroups is more expensive than multiple connections from one netgroup
10818:17 <dhruvm> Speaking of net groups
10918:17 <dhruvm> Q: What is a net group? Why can Bitcoin Core group peers by ASNs instead of /16 subnets since V0.20?
11018: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
11118: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
11218:17 <Murch> joelklabo: Good question, can someone enlighten us?
11318:17 <sipa> joelklabo: milliseconds, i think
11418:17 <sipa> just until the net handling loop passes that node
11518:17 <dhruvm> Murch: joelklabo: ThreadSocketHandler takes care of the disconnections. It should be pretty fast.
11618:17 <joelklabo> Ah, not that long
11718:18 <sipa> mango: we only evict when we're running out of incoming connection slots, not before
11818: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
11918:19 <michaelfolkson> Murch: But you don't know why you've been disconnected
12018:19 <michaelfolkson> You're flailing in the dark
12118:19 <thomasb06> (lost)
12218:19 <michaelfolkson> I guess you could try things and see what causes disconnection
12318:20 <schmidty> dhruvm: -asmap config option from 0.20.0
12418:20 <Murch> michaelfolkson: You do know that you weren't in one of the protected categories, though
12518:20 <michaelfolkson> Murch: Right
12618:20 <dhruvm> schmidty: that's right
12718:21 <dhruvm> Can anyone tell us why ASNs are better net groups copared to /16 subnets?
12818:21 <Murch> dhruvm: The idea is that the ASNs split the network up better than the subnets
12918:21 <dhruvm> Murch: How so?
13018:21 <pinheadmz> I think IP distirbution has gotten mixed up between service providers.
13118:22 <pinheadmz> so you cant assume that 1.x.x.x and 2.x.x.x are in different parts of the world anymore
13218:22 <dhruvm> pinheadmz: exactly!
13318:22 <jonatack> see the asmap review club session: https://bitcoincore.reviews/16702
13418: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.
13518:22 <pinheadmz> now its possible both of those netgroups are owned by amazon,
13618: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
13718:23 <sipa> michaelfolkson: or just create more connections and see which live
13818:23 <sipa> michaelfolkson: or even better, decide that because of these protections, it"s not worth trying to attack
13918: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
14018:23 <stacie> the answer to :)
14118:23 <dhruvm> Ok, next question is related to an ongoing thread.
14218:23 <dhruvm> Q: Can an attacker predict which net groups are most likely to offer them protection from eviction?
14318:24 <emzy> Apple for example has 17.0.0.0/8
14418:27 <Murch> stacie: Only stumbling around in the dark here myself! Networking stuff is not really my forte. ;)
14518:27 <troygiorshev> dhruvm: well the code comments say no :D I'm still thinking of a good justification though...
14618:27 <dhruvm> troygiorshev: :)
14718:27 <Murch> dhruvm: Not really, because we never broadcast more than a subset of our peer connections
14818:27 <dhruvm> Murch: That's right
14918: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).
15018: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?
15118:27 <troygiorshev> Does someone have a quick explanation of KeyedNetGroup? Is the ordering exploitable?
15218:27 <dhruvm> michaelfolkson: I think CompareNetGroupKeyed sorts the peers by net group, not by count in the net group
15318:28 <Murch> michaelfolkson: That would sort of require the attacker again to do useful work, by populating underrepresented groups. :D
15418: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
15518:28 <dhruvm> michaelfolkson: so in terms of the protection from eviction due to net groups, the cardinality of the net group doesn't matter
15618: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?
15718:29 <michaelfolkson> Murch: If the underrepresented group is entirely populated by malicious parties that is a problem :)
15818:29 <Murch> michaelfolkson: huh, why? :)
15918:30 <jonatack> prayank: that's a relevant question
16018: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)
16118:30 <dhruvm> michaelfolkson: prayank: We have added protections for tor peers: https://github.com/bitcoin/bitcoin/blob/fabecce/src/net.cpp#L973
16218: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
16318: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
16418:30 <murtyjones> jonatack right, looks like they can be explicitly protected https://github.com/bitcoin/bitcoin/blob/1b75f2542d154ba6f9f75bc2c9d7a0ca54de784c/src/net.cpp#L962
16518: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
16618:31 <prayank> Interesting. Thanks
16718:31 <jonatack> recently the code was changed to protect tor peers, which by their longer ping times were being disadvantaged by our criteria
16818:31 <Murch> michaelfolkson: That only helps them to get one additional node not evicted though. Unless I'm missing something
16918:32 <sipa> jonatack: right, but anything netgroup related doesn't apply to tor connections
17018:32 <dhruvm> Ok, let's move on to the heart of the PR.
17118:32 <dhruvm> Q: Why is eviction guaranteed if we have at least 29 eviction candidates?
17218:33 <sipa> dhruvm: i'm confused by that, we should only be evicting when we're out of connection slots
17318:33 <Murch> Whoever answers this one should respond to nehan's comment. :)
17418:33 <sipa> (but haven't looked at that code in a while, or your PR)
17518:33 <nehan> yeah, i'm confused. this seems empirical more than guaranteed? i'm very curious where those numbers came from!
17618: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.
17718:34 <nehan> and how someone updating the eviction logic can figure out when/how to update the test...
17818:34 <schmidty> troygiorshev: so the DeterministicRandomizer prevents an attacker from knowing which net group will be preserved it looks like
17918:34 <sipa> dhruvm: ah, of course
18018:34 <dhruvm> sipa: SelectNodeToEvict assumes you want to evict
18118: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?"
18218:34 <sipa> schmidty: yup
18318: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
18418:36 <Murch> So, across the categories for protecting nodes from eviction, the total sum of protected nodes sums up to 28
18518:36 <jonatack> (here's the PR that made that change "Protect localhost and block-relay-only peers from eviction" https://github.com/bitcoin/bitcoin/pull/19670)
18618: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.
18718: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
18818: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
18918:37 <dhruvm> nehan: it was derived empirically but we can also see why this is the case by reviewing eviction logic in SelectNodeToEvict
19018: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
19118:38 <nehan> ok now explain the 20 :) why isn't it 8?
19218:38 <MarcoFalke> 4+8+4+0+4=20
19318:38 <dhruvm> Ok, so for 29:
19418: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.
19518:38 <schmidty> 4+8+4+8+4 = 28?
19618:39 <dhruvm> schmidty: correct!
19718: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
19818:39 <dhruvm> nehan has already moved on to the more interesting question
19918: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?
20018:39 <nehan> MarcoFalke: couldn't some of those nodes overlap?
20118:39 <jonatack> see test/functional/p2p_eviction.py for the 20
20218:39 <stacie> nehan subtract the 8 in case there aren't any non-relay-tx peers? err.. that's my best guess
20318:39 <Murch> Okay, I may be misreading that on localhost
20418:39 <jonatack> lines 40-41
20518:40 <Murch> dhruvm probably did more research than me ^^
20618:40 <nehan> jonatack: aha! thank you!
20718:40 <jonatack> nehan: :)
20818:40 <dhruvm> stacie: you're close. why would the randomly generated peers not qualify for the non-tx-relay protection?
20918:40 <MarcoFalke> nehan: nodes are removed from the list, so they can't overlap
21018:41 <nehan> MarcoFalke: ah right, EraseLastKElements, thanks
21118:42 <Murch> nehan: I think it's 8 from non-tx-relay peers and 4 _more_ by last novel block.
21218:42 <Murch> Each category removes eviction candidates, any new category will pick from the remaining
21318:43 <jonatack> Murch: seems so
21418:43 <jonatack> since #19670
21518:43 <dhruvm> Right. So it is indeed about the 8 non-tx-relay peers
21618: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?
21718:43 <Murch> so, it's only non-tx-relay peers, because we are not guaranteed to have any
21818:43 <dhruvm> guaranteed non-eviction is only on 4(CompareNetGroupKeyed) + 8(ReverseCompareNodeMinPingTime) + 4(CompareNodeTXTime) + 4(CompareNodeBlockTime) = 20.
21918:43 <Murch> oh, stacie is way ahead of me
22018: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
22118:45 <dhruvm> Q: How does the new unit test leverage this information about guaranteed eviction and non-eviction?
22218: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
22318:47 <stacie> dhruvm that makes sense. Thanks!
22418:47 <murtyjones> https://github.com/bitcoin/bitcoin/pull/20477/files#diff-489a7da84b6a2cfb42207e32a69bf0f92a306310a5d6dc1ec72dc54a32d7817bR847
22518:48 <dhruvm> murtyjones: correct!
22618:48 <dhruvm> although only at most one is ever evicted
22718: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.
22818: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.
22918:48 <dhruvm> schmidty: you got it
23018:49 <nehan> so interestingly, after https://github.com/bitcoin/bitcoin/blob/fabecce/src/net.cpp#L968 the vEvictioNCandidates.size() could be 0, and total_protect_size will end up being 0. just in case that helps anyone else.
23118: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.
23218:49 <dhruvm> Murch: exactly
23318:50 <MarcoFalke> I don't get the 'continue' in the test either
23418:50 <dhruvm> MarcoFalke: that bit is definitely awkward. I failed to offer a simpler formulation though.
23518:51 <dhruvm> MarcoFalke: the continue is trying to cover for the 21-28 case where we don't have eviction guarantees
23618: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.
23718:52 <Murch> dhruvm: no it happens for everything below 29
23818:52 <MarcoFalke> dhruvm: The checks that follow are for non-evicition, not for eviction
23918: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?
24018:52 <dhruvm> Murch: MarcoFalke: Yes, you are correct. Sorry.
24118:53 <dhruvm> nehan: that's a good question. we should probably be checking for that.
24218:53 <Murch> I guess it only checks whether the protected groups are full, because the count in the categories is not stable below 29
24318:54 <dhruvm> nehan: instead of checking for does eviction happen, the test should be "if eviction happens, the correct one does"
24418:54 <dhruvm> This PR was a good introduction to some modern idiomatic C++ concepts for me. Let's talk about move semantics and lambdas.
24518:54 <dhruvm> Q: What does std::move do here: https://github.com/bitcoin-core-review-club/bitcoin/commit/fbba7d8aada5b1d7a63ad4133dee32533d6700f2#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02R1008 ?
24618:54 <nehan> Murch: I don't understand what you just said. it definitely exits out via the continue if 20 < number_of_nodes < 29?
24718:55 <jnewbery> nehan: I think you're right that you could remove the else continue and the test would run fine
24818:55 <murtyjones> i interpret std::move as moving the node to evict from vEvictionCandidates to node_id_to_evict
24918:55 <MarcoFalke> dhruvm: std::move makes a static_cast to the "double-ampersand type"
25018:56 <murtyjones> or rather, maybe moving depending on the result of SelectNodeToEvict
25118:56 <jonatack> and avoids a copy
25218: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
25318:56 <dhruvm> MarcoFalke: jonatack: correct!
25418: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.
25518:56 <dhruvm> More about move constructors here: https://en.cppreference.com/w/cpp/language/move_constructor
25618: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
25718:56 <jonatack> https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Res-move
25818:56 <jnewbery> because we're testing what's protected, not what's evicted
25918:56 <jonatack> "Write std::move() only when you need to explicitly move an object to another scope"
26018: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
26118:57 <dhruvm> Last question.
26218:57 <MarcoFalke> michaelfolkson: I'd imagine this is hard to setup as functional test
26318:57 <dhruvm> Q: What is the purpose of the candidate_setup_fn lambda here: https://github.com/bitcoin-core-review-club/bitcoin/commit/cadd93f35d1bbf662e348a0dee172cdf4af6a903#diff-489a7da84b6a2cfb42207e32a69bf0f92a306310a5d6dc1ec72dc54a32d7817bR809 ?
26418:57 <murtyjones> gotta run but thank you dhruvm this was very informative!
26518:57 <troygiorshev> michaelfolkson: tbh here it's "if python then functional, if c++ then unit"
26618:58 <michaelfolkson> troygiorshev: Is that the Core philosophy? :) I wasn't aware. Whether the author wants to write C++ or Python?
26718:58 <troygiorshev> michaelfolkson: that said, because we're calling IsEvicted directly, it's a unit test
26818: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
26918: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
27018:58 <Murch> jnewbery: I don't understand why the test would work as written when the protected categories wouldn't be full
27118:59 <michaelfolkson> Interesting, thanks troygiorshev sipa
27218: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.
27319: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
27419:00 <jnewbery> I really like the way the unit test is constructed. Passing a lambda to manipulate the eviction candidates is neat
27519: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!
27619:00 <sipa> thanks, dhruvm!
27719:00 <glozow> thank you dhruvm!!!
27819:00 <dhruvm> #endmeeting