Prior to this commit, the peer was connected, and then the services and
connectivity fields in the CNode object were manually set. Instead, send
p2p `version` and `verack` messages, and have net_processing's internal
logic set the state of the node.
This ensures that the node's internal state is consistent with how it
would be set in the live code.
Prior to this commit, `dummyNode1.nServices` was set to `NODE_NONE`
which was not a problem since `CNode::fClient` and
`CNode::m_limited_node` are default initialised to false. Now that we
are doing the actual version handshake, the values of `fClient` and
`m_limited_node` are set during the handshake and cause the test to fail
if we do not set `dummyNode1.nServices` to a reasonable value
(NODE_NETWORK | NODE_WITNESS).
`Misbehaving` has several coding related issues (ignoring the conceptual
issues here for now):
* It is public, but it is not supposed to be called from outside of
net_processing. Fix that by making it private and creating a public
`UnitTestMisbehaving` method for unit testing only.
* It doesn't do anything if a `nullptr` is passed. It would be less
confusing to just skip the call instead. Fix that by passing `Peer&`
to `Misbehaving()`.
* It calls `GetPeerRef`, causing `!m_peer_mutex` lock annotations to be
propagated. This is harmless, but verbose. Fix it by removing the no
longer needed call to `GetPeerRef` and the no longer needed lock
annotations.
778343a379 scripted-diff: Rename PeerManagerImpl members (dergoegge)
91c339243e [net processing] Move nHighestFastAnnounce into PeerManagerImpl (dergoegge)
10b83e2aa3 [net processing] Move block cache state into PeerManagerImpl (dergoegge)
a4c55a93ef [net processing] Inline and simplify UpdatePreferredDownload (dergoegge)
490c08f96a [net processing] Move nPreferredDownload into PeerManagerImpl (dergoegge)
a292df283a [net processing] Move mapNodeState into PeerManagerImpl (dergoegge)
37ecaf3e7a [net processing] Move CNodeState declaration above PeerManagerImpl (dergoegge)
Pull request description:
This PR moves the remaining net processing globals into `PeerManagerImpl`. This will make testing the peer manager in isolation easier and also acts as a code clean up.
ACKs for top commit:
jnewbery:
Code review ACK 778343a379
MarcoFalke:
ACK 778343a379 🗒
Tree-SHA512: 4f22105d1de37b94c3ef349f38784a30cf8d450d394a6a7849e5bd78940a71e3edbffa3d25e8efb35d7f698fd255f199de7bd4c33e23af5621a6e4e67ed43cb5
36f814c0e8 [netgroupman] Remove NetGroupManager::GetAsmap() (John Newbery)
4709fc2019 [netgroupman] Move asmap checksum calculation to NetGroupManager (John Newbery)
1b978a7e8c [netgroupman] Move GetMappedAS() and GetGroup() logic to NetGroupManager (John Newbery)
ddb4101e63 [net] Only use public CNetAddr functions and data in GetMappedAS() and GetGroup() (John Newbery)
6b2268162e [netgroupman] Add GetMappedAS() and GetGroup() (John Newbery)
19431560e3 [net] Move asmap into NetGroupManager (John Newbery)
17c24d4580 [init] Add netgroupman to node.context (John Newbery)
9b3836710b [build] Add netgroup.cpp|h (John Newbery)
Pull request description:
The asmap data is currently owned by addrman, but is used by both addrman and connman. #22791 made the data const and private (so that it can't be updated by other components), but it is still passed out of addrman as a reference to const, and used by `CNetAddress` to calculate the group and AS of the net address.
This RFC PR proposes to move all asmap data and logic into a new `NetGroupManager` component. This is initialized at startup, and the client components addrman and connman simply call `NetGroupManager::GetGroup(const CAddress&)` and `NetGroupManager::GetMappedAS(const CAddress&)` to get the net group and AS of an address.
ACKs for top commit:
mzumsande:
Code Review ACK 36f814c0e8
jnewbery:
CI failure seems spurious. I rebased onto latest master to trigger a new CI run, but whilst I was doing that, mzumsande ACKed 36f814c0e8, so I've reverted to that.
dergoegge:
Code review ACK 36f814c0e8
Tree-SHA512: 244a89cdfd720d8cce679eae5b7951e1b46b37835fccb6bdfa362856761bb110e79e263a6eeee8246140890f3bee2850e9baa7bc14a388a588e0e29b9d275175
fadc0c80ae p2p: Make timeout mockable and type safe, speed up test (MarcoFalke)
fa6d5a238d scripted-diff: Rename m_last_send and m_last_recv (MarcoFalke)
Pull request description:
Use type-safe time for better code readability/maintainability and mockable time for better testability. This speeds up the p2p_timeout test.
This is also a bugfix for intermittent test issues like: https://cirrus-ci.com/task/4769904156999680?command=ci#L2836Fixes#20654
ACKs for top commit:
laanwj:
Code review ACK fadc0c80ae
naumenkogs:
ACK fadc0c80ae
Tree-SHA512: 28c6544c97f188c8a0fbc80411c74ab74ffd055885322c325aa3d1c404b29c3fd70a737e86083eecae58ef394db1cb56bc122d06cff63742aa89a8e868730c64
4740fe8212 test: Add test for block relay only eviction (Martin Zumsande)
Pull request description:
Adds a unit test for block-relay-only eviction logic added in #19858, which was not covered by any tests before. The added test is very similar to the existing `stale_tip_peer_management` unit test, which tests the analogous logic for regular outbound peers.
ACKs for top commit:
glozow:
reACK 4740fe8212
rajarshimaitra:
tACK 4740fe8212
shaavan:
ACK 4740fe8212. Great work @ mzumsande!
LarryRuane:
ACK 4740fe8212
Tree-SHA512: 5985afd7d8f7ae311903dbbf6b7d526e16309c83c88ae6dd6551960c0b186156310a6be0cf6b684f82ac1378d0fc5aa3717f0139e078471013fceb6aebe81bf6
Don't schedule class PeerManagerImpl's background tasks from its
constructor, but instead do that from a separate method,
StartScheduledTasks(), that can be called later at the end of startup,
after other things, such as the active chain, are initialzed.
Save the banlist in `banlist.json` instead of `banlist.dat`.
This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).
Only read `banlist.dat` if it exists and `banlist.json` does not
exist (first start after an upgrade).
Supersedes https://github.com/bitcoin/bitcoin/pull/20904
Resolves https://github.com/bitcoin/bitcoin/issues/19748
6f994882de validation: Farewell, global Chainstate! (Carl Dong)
972c5166ee qt/test: Reset chainman in ~ChainstateManager instead (Carl Dong)
6c3b5dc0c1 scripted-diff: tree-wide: Remove all review-only assertions (Carl Dong)
3e82abb8dd tree-wide: Remove stray review-only assertion (Carl Dong)
f323248aba qt/test: Use existing chainman in ::TestGUI (can be scripted-diff) (Carl Dong)
6c15de129c scripted-diff: wallet/test: Use existing chainman (Carl Dong)
ee0ab1e959 fuzz: Initialize a TestingSetup for test_one_input (Carl Dong)
0d61634c06 scripted-diff: test: Use existing chainman in unit tests (Carl Dong)
e197076219 test: Pass in CoinsTip to ValidateCheckInputsForAllFlags (Carl Dong)
4d99b61014 test/miner_tests: Pass in chain tip to CreateBlockIndex (Carl Dong)
f0dd5e6bb4 test/util: Use existing chainman in ::PrepareBlock (Carl Dong)
464c313e30 init: Use existing chainman (Carl Dong)
Pull request description:
Based on: #21767
à la Mr. Sandman
```
Mr. Chainman, bring me a tip (bung, bung, bung, bung)
Make it the most work that I've ever seen (bung, bung, bung, bung)
Rewind old tip till we're at the fork point (bung, bung, bung, bung)
Then tell it that it's time to call Con-nectTip
Chainman, I'm so alone (bung, bung, bung, bung)
No local objects to call my own (bung, bung, bung, bung)
Please make sure I have a ref
Mr. Chainman, bring me a tip!
```
This is the last bundle in the #20158 series. Thanks everyone for their diligent review.
I would like to call attention to https://github.com/bitcoin/bitcoin/issues/21766, where a few leftover improvements were collated.
- Remove globals:
- `ChainstateManager g_chainman`
- `CChainState& ChainstateActive()`
- `CChain& ChainActive()`
- Remove all review-only assertions.
ACKs for top commit:
jamesob:
reACK 6f994882de based on the contents of
ariard:
Code Review ACK 6f99488.
jnewbery:
utACK 6f994882de
achow101:
Code Review ACK 6f994882de
ryanofsky:
Code review ACK 6f994882de.
Tree-SHA512: 4052ea79360cf0efd81ad0ee3f982e1d93aab1837dcec75f875a56ceda085de078bb3099a2137935d7cc2222004ad88da94b605ef5efef35cb6bc733725debe6
This is a non-functional change that replaces the `CNode` on-stack
variables with `CNode` pointers.
The reason for this is that it would allow us to add those `CNode`s
to `CConnman::vNodes[]` which in turn would allow us to check that they
are disconnected properly - a `CNode` object must be in
`CConnman::vNodes[]` in order for its `fDisconnect` flag to be set.
If we store pointers to the on-stack variables in `CConnman` then it
would crash at the end, trying to `delete` them.
Collects all the orphan handling globals into a single member var in
net_processing, and ensures access is encapuslated into the interface
functions. Also adds doxygen comments for methods.
Rather than checking net_processing's internal implementation of
AddOrphanTx, test txorphanage's exported AddTx interface. Note that
this means AddToCompactExtraTransactions is no longer tested here.
After commit ddefb5c0b7 nVersion is no
longer used in p2p logic when sending messages. Only when receiving
messages, but in this test no messages are received.
ddefb5c0b7 p2p: Use the greatest common version in peer logic (Hennadii Stepanov)
e084d45562 p2p: Remove SetCommonVersion() from VERACK handler (Hennadii Stepanov)
8d2026796a refactor: Rename local variable nSendVersion (Hennadii Stepanov)
e9a6d8b13b p2p: Unify Send and Receive protocol versions (Hennadii Stepanov)
Pull request description:
On master (6fef85bfa3) `CNode` has two members to keep protocol version:
- `nRecvVersion` for received messages
- `nSendVersion` for messages to send
After exchanging with `VERSION` and `VERACK` messages via protocol version `INIT_PROTO_VERSION`, both nodes set `nRecvVersion` _and_ `nSendVersion` to _the same_ value which is the greatest common protocol version.
This PR:
- replaces two `CNode` members, `nRecvVersion` `nSendVersion`, with `m_greatest_common_version`
- removes duplicated getter and setter
There is no change in behavior on the P2P network.
ACKs for top commit:
jnewbery:
ACK ddefb5c0b7
naumenkogs:
ACK ddefb5c0b7
fjahr:
Code review ACK ddefb5c0b7
amitiuttarwar:
code review but untested ACK ddefb5c0b7
benthecarman:
utACK `ddefb5c`
Tree-SHA512: 5305538dbaa5426b923b0afd20bdef4f248d310855d1d78427210c00716c67b7cb691515c421716b6157913e453076e293b10ff5fd2cd26a8e5375d42da7809d
