--- Day changed Wed Mar 25 2020 00:21 -!- seven_ [~seven@2a00:ee2:410c:1300:d50e:72dc:2b91:966e] has joined #bitcoin-core-pr-reviews 00:25 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has quit [Remote host closed the connection] 00:26 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has joined #bitcoin-core-pr-reviews 00:30 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has quit [Ping timeout: 246 seconds] 00:37 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has joined #bitcoin-core-pr-reviews 00:40 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 00:43 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 00:43 -!- vasild_ is now known as vasild 00:53 -!- felixfoertsch23 [~felixfoer@i6DFA620C.versanet.de] has quit [Ping timeout: 264 seconds] 01:10 -!- felixfoertsch [~felixfoer@2001:16b8:5041:2a00:ad41:93ff:f87d:6ee7] has joined #bitcoin-core-pr-reviews 02:15 -!- felixfoertsch [~felixfoer@2001:16b8:5041:2a00:ad41:93ff:f87d:6ee7] has quit [Ping timeout: 246 seconds] 02:15 -!- felixfoertsch [~felixfoer@i577B0BE6.versanet.de] has joined #bitcoin-core-pr-reviews 02:20 -!- felixfoertsch [~felixfoer@i577B0BE6.versanet.de] has quit [Ping timeout: 256 seconds] 02:24 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has quit [Remote host closed the connection] 02:25 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has joined #bitcoin-core-pr-reviews 02:29 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has quit [Ping timeout: 256 seconds] 02:32 -!- felixfoertsch [~felixfoer@92.117.60.3] has joined #bitcoin-core-pr-reviews 03:19 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has joined #bitcoin-core-pr-reviews 03:25 -!- felixfoertsch23 [~felixfoer@200116b840953601ad4193fff87d6ee7.dip.versatel-1u1.de] has joined #bitcoin-core-pr-reviews 03:26 -!- felixfoertsch [~felixfoer@92.117.60.3] has quit [Ping timeout: 240 seconds] 03:42 -!- emilengler [~emilengle@stratum0/entity/emilengler] has joined #bitcoin-core-pr-reviews 04:03 -!- Elvie68Eichmann [~Elvie68Ei@ns334669.ip-5-196-64.eu] has joined #bitcoin-core-pr-reviews 04:09 -!- Elvie68Eichmann [~Elvie68Ei@ns334669.ip-5-196-64.eu] has quit [Ping timeout: 264 seconds] 04:40 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has quit [Remote host closed the connection] 05:32 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Remote host closed the connection] 05:33 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined #bitcoin-core-pr-reviews 05:43 -!- slivera [~slivera@217.138.204.74] has quit [Remote host closed the connection] 06:05 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has joined #bitcoin-core-pr-reviews 07:32 -!- apaval [~apaval@68.183.88.61] has quit [Remote host closed the connection] 07:33 -!- apaval [~apaval@68.183.88.61] has joined #bitcoin-core-pr-reviews 07:43 -!- nothingmuch [~nothingmu@unaffiliated/nothingmuch] has quit [Quit: ZNC - http://znc.in] 08:03 -!- Talkless [~Talkless@hst-227-49.splius.lt] has joined #bitcoin-core-pr-reviews 08:15 -!- vindard [~vindard@190.83.165.233] has quit [Ping timeout: 264 seconds] 08:15 -!- vindard [~vindard@200.7.91.60] has joined #bitcoin-core-pr-reviews 08:19 -!- apaval [~apaval@68.183.88.61] has quit [Remote host closed the connection] 08:19 -!- apaval [~apaval@68.183.88.61] has joined #bitcoin-core-pr-reviews 09:05 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has joined #bitcoin-core-pr-reviews 09:23 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 09:40 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Ping timeout: 240 seconds] 09:44 < jnewbery> We'll get started in about 15 minutes. Notes and questions: https://bitcoincore.reviews/18238.html 09:46 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:a860:d354:f894:e7e6] has joined #bitcoin-core-pr-reviews 09:47 -!- gchain [gustafmatr@gateway/shell/matrix.org/x-gnzuchyiruemqunc] has quit [Ping timeout: 256 seconds] 09:50 -!- vindard [~vindard@200.7.91.60] has quit [Read error: Connection reset by peer] 09:50 -!- gchain [gustafmatr@gateway/shell/matrix.org/x-xmumgnyxkogydjta] has joined #bitcoin-core-pr-reviews 09:51 -!- vindard [~vindard@190.83.165.233] has joined #bitcoin-core-pr-reviews 09:53 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 09:57 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:a860:d354:f894:e7e6] has quit [Quit: Sleep mode] 09:58 -!- r251d [~r251d@162-196-59-192.lightspeed.irvnca.sbcglobal.net] has joined #bitcoin-core-pr-reviews 09:59 -!- gleb [~gleb@cpe-67-244-100-77.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews 10:00 < amiti> #startmeeting 10:00 < jnewbery> hi 10:00 < gleb> hi! 10:00 < jkczyz> hi 10:00 -!- lightlike [~lightlike@p200300C7EF13390088008C3F273EB117.dip0.t-ipconnect.de] has joined #bitcoin-core-pr-reviews 10:00 < felixweis> hi 10:00 < lightlike> hi 10:00 < amiti> hi everyone! welcome to this weeks edition of PR review club 10:00 < nehan_> hi 10:01 < sipa> hi 10:01 < amiti> this weeks PR is ... quite a doozy! 10:01 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:a860:d354:f894:e7e6] has joined #bitcoin-core-pr-reviews 10:01 -!- gloria56 [49fcfb03@c-73-252-251-3.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 10:01 < r251d> hi 10:01 < michaelfolkson> hi 10:01 < amiti> who has gotten a chance to review the pr? y/n 10:02 < gleb> y, I was ready to ACK module little things I asked about. 10:02 < jnewbery> 0.7 y 10:02 < lightlike> y 10:02 < jkczyz> y (briefly) 10:03 < michaelfolkson> y (also briefly) 10:03 < jonatack> hi 10:03 < nehan_> y but also briefly 10:03 < amiti> what are initial impressions? 10:03 < amiti> I saw some of you left concept ACKs on the PR 10:04 -!- ariard [~ariard@167.99.46.220] has joined #bitcoin-core-pr-reviews 10:04 < nehan_> i'm a bit unclear on why this is important 10:05 < amiti> can anyone shed light on some context that makes this PR relevant / helpful? 10:05 < jnewbery> Concept ACK! I think I prefer the approach over #15505, which adds a counter to net 10:05 < lightlike> I like it better than the approach in the predecessor because it is less invasive. 10:06 < nehan_> what is the downside of waiting until the txn request times out to request from another peer? 10:06 < andrewtoth> hi 10:06 < jkczyz> Generally like the idea but implementation-wise would prefer if m_tx_announced did not have a dual use, if possible 10:07 < michaelfolkson> nehan: You'd want to get the transaction as fast as possible for performance reasons I'd guess. Especially if you're a miner 10:07 < amiti> nehan_: one explicit downside comes if we implement the changes in PR #17303, which is trying to remove mapRelay 10:07 < sipa> nehan_: are you asking why avoiding a 2 minute delay in tx propagation is desirable? 10:07 < gleb> Let's put it this way. How long would we wait before this PR even if NOTFOUND is received? 10:08 < nehan_> michaelfolk: amiti: sipa: thanks! 10:08 < nehan_> sipa: yes, in the cases this PR affects. perhaps this is obvious to everyone but me! 10:08 < jkczyz> nehan_: also some privacy benefits from 17303 which this PR is a prerequisite for 10:08 < lightlike> not wasting time seems a good idea in itself - I didn't completely understand the DOS connection with PR#17303, can someone explain this a bit more? 10:08 < sipa> if we were talking about a few seconds, this would probably be harmless, as our normal tx propagation mechanisms has delays of that order 10:09 < sipa> but minutes beings it in the same order of magnitude as blocks, so it very well may interfere with block propagation too (as bip 152 relay relies on having good tx propagation) 10:10 < jnewbery> lightlike: we currently keep a mapRelay of transactions we've recently announced. Even if a transaction is subsequently removed from the mempool (for expiry or some other reason), it's still available to be fetched by peers we've announced it to 10:10 < gleb> It's not super-clear it's 2 minutes though. I find the existing comment confusing. 10:10 < amiti> lightlike: my understanding is... mapRelay has a different logic for transactions it keeps than the mempool logic. so, in the case of a mempool with lots of tx churn, mapRelay might keep the txn for longer. if we remove it, then nodes with small mempools could more frequently be responding with `NOTFOUND`, and tying up the connection for upto the 2 minute period even though behaving honestly 10:11 < gleb> The comment says: "request unlike it's inflight from someone", and current code ALREADY cleans inflight when NOTFOUND is sent. 10:11 < jnewbery> 17303 proposed to remove this mapRelay. That means if the tx is removed from the mempool between the INV announcement and GETDATA request, then the node announcing it would have prevented the receiving node from fetching it from elsewhere for a minute 10:11 < gleb> While that comment says that currently, it's implicit, and the logic actually doesn't look at "inflight" variable 10:12 < sipa> amiti: that's my my understanding too 10:12 < lightlike> ah ok, thanks jnewbery, amiti 10:12 < sipa> mapRelay tries to implement a policy of "we promise to keep everything we've relayed recently available for fetching, regardless of what the mempool does" 10:13 < amiti> also, there's a lot of nuanced logic of how nodes are queued via the `process_time` that I'm just starting to wrap my head around... I'm interested in better familiarizing myself the implementation to understand different attack possibilities 10:13 < sipa> mapRelay predates the limited-size mempool though; i think its original purpose was making sure txn could be downloaded still even when a block confirmed them in the mean time 10:14 < amiti> so, can someone summarize this PR? how does it speed up retries? 10:14 < jnewbery> mapRelay existed in the initial git commit 10:15 < ariard> it speeds up retries by a) instead of relaying on GETDDATA_TX_INTERVAL for a non-received transaction and b) assuming we receive a NOTFOUND, we're gong to retry download with other peers 10:15 -!- felixfoertsch23 [~felixfoer@200116b840953601ad4193fff87d6ee7.dip.versatel-1u1.de] has quit [Quit: ZNC 1.7.4 - https://znc.in] 10:16 -!- felixfoertsch [~felixfoer@200116b840953601ad4193fff87d6ee7.dip.versatel-1u1.de] has joined #bitcoin-core-pr-reviews 10:16 < andrewtoth> ajtowns' header comment is a good summary: Anytime we see a NOTFOUND in response to a request for a tx, look through 10:16 < andrewtoth> each of our peers for anyone else who announced the tx, find one who 10:16 < andrewtoth> doesn't already have its inflight tx count maxed out, and of those, 10:16 < andrewtoth> make the one who'd look at it first, look at it asap. 10:17 < jonatack> nehan_: thanks for your question; i reckon a non-trivial number of observers wondered the same or appreciated confirmation on the answers 10:17 < michaelfolkson> Yup definitely jonatack nehan_ 10:18 < sipa> andrewtoth: so effectively, receiving a NOTFOUND makes us skip the timeout 10:18 < amiti> ariard, andrewtoth, sipa: exactly 10:18 < amiti> andrewtoth: you already mentioned this / OP mentions, but can someone make extra clear- how does this PR identify who to ask next for the txn? 10:19 < jnewbery> I was confused about how the 'asap' part is implemented. Comment here: https://github.com/bitcoin/bitcoin/pull/18238#discussion_r397925056 10:19 < amiti> bonus points: how is this different than the previous attempt at this functionality? PR 15505 10:19 < jnewbery> it brings forward the m_tx_process_time, but doesn't change the g_already_asked_for time so I don't know how a new GETDATA request is triggered 10:19 < nehan_> jonatack: you're welcome! i get why this is better than #15505 and why one might want something like it for #17303 but i'm not yet convinced #17303 is that important. and "always reduce tx propagation times" doesn't seem like a good metric to use without considering 1) when things are actually slowed down and 2) additional code complexity. 10:20 < nehan_> nehan_: but i don't want to distract from actually reviewing the PR! 10:20 < michaelfolkson> We're requesting the tx from a peer who announced the tx 10:21 < michaelfolkson> Oh that's the same PR 15505 10:21 < lightlike> in 15505, we would retry only from outbound peers, and not look at the originially scheduled process time but try to request from those who had announced the TX (whenever that was) with the same probability. 10:22 < gleb> jnewbery: I think g_already_asked_for is time of the previous request, and we only care it's not something in future, so should be fine? Why would it ever be in future and prevent us from triggering? 10:22 < amiti> nehan_: I think these questions are great! questioning the fundamental WHY of a PR is a great approach :) 10:22 < sipa> nehan_: certainly a blanket "reduce tx propagation times" would also be terrible for privacy; it's just that the delays involved here are potentially far higher than the time delays we normally target for privacy protection 10:22 < sipa> (which is in the order of seconds) 10:22 < nehan_> sipa: but the original justification for this from sdaftuar was not reducing delays, it was not turning small mempool nodes into dos'ers 10:23 < nehan_> https://github.com/bitcoin/bitcoin/pull/17303#issuecomment-547589047 10:23 < gleb> jnewbery: now that I look at 'minus GETDATA_TX_INTERVAL', I think you're right... 10:24 < ariard> gleb: because it's previous request time - GETDATA_TX_INTERVAL, so this would prevent new retry if NOTFOUND sequence happens less than 1 min ago 10:24 < jnewbery> gleb: because in SendMessages(), we'll only send a GETDATA for transactions that were last requested over a minute ago: https://github.com/bitcoin/bitcoin/blob/60a39a96fc04c9bde98f0a55c0deb2a0b3fc25a7/src/net_processing.cpp#L4048 10:24 < ariard> there is 2 times check one against tx_process_time and on against g_already_asked_for 10:24 < gleb> yes, good catch! This is a good justification to have a test there :) I guess me and aj just missed this part. 10:25 < sipa> nehan_: right; the problem (going from memory here, haven't checked the comments lately) is that our desire to get rid of mapRelay would trigger requested transactions not being found in honest situations a lot more, effectively hurting tx propagation (by delaying things in the order of minutes) 10:26 < amiti> ok, moving forward... can someone explain in human words what information `TxDownloadState` keeps track of? 10:26 < sipa> what i meant is that there is nuance: short delays are not a problem, and (in some settings) even desirable for privacy - but very big ones actually hurt propagation 10:26 < amiti> and how it gets changed by this PR? 10:28 < jnewbery> nehan_: perhaps more context on 17303 is that mapRelay doesn't have great memory bounds, so we'd like to remove it. Suhas tried to use it for improving tx relay privacy in https://github.com/bitcoin/bitcoin/pull/14220, but had to abandon that approach because it would have made memory exhaustion possible 10:28 < nehan_> sipa: so "node with small mempool being a dos'er" actually means "node forcing me to wait a long time before getting this txn"? i see. i think i misunderstood and thought the small mempool nodes were actually sending more messages in some way. 10:28 < nehan_> jnewbery: thanks! 10:29 < gleb> nehan_: yeah, sipa's explanation couple messages ago also worked much better for the than that I read somewhere before. 10:29 < sipa> nehan_: that's my understanding 10:29 < amiti> I spent a bunch of time trying to understand this struct & how its used. its fundamental to the coordination of requesting txns from different peers, so is central to this implementation 10:29 < amiti> nehan_, sipa: thats my understanding too 10:30 < michaelfolkson> amiti: So it is a struct to manage the timing of when to download the announced transactions 10:30 < ariard> amiti: a per-peer state machine to manage tx relay bundled with timers ? 10:30 < jnewbery> amiti: TxDownloadState (like all things) started small, and then got more bloated and had its members start overlapping in meaning 10:30 < amiti> cool! that all sounds right 10:31 < amiti> so what is this PR changing? 10:31 < jnewbery> ariard's answer is better than mine 10:31 < gleb> Yeah, that overlapping is not something I saw much in software before, but I have an intuition that it might be the best way in this particular case. 10:31 < amiti> gleb: can you explain the overlapping? 10:31 < gleb> I have a big desire to get rid of the overlapping of maps, but i couldn't find a way to do it well. 10:31 < amiti> are you talking about the one introduced in this PR? 10:31 < gleb> ye 10:31 < amiti> can you make it explicit 10:32 < gleb> Well, we have tx->time, and time->tx maps... Let me pull it up again 10:32 < amiti> yup exactly, `m_tx_process_time` is a multimap of time -> tx 10:33 < gleb> m_tx_announced, and m_tx_in_flight often go together and we do similar things to them, for example. 10:33 < jnewbery> I think there are three states that a tx can be in: (i) the peer has announced it and we haven't requested it from that peer; (ii) the peer has announced it, we've requested it and are waiting for delivery; (iii) the peer announced it, we requested it, and it timed out 10:33 < amiti> and this PR is turning `m_tx_announced` into a map of tx -> time 10:34 < amiti> ah, and for clarity, when I say time, I mean `process_time` 10:34 < ariard> yes but here introducing time tracking in m_tx_announced is just now to select a peer for retry 10:35 < sipa> it may be useful to write a consistency-check function for all these data structures, and have a command-line option that causes it to be run occasionally (similar to -checkmempool) 10:35 < sipa> unless they can be simplified of course, which is always preferable 10:35 < gleb> yeah, good idea. 10:35 < jnewbery> sipa: or turn this into a class and have an interface to that class rather than code all over the place reaching in and manipulating the members 10:35 < sipa> jnewbery: yup 10:36 < sipa> absolutely 10:36 < amiti> sipa: oh interesting. I'm not familiar. so the idea is a user can invoke a cli option that kicks off periodic consistency checks? 10:36 < jnewbery> amiti: see -checkmempool 10:37 < jkczyz> I tend to agree that making the code explicit (and hopefully simpler) is preferable over dependencies between fields and explanatory comments 10:37 < amiti> I'm definitely in favor of simplifying the code. there's a lot of implicit expectations that require additional code to support when writing features, but are a bug if they occur 10:37 < sipa> amiti: see CTxMempool::check in particular 10:37 < jnewbery> (and nCheckFrequency in txmempool.cpp|h) 10:38 < sipa> it's useful too for reviewers, as it spells out explicitly what expectations there are 10:38 < sipa> though i wouldn't call that function in particular very readable... 10:38 < amiti> for example, `m_tx_process_time` is a multimap, so can technically be a many to many, but txns should only be queued for one time in the future (per node), so this really should be a many to one 10:38 < amiti> (👆🏽 fact check?) 10:39 < sipa> that sounds easy to verify, if true 10:39 -!- glowang51 [49bd0c26@c-73-189-12-38.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 10:39 < jnewbery> amiti: that should be true. If a tx appears more than once in m_tx_process_time, then there's a logic bug somewhere 10:40 < jonatack> for anyone looking, -checkmempool is a "hidden" debug option 10:40 < jonatack> bitcoind -help-debug | grep -A2 checkmempool 10:40 < gleb> Anyway, this probably should be a separate PR? There are probably a bunch of other consistency checks we may do. I would say the goal here should be to minimize cross-var dependencies. 10:40 < sipa> gleb: agreed 10:41 < amiti> ya, good point 10:41 < amiti> ok lets jump to another question about this PR 10:41 < amiti> what are potential issues around this approach? 10:42 < ariard> jnewbery: about you're other comment on a swarm of dishonest inbounds delaying tx relay, that's assume there isn't at least a honest peer with a announcement time better 10:42 < ariard> than malicisus ones ? 10:42 < jnewbery> I think the class would be a container or {txid, state (i, ii or iii above), timestamp} with indexes into those objects 10:42 < amiti> gleb: you and aj had some discussion about a DoS vector. can anybody (gleb or otherwise) explain the concern here? 10:43 < gleb> ariard: can you send a link to the comment? I can't find anything by searching for "swarm" lol 10:43 < ariard> gleb: https://github.com/bitcoin/bitcoin/pull/18238#discussion_r397928486 10:43 < ariard> isn't only retrying with outbounds would be better to avoid this kind of attack or more severe ones ? 10:44 < jnewbery> ariard: the honest peer's time to retry gets reset here: https://github.com/bitcoin/bitcoin/blob/60a39a96fc04c9bde98f0a55c0deb2a0b3fc25a7/src/net_processing.cpp#L4063 10:44 < gleb> I think nullifying helps with not juggling? 10:44 < gleb> This comparison to chrono::zero helps to make sure we don't force-request from the already NOTFOUNDed peer. 10:44 < jnewbery> so as long as the adversary is continuing to juggle NOTFOUNDs/INVs, then he might be able to prevent the victim ever requesting the tx from the honest node 10:45 < amiti> ok time check- 15 minutes left 10:45 < ariard> jnewbery: okay and by constantly re-inving attack, he is sure to always trump honest peers 10:46 < jnewbery> gleb: comment is here: https://github.com/bitcoin/bitcoin/pull/18238#discussion_r397928486. The m_tx_announced is not nulled for peers that send NOTFOUND 10:46 < amiti> just want to call out that this has turned into a fairly advanced / deep divey session, and if anybody is lurking and unclear on any fundamentals, I encourage speaking up and ask your questions. I bet you're not the only one wondering :) 10:46 < jnewbery> amiti: +1 10:47 < ariard> okay but what not requesting only from outbounds? As initial approach in 15505 10:48 < michaelfolkson> I'll throw a high level question into the mix. Is it worth trying to understand more about your peers e.g. whether they are pruned/unpruned, their mempool policy etc. The approach as I understand it is to just treat all your peers as if you know nothing about them 10:48 < jonatack> Great stuff, amiti. I think this discussion is going to be very helpful for reviewing the PR. 10:48 < ariard> I expect outbounds to have mempool of a consistant-size, given they already offer bandwidth/ports to the network 10:48 < gleb> jnewbery: i think you are right. I was thinking about the same issue, but thought it was handled for wrong reason i guess. 10:48 < fjahr> hi 10:48 < amiti> hi fjahr :) 10:48 < jnewbery> ariard: I guess in RetryProcessTx() we could change the 'best' heuristic to always prefer outbounds? 10:50 -!- glowang51 [49bd0c26@c-73-189-12-38.hsd1.ca.comcast.net] has left #bitcoin-core-pr-reviews [] 10:50 < jnewbery> michaelfolkson: pruned/unpruned doesn't make any difference here (we're talking about GETDATA requests for unconfirmed txs, not blocks) 10:50 < ariard> jnewbery: yes that's my point, but is doing so would achieve goal of PR, there is still worst-case hitting 1min window in case of really bad-connected outbounds? 10:50 < amiti> RE logic of the outbound vs the inbound, isn't there already a preference built in to when you initially schedule the process_time, and by finding the best time and bumping up, you'd be honoring the same order? 10:50 < amiti> am I missing something? 10:50 < jnewbery> ariard: no, in the case that no outbound peer has announced the tx, the best would be the best inbound 10:51 < jnewbery> amiti: see aj's comment here: https://github.com/bitcoin/bitcoin/pull/18238#discussion_r396884515 10:51 < ariard> jnewbery: so iterate first on outbound, then inbound, but a malicious inbound can still make you juggle indefinetely? 10:51 < gleb> amiti: yeah, but it's possible (probably very early in the overall relay) for inbounds to occupy fastest slots, and I guess there's worrying about that scenario to be exploited further. 10:51 < ariard> (likely the announced tx is a malicious one) 10:52 < jnewbery> because the NOTFOUND can arrive at any time, we might have rescheduled our outbounds to be after our inbounds here: https://github.com/bitcoin/bitcoin/blob/60a39a96fc04c9bde98f0a55c0deb2a0b3fc25a7/src/net_processing.cpp#L4063 10:53 < amiti> ah. right. 10:53 < jnewbery> ariard: I think by null'ing rather than deleting entries from m_tx_announced here: https://github.com/bitcoin/bitcoin/pull/18238#discussion_r397928486, the malicious peer can't juggle NOTFOUND/INVs 10:54 < gleb> We should absolutely prevent juggling. Once that is done, it's worth discussing other threats (which I think are probably minor) 10:54 < michaelfolkson> jnewbery: A NOTFOUND can be response for a transaction request from a block / confirmed transaction, just not related to this particular PR. Got it 10:54 < gleb> Like, an attacker would have to come really early in the relay, and the damage is InvBlock for a minute or two at most. I think that's desirable requirement. 10:55 < gleb> (similar to what is already possible) 10:55 -!- glowang100 [49bd0c26@c-73-189-12-38.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 10:57 < gleb> this PR turned out to be much more complicated than I anticipated, especially after all the reviews, triggered by this review club too :) 10:57 < amiti> 3 minutes left ! 10:57 < amiti> yeah its suuuuper nuanced! effective tx download is HARD 10:58 < jkczyz> +1 IMHO, the PR exhibits why striving to reduce complexity (or hiding it behind an interface when not possible) is so important. Even dependencies between fields of a small struct can have an enormous impact on code understanding and review velocity, not to mention the potential for introducing bugs 10:58 < michaelfolkson> Still a bit confused. Why are there no "announcements" for confirmed transactions, transactions already in a block? 10:58 < jnewbery> jkczyz: absoluely agree! 10:58 < amiti> jkczyz: strong agree 10:58 < jonatack> jkczyz: ^ this 10:58 < sipa> michaelfolkson: the block is the announcement? 10:58 -!- glowang100 [49bd0c26@c-73-189-12-38.hsd1.ca.comcast.net] has quit [Remote host closed the connection] 10:59 < michaelfolkson> But a a node could be really behind and announce a transaction that everyone already knows about and has been included in a block? 10:59 < jnewbery> michaelfolkson: transaction relay is for unconfirmed transactions. Confirmed transactions are included in the BLOCK message 10:59 < nehan_> thanks amiti! 11:00 < jnewbery> michaelfolkson: tranaction wouldn't be valid for nodes that are caught up because its inputs have been spent 11:00 < amiti> alright folks! thats time 11:00 < ariard> michaelfolkson: see g_recent_confirmed_transactions, to avoid reqquesting tx already confirmed 11:00 < amiti> #endmeeting 11:00 -!- gloria56 [49fcfb03@c-73-252-251-3.hsd1.ca.comcast.net] has quit [Remote host closed the connection] 11:00 < amiti> thank you all :) 11:00 < jnewbery> thanks amiti. That was fun! 11:00 < lightlike> thanks! 11:00 < michaelfolkson> Cool, thanks fro the answers 11:00 < michaelfolkson> *for 11:00 < amiti> that was a fun discussion. I have a LOT of questions to go investigate :) 11:00 < michaelfolkson> And thanks amiti. Great hosting 11:01 < sipa> thanks amiti! 11:01 < jonatack> thanks amiti and everyone! 11:01 < jkczyz> amiti: thanks! Learning a lot 11:01 -!- r251d [~r251d@162-196-59-192.lightspeed.irvnca.sbcglobal.net] has quit [Quit: r251d] 11:01 < felixweis> thanks! 11:01 < emzy> Thanks amiti and everyone! 11:01 -!- lightlike [~lightlike@p200300C7EF13390088008C3F273EB117.dip0.t-ipconnect.de] has quit [Quit: Leaving] 11:03 < sipa> [A 11:04 < andrewtoth> thanks amiti and everyone! 11:11 < jnewbery> meeting log is posted: https://bitcoincore.reviews/18238.html 11:18 < jnewbery> next week's notes are also posted: https://bitcoincore.reviews/18401.html we'll be talking about a couple more taproot preparation PRs 11:28 -!- molly [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 11:30 -!- mol [~molly@unaffiliated/molly] has quit [Ping timeout: 256 seconds] 11:37 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:a860:d354:f894:e7e6] has quit [Quit: Sleep mode] 11:40 -!- molly [~molly@unaffiliated/molly] has quit [Ping timeout: 256 seconds] 11:44 -!- mol [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 11:51 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:a860:d354:f894:e7e6] has joined #bitcoin-core-pr-reviews 11:52 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:a860:d354:f894:e7e6] has quit [Client Quit] 12:28 -!- Talkless [~Talkless@hst-227-49.splius.lt] has quit [Quit: Konversation terminated!] 12:40 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 12:44 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 12:44 -!- vasild_ is now known as vasild 12:48 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 264 seconds] 12:52 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #bitcoin-core-pr-reviews 13:02 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has quit [Remote host closed the connection] 13:49 -!- masterdonx2 [~masterdon@103.108.117.132] has joined #bitcoin-core-pr-reviews 13:49 -!- MasterdonX [~masterdon@103.108.117.132] has quit [Ping timeout: 250 seconds] 13:56 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 13:58 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Ping timeout: 240 seconds] 14:17 -!- slivera [~slivera@217.138.204.73] has joined #bitcoin-core-pr-reviews 14:40 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 14:52 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Quit: jb55] 15:22 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Ping timeout: 260 seconds] 15:27 -!- fengling [~qinfengli@45.32.53.207] has quit [Ping timeout: 256 seconds] 15:29 -!- fengling [~qinfengli@45.32.53.207] has joined #bitcoin-core-pr-reviews 15:31 -!- jonatack [~jon@37.172.90.77] has joined #bitcoin-core-pr-reviews 16:00 -!- apaval [~apaval@68.183.88.61] has quit [Remote host closed the connection] 16:00 -!- apaval [~apaval@68.183.88.61] has joined #bitcoin-core-pr-reviews 16:02 -!- emilengler [~emilengle@stratum0/entity/emilengler] has quit [Remote host closed the connection] 16:02 -!- emilengler [~emilengle@stratum0/entity/emilengler] has joined #bitcoin-core-pr-reviews 16:30 < aj> fwiw, i thought briefly about encapsulating more of the logic into the class, but quickly gave up because it was a bit intrusive; so i think feedback that it'd be less confusing with better encapuslation is helpful 16:42 < jonatack> 👍 16:51 -!- gleb [~gleb@cpe-67-244-100-77.nyc.res.rr.com] has quit [Read error: Connection reset by peer] 16:51 -!- gleb [~gleb@cpe-67-244-100-77.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews 16:56 -!- gleb [~gleb@cpe-67-244-100-77.nyc.res.rr.com] has quit [Ping timeout: 256 seconds] 16:57 -!- gleb [~gleb@cpe-67-244-100-77.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews 17:04 -!- gleb1 [~gleb@cpe-67-244-100-77.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews 17:06 -!- gleb [~gleb@cpe-67-244-100-77.nyc.res.rr.com] has quit [Ping timeout: 250 seconds] 17:07 -!- gleb [~gleb@cpe-67-244-100-77.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews 17:09 -!- gleb1 [~gleb@cpe-67-244-100-77.nyc.res.rr.com] has quit [Ping timeout: 256 seconds] 17:32 -!- jonatack_ [~jon@37.167.7.26] has joined #bitcoin-core-pr-reviews 17:34 -!- jonatack [~jon@37.172.90.77] has quit [Ping timeout: 240 seconds] 18:01 -!- slivera_ [~slivera@181.215.46.112] has joined #bitcoin-core-pr-reviews 18:04 -!- slivera [~slivera@217.138.204.73] has quit [Ping timeout: 240 seconds] 18:37 -!- slivera__ [~slivera@217.138.204.72] has joined #bitcoin-core-pr-reviews 18:39 -!- slivera_ [~slivera@181.215.46.112] has quit [Ping timeout: 240 seconds] 18:39 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Remote host closed the connection] 18:39 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 18:54 -!- seddd [~user@gateway/tor-sasl/seddd] has joined #bitcoin-core-pr-reviews 18:54 -!- apaval [~apaval@68.183.88.61] has quit [Remote host closed the connection] 18:54 -!- apaval [~apaval@68.183.88.61] has joined #bitcoin-core-pr-reviews 19:08 -!- emilengler [~emilengle@stratum0/entity/emilengler] has quit [Quit: Leaving] 19:52 -!- slivera__ [~slivera@217.138.204.72] has quit [Ping timeout: 256 seconds] 20:32 -!- felixfoertsch [~felixfoer@200116b840953601ad4193fff87d6ee7.dip.versatel-1u1.de] has quit [Ping timeout: 272 seconds] 20:32 -!- felixfoertsch23 [~felixfoer@i577B008B.versanet.de] has joined #bitcoin-core-pr-reviews 21:40 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection] 21:40 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Remote host closed the connection] 21:40 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 21:40 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 22:49 -!- apaval [~apaval@68.183.88.61] has quit [Ping timeout: 240 seconds] 22:55 -!- Talkless [~Talkless@hst-227-49.splius.lt] has joined #bitcoin-core-pr-reviews 22:56 -!- Talkless [~Talkless@hst-227-49.splius.lt] has quit [Client Quit] 23:18 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Quit: ZNC - http://znc.sourceforge.net] 23:20 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #bitcoin-core-pr-reviews 23:40 -!- apaval [~apaval@68.183.88.61] has joined #bitcoin-core-pr-reviews 23:43 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has joined #bitcoin-core-pr-reviews