5730a43703f7e5a5ca26245ba3b55fbdd027d0b6 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f93979b274a37de013502b05d25fad8 net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d9072b7d5a36a6491784bdeb9247e91fb0 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450dde6a54bd785504c923adfb45c7060 p2p: AddrFetch - don't disconnect on self-announcements (Martin Zumsande)
Pull request description:
AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via `getaddr` and disconnect after receiving them.
This is done by disconnecting after receiving the first `addr`. However, it is no longer working as intended, because nowadays, the first `addr` a typical bitcoin core node sends is its self-announcement.
So we'll disconnect before the peer gets a chance to answer our `getaddr`.
I checked that this affects both `-seednode` peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn't work for us.
The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more `addr`. This is silly and not in line with AddrFetch peer being intended to be short-lived peers.
Fix this by only disconnecting after receiving an `addr` message of size > 1.
[Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven't received any suitable `addr`, and a functional test.
ACKs for top commit:
amitiuttarwar:
reACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6
naumenkogs:
ACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
b1d905c225e87a4a289c0cd3593c6c21cea3fba7 p2p: earlier continuation when no remaining eviction candidates (Vasil Dimov)
c9e8d8f9b168dec2bc7b845da38449e96708cf8e p2p: process more candidates per protection iteration (Jon Atack)
02e411ec456af80d1da76085a814c68bb3aca6de p2p: iterate eviction protection only on networks having candidates (Jon Atack)
5adb06457403f8c1d874e9c6748ecbb78ef8fa2b bench: add peer eviction protection benchmarks (Jon Atack)
566357f8f7471f74729297868917aa32f6d3c390 refactor: move GetRandomNodeEvictionCandidates() to test utilities (Jon Atack)
Pull request description:
This follow-up to #21261 improves `ProtectEvictionCandidatesByRatio()` for better performance.
Benchmarks are added; the performance improvement is between 2x and 5x for the benchmarked cases (CPU 2.50GHz, Turbo off, performance mode, Debian Clang 11 non-debug build).
```
$ ./src/bench/bench_bitcoin -filter="EvictionProtection*.*"
```
The refactored code is well-covered by existing unit tests and also a fuzzer.
- `$ ./src/test/test_bitcoin -t net_peer_eviction_tests`
- `$ FUZZ=node_eviction ./src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/node_eviction`
ACKs for top commit:
klementtan:
Tested and code review ACK b1d905c2.
vasild:
ACK b1d905c225e87a4a289c0cd3593c6c21cea3fba7
jarolrod:
ACK b1d905c225e87a4a289c0cd3593c6c21cea3fba7
Tree-SHA512: a3a6607b9ea2fec138da9780c03f63e177b6712091c5a3ddc3804b896a7585216446310280791f5e20cc023d02d2f03a4139237e12b5c1d7f2a1fa1011610e96
4101ec9d2e05a35c35f587a28f1feee6cebcc61b doc: mention that we enforce port=0 in I2P (Vasil Dimov)
e0a2b390c144e123e2fc8a289fdff36815476964 addrman: reset I2P ports to 0 when loading from disk (Vasil Dimov)
41cda9d075ebcab1dbb950160ebe9d0ba7b5745e test: ensure I2P ports are handled as expected (Vasil Dimov)
4f432bd738c420512a86a51ab3e00323f396b89e net: do not connect to I2P hosts on port!=0 (Vasil Dimov)
1f096f091ebd88efb18154b8894a38122c39624f net: distinguish default port per network (Vasil Dimov)
aeac3bce3ead1f24ca782079ef0defa86fd8cb98 net: change I2P seeds' ports to 0 (Vasil Dimov)
38f900290cc3a839e99bef13474d35e1c02e6b0d net: change assumed I2P port to 0 (Vasil Dimov)
Pull request description:
_This is an alternative to https://github.com/bitcoin/bitcoin/pull/21514, inspired by https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-815049933. They are mutually exclusive. Just one of them should be merged._
Change assumed ports for I2P to 0 (instead of the default 8333) as this is closer to what actually happens underneath with SAM 3.1 (https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-812632520, https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-816564719).
Don't connect to I2P peers with advertised port != 0 (we don't specify a port to our SAM 3.1 proxy and it always connects to port = 0).
Note, this change:
* Keeps I2P addresses with port != 0 in addrman and relays them to others via P2P gossip. There may be non-bitcoin-core-22.0 peers using SAM 3.2 and for them such addresses may be useful.
* Silently refuses to connect to I2P hosts with port != 0. This is ok for automatically chosen peers from addrman. Not so ok for peers provided via `-addnode` or `-connect` - a user who specifies `foo.b32.i2p:1234` (non zero port) may wonder why "nothing is happening".
Fixes#21389
ACKs for top commit:
laanwj:
Code review ACK 4101ec9d2e05a35c35f587a28f1feee6cebcc61b
jonatack:
re-ACK 4101ec9d2e05a35c35f587a28f1feee6cebcc61b per `git range-diff efff9c3 0b0ee03 4101ec9`, built with DDEBUG_ADDRMAN, did fairly extensive testing on mainnet both with and without a peers.dat / -dnsseeds=0 to test boostrapping.
Tree-SHA512: 0e3c019e1dc05e54f559275859d3450e0c735596d179e30b66811aad9d5b5fabe3dcc44571e8f7b99f9fe16453eee393d6e153454dd873b9ff14907d4e6354fe
2feec3ce3130961f98ceb030951d0e46d3b9096c net: don't bind on 0.0.0.0 if binds are restricted to Tor (Vasil Dimov)
Pull request description:
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
ACKs for top commit:
laanwj:
Code review ACK 2feec3ce3130961f98ceb030951d0e46d3b9096c
jonatack:
utACK 2feec3ce3130961f98ceb030951d0e46d3b9096c per `git diff a004833 2feec3c`
hebasto:
ACK 2feec3ce3130961f98ceb030951d0e46d3b9096c, tested on Linux Mint 20.1 (x86_64):
Tree-SHA512: a04483af601706da928958b92dc560f9cfcc78ab0bb9d74414636eed1c6f29ed538ce1fb5a17d41ed82c9c9a45ca94899d0966e7ef93da809c9bcdcdb1d1f040
in ProtectEvictionCandidatesByRatio().
With this change, `if (n.count == 0) continue;` will be true
if a network had candidates protected in the first iterations
and has no candidates remaining to be protected in later iterations.
Co-authored-by: Jon Atack <jon@atack.com>
for the usual case when some of the protected networks
don't have eviction candidates, to reduce the number
of iterations in ProtectEvictionCandidatesByRatio().
Picks up an idea in ef411cd2 that I had dropped.
in ProtectEvictionCandidatesByRatio().
Thank you to Vasil Dimov, whose suggestions during a post-merge
discussion about PR 21261 reminded me that I had done this in
earlier versions of the PR, e.g. commits like ef411cd2.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
79c02c88b347f1408a2db307db2654917f9b0bcc Randomize message processing peer order (Pieter Wuille)
Pull request description:
Right now, the message handling loop iterates the list of nodes always in the same order: the order they were connected in (see the `vNodes` vector). For some parts of the net processing logic, this order matters. Transaction requests are assigned explicitly to peers since #19988, but many other parts of processing work on a "first-served-by-loop-first" basis, such as block downloading. If peers can predict this ordering, it may be exploited to cause delays.
As there isn't anything particularly optimal about the current ordering, just make it unpredictable by randomizing.
Reported by Crypt-iQ.
ACKs for top commit:
jnewbery:
ACK 79c02c88b3
Crypt-iQ:
ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
sdaftuar:
utACK 79c02c88b347f1408a2db307db2654917f9b0bcc
achow101:
Code Review ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
jamesob:
crACK 79c02c88b3
jonatack:
ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
vasild:
ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
theStack:
ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
Tree-SHA512: 9a87c4dcad47c2d61b76c4f37f59674876b78f33f45943089bf159902a23e12de7a5feae1a73b17cbc3f2e37c980ecf0f7fd86af9e6fa3a68099537a3c82c106
This commit extends our inbound eviction protection to I2P peers to
favorise the diversity of peer connections, as peers connected
through the I2P network are otherwise disadvantaged by our eviction
criteria for their higher latency (higher min ping times) relative
to IPv4 and IPv6 peers, as well as relative to Tor onion 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 I2P candidates before localhost and onion ones
in terms of opportunity to recover unused remaining protected slots
from the previous iteration, guesstimating that most nodes allowing
both onion and I2P inbounds will have more onion peers, followed by
localhost, then I2P, as I2P support is only being added in the
upcoming v22.0 release.
with a more abstract framework to allow easily extending inbound
eviction protection to peers connected through new higher-latency
networks that are disadvantaged by our inbound eviction criteria,
such as I2P and perhaps other BIP155 networks in the future like
CJDNS. This is a change in behavior.
The algorithm is a basically a multi-pass knapsack:
- Count the number of eviction candidates in each of the disadvantaged
privacy networks.
- Sort the networks from lower to higher candidate counts, so that
a network with fewer candidates will have the first opportunity
for any unused slots remaining from the previous iteration. In
the case of a tie in candidate counts, priority is given by array
member order from first to last, guesstimated to favor more unusual
networks.
- Iterate through the networks in this order. On each iteration,
allocate each network an equal number of protected slots targeting
a total number of candidates to protect, provided any slots remain
in the knapsack.
- Protect the candidates in that network having the longest uptime,
if any in that network are present.
- Continue iterating as long as we have non-allocated slots
remaining and candidates available to protect.
Localhost peers are treated as a network like Tor or I2P by aliasing
them to an unused Network enumerator: Network::NET_MAX.
The goal is to favorise diversity of our inbound connections.
Credit to Vasil Dimov for improving the algorithm from single-pass
to multi-pass to better allocate unused protection slots.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
as EraseLastKElements() called in the next line performs the same operation.
Thanks to Martin Zumsande (lightlike) for seeing this while reviewing.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
ffff0d04425a616c14fc4a562e8ef93d286705f8 refactor: Switch serialize to uint8_t (1/n) (MarcoFalke)
Pull request description:
Replace `char` -> `uint8_t` in serialization where a sign doesn't make sense (char might be signed/unsigned).
ACKs for top commit:
practicalswift:
cr ACK ffff0d04425a616c14fc4a562e8ef93d286705f8: patch looks correct and commit hash is ffffresh (was bbbbadass)
kristapsk:
ACK ffff0d04425a616c14fc4a562e8ef93d286705f8
Tree-SHA512: cda682280c21d37cc3a6abd62569732079b31d18df3f157aa28bed80bd6f9f29a7db5c133b1f57b3a8f8d5ba181a76e473763c6e26a2df6d9244813f56f893ee
fe3d17df04decc4e856121eb311636977d60f80f net: ignore block-relay-only peers when skipping DNS seed (Anthony Towns)
Pull request description:
Since #17428 bitcoind will attempt to reconnect to two block-relay-only anchors before doing any other outbound connections. When determining whether to use DNS seeds, it will currently see these two peers and decide "we're connected to the p2p network, so no need to lookup DNS" -- but block-relay-only peers don't do address relay, so if your address book is full of invalid addresses (apart from your anchors) this behaviour will prevent you from recovering from that situation.
This patch changes it so that it only skips use of DNS seeds when there are two full-outbound peers, not just block-relay-only peers.
ACKs for top commit:
Sjors:
utACK fe3d17d
amitiuttarwar:
ACK fe3d17df04decc4e856121eb311636977d60f80f, this impacts the very common case where we stop/start a node, persisting anchors & have a non-empty addrman (although, to be clear, wouldn't be particularly problematic in the common cases where the addrman has valid addresses)
mzumsande:
ACK fe3d17df04decc4e856121eb311636977d60f80f
jonatack:
ACK fe3d17df04decc4e856121eb311636977d60f80f
prayank23:
tACK fe3d17df04
Tree-SHA512: 9814b0d84321d7f45b5013eb40c420a0dd93bf9430f5ef12dce50d1912a18d5de2070d890a8c6fe737a3329b31059b823bc660b432d5ba21f02881dc1d951e94
0829516d1f3868c1c2ba507feee718325d81e329 [refactor] Remove unused ForEachNodeThen() template (John Newbery)
09cc66c00e1d5fabe11ffcc32cad060e6b483b20 scripted-diff: rename address relay fields (John Newbery)
76568a3351418c878d30ba0373cf76988f93f90e [net processing] Move addr relay data and logic into net processing (John Newbery)
caba7ae8a505a4b4680a9d7618f65c4e8579a1e2 [net processing] Make RelayAddress() a member function of PeerManagerImpl (John Newbery)
86acc9646968213aaa4408635915b1bfd75a10c9 [net processing] Take NodeId instead of CNode* as originator for RelayAddress() (John Newbery)
Pull request description:
This continues the work of moving application layer data into net_processing, by moving all addr data into the new Peer object added in #19607.
For motivation, see #19398.
ACKs for top commit:
laanwj:
Code review ACK 0829516d1f3868c1c2ba507feee718325d81e329
mzumsande:
ACK 0829516d1f3868c1c2ba507feee718325d81e329, reviewed the code and ran tests.
sipa:
utACK 0829516d1f3868c1c2ba507feee718325d81e329
hebasto:
re-ACK 0829516d1f3868c1c2ba507feee718325d81e329
Tree-SHA512: efe0410fac288637f203eb37d1999910791e345872d37e1bd5cde50e25bb3cb1c369ab86b3a166ffd5e06ee72e4508aa2c46d658be6a54e20b4f220d2f57d0a6
7075f604e8d0b21b2255fa57e20cd365dc10a288 scripted-diff: update noban documentation in net_processing.cpp (Jon Atack)
a95540cf435029f06e56749802d71315ca76b0dd scripted-diff: rename NetPermissionFlags enumerators (Jon Atack)
810d0929c1626bba141af3f779a3c9cd6ece7e75 p2p, refactor: make NetPermissionFlags a uint32 enum class (Jon Atack)
7b55a9449778c5ac89799ce4c607c8c8d797ddfb p2p: NetPermissions::HasFlag() pass flags param by value (Jon Atack)
91f6e6e6d1720e1154ad3f70a5098e9028efa84a scripted-diff: add NetPermissionFlags scopes where not already present (Jon Atack)
Pull request description:
While reviewing #20196, I noticed the `NetPermissionFlags` enums are frequently called as if they were scoped, yet are still global. This patch upgrades `NetPermissionFlags` to a scoped class enum and updates the enumerator naming, similarly to #19771. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#enum-enumerations for more info.
This change would eliminate the class of bugs like https://github.com/bitcoin/bitcoin/pull/20196#discussion_r610770148 and #21644, as only defined operations on the flags would compile.
ACKs for top commit:
laanwj:
Code review ACK 7075f604e8d0b21b2255fa57e20cd365dc10a288
vasild:
ACK 7075f604e8d0b21b2255fa57e20cd365dc10a288
Tree-SHA512: 7fcea66ee499f059efc78c934b5f729b3c8573fe304dee2c27c837c2f662b89324790568246d75b2a574cf9f059b42d3551d928996862f4358055eb43521e6f4
105941b726c078642e785ecb7b6834ba814381b0 net: use stronger AddLocal() for our I2P address (Vasil Dimov)
Pull request description:
There are two issues:
### 1. Our I2P address not added to local addresses.
* `externalip=` is used with an IPv4 address (this sets automatically `discover=0`)
* No `discover=1` is used
* `i2psam=` is used
* No `externalip=` is used for our I2P address
* `listenonion=1 torcontrol=` are used
In this case `AddLocal(LOCAL_MANUAL)` [is used](94f83534e4/src/torcontrol.cpp (L354)) for our `.onion` address and `AddLocal(LOCAL_BIND)` [for our](94f83534e4/src/net.cpp (L2247)) `.b32.i2p` address, the latter being [ignored](94f83534e4/src/net.cpp (L232-L233)) due to `discover=0`.
### 2. Our I2P address removed from local addresses even if specified with `externalip=` on I2P proxy restart.
* `externalip=` is used with our I2P address (this sets automatically `discover=0`)
* No `discover=1` is used
* `i2psam=` is used
In this case, initially `externalip=` causes our I2P address to be [added](94f83534e4/src/init.cpp (L1266)) with `AddLocal(LOCAL_MANUAL)` which overrides `discover=0` and works as expected. However, if later the I2P proxy is shut down [we do](94f83534e4/src/net.cpp (L2234)) `RemoveLocal()` in order to stop advertising our I2P address (since we have lost I2P connectivity). When the I2P proxy is started and we reconnect to it, restoring the I2P connectivity, [we do](94f83534e4/src/net.cpp (L2247)) `AddLocal(LOCAL_BIND)` which does nothing due to `discover=0`.
To resolve those two issues, use `AddLocal(LOCAL_MANUAL)` for I2P which is also what we do with Tor.
ACKs for top commit:
laanwj:
Code review ACK 105941b726c078642e785ecb7b6834ba814381b0
Tree-SHA512: 0c9daf6116b8d9c34ad7e6e9bbff6e8106e94e4394a815d7ae19287aea22a8c7c4e093c8dd8c58a4a1b1412b2575a9b42b8a93672c8d17f11c24508c534506c7
- drop redundant PF_ permission flags prefixes
- drop ALL_CAPS naming per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-caps
- rename IsImplicit to Implicit
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
s 'PF_NONE' 'None'
s 'PF_BLOOMFILTER' 'BloomFilter'
s 'PF_RELAY' 'Relay'
s 'PF_FORCERELAY' 'ForceRelay'
s 'PF_DOWNLOAD' 'Download'
s 'PF_NOBAN' 'NoBan'
s 'PF_MEMPOOL' 'Mempool'
s 'PF_ADDR' 'Addr'
s 'PF_ISIMPLICIT' 'Implicit'
s 'PF_ALL' 'All'
-END VERIFY SCRIPT-
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" -- 'src' ':!src/net_permissions.h' | xargs sed -i -E "s/([^:])$1/\1NetPermissionFlags::$1/"; }
s 'PF_NONE'
s 'PF_BLOOMFILTER'
s 'PF_RELAY'
s 'PF_FORCERELAY'
s 'PF_DOWNLOAD'
s 'PF_NOBAN'
s 'PF_MEMPOOL'
s 'PF_ADDR'
s 'PF_ISIMPLICIT'
s 'PF_ALL'
-END VERIFY SCRIPT-
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
792be53d3e9e366b9f6aeee7a1eeb912fa28062e refactor: Replace std::bind with lambdas (Hennadii Stepanov)
a508f718f3e087c96a306399582a85df2e1d53ae refactor: Use appropriate thread constructor (Hennadii Stepanov)
30e44482152488a78f2c495798a75e6f553dc0c8 refactor: Make TraceThread a non-template free function (Hennadii Stepanov)
Pull request description:
This PR does not change behavior.
Its goal is to improve readability and maintainability of the code.
ACKs for top commit:
jnewbery:
utACK 792be53d3e9e366b9f6aeee7a1eeb912fa28062e
jonatack:
tACK 792be53d3e9e366b9f6aeee7a1eeb912fa28062e
MarcoFalke:
cr ACK 792be53d3e9e366b9f6aeee7a1eeb912fa28062e
Tree-SHA512: a03142f04f370f6bc02bd3ddfa870819b51740fcd028772241d68c84087f95a2d78207cbd5edb3f7c636fcf2d76192d9c59873f8f0af451d3b05c0cf9cf234df
36fb036d25e2a3016b36873456e5a9e6251ffef8 p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT (Jon Atack)
4e0d5788ba5771c81bc0ff2e6523cf9accddae46 test: add net permissions noban/download unit test coverage (Jon Atack)
dde69f20a01acca64ac21cb13993c6e4f8709f23 p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() (Jon Atack)
Pull request description:
This is a bugfix follow-up to #16248 and #19191 that was noticed in #21506. Both v0.21 and master are affected.
Since #19191, noban is a multi-flag that implies download, so the conditional in `CConnman::Bind()` using a bitwise AND on noban will return the same result for both the noban status and the download status. This means that download peers are incorrectly not being added to local addresses because they are mistakenly seen as noban peers.
The second commit adds unit test coverage to illustrate and test the noban/download relationship and the `NetPermissions` operations involving them.
The final commit adds documentation and disallows calling `NetPermissions::ClearFlag()` with any second param other than `NetPermissionFlags` "implicit" -- per current usage in the codebase -- because `ClearFlag()` should not be called with any second param that is a subflag of a multiflag, e.g. "relay" or "download," as that would leave the result in an invalid state corresponding to none of the existing NetPermissionFlags. Thanks to Vasil Dimov for noticing this.
ACKs for top commit:
theStack:
re-ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8 ☕
vasild:
ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8
hebasto:
ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8, I have reviewed the code and it looks OK, I agree it can be merged.
kallewoof:
Code review ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8
Tree-SHA512: 5fbc7ddbf31d06b35bf238f4d77ef311e6b6ef2e1bb9893f32f889c1a0f65774a3710dcb21d94317fe6166df9334a9f2d42630809e7fe8cbd797dd6f6fc49491
There are two issues:
1. Our I2P address not added to local addresses.
* `externalip=` is used with an IPv4 address (this sets automatically
`discover=0`)
* No `discover=1` is used
* `i2psam=` is used
* No `externalip=` is used for our I2P address
* `listenonion=1 torcontrol=` are used
In this case `AddLocal(LOCAL_MANUAL)` is used for our `.onion` address
and `AddLocal(LOCAL_BIND)` for our `.b32.i2p` address, the latter being
ignored due to `discover=0`.
2. Our I2P address removed from local addresses even if specified
with `externalip=` on I2P proxy restart.
* `externalip=` is used with our I2P address (this sets automatically
`discover=0`)
* No `discover=1` is used
* `i2psam=` is used
In this case, initially `externalip=` causes our I2P address to be added
with `AddLocal(LOCAL_MANUAL)` which overrides `discover=0` and works as
expected. However, if later the I2P proxy is shut down we do
`RemoveLocal()` in order to stop advertising our I2P address (since we
have lost I2P connectivity). When the I2P proxy is started and we
reconnect to it, restoring the I2P connectivity, we do
`AddLocal(LOCAL_BIND)` which does nothing due to `discover=0`.
To resolve those two issues, use `AddLocal(LOCAL_MANUAL)` for I2P which
is also what we do with Tor.
d66f283ac07edce432b964f7f814631f5a5bc33b scripted-diff: Replace three dots with ellipsis in the UI strings (Hennadii Stepanov)
Pull request description:
This PR is split from #21463.
The change was suggested on [Transifex.com](https://www.transifex.com/bitcoin/bitcoin/), and it does not touch `LogPrint` and `LogPrintf` calls.
The only comment on #21463 [was](9030e4b5a6 (r597220100)):
> Mind that these messages also end up in the log. In principle the log is already UTF-8 (as are all strings and text in bitcoind). But, just noting, that it might make browsing the log a less pleasant experience on systems with misconfigured locale like some BSDs by default.
ACKs for top commit:
laanwj:
ACK d66f283ac07edce432b964f7f814631f5a5bc33b
Tree-SHA512: 5ab1cb3160f3f996f1ad7d7486662da3eb7f06a857f4a1874963ce10caed5b86b0ad6151b1b9ebeb2b8aa5f0c85efad3b768ea9cafe5db86f78f88912b756d1e
- Use `SanitizeString` when logging message errors to make sure that the
message type is sanitized.
- For the `MESSAGESTART` error don't inspect and log header details at
all: receiving invalid start bytes makes it likely that the packet isn't
even formatted as valid P2P message. Logging the four unexpected start
bytes should be enough.
- Update `p2p_invalid_messages.py` test to check this.
Issue reported by gmaxwell.
9096b13a4764873511b65f32a005ce4738b0d81c net: remove unnecessary check of CNode::cs_vSend (Vasil Dimov)
Pull request description:
It is not possible to have a node in `CConnman::vNodesDisconnected` and
its reference count to be incremented - all `CNode::AddRef()` are done
either before the node is added to `CConnman::vNodes` or while holding
`CConnman::cs_vNodes` and the object being in `CConnman::vNodes`.
So, the object being in `CConnman::vNodesDisconnected` and its reference
count being zero means that it is not and will not start to be used by
other threads.
So, the lock of `CNode::cs_vSend` in `CConnman::DisconnectNodes()` will
always succeed and is not necessary.
Indeed all locks of `CNode::cs_vSend` are done either when the reference
count is >0 or under the protection of `CConnman::cs_vNodes` and the
node being in `CConnman::vNodes`.
ACKs for top commit:
MarcoFalke:
review ACK 9096b13a4764873511b65f32a005ce4738b0d81c 🏧
jnewbery:
utACK 9096b13a4764873511b65f32a005ce4738b0d81c
Tree-SHA512: 910899cdcdc8934642eb0c40fcece8c3b01b7e20a0b023966b9d6972db6a885cb3a9a04e9562bae14d5833967e45e2ecb3687b94d495060c3da4b1f2afb0ac8f
8c8237a4a10feb2ac9ce46f67b5d14bf879b670f net, refactor: Fix style in CConnman::StopNodes (Hennadii Stepanov)
229ac1892d807a1eea5a7c24ae0fe27dc913b1bd net: Combine two loops into one, and update comments (Hennadii Stepanov)
a3d090d1103cd6c25daf07afdf4e65febca6d3f7 net: Restrict period when cs_vNodes mutex is locked (Hennadii Stepanov)
Pull request description:
This PR restricts the period when the `cs_vNodes` mutex is locked, prevents the only case when `cs_vNodes` could be locked before the `::cs_main`.
This change makes the explicit locking of recursive mutexes in the explicit order redundant.
ACKs for top commit:
jnewbery:
utACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f
vasild:
ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f
ajtowns:
utACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f - logic seems sound
MarcoFalke:
review ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f 👢
Tree-SHA512: a8277924339622b188b12d260a100adf5d82781634cf974320cf6007341f946a7ff40351137c2f5369aed0d318f38aac2d32965c9b619432440d722a4e78bb73
