The PR branch HEAD was f51006b818 at the time of this review club meeting.
There are a variety of different types of P2P connections that Bitcoin Core nodes
can have with peers. See the ConnectionType enum in src/net.h for the different
Until now, the Bitcoin Core functional test framework allowed only 2 types of
manual connections from one bitcoind instance to another. See the
BitcoinTestFramework.connect_nodes() method, which calls the addnode RPC.
inbound connections made from the test framework to the bitcoind
instance. See the TestNode.add_p2p_connection() method.
This week’s PR adds the ability to create 2 additional types of regtest P2P
connections from the bitcoind instances to the functional test framework:
full outbound connections
block-relay-only outbound connections
The PR adds functionality to both the bitcoind C++ code and to the functional
test framework Python code:
In bitcoind, a test-only addconnection RPC is added to allow opening
full outbound or block-relay-only outbound connections in regtest.
In the functional test framework, TestNode.add_outbound_p2p_connection()
is added to enable making either full outbound or block-relay-only outbound
connections, along with P2PInterface.peer_accept_connection() to allow the
test framework to accept and listen to incoming connections.
We’ll focus mainly on the C++ code in this review club.
What are Bitcoin Core’s different connection types?
In the test framework, what is the difference between connecting two
bitcoind nodes using connect_nodes() and using a P2PConnection object?
Why would it be useful to create outbound or block-relay-only connections in
the test framework?
How does this PR enable creating outbound and block-relay-only connections
in the test framework?
Why do we make some RPC methods hidden, or “test only”?
For each different type of connection, what is the maximum number of
connections that Bitcoin Core will have by default?
What are CSemaphore and CSemaphoreGrant used for in CConnman?
<amiti> sergei-t: good question, since outbound-full-relay & block-relay-only are the most common connections, I focused on those for this PR, but went a route that would allow for adding more connection types in the future
<stacie> I have a q about the manual connection type. Are these connections restricted to being full relay or block relay only? I’m assuming they default to full relay but could they be block relay only?
<jnewbery> stacie: test code needs to be maintained just the same as all other code, so I'd NACK it if it doesn't meet the quality bar or doesn't add enough value to justify the ongoing maintainance cost
<amiti> sergei-t: I don't think thats the only difference. for example, if you restart your node, you'll reconnect to your manually added peers. whereas you probably wont reconnect to the same full-relay outbound peers.
<willcl_ark> Connecting two nodes directly connects them via their listening p2p ports whereas using P2P.py passes the connection through the P2PConnection class, which lets you instrument and act upon each message exchanged.
<jnewbery> sergei-t: I think you're getting your terminology mixed up. A test contains one or more bitcoind instances under test. We can interact with those instances from the test framework using the RPC interface or the P2P interface. We can also get the different bitcoind instances to connect to each other using p2p.
<jnewbery> P2PConnection is a low-level class that the Python framework uses to connect to the bitcoind node (and after this PR is merged, it will be able to accept an outbound connection _from_ the bitcoind node)
<jnewbery> sergei-t: there's no "fake" node. The test framework can open P2P connections to the bitcoind instances. You could call the test framework a "fake" node if you wanted to, but I don't think it makes things any clearer (in fact, I think it's confusing terminology)
<amiti> for people interested in learning more: when you are writing a functional test, in `set_test_params` you have to set `self.num_nodes`. this creates `TestNode` objects that wrap a bitcoind instance (and does things like forward along RPC calls)
<lightlike> sergei-t: I think it is a matter of terminology. I still think of the P2PConnection instances as objects, mock/fake nodes that we control and use to send the real node whatever p2p messages we wish.
<jnewbery> sergei: question 2 should say `P2PConnection`, not "P2P Connection". The difference is that the second option is creating a p2p connection using the python P2PConnection object, which as lightlike says, is a mock/fake to test the p2p interface
<sergei-t> because the behavior of these RPCs is not triggered by an RPC call in "real world"? I.e., full relay connections only appear to randomly chosen nodes, it the node is specified, it's a manual connection by definition
<jnewbery> sergei-t: it's possible, but I think a good general principle is that new features/interfaces should be as tightly restricted as possible to begin with. It's much easier to give users features than take them away.
<amiti> when implementing, I initially tried adding the functionality to the `addnode`RPC, but didn't like adding yet another definition to an already overloaded endpoint, so separated it out into a new RPC endpoint. then made it test only for mostly these reasons, like more flexibility & its easier to isolate test needs vs trying to encapsulate all possible uses on mainnet
<amiti> addr_fetch connections are short-lived connections only (possibly) made on startup, I don't remember exactly how the counting works but for the majority of the time running a node, we won't have any open
<amiti> so, if you're starting up a node for the first time. you connect to the DNS seeds with addr fetch connections. they send you back addresses of nodes to populate your addrman, you close those connections and start opening connections to the general network
<amiti> there's also a fallback set of seeds incase there's a crazy infrastructure attack or you don't want to connect to DNS, but I believe these are the only two groups that we automatically connect to via addrfetch
<pedr0fr> I have a question. I want to try the ZMQ sequence notifier (merged on the 23rd of September - PR #19572).I am willing to wait for bitcoin core 0.21.0rc1, which, if I understand correctly, was planned for the 1st of November (https://github.com/bitcoin/bitcoin/issues/18947). When is 0.21.0rc1 expected to be released? Thank you.
<willcl_ark> amiti: well you say we only do it at fresh startup; what about a nodes that's starting up again after being offline for a while, or a node that's been running for some time without being turned off
<sipa> so instead of asking a DNS seed, we *connect* to one (which means the exit node we're using will resolve for us, and open a connection to one of the resolved IPs); we still want a lot of IPs though, so we ask that node