709af67add93f6674fb80e3ae8e3f175653a62f0 p2p: replace RecursiveMutex `m_total_bytes_sent_mutex` with Mutex (w0xlt)
8be75fd0f0039eeea5f9af7c1eb17c584ed9f507 p2p: add assertions and negative TS annotations for `m_total_bytes_sent_mutex` (w0xlt)
a237a065cc2c6337e3797cc30a0f84c56c6d2f3b scripted-diff: rename cs_totalBytesSent -> m_total_bytes_sent_mutex (w0xlt)
Pull request description:
Related to #19303, this PR gets rid of the RecursiveMutex `cs_totalBytesSent` and also adds `AssertLockNotHeld` macros combined with `LOCKS_EXCLUDED` thread safety annotations to avoid recursive locking.
ACKs for top commit:
jonatack:
ACK 709af67add93f6674fb80e3ae8e3f175653a62f0 per `git range-diff 7a4ac71 eff7918 709af67`, rebase to master, clang 15 debug build, and build with -Wthread-safety-negative
vasild:
ACK 709af67add93f6674fb80e3ae8e3f175653a62f0
hebasto:
ACK 709af67add93f6674fb80e3ae8e3f175653a62f0, tested on Ubuntu 22.04.
Tree-SHA512: 560b4e6c92b1511911d69185207df6ee809db09b96d97f96430d8d2595dc05c98cc691aaec8a58ef87cf2ab0a98675c210b8ce0be3dedb81e31114bbbfdfd8be
36f814c0e84d009c0e0aa26981a20ac4cf338a85 [netgroupman] Remove NetGroupManager::GetAsmap() (John Newbery)
4709fc2019e27e74be02dc5fc123b9f6f46d7990 [netgroupman] Move asmap checksum calculation to NetGroupManager (John Newbery)
1b978a7e8c71dcc1501705022e66f6779c8c4528 [netgroupman] Move GetMappedAS() and GetGroup() logic to NetGroupManager (John Newbery)
ddb4101e6377a998b7c598bf52217b47698ddec9 [net] Only use public CNetAddr functions and data in GetMappedAS() and GetGroup() (John Newbery)
6b2268162e96bc4fe1a3ebad454996b1d3d4615c [netgroupman] Add GetMappedAS() and GetGroup() (John Newbery)
19431560e3e1124979c60f39eca9429c4a0df29f [net] Move asmap into NetGroupManager (John Newbery)
17c24d458042229e00dd4e0b75a32e593be29564 [init] Add netgroupman to node.context (John Newbery)
9b3836710b8160d212aacd56154938e5bb4b26b7 [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 36f814c0e84d009c0e0aa26981a20ac4cf338a85
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 36f814c0e84d009c0e0aa26981a20ac4cf338a85
Tree-SHA512: 244a89cdfd720d8cce679eae5b7951e1b46b37835fccb6bdfa362856761bb110e79e263a6eeee8246140890f3bee2850e9baa7bc14a388a588e0e29b9d275175
We'll move the transaction relay data into Peer in subsequent commits,
but the inbound eviction logic needs to know if the peer is relaying
txs and if the peer has loaded a bloom filter.
This is currently redundant information with m_tx_relay->fRelayTxes,
but when m_tx_relay is moved into net_processing, then we'll need these
separate fields in CNode.
`GetListenPort()` uses a simple logic: "if `-port=P` is given, then we
must be listening on `P`, otherwise we must be listening on `8333`".
This is however not true if `-bind=` has been provided with `:port` part
or if `-whitebind=` has been provided. Thus, extend `GetListenPort()` to
return the port from `-bind=` or `-whitebind=`, if any.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (cases 1. 2. 3. 5.)
Rename `CaptureMessage()` to `CaptureMessageToFile()` and introduce a
`std::function` variable called `CaptureMessage` whose value can be
changed by unit tests, should they need to inspect message contents.
b7be28cac50046b9f2ddfe63ecafccc80649a36c test: add combined CJDNS/I2P/localhost/onion eviction protection tests (Jon Atack)
0a1bb84770b403ab5cbd9d5474c76f91ce58e8f6 test: add tests for inbound eviction protection of CJDNS peers (Jon Atack)
0c00c0c981fc0b6cec101e68e8c1aeda1ccf33bb test: fix off-by-one logic in an eviction protection test (Jon Atack)
f7b8094d611531c6b41a94715dbc01f56257ccd2 p2p: extend inbound eviction protection by network to CJDNS peers (Jon Atack)
Pull request description:
Extend inbound eviction protection for peers connected over CJDNS, as is the case for peers connected via onion, localhost, and I2P since #21261 and #20197. CJDNS peers seem to have better min ping latency than onion and I2P peers but still higher than that of unencrypted IPv4/6 peers and can be disadvantaged under our eviction criteria. They are also very few in number, which is a further reason to protect them, as the goal of this logic is to favorise the diversity of our peer connections. CJDNS support was added in #23077 for the upcoming v23 release.
ACKs for top commit:
laanwj:
Concept and code review ACK b7be28cac50046b9f2ddfe63ecafccc80649a36c
w0xlt:
tACK b7be28c
Tree-SHA512: 89ebdd217602e16ae14b9bd0d5a25fc09f9b2384c951f820bc0f5a6d8452bbc9042065db817d5d5296c0ad22988491a83fc5b9a611e660c40ebd4f03448c4061
36ee76d1afbb278500fc8aa01606ec933b52c17d net: remove unused CNetAddr::GetHash() (Vasil Dimov)
d0abce9a50dd4f507e3a30348eabffb7552471d5 net: include the port when deciding a relay destination (Vasil Dimov)
2e38a0e6865187d1f0d0f016d3df7cce414a7c4f net: add CServiceHash constructor so the caller can provide the salts (Vasil Dimov)
97208634b96f2d9a55f2ead7b0ef407da729d7bd net: open p2p connections to nodes that listen on non-default ports (Vasil Dimov)
Pull request description:
By default, for mainnet, the p2p listening port is 8333. Bitcoin Core
has a strong preference for only connecting to nodes that listen on that
port.
Remove that preference because connections over clearnet that involve
port 8333 make it easy to detect, analyze, block or divert Bitcoin p2p
traffic before the connection is even established (at TCP SYN time).
For further justification see the OP of:
https://github.com/bitcoin/bitcoin/pull/23306
ACKs for top commit:
laanwj:
Concept and light code review ACK 36ee76d1afbb278500fc8aa01606ec933b52c17d
prayank23:
ACK 36ee76d1af
stickies-v:
tACK 36ee76d1a
jonatack:
ACK 36ee76d1afbb278500fc8aa01606ec933b52c17d
glozow:
utACK 36ee76d1afbb278500fc8aa01606ec933b52c17d
Tree-SHA512: 7f45ab7567c51c19fc50fabbaf84f0cc8883a8eef84272b76435c014c31d89144271d70dd387212cc1114213165d76b4d20a5ddb8dbc958fe7e74e6ddbd56d11
0eea83a85ec6b215d44facc2b16ee1b035275a6b scripted-diff: rename `proxyType` to `Proxy` (Vasil Dimov)
e53a8505dbb6f9deaae8ac82793a4fb760a1e0a6 net: respect -onlynet= when making outbound connections (Vasil Dimov)
Pull request description:
Do not make outbound connections to hosts which belong to a network
which is restricted by `-onlynet`.
This applies to hosts that are automatically chosen to connect to and to
anchors.
This does not apply to hosts given to `-connect`, `-addnode`,
`addnode` RPC, dns seeds, `-seednode`.
Fixes https://github.com/bitcoin/bitcoin/issues/13378
Fixes https://github.com/bitcoin/bitcoin/issues/22647
Supersedes https://github.com/bitcoin/bitcoin/pull/22651
ACKs for top commit:
naumenkogs:
utACK 0eea83a85ec6b215d44facc2b16ee1b035275a6b
prayank23:
reACK 0eea83a85e
jonatack:
ACK 0eea83a85ec6b215d44facc2b16ee1b035275a6b code review, rebased to master, debug built, and did some manual testing with various config options on signet
Tree-SHA512: 37d68b449dd6d2715843fc84d85f48fa2508be40ea105a7f4a28443b318d0b6bd39e3b2ca2a6186f2913836adf08d91038a8b142928e1282130f39ac81aa741b
By default, for mainnet, the p2p listening port is 8333. Bitcoin Core
has a strong preference for only connecting to nodes that listen on that
port.
Remove that preference because connections over clearnet that involve
port 8333 make it easy to detect, analyze, block or divert Bitcoin p2p
traffic before the connection is even established (at TCP SYN time).
For further justification see the OP of:
https://github.com/bitcoin/bitcoin/pull/23306
ef5014d256638735b292672c774446db4003f03b style: wrap long lines in CNode creation and add some comments (Vasil Dimov)
b68349164827f14c472201cad54c4e19a3321261 scripted-diff: rename CNode::cs_hSocket to CNode::m_sock_mutex (Vasil Dimov)
c41a1162ac4da437c5d755e8fe2bf636bed22b0f net: use Sock in CNode (Vasil Dimov)
c5dd72e146dd8fa77d29c8689a42322a4d1ec780 fuzz: move FuzzedSock earlier in src/test/fuzz/util.h (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Change `CNode` to use a pointer to `Sock` instead of a bare `SOCKET`.
This will help mocking / testing / fuzzing more code.
ACKs for top commit:
jonatack:
re-ACK ef5014d256638735b292672c774446db4003f03b changes since last review are the removal of an unneeded dtor and the addition of a style commit
w0xlt:
reACK ef5014d
PastaPastaPasta:
utACK ef5014d256638735b292672c774446db4003f03b, I have reviewed the code, and believe it makes sense to merge
theStack:
Cod-review ACK ef5014d256638735b292672c774446db4003f03b
Tree-SHA512: 7f5414dd339cd2f16f7cbdc5fcec238d68b6d50072934aea10b901f409da28ff1ece6db6e899196616aa8127b8b25ab5b86d000bdcee58b4cadd7a3c1cf560c5
Warning: Replacing fs::system_complete calls with fs::absolute calls
in this commit may cause minor changes in behaviour because fs::absolute
no longer strips trailing slashes; however these changes are believed to
be safe.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
fa5d2e678c809c26bd40d7e7c171529d3ffb5903 Remove unused char serialize (MarcoFalke)
fa24493d6394b3a477535f480664c9596f18e3c5 Use spans of std::byte in serialize (MarcoFalke)
fa65bbf217b725ada35107b4ad646d250228355c span: Add BytePtr helper (MarcoFalke)
Pull request description:
This changes the serialize code (`.read()` and `.write()` functions) to take a `Span` instead of a pointer and size. This is a breaking change for the serialize interface, so at no additional cost we can also switch to `std::byte` (instead of using `char`).
The benefits of using `Span`:
* Less verbose and less fragile code when passing an already existing `Span`(-like) object to or from serialization
The benefits of using `std::byte`:
* `std::byte` can't accidentally be mistaken for an integer
The goal here is to only change serialize to use spans of `std::byte`. If needed, `AsBytes`, `MakeUCharSpan`, ... can be used (temporarily) to pass spans of the right type.
Other changes that are included here:
* [#22167](https://github.com/bitcoin/bitcoin/pull/22167) (refactor: Remove char serialize by MarcoFalke)
* [#21906](https://github.com/bitcoin/bitcoin/pull/21906) (Preserve const in cast on CTransactionSignatureSerializer by promag)
ACKs for top commit:
laanwj:
Concept and code review ACK fa5d2e678c809c26bd40d7e7c171529d3ffb5903
sipa:
re-utACK fa5d2e678c809c26bd40d7e7c171529d3ffb5903
Tree-SHA512: 08ee9eced5fb777cedae593b11e33660bed9a3e1711a7451a87b835089a96c99ce0632918bb4666a4e859c4d020f88fb50f2dd734216b0c3d1a9a704967ece6f
This commit extends our inbound eviction protection to CJDNS peers to
favorise the diversity of peer connections, as peers connected
through the CJDNS network are otherwise disadvantaged by our eviction
criteria for their higher latency (higher min ping times) relative
to IPv4 and IPv6 peers.
The `networks` array is order-dependent in the case of a tie in
candidate counts between networks; earlier array members receive
priority in the case of a tie.
Therefore, we place CJDNS candidates before I2P, localhost, and onion
ones in terms of opportunity to recover unused remaining protected
slots from the previous iteration, estimating that most nodes allowing
several inbound privacy networks will have more onion, localhost or
I2P peers than CJDNS ones, as CJDNS support is only being added in the
upcoming v23.0 release.
dec787d8ac2e8fb42db87431dd622bf44897bc4e refactor: replace RecursiveMutex `m_addr_local_mutex` with Mutex (w0xlt)
93609c1dfad70961697d0d12bf01cd34b8ceb6c8 p2p: add assertions and negative TS annotations for m_addr_local_mutex (w0xlt)
c4a31ca267f74bff76a43878177d05d22825a203 scripted-diff: rename cs_addrLocal -> m_addr_local_mutex (w0xlt)
Pull request description:
This PR is related to #19303 and gets rid of the `RecursiveMutex cs_addrLocal`.
ACKs for top commit:
hebasto:
ACK dec787d8ac2e8fb42db87431dd622bf44897bc4e, I have reviewed the code and it looks OK, I agree it can be merged.
shaavan:
reACK dec787d8ac2e8fb42db87431dd622bf44897bc4e
Tree-SHA512: b7a043bfd4e2ccbe313bff21ad815169db6ad215ca96daf358ce960c496a548b4a9e90be9e4357430ca59652b96df87c097450118996c6d4703cbaabde2072d0
224d87855ec38cc15866d9673e1b19942a82c1cd net, refactor: Drop tautological local variables (Hennadii Stepanov)
3073a9917b31d15ba958ea8148585633ba905f8b scripted-diff: Rename CNetMessage::m_command with CNetMessage::m_type (Hennadii Stepanov)
Pull request description:
https://github.com/bitcoin/bitcoin/pull/18533#issue-594592488:
> a message is not a command, but simply a message of some type
Continuation of bitcoin/bitcoin#18533 and bitcoin/bitcoin#18937.
ACKs for top commit:
theStack:
Concept and code-review ACK 224d87855ec38cc15866d9673e1b19942a82c1cd
shaavan:
Code Review ACK 224d87855ec38cc15866d9673e1b19942a82c1cd
w0xlt:
crACK 224d878
Tree-SHA512: 898cafb44708dae1413fcc1533d809d75878891354f1b5edaaec1287f4921c31adc9330f4d42d82544a39689886bc17fee71ea587f9199fd5cc849d376f82176
9b8dcb25b57ad31b77c9f37d9a1f5b07dc6378b4 [net processing] Rename PoissonNextSendInbound to NextInvToInbounds (John Newbery)
ea99f5d01e56ab0192d211da1034ffb299876937 [net processing] Move PoissonNextSendInbound to PeerManager (John Newbery)
bb060746df22c956b8f44e5b8cd1ae4ed73faddc scripted-diff: replace PoissonNextSend with GetExponentialRand (John Newbery)
03cfa1b6035dbcf6a414f9bc432bd9e612801ebb [refactor] Use uint64_t and std namespace in PoissonNextSend (John Newbery)
9e64d69bf74c8a381fb59841519cc3736bce14d4 [move] Move PoissonNextSend to src/random and update comment (John Newbery)
Pull request description:
`PoissonNextSend` and `PoissonNextSendInbound` are used in the p2p code to obfuscate various regularly occurring processes, in order to make it harder for others to get timing-based information deterministically.
The naming of these functions has been confusing to several people (including myself, see also #23347) because the resulting random timestamps don't follow a Poisson distribution but an exponential distribution (related to events in a Poisson process, hence the name). This PR
- moves `PoissonNextSend()` out of `net` to `random` and renames it to `GetExponentialRand()`
- moves `PoissonNextSendInbound()` out of `CConnman` to `PeerManager` and renames it to `NextInvToInbounds()`
- adds documentation for these functions
This is work by jnewbery - due to him being less active currently, I opened the PR and will address feedback.
ACKs for top commit:
jnewbery:
ACK 9b8dcb25b5
hebasto:
ACK 9b8dcb25b57ad31b77c9f37d9a1f5b07dc6378b4, I have reviewed the code and it looks OK, I agree it can be merged.
theStack:
ACK 9b8dcb25b57ad31b77c9f37d9a1f5b07dc6378b4 📊
Tree-SHA512: 85c366c994e7147f9981fe863fb9838502643fa61ffd32d55a43feef96a38b79a5daa2c4d38ce01074897cc95fa40c76779816edad53f5265b81b05c3a1f4f50
-BEGIN VERIFY SCRIPT-
s() { sed -i 's/cs_mapLocalHost/g_maplocalhost_mutex/g' $1; }
s src/net.cpp
s src/net.h
s src/rpc/net.cpp
s src/test/net_tests.cpp
-END VERIFY SCRIPT-
-BEGIN VERIFY SCRIPT-
sed -i 's/std::string m_command;/std::string m_type;/g' ./src/net.h
sed -i 's/* command and size./* type and size./g' ./src/net.h
sed -i 's/msg.m_command/msg.m_type/g' ./src/net.cpp ./src/net_processing.cpp ./src/test/fuzz/p2p_transport_serialization.cpp
-END VERIFY SCRIPT-
This distribution is used for more than just the next inv send, so make
the name more generic.
Also rename to "exponential" to avoid the confusion that this is a
poisson distribution.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" $(git grep -l "$1" ./src) ; }
ren PoissonNextSend GetExponentialRand
ren "a poisson timer" "an exponential timer"
-END VERIFY SCRIPT-
6bf6e9fd9dece67878595a5f3361851c25833c49 net: change CreateNodeFromAcceptedSocket() to take Sock (Vasil Dimov)
9e3cbfca7c9efa620c0cce73503772805cc1fa82 net: use Sock in CConnman::ListenSocket (Vasil Dimov)
f8bd13f85ae5404adef23a52719d804a5c36b1e8 net: add new method Sock::Accept() that wraps accept() (Vasil Dimov)
Pull request description:
_This is a piece of https://github.com/bitcoin/bitcoin/pull/21878, chopped off to ease review._
Introduce an `accept(2)` wrapper `Sock::Accept()` and extend the usage of `Sock` in `CConnman::ListenSocket` and `CreateNodeFromAcceptedSocket()`.
ACKs for top commit:
laanwj:
Code review ACK 6bf6e9fd9dece67878595a5f3361851c25833c49
jamesob:
ACK 6bf6e9fd9dece67878595a5f3361851c25833c49 ([`jamesob/ackr/21879.2.vasild.wrap_accept_and_extend_u`](https://github.com/jamesob/bitcoin/tree/ackr/21879.2.vasild.wrap_accept_and_extend_u))
jonatack:
ACK 6bf6e9fd9dece67878595a5f3361851c25833c49 per `git range-diff ea989de 976f6e8 6bf6e9f` -- only change since my last review was `s/listen_socket.socket/listen_socket.sock->Get()/` in `src/net.cpp: CConnman::SocketHandlerListening()` -- re-read the code changes, rebase/debug build/ran units following my previous full review (https://github.com/bitcoin/bitcoin/pull/21879#pullrequestreview-761251278)
w0xlt:
tACK 6bf6e9f
Tree-SHA512: dc6d1acc4f255f1f7e8cf6dd74e97975cf3d5959e9fc2e689f74812ac3526d5ee8b6a32eca605925d10a4f7b6ff1ce5e900344311e587d19786b48c54d021b64
fad943821e35d0eb2143061e718f0193e12a4c71 scripted-diff: Rename touched member variables (MarcoFalke)
fa663a4c0d20487ed3f7a93e1c2ca9932b05f5a8 Use mockable time for peer connection time (MarcoFalke)
fad7ead146a152f46b25ce6623e05cbb1dbc8cca refactor: Use type-safe std::chrono in net (MarcoFalke)
Pull request description:
Benefits:
* Type-safe
* Mockable
* Allows to revert a temporary test workaround
ACKs for top commit:
naumenkogs:
ACK fad943821e35d0eb2143061e718f0193e12a4c71
shaavan:
ACK fad943821e35d0eb2143061e718f0193e12a4c71
Tree-SHA512: af9bdfc695ab727b100c6810a7289d29b02b0ea9fa4fee9cc1f3eeefb52c8c465ea2734bae0c1c63b3b0d6264ba2c493268bc970ef6916570eb166de77829d82
eaf6be0114a6d7763767da9496907fe8a670ff9e [net processing] Do not request transaction relay from feeler connections (John Newbery)
0220b834b175dc8c45a2c60213474a72c0ef8193 [test] Add testing for outbound feeler connections (John Newbery)
Pull request description:
Feelers are short-lived connections used to test the viability of peers. The bitcoind node will periodically open feeler connections to addresses in its addrman, wait for a `version` message from the peer, and then close the connection.
Currently, we set `fRelay` to `1` in the `version` message for feeler connections, indicating that we want the peer to relay transactions to us. However, we close the connection immediately on receipt of the `version` message, and so never process any incoming transaction announcements. This PR changes that behaviour to instead set `fRelay` to `0` indicating that we do not wish to receive transaction announcements from the peer.
This PR also extends the `addconnection` RPC to allow creating outbound feeler connections from the node to the test framework, and a test to verify that the node sets `fRelay` to `0` in the `version` message to feeler connections.
ACKs for top commit:
naumenkogs:
ACK eaf6be0114a6d7763767da9496907fe8a670ff9e
MarcoFalke:
review ACK eaf6be0114a6d7763767da9496907fe8a670ff9e 🏃
Tree-SHA512: 1c56837dbd0a396fe404a5e39f7459864d15f666664d6b35ad109628b13158e077e417e586bf48946a23bd5cbe63716cb4bf22cdf8781b74dfce6047b87b465a
fadc0c80ae4e526fb2b503f7cc02f6122aaf1de5 p2p: Make timeout mockable and type safe, speed up test (MarcoFalke)
fa6d5a238d2c94440105ddd4f1554f85659d6c5b 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 fadc0c80ae4e526fb2b503f7cc02f6122aaf1de5
naumenkogs:
ACK fadc0c80ae4e526fb2b503f7cc02f6122aaf1de5
Tree-SHA512: 28c6544c97f188c8a0fbc80411c74ab74ffd055885322c325aa3d1c404b29c3fd70a737e86083eecae58ef394db1cb56bc122d06cff63742aa89a8e868730c64
