--- Log opened Wed Aug 11 00:00:35 2021
00:09 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
00:37 -!- hex17or [~hex17or@gateway/tor-sasl/hex17or] has quit [Ping timeout: 244 seconds]
00:39 -!- hex17or [~hex17or@gateway/tor-sasl/hex17or] has joined #bitcoin-core-pr-reviews
02:20 -!- b10c [uid500648@id-500648.charlton.irccloud.com] has joined #bitcoin-core-pr-reviews
04:21 -!- babasancheti [~babasanch@43.249.232.30] has quit [Quit: Client closed]
04:49 -!- Zenton [~user@user/zenton] has quit [Remote host closed the connection]
07:41 -!- janb [~arjan@home.connected.by.freedominter.net] has joined #bitcoin-core-pr-reviews
08:35 -!- muhblockchain [~muhblockc@user/muhblockchain] has joined #bitcoin-core-pr-reviews
08:49 -!- lightlike [~lightlike@user/lightlike] has joined #bitcoin-core-pr-reviews
09:23 -!- Naiza [~Naiza@180.188.251.211] has joined #bitcoin-core-pr-reviews
09:56 -!- raj [~raj_@2409:4061:28c:fa97:40c0:3b6c:57b7:2a1d] has joined #bitcoin-core-pr-reviews
10:00 < lightlike> #startmeeting
10:00 < emzy> hi
10:00 < lightlike> Hi everyone! Welcome to the PR Review club!
10:00 < schmidty> hi
10:00 < theStack> hi
10:00 < glozow> hi!
10:00 < b10c> hi!
10:00 < janb> hi
10:00 < Naiza> hi!
10:00 < larryruane> hi
10:01 < lightlike> Is anybody here for the first time this week?
10:01 < raj> hi
10:01 -!- observer18 [~observer@cpe-23-242-148-67.socal.res.rr.com] has joined #bitcoin-core-pr-reviews
10:02 < lightlike> Seems to be not the case 
10:02 < lightlike> So, today we'll be looking at #20295 (getblockfrompeer).
10:02 < lightlike> Notes and questions are at the website  (https://bitcoincore.reviews/20295 ) as usual.
10:02 < lightlike> Did you review the PR? (y/n)
10:03 < raj> y
10:03 < larryruane> y
10:03 < janb> n
10:03 < theStack> n
10:03 < emzy> n
10:03 < Naiza> y
10:03 < lightlike> For those who had the chance to take a look, what is your impression? (Concept ACK, approach ACK, tested ACK, or NACK?)
10:04 -!- chunkblob [~chunkblob@cpe-184-153-97-30.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews
10:04 < glozow> y
10:04 < raj> tested ACK. Seems like an useful rpc command. Though might warrant some future improvements. 
10:04 < larryruane> tested ACK, although I don't have a strong understanding of the motivation
10:05 < Naiza> tested ACK, but couldn't understand the real application of the added feature.
10:05 < lightlike> ok, let's begin with the questions - the first one is about the motivation:
10:05 < jnewbery> hi
10:06 < lightlike> What are the possible applications of the new RPC getblockfrompeer
10:06 < raj> lightlike, majorly to get blocks from the orphan chain? 
10:06 < larryruane> the PR mentions testing, but it's unclear to me exactly how it helps with testing
10:06 < b10c> Concept ACK, I see how it's useful for the forkmonitor and other applications that require blocks from stale chains
10:07 < lightlike> raj: I agree, that is one application.
10:07 < larryruane> also the PR mentions the fork monitor https://forkmonitor.info/nodes/btc (which I think the author maintains)
10:07 < glozow> i was a bit unclear on this - is it common for regular users to want data for stale blocks?
10:07 < sipa_> raj: i'll only mention this once, but a per peeve of mine is that blocks in a non-active branch of the chain shouldn't be called orphaned. it's not like they don't have parents...
10:08 < theStack> getting pruned blocks seems to be another application (discussed in one of the comments on top by Fi3 and Sjors)
10:08 < glozow> sipa_: do you call them stale? or?
10:08 < jnewbery> There's a good writeup of stale blocks here: https://bitcoin.stackexchange.com/questions/5859/what-are-orphaned-and-stale-blocks/5869#5869
10:08 -!- thebatzuk [~thebatzuk@200.68.140.8] has joined #bitcoin-core-pr-reviews
10:08 < raj> sipa_, yes thats correct. I should have wrote stale blocks, but i mixed it up with orphan transactions.. :D 
10:09 < glozow> great reason to not call them orphan blocks hoho
10:09 < lightlike> larryruane: Not sure about testing, I think it's more about being able to better observe and analyze what is happening on mainnet with stale blocks.
10:09 < glozow> theStack: oh, getting pruned blocks seems to be a good reason
10:09 < lightlike> One other application that was mentioned was to be able to retrieve old blocks on a pruned node.
10:10 < lightlike> theStack: yes, what you said (missed your post)
10:10 < raj> lightlike, it also seems there should be a way to fetch any blocks from the network (as long as it known to be in the tree). Isn't there any current way to do that? 
10:10 < glozow> wait, if you manually download a pruned block from a peer, do you prune it again immediately if it's old?
10:11 < schmidty> that was my original question as well ^
10:11 < lightlike> raj: Not manually, I don't think so.
10:11 < jnewbery> right, you could imagine having a full node and wanting to fetch a transaction, but the block that it's in has been pruned. You could run this commend to redownload the block, and then call getrawtransaction with the txid and blockhash
10:12 < lightlike> glozow: I'm not sure how quickly it would be pruned.
10:12 < larryruane> glozow: I wondered the same thing, so I fetched an older block (my node is pruned) about an hour ago and it's still there (`getblock` shows it)
10:12 < lightlike> I think at the latest it would be pruned at the next restart.
10:13 < larryruane> jnewbery: that's cool, so it's not just for accessing stale blocks
10:14 < lightlike> next question is about the automatic block download algorithm:
10:14 < lightlike> Where in the codebase does a node make the autonomous decision whether to request a block for download after hearing about it from a peer? What is the main decision criterion?
10:14 < sipa_> but i assume there is no guarantee about how long it'll stay available when in pruning mode?
10:15 < larryruane> lightlike: "... pruned at the next restart" I just restarted my node, and that old block (beyond my prune window) is still there... I think it's because it's now in the `blocks` directory (which is persisted)
10:16 < jnewbery> I think that if we redownloaded a block we'd generally keep it for at least a couple of days. We'd write it to the latest block file (after the tip block), and we'd only prune that file once the tip had been buried by 288 blocks.
10:16 < sipa_> larryruane: normally downloaded blocks are also stored in the blocks directory, and they are subject to pruning
10:17 < larryruane> jnewbery: that makes sense ... i know it never removes blocks from the middle `blk?????.dat` files
10:17 < b10c> jnewbery: uh that makes sense
10:17 < glozow> jnewbery: so pruning is per-file basis?
10:17 < sipa_> yes, pruning is per file
10:17 < jnewbery> the minimum you can set pruning to is 550MB. Rationale is here: https://github.com/bitcoin/bitcoin/blob/77e23ca945030d557559a7391cb8bd368693548c/src/validation.h#L91-L99
10:17 < b10c> on the other hand, if we call getblockfrompeer on many pruned blocks, could we cause the current chain tip to be pruned?
10:18 < theStack> is it already possible now to explicitely keep old blocks on a pruned node? or would this PR enable the first time ever to have "gaps" between blocks?
10:18 < sipa_> theStack: gaps are always possible, because blocks are downloafed out of.order
10:18 < sipa_> and files are deleted when they only contain blocks that are old eno7gh
10:19 < theStack> sipa_: ah, good to know. i was assuming the download order is strictly linear
10:19 < b10c> sipa_: thanks, that answers my question
10:19 < sipa_> that's hard to do when you're downloading from multiple peers in parallel :)
10:19 -!- thebatzuk [~thebatzuk@200.68.140.8] has quit [Ping timeout: 256 seconds]
10:19 < sipa_> downloading is parallel since headers-fire fetching was introduced in 0.10
10:19 < larryruane> sipa_: but isn't it the case that if we advertise ourself as a NETWORK_LIMITED node, we may not send a block even though we have it?
10:20 < sipa_> larryruane: unsure
10:20 < larryruane> (but we still do have access to it locally of course)
10:20 < jnewbery> I may not have got that exactly 100% accurate, but roughly I'd expect the block to stay around for a short while due to the way files are pruned and the minimum prune value
10:20 < larryruane> i thought murch said that a week or 2 ago
10:20 < glozow> larryruane: think so, only send last 288 blocks
10:21 < larryruane> yeah i think it had to do with preventing fingerprinting
10:21 < glozow> per BIP159 https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki
10:21 < jnewbery> larryruane: yes, if we're NODE_NETWORK_LIMITED, we won't provide blocks more than a certain depth: https://github.com/bitcoin/bitcoin/blob/77e23ca945030d557559a7391cb8bd368693548c/src/net_processing.cpp#L1787-L1794
10:22 < jnewbery> *blocks below more than a certain depth
10:22 < lightlike> That's good, if it stays long enough to make analysis via other RPCs possible, it's a valid use case for the RPC imo.
10:22 < glozow> yeah makes sense
10:23 < lightlike> As for the last q: I think the relevant logic is in ProcessHeadersMessage() https://github.com/bitcoin/bitcoin/blob/0b5344b0d18788e011f2d4a279c8c12a29f1428a/src/net_processing.cpp#L2126-L2140
10:24 < jnewbery> ah yes, here's the constant for keeping all files that contain a block within the last 2 days: /** Block files containing a block-height within MIN_BLOCKS_TO_KEEP of ::ChainActive().Tip() will not be pruned. */
10:24 < raj> lightlike, thanks. I was trying but was lost in the maze..  
10:24 < jnewbery> static const unsigned int MIN_BLOCKS_TO_KEEP = 288;
10:24 < larryruane> while testing this PR, i ran `getchaintips` but was surprised that i was not able to fetch any of those stale blocks from my peers (although i didn't try every one) ... maybe those peers just didn't ever consider those blocks to be part of best chain?
10:24 < lightlike> So, if the last header of a chain of headers doesn't have at least as much work as our chain, we wouldn't download the block automatically.
10:24 < jnewbery> oops, meant to paste the link: https://github.com/bitcoin/bitcoin/blob/77e23ca945030d557559a7391cb8bd368693548c/src/validation.h#L87-L88
10:25 < theStack> hope that my question is not too much off-topic, but i wondered (more than once) what is rationale of the 550 MB minimum prune size? i expected more like a multiple of 144 :) (i guess it's correlated with MIN_BLOCKS_TO_KEEP somehow though?)
10:25 < jnewbery> theStack: https://github.com/bitcoin/bitcoin/blob/77e23ca945030d557559a7391cb8bd368693548c/src/validation.h#L91-L99
10:27 < lightlike> larryruane: yes, maybe they never downloaded the blocks either.
10:27 < jnewbery> (it's very conservatively trying to keep at least the most recent 288 blocks and undo data)
10:27 < theStack> jnewbery: thanks! seems i was blind in the past :D
10:27 < larryruane> in case this is helpful to anyone, here's a command you can use to see which of your peers are non-pruning (look for NETWORK): `bitcoin-cli getpeerinfo|jq '.[]|(.id,.servicesnames)'`
10:27 < lightlike> Next q: The first commit collects various Ensure* helper functions and moves them to rpc/server_util.h. What is the purpose of these functions, and why did this PR move them?
10:27 < larryruane> (i love jq)
10:28 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has joined #bitcoin-core-pr-reviews
10:28 < raj> lightlike, this begs the question, if most nodes doesn't download stale blocks, doesn't that effectively reduces usefulness of this rpc?
10:28 < larryruane> lightlike: rather than asserting, if we're handling an RPC, we just want to throw (user error, not internal error)
10:29 < larryruane> but one thing i'm unclear on is why we would sometimes not have those things (like a mempool or a node context)
10:30 < lightlike> raj: yes, there seems to be some manual steps possible (try existing peers, if you don't get the block you want disconnect and them and try others) that could be automated
10:30 < b10c> ray: when the forkmonitor detects two blocks, at least one of their nodes will have the block. And for everyone else it can still be useful to get pruned blocks
10:30 < b10c> detects a stale block*
10:31 < jnewbery> b10c: because at least one of their peers must have sent a header or cmpctblock for the stale block?
10:32 < lightlike> larryruane: yes - also I think it's possible (or will be with increasing modularity) not to have certain parts (mempool, peerman etc.), in which case the specific rpcs don't make sense but an assert would be wrong. 
10:33 < larryruane> there use of `std::any` with those Ensure functions that i don't  understand, but that's probably too deep of a rabbit hole
10:33 < glozow> lightlike: i agree about the automating, could just try all existing peers instead of requiring a nodeid to be specified
10:34 < larryruane> i think the commit to move the Ensure functions fixes a circular dependency, but I don't understand very well
10:34 < lightlike> glozow: yes - I think the author didn't include that functionality on purpose, which is connected to the next question:
10:35 < lightlike> What happens if we request a block from a peer (via getblockfrompeer), but the peer doesn’t have the block either?
10:35 < jnewbery> glozow: if it was automated how long should the node wait before trying the next peer?
10:35 < larryruane> lightlike: by enabling "net" logging, the answer seems to be that the peer just doesn't reply
10:36 < larryruane> (i wonder if it would ban us if we ask too many times?)
10:36 < lightlike> larryruane: correct! This PR uses EnsurePeerman() in rpc/blockchain, which was located in rpc/net before. But rpc/net includes other stuff from rpc/blockchain, hence the circular dependency
10:37 < larryruane> and there's a tool that detects those dependencies, but there's a way to add exceptions, and we'd like to keep the exceptions to a minimum (i think)
10:37 < lightlike> larryruane: yes - there is no NOTFOUND send like it would be for transactions if the peer doesnt have the block.
10:38 < glozow> jnewbery: idk oops
10:38 < raj> Cant the node just say "sorry I dont have the block"?
10:39 < raj> jnewbery, then we would also know how long to wait and then try the next one?
10:39 < lightlike> so that means automation would either send to all peers at the same time (and potentially receive the block many times which would be wasteful), or it would need to define some kind of waiting period after requesting from one peer, which introduces some complications.
10:39 < jnewbery> larryruane: right. Do you know where the logic is to serve blocks to peers?
10:40 < larryruane> net_processing i would say, but i'll have to look it up
10:41 < larryruane> `PeerManagerImpl::ProcessGetBlockData()` is one place
10:42 < jnewbery> larryruane: yes, it's ProcessGetBlockData(). Here's where we drop the request if we don't have the block: https://github.com/bitcoin/bitcoin/blob/77e23ca945030d557559a7391cb8bd368693548c/src/net_processing.cpp#L1768-L1771
10:42 < lightlike> raj: I think it could (thats exactly what NOTFOUND does for transactions), but this is not currently implemented (probably because it is not such a common situation for blocks?)
10:42 < lightlike> Next q: This PR requests a block but adds no additional logic for dealing with the answer to the manual block request. Why is this not necessary?
10:44 < jnewbery> notfound has never been sent in response to a block request. It was added here for transactions: https://github.com/bitcoin/bitcoin/pull/2192/files
10:45 < raj> lightlike, yes that makes sense.. 
10:46 < glozow> "getdata obviously needs a response" ? https://github.com/bitcoin/bitcoin/pull/2192#issuecomment-12540792
10:46 < larryruane> lightlike: ".. not necessary?" because it's seamlessly handled here https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L3711
10:47 < larryruane> it's a cool design that when we receive a block, we don't really care *why* we're receiving it ... we're like, oh cool, here's another block i can learn about!
10:47 < lightlike> larryruane: yes, in particular AcceptBlock() in validation just deals fine with saving the block to disk, but only because we have marked it as requested before (BlockRequested())
10:48 < glozow> larryruane: as long as it's a block we asked for
10:48 < lightlike> which leads to the next q: What would happen if we’d use getblockfrompeer to request a block for which our node didn’t previously know the header?
10:48 < larryruane> glozow: lightlike: +1 thanks didn't know that
10:49 < lightlike> relevant code is https://github.com/bitcoin/bitcoin/blob/0b5344b0d18788e011f2d4a279c8c12a29f1428a/src/validation.cpp#L3421-L3431
10:50 < larryruane> lightlike: again I enabled "net" logging and tried this (by just specifying a nonexistent block hash arg) and it did send it out to our peer... who just ignored it (didn't reply)
10:50 < larryruane> so our node does send it out (even if we don't have it in our block index)
10:51 < lightlike> larryruane: yes -  but that could be because the peer doesnt have the block, not because you didnt know the header.
10:51 < lightlike> i tried it too by extending the functional test.
10:52 < lightlike> But yes, the node sends it out - and if the peer knows about the block it will send it to us anyway - but we will drop it because we didn't call BlockRequested() in this case.
10:53 < jnewbery> lightlike: does validation ignore the block because  force_processing is set to false?
10:53 < jnewbery> (in ProcessNewBlock())
10:55 < lightlike> jnewbery: not sure, I thought AcceptBlock() would ignore it because of the last criterion in https://github.com/bitcoin/bitcoin/blob/0b5344b0d18788e011f2d4a279c8c12a29f1428a/src/validation.cpp#L3421-L3431 (if it has lower work)
10:55 < jnewbery> here's the logic in AcceptBlock() that doesn't save the block if we didn't request it and it isn't part of the best chain: https://github.com/bitcoin/bitcoin/blob/77e23ca945030d557559a7391cb8bd368693548c/src/validation.cpp#L3413-L3424
10:55 -!- RandyMcMillan [~RandyMcMi@47.205.7.87] has joined #bitcoin-core-pr-reviews
10:55 < lightlike> but in this case, I wonder why we even bother requesting it, and don't just return an error for the RPC.
10:56 < jnewbery> haha snap
10:56 < jnewbery> interestingly if that TODO in AcceptBlock is implemented, then I don't think this RPC could ever work
10:56 < jnewbery>     // TODO: Decouple this function from the block download logic by removing fRequested
10:56 < jnewbery>     // This requires some new chain data structure to efficiently look up if a
10:56 < jnewbery>     // block is in a chain leading to a candidate for best tip, despite not
10:56 < jnewbery>     // being such a candidate itself.
10:57 < lightlike> since time is getting low, last q: Can you think of ways to further improve the new RPC command in the future? (some ideas are mentioned in the PR discussion)
10:58 < larryruane> jnewbery: that logic in AcceptBlock() seems to prevent DoS vector, because those checks are very quick (we don't even look at the transactions for example)
10:59 < larryruane> jnewbery: good find, that TODO should be mentioned in the PR!
11:00 < jnewbery> larryruane: I imagine sjors didn't know about it (I didn't know about it until 3 minutes ago)
11:00 < larryruane> I just want to say, the review that jnewbery did on this PR is amazing, I hope I can be half as good someday! If you haven't already, check out the resolved comments
11:00 < lightlike> I think some steps toward automation would make sense, it seems bothersome to request from each peer manually.
11:01 < lightlike> but time's up
11:01 < lightlike> #endmeeting
11:01 < jnewbery> lightlike: I expect that would be quite a lot of code. I don't think it would slot easily into our block request logic
11:01 < larryruane> thank you, lightlike ! that was great!
11:01 -!- RandyMcMillan [~RandyMcMi@47.205.7.87] has quit [Ping timeout: 245 seconds]
11:01 < theStack> thanks for hosting lightlike!
11:01 < b10c> thanks lightlike!
11:01 < glozow> thanks lightlike! very insightful meeting
11:01 < raj> thanks lightlike , nice session.. 
11:01 < emzy> Thanks lightlike!
11:01 < jnewbery> larryruane: aww shucks ❤️
11:01 < janb> thanks
11:01 -!- raj [~raj_@2409:4061:28c:fa97:40c0:3b6c:57b7:2a1d] has quit [Quit: Leaving]
11:01 < jnewbery> lightlike: great meeting. Thank you!
11:02 < jnewbery> next week we have a special meeting on testing the v22 release candidate, hosted by josibake. It'll be a good one!
11:04 < theStack> general question: do we increase misbehaviour score only if a peer actively behaves bad (e.g. by sending messages that don't respect size limits), or is there also the case that we punish nodes that omit information we ask for that they should have? e.g. requesting an old block from a peer announcing NODE_NETWORK should always succeed, otherwise the peer lied (or for whatever other 
11:04 < theStack> reason doesn't want to serve us the block)
11:04 < sipa_> we disconnect peers that time out responsing to block requests
11:04 < sipa_> no dos, just disconnect
11:05 < larryruane> is giving a peer dos points more severe than disconnect? I always forget ... or is disconnect like the ultimate high dos score?
11:06 < theStack> sipa_: okay, thanks
11:06 < theStack> larryruane: since at some dos point score the disconnect happens, i'd say that disconnecting _is_ the ultimate high dos score :)
11:17 -!- Naiza [~Naiza@180.188.251.211] has quit [Quit: Connection closed]
11:17 < sipa_> larryruane: no, they're distinct
11:18 < sipa_> because triggering disconnect through dos score also results in the IP becoming discouraged
11:20 < theStack> so a peer that is disconnected due to not responsing to block requests in time can immediately connect again without long-term consequences?
11:24 -!- Blue_Moon333 [~Blue_Moon@189.149.186.140] has joined #bitcoin-core-pr-reviews
11:35 < sipa_> theStack: indeed
11:35 < sipa_> it's just to prevent downloading the same block twice
12:18 -!- Blue_Moon333 [~Blue_Moon@189.149.186.140] has quit [Quit: Connection closed]
12:32 -!- babasancheti [~babasanch@43.249.232.30] has joined #bitcoin-core-pr-reviews
12:35 -!- babasancheti [~babasanch@43.249.232.30] has quit [Client Quit]
13:15 -!- meshcollider [meshcollid@user/meshcollider] has quit [Remote host closed the connection]
13:33 -!- mdrollette [~mdrollett@user/mdrollette] has joined #bitcoin-core-pr-reviews
13:36 -!- meshcollider [meshcollid@meshcollider.jujube.ircnow.org] has joined #bitcoin-core-pr-reviews
13:41 -!- sipa_ is now known as sipa
14:58 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
15:28 -!- observer18 [~observer@cpe-23-242-148-67.socal.res.rr.com] has quit [Quit: Connection closed]
15:48 -!- hex17or [~hex17or@gateway/tor-sasl/hex17or] has quit [Ping timeout: 244 seconds]
15:51 -!- hex17or [~hex17or@gateway/tor-sasl/hex17or] has joined #bitcoin-core-pr-reviews
15:55 -!- hex17or [~hex17or@gateway/tor-sasl/hex17or] has quit [Remote host closed the connection]
15:56 -!- hex17or [~hex17or@gateway/tor-sasl/hex17or] has joined #bitcoin-core-pr-reviews
15:59 -!- lightlike [~lightlike@user/lightlike] has quit [Quit: Leaving]
16:08 -!- meshcollider [meshcollid@meshcollider.jujube.ircnow.org] has quit [Changing host]
16:08 -!- meshcollider [meshcollid@user/meshcollider] has joined #bitcoin-core-pr-reviews
17:13 -!- b10c [uid500648@id-500648.charlton.irccloud.com] has quit [Quit: Connection closed for inactivity]
20:36 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has quit [Remote host closed the connection]
20:36 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has joined #bitcoin-core-pr-reviews
21:40 -!- belcher_ [~belcher@user/belcher] has joined #bitcoin-core-pr-reviews
21:44 -!- belcher [~belcher@user/belcher] has quit [Ping timeout: 248 seconds]
23:01 -!- belcher_ is now known as belcher
23:23 -!- chunkblob [~chunkblob@cpe-184-153-97-30.nyc.res.rr.com] has quit [Ping timeout: 245 seconds]
--- Log closed Thu Aug 12 00:00:36 2021