--- Day changed Wed Nov 13 2019 00:16 -!- Moller40 [~mr@82.103.128.151] has quit [Ping timeout: 240 seconds] 00:18 -!- Moller40 [~mr@82.103.130.178] has joined #bitcoin-core-pr-reviews 01:22 -!- Moller40 [~mr@82.103.130.178] has quit [Ping timeout: 265 seconds] 01:24 -!- Moller40_ [~mr@82.103.128.151] has joined #bitcoin-core-pr-reviews 01:59 -!- openoms [~quassel@185.174.156.219] has joined #bitcoin-core-pr-reviews 02:17 -!- Moller40_ [~mr@82.103.128.151] has quit [Quit: -a- Connection Timed Out] 02:18 -!- Moller40 [~mr@82.103.128.151] has joined #bitcoin-core-pr-reviews 02:20 -!- Moller40 [~mr@82.103.128.151] has quit [Client Quit] 02:20 -!- emilengler [~emilengle@unaffiliated/emilengler] has joined #bitcoin-core-pr-reviews 02:29 -!- Zenton [~user@unaffiliated/vicenteh] has quit [Remote host closed the connection] 02:32 -!- diogosergio [~diogoserg@212.36.34.126] has joined #bitcoin-core-pr-reviews 02:53 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has quit [Ping timeout: 276 seconds] 02:54 -!- seven__ [~seven@BSN-77-101-62.static.siol.net] has joined #bitcoin-core-pr-reviews 02:56 -!- seven_ [~seven@BSN-77-101-62.static.siol.net] has quit [Ping timeout: 276 seconds] 02:59 -!- Zenton [~user@unaffiliated/vicenteh] has joined #bitcoin-core-pr-reviews 04:05 -!- diogosergio [~diogoserg@212.36.34.126] has quit [Ping timeout: 240 seconds] 04:21 -!- jonatack [~jon@213.152.161.74] has joined #bitcoin-core-pr-reviews 04:40 -!- diogosergio [~diogoserg@212.36.34.126] has joined #bitcoin-core-pr-reviews 05:06 -!- rabidus [~rabidus@85-23-137-40.bb.dnainternet.fi] has joined #bitcoin-core-pr-reviews 05:13 -!- jonatack [~jon@213.152.161.74] has quit [Quit: jonatack] 05:16 -!- jasonzhouu [~jason@2409:8a28:c17:26a0:33e0:1d4d:1380:ee42] has joined #bitcoin-core-pr-reviews 05:48 < MarcoFalke> I think the channel topic is incorrect and today's meeting is at 18:00 UTC . Convert to your local timezone here: https://www.wolframalpha.com/input/?i=18%3A00+UTC 05:49 < MarcoFalke> jnewbery: Edit channel topic? ^ 06:11 < aj> huh? it used to be at 1900 UTC and not change for lack of daylight savings, i thought? 06:12 < aj> oh, wrong channel haha 06:13 < aj> the website says 18:00 utc 06:26 -!- shesek [~shesek@unaffiliated/shesek] has quit [Read error: Connection reset by peer] 06:26 -!- shesek [~shesek@185.3.145.80] has joined #bitcoin-core-pr-reviews 06:26 -!- shesek [~shesek@185.3.145.80] has quit [Changing host] 06:26 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews 06:40 -!- jonatack [~jon@37.164.55.120] has joined #bitcoin-core-pr-reviews 06:57 < pinheadmz> I know I'm early, but I'm playing with this week's PR and I have some general Q's before I forget... 06:57 < pinheadmz> 1 why is the loglevel for the testframework not `debug` (or even `spam`) by default? Today's PR modifies a test where I found the `debug` level output especially helpful 06:58 < pinheadmz> 2 the python tests often have random elements or non-deterministic elements. Today's test runs 100 times and breaks once a certain case is found -- 100 iterations make the expected behavior very likely, but couldn't the tests be written more acutely? 07:02 < MarcoFalke> re 1) log level of debug by default would be a bit spammy in the normal case (no failures expected). If you debug a test, you are assumed to either set -l DEBUG or combine the logs after a failure has been detected, in which case the debug messages are included as well 07:02 < MarcoFalke> re 2) let's discuss that in the meeting 07:02 < pinheadmz> ty ! 07:25 -!- ChanServ changed the topic of #bitcoin-core-pr-reviews to: Meeting every Wednesday at 18:00 UTC | See https://bitcoincore.reviews/ for details | This channel is logged 07:26 <@jnewbery> MarcoFalke: channel topic updated. Thanks for the prod. 07:26 <@jnewbery> Meeting today is 18:00 UTC (2.5 hours from now) 07:28 -!- jasonzhouu [~jason@2409:8a28:c17:26a0:33e0:1d4d:1380:ee42] has quit [Remote host closed the connection] 07:28 -!- jason_ [~jason@144.202.127.53] has joined #bitcoin-core-pr-reviews 07:29 -!- jason_ is now known as jasonzhouu 07:43 -!- jonatack [~jon@37.164.55.120] has quit [Ping timeout: 265 seconds] 08:12 -!- Talkless [~Talkless@hst-227-49.splius.lt] has joined #bitcoin-core-pr-reviews 08:23 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has joined #bitcoin-core-pr-reviews 08:29 -!- diogosergio [~diogoserg@212.36.34.126] has quit [Ping timeout: 268 seconds] 08:45 -!- diogosergio [~diogoserg@212.36.34.126] has joined #bitcoin-core-pr-reviews 08:50 -!- diogosergio [~diogoserg@212.36.34.126] has quit [Ping timeout: 268 seconds] 09:06 -!- diogosergio [~diogoserg@212.36.34.126] has joined #bitcoin-core-pr-reviews 09:06 -!- coma-shuffling [~coma-shuf@195.181.160.175.adsl.inet-telecom.org] has joined #bitcoin-core-pr-reviews 09:07 < coma-shuffling> We're starting at 18 UTC, correct? 09:08 -!- openoms [~quassel@185.174.156.219] has quit [Remote host closed the connection] 09:09 < jonatack> yup in 52 min 09:10 -!- diogosergio [~diogoserg@212.36.34.126] has quit [Ping timeout: 240 seconds] 09:22 < coma-shuffling> PR for today: https://bitcoincore.reviews/17303.html https://github.com/bitcoin/bitcoin/pull/17303 09:40 -!- Bertrand88Conn [~Bertrand8@ns334669.ip-5-196-64.eu] has joined #bitcoin-core-pr-reviews 09:41 -!- ysangkok [janus@anubis.0x90.dk] has joined #bitcoin-core-pr-reviews 09:44 -!- Bertrand88Conn [~Bertrand8@ns334669.ip-5-196-64.eu] has quit [Read error: Connection reset by peer] 09:47 -!- Gordon85Gleichne [~Gordon85G@ns334669.ip-5-196-64.eu] has joined #bitcoin-core-pr-reviews 09:52 <@jnewbery> we'll get started in about 10 minutes 09:52 -!- Gordon85Gleichne [~Gordon85G@ns334669.ip-5-196-64.eu] has quit [Ping timeout: 252 seconds] 09:53 -!- b10c [~Thunderbi@muedsl-82-207-236-016.citykom.de] has joined #bitcoin-core-pr-reviews 10:00 <@jnewbery> #startmeeting 10:00 < MarcoFalke> ping 10:00 < kanzure> hi 10:00 < amiti> hi 10:00 < ajonas> Hi 10:00 < coma-shuffling> hi 10:00 < MarcoFalke> hi 10:00 < fjahr> hi 10:00 < jonatack> hi 10:00 <@jnewbery> hi 10:00 < pinheadmz> hi 10:00 <@jnewbery> Hi folks. MarcoFalke has very kindly offered to host this week. I'll say a few words and then hand over to him. 10:00 <@jnewbery> First, I wanted to repeat what I said last week about using this channel. 10:00 <@jnewbery> Please just jump right in! The host will prompt the conversation with the questions at https://bitcoincore.reviews/, but don't feel like you need to wait until after those questions to make your comment or ask your question. 10:01 < b10c> hi 10:01 <@jnewbery> You're not being rude if the host asks a question and you make an unrelated comment! 10:01 <@jnewbery> We can have multiple threads going at the same time. If it gets too confusing the host can step in and ask people to address one topic first, but in general just go for it. 10:01 <@jnewbery> Also, don't ask to ask questions. There's no need to say things like "I have a question that I think might be off topic" or "is it ok if I ask my question now?". Just jump right in and ask. If it's off-topic, then people will tell you! 10:01 < fanquake> hi 10:01 < ariard> hi 10:01 <@jnewbery> There were a couple of questions asked before the meeting. I'll repeat them here so they make it to the meeting log. 10:01 <@jnewbery> < pinheadmz> 1 why is the loglevel for the testframework not `debug` (or even `spam`) by default? Today's PR modifies a test where I found the `debug` level output especially helpful 10:01 <@jnewbery> < pinheadmz> 2 the python tests often have random elements or non-deterministic elements. Today's test runs 100 times and breaks once a certain case is found -- 100 iterations make the expected behavior very likely, but couldn't the tests be written more acutely? 10:01 <@jnewbery> < MarcoFalke> re 1) log level of debug by default would be a bit spammy in the normal case (no failures expected). If you debug a test, you are assumed to either set -l DEBUG or combine the logs after a failure has been detected, in which case the debug messages are included as well 10:01 <@jnewbery> < MarcoFalke> re 2) let's discuss that in the meeting 10:01 <@jnewbery> ok, that's all I had to say. Over to Marco... 10:02 < MarcoFalke> hi, so let's get started with the basics. Everyone is familiar with how tx relay works in Bitcoin? 10:02 < MarcoFalke> Any questions about the gossip protocol? 10:02 < jkczyz> hi 10:03 < jasonzhouu> hi 10:03 < MarcoFalke> Ok, jumping into the questions, then 10:04 < MarcoFalke> So let's start by the general goal of this pull request 10:04 < MarcoFalke> Anyone want to take a try at giving a brief summary? 10:05 < pinheadmz> ok 10:05 < pinheadmz> seems like, there is this mapRelay where we kinda buffer TXs even after theyve been evicted from the mempool, for up to 15 minutes 10:05 < pinheadmz> This PR takes that out - so if a peer requests a tx that isnt STILL in the mempool, they get a notfound back 10:05 < coma-shuffling> reduce resource load, some privacy improvments? 10:06 < MarcoFalke> both correct 10:06 < pinheadmz> I dont understand what the mapRelay was used for beore this PR - the change makes so much sense ! 10:06 < ajonas> So relay transaction seems like sort of an odd thing (was added for caching purposes?) and generally removing it simplifies the logic but also I'm a little unclear as to why this was added in the beginning 10:06 <@jnewbery> I had the same question. Why didn't we always just relay from the mempool? Privacy/fingerprinting concerns? 10:07 < jasonzhouu> +1 10:07 < MarcoFalke> mapRelay has been introduced before I was around, so I might not be able to answer that question with certainty 10:08 < jasonzhouu> Is relay momory mainly used to increase privacy, comparing to memery pool? 10:08 <@jnewbery> as a tip for code excavation, `git log -S ` shows all commits where that string is part of the diff 10:08 <@jnewbery> if I run `git log -SmapRelay`, I see that it was there in the first commit 10:09 < MarcoFalke> Previously transactions have been copied around in full, and CTransactionRef was introduced later on 10:09 < pinheadmz> jnewbery: cool trick tnx! and interesting its so old 10:09 < pinheadmz> who's sirius-m ?! 10:09 < MarcoFalke> It might have been neccessary in the early design of Bitcoin Core to have a copy of the tx around 10:10 < MarcoFalke> I think sirius has transformed the svn repo into a git repo 10:10 < ariard> isn't mapRelay a reason to be a useful tx relay peer if you have a mempool set to 0? 10:10 < ariard> or before than mempool was operational? 10:10 <@jnewbery> sirius is Martii Malmi. I think he was the second contributor to Bitcoin after Satoshi 10:11 < MarcoFalke> ariard: transactions are only added to mapRelay after they made it into the mempool 10:11 < fjahr> I did not dig deeper into this but on some level it made sense to me to decouple mempool behavior from relay behavior for privacy reasons. 10:11 < coma-shuffling> jnewbery correct 10:11 <@jnewbery> ariard: you can't relay transactions without a mempool 10:12 <@jnewbery> AcceptToMemoryPool is where we do our standardness checks 10:12 < ariard> jnewbery, MarcoFalke: looking on first git commit, seems mapRelay was already there 10:13 < MarcoFalke> fjahr: Good point. However, I think mempool and relay are too closely coupled to achieve good tx-origin privacy . There are a lot of tricks you can play to leak privacy 10:13 < pinheadmz> also interesting that the old txs in the maprelay arent pruned on a timeout or anything, just when sendMessages() is called 10:13 < pinheadmz> (IIUC) 10:14 <@jnewbery> pinheadmz: right, it's lazy deletion 10:14 < MarcoFalke> pinheadmz: The send messages function is called in a tight loop anyway, so it is close enough to a timeout 10:16 < coma-shuffling> FYI mapRelay is in the bitcoin-0.1.0 source code https://bitcointalk.org/index.php?topic=68121.0 10:16 < MarcoFalke> I think the main goal of the mapRelay expiry is to have some upper bound on its memory usage, which is achieved by the tight loop. (To some extend) 10:17 < MarcoFalke> Ok, my first question: 10:17 < MarcoFalke> What is a shared pointer and why are transactions in Bitcoin Core generally managed as a shared pointer (CTransactionRef)? 10:17 < coma-shuffling> It's referenced in relation to "CRITICAL_BLOCK" parts 10:18 < ajonas> shared_ptr is a smart pointer that retains shared ownership of an object through a pointer. 10:18 < ajonas> google!! 10:18 < jkczyz> It is a smart pointer that uses reference counting to determine when the memory can be deleted 10:19 -!- davterra [~none@195.242.213.120] has joined #bitcoin-core-pr-reviews 10:19 < MarcoFalke> Yes, so why are transactions in bitcoin core managed that way? 10:19 < ajonas> because they are managed in a few different places 10:19 < jonatack> MarcoFalke: with a shared_ptr as opposed to a unique_ptr? 10:20 < fjahr> and because it saves memory 10:20 < MarcoFalke> fjahr: Yes 10:20 < jkczyz> Someone decided to punt on ownership :P 10:20 < jonatack> yes, when you heap-allocate a resource that needs to be shared among multiple objects 10:20 < MarcoFalke> jonatack: Good question! any takers? 10:21 < jonatack> there was a PR review discussion on this between sipa/gmax and jonas schnelli recently 10:21 < jkczyz> jonatack: Do you have a reference? I'd be intereted in reading 10:22 < jonatack> shared = more flexibility but more going on so slight perf hit, iirc 10:22 < jonatack> wrt unique 10:22 < MarcoFalke> jonatack: It is a shared_ptr to minimize mental load. I think a unique_ptr makes most sense when there is exaclty one owner, but in Bitcoin Core there can be many owners of a single transaction: E.g. (1) the most recent block, (2) the mempool, (3) mapRelay, (4) anything I forgot? 10:22 <@jnewbery> jonatack: this one: https://github.com/bitcoin/bitcoin/pull/14046#issuecomment-415814511 ? 10:23 < ajonas> am I right with a unique pointer once it's destroyed in one place it is reclaimed, but shared is all have to destroy it in order to deallocate? 10:23 < jonatack> jnewbery: yes, ty! 10:23 < MarcoFalke> (4) the miner 10:23 < jonatack> jnewbery: please give a masterclass on finding things quickly in GH :) 10:24 <@jnewbery> https://github.com/bitcoin/bitcoin/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+sipa+gmaxwell+jonasschnelli+shared 10:24 < MarcoFalke> ajonas: The shared pointer uses a reference count as pointed out by jkczyz. This is a slight overhead over unique_ptr, but gives more flexibility 10:24 < MarcoFalke> The shared pointer releases the memory when the count goes to 0 10:25 < jonatack> jnewbery: oh ty, using the PR filters in the GH web interface. will use that more. 10:25 < ajonas> marcofalke: OK. Guess I didn't quite get that. 10:26 < MarcoFalke> so unique_ptr is like a shared_ptr where the count can only be 0 or 1 exactly 10:26 < MarcoFalke> ok, going to the next question 10:26 < MarcoFalke> What is the difference between the memory pool (mempool) and the relay memory (mapRelay)? 10:27 < pinheadmz> one thing for sure, made clear by this PR: mempool txs can be evicted for several reasons. the maprealy just expires after a set time 10:27 < MarcoFalke> pinheadmz: Correct 10:27 < MarcoFalke> Any other ideas what is different between them? 10:28 < ariard> the mempool is a storage for unconf txn to help block construction 10:28 < MarcoFalke> Or rather: What is common between them? 10:28 < jasonzhouu> relay memory needs `inv` 10:28 < ariard> it also serves for fee estimation and tx relay 10:28 < MarcoFalke> jasonzhouu: Right. Transactions are only added to the relay memory after the first inv for the tx has been sent out 10:28 < MarcoFalke> ariard: Correct 10:29 < fjahr> mempool can be restricted in size and maprelay not I think 10:29 < ariard> so there is an overlap between goals of both rn 10:29 < MarcoFalke> fjahr: Good point. Can you explain why? 10:29 < pinheadmz> i wonder if the original design was mempool would just be for cooking up blocks, and maprelay was for p2p ? 10:30 < MarcoFalke> Or let's start easy: Can it be that the mempool and mapRelay have the same transactions in them? 10:30 < ariard> there is a maxmempool option to restrict size 10:30 < fjahr> yes 10:30 < amiti> mapRelay will only have txns that have been previously accepted to the mempool, but they might no longer be there.. 10:30 < ariard> pinheadmz: yep was my wonder earlier 10:30 < jasonzhouu> If mempool don't has limit size, then there is posibility of DoS attack. 10:30 < fjahr> during the first 15min after startup anyway 10:30 < amiti> I think the reason theres no explicit constraint on mapRelay is bc its implicit with size of mempool + time out ? 10:31 < MarcoFalke> amiti: Yes, sort of 10:31 <@jnewbery> It might surprise you all to know that mempool limiting was only added in v0.12.0: https://bitcoincore.org/en/releases/0.12.0/#memory-pool-limiting 10:32 < MarcoFalke> Good old days when 0.11.0 nodes ran OOM 10:32 <@jnewbery> prior to that you could OOM a node by filling up its mempool 10:32 < MarcoFalke> Even with txs that pay no fee :) 10:32 < fjahr> the point of the maprelay is to help with relay and so restricting it would hinder it so that would work against that goal 10:33 < pinheadmz> jnewbery: no surprise, there was no volume back then :-) 10:33 < MarcoFalke> ok, next question: Is there an upper bound on the memory usage of the relay memory (mapRelay)? Under what circumstances is the expected memory usage low, under what circumstances is the memory usage high? (Hint: CTransactionRef is a shared pointer) 10:33 <@jnewbery> At a high level, I'd answer the question by saying that the mempool has a lot more structure than mapRelay 10:33 < jonatack> jnewbery: was that the vuln demoed live onstage in 2016 at Breaking Bitcoin in Paris? 10:34 < MarcoFalke> jnewbery: Also good point 10:34 <@jnewbery> jonatack: no, that was something different 10:34 < amiti> MarcoFalke: could you elaborate on "Yes, sort of"? what is correct / what am I missing ? 10:34 < MarcoFalke> amiti: You are correct 10:34 <@jnewbery> the mempool should always be consistent (ie the entire mempool can be made into a valid block if we ignore block weight limits) 10:35 <@jnewbery> the mempool is also indexed in various ways that are useful for its clients 10:35 < MarcoFalke> ok, let's move on to the next question 10:35 < MarcoFalke> Is there an upper bound on the memory usage of the relay memory (mapRelay)? 10:36 < ajonas> so Suhas said that "mapRelay is bounded by our rate-limited transaction relay algorithm" https://github.com/bitcoin/bitcoin/pull/14220#issuecomment-430710707 10:36 < MarcoFalke> amiti already gave an answer, but could someone elaborate the point 10:36 < ariard> there is an upper bound based on time and not on size, compare to the mempool 10:37 < MarcoFalke> ajonas: Good point. That is one way to look at it. Does that give us nice estimates about the memory usage of mapRelay? 10:38 < MarcoFalke> ariard: Good point. But why is it not on size? Note that transactions are only added to mapRelay after they entered the mempool? 10:39 < pinheadmz> hmm all this talk about sahred pointers - maprealy shouldn't increase mem usage at all except in the case that mempool evicts a tx that maprelay still hangs on to 10:39 < amiti> could mapRelay theoretically increase memory usage a lot if theres a lot of churn in the mempool? eg. small mempool and txns are getting INVed but then kicked out really rapidly 10:40 < jasonzhouu> The max of relay memory is txs that have ever been included in mempool? 10:40 < jasonzhouu> in the last 15 minutes 10:40 < MarcoFalke> pinheadmz: Yes. Though, we store the txid and expiry time. 10:40 < MarcoFalke> amiti: Yes. 10:41 < MarcoFalke> jasonzhouu: Yes 10:41 < ariard> MarcoFalke: because there is an assumption than size of mapRelay can't be bigger than mempool one due to dependency of former on latter ? 10:42 <@jnewbery> ariard: I'm not sure if there's any such assumption (and if there were, it'd be invalid because of the reason amiti said) 10:42 < MarcoFalke> So under what circumstances are txs evicted from the memory? 10:42 < MarcoFalke> *mempool 10:42 < amiti> for the reasons you listed in the PR description ... :) 10:42 -!- seven__ [~seven@BSN-77-101-62.static.siol.net] has quit [Remote host closed the connection] 10:42 < MarcoFalke> amiti: heh 10:42 < amiti> expiry, size limit, reorg, block, conflict, replaced 10:42 < pinheadmz> RBF, blcok inclusion, mempool size limit 10:43 -!- seven__ [~seven@BSN-77-101-62.static.siol.net] has joined #bitcoin-core-pr-reviews 10:43 < amiti> I have some questions about concerns that reviewers brought up... 10:43 < jasonzhouu> 1. be included in block 10:43 < amiti> sdaftuar mentions these changes can turn nodes with small mempools into "accidental DoSers", which I didn't fully understand.. so a node would send an INV to its peer, peer asks for GETDATA, during that time the txn has been evicted from the node's mempool so then it responds with NOTFOUND. why is this a DoS though? is it bc the peer waits until the request times out before asking another node for the tx? 10:43 < jasonzhouu> 2. be replaced by higher fee tx 10:44 < MarcoFalke> On a high level I found that rbfed txs and txs included in blocks were the primary reason that mapRelay uses more memory than the mempool 10:44 < MarcoFalke> (Assuming the mempool does not operate on its size limit) 10:44 < MarcoFalke> amiti: Good question! 10:44 < pinheadmz> That alone is a great reason for this PR - why relay a tx that has already been RBF'd 10:45 < amiti> ^ maybe because of privacy reasons, as mentioned from naumenkogs review 10:45 < MarcoFalke> amiti: You answered that question yourself: "the peer waits until the request times out before asking another node for the tx" 10:45 < jonatack> MarcoFalke: how did you find that? 10:46 < MarcoFalke> jonatack: Run a Bitcoin Core full node with some added LogPrintf calls 10:46 < jonatack> "net" logging too? 10:46 < MarcoFalke> amiti: more specifically, we ignore NOTFOUND from peers to the extend that we don't use them to reschedule the tx download from a different peer 10:47 <@jnewbery> amiti: also look at https://github.com/bitcoin/bitcoin/pull/15834. Before that, there was a bug that if a node INVed us a transaction and then didn't respond to the GETDATA, it wouldn't clear from our tx-in-flight map 10:47 < MarcoFalke> amiti: See https://github.com/bitcoin/bitcoin/pull/15505 10:47 < MarcoFalke> Ok, moving to the next question 10:47 < MarcoFalke> Transactions are added to mapRelay when they are announced via an inv. Does mapRelay keep track of which peers it announced a transaction to? If no, how does that affect transaction relay privacy? 10:48 < amiti> jnewbery, MarcoFalke: thanks :) 10:49 < pinheadmz> we dont 10:49 < ajonas> MarcoFalke: I don't believe it does keep track, which is what Gleb is proposing with a bloom filter to track it. 10:49 < pinheadmz> mapRelay.insert(std::make_pair(hash, std::move(txinfo.tx))); 10:49 < amiti> I believe this PR added the param `m_last_inv_sent` to keep track of the time the last set of INVs was announced to a peer, but doesn't keep track of specifically which one 10:49 < amiti> *ones 10:50 < MarcoFalke> pinheadmz: ajonas: Is the issue in the current code or is that introduced in my change? 10:50 < MarcoFalke> And how does it affect privacy? 10:50 < ajonas> current 10:50 < MarcoFalke> I.e. what is the attack scenario? 10:50 < pinheadmz> yeah maprealy is kinda dumb, just a list of txs, no peer info 10:51 < ajonas> means it's still vulnerable to inferring your location in the tx relay network topology 10:51 < MarcoFalke> How would you leak that? 10:51 < pinheadmz> but i feel like i must be missing something, bc we woldnt send the same tx to a peer twice, or if it was the peer that sent it to us in the first place... 10:52 < MarcoFalke> pinheadmz: We do (if they send a GETDATA twice) 10:52 < ajonas> Gleb spells it out here: https://github.com/bitcoin/bitcoin/pull/17303#issuecomment-550460346 10:52 < jonatack> flooding with the same tx iiuc 10:52 < ajonas> basically flood network with tx_a and then selectively announce tx_b (RBF of tx_a). Start querying for tx_a - whoever responds with NOTFOUND received tx_b which means the node is closer to the node target. 10:53 < pinheadmz> ajonas: wouldnt that be the behavior AFTER this is merged? 10:53 < MarcoFalke> ajonas: Yes. And the privacy leak that allows us to infer the source of a transaction? (current master code) 10:53 < MarcoFalke> Yeah, sorry. I was asking about BEFORE this is merged 10:54 < pinheadmz> because the maprelay would still send tx_a to anyone who asked, even if the replacement was in the mempool 10:54 < pinheadmz> heh, well I guess you could send a MEMPOOL request ? 10:54 < amiti> I think currently the attack would be to have lots of connections to nodes on the network & send out GETDATAs for any new txn to identify which nodes already have the txn 10:55 < amiti> and they would respond even though they didnt send you an INV message yet 10:55 < MarcoFalke> amiti: Correct! 10:55 < MarcoFalke> amiti: How many connections do you need per node on the network? 10:56 < amiti> uhhh... I'm guessing more than one bc otherwise you wouldn't ask this question? 10:56 < amiti> but I don't see why 10:56 < ajonas> just 1 10:56 < MarcoFalke> Yes, just one inbound cnnection is enough 10:56 < amiti> oh lol 10:56 < MarcoFalke> Ok, to wrap up... 10:57 < coma-shuffling> wasn't such an attack presented at Scaling Bitcoin Tel Aviv? 10:57 < MarcoFalke> Any questions left? 10:57 < pinheadmz> coma-shuffling: that involved orphan txs i think 10:57 < coma-shuffling> ah 10:57 < pinheadmz> MarcoFalke: yeah i was just wondering about testing 10:57 < MarcoFalke> < pinheadmz> 2 the python tests often have random elements or non-deterministic elements. Today's test runs 100 times and breaks once a certain case is found -- 100 iterations make the expected behavior very likely, but couldn't the tests be written more acutely? 10:58 < amiti> ya- question around naumenkogs review. MarcoFalke: what do you think about the concern? 10:58 < pinheadmz> i downloaded the PR and tried the test with and without the change. also tried the new test against the current code (without PR) nothing really changes there 10:58 < pinheadmz> I see in the test you are being explicit that if a node did relay the tx back, you check that they got it in an inv or tx message 10:58 <@jnewbery> suhas suggests that improving our NOTFOUND behaviour should be a precondition for this (https://github.com/bitcoin/bitcoin/pull/17303#issuecomment-547589047). Do you agree? 10:58 < MarcoFalke> amiti: I think that my set of changes should be kept to a minimum that is still an improvement. I don't claim to fix all privacy issues and I think even fixing one is reason enough 10:58 < amiti> ya I agree 10:58 < MarcoFalke> amiti: Also the memory improvements might be worth it 10:59 < MarcoFalke> amiti: Not to mention bandwidth 10:59 < MarcoFalke> jnewbery: I disagree 10:59 < ajonas> but reintroducing mapRelay again seems like a lot of churn 10:59 < ajonas> as Gleb is suggesting 11:00 < MarcoFalke> jnewbery: I think that suhas DOS case is an edge case and should not happen very often in the network. 11:00 < jonatack> MarcoFalke: do you think the new tests cover the change of behavior sufficiently? What do you think about the current state of our p2p testing? 11:00 < MarcoFalke> INV("endmeeting") 11:00 < amiti> lol! 11:00 < fjahr> NOTFOUND 11:00 < MarcoFalke> lets keep going for two more minutes 11:01 < jonatack> e.g. Suhas' issue here: https://github.com/bitcoin/bitcoin/issues/14210 11:01 < jonatack> "Test coverage of our networking code" 11:01 < MarcoFalke> jnewbery: And we do handle the case (even though it takes 120 secs or so to timeout) 11:02 < MarcoFalke> jnewbery: Though, I'd like to see suhas fix as well. Assuming it doesn't itself result in a CPU DOS vector 11:02 < MarcoFalke> ajonas: I don't think reintroducing mapRelay should be done 11:02 < MarcoFalke> ajonas: If it is necessary, I'd rather have a limited map of map_recently_rbfd_txs 11:03 < MarcoFalke> jonatack: pinheadmz: Good question about testing! 11:03 < amiti> ya, I was wondering that .... if the attack is just about RBF txns, we'd hypothetically only have to keep track of those 11:03 < MarcoFalke> What are the complications to test such a change? I.e. how to test the privacy guarantees? 11:03 < MarcoFalke> amiti: Yes, agree. 11:04 < MarcoFalke> Has anyone seen my test changes? What is the test doing before and what is it doing after my changes? 11:04 < MarcoFalke> We will wrap up in one minute :) 11:05 < MarcoFalke> #endmeeting 11:05 < pinheadmz> all you added to the test was checking that if the tx was already announced to us, the txid exists in a inv or tx message 11:05 <@jnewbery> Thanks MarcoFalke. Great meeting! 11:05 < coma-shuffling> thanks MarcoFalke 11:05 < MarcoFalke> jnewbery: Thanks for suggesting it to me 11:06 < jasonzhouu> Thanks MacroFalke 11:06 < jonatack> thanks everyone 11:06 < MarcoFalke> Happy to do more of these 11:06 < ariard> thanks MarcoFalke! 11:06 < pinheadmz> thanks everyone! 11:06 < amiti> thank you ! 11:06 < jonatack> thank you MarcoFalke for hosting] 11:06 < ajonas> thanks Marco 11:06 < MarcoFalke> thanks everyone for the answers and questions. Really high quality today 11:06 <@jnewbery> I'll try to get notes and questions up for next week's meeting before the weekend. I'm travelling so I might not be able to attend but I think I have a host lined up 11:07 < jonatack> jasonzhouu: MacroFalke, i like that +1 11:07 < pinheadmz> !!! 11:18 <@jnewbery> Meeting Log is published: https://bitcoincore.reviews/17303.html 11:31 < jonatack> thanks jnewbery 11:32 -!- coma-shuffling [~coma-shuf@195.181.160.175.adsl.inet-telecom.org] has left #bitcoin-core-pr-reviews [] 11:34 < jonatack> tangent to the smart pointers discussion above, if helpful to anyone, note that doc/developer-notes.md lines 501-509 discusses Bitcoin Core unique_ptr usage 11:40 -!- jasonzhouu [~jason@144.202.127.53] has quit [Ping timeout: 276 seconds] 12:03 -!- emilengler [~emilengle@unaffiliated/emilengler] has quit [Quit: Leaving] 12:47 -!- Talkless [~Talkless@hst-227-49.splius.lt] has quit [Quit: Konversation terminated!] 12:52 -!- wullon [~wullon@241.243.86.88.rdns.comcable.net] has quit [Ping timeout: 268 seconds] 12:59 -!- b10c [~Thunderbi@muedsl-82-207-236-016.citykom.de] has quit [Ping timeout: 268 seconds] 13:54 <@jnewbery> Notes and questions for next week are up. Thanks to pinheadmz for hosting and writing the notes! 13:54 <@jnewbery> https://bitcoincore.reviews/16442.html 13:55 < pinheadmz> w00t! 13:56 < MarcoFalke> sweet 14:18 -!- b10c [~Thunderbi@muedsl-82-207-236-016.citykom.de] has joined #bitcoin-core-pr-reviews 14:27 -!- wullon [~wullon@241.243.86.88.rdns.comcable.net] has joined #bitcoin-core-pr-reviews 14:30 -!- diogosergio [~diogoserg@176.24.23.243] has joined #bitcoin-core-pr-reviews 15:12 -!- diogosergio [~diogoserg@176.24.23.243] has quit [Ping timeout: 245 seconds] 15:13 -!- pinheadmz [~matthewzi@pool-100-33-69-78.nycmny.fios.verizon.net] has quit [Quit: pinheadmz] 15:16 -!- diogosergio [~diogoserg@176.24.23.243] has joined #bitcoin-core-pr-reviews 15:29 -!- b10c [~Thunderbi@muedsl-82-207-236-016.citykom.de] has quit [Ping timeout: 240 seconds] 15:49 -!- diogosergio [~diogoserg@176.24.23.243] has quit [Quit: Lost terminal] 15:51 -!- openoms [~quassel@185.174.156.219] has joined #bitcoin-core-pr-reviews 17:18 -!- openoms [~quassel@185.174.156.219] has quit [Ping timeout: 265 seconds] 17:56 -!- soju [uid403160@gateway/web/irccloud.com/x-xbxryqnnquoemlev] has quit [Quit: Connection closed for inactivity] 18:02 -!- soju [uid403160@gateway/web/irccloud.com/x-ujnbswrsbuznqxph] has joined #bitcoin-core-pr-reviews 18:14 -!- jasonzhouu [~jason@144.202.127.53] has joined #bitcoin-core-pr-reviews 19:00 -!- designwish [~designwis@51.ip-51-68-136.eu] has quit [Quit: ZNC - https://znc.in] 19:07 -!- designwish [~designwis@51.ip-51-68-136.eu] has joined #bitcoin-core-pr-reviews 19:17 -!- felixfoertsch23 [~felixfoer@2001:16b8:505c:7e00:1923:2d3a:1e64:23b9] has joined #bitcoin-core-pr-reviews 19:18 -!- felixfoertsch [~felixfoer@2001:16b8:50eb:bd00:1923:2d3a:1e64:23b9] has quit [Ping timeout: 252 seconds] 20:28 -!- jasonzhouu [~jason@144.202.127.53] has quit [Quit: Leaving] 20:36 -!- soju [uid403160@gateway/web/irccloud.com/x-ujnbswrsbuznqxph] has quit [Quit: Connection closed for inactivity] 20:40 -!- jason [~jason@144.202.127.53] has joined #bitcoin-core-pr-reviews 20:40 -!- jason [~jason@144.202.127.53] has quit [Client Quit] 21:27 -!- felixfoertsch23 [~felixfoer@2001:16b8:505c:7e00:1923:2d3a:1e64:23b9] has quit [Quit: ZNC 1.7.3 - https://znc.in] 21:28 -!- felixfoertsch [~felixfoer@92.117.41.91] has joined #bitcoin-core-pr-reviews