--- Day changed Wed Dec 09 2020 00:25 -!- da39a3ee5e6b4b0d [~da39a3ee5@171.5.29.209] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…] 00:36 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Read error: Connection reset by peer] 00:38 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has joined #bitcoin-core-pr-reviews 00:46 -!- musdom [~Thunderbi@202.184.0.102] has quit [Ping timeout: 256 seconds] 00:57 -!- virtu [~virtu@gateway/tor-sasl/virtu] has quit [Ping timeout: 240 seconds] 01:12 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has quit [Ping timeout: 240 seconds] 02:11 -!- da39a3ee5e6b4b0d [~da39a3ee5@mx-ll-171.5.29-209.dynamic.3bb.co.th] has joined #bitcoin-core-pr-reviews 02:37 -!- belcher_ is now known as belcher 02:40 -!- musdom [~Thunderbi@202.184.0.102] has joined #bitcoin-core-pr-reviews 03:09 -!- darosior9 is now known as darosior 03:10 -!- da39a3ee5e6b4b0d [~da39a3ee5@mx-ll-171.5.29-209.dynamic.3bb.co.th] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…] 03:16 -!- seven_ [~seven@cpe-90-157-197-248.static.amis.net] has joined #bitcoin-core-pr-reviews 03:16 -!- ctrlbreak [~ctrlbreak@159.2.182.106] has quit [Ping timeout: 272 seconds] 03:24 -!- peterrizzo_ [~peterrizz@ool-44c18924.dyn.optonline.net] has joined #bitcoin-core-pr-reviews 04:24 -!- da39a3ee5e6b4b0d [~da39a3ee5@171.5.29.209] has joined #bitcoin-core-pr-reviews 04:41 -!- pinheadmz [~pinheadmz@71.190.30.138] has joined #bitcoin-core-pr-reviews 04:55 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 05:01 -!- da39a3ee5e6b4b0d [~da39a3ee5@171.5.29.209] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…] 05:17 -!- peterrizzo_ [~peterrizz@ool-44c18924.dyn.optonline.net] has quit [Quit: peterrizzo_] 05:29 -!- ctrlbreak [~ctrlbreak@159.2.165.130] has joined #bitcoin-core-pr-reviews 05:35 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 05:35 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Disconnected by services] 05:35 -!- vasild_ is now known as vasild 05:41 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection] 05:41 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews 05:42 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection] 05:42 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews 05:48 -!- instagibbs [~greg@061093103011.ctinets.com] has joined #bitcoin-core-pr-reviews 05:58 -!- peterrizzo_ [~peterrizz@ool-44c18924.dyn.optonline.net] has joined #bitcoin-core-pr-reviews 06:05 -!- heebs [68c2dcef@104.194.220.239] has joined #bitcoin-core-pr-reviews 06:26 -!- heebs is now known as hieblmi 07:03 -!- seven_ [~seven@cpe-90-157-197-248.static.amis.net] has quit [Remote host closed the connection] 07:03 -!- seven_ [~seven@cpe-90-157-197-248.static.amis.net] has joined #bitcoin-core-pr-reviews 07:52 -!- kouloumos [59d260bf@ppp089210096191.access.hol.gr] has quit [Ping timeout: 245 seconds] 07:52 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Remote host closed the connection] 07:52 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection] 07:52 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 07:52 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 08:03 -!- kouloumos [59d260bf@ppp089210096191.access.hol.gr] has joined #bitcoin-core-pr-reviews 08:04 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Remote host closed the connection] 08:05 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 08:15 < dhruvm> ja: jnewbery: thank you 08:20 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 08:29 -!- prayank [~andr0irc@2409:4053:2e80:6b21:9cd7:456a:ee7f:b0d6] has joined #bitcoin-core-pr-reviews 08:29 -!- cangr [~cangr@dynamic-077-010-075-180.77.10.pool.telefonica.de] has joined #bitcoin-core-pr-reviews 08:32 -!- Guest56 [47edaff3@gateway/web/cgi-irc/kiwiirc.com/ip.71.237.175.243] has joined #bitcoin-core-pr-reviews 08:32 -!- Guest56 is now known as samuel-pedraza 08:35 -!- cangr [~cangr@dynamic-077-010-075-180.77.10.pool.telefonica.de] has quit [Quit: leaving] 08:35 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has joined #bitcoin-core-pr-reviews 08:40 -!- effexzi [uid474242@gateway/web/irccloud.com/x-ybbcgefqdjjcuavw] has joined #bitcoin-core-pr-reviews 08:41 < dhruvm> Hello, everyone! We will get started in 19 minutes. PR Review Club for #20477: test/net: Add unit testing of node eviction logic. 08:41 < dhruvm> https://github.com/bitcoin/bitcoin/pull/20477 08:45 -!- cangr [~cangr@128.90.132.249] has joined #bitcoin-core-pr-reviews 08:49 -!- belcher_ [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 08:49 -!- belcher [~belcher@unaffiliated/belcher] has quit [Ping timeout: 240 seconds] 08:50 -!- elle [~ellemouto@155.93.252.70] has joined #bitcoin-core-pr-reviews 08:53 -!- glozow [uid453516@gateway/web/irccloud.com/x-sunfxzxwwbiziafi] has joined #bitcoin-core-pr-reviews 08:55 -!- stacie [~stacie@c-24-60-139-217.hsd1.ma.comcast.net] has joined #bitcoin-core-pr-reviews 08:56 -!- murtyjones [ae612166@cpe-174-97-33-102.cinci.res.rr.com] has joined #bitcoin-core-pr-reviews 08:58 -!- mango [~mango@c-73-71-224-94.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 08:58 -!- joelklabo [~textual@108-196-216-127.lightspeed.sntcca.sbcglobal.net] has joined #bitcoin-core-pr-reviews 08:59 < joelklabo> 👋 hi everyone 08:59 < cangr> hi 08:59 -!- andozw [ae1f1f5f@174.31.31.95] has joined #bitcoin-core-pr-reviews 08:59 < glozow> sup fam 08:59 < michaelfolkson> One must wait until 5pm to say hi 09:00 < MarcoFalke> hi 09:00 < dhruvm> #startmeeting 09:00 < emzy> hi 09:00 < elle> hi :) 09:00 < hieblmi> hi 09:00 < michaelfolkson> (Joke) 09:00 < pinheadmz> hi 09:00 < willcl_ark> hi 09:00 < dhruvm> Hello everyone! Welcome to the PR Review Club for #20477: test/net: Add unit testing of node eviction logic. 09:00 < samuel-pedraza> hi 09:00 < nehan> hi 09:00 < mango> hi 09:00 < dhruvm> Please say hi so everyone knows who is in attendance. 09:00 < andozw> hiiii 09:00 < joelklabo> haha, michaelfolkson 09:00 < schmidty> hi 09:00 < murtyjones> hi 09:00 < troygiorshev> hi! 09:00 < joelklabo> jumped the gun 09:00 < jnewbery> hi! 09:00 < jonatack> hi 09:00 < prayank> hello world 09:00 < glozow> hi 09:00 < kouloumos> hi! 09:00 < dhruvm> Anyone here for the first time? 09:00 < stacie> hi 09:00 < samuel-pedraza> yes, hi! 09:00 < murtyjones> I am 09:00 < thomasb06> hi 09:00 < kouloumos> yes 09:00 < dhruvm> welcome, samuel-pedraza! 09:01 < amiti> hi ! 09:01 < dhruvm> nice to meet you, murtyjones, kouloumos 09:01 < robot-dreams> hi 09:01 < Murch> ih 09:01 < Murch> hi 09:01 < dhruvm> A reminder of meeting convention: When you have a question, don't ask to ask, just ask. 09:01 < dhruvm> Did everyone get a chance to review the PR? How about a quick y/n from everyone 09:01 < jnewbery> y 09:01 < mango> y 09:01 < nehan> y 09:01 < stacie> y 09:02 < samuel-pedraza> y 09:02 < troygiorshev> y 09:02 < pinheadmz> n :-( lkurking 09:02 < thomasb06> y 09:02 < murtyjones> n 09:02 < kouloumos> n 09:02 < jonatack> y 09:02 < cangr> n 09:02 < joelklabo> y 09:02 < glozow> n but somewhat familiar 09:02 < prayank> n 09:02 < emzy> y (only tested it) 09:02 < Murch> y 09:02 < dhruvm> Awesome, let's get started 09:02 < dhruvm> Q: Why is it important to allow for evictions? What are the risks in rejecting new inbounds if slots are full? 09:02 < sipa> n 09:02 < effexzi> Hi 09:03 < joelklabo> could allow an eclipse attacker to replace all connections with theirs? 09:03 < thomasb06> we risk to be connected to only malcious nodes if we keep only old ones 09: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.) 09:03 < mango> Nodes with certain properties are prioritized (lower latency, better relay services) 09: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 09:04 < Murch> dhruvm: you could get stuck in a subcluster that loses connection to the main network 09:04 < dhruvm> That's right, joelklabo, amiti! 09: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. 09:04 < dhruvm> Good point on diversity stacie. 09:05 < Murch> (if nobody ever replaces their connections) 09:05 < dhruvm> Murch: network partitions are more probably if we don't evict for new inbounds, yes. 09:05 -!- anir [40bba0b7@64.187.160.183] has joined #bitcoin-core-pr-reviews 09:05 < dhruvm> s/probably/probable 09:05 < troygiorshev> Murch: +1, a static topology would be much easier to attack imo 09:05 -!- anonymous5379 [d4662c37@212.102.44.55] has joined #bitcoin-core-pr-reviews 09:05 < dhruvm> Q: What are the various ways AttemptToEvictConnection() tries to increase the cost of an eclipse attack? 09:05 < glozow> what a nice combo of individual node security + overall network health 09: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) 09:06 < Murch> glozow: +1 09:06 < thomasb06> There are ten: nTimeConnected, nMinPingUsecTime, ..., m_is_local 09:06 < sipa> the primary defense against eclipse attacks is our outbound peer selection, as those are far less under attacker control than inbound 09:06 -!- album [~album@195.181.160.175.adsl.inet-telecom.org] has joined #bitcoin-core-pr-reviews 09: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 09:07 * album sneaks in late into the back 09: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)? 09:08 < jonatack> recently gave us new txns and blocks 09:08 < dhruvm> thomasb06: Murch: jonatack: correct 09: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. 09:08 < thomasb06> (oof) 09:09 < sipa> dhruvm: right, it's a second order protection 09: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)? 09:09 < murtyjones> does that mean that blocksonly nodes are more likely to be evicted? 09:09 -!- dappdever [~dappdever@86.106.143.60] has joined #bitcoin-core-pr-reviews 09: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 09:10 < thomasb06> An attack would be easier 09:10 < dhruvm> The new questions refers to code here: https://github.com/bitcoin/bitcoin/blob/f35e4d90/src/net.cpp#L932 09:11 < jonatack> some trivia, the original motivation behind the -netinfo dashboard was specifically to observe some of these inbound eviction criteria 09:11 < jonatack> murtyjones: block-relay-only connections are a type of outbound (not inbound) connection 09:11 < troygiorshev> jonatack: cool, thanks! 09:11 < dhruvm> murtyjones: https://github.com/bitcoin/bitcoin/blob/fabecce/src/net.cpp#L961 09: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? 09:12 < murtyjones> dhruvm jonatack thank you 09: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? 09: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? 09: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? 09:14 < joelklabo> can you stay in the waiting to disconnect state for a long time? 09:14 < Murch> Or do you mean, a node would change their netgroup to remain connected? 09: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. 09: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 09:14 < Murch> stacie: I don't think the node would know it'll get disconnected until we disconnect, would it? 09:15 < thomasb06> stacie: when the node reconnects, it becomes a good candidate to many criteria maybe 09: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 09:15 < sipa> stacie, Murch: an overall assumption is that access to multiple netgroups is more expensive than multiple connections from one netgroup 09:16 < dhruvm> Speaking of net groups 09:16 < dhruvm> Q: What is a net group? Why can Bitcoin Core group peers by ASNs instead of /16 subnets since V0.20? 09:16 < 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 09:16 < 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 09:16 < Murch> joelklabo: Good question, can someone enlighten us? 09:17 < sipa> joelklabo: milliseconds, i think 09:17 < sipa> just until the net handling loop passes that node 09:17 < dhruvm> Murch: joelklabo: ThreadSocketHandler takes care of the disconnections. It should be pretty fast. 09:17 < joelklabo> Ah, not that long 09:18 < sipa> mango: we only evict when we're running out of incoming connection slots, not before 09: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 09:19 < michaelfolkson> Murch: But you don't know why you've been disconnected 09:19 < michaelfolkson> You're flailing in the dark 09:19 < thomasb06> (lost) 09:19 < michaelfolkson> I guess you could try things and see what causes disconnection 09:20 -!- prata [67699813@103.105.152.19] has joined #bitcoin-core-pr-reviews 09:20 < schmidty> dhruvm: -asmap config option from 0.20.0 09:20 < Murch> michaelfolkson: You do know that you weren't in one of the protected categories, though 09:20 < michaelfolkson> Murch: Right 09:20 < dhruvm> schmidty: that's right 09:21 < dhruvm> Can anyone tell us why ASNs are better net groups copared to /16 subnets? 09:21 < Murch> dhruvm: The idea is that the ASNs split the network up better than the subnets 09:21 -!- larryruane_ [uid473749@gateway/web/irccloud.com/x-ucsaxcwppqjkhsjq] has joined #bitcoin-core-pr-reviews 09:21 < dhruvm> Murch: How so? 09:21 < pinheadmz> I think IP distirbution has gotten mixed up between service providers. 09:22 < pinheadmz> so you cant assume that 1.x.x.x and 2.x.x.x are in different parts of the world anymore 09:22 < dhruvm> pinheadmz: exactly! 09:22 < jonatack> see the asmap review club session: https://bitcoincore.reviews/16702 09: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. 09:22 < pinheadmz> now its possible both of those netgroups are owned by amazon, 09: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 09:23 < sipa> michaelfolkson: or just create more connections and see which live 09:23 < sipa> michaelfolkson: or even better, decide that because of these protections, it"s not worth trying to attack 09: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 09:23 < stacie> the answer to :) 09:23 < dhruvm> Ok, next question is related to an ongoing thread. 09:23 < dhruvm> Q: Can an attacker predict which net groups are most likely to offer them protection from eviction? 09:24 < emzy> Apple for example has 17.0.0.0/8 09:24 < Murch> stacie: Only stumbling around in the dark here myself! Networking stuff is not really my forte. ;) 09:25 < troygiorshev> dhruvm: well the code comments say no :D I'm still thinking of a good justification though... 09:25 < dhruvm> troygiorshev: :) 09:25 < Murch> dhruvm: Not really, because we never broadcast more than a subset of our peer connections 09:26 < dhruvm> Murch: That's right 09:26 < 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). 09:26 < michaelfolkson> dhruvm: If the attacker knows which net groups are less populated on the network due to gossiping. Or are they evenly distributed somehow? 09:27 < troygiorshev> Does someone have a quick explanation of KeyedNetGroup? Is the ordering exploitable? 09:27 < dhruvm> michaelfolkson: I think CompareNetGroupKeyed sorts the peers by net group, not by count in the net group 09:28 < Murch> michaelfolkson: That would sort of require the attacker again to do useful work, by populating underrepresented groups. :D 09: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 09:28 < dhruvm> michaelfolkson: so in terms of the protection from eviction due to net groups, the cardinality of the net group doesn't matter 09: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? 09:29 < michaelfolkson> Murch: If the underrepresented group is entirely populated by malicious parties that is a problem :) 09:29 < Murch> michaelfolkson: huh, why? :) 09:30 < jonatack> prayank: that's a relevant question 09: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) 09:30 < dhruvm> michaelfolkson: prayank: We have added protections for tor peers: https://github.com/bitcoin/bitcoin/blob/fabecce/src/net.cpp#L973 09: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 09: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 09:30 < murtyjones> jonatack right, looks like they can be explicitly protected https://github.com/bitcoin/bitcoin/blob/1b75f2542d154ba6f9f75bc2c9d7a0ca54de784c/src/net.cpp#L962 09: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 09:31 < prayank> Interesting. Thanks 09:31 < jonatack> recently the code was changed to protect tor peers, which by their longer ping times were being disadvantaged by our criteria 09:31 < Murch> michaelfolkson: That only helps them to get one additional node not evicted though. Unless I'm missing something 09:32 < sipa> jonatack: right, but anything netgroup related doesn't apply to tor connections 09:32 < dhruvm> Ok, let's move on to the heart of the PR. 09:32 < dhruvm> Q: Why is eviction guaranteed if we have at least 29 eviction candidates? 09:33 < sipa> dhruvm: i'm confused by that, we should only be evicting when we're out of connection slots 09:33 < Murch> Whoever answers this one should respond to nehan's comment. :) 09:33 < sipa> (but haven't looked at that code in a while, or your PR) 09:33 < nehan> yeah, i'm confused. this seems empirical more than guaranteed? i'm very curious where those numbers came from! 09: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. 09:34 < nehan> and how someone updating the eviction logic can figure out when/how to update the test... 09:34 < schmidty> troygiorshev: so the DeterministicRandomizer prevents an attacker from knowing which net group will be preserved it looks like 09:34 < sipa> dhruvm: ah, of course 09:34 < dhruvm> sipa: SelectNodeToEvict assumes you want to evict 09: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?" 09:34 < sipa> schmidty: yup 09: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 09:36 < Murch> So, across the categories for protecting nodes from eviction, the total sum of protected nodes sums up to 28 09: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) 09: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. 09: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 09: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 09:37 < dhruvm> nehan: it was derived empirically but we can also see why this is the case by reviewing eviction logic in SelectNodeToEvict 09: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 09:38 < nehan> ok now explain the 20 :) why isn't it 8? 09:38 < MarcoFalke> 4+8+4+0+4=20 09:38 < dhruvm> Ok, so for 29: 09: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. 09:38 < schmidty> 4+8+4+8+4 = 28? 09:39 < dhruvm> schmidty: correct! 09: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 09:39 < dhruvm> nehan has already moved on to the more interesting question 09: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? 09:39 < nehan> MarcoFalke: couldn't some of those nodes overlap? 09:39 < jonatack> see test/functional/p2p_eviction.py for the 20 09:39 < stacie> nehan subtract the 8 in case there aren't any non-relay-tx peers? err.. that's my best guess 09:39 < Murch> Okay, I may be misreading that on localhost 09:39 < jonatack> lines 40-41 09:40 < Murch> dhruvm probably did more research than me ^^ 09:40 < nehan> jonatack: aha! thank you! 09:40 < jonatack> nehan: :) 09:40 < dhruvm> stacie: you're close. why would the randomly generated peers not qualify for the non-tx-relay protection? 09:40 < MarcoFalke> nehan: nodes are removed from the list, so they can't overlap 09:41 < nehan> MarcoFalke: ah right, EraseLastKElements, thanks 09:42 < Murch> nehan: I think it's 8 from non-tx-relay peers and 4 _more_ by last novel block. 09:42 < Murch> Each category removes eviction candidates, any new category will pick from the remaining 09:43 < jonatack> Murch: seems so 09:43 < jonatack> since #19670 09:43 -!- cangr [~cangr@128.90.132.249] has quit [Ping timeout: 240 seconds] 09:43 < dhruvm> Right. So it is indeed about the 8 non-tx-relay peers 09: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? 09:43 < Murch> so, it's only non-tx-relay peers, because we are not guaranteed to have any 09:43 < dhruvm> guaranteed non-eviction is only on 4(CompareNetGroupKeyed) + 8(ReverseCompareNodeMinPingTime) + 4(CompareNodeTXTime) + 4(CompareNodeBlockTime) = 20. 09:43 < Murch> oh, stacie is way ahead of me 09: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 09:45 < dhruvm> Q: How does the new unit test leverage this information about guaranteed eviction and non-eviction? 09:45 -!- cangr [~cangr@dynamic-077-010-075-180.77.10.pool.telefonica.de] has joined #bitcoin-core-pr-reviews 09: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 09:47 < stacie> dhruvm that makes sense. Thanks! 09:47 < murtyjones> https://github.com/bitcoin/bitcoin/pull/20477/files#diff-489a7da84b6a2cfb42207e32a69bf0f92a306310a5d6dc1ec72dc54a32d7817bR847 09:47 -!- mango [~mango@c-73-71-224-94.hsd1.ca.comcast.net] has quit [Quit: Textual IRC Client: www.textualapp.com] 09:48 < dhruvm> murtyjones: correct! 09:48 < dhruvm> although only at most one is ever evicted 09: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. 09: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. 09:48 < dhruvm> schmidty: you got it 09: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. 09: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. 09:49 < dhruvm> Murch: exactly 09:50 < MarcoFalke> I don't get the 'continue' in the test either 09:50 < dhruvm> MarcoFalke: that bit is definitely awkward. I failed to offer a simpler formulation though. 09:51 < dhruvm> MarcoFalke: the continue is trying to cover for the 21-28 case where we don't have eviction guarantees 09: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. 09:52 < Murch> dhruvm: no it happens for everything below 29 09:52 < MarcoFalke> dhruvm: The checks that follow are for non-evicition, not for eviction 09: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? 09:52 < dhruvm> Murch: MarcoFalke: Yes, you are correct. Sorry. 09:53 < dhruvm> nehan: that's a good question. we should probably be checking for that. 09:53 < Murch> I guess it only checks whether the protected groups are full, because the count in the categories is not stable below 29 09:54 < dhruvm> nehan: instead of checking for does eviction happen, the test should be "if eviction happens, the correct one does" 09:54 < dhruvm> This PR was a good introduction to some modern idiomatic C++ concepts for me. Let's talk about move semantics and lambdas. 09:54 < dhruvm> Q: What does std::move do here: https://github.com/bitcoin-core-review-club/bitcoin/commit/fbba7d8aada5b1d7a63ad4133dee32533d6700f2#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02R1008 ? 09:54 < nehan> Murch: I don't understand what you just said. it definitely exits out via the continue if 20 < number_of_nodes < 29? 09:55 < jnewbery> nehan: I think you're right that you could remove the else continue and the test would run fine 09:55 < murtyjones> i interpret std::move as moving the node to evict from vEvictionCandidates to node_id_to_evict 09:55 < MarcoFalke> dhruvm: std::move makes a static_cast to the "double-ampersand type" 09:56 < murtyjones> or rather, maybe moving depending on the result of SelectNodeToEvict 09:56 < jonatack> and avoids a copy 09: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 09:56 < dhruvm> MarcoFalke: jonatack: correct! 09: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. 09:56 < dhruvm> More about move constructors here: https://en.cppreference.com/w/cpp/language/move_constructor 09: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 09:56 < jonatack> https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Res-move 09:56 < jnewbery> because we're testing what's protected, not what's evicted 09:56 < jonatack> "Write std::move() only when you need to explicitly move an object to another scope" 09: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 09:57 < dhruvm> Last question. 09:57 < MarcoFalke> michaelfolkson: I'd imagine this is hard to setup as functional test 09: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 ? 09:57 < murtyjones> gotta run but thank you dhruvm this was very informative! 09:57 < troygiorshev> michaelfolkson: tbh here it's "if python then functional, if c++ then unit" 09:57 -!- murtyjones [ae612166@cpe-174-97-33-102.cinci.res.rr.com] has quit [Remote host closed the connection] 09:58 < michaelfolkson> troygiorshev: Is that the Core philosophy? :) I wasn't aware. Whether the author wants to write C++ or Python? 09:58 < troygiorshev> michaelfolkson: that said, because we're calling IsEvicted directly, it's a unit test 09: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 09: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 09:58 < Murch> jnewbery: I don't understand why the test would work as written when the protected categories wouldn't be full 09:59 < michaelfolkson> Interesting, thanks troygiorshev sipa 09: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. 10: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 10:00 < jnewbery> I really like the way the unit test is constructed. Passing a lambda to manipulate the eviction candidates is neat 10: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! 10:00 < sipa> thanks, dhruvm! 10:00 < glozow> thank you dhruvm!!! 10:00 < dhruvm> #endmeeting 10:00 < Murch> Thanks! 10:00 < jnewbery> dhruvm: thanks for hosting :D 10:00 < anonymous5379> thanks, dhruvm 10:00 < troygiorshev> thanks dhruvm! 10:00 < MarcoFalke> thanks for hosting dhruvm 10:00 < nehan> thank you! 10:00 < stacie> Thanks dhruvm! 10:00 < thomasb06> thanks dhruvm 10:00 < schmidty> ty dhruvm 10:00 < album> thanks dhruvm 10:00 < dhruvm> jnewbery: jonatack: thank you for the opportunity! 10:00 < samuel-pedraza> thanks dhruvm 10:00 < emzy> thanks dhruvm! 10:00 < hieblmi> thank you! 10:00 < anir> thanks dhruvm! 10:00 < prayank> Thanks dhruvm and everyone 10:00 < jonatack> jnewbery: agree, std::move is misnamed 10:00 -!- elle [~ellemouto@155.93.252.70] has quit [Quit: leaving] 10:00 < joelklabo> thanks dhruvm! 10:01 -!- andozw [ae1f1f5f@174.31.31.95] has quit [Remote host closed the connection] 10:01 -!- hieblmi [68c2dcef@104.194.220.239] has quit [Remote host closed the connection] 10:01 < kouloumos> thank you, it was at least an inspiring first PR review club for me 10:01 < Murch> jnewbery: Yeah, it's very clean but thorough, I think 10:01 < jonatack> Thanks dhruvm! Great job 🏆 10:01 < jnewbery> Next week, willcl_ark is hosting. We'll get notes and questions posted by the weekend. 10:01 < Murch> nehan is right though, at lower numbers, it could check whether the first categories are protected up to the number of nodes 10:02 < jnewbery> and then we'll take a two week break for the holidays 10:02 < jnewbery> so make sure you come along next week for the final review club of 2020 10:02 < pinheadmz> nice! willcl_ark loking forward to it! 10:03 -!- samuel-pedraza [47edaff3@gateway/web/cgi-irc/kiwiirc.com/ip.71.237.175.243] has quit [Quit: Connection closed] 10:04 < emzy> jnewbery: In wumpus calendar there is a meetup 12/23 and 12/30. 10:05 < MarcoFalke> the calendar might just be a weekly event 10:05 -!- prata [67699813@103.105.152.19] has left #bitcoin-core-pr-reviews [] 10:06 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews 10:07 < emzy> I just made a local note in my calendar :) 10:07 -!- cangr [~cangr@dynamic-077-010-075-180.77.10.pool.telefonica.de] has quit [Quit: leaving] 10:08 < dhruvm> jonatack: I often think std::move should be rvalue_cast or something like that 10:09 < dhruvm> jonatack: std::move sounds like an expensive call :D 10:24 < jonatack> dhruvm: yup, Scott Meyers, page 159 in the book jnewbery mentioned, rvalue_cast 10:24 -!- album [~album@195.181.160.175.adsl.inet-telecom.org] has quit [Ping timeout: 265 seconds] 10:25 < jonatack> (good reminder to re-read it) 10:25 -!- kouloumos [59d260bf@ppp089210096191.access.hol.gr] has quit [Remote host closed the connection] 10:27 -!- heebs [68c2dcef@104.194.220.239] has joined #bitcoin-core-pr-reviews 11:00 -!- heebs [68c2dcef@104.194.220.239] has quit [Remote host closed the connection] 11:02 -!- dappdever [~dappdever@86.106.143.60] has quit [] 11:04 < jonatack> The meeting log is up at https://bitcoincore.reviews/20477 11:06 -!- musdom [~Thunderbi@202.184.0.102] has quit [Ping timeout: 264 seconds] 11:24 -!- stacie [~stacie@c-24-60-139-217.hsd1.ma.comcast.net] has quit [Quit: stacie] 11:30 -!- virtu [~virtu@gateway/tor-sasl/virtu] has joined #bitcoin-core-pr-reviews 11:35 -!- anonymous5379 [d4662c37@212.102.44.55] has quit [Remote host closed the connection] 12:05 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!] 12:20 -!- effexzi [uid474242@gateway/web/irccloud.com/x-ybbcgefqdjjcuavw] has quit [Quit: Connection closed for inactivity] 12:53 -!- glozow [uid453516@gateway/web/irccloud.com/x-sunfxzxwwbiziafi] has quit [Quit: Connection closed for inactivity] 13:16 -!- belcher_ is now known as belcher 13:47 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has quit [Remote host closed the connection] 13:56 -!- prayank [~andr0irc@2409:4053:2e80:6b21:9cd7:456a:ee7f:b0d6] has quit [Ping timeout: 260 seconds] 14:44 -!- prayank [~andr0irc@2409:4053:2e80:6b21:9cd7:456a:ee7f:b0d6] has joined #bitcoin-core-pr-reviews 14:47 -!- prayank [~andr0irc@2409:4053:2e80:6b21:9cd7:456a:ee7f:b0d6] has quit [Read error: Connection reset by peer] 14:57 -!- jrawsthorne [~jrawsthor@static.235.41.217.95.clients.your-server.de] has quit [Read error: Connection reset by peer] 14:57 -!- jrawsthorne [~jrawsthor@static.235.41.217.95.clients.your-server.de] has joined #bitcoin-core-pr-reviews 15:09 -!- peterrizzo_ [~peterrizz@ool-44c18924.dyn.optonline.net] has quit [Read error: Connection reset by peer] 15:09 -!- peterrizzo_ [~peterrizz@ool-44c18924.dyn.optonline.net] has joined #bitcoin-core-pr-reviews 15:26 -!- seven_ [~seven@cpe-90-157-197-248.static.amis.net] has quit [Ping timeout: 260 seconds] 15:42 -!- peterrizzo_ [~peterrizz@ool-44c18924.dyn.optonline.net] has quit [Quit: peterrizzo_] 16:01 -!- da39a3ee5e6b4b0d [~da39a3ee5@mx-ll-171.5.29-209.dynamic.3bb.co.th] has joined #bitcoin-core-pr-reviews 16:40 -!- anir [40bba0b7@64.187.160.183] has quit [Remote host closed the connection] 16:41 -!- virtu [~virtu@gateway/tor-sasl/virtu] has quit [Ping timeout: 240 seconds] 16:43 -!- virtu [~virtu@gateway/tor-sasl/virtu] has joined #bitcoin-core-pr-reviews 17:35 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Disconnected by services] 17:35 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 17:35 -!- vasild_ is now known as vasild 17:41 -!- prayank [~andr0irc@2409:4053:2e80:6b21:9cd7:456a:ee7f:b0d6] has joined #bitcoin-core-pr-reviews 17:43 -!- prayank [~andr0irc@2409:4053:2e80:6b21:9cd7:456a:ee7f:b0d6] has quit [Read error: Connection reset by peer] 17:44 -!- prayank [~andr0irc@2409:4053:2e80:6b21:9cd7:456a:ee7f:b0d6] has joined #bitcoin-core-pr-reviews 18:19 -!- prayank [~andr0irc@2409:4053:2e80:6b21:9cd7:456a:ee7f:b0d6] has quit [Ping timeout: 258 seconds] 19:09 -!- prayank [~andr0irc@2409:4053:2e80:6b21:9cd7:456a:ee7f:b0d6] has joined #bitcoin-core-pr-reviews 19:12 -!- prayank [~andr0irc@2409:4053:2e80:6b21:9cd7:456a:ee7f:b0d6] has quit [Read error: Connection reset by peer] 19:22 -!- musdom [~Thunderbi@202.184.0.102] has joined #bitcoin-core-pr-reviews 19:28 -!- samuel-pedraza [47edaff3@gateway/web/cgi-irc/kiwiirc.com/ip.71.237.175.243] has joined #bitcoin-core-pr-reviews 19:30 -!- belcher [~belcher@unaffiliated/belcher] has quit [Ping timeout: 258 seconds] 19:35 -!- belcher [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 19:45 -!- prayank [~andr0irc@2409:4053:2e80:6b21:9cd7:456a:ee7f:b0d6] has joined #bitcoin-core-pr-reviews 20:07 -!- ja [janus@anubis.0x90.dk] has left #bitcoin-core-pr-reviews [] 20:48 -!- musdom [~Thunderbi@202.184.0.102] has quit [Ping timeout: 240 seconds] 20:59 -!- belcher [~belcher@unaffiliated/belcher] has quit [Ping timeout: 264 seconds] 20:59 -!- belcher [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 21:16 -!- da39a3ee5e6b4b0d [~da39a3ee5@mx-ll-171.5.29-209.dynamic.3bb.co.th] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…] 21:23 -!- da39a3ee5e6b4b0d [~da39a3ee5@mx-ll-171.5.29-209.dynamic.3bb.co.th] has joined #bitcoin-core-pr-reviews 21:26 -!- da39a3ee5e6b4b0d [~da39a3ee5@mx-ll-171.5.29-209.dynamic.3bb.co.th] has quit [Client Quit] 21:48 -!- shesek [~shesek@unaffiliated/shesek] has quit [Remote host closed the connection] 22:10 -!- jkczyz [sid419941@gateway/web/irccloud.com/x-ibaprrhpcssnfiuj] has quit [Read error: Connection reset by peer] 22:10 -!- jkczyz [sid419941@gateway/web/irccloud.com/x-iecubrfqgzplxinh] has joined #bitcoin-core-pr-reviews 22:11 -!- dburkett [sid411344@gateway/web/irccloud.com/x-jsncuetqmrckjftg] has quit [Read error: Connection reset by peer] 22:11 -!- moneyball [sid299869@gateway/web/irccloud.com/x-wjkdiocnrkijgqvm] has quit [Ping timeout: 268 seconds] 22:11 -!- dburkett [sid411344@gateway/web/irccloud.com/x-tkxwhnhavbsakwkd] has joined #bitcoin-core-pr-reviews 22:11 -!- avril [wha@unaffiliated/avril] has quit [Quit: later skater] 22:11 -!- moneyball [sid299869@gateway/web/irccloud.com/x-rochyfnjwpmypxlr] has joined #bitcoin-core-pr-reviews 22:11 -!- avril [wha@chick.sex0r.net] has joined #bitcoin-core-pr-reviews 22:11 -!- avril [wha@chick.sex0r.net] has quit [Changing host] 22:11 -!- avril [wha@unaffiliated/avril] has joined #bitcoin-core-pr-reviews 22:28 -!- da39a3ee5e6b4b0d [~da39a3ee5@171.5.29.209] has joined #bitcoin-core-pr-reviews 23:28 -!- gleb [~gleb@178.150.137.228] has quit [Ping timeout: 260 seconds] 23:33 -!- samuel-pedraza [47edaff3@gateway/web/cgi-irc/kiwiirc.com/ip.71.237.175.243] has quit [Ping timeout: 240 seconds] 23:41 -!- musdom [~Thunderbi@202.184.0.102] has joined #bitcoin-core-pr-reviews 23:45 -!- musdom [~Thunderbi@202.184.0.102] has quit [Ping timeout: 256 seconds]