The PR branch HEAD was 153e86e at the time of this review club meeting.
Notes
By default, a Bitcoin Core node allows up to 125 connections (8 of which are outbound) to other peers.
There are different kinds of connections, such as block-relay-only, inbound, outbound-full-relay, manual, feeler, block-relay and addr-fetch.
You can use the getpeerinfo RPC to get data about each connected network peer such as connection type, permissions and other information.
-whitelist is a startup option that allows to add permission flags to the peers connecting from the given IP address or CIDR-notated network. It uses the same permissions as -whitebind: bloomfilter, noban, forcerelay, relay, mempool, download, addr).
This PR builds on the work done in #17167. #17167 proposed to change -whitelist to allow whitelisting outgoing connections, and this PR adds the addpermissionflags RPC to it.
-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?
Considering we already have the -whitelist startup option, why would an RPC be useful? What do we want to avoid?
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?
ConnectionDirection can be Both, In, Out or None. What happens in TryParsePermissionFlags if it is None? In which scenarios can this happen?
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?
(Bonus) How could this PR avoid the “problem” presented in #26970?
<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?
<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."
<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).
<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
<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)
<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
<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?
<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?
<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...
<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`
<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
<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
<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?
<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?
<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'