df69b22f2e doc: improve documentation around connection limit maximums (Amiti Uttarwar)
adc171edf4 scripted-diff: Rename connection limit variables (Amiti Uttarwar)
e9fd9c0225 net: add m_max_inbound to connman (Amiti Uttarwar)
c25e0e0555 net, refactor: move calculations for connection type limits into connman (Amiti Uttarwar)
Pull request description:
This is joint work with amitiuttarwar.
This has the first few commits of #28463. It is not strictly a prerequisite for that, but has changes that in our opinion make sense on their own.
It improves the handling of maximum numbers for different connection types (that are set during init and don’t change after) by:
* moving all calculations into one place, `CConnMan::Init()`. Before, they were dispersed between `Init`, `CConnman::Init` and other parts of `CConnman`, resulting in some duplicated test code.
* removing the possibility of having a negative maximum of inbound connections, which is hard to argue about
* renaming of variables and doc improvements
ACKs for top commit:
amitiuttarwar:
co-author review ACK df69b22f2e
naumenkogs:
ACK df69b22f2e
achow101:
ACK df69b22f2e
Tree-SHA512: 913d56136bc1df739978de50db67302f88bac2a9d34748ae96763288d97093e998fc0f94f9b6eff12867712d7e86225af6128f4170bf2b5b8ab76f024870a22c
`vfLimited`, `IsReachable()`, `SetReachable()` need not be in the `net`
module. Move them to `netbase` because they will be needed in
`LookupSubNet()` to possibly flip the result to CJDNS (if that network
is reachable).
In the process, encapsulate them in a class.
`NET_UNROUTABLE` and `NET_INTERNAL` are no longer ignored when adding
or removing reachable networks. This was unnecessary.
A "version" message in the V1 protocol starts with a fixed 16 bytes:
* The 4-byte network magic
* The 12-byte zero-padded command "version" plus 5 0x00 bytes
The current code detects incoming V1 connections by just looking at the
first 12 bytes (matching an earlier version of BIP324), but 16 bytes is
more precise. This isn't an observable difference right now, as a 12 byte
prefix ought to be negligible already, but it may become observable with
future extensions to the protocol, so make the code match the
specification.
Extract the logic for calculating & maintaining inbound connection limits to be
a member within connman for consistency with other maximum connection limits.
Note that we now limit m_max_inbound to 0 and don't call
AttemptToEvictConnection() when we don't have any inbounds.
Previously, nMaxInbound could become negative if the user ran with a low
-maxconnections, which didn't break any logic but didn't make sense.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Currently the logic is fragmented between init and connman. Encapsulating this
logic within connman allows for less mental overhead and easier reuse in tests.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
When an outbound v2 connection is disconnected without receiving anything, but at
least 24 bytes of our pubkey were sent out (enough to constitute an invalid v1
header), add them to a queue of reconnections to be tried.
The reconnections are in a queue rather than performed immediately, because we should
not block the socket handler thread with connection creation (a blocking operation
that can take multiple seconds).
See also https://github.com/bitcoin/bips/pull/1498
The benefit is a simpler implementation:
- The protocol state machine does not need separate states for garbage
authentication and version phases.
- The special case of "ignoring the ignore bit" is removed.
- The freedom to choose the contents of the garbage authentication
packet is removed. This simplifies testing.
Move functions requiring the netaddress.h include out of
libbitcoinkernel source files.
The netaddress.h file contains many non-consensus related definitions
and should thus not be part of the libbitcoinkernel. This commit makes
netaddress.h no longer a required include for users of the
libbitcoinkernel.
This commit is part of the libbitcoinkernel project, namely its stage 1
step 3: Decouple most non-consensus headers from libbitcoinkernel.
The protocol.h file contains many non-consensus related definitions and
should thus not be part of the libbitcoinkernel. This commit makes
protocol.h no longer a required include for users of the
libbitcoinkernel.
This commit is part of the libbitcoinkernel project, namely its stage 1
step 3: Decouple most non-consensus headers from libbitcoinkernel.
Co-Authored-By: Cory Fields <cory-nospam-@coryfields.com>
Before this commit the V2Transport::m_send_buffer is used to store the
garbage:
* During MAYBE_V1 state, it's there despite not being sent.
* During AWAITING_KEY state, while it is being sent.
* At the end of the AWAITING_KEY state it cannot be wiped as it's still
needed to compute the garbage authentication packet.
Change this by introducing a separate m_send_garbage field, taking over
the first and last role listed above. This means the garbage is only in
the send buffer when it's actually being sent, removing a few special
cases related to this.
This introduces a V2Transport with a basic subset of BIP324 functionality:
* no ability to send garbage (but receiving is supported)
* no ability to send decoy packets (but receiving them is supported)
* no support for short message id encoding (neither encoding or decoding)
* no waiting until 12 non-V1 bytes have been received
* (and thus) no detection of V1 connections on the responder side
(on the sender side, detecting V1 is not supported either, but that needs
to be dealt with at a higher layer, by reconnecting)
Before this commit, there are only two possibly outcomes for the "more" prediction
in Transport::GetBytesToSend():
* true: the transport itself has more to send, so the answer is certainly yes.
* false: the transport has nothing further to send, but if vSendMsg has more message(s)
left, that still will result in more wire bytes after the next
SetMessageToSend().
For the BIP324 v2 transport, there will arguably be a third state:
* definitely not: the transport has nothing further to send, but even if vSendMsg has
more messages left, they can't be sent (right now). This happens
before the handshake is complete.
To implement this, we move the entire decision logic to the Transport, by adding a
boolean to GetBytesToSend(), called have_next_message, which informs the transport
whether more messages are available. The return values are still true and false, but
they mean "definitely yes" and "definitely no", rather than "yes" and "maybe".
This furthers transport abstraction by removing the assumption that a message
can always immediately be converted to wire bytes. This assumption does not hold
for the v2 transport proposed by BIP324, as no messages can be sent before the
handshake completes.
This is done by only keeping (complete) CSerializedNetMsg objects in vSendMsg,
rather than the resulting bytes (for header and payload) that need to be sent.
In SocketSendData, these objects are handed to the transport as permitted by it,
and sending out the bytes the transport tells us to send. This also removes the
nSendOffset member variable in CNode, as keeping track of how much has been sent
is now a responsability of the transport.
This is not a pure refactor, and has the following effects even for the current
v1 transport:
* Checksum calculation now happens in SocketSendData rather than PushMessage.
For non-optimistic-send messages, that means this computation now happens in
the network thread rather than the message handler thread (generally a good
thing, as the message handler thread is more of a computational bottleneck).
* Checksum calculation now happens while holding the cs_vSend lock. This is
technically unnecessary for the v1 transport, as messages are encoded
independent from one another, but is untenable for the v2 transport anyway.
* Statistics updates about per-message sent bytes now happen when those bytes
are actually handed to the OS, rather than at PushMessage time.
This more accurately captures the intent of limiting send buffer size, as
many small messages can have a larger overhead that is not counted with the
current approach.
It also means removing the dependency on the header size (which will become
a function of the transport choice) from the send buffer calculations.
The rest of net.cpp already uses Params() to determine chainparams in many
places (and even V1Transport itself does so in some places).
Since the only chainparams dependency is through the message start characters,
just store those directly in the transport.
This makes the sending side of P2P transports mirror the receiver side: caller provides
message (consisting of type and payload) to be sent, and then asks what bytes must be
sent. Once the message has been fully sent, a new message can be provided.
This removes the assumption that P2P serialization of messages follows a strict structure
of header (a function of type and payload), followed by (unmodified) payload, and instead
lets transports decide the structure themselves.
It also removes the assumption that a message must always be sent at once, or that no
bytes are even sent on the wire when there is no message. This opens the door for
supporting traffic shaping mechanisms in the future.
Now that the Transport class deals with both the sending and receiving side
of things, make the receive side have function names that clearly indicate
they're about receiving.
* Transport::Read() -> Transport::ReceivedBytes()
* Transport::Complete() -> Transport::ReceivedMessageComplete()
* Transport::GetMessage() -> Transport::GetReceivedMessage()
* Transport::SetVersion() -> Transport::SetReceiveVersion()
Further, also update the comments on these functions to (among others) remove
the "deserialization" terminology. That term is better reserved for just the
serialization/deserialization between objects and bytes (see serialize.h), and
not the conversion from/to wire bytes as performed by the Transport.
Rather than relying on the caller to prevent concurrent calls to the
various receive-side functions of Transport, introduce a private m_cs_recv
inside the implementation to protect the lock state.
Of course, this does not remove the need for callers to synchronize calls
entirely, as it is a stateful object, and e.g. the order in which Receive(),
Complete(), and GetMessage() are called matters. It seems impossible to use
a Transport object in a meaningful way in a multi-threaded way without some
form of external synchronization, but it still feels safer to make the
transport object itself responsible for protecting its internal state.
This allows state that is shared between both directions to be encapsulated
into a single object. Specifically the v2 transport protocol introduced by
BIP324 has sending state (the encryption keys) that depends on received
messages (the DH key exchange). Having a single object for both means it can
hide logic from callers related to that key exchange and other interactions.
3388e523a1 Rework receive buffer pushback (Pieter Wuille)
Pull request description:
See https://github.com/ElementsProject/elements/issues/1233. There, it has been observed that if both sides of a P2P connection have a significant amount of data to send, a stall can occur, where both try to drain their own send queue before trying to receive. The same issue seems to apply to the current Bitcoin Core codebase, though I don't know whether it's a frequent issue for us.
The core issue is that whenever our optimistic send fails to fully send a message, we do subsequently not even select() for receiving; if it then turns out that sending is not possible either, no progress is made at all. To address this, the solution used in this PR is to still select() for both sending and receiving when an optimistic send fails, but skip receiving if sending succeeded, and (still) doesn't fully drain the send queue.
This is a significant reduction in how aggressive the "receive pushback" mechanism is, because now it will only mildly push back while sending progress is made; if the other side stops receiving entirely, the pushback disappears. I don't think that's a serious problem though:
* We still have a pushback mechanism at the application buffer level (when the application receive buffer overflows, receiving is paused until messages in the buffer get processed; waiting on our own net_processing thread, not on the remote party).
* There are cases where the existing mechanism is too aggressive; e.g. when the send queue is non-empty, but tiny, and can be sent with a single send() call. In that case, I think we'd prefer to still receive within the same processing loop of the network thread.
ACKs for top commit:
ajtowns:
ACK 3388e523a1
naumenkogs:
ACK 3388e523a1
mzumsande:
Tested ACK 3388e523a1
Tree-SHA512: 28960feb3cd2ff3dfb39622510da62472612f88165ea98fc9fb844bfcb8fa3ed3633f83e7bd72bdbbbd37993ef10181b2e1b34836ebb8f0d83fd1c558921ec17
1b52d16d07 p2p: network-specific management of outbound connections (Martin Zumsande)
65cff00cee test: Add test for outbound protection by network (Martin Zumsande)
034f61f83b p2p: Protect extra full outbound peers by network (Martin Zumsande)
654d9bc276 p2p: Introduce data struct to track connection counts by network (Amiti Uttarwar)
Pull request description:
This is joint work with mzumsande.
This is a proposal to diversify outbound connections with respect to reachable networks. The existing logic evaluates peers for connection based purely on the frequency of available addresses in `AddrMan`. This PR adds logic to automatically connect to alternate reachable networks and adds eviction logic that protects one existing connection to each network.
For instance, if `AddrMan` is populated primarily with IPv4 and IPv6 addresses and only a handful of onion addresses, it is likely that we won't establish any automatic outbound connections to Tor, even if we're capable of doing so. For smaller networks like CJDNS, this is even more of an issue and often requires adding manual peers to ensure regularly being connected to the network.
Connecting to multiple networks improves resistance to eclipse attacks for individual nodes. It also benefits the entire p2p network by increasing partition resistance and privacy in general.
The automatic connections to alternate networks is done defensively, by first filling all outbound slots with random addresses (as in the status quo) and then adding additional peers from reachable networks the node is currently not connected to. This approach ensures that outbound slots are not left unfilled while attempting to connect to a network that may be unavailable due to a technical issue or misconfiguration that bitcoind cannot detect.
Once an additional peer is added and we have one more outbound connection than we want, outbound eviction ensures that peers are protected if they are the only ones for their network.
Manual connections are also taken into account: If a user already establishes manual connections to a trusted peer from a network, there is no longer a need to make extra efforts to ensure we also have an automatic connection to it (although this may of course happen by random selection).
ACKs for top commit:
naumenkogs:
ACK 1b52d16d07
vasild:
ACK 1b52d16d07
Tree-SHA512: 5616c038a5fbb868d4c46c5963cfd53e4599feee25db04b0e18da426d77d22e0994dc4e1da0b810f5b457f424ebbed3db1704f371aa6cad002b3565b20170ec0
Diversify outbound connections with respect to
networks: Every ~5 minutes, try to add an extra connection
to a reachable network which we currently don't have a connection to.
This is done defensively - only try management with respect to networks
after all existing outbound slots are filled.
The resulting situation with an extra outbound peer will be handled
by the extra outbound eviction logic, which protects peers from
eviction if they are the only ones for their network.
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
If a peer is the only one of its network, protect it from eviction.
This improves the diversity of outbound connections with respect to
reachable networks.
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Connman uses this new map to keep a count of active OUTBOUND_FULL_RELAY and
MANUAL connections. Unused until next commit.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
e7cf8657e1 test: add unit test for local address advertising (Martin Zumsande)
f4754b9dfb net: restrict self-advertisements with privacy networks (Martin Zumsande)
e4d541c7cf net, refactor: pass reference for peer address in GetReachabilityFrom (Martin Zumsande)
62d73f5370 net, refactor: pass CNode instead of CNetAddr to GetLocalAddress (Martin Zumsande)
Pull request description:
The current logic for self-advertisements works such that we detect as many local addresses as we can, and then, using the scoring matrix from `CNetAddr::GetReachabilityFrom()`, self-advertise with the address that fits best to our peer.
It is in general not hard for our peers to distinguish our self-advertisements from other addrs we send them, because we self-advertise every ~24h and because the first addr we send over a connection is likely our self-advertisement.
`GetReachabilityFrom()` currently only takes into account actual reachability, but not whether we'd _want_ to announce our identity for one network to peers from other networks, which is not straightforward in connection with privacy networks.
While the general approach is to prefer self-advertising with the address for the network our peer is on, there are several special situations in which we don't have one, and as a result could allow self-advertise other local addresses, for example:
A) We run i2p and clearnet, use `-i2pacceptincoming=0` (so we have no local i2p address), and we have a local ipv4 address. In this case, we'd advertise the ipv4 address to our outbound i2p peers.
B) Our `-discover` logic cannot detect any local clearnet addresses in our network environment, but we are actually reachable over clearnet. If we ran bitcoind clearnet-only, we'd always advertise the address our peer sees us with instead, and could get inbound peers this way. Now, if we also have an onion service running (but aren't using tor as a proxy for clearnet connections), we could advertise our onion address to clearnet peers, so that they would be able to connect our clearnet and onion identities.
This PR tries to avoid these situations by
1.) never advertising our local Tor or I2P address to peers from other networks.
2.) never advertising local addresses from non-anonymity networks to peers from Tor or I2P
Note that this affects only our own self-advertisements, the rules to forward other people's addrs are not changed.
[Edit] after Initial [discussion](https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1497176155): CJDNS is not being treated like Tor and I2P at least for now, because it has different privacy properties and for the practical reason that it has still very few bitcoin nodes.
ACKs for top commit:
achow101:
ACK e7cf8657e1
vasild:
ACK e7cf8657e1
luke-jr:
utACK e7cf8657e1
Tree-SHA512: 3db8415dea6f82223d11a23bd6cbb3b8cf68831321280e926034a1f110cbe22562570013925f6fa20d8f08e41d0202fd69c733d9f16217318a660d2a1a21b795
