--- Day changed Thu May 14 2020
00:13 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 272 seconds]     
00:30 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #bitcoin-core-pr-reviews     
01:33 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
01:41 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 272 seconds]     
02:03 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 265 seconds]     
02:06 -!- theStack [~honeybadg@vps1648322.vs.webtropia-customer.com] has joined #bitcoin-core-pr-reviews     
02:17 -!- dfmb_ [~dfmb_@unaffiliated/dfmb/x-4009105] has joined #bitcoin-core-pr-reviews     
03:17 -!- slivera_ [~slivera@14-200-89-62.tpgi.com.au] has joined #bitcoin-core-pr-reviews     
03:19 -!- slivera [~slivera@116.206.228.243] has quit [Read error: Connection reset by peer]     
03:37 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
03:42 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 246 seconds]     
04:04 -!- ghost43_ [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection]     
04:04 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews     
04:14 -!- driver [~driver@195.181.160.175.adsl.inet-telecom.org] has quit [Ping timeout: 256 seconds]     
04:52 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 256 seconds]     
04:55 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds]     
04:57 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews     
05:08 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #bitcoin-core-pr-reviews     
05:12 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews     
05:13 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 256 seconds]     
05:15 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #bitcoin-core-pr-reviews     
05:16 -!- slivera__ [~slivera@116.206.228.244] has joined #bitcoin-core-pr-reviews     
05:19 -!- slivera_ [~slivera@14-200-89-62.tpgi.com.au] has quit [Ping timeout: 256 seconds]     
05:31 -!- shaunsun [~shaunsun@c-76-26-29-34.hsd1.fl.comcast.net] has joined #bitcoin-core-pr-reviews     
05:32 -!- molakala [~sumanth.b@2601:192:4701:87f0:257a:5b91:a1f4:3f48] has joined #bitcoin-core-pr-reviews     
05:33 -!- shaunsun_ [shaunsun@gateway/vpn/privateinternetaccess/shaunsun] has joined #bitcoin-core-pr-reviews     
05:36 -!- shaunsun [~shaunsun@c-76-26-29-34.hsd1.fl.comcast.net] has quit [Ping timeout: 265 seconds]     
05:38 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
05:41 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection]     
05:42 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews     
05:43 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 260 seconds]     
05:45 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews     
05:46 -!- shaunsun_ [shaunsun@gateway/vpn/privateinternetaccess/shaunsun] has quit [Quit: Leaving]     
05:48 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 265 seconds]     
05:54 -!- slivera__ [~slivera@116.206.228.244] has quit [Ping timeout: 246 seconds]     
06:19 -!- davterra [~dulyNoded@2601:603:4f00:63d0::5] has joined #bitcoin-core-pr-reviews     
06:53 -!- troygiorshev [~troy@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has joined #bitcoin-core-pr-reviews     
07:31 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Quit: jonatack]     
07:33 -!- molz_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews     
07:35 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 265 seconds]     
07:39 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
07:53 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 246 seconds]     
08:23 -!- Furman78Eichmann [~Furman78E@static.57.1.216.95.clients.your-server.de] has joined #bitcoin-core-pr-reviews     
08:26 -!- Furman78Eichmann [~Furman78E@static.57.1.216.95.clients.your-server.de] has quit [Remote host closed the connection]     
08:26 -!- Nola3Satterfield [~Nola3Satt@static.57.1.216.95.clients.your-server.de] has joined #bitcoin-core-pr-reviews     
08:28 -!- Nola3Satterfield [~Nola3Satt@static.57.1.216.95.clients.your-server.de] has quit [Remote host closed the connection]     
08:30 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
08:31 -!- Shayne64Rath [~Shayne64R@static.57.1.216.95.clients.your-server.de] has joined #bitcoin-core-pr-reviews     
08:45 < raj_149> Hi, is it possible to trigger gdb debug from functional test into a node's binary? I want to run a functional test, but i want the control to stop inside a cpp function coresponding to a rpc call and step by step from there. Is it even possible?
08:46 < jnewbery> raj_149: https://github.com/bitcoin/bitcoin/blob/eb2ffbb7c1347115e6a3d6fa30f909959b6170a5/test/README.md#attaching-a-debugger
08:49 -!- sr_gi_ [~sr_gi@183.red-83-34-186.dynamicip.rima-tde.net] has joined #bitcoin-core-pr-reviews     
08:50 < raj_149> jnewbery: thanks a lot.
08:51 -!- sr_gi [~sr_gi@128.red-79-154-168.dynamicip.rima-tde.net] has quit [Ping timeout: 256 seconds]     
08:52 < raj_149> one question. Do i need to stop at the rpc call or just after it?
08:53 < jnewbery> you're hoping to catch the node's processing of the RPC call? If so, stop the functional test just before the RP call, then attach a gdb debugger to bitcoind with a breakpoint in the RPC server, then step forward in the functional test to call the RPC.
08:54 < jnewbery> the test may time out the RPC call and maybe try to kill the bitcoind process. Running the test with --noshutdown should prevent that.
08:55 -!- Shayne64Rath [~Shayne64R@static.57.1.216.95.clients.your-server.de] has quit [Ping timeout: 256 seconds]     
08:55 < jnewbery> reminder to everyone that we'll get started with the BIP 157 special meeting in just over an hour. Notes here: https://bitcoincore.reviews/18960.html
08:56 < raj_149> Thanks. Does this necessarily require pdb? or i can just stop the test using any method?
08:56 < jnewbery> pdb is part of Python's standard library, so if you can run the tests, you'll have it
08:57 < ja> raj_149: if you're using python3.8 you can use breakpoint(), a bit cleaner
08:58 < ja> raj_149: and breakpoint() can work with whatever debugger of course, that is the whole point ;)
09:01 < ja> oh sorry, it is already in 3.7
09:03 -!- molz_ [~mol@unaffiliated/molly] has quit [Ping timeout: 264 seconds]     
09:12 < fjahr> raj_149: https://github.com/fjahr/debugging_bitcoin#debugging-from-functional-tests
09:14 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 265 seconds]     
09:23 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds]     
09:26 -!- lightlike [~lightlike@p200300C7EF1BAB0059BDFFF3A040C7D2.dip0.t-ipconnect.de] has joined #bitcoin-core-pr-reviews     
09:32  * MarcoFalke has never used a breakpoint in python, only print() or log.info() to debug
09:32 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Remote host closed the connection]     
09:32 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews     
09:35 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews     
09:36 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined #bitcoin-core-pr-reviews     
09:42 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined #bitcoin-core-pr-reviews     
09:43 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has joined #bitcoin-core-pr-reviews     
09:48 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews     
09:51 -!- Caralie [185a5a32@cpe-24-90-90-50.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews     
09:53 -!- molakala [~sumanth.b@2601:192:4701:87f0:257a:5b91:a1f4:3f48] has quit [Ping timeout: 240 seconds]     
09:56 < jonatack> For info, if things go as planned, I'll be on a schedule that isn't compatible with the meeting times. OTOH, I might be able to host some a few hours later for people in Asian time zones. TBC.
10:00 < jnewbery> #startmeeting
10:00 < jnewbery> hi     
10:00 < michaelfolkson> hi     
10:00 < theStack> hi     
10:00 < troygiorshev> hi     
10:00 < fjahr> hi     
10:01 < jkczyz> hi     
10:01 < jonatack> hi     
10:01 < Caralie> hi     
10:01 < lightlike> hi     
10:01 < MarcoFalke> hi     
10:01 < andrewtoth> hi     
10:01 < jnewbery> thanks for joining pt 2 of the BIP 157 special edition review club series!
10:01 < jnewbery> notes/questions in the normal place: https://bitcoincore.reviews/18960.html
10:02 < jnewbery> who had a chance to review the PR? y/n
10:02 < thomasb06> hi     
10:02 < theStack> y     
10:02 < troygiorshev> n     
10:02 < andrewtoth> n     
10:02 < jkczyz> y     
10:02 < thomasb06> n     
10:03 < fjahr> y earlier but missed that there were notes
10:03 < jnewbery> and for those that did review it, did you look at the alternative cache implementations in https://github.com/bitcoin-core-review-club/bitcoin/releases/tag/pr18960.1 and https://github.com/bitcoin-core-review-club/bitcoin/releases/tag/pr18960.2 ?
10:03 < jkczyz> yup     
10:04 < michaelfolkson> Somewhat. Not to a point where I understood exactly what was going on in all 3
10:04 < jnewbery> great. So PR 18960 is fairly straightforward and small, so I think the interesting discussion this week will be around how the implementations differ
10:04 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
10:05 < jnewbery> Is anyone able to give a high-level summary of the different approaches?
10:06 < jkczyz> Roughly: (1) populates the cache as blocks are connected and needs to manage reorgs (2) populates when filters are requested in the message handler (3) moves the cache to the BlockFilterIndex
10:06 < theStack> 1) update cache on block tip update, cache by height 2) update cache on getcfcheckpt request, cache by height 3) update cache on getfcheckpt request, cache by block hash
10:07 < jnewbery> jkczyz theStack: great summaries. Thanks!
10:07 < jkczyz> theStack: ah yes, height vs hash too
10:07 < jnewbery> So q1: why are locks added? Are they needed?
10:08 < jkczyz> I suppose it would be needed if populating and querying are done on separate threads, which is not in (3) but I assume so in (1) and (2)
10:09 < theStack> they are needed because LookupFilterHeader could be also called from a different thread -- currently an RPC call i think
10:09 < jnewbery> jkczyz: yes. We don't want to read/write the same data in different threads without a lock
10:10 < jnewbery> theStack: good spot. I think we can call LookupFilterHeader() from both the message handler thread (net processing) and an RPC thread
10:10 < jnewbery> so without a lock there could be a race
10:11 < jnewbery> I guess I should say http helper thread, since after https://github.com/bitcoin/bitcoin/pull/17631, the filter headers will also be fetchable over REST
10:12 < jkczyz> so needed in (3) actually
10:12 < jnewbery> I think implementation (2) strictly doesn't need a lock since all the reading/writing of the cache happens in the message handler thread
10:12 < jnewbery> jkczyz: right
10:13 < jnewbery> ok, onto q2. The first implementation could cause a data race on shutdown (full log). Can you find where the data race is?
10:14 < jnewbery> (and as always, feel free to ask any questions any time. I only put the set questions out to prompt discussion)
10:15 < theStack> i saw that the original implementation used std::mutex, while in the codebase mostly our custom (Recursive)Mutex seems to be used -- is there any guide on when to use what?
10:16 < jnewbery> MarcoFalke is probably best placed to answer this, if he's still around
10:17 < jnewbery> RecursiveMutex allows us to take locks multiple times, which happens in some places. I think a long term goal might be to git rid of that usage pattern?
10:18 < aj> our custom Mutex (and RecursiveMutex) are just the standard ones, with extra code to debug lock order violations (which might result in deadlocks) and to support clangs thread safety compile time checks
10:20 < jnewbery> The data race on shutdown was in a UpdatedBlockTip() callback. See L5727 of https://travis-ci.org/github/bitcoin/bitcoin/jobs/665594933. I don't know exactly why that happens, but I guess it's what caused jimpo to move to implementation (2), which doesn't use that callback
10:20 < theStack> aj: ok, so i deduce from that that it's general a good idea to use them
10:21 < jnewbery> perhaps it'd be a good idea to talk aout performance. theStack, I see you've done some tests. Can you summarise what you did?
10:21 < sipa> for a mutex which is a private field in a class, which is not exposed to any external modules, shouldn't need our lock assertion wrappers
10:23 < aj> sipa: and none of the class's member functions take any other locks, or call functions that take any other locks? otherwise you could still get lock order violations?
10:23 < fjahr> here is the change he pushed after sipa pointed out the data race: https://github.com/bitcoin/bitcoin/compare/f68aa2c1a40afb49969b9120ec43e2a5040724f1..7608b32fc27254c84c16090af9a32d6bbdf60ec7#diff-eff7adeaec73a769788bb78858815c91
10:23 < theStack> jnewbery: sure. basically the idea was to send repeatedly send getcfcheckpt requests to a full node (as it has enough blocks to take a substantial amout of time, compared to e.g. a regtest node) and measure how long it would take -- with and without the cache
10:24 < theStack> i summarized the results here, but they are not very exciting, there doesn't seem to be much difference: https://github.com/bitcoin/bitcoin/pull/18960#issuecomment-628761578
10:24 < theStack> maybe it's my testing approach though :)
10:26 < jnewbery> theStack: thanks for sharing. I wonder how much of the time is parsing/serialization/message sending overhead
10:27 < jnewbery> I think a better test might be to add LogPrint statements before and after the call to ProcessGetCFCheckPt() and then check debug.log
10:28 < theStack> jnewbery: yes the mentioned overhead could definitely distort the result; also the ping part of send_and_ping(...) is included in the time, maybe it would be better to directly wait for the cfcheckpt answer
10:28 < theStack> jnewbery: ah, nice idea with the LogPrint statements!
10:28 < fjahr> testing this in isolation like this guarantees that the leveldb cache will be hit in the version where there is no in memory cache, right? I am not sure if that would always be the case for a node running in the network.
10:29 < fjahr> but probably more likely the more adoption we see, so I guess it's ok?
10:29 < jnewbery> fjahr: right. I think that's where testing it gets tricky. I don't know where other caching happens
10:31 < jnewbery> Perhaps running this on a main net node and periodically sending getcfcheckpt messages is a more realistic test
10:31 < jkczyz> theStack: did you notice much difference between the first and subsequent runs? One difference between the implementations is (3) needs to warm up the cache on the initial fetch
10:32 < jkczyz> I guess (2) does as well
10:33 < jnewbery> jkczyz: right. (2) and (3) need an initial getcfcheckpt request to warm the cache. (1) needs a new block to warm the cache
10:33 < theStack> jkczyz: not really, on the posted result the request for up to block 630000 (i.e. 630 checkpoint headers) takes initially 50.92ms and the average of 99 more subsequent requests is 50.8ms
10:34 < jnewbery> It's possible to modify (3) to automatically fill the cache on startup, but I didn't think it was worth adding the code just to make the first getcfcheckpt response faster
10:35 < jnewbery> theStack: I expect much of that 50ms is overhead. If you think about how the message gets sent from the python test framework to the node, and then added to the peers receive buffer, and then the buffer processed on a messagehanderthread loop, that's got to be a lot of it.
10:36 < theStack> jnewbery: yes, that's also my assumption here
10:36 < jnewbery> There are 100ms sleeps in the messagehandler thread between looping through all peers: https://github.com/jnewbery/bitcoin/blob/7ad83ed252914d6d5b8ed81f103aecf052c68fb7/src/net.cpp#L2061
10:37 < thomasb06> where the corresponding tests are?
10:37 < jnewbery> on a non-busy node, 50ms is the average time you have to wait for the message handler thread to wake up
10:37 < theStack> it's a pity that there is no easy way to just create a node with a huge block count
10:37 < theStack> jnewbery: oh, that explains a lot then
10:38 < jnewbery> thomasb06: https://github.com/jnewbery/bitcoin/blob/7ad83ed252914d6d5b8ed81f103aecf052c68fb7/test/functional/p2p_blockfilters.py#L71
10:38 < thomasb06> thanks
10:38 < jnewbery> theStack: yeah, I only just realised that myself
10:38 < theStack> then i guess your suggested approach with logging directly in the C++ code itself is really the way to go; given that the time resolution in the log files is small enough
10:39 < theStack> (if not, one can sure find another way to directly measure the elapsed time)
10:39 < jnewbery> yes, time resolution in the logging files can be microseconds
10:39 -!- gzhao408 [49fcfb03@c-73-252-251-3.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
10:40 < jnewbery> take a look at https://github.com/jnewbery/bitcoin/blob/7ad83ed252914d6d5b8ed81f103aecf052c68fb7/test/functional/test_framework/test_node.py#L355. Perhaps that can be modified to take timestamps from the debug log.
10:40 < jnewbery> it's a way for the test framework to read the node's debug log during a test
10:43 < jkczyz> Is the cache recommended in BIP 157? I couldn't find it just now but thought I had read about it awhile back
10:43 < jnewbery> ok, final question from me: Which approach do you prefer? Why? 1/2 are acceptable answers :)
10:43 < theStack> jnewbery: good idea. i was thinking about manually analyzing the log in worst case, but of course if that can be automatized as well it'd be awesome
10:43 < jnewbery> jkczyz: I thought I remembered reading it in a bip or somewhere else, but I couldn't see it last time I checked
10:44 < theStack> jkczyz: i don't think BIPs usually describe implementation details about performance improvements in such a detailled manner
10:44 < jkczyz> I like (3) for its simplicity (no need to worry about re-orgs) and because it separates caching from the network layer
10:45 < jnewbery> It doesn't seem to be in the BIP history, so maybe I imagined it
10:45 < jnewbery> ah. Here it is: https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki#node-operation: "They also SHOULD keep every checkpoint header in memory, so that getcfcheckpt requests do not result in many random-access disk reads."
10:46 < thomasb06> talking about node operation, what are filters for?
10:47 < theStack> jnewbery: also prefer (3) for its absolute simplicity! from an algorithmic perspective, the cache access in (1)/(2) happens in O(1) though, while in (3) it's O(log N), but considering the small amount of elements in the cache, this doesn't really matter
10:47 < fjahr> I also prefer (3) for it's simplicity
10:47 < michaelfolkson> Yeah 3 does seem better. Did you write it from scratch jnewbery or did you look at the first two and think there are better ways to do this?
10:48 < michaelfolkson> thomasb06: Filters are for light clients. So they can work out which blocks they need to request from full nodes
10:48 < thomasb06> michaelfolkson: ok
10:49 < nehan> nice property of (3) (i think) is that the cache is only populated if this feature is actually used (requests are made)
10:49 < jnewbery> theStack: BIPs do often go into implementation details that are not strictly necessary to the protocol. I'd argue that the schnoor BIP's section on signing: https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#default-signing, which documents lots of footguns is similar in some ways.
10:51 < jnewbery> michaelfolkson: moving the cache to the indexer was one of my requests in the original BIP: https://github.com/bitcoin/bitcoin/pull/16442#issuecomment-555256244 , so I wanted to implement it to see what it would look like
10:51 < nehan> i had a question about (3): the lock is taken out twice in LookupFilterHeader(). what might have changed between those two locks? https://github.com/bitcoin-core-review-club/bitcoin/commit/7ad83ed252914d6d5b8ed81f103aecf052c68fb7#diff-5ad30723a078a5a4b5f92c9aa0369ac2R412
10:52 < thomasb06> what is a DoS attack, denial of service?
10:52 < nehan> could the header have been inserted somewhere else?
10:52 < michaelfolkson> thomasb06: Yes
10:53 < jnewbery> nehan: you're right that the cache is only populated if it's used in implementation (3). I think that's also true in implementation (2), but not (1), which will always fill the cache when the chain advances
10:53 -!- gzhao408 [49fcfb03@c-73-252-251-3.hsd1.ca.comcast.net] has quit [Remote host closed the connection]     
10:53 < jkczyz> nehan: I think it may be to avoid holding the lock when it is not necessary to do so
10:53 < jnewbery> nehan: that's a great question! Does anyone have any thoughts?
10:54 < thomasb06> michaelfolkson: so what's wrong if nodes generate filters dynamically?
10:54 < jnewbery> when you say 'could the header have been inserted somewhere else?' do you mean in a different thread?
10:54 < nehan> jnewbery: yes
10:54 < theStack> i'd say it's generally a good practice to not hold locks longer than necessary
10:55 < nehan> releasing and taking out a lock again in the same function means the world might have changed out from under you.
10:55 < michaelfolkson> For the benefit of future code changes theStack?
10:56 < sipa> michaelfolkson: to reduce latency of other code waiting on the lock
10:56 < michaelfolkson> Ah ok
10:56 < jnewbery> right. So we'd need to check where LookupFilterHeader() could be called from, which is net_processing (when a getcfcheckpt request is received) or rpc/blockchain (when the getblockfilter RPC is called)
10:56 < jnewbery> so there is the possibility that there are two threads executing this function simultaneously. Is that a problem?
10:57 < nehan> i guess they'd just both put the same thing in the map
10:57 < jnewbery> does 'the world changing out from under us' cause a problem in this case?
10:57 < fjahr> nehan: the operations don't depend on each other so I think it does not matter
10:57 < nehan> fjahr: the operations definitely depend on each other. i'm not sure i understand what you're saying
10:58 < michaelfolkson> thomasb06: I don't know what you mean in this example by "dynamically". What doc are you referring to?
10:58 < nehan> fjahr: it's a get - no return - put paradigm. the thing might have been put in there by another thread
10:59 < thomasb06> michaelfolkson: the Node Operation: "Nodes SHOULD NOT generate filters dynamically on request, as malicious peers may be able to perform DoS attacks by requesting small filters derived from large blocks."
10:59 < jnewbery> nehan: right. So we could have two threads not able to get from the cache and both going through this logic: https://github.com/jnewbery/bitcoin/blob/7ad83ed252914d6d5b8ed81f103aecf052c68fb7/src/index/blockfilterindex.cpp#L405-L410
10:59 < jnewbery> and then both trying to put in the cache
10:59 < jnewbery> so the second thread tries to emplace the same object into the map that's already there
11:00 < jnewbery> which isn't a problem. emplace() is a no-op on the map if there's already something with that key: https://en.cppreference.com/w/cpp/container/map/emplace
11:00 < jnewbery> That was a great question. I think it probably deserves a code comment to explain that.
11:01 < nehan> jnewbery: yes that is a scary code pattern to me
11:02 < michaelfolkson> thomasb06: I think that is explained in the preceding paragraph. Nodes should generate filters and persist them rather than generate them on request
11:02 < jnewbery> There's no harm in locking for the entire function if you think that's better. There's going to be ~0 lock contention
11:02 < fjahr> so the second pattern does not fail depending on the first pattern, that's what I meant
11:02 < jnewbery> since it's only when a P2P message and RPC request arrive at the same time
11:02 < jnewbery> ok, that's time. Thanks for participating, everyone!
11:02 < jnewbery> See you all next week
11:02 < nehan> jnewbery: less cognitive overhead for the next developer who reads the code if you just lock the whole function
11:02 < jkczyz> drawback is you need to hold the lock even when not accessing the cache, unless refactoring and duplicating a little code
11:02 < nehan> thanks!
11:03 < jnewbery> and don't forget to leave your comments/review on the PR
11:03 < jkczyz> thanks, jnewbery!
11:03 < fjahr> Thanks jnewbery!
11:03 < theStack> thanks for hosting jnewbery!
11:03 < Caralie> thank you!
11:03 < troygiorshev> thanks jnewbery
11:03 < jnewbery> #endmeeting
11:03 -!- Caralie [185a5a32@cpe-24-90-90-50.nyc.res.rr.com] has quit [Remote host closed the connection]     
11:03 < michaelfolkson> thomasb06: If they are forced to generate many filters in a short period of time that could crash them
11:03 < michaelfolkson> Thanks jnewbery
11:03 < thomasb06> Thanks jnewbery
11:04 < thomasb06> michaelfolkson: filter refer to blocks in the ledger?
11:04 < theStack> jkczyz: as a compromise, one could only hold the lock if is_checkpoint is true :) but i don't know if that increases readability either
11:05 < thomasb06> *filters
11:05 < michaelfolkson> thomasb06: The filter is what the light client uses to work out what blocks to request from the full node.
11:06 < sipa> uncontended lock grabs are very fast
11:06 < jkczyz> theStack: yeah, that's what I meant about refactoring. Since you would need to duplicate the LookupOne code block
11:07 < thomasb06> michaelfolkson: and the light nodes should ask predefined filters not to flood the full node?
11:07 < michaelfolkson> thomasb06: If the node was serving blocks straight away instead of filters then it wouldn't be a light client. It would be a full node
11:07 < nehan> git(hub) question: how do people keep track of and review different versions of a PR?
11:08 < nehan> i had the three different versions open in different browser tabs and was eyeballing the diffs which was challenging
11:08 < sipa> you can locally diff them
11:09 < thomasb06> michaelfolkson: filters are for light nodes to give informations to other light nodes?
11:09 < nehan> sipa: what's your workflow?
11:09 < michaelfolkson> thomasb06: It is more the full node generates them and persists them so when a light client needs them the full node can provide them. Rather than generating them on request
11:10 < nothingmuch> qi     
11:11 < michaelfolkson> thomasb06: Filters are provided to light clients by full nodes. Afaik light clients don't communicate with each other and there are no plans for them to
11:11 < thomasb06> michaelfolkson: generating them on request allows a DoS on the full node?
11:12 < michaelfolkson> thomasb06: If they haven't generated any and then light clients request them then yeah it sounds like that is a DoS vector
11:13 < thomasb06> michaelfolkson: light clients don't communicate to each other, I missed this point...
11:13 < thomasb06> michaelfolkson: thanks
11:13 < michaelfolkson> thomasb06: No problem
11:15 < theStack> nehan: in this particular case i also reviewed each of them in a different browser tab
11:27 -!- pinheadmz [~pinheadmz@pool-100-33-69-78.nycmny.fios.verizon.net] has quit [Read error: Connection reset by peer]     
11:31 -!- molakala [~sumanth.b@2601:192:4701:87f0:257a:5b91:a1f4:3f48] has joined #bitcoin-core-pr-reviews     
11:32 < ja> nehan: what do you mean by workflow? why not just "git checkout new_pr_commit; git diff old_pr_commit"? is it because you assume that the pr keeps the same logical (not actual) commits and you want to see the diff between the git commits pairwise independently?
11:33 -!- pinheadmz [~pinheadmz@pool-100-33-69-78.nycmny.fios.verizon.net] has joined #bitcoin-core-pr-reviews     
11:42 -!- theStack [~honeybadg@vps1648322.vs.webtropia-customer.com] has quit [Quit: Lost terminal]     
12:30 < nehan> ja: thanks, i'm not sure! yeah i think just things like that. i guess we would have to do that 3-ways with this one
12:32 -!- lightlike [~lightlike@p200300C7EF1BAB0059BDFFF3A040C7D2.dip0.t-ipconnect.de] has left #bitcoin-core-pr-reviews ["Leaving"]     
12:43 < troygiorshev> nehan: if I can throw in my 0.02, KDiff3 is more or less designed for 3-way diffs
12:46 < sipa> git diff can also do 3-way diffs
12:46 < sipa> (i've configured that to be the default)
12:46 -!- TheRec [~toto@drupal.org/user/146860/view] has quit [Read error: Connection reset by peer]     
12:47 < gwillen> there is a command called "git range-diff" that does what I think you want nehan
12:48 < gwillen> it does a diff-of-diffs
12:48 < gwillen> it's fairly new, I'm not sure what version of git you need
12:48 -!- TheRec [~toto@84-75-225-47.dclient.hispeed.ch] has joined #bitcoin-core-pr-reviews     
12:48 -!- TheRec [~toto@84-75-225-47.dclient.hispeed.ch] has quit [Changing host]     
12:48 -!- TheRec [~toto@drupal.org/user/146860/view] has joined #bitcoin-core-pr-reviews     
12:49 < nehan> gwillen: sipa: troygiorshe: thanks! i'll check all of those out. in general i love learning people's commandline/workflow tricks
12:49 < gwillen> I guess it's just one of the various tools you can use for this kind of comparison, but if you specifically want to know "what changed in the latest force-push", I like range-diff for this a lot
12:50 < gwillen> (also, there is a very specific trick for checking out a commit which github still has, but you don't have locally, but which has been force-pushed over so github won't let you fetch it -- I should document it somewhere, perhaps in productivity.md?)
12:50 < nehan> gwillen: wow yeah please do
12:51 < gwillen> the short version is, if you have the commit open with a github ".../commit/..." change "commit" in the url to "commits" which will give you the branch view, then hit the branch dropdown and create a new branch, then you can pull the branch. You have to do this in a fork you can create branches in, but I think you can fork a repo just for this and it still works.
13:30 < instagibbs> 3-way diff takes a bit of eye training and i dont do it enough so I lose the muscle...
13:30 < sipa> see what's simplest, the diff between middle and top, the the diff between middle and bottom
13:31 < sipa> then apply that diff to the other one
13:55 < gwillen> I don't have a working 3-way diff for OS X that I don't hate, suggestions welcome
13:56 < gwillen> instead I just manually diff base-mine and base-theirs
13:59 -!- slivera [~slivera@116.206.228.244] has joined #bitcoin-core-pr-reviews     
13:59 -!- slivera [~slivera@116.206.228.244] has quit [Remote host closed the connection]     
14:05 -!- molakala [~sumanth.b@2601:192:4701:87f0:257a:5b91:a1f4:3f48] has quit [Quit: Leaving]     
14:35 -!- shesek [~shesek@unaffiliated/shesek] has quit [Ping timeout: 265 seconds]     
14:37 -!- shesek [~shesek@185.3.145.28] has joined #bitcoin-core-pr-reviews     
14:37 -!- shesek [~shesek@185.3.145.28] has quit [Changing host]     
14:37 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews     
14:42 -!- shesek [~shesek@unaffiliated/shesek] has quit [Ping timeout: 256 seconds]     
14:43 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 246 seconds]     
14:44 -!- troygiorshev [~troy@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has quit [Ping timeout: 265 seconds]     
14:46 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews     
14:46 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews     
15:01 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has quit [Ping timeout: 258 seconds]     
15:02 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has joined #bitcoin-core-pr-reviews     
15:05 -!- dfmbbtc [~dfmb_@unaffiliated/dfmb/x-4009105] has joined #bitcoin-core-pr-reviews     
15:09 -!- dfmb_ [~dfmb_@unaffiliated/dfmb/x-4009105] has quit [Ping timeout: 260 seconds]     
15:10 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Quit: WeeChat 2.8]     
15:12 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has quit [Quit: ERC (IRC client for Emacs 26.3)]     
15:20 -!- troygiorshev [~troy@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has joined #bitcoin-core-pr-reviews     
15:27 -!- dfmbbtc [~dfmb_@unaffiliated/dfmb/x-4009105] has quit [Quit: Leaving]     
15:50 -!- troygiorshev [~troy@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has quit [Ping timeout: 272 seconds]     
16:00 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Quit: Leaving]     
16:32 -!- gzhao408 [49fcfb03@c-73-252-251-3.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
16:34 -!- sr_gi_ [~sr_gi@183.red-83-34-186.dynamicip.rima-tde.net] has quit [Remote host closed the connection]     
16:48 -!- troygiorshev [~troy@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has joined #bitcoin-core-pr-reviews     
16:57 -!- troygiorshev [~troy@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has quit [Ping timeout: 258 seconds]     
18:02 -!- troygiorshev [~troy@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has joined #bitcoin-core-pr-reviews     
18:23 -!- gzhao408 [49fcfb03@c-73-252-251-3.hsd1.ca.comcast.net] has quit [Remote host closed the connection]     
18:48 -!- gleb [~gleb@cpe-67-244-100-77.nyc.res.rr.com] has quit [Ping timeout: 258 seconds]     
18:57 -!- septerr [4c7ed3c8@c-76-126-211-200.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
18:58 -!- septerr [4c7ed3c8@c-76-126-211-200.hsd1.ca.comcast.net] has quit [Remote host closed the connection]     
19:10 < jnewbery> theStack: nehan: jkczyz: thanks for great input today. I implemented some benchmarking debug logs and updated the PR. I'll make the suggested changes to unordered_map and locking tomorrow
19:58 -!- troygiorshev [~troy@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has quit [Ping timeout: 260 seconds]     
20:13 -!- troygiorshev [~troy@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has joined #bitcoin-core-pr-reviews     
20:20 -!- troygiorshev [~troy@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has quit [Ping timeout: 256 seconds]     
20:59 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds]     
21:01 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews     
21:07 -!- davterra [~dulyNoded@2601:603:4f00:63d0::5] has quit [Quit: Leaving]     
21:23 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds]     
21:24 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews     
22:22 < raj_149> ja: Thanks for the breakpoint() suggestion. Much cleaner than pdb console. It worked like a charm. I have setup gdb in attach mode inside vscode, and now it has became supe easy to step through any function callback into core.
22:23 < ja> good :)
22:51 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Quit: jonatack]     
23:01 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined #bitcoin-core-pr-reviews