--- Log opened Wed May 28 00:00:56 2025
00:18 -!- kevkevin [~kevkevin@209.242.39.30] has quit [Ping timeout: 265 seconds]
00:48 -!- kevkevin [~kevkevin@209.242.39.30] has joined #bitcoin-core-pr-reviews
01:08 -!- kevkevin [~kevkevin@209.242.39.30] has quit [Ping timeout: 265 seconds]
01:35 -!- kevkevin [~kevkevin@209.242.39.30] has joined #bitcoin-core-pr-reviews
01:57 -!- kevkevin [~kevkevin@209.242.39.30] has quit [Ping timeout: 276 seconds]
02:32 -!- kevkevin [~kevkevin@209.242.39.30] has joined #bitcoin-core-pr-reviews
02:33 -!- Holz [~Holz@user/Holz] has quit [Ping timeout: 248 seconds]
02:33 -!- Holz [~Holz@user/Holz] has joined #bitcoin-core-pr-reviews
03:36 -!- kevkevin [~kevkevin@209.242.39.30] has quit [Ping timeout: 244 seconds]
04:08 -!- kevkevin [~kevkevin@209.242.39.30] has joined #bitcoin-core-pr-reviews
04:30 -!- kevkevin [~kevkevin@209.242.39.30] has quit [Ping timeout: 244 seconds]
04:57 -!- kevkevin [~kevkevin@209.242.39.30] has joined #bitcoin-core-pr-reviews
06:59 < jonatack> T-2 hours to this week's Bitcoin Core PR Review club. It is on https://bitcoincore.reviews/32051, "p2p: protect addnode peers during IBD" and I'll be hosting.
07:00 < stickies-v> the meeting is at 5PM UTC, so that should be T-3 hours I think!
07:02 < jonatack> thanks -- did daylight savings time just kick in? hehe
07:03 < jonatack> El Salvador doesn't do DST, so that may explain that I'm off by one
07:03 < jonatack> T-3 hours!
07:03 < stickies-v> that would depend on where you live! review club is scheduled in UTC time, so we don't have DST
07:11 -!- maflcko [~none@107.172.8.183] has quit [Remote host closed the connection]
07:15 -!- maflcko [~none@107.172.8.183] has joined #bitcoin-core-pr-reviews
07:17 -!- pablomartin [~pablomart@2800:2143:20c0:137d::4f90] has quit [Ping timeout: 276 seconds]
07:18 -!- jonatack [~jonatack@user/jonatack] has quit [Ping timeout: 252 seconds]
07:19 -!- maflcko [~none@107.172.8.183] has quit [Client Quit]
07:19 -!- maflcko [~none@107.172.8.183] has joined #bitcoin-core-pr-reviews
07:19 -!- pablomartin [~pablomart@2800:2143:20c0:137d::4f90] has joined #bitcoin-core-pr-reviews
08:22 -!- robszarka [~szarka@2603:3003:4eac:100:e02e:5e2d:e578:d3d4] has joined #bitcoin-core-pr-reviews
08:25 -!- szarka [~szarka@2603:3003:4eac:100:29dd:3f8:a14c:3379] has quit [Ping timeout: 248 seconds]
08:35 -!- jonatack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
09:31 < jonatack> T-30 minutes
09:34 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has joined #bitcoin-core-pr-reviews
09:46 < jonatack> https://x.com/jonatack/status/1927768398630387973
09:46 < jonatack> poll: who introduced the addnode code to bitcoin core?
09:56 -!- stringintech [~stringint@2602:fa59:3:451::1] has joined #bitcoin-core-pr-reviews
09:56 -!- yuvic [~yuvic@103.191.131.242] has joined #bitcoin-core-pr-reviews
09:56 -!- yuvic [~yuvic@user/yuvic] has changed host
09:57 -!- monlovesmango [~monlovesm@154.47.22.79] has joined #bitcoin-core-pr-reviews
09:59 -!- enochazariah [~enochazar@197.210.71.181] has joined #bitcoin-core-pr-reviews
10:00 < jonatack> #startmeeting
10:00 < corebot> jonatack: Meeting started at 2025-05-28T17:00+0000
10:00 < corebot> jonatack: Current chairs: jonatack
10:00 < corebot> jonatack: Useful commands: #action #info #idea #link #topic #motion #vote #close #endmeeting
10:00 < corebot> jonatack: See also: https://hcoop-meetbot.readthedocs.io/en/stable/
10:00 < corebot> jonatack: Participants should now identify themselves with '#here' or with an alias like '#here FirstLast'
10:00 < jonatack> Hi and welcome to this week's review club!
10:00 < stringintech> Hi!
10:00 < monlovesmango> hello :)
10:00 < yuvic> hi
10:00 < jonatack> Today we'll be discussing "p2p: protect addnode peers during IBD"
10:00 < enochazariah> hello
10:00 < jonatack> Review club url:
10:01 < jonatack> https://bitcoincore.reviews/32051
10:01 < jonatack> Bitcoin Core PR url:
10:01 < jonatack> https://github.com/bitcoin/bitcoin/pull/32051
10:01 < jonatack> Anyone here for the first time? Feel free to say hi, even if you're only observing.
10:01 < jonatack> This discussion is ad hoc and asynchronous, so feel free to continue conversation on previous questions when we move on, or raise any other questions or thoughts you have.
10:02 < jonatack> To get the convo warmed up: Anyone know who originally introduced the addnode code into Bitcoin Core?
10:03 < jonatack> stringintech: monlovesmango: yuvic: enochazariah: welcome!
10:04 < monlovesmango> no idea..
10:04 < yuvic> maybe laanwj?
10:04 < enochazariah> i do not know that
10:04 < stringintech> satoshi? seeing him in addnode commits in git log :))
10:05 < jonatack> :D if my memory serves, it was gregory maxwell ("gmax" in the poll at https://x.com/jonatack/status/1927768398630387973)
10:05 < jonatack> So
10:05 < jonatack> an interesting observation, I have found, from out in the field in Central America,
10:05 < jonatack> is that bitcoind in general is quite robust in dealing with a hostile environment of poor or intermittent internet connection
10:05 < jonatack> e.g. where browsing the internet might be painfully slow or no longer really viable, but your bitcoind node survives/thrives quite well
10:06 < jonatack> in contrast
10:06 < jonatack> the stalling/timeout logic seems less tolerant and adapted, to a slow hostile environment
10:06 < jonatack> as it lacks the ability to monitor and adapt accordingly
10:06 < jonatack> which, as noted in the notes, mzumsande was looking at improving
10:07 < jonatack> still, I noticed the disconnections were heavily affecting some of my addnode peers that were being targeted for no fault of their own, some of which were low latency (like cjdns peers) or medium/higher latency (like tor and i2p peers)
10:07 < jonatack> which motivated this pull request.
10:07 < jonatack> Did anyone get the chance to review the notes and/or PR (y/n)?
10:07 < stringintech> y
10:07 < monlovesmango> yes
10:07 < yuvic> y
10:07 < enochazariah> yes
10:07 < jonatack> excellent
10:08 < jonatack> I'll add some bonus questions about the code, but let's begin with the more general ones
10:08 < jonatack> 1. What is an addnode peer? How can you specify them to your node?
10:09 < jonatack> (this is a practical question for anyone who runs a node)
10:09 < yuvic> peer which we manually add or connect, using addnode rpc or -addnode/-connect config.
10:10 < jonatack> yes! and why would you want to do that, as a node runner
10:10 < enochazariah> peer that a node can connect to, it can be speified to the node by using the command line
10:11 < jonatack> right, either via CLI/RPC addnode (onetry for a one-shot attempt, or "add" to add it to the addnode list)
10:11 < monlovesmango> so you can connect to a peer that you know to be honest
10:11 < yuvic> as I trust that peer or also to sync up faster during IBD
10:11 < stringintech> to maintain connections to nodes I trust in case for example the core peer selection fails for some reason
10:11 < enochazariah> I think a reason why someone would want to add node to add more trust in the network, verify don't trust
10:11 < jonatack> or in your bitcoin.conf file with addnode=<peer> ... one per line
10:12 < jonatack> So: to ensure a connection to a *trusted* peer
10:12 < jonatack> What is a fundamental protection that a trusted peer can provide to your node?
10:12 < jonatack> assuming they are an honest peer becaue you know/trust them
10:13 < stringintech> it can help prevent the node from being isolated from the rest of the network (not having the knowledge of the best chain anymore) by malicious peers
10:13 < yuvic> yes to sync with the best chain
10:13 < monlovesmango> would probably provide protection against a sybil attack
10:13 < enochazariah> protection from isolation
10:14 < jonatack> right!
10:14 < jonatack> https://river.com/learn/terms/e/eclipse-attack/
10:14 < jonatack> "An eclipse attack targets particular nodes in a network by surrounding them and obscuring their view of the entire network. For example, if a Bitcoin node has eight connections to other nodes, and an attacker controls all eight of those nodes, the attacker can refuse to relay any new blocks that miners produce. Although the rest of the network continues to process new blocks, the
10:14 < jonatack> victim node will be unaware that blocks are coming in."
10:14 < jonatack> It only takes one single honest peer connection to break out of an eclipe attack
10:14 < monlovesmango> ah so there is a term for it hah
10:15 < jonatack> yes
10:15 < yuvic> yes eclipse attack
10:15 < jonatack> So, by adding a trusted peer, your node cannot be successfully eclipsed unless your addnode peer connections are also eclipsed
10:17 < stringintech> then if we are choosing more than one addnode peer, they should be also geographically diverse so that for example if one region is compromised, our node can still maintain connections to rest of the network...
10:17 < jonatack> Therefore, it's a good idea to add some trusted peers using the addnode config option or rpc/cli 
10:17 < jonatack> stringintech: sgtm
10:17 < jonatack> You can have up to 8 addnode peer connections simultaneously
10:18 < jonatack> In addition to the limit of 8 autamatic full outbound conns and 2 block-relay-only ones
10:18 < jonatack> (along with transient connections, like feelers or extra block-relay-only conns
10:19 < jonatack> or addr_fetch ones)
10:19 < jonatack> see src/node/connection_types.h for details
10:19 < stringintech> Thanks for the details
10:19 < jonatack> Now, onto the code
10:20 < jonatack> What function contains the Bitcoin Core stalling and timeout logic?
10:20 < stringintech> PeerManagerImpl::SendMessages()
10:20 < jonatack> this wasn't a question to prepare in advance, but you can search the codebase right now if you like
10:20 < enochazariah> SendMessages
10:20 < enochazariah> in PeerManagerImpl
10:20 < jonatack> excellent
10:20 < yuvic> SendMessages
10:21 < jonatack> From where is SendMessages() called?
10:21 < jonatack> (who it its caller)
10:21 < stringintech> connection manager
10:22 < jonatack> stringintech: can you elaborate?
10:22 < jonatack> it is called from a thread, currently inside src/net.cpp
10:23 < stringintech> I should go back for source for detail
10:23 < stringintech> but I guess we would loop over peers
10:23 < stringintech> can call this after ProcessMessages
10:23 < enochazariah> Not sure, but i think THe ThreadMessageHandler is the method that calls the SendMessages
10:23 < jonatack> correct for both
10:23 < enochazariah> *SendMessage
10:23 < jonatack> void CConnman::ThreadMessageHandler()
10:24 < jonatack> that calls ProcessMessages() and then SendMessages() for each peer, if not flagged for disconnection
10:25 < jonatack> Now, on to the PR
10:25 < jonatack> I was pleasantly surprised by pinheadz's review here https://github.com/bitcoin/bitcoin/pull/32051#pullrequestreview-2864676242
10:26 < jonatack> "I believe this also closes an 11-year-old issue: https://github.com/bitcoin/bitcoin/issues/5097"
10:27 < yuvic> yes that was interesting
10:27 < jonatack> and also:
10:27 < jonatack> https://github.com/bitcoin/bitcoin/issues/9213
10:27 < jonatack> that I need to read and look into TBH
10:27 < stringintech> Nice!
10:28 -!- Talkless [~Talkless@138.199.6.197] has joined #bitcoin-core-pr-reviews
10:28 < jonatack> (as well as the review comments in https://github.com/bitcoin/bitcoin/pull/25880)
10:29 < jonatack> Back to this PR
10:29 < jonatack> The PR as it is currently, is actually not well-named
10:29 < jonatack> because only the first commit affects the IBD issues that I observed
10:29 < jonatack> that first commit gives addnode peers more time. apart from that, it doesn't protect them from disconnection.
10:30 < stringintech> I had a question: Is the PR intentionally focusing on “-addnode” peers and not “-connect” peers?
10:30 < jonatack> I'd be curious to hear if you have any thoughts or suggestions on the changes
10:31 < jonatack> stringintech: good question. Yes, I was focusing on addnode peers.
10:31 < monlovesmango> have you been able to test disconnection frequency after increasing timeout allowed?
10:31 < jonatack> Making a note to verify the effect on a -connect peer. 
10:32 < jonatack> monlovesmango: yes, I saw much fewer disconnections with the ping times of my peers with my internet speed
10:33 < jonatack> from 100 or more per minute to a few an hour
10:33 < monlovesmango> nice
10:33 < jonatack> it still took more than a month for that node to sync...
10:34 < yuvic> I had a similar question as mzumsande's comment on the pr -> https://github.com/bitcoin/bitcoin/pull/32051/commits/3463a7f4813c3eece5ba9a260670a76e3f8d38ab#r1999313868
10:34 < jonatack> like 5 weeks, but all those addnode disconnections were not helping, as those peers were being re-connected right away again afterward
10:34 < monlovesmango> if it isn't actually protect them from disconnection then I would say I concept ACK and approach ACK. but if it is protecting from disconnection then I think there needs to be more thought into what desired IBD behavior would be
10:34 < monlovesmango> jonatack: omg hahah
10:36 < jonatack> yuvic: yes, the high frequency of disconnections I was seeing during IBD were not of that logic 
10:37 < jonatack> those are disconnections that occur infrequently (for me) after IBD is completed
10:38 < yuvic> got it!
10:38 < jonatack> I need to consider whether to potentially drop the second (and maybe third) commit to keep it focused on IBD only, when the high number of disconnections I was seeing took place
10:39 < jonatack> or, alternatively, try to implement his review suggestion to clear the block requests
10:39 < enochazariah> I've got a bit of a question
10:39 < enochazariah> does this not raise up a silent stall? i mean, if the system does not have the inhererent mechanism to re-request that block from another peer, then the IBD could effectively stall, preventing a disconnection, but introducing a silent stall
10:40 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has quit [Quit: grettke]
10:41 < jonatack> enochazariah: I need to look at that (test coverage could be useful, as well, but maybe non-trivial to do, and only if it is reliable)
10:42 < enochazariah> okay
10:42 < jonatack> The review comments turned up valuable history on this that I need to review.
10:43 < jonatack> I didn't necessarily expect the PR to be merged quickly as-is, but hoped to gain insight as to what would be best.
10:44 < jonatack> A long-term goal since years that comes up now and then in developer discussions
10:44 < jonatack> is how to score peers based on the resources they consume
10:45 < jonatack> see, for instance: https://github.com/bitcoin/bitcoin/pull/31672
10:45 < yuvic> there was an issue for this
10:45 < yuvic> yes by vasild
10:46 < jonatack> yuvic: yes. how to measure this an open question.
10:47 < jonatack> i'm not sure how useful the cpu load is, as I have been testing it, and the load seems to often be higher when the peer is first connected, and then go down to normal levels 
10:47 < jonatack> and not necessarily indicate a bad peer
10:47 < jonatack> (to be continued)
10:48 < jonatack> i haven't seen it yet as useful -- on its own -- to qualify a peer to be disconnected
10:49 < jonatack> this perhaps connects to aj towns' review suggestion about scoring peers and using that to scale the number of blocks we request from them
10:50 < jonatack> If anyone comes up with test coverage for the stalling or timeout logic here, I'd be very happy to look at it and bring it into the PR
10:51 < enochazariah> scoring them and using as an order, so a much higher score would mean higher chances of being requested
10:51 < jonatack> right now, the changes in this PR do not break any tests...
10:52 < stringintech> Regarding the timeout logics this PR touches, I could only find p2p_ibd_stalling.py, which covers the IBD block stalling timeout (and should possibly be adapted to reflect the addnode changes). I didn't find any integration tests for the initial header sync timeout and regular block download timeout. Am I right??
10:52 < yuvic> yes, test coverage would be interesting
10:53 < jonatack> stringintech: neat, maybe addnode connections could be added to that functional test file
10:54 < jonatack> stringintech: as for the header sync and block download, I have not yet looked
10:55 < stringintech> Hmm... I'd be happy to work on the missing ones (regardless of the PR changes) and open a PR (in case it is out of the scope for this PR of course). If it merges first, the addnode PR could adapt them accordingly too.
10:55 < stringintech> Have to double check to see if they are actually missing
10:55 < jonatack> stringintech: that would be great!
10:56 < jonatack> please ping me (on the github PR, or via IRC or DM on twitter/x) if you come up with coverage
10:56 < enochazariah> stringintech that would be nice
10:57 < jonatack> any final thoughts or questions?
10:57 < jonatack> 2 minutes left
10:58 < yuvic> thanks, nothing from my side!
10:58 < enochazariah> Nothing from my end
10:58 < jonatack> Appreciate you all participating! 
10:59 < jonatack> Don't hesitate to leave a review comment or feedback on that PR or propose test coverage or improvements to it there
10:59 < enochazariah> Thank you jonatack
10:59 < stringintech> Thank you jonatack!
10:59 < jonatack> Thank you!
10:59 < jonatack> #endmeeting
10:59 < corebot> jonatack: Meeting ended at 2025-05-28T17:59+0000
10:59 < corebot> jonatack: Raw log: https://achow101.com/ircmeetings/2025/bitcoin-core-pr-reviews.2025-05-28_17_00.log.json
10:59 < corebot> jonatack: Formatted log: https://achow101.com/ircmeetings/2025/bitcoin-core-pr-reviews.2025-05-28_17_00.log.html
10:59 < corebot> jonatack: Minutes: https://achow101.com/ircmeetings/2025/bitcoin-core-pr-reviews.2025-05-28_17_00.html
10:59 < yuvic> Thank you jonatack
11:00 -!- yuvic [~yuvic@user/yuvic] has quit [Quit: yuvic]
11:03 -!- stringintech [~stringint@2602:fa59:3:451::1] has quit [Quit: Client closed]
11:09 -!- enochazariah [~enochazar@197.210.71.181] has quit [Quit: Client closed]
11:15 -!- enochazariah [~enochazar@197.210.71.181] has joined #bitcoin-core-pr-reviews
11:20 -!- monlovesmango [~monlovesm@154.47.22.79] has quit [Ping timeout: 252 seconds]
11:34 -!- enochazariah [~enochazar@197.210.71.181] has quit [Quit: Client closed]
12:04 -!- sliv3r__ [~sliv3r__@user/sliv3r-:76883] has quit [Quit: ZNC 1.8.2+deb3.1+deb12u1 - https://znc.in]
12:05 -!- sliv3r__ [~sliv3r__@user/sliv3r-:76883] has joined #bitcoin-core-pr-reviews
12:29 -!- Talkless [~Talkless@138.199.6.197] has quit [Quit: Konversation terminated!]
12:30 -!- joetor5 [~joetor5@syn-070-119-066-121.res.spectrum.com] has joined #bitcoin-core-pr-reviews
12:31 -!- joetor5 [~joetor5@user/joetor5] has changed host
12:34 -!- joetor5 [~joetor5@user/joetor5] has quit [Remote host closed the connection]
12:45 < jonatack> git show e66ec79b18717bf83b7dbbe54f844b4463dabdeb
12:46 < jonatack> stringintech was right; satoshi referred to -addnode in that commit
13:35 -!- jon_atack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
13:38 -!- jonatack [~jonatack@user/jonatack] has quit [Ping timeout: 265 seconds]
13:38 -!- jonatack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
13:41 -!- jon_atack [~jonatack@user/jonatack] has quit [Ping timeout: 268 seconds]
13:49 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has joined #bitcoin-core-pr-reviews
13:57 -!- robszarka [~szarka@2603:3003:4eac:100:e02e:5e2d:e578:d3d4] has quit [Quit: Leaving]
13:57 -!- szarka [~szarka@2603:3003:4eac:100:e02e:5e2d:e578:d3d4] has joined #bitcoin-core-pr-reviews
14:43 -!- madiqq82 [~madiqq83@user/madiqq83] has joined #bitcoin-core-pr-reviews
14:48 -!- madiqq82 [~madiqq83@user/madiqq83] has quit [Remote host closed the connection]
14:48 -!- madiqq83 [~madiqq83@user/madiqq83] has joined #bitcoin-core-pr-reviews
15:39 -!- madiqq83 [~madiqq83@user/madiqq83] has quit [Ping timeout: 272 seconds]
16:53 -!- instagibbs [~instagibb@pool-100-15-116-202.washdc.fios.verizon.net] has quit [Quit: Ping timeout (120 seconds)]
16:54 -!- instagibbs [~instagibb@pool-100-15-116-202.washdc.fios.verizon.net] has joined #bitcoin-core-pr-reviews
21:11 -!- robszarka [~szarka@2603:3003:4eac:100:e02e:5e2d:e578:d3d4] has joined #bitcoin-core-pr-reviews
21:13 -!- rszarka [~szarka@2603:3003:4eac:100:e02e:5e2d:e578:d3d4] has joined #bitcoin-core-pr-reviews
21:14 -!- szarka [~szarka@2603:3003:4eac:100:e02e:5e2d:e578:d3d4] has quit [Ping timeout: 248 seconds]
21:16 -!- robszarka [~szarka@2603:3003:4eac:100:e02e:5e2d:e578:d3d4] has quit [Ping timeout: 248 seconds]
21:34 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has quit [Quit: grettke]
21:34 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has joined #bitcoin-core-pr-reviews
21:36 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has quit [Client Quit]
21:36 -!- PaperSword [~Thunderbi@securemail.qrsnap.io] has quit [Quit: PaperSword]
23:55 -!- kevkevin [~kevkevin@209.242.39.30] has quit [Remote host closed the connection]
--- Log closed Thu May 29 00:00:57 2025