To make eclipse attacks more difficult, regularly initiate outbound connections
and stay connected long enough to sync headers and potentially learn of new
blocks. If we learn a new block, rotate out an existing block-relay peer in
favor of the new peer.
This augments the existing outbound peer rotation that exists -- currently we
make new full-relay connections when our tip is stale, which we disconnect
after waiting a small time to see if we learn a new block. As block-relay
connections use minimal bandwidth, we can make these connections regularly and
not just when our tip is stale.
Like feeler connections, these connections are not aggressive; whenever our
timer fires (once every 5 minutes on average), we'll try to initiate a new
block-relay connection as described, but if we fail to connect we just wait for
our timer to fire again before repeating with a new peer.
This removes most transaction request logic from net_processing, and
replaces it with calls to a global TxRequestTracker object.
The major changes are:
* Announcements from outbound (and whitelisted) peers are now always
preferred over those from inbound peers. This used to be the case for the
first request (by delaying the first request from inbound peers), and
a bias afters. The 2s delay for requests from inbound peers still exists,
but after that, if viable outbound peers remain for any given transaction,
they will always be tried first.
* No more hard cap of 100 in flight transactions per peer, as there is less
need for it (memory usage is linear in the number of announcements, but
independent from the number in flight, and CPU usage isn't affected by it).
Furthermore, if only one peer announces a transaction, and it has over 100
in flight and requestable already, we still want to request it from them.
The cap is replaced with an additional 2s delay (possibly combined with the
existing 2s delays for inbound connections, and for txid peers when wtxid
peers are available).
Includes functional tests written by Marco Falke and Antoine Riard.
-BEGIN VERIFY SCRIPT-
sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test)
sed -i 's/peer_logic/peerman/g' $(git grep -l peer_logic ./src ./test)
-END VERIFY SCRIPT-
PeerLogicValidation was originally net_processing's implementation to
the validation interface. It has since grown to contain much of
net_processing's logic. Therefore rename it to reflect its
responsibilities.
Suggested in
https://github.com/bitcoin/bitcoin/pull/10756#pullrequestreview-53892618.
Keep a references to chainparams, rather than calling the global
Params() function every time it's needed. This is fine, since
globalChainParams does not get updated once it's been set, and it's
available at the point of constructing the PeerLogicValidation object.
We don't have a project style for ordering class members, but it always
makes sense to have no more than one of each public/protected/private
specifier.
Also move documentation for MaybeDiscourageAndDisconnect to the header.
8e35bf5906 scripted-diff: rename misbehavior members (John Newbery)
1f96d2e673 [net processing] Move misbehavior tracking state to Peer (John Newbery)
7cd4159ac8 [net processing] Add Peer (John Newbery)
aba03359a6 [net processing] Remove CNodeState.name (John Newbery)
Pull request description:
We currently have two structures for per-peer data:
- `CNode` in net, which should just contain connection layer data (eg socket, send/recv buffers, etc), but currently also contains some application layer data (eg tx/block inventory).
- `CNodeState` in net processing, which contains p2p application layer data, but requires cs_main to be locked for access.
This PR adds a third struct `Peer`, which is for p2p application layer data, and doesn't require cs_main. Eventually all application layer data from `CNode` should be moved to `Peer`, and any data that doesn't strictly require cs_main should be moved from `CNodeState` to `Peer` (probably all of `CNodeState` eventually).
`Peer` objects are stored as shared pointers in a net processing global map `g_peer_map`, which is protected by `g_peer_mutex`. To use a `Peer` object, `g_peer_mutex` is locked, a copy of the shared pointer is taken, and the lock is released. Individual members of `Peer` are protected by different mutexes that guard related data. The lifetime of the `Peer` object is managed by the shared_ptr refcount.
This PR adds the `Peer` object and moves the misbehaving data from `CNodeState` to `Peer`. This allows us to immediately remove 15 `LOCK(cs_main)` instances.
For more motivation see #19398
ACKs for top commit:
laanwj:
Code review ACK 8e35bf5906
troygiorshev:
reACK 8e35bf5906 via `git range-diff master 9510938 8e35bf5`
theuni:
ACK 8e35bf5906.
jonatack:
ACK 8e35bf5906 keeping in mind Cory's comment (https://github.com/bitcoin/bitcoin/pull/19607#discussion_r470173964) for the follow-up
Tree-SHA512: ad84a92b78fb34c9f43813ca3dfbc7282c887d55300ea2ce0994d134da3e0c7dbc44d54380e00b13bb75a57c28857ac3236bea9135467075d78026767a19e4b1
Hold a reference to connman rather than a pointer because:
- PeerLogicValidation can't run without a connman
- The pointer never gets reseated
The alternative is to always assert that the pointer is non-null before
dereferencing.
Change the name from connman to m_connman at the same time to conform
with current style guidelines.
655b195747 [net processing] Continue SendMessages processing if not disconnecting peer (John Newbery)
a49781e56d [net processing] Only call MaybeDiscourageAndDisconnect from SendMessages (John Newbery)
a1d5a428a2 [net processing] Fix bad indentation in SendMessages() (John Newbery)
1a1c23f8d4 [net processing] Change cs_main TRY_LOCK to LOCK in SendMessages() (John Newbery)
Pull request description:
The motivation for this PR is to reduce the scope of cs_main locking in misbehavior logic. It is the first set of commits from a larger branch to move the misbehavior data out of CNodeState and into a new struct that doesn't take cs_main.
There are some very minor behavior changes in this branch, such as:
- Not checking for discouragement/disconnect in `ProcessMessages()` (and instead relying on the following check in `SendMessages()`)
- Checking for discouragement/disconnect as the first action in `SendMessages()` (and not doing ping message sending first)
- Continuing through `SendMessages()` if `MaybeDiscourageAndDisconnect()` doesn't disconnect the peer (rather than dropping out of `SendMessages()`
ACKs for top commit:
jonatack:
re-ACK 655b195 per `git range-diff 505b4ed f54af5e 655b195`, code/commit messages review, a bit of code history, and debug build.
MarcoFalke:
ACK 655b195747 only some style-nits 🚁
promag:
Code review ACK 655b195747.
ariard:
Code Review ACK 655b195
Tree-SHA512: fd6d7bc6bb789f5fb7771fb6a45f61a8faba32af93b766554f562144f9631d15c9cc849a383e71743ef73e610b4ee14853666f6fbf08a3ae35176d48c76c65d3
This adds a field to CNodeState that tracks whether to relay transactions with
that peer via wtxid, instead of txid. As of this commit the field will always
be false, but in a later commit we will add a way to negotiate turning this on
via p2p messages exchanged with the peer.
Although we currently don't do this, it should be possible to create a
CConnman or PeerLogicValidation without a Banman instance. Therefore
always check that banman exists before dereferencing the pointer.
Also add comments to the m_banman members of CConnman and
PeerLogicValidation to document that these may be nullptr.
`nMisbehavior` is a tally in `CNodeState` that can be incremented from
anywhere. That almost always happens inside a `ProcessMessages()` call
(because we increment the misbehavior score when receiving a bad
messages from a peer), but not always. See, for example, the call to
`MaybePunishNodeForBlock()` inside `BlockChecked()`, which is an
asynchronous callback from the validation interface, executed on the
scheduler thread.
As long as `MaybeDiscourageAndDisconnect()` is called regularly for the
node, then the misbehavior score exceeding the 100 threshold will
eventually result in the peer being punished. It doesn't really matter
where that `MaybeDiscourageAndDisconnect()` happens, but it makes most
sense in `SendMessages()` which is where we do general peer
housekeeping/maintenance.
Therefore, remove the `MaybeDiscourageAndDisconnect()` call in
`ProcessMessages()` and move the `MaybeDiscourageAndDisconnect()` call
in `SendMessages()` to the top of the function. This moves it out of the
cs_main lock scope, so take that lock directly inside
`MaybeDiscourageAndDisconnect()`.
Historic note: `MaybeDiscourageAndDisconnect()` was previously
`SendRejectsAndCheckIfBanned()`, and before that was just sending
rejects. All of those things required cs_main, which is why
`MaybeDiscourageAndDisconnect()` was called after the ping logic.
