--- Log opened Wed Feb 15 00:00:45 2023
00:08 -!- znow_ [~codeautis@user/codeautist] has quit [Quit: leaving]
00:23 -!- codo [~codeautis@user/codeautist] has joined #bitcoin-core-pr-reviews
03:56 -!- MrFrancis [~MrFrancis@2001:8a0:fa4c:901:17b:8d8:97fb:b3d7] has joined #bitcoin-core-pr-reviews
03:56 -!- MrFrancis [~MrFrancis@2001:8a0:fa4c:901:17b:8d8:97fb:b3d7] has quit [Client Quit]
04:51 -!- ___nick___ [~quassel@82.9.109.62] has joined #bitcoin-core-pr-reviews
05:01 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:9c54:b7e5:8572:ce2a] has quit [Remote host closed the connection]
05:01 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:9c54:b7e5:8572:ce2a] has joined #bitcoin-core-pr-reviews
05:03 -!- ___nick___ [~quassel@82.9.109.62] has quit [Quit: https://quassel-irc.org - Chat comfortably. Anywhere.]
05:05 -!- ___nick___ [~quassel@cpc68289-cdif17-2-0-cust317.5-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews
05:06 -!- ___nick___ [~quassel@cpc68289-cdif17-2-0-cust317.5-1.cable.virginm.net] has quit [Client Quit]
06:58 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:9c54:b7e5:8572:ce2a] has quit [Remote host closed the connection]
06:58 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:9c54:b7e5:8572:ce2a] has joined #bitcoin-core-pr-reviews
07:01 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
07:17 -!- Zenton [~user@user/zenton] has joined #bitcoin-core-pr-reviews
07:59 -!- tvpeter [~tvpeter@102.88.35.38] has joined #bitcoin-core-pr-reviews
08:04 -!- tvpeter [~tvpeter@102.88.35.38] has quit [Quit: Connection closed]
08:04 -!- tvpeter [~tvpeter@102.88.35.28] has joined #bitcoin-core-pr-reviews
08:19 -!- tvpeter [~tvpeter@102.88.35.28] has quit [Quit: Connection closed]
08:20 -!- tvpeter [~tvpeter@102.88.35.38] has joined #bitcoin-core-pr-reviews
08:22 -!- tvpeter [~tvpeter@102.88.35.38] has quit [Client Quit]
08:32 -!- dzxzg [~dzxzg@166.198.34.4] has joined #bitcoin-core-pr-reviews
08:48 -!- hernanmarino [~hernanmar@181.99.169.107] has joined #bitcoin-core-pr-reviews
08:49 < brunoerg> 10 minutes to begin! 
08:52 -!- b_101_ [~robert@189.236.53.233] has joined #bitcoin-core-pr-reviews
08:54 -!- b_101 [~robert@185.242.5.35] has quit [Ping timeout: 255 seconds]
08:55 < hernanmarino> :)
08:56 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has joined #bitcoin-core-pr-reviews
08:57 -!- b_101 [~robert@185.242.5.35] has joined #bitcoin-core-pr-reviews
08:57 -!- grettke [~grettke@184.55.131.155] has joined #bitcoin-core-pr-reviews
08:59 -!- b_101_ [~robert@189.236.53.233] has quit [Ping timeout: 246 seconds]
09:00 < brunoerg> #startmeeting
09:00 < LarryRuane> hi!
09:00 < brunoerg> hi, everyone! 
09:00 < brunoerg> feel free to say hi! :)
09:00 -!- d33r_gee [~d33r_gee@45-27-31-99.lightspeed.sntcca.sbcglobal.net] has joined #bitcoin-core-pr-reviews
09:00 < codo> hi
09:00 < d33r_gee> hello
09:00 < svav> Hi
09:00 < hernanmarino> Hi Bruno and everyone 
09:00 -!- pablomartin [~pablomart@78-157-209-155.as42831.net] has joined #bitcoin-core-pr-reviews
09:00 < brunoerg> anyone here for the first time?
09:01 < effexzi> Hi every1 
09:01 < pablomartin> hello!
09:01 < brunoerg> Today we’re gonna see: https://bitcoincore.reviews/26441
09:02 < brunoerg> We will discuss the questions but feel free to ask/discuss anything else during it.
09:02 < glozow> hi
09:02 < dzxzg> hi
09:03 < brunoerg> hi, @glozow and @dzxzg
09:03 < brunoerg> so, let's begin!
09:03 < brunoerg> Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?
09:04 < LarryRuane> I just started reviewing, did not get to the point of understanding even basic concepts
09:04 < svav> I read the notes
09:04 < brunoerg> Nice! 
09:04 < brunoerg> So, what does this PR do?
09:05 < hernanmarino> read the code, approach ACK . I have a couple of doubts regarding the questions, that I'm hoping to clear on this chat :)
09:05 < svav> It adds whitelisting for outbound connections
09:05 < hernanmarino> It implements a new RPC command to whitelist connections 
09:05 < brunoerg> feel free to ask anything, hernanmarino!
09:06 -!- roze_paul [~quassel@142.243.254.224] has joined #bitcoin-core-pr-reviews
09:06 < hernanmarino> brunoerg: will do later when we get there :)
09:06 < brunoerg> svav and hernanmarino you're right, but I'd say this PR does both!
09:06 < roze_paul> hi
09:06 < hernanmarino> yes
09:07 < brunoerg> ok, so this PR adds whitelisting for outbound connections as well as implements a new RPC command for it
09:07 < brunoerg> let's go to the question n3 to understand some motivations
09:07 < brunoerg> -whitelist only allows to add permission flags to inbound peers. Why only for inbound ones? Does it make sense to extend the permissions to outbound peers? Why?
09:08 < hernanmarino> for the first question, i believe it's because you have more risk of being victim of an attack
09:09 < LarryRuane> could someone first summarize for me, if not too difficult, what the concept of whitelisting is all about?
09:09 < dzxzg> https://github.com/bitcoin/bitcoin/pull/17167
09:09 < dzxzg> Vasild offers one example of a use case: "This would allow us to use transient (aka one-time / random / disposable) I2P addresses when making outgoing connections to all nodes, except to whitelisted ones for which we would use our persistent I2P address."
09:10 < brunoerg> what kind of attack, hernanmarino? and why?
09:10 < svav> whitelisting is just saying connections from or to a given IP address is ok
09:10 < svav> a whitelist is a list of IP addresses usually
09:11 < brunoerg> LarryRuane: giving some permissions for peers
09:11 < LarryRuane> sorry, but why would a connection to or from a given IP *not* be okay?
09:12 < glozow> I imagine the answer to the first question is that the expected use case = manually connecting one node to another, so you `addnode` on one (which initiates a manual) and `whitelist` on the other (which receives an inbound).
09:12 < hernanmarino> perhaps I'm thinking out loud , and there are other reasons, but the simplest form of attack I can think of are eclipse attacks, where the attacker chooses to isolate you 
09:13 < brunoerg> you can whitelist a peer to immune it to DoS banning, for example.
09:13 -!- b_101 [~robert@185.242.5.35] has quit [Read error: Connection reset by peer]
09:13 -!- kevkevin [~kevkevin@c-98-226-43-69.hsd1.il.comcast.net] has joined #bitcoin-core-pr-reviews
09:13 < brunoerg> or all transactions they broadcast are always relays
09:14 < brunoerg> glozow: it makes sense
09:14 < LarryRuane> this helps me a lot, https://github.com/bitcoin/bitcoin/blob/master/src/net_permissions.h#L18
09:16 < brunoerg> about the other part of the question: does it make sense to extend the permissions to outbound peers?
09:16 < glozow> is this including manual connections? or only automatic outbound?
09:16 -!- b_101 [~robert@185.242.5.35] has joined #bitcoin-core-pr-reviews
09:17 < roze_paul> @brunoerg Well an inbound peer could always overwrite an outboundpeers permissions
09:17 < brunoerg> glozow: I think it includes manual as well
09:18 < LarryRuane> brunoerg: the answer must be yes, but I'm not sure why... our outbound peers typically are chosen randomly from addrman, which are gossiped to us, so could be anything .. so might make sense to treat certain addresses in a special way? (but I'm not sure)
09:19 < hernanmarino> brunoerg : this was one of the question I had doubts with, but i guess the PR author can tell us a little bit about it :)
09:20 < LarryRuane> I see this comment https://github.com/bitcoin/bitcoin/pull/10594#issue-236018335 but it doesn't explain why
09:21 < brunoerg> Well, we have different permissions: bloomfilter, relay, forcerelay, download, noban, mempool and addr
09:22 < brunoerg> I could use noban with outgoing peers to speed up relay
09:23 < roze_paul> also thinking outloud here: an outbound whitelist -mempool between two miners in a pool could ensure they  had the same mempool, which is something they could want, if only to ensure equal state with one another
09:24 < hernanmarino> yes, noban is the first use case that came to my mind for outgoing whitelisting
09:25 < LarryRuane> so a node operator might give noban permission to an address that it knows is a "good" peer? Like, maybe this node operator also runs it?
09:25 < brunoerg> LarryRuane: yes! 
09:27 -!- b_101_ [~robert@189.236.53.233] has joined #bitcoin-core-pr-reviews
09:28 < brunoerg> I think we can go to the next question but we can continue discussing that one
09:28 < lightlike> would this make sense for automatic outbound peers? with thousands of possible peers in the network it seems unlikely that we'll connect to any given peer again anytime soon, so would permanent permissions make sense?
09:28 < pablomartin>  +1 roze_paul
09:28 < brunoerg> Considering we already have the -whitelist startup option, why would an RPC be useful? What do we want to avoid?
09:28 < brunoerg> > would this make sense for automatic outbound peers?
09:29 < brunoerg> I don't believe permanent permissions make sense, perhaps this is one of the motivations for the RPC
09:30 -!- b_101 [~robert@185.242.5.35] has quit [Ping timeout: 260 seconds]
09:30 < brunoerg> thinking about it, could make sense add a "remove" option in the RPC
09:31 < roze_paul> Right now to change the whitelist Core must reboot. With an RPC, the whitelist can update 'onthefly'
09:31 < pablomartin> brunoberg: yeah, and an update?
09:31 < brunoerg> roze_paul: perfect! 
09:31 < LarryRuane> the permissions set by this new RPC are not persisted to disk, right? (they're lost if a restarts happens?)
09:32 < brunoerg> LarryRuane: you're right
09:32 < LarryRuane> (pretty sure, just wanted to double-check)
09:32 < LarryRuane> could this new RPC possibly also be useful for testing?
09:32 < roze_paul> ..but they can be added to the config.file?
09:32 < brunoerg> LarryRuane: yes, we're gonna see it in the last question
09:33 -!- b_101 [~robert@185.242.5.35] has joined #bitcoin-core-pr-reviews
09:33 < LarryRuane> brunoerg: thanks, sorry, i'm under-prepared today!
09:33 < brunoerg> LarryRuane: don't worry! 
09:34 < brunoerg> but yes, the motivation is basically to be able to manage the permissions without having to restart our node.
09:34 -!- b_101_ [~robert@189.236.53.233] has quit [Ping timeout: 246 seconds]
09:35 < brunoerg> any other question?
09:36 < brunoerg> so, let's go the next one!
09:36 < brunoerg> This PR adds a ConnectionDirection parameter in TryParsePermissionFlags to control whether it will apply the permissions to inbound or outbound connections. In netbase.h,ConnectionDirection has 2 operators overloading. Could you explain how Operator Overloading in C++ works and how it has been used in ConnectionDirection?
09:39 < brunoerg> Anyone wants to explain Operator Overloading?
09:39 < roze_paul> Operator overloading is the usage of custom-operator definitions for custom classes. IIUC a custom  class will not be able to, for instance, add two values together, even if they appear to be integers, unless that effect is defined in the class setup...
09:39 < roze_paul> for the second part, i believe the connectiondirection[class?] uses standard bitmasking
09:39 < hernanmarino> A quick read (as of now) suggests & and | operations for easy configuration of combinations of permissions
09:39 < roze_paul> ie around line 32. 1U <<0 1U << 1 etc is just bitmasking operations
09:39 < roze_paul> oh am i way off the mark?
09:40 < brunoerg> roze_paul: you're right
09:40 < LarryRuane> if we have a `ConnectionDirection` variable, we can use a natural syntax to "add" another connection direction to it ... even though this is a `class enum`
09:40 < hernanmarino> sorry not permissions, directions 
09:40 < brunoerg> LarryRuane: nice! 
09:41 -!- dzxzg [~dzxzg@166.198.34.4] has quit [Quit: Client closed]
09:41 < LarryRuane> if you use just plain `enum` rather than `class enum`, you can treat variables and expressions as just normal integers... but using `class enum` has advantages and is the more "modern" way to do enumerations
09:41 < LarryRuane> "plain" enums are the same as C (inherited into c++ from c)
09:42 < brunoerg> Great explanation
09:43 < LarryRuane> you could also have defined `+` or maybe `+` instead of `|` (or `|=`) ... did y ou consider that?
09:43 < brunoerg> no, but i'm gonna take a look at it, can make sense
09:43 < LarryRuane> i guess the "or" semantic is better though, because if the variable already has a direction, and you "add" the same one to it, it shouldn't change
09:44 < brunoerg> and then we avoid an unnecessary change
09:44 < LarryRuane> i.e. if variable has `In` and you want to make sure it has `In` then i guess "or" is better
09:44 -!- dzxzg [~dzxzg@166.198.34.4] has joined #bitcoin-core-pr-reviews
09:45 < hernanmarino> I like boolean operators more 
09:45 < LarryRuane> hernanmarino: +1 as i think about it more, i agree
09:45 < brunoerg> about question n6: ConnectionDirection can be Both, In, Out or None. What happens in TryParsePermissionFlags if it is None? In which scenarios can this happen?
09:49 < brunoerg> Basically, if we don't specify "in" or "out", the permissions will be applied for inbound or outbound peers? or both? 
09:50 < LarryRuane> looks like only In
09:50 < LarryRuane> (to be backward compatible?)
09:50 < brunoerg> perfect! 
09:50 < brunoerg> if we don't specify the direction, it will apply only for In
09:52 < brunoerg> to keep it backward compatible
09:52 < brunoerg> ok, let's jump in next question
09:52 < brunoerg> In the addpermissionflags RPC we receive an array of permission flags and the IP (or network). However, we convert it to a string of the following format: “<[permissions@]IP address or network>”. Why?
09:53 < codo> So it can be parsed by the same function that parses for -whitelist?
09:53 < brunoerg> codo: yes!
09:53 < hernanmarino> cool
09:54 < brunoerg> it's simpler to convert it to a string of that format than changing the whole function that makes the validation.
09:55 < brunoerg> `TryParsePermissionFlags` is built to handle with strings like [permissions]@ip
09:55 < brunoerg> because this is how -whitelist works
09:56 < brunoerg> -whitelist=[permissions]@ip
09:56 < brunoerg> right?
09:57 < brunoerg> ok, let's discuss the last question
09:57 < brunoerg> How could this PR avoid the “problem” presented in #26970?
09:59 < hernanmarino> By allowing for easier testing ?
09:59 < brunoerg> Yes, it could speed up our tests
09:59 < LarryRuane> we could whitelist 127.0.0.1 in the outbound direction also?
09:59 < brunoerg> See that 'In the functional test wallet_groups.py we whitelist peers on all nodes (-whitelist=noban@127.0.0.1) to enable immediate tx relay for fast mempool synchronization'
09:59 < svav> It's something to do with making outbound connections faster
10:00 < brunoerg> but it doesn't work because -whitelist only works for inbound peers
10:01 < LarryRuane> brunoerg: that's cool, just wondering, have you tried it (undo the 26970 fix)?
10:01 < brunoerg> not yet, it's on my plan
10:01 < brunoerg> but other reviewers pointed that it could be a fix/improvement
10:01 < brunoerg> #endmeeting
10:02 < d33r_gee> tahnks brunoerg and every1
10:02 < brunoerg> thanks, everyone!
10:02 < svav> Thanks brunoerg and all!
10:02 < brunoerg> it was a pleasure
10:02 < hernanmarino> thanks Bruno !
10:02 < codo> thanks brunoerg 
10:02 -!- d33r_gee [~d33r_gee@45-27-31-99.lightspeed.sntcca.sbcglobal.net] has quit [Quit: Connection closed]
10:02 < roze_paul> thanks bruno!
10:03 < brunoerg> LarryRuane: I'm testing it now and seems work
10:03 < LarryRuane> thanks @brunoerg and everyone else! ... cool!
10:03 < brunoerg> some ppl suggested to me to split this PR into 2, one for outgoing stuff and other for the RPC
10:05 < brunoerg> so we could have the outgoing one first and use it in the functional test
10:05 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has quit [Quit: Connection closed]
10:05 < pablomartin> thanks Bruno!
10:05 < hernanmarino> I don't know about that, but might help some reviewers in favor of only one of both functionalities 
10:06 < brunoerg> sure! @hernanmarino
10:07 < LarryRuane> just to confirm my understanding, have the outgoing functionality but at first only available as a config option? (then RPC in a second PR?)
10:09 < hernanmarino> LarryRuane: that is what I understand as well
10:10 < glozow> thank you brunoerg!
10:10 < brunoerg> LarryRuane: yes!
10:12 -!- roze_paul [~quassel@142.243.254.224] has quit [Quit: https://quassel-irc.org - Chat comfortably. Anywhere.]
10:24 -!- pablomartin [~pablomart@78-157-209-155.as42831.net] has quit [Ping timeout: 248 seconds]
10:32 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:9c54:b7e5:8572:ce2a] has quit [Remote host closed the connection]
10:34 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:9c54:b7e5:8572:ce2a] has joined #bitcoin-core-pr-reviews
10:36 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
11:04 -!- instagibbs_ [~instagibb@pool-100-15-126-231.washdc.fios.verizon.net] has joined #bitcoin-core-pr-reviews
11:07 -!- instagibbs [~instagibb@pool-100-15-126-231.washdc.fios.verizon.net] has quit [Ping timeout: 265 seconds]
11:43 -!- dzxzg [~dzxzg@166.198.34.4] has quit [Quit: Client closed]
12:21 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
13:41 -!- brunoerg_ [~brunoerg@2804:14c:3bfb:8a:9c54:b7e5:8572:ce2a] has joined #bitcoin-core-pr-reviews
13:41 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:9c54:b7e5:8572:ce2a] has quit [Read error: Connection reset by peer]
14:04 -!- pablomartin [~pablomart@78-157-209-152.as42831.net] has joined #bitcoin-core-pr-reviews
14:08 -!- pablomartin4btc [~pablomart@190.195.73.51] has joined #bitcoin-core-pr-reviews
14:11 -!- pablomartin [~pablomart@78-157-209-152.as42831.net] has quit [Ping timeout: 255 seconds]
14:31 -!- __gotcha [~Thunderbi@ldd29-1-78-210-28-87.fbx.proxad.net] has joined #bitcoin-core-pr-reviews
14:37 -!- brunoerg_ [~brunoerg@2804:14c:3bfb:8a:9c54:b7e5:8572:ce2a] has quit [Remote host closed the connection]
14:38 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:9c54:b7e5:8572:ce2a] has joined #bitcoin-core-pr-reviews
14:57 -!- kevkevin [~kevkevin@c-98-226-43-69.hsd1.il.comcast.net] has quit [Ping timeout: 246 seconds]
15:21 -!- pablomartin4btc [~pablomart@190.195.73.51] has quit [Read error: Connection reset by peer]
15:24 -!- pablomartin [~pablomart@190.195.73.51] has joined #bitcoin-core-pr-reviews
15:44 -!- grndslm [~grndslm@2607:fb90:d52a:5010:a0ce:62be:5ed1:8f9e] has quit [Read error: Connection reset by peer]
16:02 -!- __gotcha [~Thunderbi@ldd29-1-78-210-28-87.fbx.proxad.net] has quit [Ping timeout: 246 seconds]
16:12 -!- pablomartin [~pablomart@190.195.73.51] has quit [Quit: Leaving]
16:18 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
17:24 -!- hernanmarino [~hernanmar@181.99.169.107] has quit [Quit: Leaving]
18:56 -!- b_101_ [~robert@185.242.5.35] has joined #bitcoin-core-pr-reviews
18:59 -!- b_101 [~robert@185.242.5.35] has quit [Ping timeout: 255 seconds]
19:34 -!- Yihen [~textual@122.115.32.55] has quit [Read error: Connection reset by peer]
19:48 -!- BUSY [~BUSY@user/busy] has quit [Remote host closed the connection]
--- Log closed Thu Feb 16 00:00:45 2023