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>
ffff0d0442 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 ffff0d0442: patch looks correct and commit hash is ffffresh (was bbbbadass)
kristapsk:
ACK ffff0d0442
Tree-SHA512: cda682280c21d37cc3a6abd62569732079b31d18df3f157aa28bed80bd6f9f29a7db5c133b1f57b3a8f8d5ba181a76e473763c6e26a2df6d9244813f56f893ee
fe3d17df04 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 fe3d17df04, 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 fe3d17df04
jonatack:
ACK fe3d17df04
prayank23:
tACK fe3d17df04
Tree-SHA512: 9814b0d84321d7f45b5013eb40c420a0dd93bf9430f5ef12dce50d1912a18d5de2070d890a8c6fe737a3329b31059b823bc660b432d5ba21f02881dc1d951e94
0829516d1f [refactor] Remove unused ForEachNodeThen() template (John Newbery)
09cc66c00e scripted-diff: rename address relay fields (John Newbery)
76568a3351 [net processing] Move addr relay data and logic into net processing (John Newbery)
caba7ae8a5 [net processing] Make RelayAddress() a member function of PeerManagerImpl (John Newbery)
86acc96469 [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 0829516d1f
mzumsande:
ACK 0829516d1f, reviewed the code and ran tests.
sipa:
utACK 0829516d1f
hebasto:
re-ACK 0829516d1f
Tree-SHA512: efe0410fac288637f203eb37d1999910791e345872d37e1bd5cde50e25bb3cb1c369ab86b3a166ffd5e06ee72e4508aa2c46d658be6a54e20b4f220d2f57d0a6
7075f604e8 scripted-diff: update noban documentation in net_processing.cpp (Jon Atack)
a95540cf43 scripted-diff: rename NetPermissionFlags enumerators (Jon Atack)
810d0929c1 p2p, refactor: make NetPermissionFlags a uint32 enum class (Jon Atack)
7b55a94497 p2p: NetPermissions::HasFlag() pass flags param by value (Jon Atack)
91f6e6e6d1 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 7075f604e8
vasild:
ACK 7075f604e8
Tree-SHA512: 7fcea66ee499f059efc78c934b5f729b3c8573fe304dee2c27c837c2f662b89324790568246d75b2a574cf9f059b42d3551d928996862f4358055eb43521e6f4
105941b726 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 105941b726
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>
792be53d3e refactor: Replace std::bind with lambdas (Hennadii Stepanov)
a508f718f3 refactor: Use appropriate thread constructor (Hennadii Stepanov)
30e4448215 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 792be53d3e
jonatack:
tACK 792be53d3e
MarcoFalke:
cr ACK 792be53d3e
Tree-SHA512: a03142f04f370f6bc02bd3ddfa870819b51740fcd028772241d68c84087f95a2d78207cbd5edb3f7c636fcf2d76192d9c59873f8f0af451d3b05c0cf9cf234df
36fb036d25 p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT (Jon Atack)
4e0d5788ba test: add net permissions noban/download unit test coverage (Jon Atack)
dde69f20a0 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 36fb036d25☕
vasild:
ACK 36fb036d25
hebasto:
ACK 36fb036d25, I have reviewed the code and it looks OK, I agree it can be merged.
kallewoof:
Code review ACK 36fb036d25
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.
d66f283ac0 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 d66f283ac0
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.
9096b13a47 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 9096b13a47🏧
jnewbery:
utACK 9096b13a47
Tree-SHA512: 910899cdcdc8934642eb0c40fcece8c3b01b7e20a0b023966b9d6972db6a885cb3a9a04e9562bae14d5833967e45e2ecb3687b94d495060c3da4b1f2afb0ac8f
8c8237a4a1 net, refactor: Fix style in CConnman::StopNodes (Hennadii Stepanov)
229ac1892d net: Combine two loops into one, and update comments (Hennadii Stepanov)
a3d090d110 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 8c8237a4a1
vasild:
ACK 8c8237a4a1
ajtowns:
utACK 8c8237a4a1 - logic seems sound
MarcoFalke:
review ACK 8c8237a4a1👢
Tree-SHA512: a8277924339622b188b12d260a100adf5d82781634cf974320cf6007341f946a7ff40351137c2f5369aed0d318f38aac2d32965c9b619432440d722a4e78bb73
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`.
PF_NOBAN is a multi-flag that includes PF_DOWNLOAD, so the conditional
in CConnman::Bind() using a bitwise AND will return the same result
for both the "noban" status and the "download" status.
Example:
`PF_DOWNLOAD` is `0b1000000`
`PF_NOBAN` is `0b1010000`
This makes a check like `flags & PF_NOBAN` return `true` even if `flags`
is equal to `PF_DOWNLOAD`.
If `-whitebind=download@1.1.1.1:8765` is specified, then `1.1.1.1:8765`
should be added to the list of local addresses. We only want to avoid
adding to local addresses (that are advertised) a whitebind that has a
`noban@` flag.
As a result of a mis-check in `CConnman::Bind()` we would not have added
`1.1.1.1:8765` to the local addresses in the example above.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
b2ee8b207d net: Deserialize hardcoded seeds from BIP155 blob (W. J. van der Laan)
9b29d5df7f contrib: Add explicit port numbers for testnet seeds (W. J. van der Laan)
2a257de113 contrib: Add a few TorV3 seed nodes (W. J. van der Laan)
06030f7a42 contrib: generate-seeds.py generates output in BIP155 format (W. J. van der Laan)
Pull request description:
Closes#20239 and mitigates my node's problem in #21351.
- Add a few hardcoded seeds for TorV3
- As the [bitcoin-seeder](https://github.com/sipa/bitcoin-seeder) doesn't collect TorV3 addresses yet, I have extracted these from my own node using [a script](https://gist.github.com/laanwj/b3d7b01ef61ce07c2eff0a72a6b90183) and added them manually. This is intended to be a temporary stop gap until 22.0's seeds update.
- Change hardcoded seeds to variable length BIP155 binary format.
- It is stored as a single serialized blob in a byte array, instead of pseudo-IPv6 address slots. This is more flexible and, assuming most of the list is IPv4, more compact.
- Only the (networkID, addr, port) subset (CService). Services and time are construed on the fly as before.
- Change input format for `nodes_*.txt`.
- Drop legacy `0xAABBCCDD` format for IPv4. It is never generated by `makeseeds.py`.
- Stop interpreting lack of port as default port, interpret it as 'no port', to accomodate I2P and other port-less protocols (not handled in this PR). An explicit port is always generated by `makeseeds.py` so in practice this makes no difference right now.
A follow-up to this PR could do the same for I2P.
ACKs for top commit:
jonatack:
ACK b2ee8b207d
Tree-SHA512: 11a6b54f9fb0192560f2bd7b218f798f86c1abe01d1bf37f734cb88b91848124beb2de801ca4e6f856e9946aea5dc3ee16b0dbb9863799e42eec1b239d40d59d
4783115fd4 net: add ifaddrs.h include (fanquake)
879215e665 build: check if -lsocket is required with *ifaddrs (fanquake)
87deac66aa rand: only try and use freeifaddrs if available (fanquake)
Pull request description:
Fixes#21485 by linking against `-lsocket` when it's required for using `*ifaddrs` functions.
ACKs for top commit:
laanwj:
Code review ACK 4783115fd4
hebasto:
ACK 4783115fd4, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 4542e036e9b029de970eff8a9230fe45d9204bb22313d075f474295d49bdaf1f1cbb36c0c6e2fa8dbbcdba518d8d3a68a6116ce304b82414315f333baf9af0e4
40316a37cb test: add I2P test for a runaway SAM proxy (Vasil Dimov)
2d8ac77970 fuzz: add tests for the I2P Session public interface (Vasil Dimov)
9947e44de0 i2p: use pointers to Sock to accommodate mocking (Vasil Dimov)
82d360b5a8 net: change ConnectSocketDirectly() to take a Sock argument (Vasil Dimov)
b5861100f8 net: add connect() and getsockopt() wrappers to Sock (Vasil Dimov)
5a887d49b2 fuzz: avoid FuzzedSock::Recv() repeated errors with EAGAIN (Vasil Dimov)
3088f83d01 fuzz: extend FuzzedSock::Recv() to support MSG_PEEK (Vasil Dimov)
9b05c49ade fuzz: implement unimplemented FuzzedSock methods (Vasil Dimov)
Pull request description:
Change the networking code and the I2P code to be fully mockable and use `FuzzedSocket` to fuzz the I2P methods `Listen()`, `Accept()` and `Connect()`.
Add a mocked `Sock` implementation that returns a predefined data on reads and use it for a regression unit test for the bug fixed in https://github.com/bitcoin/bitcoin/pull/21407.
ACKs for top commit:
practicalswift:
Tested ACK 40316a37cb
MarcoFalke:
Concept ACK 40316a37cb
jonatack:
re-ACK 40316a37cb reviewed `git range-diff 01bb3afb 23c861d 40316a3` and the new unit test commit, debug built, ran unit tests, ran bitcoind with an I2P service and network operation with seven I2P peers (2 in, 5 out) is looking nominal
laanwj:
Code review ACK 40316a37cb
Tree-SHA512: 7fc4f129849e16e0c7e16662d9f4d35dfcc369bb31450ee369a2b97bdca95285533bee7787983e881e5a3d248f912afb42b4a2299d5860ace7129b0b19623cc8
0cca08a8ee Add unit test coverage for our onion peer eviction protection (Jon Atack)
caa21f586f Protect onion+localhost peers in ProtectEvictionCandidatesByRatio() (Jon Atack)
8f1a53eb02 Use EraseLastKElements() throughout SelectNodeToEvict() (Jon Atack)
8b1e156143 Add m_inbound_onion to AttemptToEvictConnection() (Jon Atack)
72e30e8e03 Add unit tests for ProtectEvictionCandidatesByRatio() (Jon Atack)
ca63b53ecd Use std::unordered_set instead of std::vector in IsEvicted() (Jon Atack)
41f84d5ecc Move peer eviction tests to a separate test file (Jon Atack)
f126cbd6de Extract ProtectEvictionCandidatesByRatio from SelectNodeToEvict (Jon Atack)
Pull request description:
Now that #19991 and #20210 have been merged, we can determine inbound onion peers using `CNode::m_inbound_onion` and add it to the localhost peers protection in `AttemptToEvictConnection`, which was added in #19670 to address issue #19500.
Update 28 February 2021: I've updated this to follow gmaxwell's suggestion in https://github.com/bitcoin/bitcoin/pull/20197#issuecomment-713865992.
This branch now protects up to 1/4 onion peers (connected via our tor control service), if any, sorted by longest uptime. If any (or all) onion slots remain after that operation, they are then allocated to protect localhost peers, or a minimum of 2 localhost peers in the case that no onion slots remain and 2 or more onion peers were protected, sorted as before by longest uptime.
This patch also adds test coverage for the longest uptime, localhost, and onion peer eviction protection logic to build on the welcome initial unit testing of #20477.
Suggest reviewing the commits that move code with `colorMoved = dimmed-zebra` and `colorMovedWs = allow-indentation-change`.
Closes#11537.
ACKs for top commit:
laanwj:
Code review ACK 0cca08a8ee
vasild:
ACK 0cca08a8ee
Tree-SHA512: 2f5a63f942acaae7882920fc61f0185dcd51da85e5b736df9d1fc72343726dd17da740e02f30fa5dc5eb3b2d8345707aed96031bec143d48a2497a610aa19abd
52dd40a9fe test: add missing netaddress include headers (Jon Atack)
6f09c0f6b5 util: add missing braces and apply clang format to SplitHostPort() (Jon Atack)
2875a764f7 util: add ParseUInt16(), use it in SplitHostPort() (Jon Atack)
6423c8175f p2p, refactor: pass and use uint16_t CService::port as uint16_t (Jon Atack)
Pull request description:
As noticed during review today in https://github.com/bitcoin/bitcoin/pull/20685#discussion_r584873708 of the upcoming I2P network support, `CService::port` is `uint16_t` but is passed around the codebase and into the ctors as `int`, which causes uneeded conversions and casts. We can avoid these (including in the incoming I2P code without further changes to it) by using ports with the correct type. The remaining conversions are pushed out to the user input boundaries where they can be range-checked and raise with user feedback in the next patch.
ACKs for top commit:
practicalswift:
cr ACK 52dd40a9fe: patch looks correct
MarcoFalke:
cr ACK 52dd40a9fe
vasild:
ACK 52dd40a9fe
Tree-SHA512: 203c1cab3189a206c55ecada77b9548b810281cdc533252b8e3330ae0606b467731c75f730ce9deb07cbaab66facf97e1ffd2051084ff9077cba6750366b0432
Now that we have a reliable way to detect inbound onion peers, this commit
updates our existing eviction protection of 1/4 localhost peers to instead
protect up to 1/4 onion peers (connected via our tor control service), sorted by
longest uptime. Any remaining slots of the 1/4 are then allocated to protect
localhost peers, or 2 localhost peers if no slots remain and 2 or more onion
peers are protected, sorted by longest uptime.
The goal is to avoid penalizing onion peers, due to their higher min ping times
relative to IPv4 and IPv6 peers, and improve our diversity of peer connections.
Thank you to Gregory Maxwell, Suhas Daftuar, Vasil Dimov and Pieter Wuille
for valuable review feedback that shaped the direction.
and an `m_is_onion` struct member to NodeEvictionCandidate and tests.
We'll use these in the peer eviction logic to protect inbound onion peers
in addition to the existing protection of localhost peers.
to allow deterministic unit testing of the ratio-based peer eviction protection
logic, which protects peers having longer connection times and those connected
via higher-latency networks.
Add documentation.
Change the types of `i2p::Connection::sock` and
`i2p::sam::Session::m_control_sock` from `Sock` to
`std::unique_ptr<Sock>`.
Using pointers would allow us to sneak `FuzzedSock` instead of `Sock`
and have the methods of the former called.
After this change a test only needs to replace `CreateSock()` with
a function that returns `FuzzedSock`.
Change `ConnectSocketDirectly()` to take a `Sock` argument instead of a
bare `SOCKET`. With this, use the `Sock`'s (possibly mocked) methods
`Connect()`, `Wait()` and `GetSockOpt()` instead of calling the OS
functions directly.
Introduce two new options to reach the I2P network:
* `-i2psam=<ip:port>` point to the I2P SAM proxy. If this is set then
the I2P network is considered reachable and we can make outgoing
connections to I2P peers via that proxy. We listen for and accept
incoming connections from I2P peers if the below is set in addition to
`-i2psam=<ip:port>`
* `-i2pacceptincoming` if this is set together with `-i2psam=<ip:port>`
then we accept incoming I2P connections via the I2P SAM proxy.
Our local (bind) address is already saved in `CNode::addrBind` and there
is no need to re-retrieve it again with `GetBindAddress()`.
Also, for I2P connections `CNode::addrBind` would contain our I2P
address, but `GetBindAddress()` would return something like
`127.0.0.1:RANDOM_PORT`.
Isolate the second half of `CConnman::AcceptConnection()` into a new
separate method, which could be reused if we accept incoming connections
by other means than `accept()` (first half of
`CConnman::AcceptConnection()`).
Call `GetBindAddress()` earlier in `CConnman::AcceptConnection()`. That
is specific to the TCP protocol and makes the code below it reusable for
other protocols, if the caller provides `addr_bind`, retrieved by other
means.
This check is related to an `accept()` failure. So do the check earlier,
closer to the `accept()` call.
This will allow to isolate the `accept()`-specific code at the beginning
of `CConnman::AcceptConnection()` and reuse the code that follows it.
faf48f20f1 log: Clarify log message when file does not exist (MarcoFalke)
Pull request description:
Shorter and broader alternative to #21181
Rendered diff:
```diff
@@ -1,4 +1,4 @@
-Bitcoin Core version v21.99.0-db656db2ed5a (release build)
+Bitcoin Core version v21.99.0-faf48f20f196 (release build)
Qt 5.15.2 (dynamic), plugin=wayland (dynamic)
No static plugins.
Style: adwaita / Adwaita::Style
@@ -24,8 +24,8 @@ scheduler thread start
Using wallet directory /tmp/test_001/regtest/wallets
init message: Verifying wallet(s)...
init message: Loading banlist...
-ERROR: DeserializeFileDB: Failed to open file /tmp/test_001/regtest/banlist.dat
-Invalid or missing banlist.dat; recreating
+Missing or invalid file /tmp/test_001/regtest/banlist.dat
+Recreating banlist.dat
SetNetworkActive: true
Failed to read fee estimates from /tmp/test_001/regtest/fee_estimates.dat. Continue anyway.
Using /16 prefix for IP bucketing
@@ -63,9 +63,9 @@ Bound to [::]:18444
Bound to 0.0.0.0:18444
Bound to 127.0.0.1:18445
init message: Loading P2P addresses...
-ERROR: DeserializeFileDB: Failed to open file /tmp/test_001/regtest/peers.dat
-Invalid or missing peers.dat; recreating
-ERROR: DeserializeFileDB: Failed to open file /tmp/test_001/regtest/anchors.dat
+Missing or invalid file /tmp/test_001/regtest/peers.dat
+Recreating peers.dat
+Missing or invalid file /tmp/test_001/regtest/anchors.dat
0 block-relay-only anchors will be tried for connections.
init message: Starting network threads...
net thread start
ACKs for top commit:
jnewbery:
utACK faf48f20f1
amitiuttarwar:
utACK faf48f20f1, 👍 for consistency. also checked where we create / load other `.dat` files, looks good to me.
practicalswift:
cr ACK faf48f20f1
Tree-SHA512: 697a728ef2b9f203363ac00b03eaf23ddf80bee043ecd3719265a0d884e8cfe88cd39afe946c86ab849edd1c836f05ec51125f052bdc14fe184b84447567756f
GetLocalAddrForPeer() is only called in one place. The checks inside that
function make more sense to be carried out be the caller:
- fSuccessfullyConnected is already checked at the top of
SendMessages(), so must be true when we call GetLocalAddrForPeer()
- fListen can go into the conditional before GetLocalAddrForPeer() is
called.
Gossiping addresses to peers is the responsibility of net processing.
Change AdvertiseLocal() in net to just return an (optional) address
for net processing to advertise. Update function name to reflect
new responsibility.
Moves the logic to prevent running inactivity checks until
the peer has been connected for -peertimeout time into its
own function. This will be reused by net_processing later.
fe3e993968 [p2p] No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty. Add -fixedseeds arg. (Dhruv Mehta)
Pull request description:
Closes#19795
Before PR: If `peers.dat` is empty and `-dnsseed=0`, bitcoind will fallback on to fixed seeds but only after a 60 seconds delay.
After PR: There's no 60 second delay.
To reproduce:
`rm ~/.bitcoin/peers.dat && src/bitcoind -dnsseed=0` without and with patch code
Other changes in the PR:
- `-fixedseeds` command line argument added: `-dnsseed=0 -fixedseeds=0 -addnode=X` provides a trusted peer only setup. `-dnsseed=0 -fixedseeds=0` allows for a `addnode` RPC to add a trusted peer without falling back to hardcoded seeds.
ACKs for top commit:
LarryRuane:
re-ACK fe3e993968
laanwj:
re-ACK fe3e993968
Tree-SHA512: 79449bf4e83a315be6dbac9bdd226de89d2a3f7f76d9c5640a2cb3572866e6b0e8ed67e65674c9824054cf13119dc01c7e1a33848daac6b6c34dbc158b6dba8f
615ba0eb96 test: add Sock unit tests (Vasil Dimov)
7bd21ce1ef style: rename hSocket to sock (Vasil Dimov)
04ae846904 net: use Sock in InterruptibleRecv() and Socks5() (Vasil Dimov)
ba9d73268f net: add RAII socket and use it instead of bare SOCKET (Vasil Dimov)
dec9b5e850 net: move CloseSocket() from netbase to util/sock (Vasil Dimov)
aa17a44551 net: move MillisToTimeval() from netbase to util/time (Vasil Dimov)
Pull request description:
Introduce a class to manage the lifetime of a socket - when the object
that contains the socket goes out of scope, the underlying socket will
be closed.
In addition, the new `Sock` class has a `Send()`, `Recv()` and `Wait()`
methods that can be overridden by unit tests to mock the socket
operations.
The `Wait()` method also hides the
`#ifdef USE_POLL poll() #else select() #endif` technique from higher
level code.
ACKs for top commit:
laanwj:
Re-ACK 615ba0eb96
jonatack:
re-ACK 615ba0eb96
Tree-SHA512: 3003e6bc0259295ca0265ccdeb1522ee25b4abe66d32e6ceaa51b55e0a999df7ddee765f86ce558a788c1953ee2009bfa149b09d494593f7d799c0d7d930bee8
Use the `Sock` class instead of `SOCKET` for `InterruptibleRecv()` and
`Socks5()`.
This way the `Socks5()` function can be tested by giving it a mocked
instance of a socket.
Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
Introduce a class to manage the lifetime of a socket - when the object
that contains the socket goes out of scope, the underlying socket will
be closed.
In addition, the new `Sock` class has a `Send()`, `Recv()` and `Wait()`
methods that can be overridden by unit tests to mock the socket
operations.
The `Wait()` method also hides the
`#ifdef USE_POLL poll() #else select() #endif` technique from higher
level code.
Move `CloseSocket()` (and `NetworkErrorString()` which it uses) from
`netbase.{h,cpp}` to newly added `src/util/sock.{h,cpp}`.
This is necessary in order to use `CloseSocket()` from a newly
introduced Sock class (which will live in `src/util/sock.{h,cpp}`).
`sock.{h,cpp}` cannot depend on netbase because netbase will depend
on it.
bff7c66e67 Add documentation to contrib folder (Troy Giorshev)
381f77be85 Add Message Capture Test (Troy Giorshev)
e4f378a505 Add capture parser (Troy Giorshev)
4d1a582549 Call CaptureMessage at appropriate locations (Troy Giorshev)
f2a77ff97b Add CaptureMessage (Troy Giorshev)
dbf779d5de Clean PushMessage and ProcessMessages (Troy Giorshev)
Pull request description:
This PR introduces per-peer message capture into Bitcoin Core. 📓
## Purpose
The purpose and scope of this feature is intentionally limited. It answers a question anyone new to Bitcoin's P2P protocol has had: "Can I see what messages my node is sending and receiving?".
## Functionality
When a new debug-only command line argument `capturemessages` is set, any message that the node receives or sends is captured. The capture occurs in the MessageHandler thread. When receiving a message, it is captured as soon as the MessageHandler thread takes the message off of the vProcessMsg queue. When sending, the message is captured just before the message is pushed onto the vSendMsg queue.
The message capture is as minimal as possible to reduce the performance impact on the node. Messages are captured to a new `message_capture` folder in the datadir. Each node has their own subfolder named with their IP address and port. Inside, received and sent messages are captured into two binary files, msgs_recv.dat and msgs_sent.dat, like so:
```
message_capture/203.0.113.7:56072/msgs_recv.dat
message_capture/203.0.113.7:56072/msgs_sent.dat
```
Because the messages are raw binary dumps, included in this PR is a Python parsing tool to convert the binary files into human-readable JSON. This script has been placed on its own and out of the way in the new `contrib/message-capture` folder. Its usage is simple and easily discovered by the autogenerated `-h` option.
## Future Maintenance
I sympathize greatly with anyone who says "the best code is no code".
The future maintenance of this feature will be minimal. The logic to deserialize the payload of the p2p messages exists in our testing framework. As long as our testing framework works, so will this tool.
Additionally, I hope that the simplicity of this tool will mean that it gets used frequently, so that problems will be discovered and solved when they are small.
## FAQ
"Why not just use Wireshark"
Yes, Wireshark has the ability to filter and decode Bitcoin messages. However, the purpose of the message capture added in this PR is to assist with debugging, primarily for new developers looking to improve their knowledge of the Bitcoin Protocol. This drives the design in a different direction than Wireshark, in two different ways. First, this tool must be convenient and simple to use. Using an external tool, like Wireshark, requires setup and interpretation of the results. To a new user who doesn't necessarily know what to expect, this is unnecessary difficulty. This tool, on the other hand, "just works". Turn on the command line flag, run your node, run the script, read the JSON. Second, because this tool is being used for debugging, we want it to be as close to the true behavior of the node as possible. A lot can happen in the SocketHandler thread that would be missed by Wireshark.
Additionally, if we are to use Wireshark, we are at the mercy of whoever it maintaining the protocol in Wireshark, both as to it being accurate and recent. As can be seen by the **many** previous attempts to include Bitcoin in Wireshark (google "bitcoin dissector") this is easier said than done.
Lastly, I truly believe that this tool will be used significantly more by being included in the codebase. It's just that much more discoverable.
ACKs for top commit:
MarcoFalke:
re-ACK bff7c66e67 only some minor changes: 👚
jnewbery:
utACK bff7c66e67
theStack:
re-ACK bff7c66e67
Tree-SHA512: e59e3160422269221f70f98720b47842775781c247c064071d546c24fa7a35a0e5534e8baa4b4591a750d7eb16de6b4ecf54cbee6d193b261f4f104e28c15f47
2a39ccf133 Add include for std::bind. (sinetek)
Pull request description:
Hi, this patch adds in <functional> because the GUI code makes use of std::bind.
That's all.
ACKs for top commit:
jonasschnelli:
utACK 2a39ccf133
Tree-SHA512: fb5ac07d9cd5d006182b52857b289a9926362a2f1bfa4f7f1c78a088670e2ccf39ca28214781df82e8de3909fa3e69685fe1124a7e3ead758575839f5f2277a9
This commit adds the CaptureMessage function. This will later be called
when any message is sent or received. The capture directory is fixed,
in a new folder "message_capture" in the datadir. Peers will then have
their own subfolders, named with their IP address and port, replacing
colons with underscores to keep compatibility with Windows. Inside,
received and sent messages will be captured into two binary files,
msgs_recv.dat and msgs_sent.dat.
e.g.
message_capture/203.0.113.7_56072/msgs_recv.dat
message_capture/203.0.113.7_56072/msgs_sent.dat
The format has been designed as to result in a minimal performance
impact. A parsing script is added in a later commit.
Also clean up and better comment the function. InactivityChecks() uses a
mixture of (non-mockable) system time and mockable time. Make sure
that's well documented.
Despite being marked as const in CConnman before this commit, the
function did mutate the state of the passed in CNode, which is contained
in vNodes, which is a member of CConnman. To make the function truly
const in CConnman and all its data, instead make InactivityChecks() a
pure function, return whether the peer should be disconnected, and let
the calling function (SocketHandler()) update the CNode object. Also
make the CNode& argument const.
This brings PushMessage and ProcessMessages further in line with the
style guide by fixing their if statements.
LogMessage is later called, inside an if statement, inside both of these
methods.
b4dd2ef800 [test] Test the add_outbound_p2p_connection functionality (Amiti Uttarwar)
602e69e427 [test] P2PBlocksOnly - Test block-relay-only connections. (Amiti Uttarwar)
8bb6beacb1 [test/refactor] P2PBlocksOnly - Extract transaction violation test into helper. (Amiti Uttarwar)
99791e7560 [test/refactor] P2PBlocksOnly - simplify transaction creation using blocktool helper. (Amiti Uttarwar)
3997ab9154 [test] Add test framework support to create outbound connections. (Amiti Uttarwar)
5bc04e8837 [rpc/net] Introduce addconnection to test outbounds & blockrelay (Amiti Uttarwar)
Pull request description:
The existing functional test framework uses the `addnode` RPC to spin up manual connections between bitcoind nodes. This limits our ability to add integration tests for our networking code, which often executes different code paths for different connection types.
**This PR enables creating `outbound` & `block-relay-only` P2P connections in the functional tests.** This allows us to increase our p2p test coverage, since we can now verify expectations around these connection types.
This builds out the [prototype](https://github.com/bitcoin/bitcoin/issues/14210#issuecomment-527421978) proposed by ajtowns in #14210. 🙌🏽
An overview of this branch:
- introduces a new test-only RPC function `addconnection` which initiates opening an `outbound` or `block-relay-only` connection. (conceptually similar to `addnode` but for different connection types & restricted to regtest)
- adds `test_framework` support so a mininode can open an `outbound`/`block-relay-only` connection to a `P2PInterface`/`P2PConnection`.
- updates `p2p_blocksonly` tests to create a `block-relay-only` connection & verify expectations around transaction relay.
- introduces `p2p_add_connections` test that checks the behaviors of the newly introduced `add_outbound_p2p_connection` test framework function.
With these changes, there are many more behaviors that we can add integration tests for. The blocksonly updates is just one example.
Huge props to ajtowns for conceiving the approach & providing me feedback as I've built out this branch. Also thank you to jnewbery for lots of thoughtful input along the way.
ACKs for top commit:
troygiorshev:
reACK b4dd2ef800
jnewbery:
utACK b4dd2ef800
MarcoFalke:
Approach ACK b4dd2ef800🍢
Tree-SHA512: d1cba768c19c9c80e6a38b1c340cc86a90701b14772c4a0791c458f9097f6a4574b4a4acc7d13d6790c7b1f1f197e2c3d87996270f177402145f084ef8519a6b
fad1f0fd33 net: Remove unused cs_feeFilter (MarcoFalke)
Pull request description:
A `RecursiveMutex` is overkill for setting or reading a plain integer. Even a `Mutex` is overkill, when a plain `std::atomic` can be used.
This removes 11 lines of code. Also, it is cutting down on the number of locks put on the stack at the same time, which complicates review looking out for potential lock contention.
ACKs for top commit:
jnewbery:
utACK fad1f0fd33
practicalswift:
cr ACK fad1f0fd33: patch looks correct
Tree-SHA512: 647f9b954fbf52e138d3e710937eb9131b390fef0deae03fd6a162d5a18b9f194010800bbddc8f89208d91be2802dff11c3884d04b3dd233865abd12aa3cde06
faecb74562 Expose integral m_conn_type in CNodeStats, remove m_conn_type_string (Jon Atack)
Pull request description:
Currently, strings are stored for what are actually integral (strong) enum types. This is fine, because the strings are only used as-is for the debug log and RPC. However, it complicates using them in the GUI. User facing strings in the GUI should be translated and only string literals can be picked up for translation, not runtime `std::string`s.
Fix that by removing the `std::string` members and replace them by strong enum integral types.
ACKs for top commit:
jonatack:
Code review ACK faecb74562
theStack:
Code review ACK faecb74562🌲
Tree-SHA512: 24df2bd0645432060e393eb44b8abaf20fe296457d07a867b0e735c3e2e75af7b03fc6bfeca734ec33ab816a7c8e1f8591a5ec342f3afe3098a4e41f5c2cfebb
Add a new RPC endpoint to enable opening outbound connections from
the tests. The functional test framework currently uses the addnode RPC, which
has different behavior than general outbound peers. These changes enable
creating both full-relay and block-relay-only connections. The new RPC
endpoint calls through to a newly introduced AddConnection method on
CConnman that ensures we stay within the allocated max.
31b136e580 Don't declare de facto const reference variables as non-const (practicalswift)
1c65c075ee Don't declare de facto const member functions as non-const (practicalswift)
Pull request description:
_Meta: This is the second and final part of the `const` refactoring series (part one: #20581). **I promise: no more refactoring PRs from me in a while! :)** I'll now go back to focusing on fuzzing/hardening!_
Changes in this PR:
* Don't declare de facto const member functions as non-const
* Don't declare de facto const reference variables as non-const
Awards for finding candidates for the above changes go to:
* `clang-tidy`'s [`readability-make-member-function-const`](https://clang.llvm.org/extra/clang-tidy/checks/readability-make-member-function-const.html) check ([list of `clang-tidy` checks](https://clang.llvm.org/extra/clang-tidy/checks/list.html))
* `cppcheck`'s `constVariable` check ([list of `cppcheck` checks](https://sourceforge.net/p/cppcheck/wiki/ListOfChecks/))
See #18920 for instructions on how to analyse Bitcoin Core using Clang Static Analysis, `clang-tidy` and `cppcheck`.
ACKs for top commit:
ajtowns:
ACK 31b136e580
jonatack:
ACK 31b136e580
theStack:
ACK 31b136e580❄️
Tree-SHA512: f58f8f00744219426874379e9f3e9331132b9b48e954d24f3a85cbb858fdcc98009ed42ef7e7b4619ae8af9fc240a6d8bfc1c438db2e97b0ecd722a80dcfeffe
378aedc452 [net] Add cs_vSend lock annotations (John Newbery)
673254515a [net] Move RecordBytesSent() call out of cs_vSend lock (John Newbery)
Pull request description:
RecordBytesSent() does not require cs_vSend to be locked, so reduce the scope of cs_vSend.
Also correctly annotate the CNode data members that are guarded by cs_vSend.
This is a simpler alternative to #19673.
ACKs for top commit:
jnewbery:
ok, reverting to commit 378aedc which has two ACKs already. Any style issues can be fixed up in future PRs.
troygiorshev:
ACK 378aedc452
theStack:
re-ACK 378aedc452
MarcoFalke:
review ACK 378aedc452🔌
Tree-SHA512: e9cd6c472b7e1479120c1bf2d1c640cf6d18c7d589a5f9b7dfc4875e5790adaab403a7a1b945a47e79e7249a614b8583270e4549f89b22e8a9edb2e4818b0d07
86c495223f net: add CNode::IsInboundOnion() public getter and unit tests (Jon Atack)
6609eb8cb5 net: assert CNode::m_inbound_onion is inbound in ctor (Jon Atack)
993d1ecd19 test, fuzz: fix constructing CNode with invalid inbound_onion (Jon Atack)
Pull request description:
The goal of this PR is to be able to depend on `m_inbound_onion` in AttemptToEvictConnection in #20197:
- asserts `CNode::m_inbound_onion` is inbound in the CNode ctor to have a validity check at the class boundary
- fixes a unit test and a fuzz utility that were passing invalid inbound onion values to the CNode ctor
- drops an unneeded check in `CNode::ConnectedThroughNetwork()` for its inbound status
- adds a public getter `IsInboundOnion()` that also allows unit testing it
- adds unit test coverage
ACKs for top commit:
sipa:
utACK 86c495223f
LarryRuane:
ACK 86c495223f
vasild:
ACK 86c495223f
MarcoFalke:
review ACK 86c495223f🐍
Tree-SHA512: 21109105bc4e5e03076fadd489204be00eac710c9de0127708ca2d0a10a048ff81f640f589a7429967ac3eb51d35fe24bb2b12e53e7aa3efbc47aaff6396d204
010eed3ce0 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas)
Pull request description:
Closes#5150.
This was mostly copied from #5285 by sulks, who has since quit GitHub.
The issue has remained open for 6 years, but the extra explanation still seems useful.
ACKs for top commit:
laanwj:
re-ACK 010eed3ce0
Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
ea36a453e3 [net] Make p2p recv buffer timeout 20 minutes for all peers (John Newbery)
Pull request description:
The timeout interval for the send and recv buffers was changed from 90
minutes to 20 minutes in commit f1920e86 in 2013, except for peers that
did not support the pong message (where the recv buffer timeout remained
at 90 minutes). A few observations:
- for peers that support BIP 31 (pong messages), this recv buffer
timeout is almost redundant with the ping timeout. We send a ping
message every two minutes, and set a timeout of twenty minutes to
receive the pong response. If the recv buffer was really timing out,
then the pong response would also time out.
- BIP 31 is supported by all nodes of p2p version 60000 and higher, and
has been in widespread use since 2013. I'd be very surprised if there
are many nodes on the network that don't support pong messages.
- The recv buffer timeout is not specified in any p2p BIP. We're free to
set it at any value we want.
- A peer that doesn't support BIP 31 and hasn't sent any message to us
at all in 90 minutes is unlikely to be useful for us, and is more likely
to be evicted AttemptToEvictConnection() since it'll have the worst
possible ping time and isn't providing blocks/transactions.
Therefore, we remove this check, and set the recv buffer timeout to 20
minutes for all peers. This removes the final p2p version dependent
logic from the net layer, so all p2p version data can move into the
net_processing layer.
Alternative approaches:
- Set the recv buffer timeout to 90 minutes for all peers. This almost
wouldn't be a behaviour change at all (pre-BIP 31 peers would still
have the same recv buffer timeout, and we can't ever reach a recv buffer
timeout higher than 21 minutes for post-BIP31 peers, because the pong
timeout would be hit first).
- Stop supporting peers that don't support BIP 31. BIP 31 has been in
use since 2012, and implementing it is trivial.
ACKs for top commit:
MarcoFalke:
review ACK ea36a453e3
promag:
Code review ACK ea36a453e3.
practicalswift:
cr ACK ea36a453e3: patch looks correct
ajtowns:
ACK ea36a453e3
sipa:
utACK ea36a453e3
jonatack:
Code review ACK ea36a453e3
Tree-SHA512: df290bb32d2b5d9e59a0125bb215baa92787f9d01542a7437245f1c478c7f9b9831e5f170d3cd0db2811e1b11b857b3e8b2e03376476b8302148e480d81aab19
fa86217e97 doc: Move add relay comment in net to correct place (MarcoFalke)
Pull request description:
The comment was previously attached to `m_addr_known`, but now it is attached to `id`, which is wrong.
Fix that by moving the comment to `RelayAddrsWithConn`.
ACKs for top commit:
practicalswift:
cr ACK fa86217e97: patch looks correct
jnewbery:
ACK fa86217e97
theStack:
Code review ACK fa86217e97🌳
Tree-SHA512: ec3d5f1996aded38947d2a5fd0bb63539e88f83964cd3254984002edfd51abb4dde813c7c81619a8a3a5c55b7e9ae83c8c5be8ad6c84b4593ed3bbf463fe8979
The timeout interval for the send and recv buffers was changed from 90
minutes to 20 minutes in commit f1920e86 in 2013, except for peers that
did not support the pong message (where the recv buffer timeout remained
at 90 minutes). A few observations:
- for peers that support BIP 31 (pong messages), this recv buffer
timeout is almost redundant with the ping timeout. We send a ping
message every two minutes, and set a timeout of twenty minutes to
receive the pong response. If the recv buffer was really timing out,
then the pong response would also time out.
- BIP 31 is supported by all nodes of p2p version 60000 and higher, and
has been in widespread use since 2013. I'd be very surprised if there
are many nodes on the network that don't support pong messages.
- The recv buffer timeout is not specified in any p2p BIP. We're free to
set it at any value we want.
- A peer that doesn't support BIP 31 and hasn't sent any message to us
at all in 90 minutes is unlikely to be useful for us, and is more likely
to be evicted AttemptToEvictConnection() since it'll have the worst
possible ping time and isn't providing blocks/transactions.
Therefore, we remove this check, and sent the recv buffer timeout to 20
minutes for all peers. This removes the final p2p version dependent
logic from the net layer, so all p2p version data can move into the
net_processing layer.
Alternative approaches:
- Set the recv buffer timeout to 90 minutes for all peers. This almost
wouldn't be a behaviour change at all (pre-BIP 31 peers would still
have the same recv buffer timeout, and we can't ever reach a recv buffer
timeout higher than 21 minutes for post-BIP31 peers, because the pong
timeout would be hit first).
- Stop supporting peers that don't support BIP 31. BIP 31 has been in
use since 2012, and implementing it is trivial.
It's not actually possible to change this value, so remove the
indirection of it being a conn option.
DEFAULT_MAX_UPLOAD_TIMEFRAME is a compile time constant.
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.
343dc4760f test: add test for high-bandwidth mode states in getpeerinfo (Sebastian Falbesoner)
dab6583307 doc: release note for new getpeerinfo fields "bip152_hb_{from,to}" (Sebastian Falbesoner)
a7ed00f8bb rpc: expose high-bandwidth mode states via getpeerinfo (Sebastian Falbesoner)
30bc8fab68 net: save high-bandwidth mode states in CNodeStats (Sebastian Falbesoner)
Pull request description:
Fixes#19676, "_For every peer expose through getpeerinfo RPC whether or not we selected them as HB peers, and whether or not they selected us as HB peers._" See [BIP152](https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki), in particular the [protocol flow diagram](https://github.com/bitcoin/bips/raw/master/bip-0152/protocol-flow.png). The newly introduced states are changed on the following places in the code:
* on reception of a `SENDCMPCT` message with valid version, the field `m_highbandwidth_from` is changed depending on the first integer parameter in the message (1=high bandwidth, 0=low bandwidth), i.e. it just mirrors the field `CNodeState.fPreferHeaderAndIDs`.
* after adding a `SENDCMPCT` message to the send queue, the field `m_highbandwidth_to` is changed depending on how the first integer parameter is set (same as above)
Note that after receiving `VERACK`, the node also sends `SENDCMPCT`, but that is only to announce the preferred version and never selects high-bandwidth mode, hence there is no need to change the state variables there, which are initialized to `false` anyways.
ACKs for top commit:
naumenkogs:
reACK 343dc4760f
jonatack:
re-ACK 343dc4760f per `git range-diff 7ea6499 4df1d12 343dc47`
Tree-SHA512: f4999e6a935266812c2259a9b5dc459710037d3c9e938006d282557cc225e56128f72965faffb207fc60c6531fab1206db976dd8729a69e8ca29d4835317b99f
fa5ed3b4ca net: Use Span in ReceiveMsgBytes (MarcoFalke)
Pull request description:
Pass a data pointer and a size as span in `ReceiveMsgBytes` to get the benefits of a span
ACKs for top commit:
jonatack:
ACK fa5ed3b4ca code review, rebased to current master 12a1c3ad1a, debug build, unit tests, ran bitcoind/-netinfo/getpeerinfo
theStack:
ACK fa5ed3b4ca
Tree-SHA512: 89bf111323148d6e6e50185ad20ab39f73ab3a58a27e46319e3a08bcf5dcf9d6aa84faff0fd6afb90cb892ac2f557a237c144560986063bc736a69ace353ab9d
Outbound peer logic prevents connecting to addresses that we're already
connected to, so prevent inadvertent eviction of current peers via
test-before-evict by checking this condition and marking current peer's
addresses as Good().
Co-authored-by: John Newbery <john@johnnewbery.com>
a490d074b3 doc: Add anchors.dat to files.md (Hennadii Stepanov)
0a85e5a7bc p2p: Try to connect to anchors once (Hennadii Stepanov)
5543c7ab28 p2p: Fix off-by-one error in fetching address loop (Hennadii Stepanov)
4170b46544 p2p: Integrate DumpAnchors() and ReadAnchors() into CConnman (Hennadii Stepanov)
bad16aff49 p2p: Add CConnman::GetCurrentBlockRelayOnlyConns() (Hennadii Stepanov)
c29272a157 p2p: Add ReadAnchors() (Hennadii Stepanov)
567008d2a0 p2p: Add DumpAnchors() (Hennadii Stepanov)
Pull request description:
This is an implementation of #17326:
- all (currently 2) outbound block-relay-only connections (#15759) are dumped to `anchors.dat` file
- on restart a node tries to connect to the addresses from `anchors.dat`
This PR prevents a type of eclipse attack when an attacker exploits a victim node restart to force it to connect to new, probably adversarial, peers.
ACKs for top commit:
jnewbery:
code review ACK a490d074b3
laanwj:
Code review ACK a490d074b3
Tree-SHA512: 0f5098a3882f2814be1aa21de308cd09e6654f4e7054b79f3cfeaf26bc02b814ca271497ed00018d199ee596a8cb9b126acee8b666a29e225b08eb2a49b02ddd
96571b3d4c doc: Update onion service target port numbers in tor.md (Hennadii Stepanov)
bb145c9050 net: Extend -bind config option with optional network type (Hennadii Stepanov)
92bd3c1da4 net, refactor: Move AddLocal call one level up (Hennadii Stepanov)
57f17e57c8 net: Pass onion service target to Tor controller (Hennadii Stepanov)
e3f07851f0 refactor: Rename TorController::target to m_tor_control_center (Hennadii Stepanov)
fdd3ae4d26 net, refactor: Refactor CBaseChainParams::RPCPort function (Hennadii Stepanov)
a5266d4546 net: Add alternative port for onion service (Hennadii Stepanov)
b3273cf403 net: Use network byte order for in_addr.s_addr (Hennadii Stepanov)
Pull request description:
This PR adds ability to label incoming Tor connections as different from normal localhost connections.
Closes#8973.
Closes#16693.
Default onion service target ports are:
- 8334 on mainnnet
- 18334 on testnet
- 38334 on signet
- 18445 on regtest
To set the onion service target socket manually the extended `-bind` config option could be used:
```
$ src/bitcoind -help | grep -A 6 -e '-bind'
-bind=<addr>[:<port>][=onion]
Bind to given address and always listen on it (default: 0.0.0.0). Use
[host]:port notation for IPv6. Append =onion to tag any incoming
connections to that address and port as incoming Tor connections
(default: 127.0.0.1:8334=onion, testnet: 127.0.0.1:18334=onion,
signet: 127.0.0.1:38334=onion, regtest: 127.0.0.1:18445=onion)
```
Since [pr19991.02 update](https://github.com/bitcoin/bitcoin/pull/19991#issuecomment-698882284) this PR is an alternative to #19043.
ACKs for top commit:
Sjors:
re-utACK 96571b3d4c
vasild:
ACK 96571b3d4
laanwj:
Re-ACK 96571b3d4c
Tree-SHA512: cb0eade80f4b3395f405f775e1b89c086a1f09d5a4464df6cb4faf808d9c2245474e1720b2b538f203f6c1996507f69b09f5a6e35ea42633c10e22bd733d4438
2ea62cae48 Improve docs about feeler connections (Gleb Naumenko)
Pull request description:
"feeler" and "test-before-evict" are two different strategies suggest in [Eclipse Attacks on Bitcoin’s Peer-to-Peer Network](https://www.usenix.org/system/files/conference/usenixsecurity15/sec15-paper-heilman.pdf). In our codebase, we use `ConnType::FEELER` to implement both.
It is confusing, up to the point that our documentation was just incorrect.
This PR:
- ~clarifies this aspect by renaming "ConnType::FEELER" to "ConnType::PROBE", meaning that this connections only probes that the node is operational, and then disconnects.~
- fixes the documentation
ACKs for top commit:
amitiuttarwar:
ACK 2ea62cae48. thank you!
practicalswift:
ACK 2ea62cae48
Tree-SHA512: c9c03c09eefeacec28ea199cc3f697b0a98723f2f849f7a8115edc43791f8165e296e0e25a82f0b5a4a781a7de38c8954b48bf74c714eba02cdc21f7460673e5
deb52711a1 Remove header checks out of net_processing (Troy Giorshev)
52d4ae46ab Give V1TransportDeserializer CChainParams& member (Troy Giorshev)
5bceef6b12 Change CMessageHeader Constructor (Troy Giorshev)
1ca20c1af8 Add doxygen comment for ReceiveMsgBytes (Troy Giorshev)
890b1d7c2b Move checksum check from net_processing to net (Troy Giorshev)
2716647ebf Give V1TransportDeserializer an m_node_id member (Troy Giorshev)
Pull request description:
Inspired by #15206 and #15197, this PR moves all message header verification from the message processing layer and into the network/transport layer.
In the previous PRs there is a change in behavior, where we would disconnect from peers upon a single failed checksum check. In various discussions there was concern over whether this was the right choice, and some expressed a desire to see how this would look if it was made to be a pure refactor.
For more context, see https://bitcoincore.reviews/15206.html#l-81.
This PR improves the separation between the p2p layers, helping improvements like [BIP324](https://github.com/bitcoin/bitcoin/pull/18242) and #18989.
ACKs for top commit:
ryanofsky:
Code review ACK deb52711a1 just rebase due to conflict on adjacent line
jnewbery:
Code review ACK deb52711a1.
Tree-SHA512: 1a3b7ae883b020cfee1bef968813e04df651ffdad9dd961a826bd80654f2c98676ce7f4721038a1b78d8790e4cebe8060419e3d8affc97ce2b9b4e4b72e6fa9f
This moves header size and netmagic checking out of net_processing and
into net. This check now runs in ReadHeader, so that net can exit early
out of receiving bytes from the peer. IsValid is now slimmed down, so
it no longer needs a MessageStartChars& parameter.
Additionally this removes the rest of the m_valid_* members from
CNetMessage.
This adds a CChainParams& member to V1TransportDeserializer member, and
use it in place of many Params() calls. In addition to reducing the
number of calls to a global, this removes a parameter from GetMessage
(and will later allow us to remove one from CMessageHeader::IsValid())
This commit removes the single-parameter contructor of CMessageHeader
and replaces it with a default constructor.
The single parameter contructor isn't used anywhere except for tests.
There is no reason to initialize a CMessageHeader with a particular
messagestart. This messagestart should always be replaced when
deserializing an actual message header so that we can run checks on it.
The default constructor initializes it to zero, just like the command
and checksum.
This also removes a parameter of a V1TransportDeserializer constructor,
as it was only used for this purpose.
This removes the m_valid_checksum member from CNetMessage. Instead,
GetMessage() returns an Optional.
Additionally, GetMessage() has been given an out parameter to be used to
hold error information. For now it is specifically a uint32_t used to
hold the raw size of the corrupt message.
The checksum check is now done in GetMessage.
This is intended to only be used for logging.
This will allow log messages in the following commits to keep recording
the peer's ID, even when logging is moved into V1TransportDeserializer.
In addition to adding more specificity to the log statement about the type of
connection, this change also consolidates two statements into one. Previously,
the second one should have never been hit, since block-relay connections would
match the "!IsInboundConn()" condition and return early.
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
0d04784af1 Refactor the functional test (Gleb Naumenko)
83ad65f31b Address nits in ADDR caching (Gleb Naumenko)
81b00f8780 Add indexing ADDR cache by local socket addr (Gleb Naumenko)
42ec558542 Justify the choice of ADDR cache lifetime (Gleb Naumenko)
Pull request description:
This is a follow-up on #18991 which does 3 things:
- improves privacy of a node listening to multiple addresses via adding cache index by local socket addr (suggested [here](https://github.com/bitcoin/bitcoin/pull/18991#issuecomment-668219345))
- documents on the choice of 24h cache lifetime
- addresses nits from #18991
ACKs for top commit:
jnewbery:
utACK 0d04784af1
vasild:
ACK 0d04784
jonatack:
Code review ACK 0d04784
Tree-SHA512: bb65a34dd1ce2811186d3e4469bc33e8399cebaaa494ce13041c7cff23275870e4176a719f7a72f8d779c49f8b2344bf4fa1aeb3ea4e2626d5ae76514f00a750
752e6ad533 Protect localhost and block-relay-only peers from eviction (Suhas Daftuar)
Pull request description:
Onion peers are disadvantaged under our eviction criteria, so prevent eventual
eviction of them in the presence of contention for inbound slots by reserving
some slots for localhost peers (sorted by longest uptime).
Block-relay-only connections exist as a protection against eclipse attacks, by
creating a path for block propagation that may be unknown to adversaries.
Protect against inbound peer connection slot attacks from disconnecting such
peers by attempting to protect up to 8 peers that are not relaying transactions
but have provided us with blocks.
Thanks to gmaxwell for suggesting these strategies.
ACKs for top commit:
laanwj:
Code review ACK 752e6ad533
Tree-SHA512: dbf089c77c1f747aa1dbbbc2e9c2799c628028b0918d0c336d8d0e5338acedd573b530eb3b689c7f603a17221e557268a9f5c3f585f204bfb12e5d2e76de39a3
We previously identified if we relay addresses to the connection by checking
for the existence of the m_addr_known data structure. With this commit, we
answer this question based on the connection type.
IsAddrRelayPeer() checked for the existence of the m_addr_known
Onion peers are disadvantaged under our eviction criteria, so prevent eventual
eviction of them in the presence of contention for inbound slots by reserving
some slots for localhost peers (sorted by longest uptime).
Block-relay-only connections exist as a protection against eclipse attacks, by
creating a path for block propagation that may be unknown to adversaries.
Protect against inbound peer connection slot attacks from disconnecting such
peers by attempting to protect up to 8 peers that are not relaying transactions
but appear to be full-nodes, sorted by recency of last delivered block.
Thanks to gmaxwell for suggesting these strategies.
37a480e0cd [net] Add addpeeraddress RPC method (John Newbery)
ae8051bbd8 [test] Test that getnodeaddresses() can return all known addresses (John Newbery)
f26502e9fc [addrman] Specify max addresses and pct when calling GetAddresses() (John Newbery)
Pull request description:
Currently addrman only allows a maximum of 1000 records or 23% of all records to be returned in a call to `GetAddr()`. Relax this limit and have the client specify the max records they want. For p2p, behaviour is unchanged (but the rate limiting is set inside net_processing, where it belongs). For RPC, `getnodeaddresses` can now return the complete addrman, which is helpful for testing and monitoring.
Also add a test-only RPC `addpeeraddress`, which adds an IP address:port to addrman. This is helpful for testing (eg #18991).
ACKs for top commit:
naumenkogs:
utACK 37a480e0cd
laanwj:
Code review and lightly manually tested ACK 37a480e0cd
Tree-SHA512: f86dcd410aaebaf6e9ca18ce6f23556e5e4649c1325577213d873aa09967298e65ab2dc19a72670641ae92211a923afda1fe124a82e9d2c1cad73d478ef27fdc
CAddrMan.GetAddr() would previously limit the number and percentage of
addresses returned (to ADDRMAN_GETADDR_MAX (1000) and
ADDRMAN_GETADDR_MAX_PCT (23) respectively). Instead, make it the callers
responsibility to specify the maximum addresses and percentage they want
returned.
For net_processing, the maximums are MAX_ADDR_TO_SEND (1000) and
MAX_PCT_ADDR_TO_SEND (23). For rpc/net, the maximum is specified by the
client.
01e283068b [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar)
bc5d65b3ca [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar)
2f2e13b6c2 [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar)
7f7b83deb2 [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar)
35839e963b [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar)
4972c21b67 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar)
60156f5fc4 [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar)
7b322df629 [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar)
14923422b0 [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar)
49efac5cae [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar)
d3698b5ee3 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar)
46578c03e9 [doc] Describe different connection types (Amiti Uttarwar)
442abae2ba [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar)
af59feb052 [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar)
e1bc29812d [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar)
0e52a659a2 [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar)
1521c47438 [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar)
26304b4100 [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar)
3f1b7140e9 scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar)
Pull request description:
**This is part 1 of #19315, which enables the ability to test `outbound` and `block-relay-only` connections from the functional tests.** Please see that PR for more information of overall functionality.
**This PR simplifies how we manage different connection types.** It introduces an enum with the various types of connections so we can explicitly define the connection type. The existing system relies on a series of independent flags, then has asserts scattered around to ensure that conflicting flags are not enabled at the same time. I find this approach to be both brittle and confusing. While making these changes, I found a small bug due to the silent assumptions.
This PR also proposes a rename from `OneShot` to `AddrFetch`. I find the name `OneShot` to be very confusing, especially when we also have `onetry` manual connections. Everyone I've talked to offline has agreed that the name is confusing, so I propose a potential alternative. I think this is a good opportunity for a rename since I'm creating an enum to explicitly define the connection types.
(some context for the unfamiliar: `oneshot` or `addrfetch` connections are short-lived connections created on startup. They connect to the seed peers, send a `getaddr` to solicit addresses, then close the connection.)
Overview of this PR:
* rename `oneshot` to `addrfetch`
* introduce `ConnectionType` enum
* one by one, add different connection types to the enum
* expose the `conn_type` on CNode, and use this to reduce reliance on flags (& asserts)
* fix the bug in counting different type of connections
* some additional cleanup to simplify logic and make expectations explicit/inclusive rather than implicit/exclusive.
ACKs for top commit:
jnewbery:
utACK 01e283068b
laanwj:
Code review ACK 01e283068b, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall
vasild:
ACK 01e283068
sdaftuar:
ACK 01e283068b.
fanquake:
ACK 01e283068b - I don't have as much experience with the networking code but these changes look fairly straight forward, the new code seems more robust/understandable and the additional documentation is great. I'm glad that a followup branch is already underway. There might be some more review comments here later today, so keep an eye on the discussion, however I'm going to merge this now.
jb55:
wow this code was messy before... ACK 01e283068b
Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15
Extract logic that check multiple connection types into interface functions &
structure as switch statements. This makes it very clear what touch points are
for accessing `m_conn_type` & using the switch statements enables the compiler
to warn if a new connection type is introduced but not handled for these cases.
Make the connection counts explicit and extract into interface functions around
m_conn_type. Using explicit counting and switch statements where possible
should help prevent counting bugs in the future.
The desired logic is for us to only open feeler connections after we have hit
the max count for outbound full relay connections. A short lived AddrFetch
connection (previously called oneshot) could cause ThreadOpenConnections to
miscount and mistakenly open a feeler instead of full relay.
77c507358b Make Hash[160] consume range-like objects (Pieter Wuille)
02c4cc5c5d Make CHash256/CHash160 output to Span (Pieter Wuille)
0ef97b1b10 Make MurmurHash3 consume Spans (Pieter Wuille)
e549bf8a9a Make CHash256 and CHash160 consume Spans (Pieter Wuille)
2a2182c387 Make script/standard's BaseHash Span-convertible (Pieter Wuille)
e63dcc3a67 Add MakeUCharSpan, to help constructing Span<[const] unsigned char> (Pieter Wuille)
567825049f Make uint256 Span-convertible by adding ::data() (Pieter Wuille)
131a2f0337 scripted-diff: rename base_blob::data to m_data (Pieter Wuille)
Pull request description:
This makes use of the implicit constructions and conversions to Span introduced in #18468 to simplify the hash.h interface:
* All functions that take a pointer and a length are changed to take a Span instead.
* The Hash() and Hash160() functions are changed to take in "range" objects instead of begin/end iterators.
ACKs for top commit:
laanwj:
re-ACK 77c507358b
jonatack:
Code review re-ACK 77c5073 per `git range-diff 14ceddd 49fc016 77c5073`
Tree-SHA512: 9ec929891b1ddcf30eb14b946ee1bf142eca1442b9de0067ad6a3c181e0c7ea0c99c0e291e7f6e7a18bd7bdf78fe94ee3d5de66e167401674caf91e026269771
2aac093a3d test: Add test coverage for -networkactive option (Hennadii Stepanov)
3c58129b12 net: Log network activity status change unconditionally (Hennadii Stepanov)
62fe6aa87e net: Add -networkactive option (Hennadii Stepanov)
Pull request description:
Some Bitcoin Core activity is completely local (offline), e.g., reindexing.
The `setnetworkactive` RPC command is already present. This PR adds the corresponding command-line argument / config option, and allows to start the client with disabled p2p network by providing `-networkactive=0` or `-nonetworkactive`.
This was done while reviewing #16981.
ACKs for top commit:
MarcoFalke:
re-ACK 2aac093a3d🏠
LarryRuane:
ACK 2aac093a3d
Tree-SHA512: 446d791b46d7b556d7694df7b1f88cd4fbc09301fe4eaf036b45cb8166ed806156353cc03788a07b633d5887d5eee30a7c02a2d4307141c8ccc75e0a88145636
The `setnetworkactive' RPC command is already present.
This new option allows to start the client with disabled p2p network
activity for testing or reindexing.
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.
57b0c0a93a Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater (Ben Woosley)
Pull request description:
We do not connect to peers older than 31800
ACKs for top commit:
sipa:
Code reivew ACK 57b0c0a93a
jnewbery:
Code review ACK 57b0c0a93a
vasild:
ACK 57b0c0a9
Tree-SHA512: e1ca7c9203cbad83ab7c7a2312777ad07ed6a16119169b256648b8a8738c260a5168acdd4fb33f6e4b17f51ec7e033e110b76bde55b4e3b2d444dc02c01bc2b1
fa3365430c net: Use mockable time for ping/pong, add tests (MarcoFalke)
faab4aaf2f util: Add count_microseconds helper (MarcoFalke)
Pull request description:
Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added.
Mockable time is also type-safe, since it uses `std::chrono`
ACKs for top commit:
jonatack:
Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid
troygiorshev:
ACK fa3365430c
Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
fa0540cd46 net: Extract download permission from noban (MarcoFalke)
Pull request description:
It should be possible to grant nodes in a local network (e.g. home, university, enterprise, ...) permission to download blocks even after the maxuploadtarget is hit.
Currently this is only possible by setting the `noban` permission, which has some adverse effects, especially if the peers can't be fully trusted.
Fix this by extracting a `download` permission from `noban`.
ACKs for top commit:
jonatack:
ACK fa0540c
Sjors:
re-utACK fa0540cd46
Tree-SHA512: 255566baa43ae925d93f5d0a3aa66b475a556d1590f662a88278a4872f16a1a05739a6119ae48a293011868042e05cb264cffe5822a50fb80db7333bf44376d9
1cabbddbca refactor: Use uint16_t instead of unsigned short (Aaron Hook)
Pull request description:
I wanted to see if the `up for grabs` label works and looked at PR #17822 originally opend by ahook I saw it had many acks for example by jonatack and practicalswift but needed rebasing.
So I checked out the remote branch rebased it resolved three conflicts and continued the rebase.
Hope everything is as expected (:
ACKs for top commit:
sipsorcery:
ACK 1cabbddbca.
practicalswift:
ACK 1cabbddbca -- patch looks correct :)
laanwj:
ACK 1cabbddbca
hebasto:
ACK 1cabbddbca, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 0e6bf64f274aae5dacb188358b4d5f65ccb207d4f70922f039bc4ed7934709418ddad19f8bfb7462517427837c3d2bb3f86ef284bb40e87119aad2a1e148d9d6
e8a2822119 [net] Don't try to take cs_inventory before deleting CNode (John Newbery)
3556227ddd [net] Make cs_inventory a non-recursive mutex (John Newbery)
344e831de5 [net processing] Remove PushBlockInventory and PushBlockHash (John Newbery)
Pull request description:
- Remove PushBlockInventory() and PushBlockHash(). These are one-line functions that can easy be inlined into the calling code. Doing so also allows us to eliminate the one place that cs_inventory is recursively locked.
- Make cs_inventory a nonrecursive mutex
- Remove a redundant TRY_LOCK of cs_inventory when deleting CNode.
ACKs for top commit:
sipa:
utACK e8a2822119
MarcoFalke:
ACK e8a2822119🍬
hebasto:
re-ACK e8a2822119
Tree-SHA512: dbc721d102cdef7b5827a8f2549daf8b54f543050266999a7ea56c9f36618565b71e31ce0beb1209ba2db43d15388be173355a03fb6db8ad24e2475b145050bd
This patch improves performance and resource usage around IP
addresses that are banned for misbehavior. They're already not
actually banned, as connections from them are still allowed,
but they are preferred for eviction if the inbound connection
slots are full.
Stop treating these like manually banned IP ranges, and instead
just keep them in a rolling Bloom filter of misbehaving nodes,
which isn't persisted to disk or exposed through the ban
framework. The effect remains the same: preferred for eviction,
avoided for outgoing connections, and not relayed to other peers.
Also change the name of this mechanism to better reflect reality;
they're not banned, just discouraged.
Contains release notes and several interface improvements by
John Newbery.
The TRY_LOCK(cs_inventory) in DisconnectNodes() is taken after the CNode
object has been removed from vNodes and when the CNode's nRefCount is
zero.
The only other places that cs_inventory can be taken are:
- In ProcessMessages() or SendMessages(), when the CNode's nRefCount
must be >0 (see ThreadMessageHandler(), where the refcount is
incremented before calling ProcessMessages() and SendMessages()).
- In a ForEachNode() lambda in PeerLogicValidation::UpdatedBlockTip().
ForEachNode() locks cs_vNodes and calls the function on the CNode
objects in vNodes.
Therefore, cs_inventory is never locked by another thread when the
TRY_LOCK(cs_inventory) is reached in DisconnectNodes(). Since the
only purpose of this TRY_LOCK is to ensure that the lock is not
taken by another thread, this always succeeds. Remove the check.
51e9393c1f refactor: s/command/msg_type/ in CNetMsgMaker and CSerializedNetMsg (Sebastian Falbesoner)
Pull request description:
Follow-up PR for #18533 -- another small step towards getting rid of the confusing "command" terminology. Also see PR #18610 which tackled the functional tests.
ACKs for top commit:
MarcoFalke:
ACK 51e9393c1f
Tree-SHA512: bb6f05a7be6823d5c4eab1d05b31fee944e700946827ad9425d59a3957fd879776c88c606319cbe9832d9451b275baedf913b71429ea3e01e4e82bf2d419e819
96954d1794 DNS seeds: don't query DNS while network is inactive (Anthony Towns)
fa5894f7f5 DNS seeds: wait for 5m instead of 11s if 1000+ peers are known (Anthony Towns)
Pull request description:
Changes the logic for querying DNS seeds: after this PR, if there's less than 1000 entries in addrman, it will still usually query DNS seeds after 11s (unless the first few peers tried mostly succeed), but if there's more than 1000 entries it won't try DNS seeds until 5 minutes have passed without getting multiple outbound peers. (If there's 0 entries in addrman, it will still immediately query the DNS seeds). Additionally, delays querying DNS seeds while the p2p network is not active.
Fixes#15434
ACKs for top commit:
fanquake:
ACK 96954d1794 - Ran some tests of different scenarios. More documentation is being added in #19084.
ariard:
Tested ACK 96954d1, on Debian 9.1. Both MANY_PEERS/FEW_PEERS cases work.
Sjors:
tACK 96954d1 (rebased on master) on macOS 10.15.4. It found it useful to run with `-debug=addrman` and change `DNSSEEDS_DELAY_MANY_PEERS` to something lower to test the behaviour, as well as renaming `peers.dat` to test the peer threshold.
naumenkogs:
utACK 96954d1794
Tree-SHA512: 73693db3da73bf8e76c3df9e9c82f0a7fb08049187356eac2575c4ffa455f76548dd1c86a11fc6beea8a3baf0ba020e047bebe927883c731383ec72442356005
5478d6c099 logging: thread safety annotations (Anthony Towns)
e685ca1992 util/system.cpp: add thread safety annotations for dir_locks (Anthony Towns)
a788789948 test/checkqueue_tests: thread safety annotations (Anthony Towns)
479c5846f7 rpc/blockchain.cpp: thread safety annotations for latestblock (Anthony Towns)
8b5af3d4c1 net: fMsgProcWake use LOCK instead of lock_guard (Anthony Towns)
de7c5f41ab wallet/wallet.h: Remove mutexScanning which was only protecting a single atomic bool (Anthony Towns)
c3cf2f5501 rpc/blockchain.cpp: Remove g_utxosetscan mutex that is only protecting a single atomic variable (Anthony Towns)
Pull request description:
In a few cases we need to use `std::mutex` rather than the sync.h primitives. But `std::lock_guard<std::mutex>` doesn't include the clang thread safety annotations unless you also use clang's C library, which means you can't indicate when variables should be guarded by `std::mutex` mutexes.
This adds an annotated version of `std::lock_guard<std::mutex>` to threadsafety.h to fix that, and modifies places where `std::mutex` is used to take advantage of the annotations.
It's based on top of #16112, and turns the thread safety comments included there into annotations.
It also changes the RAII classes in wallet/wallet.h and rpc/blockchain.cpp to just use the atomic<bool> flag for synchronisation rather than having a mutex that doesn't actually guard anything as well.
ACKs for top commit:
MarcoFalke:
ACK 5478d6c099🗾
hebasto:
re-ACK 5478d6c099, only renamed s/`MutexGuard`/`LockGuard`/, and dropped the commit "test/util_threadnames_tests: add thread safety annotations" since the [previous](https://github.com/bitcoin/bitcoin/pull/16127#pullrequestreview-414184113) review.
ryanofsky:
Code review ACK 5478d6c099. Thanks for taking suggestions! Only changes since last review are dropping thread rename test commit d53072ec730d8eec5a5b72f7e65a54b141e62b19 and renaming mutex guard to lock guard
Tree-SHA512: 7b00d31f6f2b5a222ec69431eb810a74abf0542db3a65d1bbad54e354c40df2857ec89c00b4a5e466c81ba223267ca95f3f98d5fbc1a1d052a2c3a7d2209790a
static constexpr CMessageHeader::HEADER_SIZE is already used in this file,
src/net.cpp, in 2 instances. This commit replaces the remaining 2 integer
values with it and adds the explicit include header.
Co-authored by: Gleb Naumenko <naumenko.gs@gmail.com>
If 1000 potential peers are known, wait for 5m before querying DNS seeds
for more peers, since eventually the addresses we already know should
get us connected. Also check every 11s whether we've got enough active
outbounds that DNS seeds aren't worth querying, and exit the dnsseed
thread early if so.
16d6113f4f Refactor message transport packaging (Jonas Schnelli)
Pull request description:
This PR factors out transport packaging logic from `CConnman::PushMessage()`.
It's similar to #16202 (where we refactor deserialization).
This allows implementing a new message transport protocol like BIP324.
ACKs for top commit:
dongcarl:
ACK 16d6113f4f FWIW
ariard:
Code review ACK 16d6113
elichai:
semiACK 16d6113f4f ran functional+unit tests.
MarcoFalke:
ACK 16d6113f4f🙎
Tree-SHA512: 8c2f8ab9f52e9b94327973ae15019a08109d5d9f9247492703a842827c5b5d634fc0411759e0bb316d824c586614b0220c2006410851933613bc143e58f7e6c1
3c1bc40205 Add extra logging of asmap use and bucketing (Gleb Naumenko)
e4658aa8ea Return mapped AS in RPC call getpeerinfo (Gleb Naumenko)
ec45646de9 Integrate ASN bucketing in Addrman and add tests (Gleb Naumenko)
8feb4e4b66 Add asmap utility which queries a mapping (Gleb Naumenko)
Pull request description:
This PR attempts to solve the problem explained in #16599.
A particular attack which encouraged us to work on this issue is explained here [[Erebus Attack against Bitcoin Peer-to-Peer Network](https://erebus-attack.comp.nus.edu.sg/)] (by @muoitranduc)
Instead of relying on /16 prefix to diversify the connections every node creates, we would instead rely on the (ip -> ASN) mapping, if this mapping is provided.
A .map file can be created by every user independently based on a router dump, or provided along with the Bitcoin release. Currently we use the python scripts written by @sipa to create a .map file, which is no larger than 2MB (awesome!).
Here I suggest adding a field to peers.dat which would represent a hash of asmap file used while serializing addrman (or 0 for /16 prefix legacy approach).
In this case, every time the file is updated (or grouping method changed), all buckets will be re-computed.
I believe that alternative selective re-bucketing for only updated ranges would require substantial changes.
TODO:
- ~~more unit tests~~
- ~~find a way to test the code without including >1 MB mapping file in the repo.~~
- find a way to check that mapping file is not corrupted (checksum?)
- comments and separate tests for asmap.cpp
- make python code for .map generation public
- figure out asmap distribution (?)
~Interesting corner case: I’m using std::hash to compute a fingerprint of asmap, and std::hash returns size_t. I guess if a user updates the OS to 64-bit, then the hash of asap will change? Does it even matter?~
ACKs for top commit:
laanwj:
re-ACK 3c1bc40205
jamesob:
ACK 3c1bc40205 ([`jamesob/ackr/16702.3.naumenkogs.p2p_supplying_and_using`](https://github.com/jamesob/bitcoin/tree/ackr/16702.3.naumenkogs.p2p_supplying_and_using))
jonatack:
ACK 3c1bc40205
Tree-SHA512: e2dc6171188d5cdc2ab2c022fa49ed73a14a0acb8ae4c5ffa970172a0365942a249ad3d57e5fb134bc156a3492662c983f74bd21e78d316629dcadf71576800c
-BEGIN VERIFY SCRIPT-
# Delete outdated alias for RecursiveMutex
sed -i -e '/CCriticalSection/d' ./src/sync.h
# Replace use of outdated alias with RecursiveMutex
sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
Instead of using /16 netgroups to bucket nodes in Addrman for connection
diversification, ASN, which better represents an actor in terms
of network-layer infrastructure, is used.
For testing, asmap.raw is used. It represents a minimal
asmap needed for testing purposes.
b6d2183858 Minor refactoring to remove implied m_addr_relay_peer. (User)
a552e8477c added asserts to check m_addr_known when it's used (User)
090b75c14b p2p: Avoid allocating memory for addrKnown where we don't need it (User)
Pull request description:
We should allocate memory for addrKnown filter only for those peers which are expected to participate in address relay.
Currently, we do it for all peers (including SPV and block-relay-only), which results in extra RAM where it's not needed.
Upd:
In future, we would still allow SPVs to ask for addrs, so allocation still will be done by default.
However, they will be able to opt-out via [this proposal](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-October/017428.html) and then we could save some more memory.
This PR still saves memory for block-relay-only peers immediately after merging.
Top commit has no ACKs.
Tree-SHA512: e84d93b2615556d466f5ca0e543580fde763911a3bfea3127c493ddfaba8f05c8605cb94ff795d165af542b594400995a2c51338185c298581408687e7812463
c72906dcc1 refactor: Remove redundant c_str() calls in formatting (Wladimir J. van der Laan)
Pull request description:
Our formatter, tinyformat, *never* needs `c_str()` for strings. Still, many places call it redundantly, resulting in longer code and a slight overhead.
Remove redundant `c_str()` calls for:
- `strprintf`
- `LogPrintf`
- `tfm::format`
(also, combined with #17095, I think this improves logging in case of unexpected embedded NULL characters)
ACKs for top commit:
ryanofsky:
Code review ACK c72906dcc1. Easy to review with `git log -p -n1 --word-diff-regex=. -U0 c72906dcc11a73fa06a0adf97557fa756b551bee`
Tree-SHA512: 9e21e7bed8aaff59b8b8aa11571396ddc265fb29608c2545b1fcdbbb36d65b37eb361db6688dd36035eab0c110f8de255375cfda50df3d9d7708bc092f67fefc
ed2dc5e48a Add override/final modifiers to V1TransportDeserializer (Pieter Wuille)
f342a5e61a Make resetting implicit in TransportDeserializer::Read() (Pieter Wuille)
6a91499496 Remove oversized message detection from log and interface (Pieter Wuille)
b0e10ff4df Force CNetMessage::m_recv to use std::move (Jonas Schnelli)
efecb74677 Use adapter pattern for the network deserializer (Jonas Schnelli)
1a5c656c31 Remove transport protocol knowhow from CNetMessage / net processing (Jonas Schnelli)
6294ecdb8b Refactor: split network transport deserializing from message container (Jonas Schnelli)
Pull request description:
**This refactors the network message deserialization.**
* It transforms the `CNetMessage` into a transport protocol agnostic message container.
* A new class `TransportDeserializer` (unique pointer of `CNode`) is introduced, handling the network buffer reading and the decomposing to a `CNetMessage`
* **No behavioral changes** (in terms of disconnecting, punishing)
* Moves the checksum finalizing into the `SocketHandler` thread (finalizing was in `ProcessMessages` before)
The **optional last commit** makes the `TransportDeserializer` following an adapter pattern (polymorphic interface) to make it easier to later add a V2 transport protocol deserializer.
Intentionally not touching the sending part.
Pre-Requirement for BIP324 (v2 message transport protocol).
Replacement for #14046 and inspired by a [comment](https://github.com/bitcoin/bitcoin/pull/14046#issuecomment-431528330) from sipa
ACKs for top commit:
promag:
Code review ACK ed2dc5e48a.
marcinja:
Code review ACK ed2dc5e48a
ryanofsky:
Code review ACK ed2dc5e48a. 4 cleanup commits added since last review. Unaddressed comments:
ariard:
Code review and tested ACK ed2dc5e.
Tree-SHA512: bab8d87464e2e8742529e488ddcdc8650f0c2025c9130913df00a0b17ecdb9a525061cbbbd0de0251b76bf75a8edb72e3ad0dbf5b79e26f2ad05d61b4e4ded6d
6170ec5d3a Do not query all DNS seed at once (Pieter Wuille)
Pull request description:
Before this PR, when we don't have enough connections after 11 seconds, we proceed to query all DNS seeds in a fixed order, loading responses from all of them.
Change this to to only query three randomly-selected DNS seed. If 11 seconds later we still don't have enough connections, try again with another one, and so on.
This reduces the amount of information DNS seeds can observe about the requesters by spreading the load over all of them.
ACKs for top commit:
Sjors:
ACK 6170ec5d3
sdaftuar:
ACK 6170ec5d3a
jonasschnelli:
utACK 6170ec5d3a - I think the risk of a single seeder codebase is orthogonal to this PR. Such risks could also be interpreted differently (diversity could also increase the risk based on the threat model).
fanquake:
ACK 6170ec5d3a - Agree with the reasoning behind the change. Did some testing with and without `-forcednsseed` and/or a `peers.dat` and monitored the DNS activity.
Tree-SHA512: 33f6be5f924a85d312303ce272aa8f8d5e04cb616b4b492be98832e3ff37558d13d2b16ede68644ad399aff2bf5ff0ad33844e55eb40b7f8e3fddf9ae43add57
0ba08020c9 Disconnect peers violating blocks-only mode (Suhas Daftuar)
937eba91e1 doc: improve comments relating to block-relay-only peers (Suhas Daftuar)
430f489027 Don't relay addr messages to block-relay-only peers (Suhas Daftuar)
3a5e885306 Add 2 outbound block-relay-only connections (Suhas Daftuar)
b83f51a4bb Add comment explaining intended use of m_tx_relay (Suhas Daftuar)
e75c39cd42 Check that tx_relay is initialized before access (Suhas Daftuar)
c4aa2ba822 [refactor] Change tx_relay structure to be unique_ptr (Suhas Daftuar)
4de0dbac9b [refactor] Move tx relay state to separate structure (Suhas Daftuar)
26a93bce29 Remove unused variable (Suhas Daftuar)
Pull request description:
Transaction relay is optimized for a combination of redundancy/robustness as well as bandwidth minimization -- as a result transaction relay leaks information that adversaries can use to infer the network topology.
Network topology is better kept private for (at least) two reasons:
(a) Knowledge of the network graph can make it easier to find the source IP of a given transaction.
(b) Knowledge of the network graph could be used to split a target node or nodes from the honest network (eg by knowing which peers to attack in order to achieve a network split).
We can eliminate the risks of (b) by separating block relay from transaction relay; inferring network connectivity from the relay of blocks/block headers is much more expensive for an adversary.
After this commit, bitcoind will make 2 additional outbound connections that are only used for block relay. (In the future, we might consider rotating our transaction-relay peers to help limit the effects of (a).)
ACKs for top commit:
sipa:
ACK 0ba08020c9
ajtowns:
ACK 0ba08020c9 -- code review, ran tests. ran it on mainnet for a couple of days with MAX_BLOCKS_ONLY_CONNECTIONS upped from 2 to 16 and didn't observe any unexpected behaviour: it disconnected a couple of peers that tried sending inv's, and it successfully did compact block relay with some block relay peers.
TheBlueMatt:
re-utACK 0ba08020c9. Pointed out that stats.fRelayTxes was sometimes uninitialized for blocksonly peers (though its not a big deal and only effects RPC), which has since been fixed here. Otherwise changes are pretty trivial so looks good.
jnewbery:
utACK 0ba08020c9
jamesob:
ACK 0ba08020c9
Tree-SHA512: 4c3629434472c7dd4125253417b1be41967a508c3cfec8af5a34cad685464fbebbb6558f0f8f5c0d4463e3ffa4fa3aabd58247692cb9ab8395f4993078b9bcdf
Transaction relay is primarily optimized for balancing redundancy/robustness
with bandwidth minimization -- as a result transaction relay leaks information
that adversaries can use to infer the network topology.
Network topology is better kept private for (at least) two reasons:
(a) Knowledge of the network graph can make it easier to find the source IP of
a given transaction.
(b) Knowledge of the network graph could be used to split a target node or
nodes from the honest network (eg by knowing which peers to attack in order to
achieve a network split).
We can eliminate the risks of (b) by separating block relay from transaction
relay; inferring network connectivity from the relay of blocks/block headers is
much more expensive for an adversary.
After this commit, bitcoind will make 2 additional outbound connections that
are only used for block relay. (In the future, we might consider rotating our
transaction-relay peers to help limit the effects of (a).)
59cb722fd0 Update configure to reject unsafe miniUPnPc API ver (Hennadii Stepanov)
ab2190557e doc: Add release notes for 15993 (Hennadii Stepanov)
02709e9560 Align formatting with clang-format (Hennadii Stepanov)
91a1b85083 Use PACKAGE_NAME in UPnP description (Hennadii Stepanov)
9f76e45b9d Drop support of insecure miniUPnPc versions (Hennadii Stepanov)
Pull request description:
1. Minimum supported miniUPnPc API version is set to 10:
- https://packages.ubuntu.com/xenial/libminiupnpc-dev
- https://packages.debian.org/jessie/libminiupnpc-dev
Refs:
- #6583
- #6789
- #10414
2. The hardcoded "Bitcoin" replaced with `PACKAGE_NAME`:
3. Also style-only commit applied.
Pardon: could not reopen my previous PR #15966.
ACKs for top commit:
ryanofsky:
utACK 59cb722fd0. Changes since last review: adding a new commit which updates configure script to fall back to disabling upnp if version is too old, adding a requested comment explaining static_assert condition, and fixing a spelling (jessy/jessie)
Tree-SHA512: 42ed11bc2fb2ec83d5dd58e2383da5444a24fd572707f6cf10b622cb8943e28adfcca4750d06801024c4472625b5ea9279516fbd9d2ccebc9bbaafe1d148e80d
-BEGIN VERIFY SCRIPT-
sed -i 's/inline std::string _(const char\* psz)/inline bilingual_str _(const char\* psz)/' src/util/translation.h
sed -i 's/return G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz;/return bilingual_str{psz, G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz};/' src/util/translation.h
sed -i 's/\b_("\([^"]\|\\"\)*")/&.translated/g' $(git grep --files-with-matches '\b_("' src)
echo Hard cases - multiline strings.
sed -i 's/"Visit %s for further information about the software.")/&.translated/g' src/init.cpp
sed -i "s/\"Only rebuild the block database if you are sure that your computer's date and time are correct\")/&.translated/g" src/init.cpp
sed -i 's/" restore from a backup.")/&.translated/g' src/wallet/db.cpp
sed -i 's/" or address book entries might be missing or incorrect.")/&.translated/g' src/wallet/wallet.cpp
echo Special case.
sed -i 's/_(COPYRIGHT_HOLDERS)/&.translated/' src/util/system.cpp test/lint/lint-format-strings.py
-END VERIFY SCRIPT-
Fixes a bug where feelers could be stuck trying to resolve a collision in the
tried table that is to an address in the same netgroup as an existing outbound peer.
Thanks to Muoi Tran for the original bug report and detailed debug logs to track
this down.
0164b0f5cf build: Remove WINVER pre define in Makefile.leveldb.inlcude (Chun Kuan Lee)
d0522ec94e Drop defunct Windows compat fixes (Ben Woosley)
d8a2992067 windows: Call SetProcessDEPPolicy directly (Chun Kuan Lee)
1bd9ffdd44 windows: Set _WIN32_WINNT to 0x0601 (Windows 7) (Chun Kuan Lee)
Pull request description:
The current minimum support Windows version is Vista. So set it to 0x0600
5a88def8ad/mingw-w64-headers/include/sdkddkver.h (L19)
Tree-SHA512: 38e2afc79426ae547131c8ad3db2e0a7f54a95512f341cfa0c06e4b2fe79521ae67d2795ef96b0192e683e4f1ba6183c010d7b4b8d6b3e68b9bf48c374c59e7d
This allows incoming connections from peers which are only banned
due to an automatic misbehavior ban if doing so won't fill inbound.
These peers are preferred for eviction when inbound fills, but may
still be kept if they fall into the protected classes. This
eviction preference lasts the entire life of the connection even
if the ban expires.
If they misbehave again they'll still get disconnected.
The main purpose of banning on misbehavior is to prevent our
connections from being wasted on unhelpful peers such as ones
running incompatible consensus rules. For inbound peers this
can be better accomplished with eviction preferences.
A secondary purpose was to reduce resource waste from repeated
abuse but virtually any attacker can get a nearly unlimited
supply of addresses, so disconnection is about the best we can
do.
These are separate events which need to be carried out by separate subsystems.
This also cleans up some whitespace and tabs in qt to avoid getting flagged by
the linter.
Current behavior is preserved.
fac2f5ecae Use C++11 default member initializers (MarcoFalke)
Pull request description:
The second and last change on this topic (c.f. #15109). Split up because the diff would otherwise interleave, making review harder than necessary.
This is not a stylistic change, but a change that avoids bugs such as:
* fix uninitialized read when stringifying an addrLocal #14728
* qt: Initialize members in WalletModel #12426
* net: correctly initialize nMinPingUsecTime #6636
* ...
Tree-SHA512: 547ae72b87aeaed5890eb5fdcff612bfc93354632b238d89e1e1c0487187f39609bcdc537ef21345e0aea8cfcf1ea48da432d672c5386dd87cf58742446a86b1
6dc4593db1 IsReachable is the inverse of IsLimited (DRY). Includes unit tests (marcaiaf)
Pull request description:
IsReachable is the inverse of IsLimited, but the implementation is duplicated (DRY)
- Changed the implementation accordingly.
- Added unit tests to document behavior and relationship
- My modification in net.cpp applies only to IsReachable.
- Applied clang-format-diffpy
Created new pull request to avoid the mess with:
https://github.com/bitcoin/bitcoin/pull/15044
Checked with supposedly conflicting PRs mentioned in the old PR. No conflicts with the specific changes in this PR.
Tree-SHA512: b132dec6cc2c788ebe4f63f228d78f441614e156743b17adebc990de0180a5872874d2724c86eeaa470b4521918bd137b0e33ebcaae77c5efc1f0d56104f6c87
fa2510d5c1 Use C++11 default member initializers (MarcoFalke)
Pull request description:
Changes:
* Remove unused constructors that leave some members uninitialized
* Remove manual initialization in each constructor and prefer C++11 default member initializers
This is not a stylistic change, but a change that avoids bugs such as:
* fix uninitialized read when stringifying an addrLocal #14728
* qt: Initialize members in WalletModel #12426
* net: correctly initialize nMinPingUsecTime #6636
* ...
Tree-SHA512: 0f896f3b9fcc464d5fc7525f7c86343ef9ce9fb13425fbc68e9a9728fd8710c2b4e2fd039ee08279ea41ff20fd92b7185cf5cca95a0bcb6a5340a1e6f03cae6b
4927bf2f25 Increase maxconnections limit when using poll. (Patrick Strateman)
11cc491a28 Implement poll() on systems which support it properly. (Patrick Strateman)
28211a4bc9 Move SocketEvents logic to private method. (Patrick Strateman)
7e403c0ae7 Move GenerateSelectSet logic to private method. (Patrick Strateman)
1e6afd0dbc Introduce and use constant SELECT_TIMEOUT_MILLISECONDS. (Patrick Strateman)
Pull request description:
Implement poll() on systems which support it properly.
This eliminates the restriction on maximum socket descriptor number.
Tree-SHA512: b945cd9294afdafcce96d547f67679d5cdd684cf257904a239cd1248de3b5e093b8d6d28d8d1b7cc923dc0b2b5723faef9bc9bf118a9ce1bdcf357c2323f5573
48b37db50 make peertimeout a debug argument, remove error message translation (Zain Iqbal Allarakhia)
8042bbfbf p2p: allow p2ptimeout to be configurable, speed up slow test (Zain Iqbal Allarakhia)
Pull request description:
**Summary:**
1. _Primary_: Adds a `debug_only=true` flag for peertimeout, defaults to 60 sec., the current hard-coded setting.
2. _Secondary_: Drastically speeds up `p2p_timeout.py` test.
3. _Secondary_: Tests that the correct code path is being tested by adding log assertions to the test.
**Rationale:**
- P2P timeout was hard-coded: make it explicitly specified and configurable, instead of a magic number.
- Addresses #13518; `p2p_timeout.py` takes 4 sec. to run instead of 61 sec.
- Makes `p2p_timeout.py` more explicit. Previously, we relied on a comment to inform us of the timeout amount being tested. Now it is specified directly in the test via passing in the new arg; `-peertimeout=3`.
- Opens us up to testing more P2P scenarios; oftentimes slow tests are the reason we don't test.
**Locally verified changes:**
_With Proposed Change (4.7 sec.):_
```
$ time ./test/functional/p2p_timeouts.py
2018-11-19T00:04:19.077000Z TestFramework (INFO): Initializing test directory /tmp/testhja7g2n7
2018-11-19T00:04:23.479000Z TestFramework (INFO): Stopping nodes
2018-11-19T00:04:23.683000Z TestFramework (INFO): Cleaning up /tmp/testhja7g2n7 on exit
2018-11-19T00:04:23.683000Z TestFramework (INFO): Tests successful
real 0m4.743s
```
_Currently on master (62.8 sec.):_
```
$ time ./test/functional/p2p_timeouts.py
2018-11-19T00:06:10.948000Z TestFramework (INFO): Initializing test directory /tmp/test6mo6k21h
2018-11-19T00:07:13.376000Z TestFramework (INFO): Stopping nodes
2018-11-19T00:07:13.631000Z TestFramework (INFO): Cleaning up /tmp/test6mo6k21h on exit
2018-11-19T00:07:13.631000Z TestFramework (INFO): Tests successful
real 1m2.836s
```
_Error message demonstrated for new argument `-peertimeout`:_
```
$ ./bitcoind -peertimeout=-5
...
Error: peertimeout cannot be configured with a negative value.
```
Tree-SHA512: ff7a244ebea54c4059407bf4fb86465714e6a79cef5d2bcaa22cfe831a81761aaf597ba4d5172fc2ec12266f54712216fc41b5d24849e5d9dab39ba6f09e3a2a
032488e6e7 Move SocketHandler logic to private method. (Patrick Strateman)
2af9cff11a Move InactivityCheck logic to private method. (Patrick Strateman)
7479b63d91 Move DisconnectNodes logic to private method. (Patrick Strateman)
edb5350c32 Move NotifyNumConnectionsChanged logic to private method. (Patrick Strateman)
Pull request description:
Working towards using poll() on unix like systems.
A number of small changes designed to separate the actual socket handling from the rest of the logic in ThreadSocketHandler.
This is a simpler version of #14147
Tree-SHA512: 72f35c8ef7649019dcbfe19537d8c9f7e3d0fc5854dc691a70c5573352230fc31c3f55565820c632e9b8cb3c55b878bed19e0ad9423100762197ac35967d8067
9256f7d13f build: avoid getifaddrs when unavailable (Cory Fields)
Pull request description:
These changes from @theuni help building when targeting platforms that don't always have getifaddrs available like Android < 24
Tree-SHA512: dbfeb83297bd6f00b7991f53eef8a04948d2d739bf47c0524d9ae5335b843b8a5c06ff98c109fe5e6192665e6d0cf58700b8aa7e2a0b410281d3c052881973ff
f34c8c466a Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. (practicalswift)
Pull request description:
Make objects in range declarations immutable by default.
Rationale:
* Immutable objects are easier to reason about.
* Prevents accidental or hard-to-notice change of value.
Tree-SHA512: cad69d35f0cf8a938b848e65dd537c621d96fe3369be306b65ef0cd1baf6cc0a9f28bc230e1e383d810c555a6743d08cb6b2b0bd51856d4611f537a12e5abb8b
9c4dc597dd Use LOCK macros for non-recursive locks (Russell Yanofsky)
1382913e61 Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection (Russell Yanofsky)
ba1f095aad MOVEONLY Move AnnotatedMixin declaration (Russell Yanofsky)
41b88e9337 Add unit test for DEBUG_LOCKORDER code (Russell Yanofsky)
Pull request description:
Make LOCK macros work with non-recursive mutexes, and use wherever possible for better deadlock detection.
Also add unit test for DEBUG_LOCKORDER code.
Tree-SHA512: 64ef209307f28ecd0813a283f15c6406138c6ffe7f6cbbd084161044db60e2c099a7d0d2edcd1c5e7770a115e9b931b486e86c9a777bdc96d2e8a9f4dc192942
Lowering the minimum relay fee is only useful when many nodes in the
p2p network also lower the fee, so to make it easier to understand
progress on that front, this includes the value of the minfeefilter in
getpeerinfo, so you at least have visibility to what fees your neighbours
are currently accepting.
3fc20632a3 qt: Set BLOCK_CHAIN_SIZE = 220 (DrahtBot)
2b6a2f4a28 Regenerate manpages (DrahtBot)
eb7daf4d60 Update copyright headers to 2018 (DrahtBot)
Pull request description:
Some trivial maintenance to avoid having to do it again after the 0.17 branch off.
(The scripts to do this are in `./contrib/`)
Tree-SHA512: 16b2af45e0351b1c691c5311d48025dc6828079e98c2aa2e600dc5910ee8aa01858ca6c356538150dc46fe14c8819ed8ec8e4ec9a0f682b9950dd41bc50518fa
793290f940 Net: Fixed a race condition when disabling the network. (lmanners)
Pull request description:
This change addresses a race condition where setnetworkactive=false wouldn't always disconnect all peers.
Before this change, the following could happen:
1. Thread A -- Begins connecting to a node.
2. Thread B -- Sets kNetworkActive=false and disconnects connected nodes.
3. Thread A -- Finishes connecting and adds node to list of connected nodes.
The node that was connected from Thread A remains connected and active,
even though kNetworkActive=false.
To fix the race, disconnections when kNetworkActive=false are now handled in the main network loop.
fixes#13038
Tree-SHA512: 6d0b7a78ae956358e796efcc034cb532c2e0d824a52ae822a3899eefc7df76327519d1f2d77362c9fddf34ef860abd095d7490d7cc02d0ba7543bf1e8c8f2535
822a2a33a7 Modified in_addr6 cast in CConman class to work with msvc. (Aaron Clauson)
Pull request description:
Fix to allow net.cpp to compile with MSVC. Without this fix the `(in6_addr)IN6ADDR_ANY_INIT` implicit cast generates a compilation error.
Tree-SHA512: f21c5002401dc93564dcf8d49fbafe7c03ad4182df1616d2ee201e2e172f1d696ca7982fb5b42a3b7d6878c8649823044a858401b4172239fb4b0cc2a38db282
d45b344ffd Bucket for inbound when scheduling invs to hide tx time (Gleb)
Pull request description:
It has been brought up to my attention that current random delays mechanism (originally intended to obfuscate transaction metadata) allows to easily estimate the time a transaction was received by a node.
It may be done by connecting multiple observer nodes to the same node. Each of those nodes will generate its own schedule of delays. Combined metadata regarding those events from different sources allows an observer to estimate transaction time.
After this patch a spy won't gain additional information by just creating multiple connections to a target.
Tree-SHA512: c71dae5ff350b614cb40a8e201fd0562d3e03e3e72a5099718cd451f0d84c66d5e52bbaf0d5b4b75137514c8efdedcc6ef4df90142b360153f04ad0721545ab1
253f592909 Add stdin, stdout, stderr to ignored export list (Chun Kuan Lee)
fc6a9f2ab1 Use IN6ADDR_ANY_INIT instead of in6addr_any (Cory Fields)
908c1d7745 GCC-7 and glibc-2.27 compat code (Chun Kuan Lee)
Pull request description:
The `__divmoddi4` code was modified from https://github.com/gcc-mirror/gcc/blob/master/libgcc/libgcc2.c . I manually find the older glibc version of log2f by objdump, use `.symver` to specify the certain version.
Tree-SHA512: e8d875652003618c73e019ccc420e7a25d46f4eaff1c7a1a6bfc1770b3b46f074b368b2cb14df541b5ab124cca41dede4e28fe863a670589b834ef6b8713f9c4
This change addresses a race condition where setnetworkactive=false wouldn't always disconnect all peers.
Before this change, the following could happen:
1. Thread A -- Begins connecting to a node.
2. Thread B -- Sets kNetworkActive=false and disconnects connected nodes.
3. Thread A -- Finishes connecting and adds node to list of connected nodes.
The node that was connected from Thread A remains connected and active,
even though kNetworkActive=false.
To fix the race, disconnections when kNetworkActive=false are now handled in the main network loop.
fixes#13038
a5bca13 Bugfix: Include <memory> for std::unique_ptr (Luke Dashjr)
Pull request description:
Not sure why all these includes were missing, but it's breaking builds for some users:
https://bugs.gentoo.org/show_bug.cgi?id=652142
(Added to all files with a reference to `std::unique_ptr`)
Tree-SHA512: 8a2c67513ca07b9bb52c34e8a20b15e56f8af2530310d9ee9b0a69694dd05e02e7a3683f14101a2685d457672b56addec591a0bb83900a0eb8e2a43d43200509
4d9b4256d8 Fix typos (Dimitris Apostolou)
Pull request description:
Unfortunately I messed up my repo while trying to squash #12593 so I created a PR with just the correct fixes.
Tree-SHA512: 295d77b51bd2a9381f1802c263de7ffb2edd670d9647391e32f9a414705b3c8b483bb0e469a9b85ab6a70919ea13397fa8dfda2aea7a398b64b187f178fe6a06
It is redundant to check for the presence of MSG_NOSIGNAL macro in
configure.ac, define HAVE_MSG_NOSIGNAL and then check whether the later
is defined in the source code. Instead we can check directly whether
MSG_NOSIGNAL is defined. Same for MSG_DONTWAIT.
In addition to that, the checks we had in configure.ac produce a
compiler warning about unused variable and thus could fail if
-Werror is present and erroneously proclaim that the macros are
not available.
* Z is the zone designator for the zero UTC offset.
* T is the delimiter used to separate date and time.
This makes it clear for the end-user that the date/time logged is
specified in UTC and not in the local time zone.
A risk exists where a malicious DNS seeder eclipses a node by returning an enormous number of IP addresses. In this commit we mitigate this risk by limiting the number of IP addresses addrman learns to 256 per DNS seeder.
e68172ed9 Add test-before-evict discipline to addrman (Ethan Heilman)
Pull request description:
This change implement countermeasures 3 (test-before-evict) suggested in our paper: ["Eclipse Attacks on Bitcoin’s Peer-to-Peer Network"](http://cs-people.bu.edu/heilman/eclipse/).
# Design:
A collision occurs when an address, addr1, is being moved to the tried table from the new table, but maps to a position in the tried table which already contains an address (addr2). The current behavior is that addr1 would evict addr2 from the tried table.
This change ensures that during a collision, addr1 is not inserted into tried but instead inserted into a buffer (setTriedCollisions). The to-be-evicted address, addr2, is then tested by [a feeler connection](https://github.com/bitcoin/bitcoin/pull/8282). If addr2 is found to be online, we remove addr1 from the buffer and addr2 is not evicted, on the other hand if addr2 is found be offline it is replaced by addr1.
An additional small advantage of this change is that, as no more than ten addresses can be in the test buffer at once, and addresses are only cleared one at a time from the test buffer (at 2 minute intervals), thus an attacker is forced to wait at least two minutes to insert a new address into tried after filling up the test buffer. This rate limits an attacker attempting to launch an eclipse attack.
# Risk mitigation:
- To prevent this functionality from being used as a DoS vector, we limit the number of addresses which are to be tested to ten. If we have more than ten addresses to test, we drop new addresses being added to tried if they would evict an address. Since the feeler thread only creates one new connection every 2 minutes the additional network overhead is limited.
- An address in tried gains immunity from tests for 4 hours after it has been tested or successfully connected to.
# Tests:
This change includes additional addrman unittests which test this behavior.
I ran an instance of this change with a much smaller tried table (2 buckets of 64 addresses) so that collisions were much more likely and observed evictions.
```
2016-10-27 07:20:26 Swapping 208.12.64.252:8333 for 68.62.95.247:8333 in tried table
2016-10-27 07:20:26 Moving 208.12.64.252:8333 to tried
```
I documented tests we ran against similar earlier versions of this change in #6355.
# Security Benefit
This is was originally posted in PR #8282 see [this comment for full details](https://github.com/bitcoin/bitcoin/pull/8282#issuecomment-237255215).
To determine the security benefit of these larger numbers of IPs in the tried table I modeled the attack presented in [Eclipse Attacks on Bitcoin’s Peer-to-Peer Network](https://eprint.iacr.org/2015/263).
**Default node:** 595 attacker IPs for ~50% attack success.
**Default node + test-before-evict:** 620 attacker IPs for ~50% attack success.
**Feeler node:** 5540 attacker IPs for ~50% attack success.
**Feeler node + test-before-evict:** 8600 attacker IPs for ~50% attack success.
The node running feeler connections has 10 times as many online IP addresses in its tried table making an attack 10 times harder (i.e. requiring the an attacker require 10 times as many IP addresses in different /16s). Adding test-before-evict increases resistance of the node by an additional 3000 attacker IP addresses.
Below I graph the attack over even greater attacker resources (i.e. more attacker controled IP addresses). Note that test-before-evict maintains some security far longer even against an attacker with 50,000 IPs. If this node had a larger tried table test-before-evict could greatly boost a nodes resistance to eclipse attacks.
Tree-SHA512: fdad4d26aadeaad9bcdc71929b3eb4e1f855b3ee3541fbfbe25dca8d7d0a1667815402db0cb4319db6bd3fcd32d67b5bbc0e12045c4252d62d6239b7d77c4395
Changes addrman to use the test-before-evict discipline in which an
address is to be evicted from the tried table is first tested and if
it is still online it is not evicted.
Adds tests to provide test coverage for this change.
This change was suggested as Countermeasure 3 in
Eclipse Attacks on Bitcoin’s Peer-to-Peer Network, Ethan Heilman,
Alison Kendler, Aviv Zohar, Sharon Goldberg. ePrint Archive Report
2015/263. March 2015.
eb91835 Add setter for g_initial_block_download_completed (Jonas Schnelli)
3f56df5 [QA] add NODE_NETWORK_LIMITED address relay and sync test (Jonas Schnelli)
158e1a6 [QA] fix mininode CAddress ser/deser (Jonas Schnelli)
fa999af [QA] Allow addrman loopback tests (add debug option -addrmantest) (Jonas Schnelli)
6fe57bd Connect to peers signaling NODE_NETWORK_LIMITED when out-of-IBD (Jonas Schnelli)
31c45a9 Accept addresses with NODE_NETWORK_LIMITED flag (Jonas Schnelli)
Pull request description:
Eventually connect to peers signalling NODE_NETWORK_LIMITED if we are out of IBD.
Accept and relay NODE_NETWORK_LIMITED peers in addrman.
Tree-SHA512: 8a238fc97f767f81cae1866d6cc061390f23a72af4a711d2f7158c77f876017986abb371d213d1c84019eef7be4ca951e8e6f83fda36769c4e1a1d763f787037
9ad6746ccd Use static_cast instead of C-style casts for non-fundamental types (practicalswift)
Pull request description:
A C-style cast is equivalent to try casting in the following order:
1. `const_cast(...)`
2. `static_cast(...)`
3. `const_cast(static_cast(...))`
4. `reinterpret_cast(...)`
5. `const_cast(reinterpret_cast(...))`
By using `static_cast<T>(...)` explicitly we avoid the possibility of an unintentional and dangerous `reinterpret_cast`. Furthermore `static_cast<T>(...)` allows for easier grepping of casts.
For a more thorough discussion, see ["ES.49: If you must use a cast, use a named cast"](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast) in the C++ Core Guidelines (Stroustrup & Sutter).
Tree-SHA512: bd6349b7ea157da93a47b8cf238932af5dff84731374ccfd69b9f732fabdad1f9b1cdfca67497040f14eaa85346391404f4c0495e22c467f26ca883cd2de4d3c
2b839ab Update chainparams comment for more info on service bits per dnsseed (Matt Corallo)
62e7642 Fall back to oneshot for DNS Seeds which don't support filtering. (Matt Corallo)
51ae766 Use GetDesireableServiceFlags in static seeds, document this. (Matt Corallo)
fb6f6b1 bluematt's testnet-seed now supports x9 (and is just a static list) (Matt Corallo)
Pull request description:
4440710 broke inserting entries into addrman from dnsseeds which
did not support service bits, as well as static seeds. Static seeds
were already being filtered by UA for 0.13.1+ (ie NODE_WITNESS), so
simply changing the default service bits to include NODE_WITNESS
(and updating docs appropriately) is sufficient. For DNS Seeds, not
supporting NODE_WITNESS is no longer useful, so instead use
non-filtering seeds as oneshot hosts irrespective of named proxy.
I've set my testnet-seed to also support x9, though because it is simply a static host, it may be useful to leave the support off so that it is used as a oneshot to get addresses from a live node instead. I'm fine with either.
Tree-SHA512: 3f17d4d2b0b84d876981c962d2b44cb0c8f95f52c56a48c6b35fd882f6d7a40805f320ec452985a1c0b34aebddb1922709156c3ceccd1b9f8363fd7cb537d21d
This allows us to not have to update the chainparams whenever a
DNS Seed changes its filtering support, as well fixes a bug
introduced in 44407100f where returned nodes will never be
attempted.
44407100f broke inserting entries into addrman from static seeds
(as well as dnsseeds which did not support service bits). Static
seeds were already being filtered by UA for 0.13.1+ (ie
NODE_WITNESS), so simply changing the default service bits to
include NODE_WITNESS (and updating docs appropriately) is
sufficient.
For DNS Seeds, we will later fix by falling back to oneshot if a
seed does not support filtering.
57ac471 Call BannedListChanged outside of cs_setBanned lock (Jonas Schnelli)
c853812 Update ban-state in case of dirty-state during periodic sweep (Jonas Schnelli)
Pull request description:
We do currently not update the UI during periodic ban list sweeps (via dump banlist).
Fixes#11612
Tree-SHA512: bffbdcc03c63042177bdd511b0a9187c211c2b5011178481e8ee3e43a71eef1e4cd6b72f73672babab142b644f62f8b56f0aac1d26d3f19372b1f8644fec9395
3830b6e net: use CreateSocket for binds (Cory Fields)
df3bcf8 net: pass socket closing responsibility up to caller for outgoing connections (Cory Fields)
9e3b2f5 net: Move IsSelectableSocket check into socket creation (Cory Fields)
1729c29 net: split socket creation out of connection (Cory Fields)
Pull request description:
Requirement for #11227.
We'll need to create sockets and perform the actual connect in separate steps, so break them up.
#11227 adds an RAII wrapper around connection attempts, as a belt-and-suspenders in case a CloseSocket is missed.
Tree-SHA512: de675bb718cc56d68893c303b8057ca062c7431eaa17ae7c4829caed119fa3f15b404d8f52aca22a6bca6e73a26fb79e898b335d090ab015bf6456cf417fc694
We use select in ConnectSocketDirectly, so this check needs to happen before
that.
IsSelectableSocket will not be relevant after upcoming changes to remove select.
a720b92 Remove includes in .cpp files for things the corresponding .h file already included (practicalswift)
Pull request description:
Remove includes in .cpp files for things the corresponding .h file already included.
Example case:
* `addrdb.cpp` includes `addrdb.h` and `fs.h`
* `addrdb.h` includes `fs.h`
Then remove the direct inclusion of `fs.h` in `addrman.cpp` and rely on the indirect inclusion of `fs.h` via the included `addrdb.h`.
In line with the header include guideline (see #10575).
Tree-SHA512: 8704b9de3011a4c234db336a39f7d2c139e741cf0f7aef08a5d3e05197e1e18286b863fdab25ae9638af4ff86b3d52e5cab9eed66bfa2476063aa5c79f9b0346
be9f38c Do not make it trivial for inbound peers to generate log entries (Matt Corallo)
Pull request description:
Based on #11580 because I'm lazy.
We should generally avoid writing to debug.log unconditionally for
inbound peers which misbehave (the peer being about to be banned
being an exception, since they cannot do this twice).
Tree-SHA512: 8e59c8d08d00b1527951b30f4842d010a4c2fc440503ade112baa2c1b9afd0e0d1c5c2df83dde25183a242af45089cf9b9f873b71796771232ffb6c5fc6cc0cc
680bc2cbb Use range-based for loops (C++11) when looping over map elements (practicalswift)
Pull request description:
Before this commit:
```c++
for (std::map<T1, T2>::iterator x = y.begin(); x != y.end(); ++x) {
T1 z = (*x).first;
…
}
```
After this commit:
```c++
for (auto& x : y) {
T1 z = x.first;
…
}
```
Tree-SHA512: 954b136b7f5e6df09f39248a6b530fd9baa9ab59d7c2c7eb369fd4afbb591b7a52c92ee25f87f1745f47b41d6828b7abfd395b43daf84a55b4e6a3d45015e3a0
We should generally avoid writing to debug.log unconditionally for
inbound peers which misbehave (the peer being about to be banned
being an exception, since they cannot do this twice).
To avoid removing logs for outbound peers, a new log is added to
notify users when a new outbound peer is connected which mimics
the version print.
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)
Pull request description:
Rationale:
* `delete ptr` is a no-op if `ptr` is `nullptr`
Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
5ce7cb9 [net] De-duplicate connection eviction logic (Thomas Snider)
Pull request description:
While reviewing the safeguards against deliberate node isolation on the network by malicious actors, I found a good de-duplication candidate.
I think this form is much more legible (the type of `cutoffs` notwithstanding). ReverseCompareNodeTimeConnected is not included in the list since the cutoff size is a function of the remaining number of nodes in the candidate eviction set.
Tree-SHA512: ed17999fa9250dcf8448329219324477117e4ecd2d41dedd72ad253e44630eef50b3232c420f1862ebbfb9b8c94efbba1a235b519e39ff5946865c7d69a75280
If our tip hasn't updated in a while, that may be because our peers are
not relaying blocks to us that we would consider valid. Allow connection
to an additional outbound peer in that circumstance.
Also, periodically check to see if we are exceeding our target number of
outbound peers, and disconnect the one which has least recently
announced a new block to us (choosing the newest such peer in the case
of tie).
Adds HasAllRelevantServices and GetRelevantServices, which check
for NETWORK|WITNESS.
This changes the following:
* Removes nRelevantServices from CConnman, disconnecting it a bit
more from protocol-level logic.
* Replaces our sometimes-connect-to-!WITNESS-nodes logic with
simply always requiring WITNESS|NETWORK for outbound non-feeler
connections (feelers still only require NETWORK).
* This has the added benefit of removing nServicesExpected from
CNode - instead letting net_processing's VERSION message
handling simply check HasAllRelevantServices.
* This implies we believe WITNESS nodes to continue to be a
significant majority of nodes on the network, but also because
we cannot sync properly from !WITNESS nodes, it is strange to
continue using our valuable outbound slots on them.
* In order to prevent this change from preventing connection to
-connect= nodes which have !WITNESS, -connect nodes are now
given the "addnode" flag. This also allows outbound connections
to !NODE_NETWORK nodes for -connect nodes (which was already true
of addnodes).
* Has the (somewhat unintended) consequence of changing one of the
eviction metrics from the same
sometimes-connect-to-!WITNESS-nodes metric to requiring
HasRelevantServices.
This should make NODE_NETWORK_LIMITED much simpler to implement.
b887676 net: remove now-unused functions (Cory Fields)
45fd754 net: remove now-superfluous numeric resolve (Cory Fields)
2416dd7 net: separate resolving and conecting (Cory Fields)
Pull request description:
This is a greatly simplified version of #10285, which only aims to address async resolving.
It essentially breaks up two wrapper functions for things only used in one place (ConnectSocketDirectly/ConnectThroughProxy) in favor of calling them directly. This allows us to fully handle resolves before attempting a connection, as is necessary for async connections.
As a bonus, I believe the logic is now much easier to follow than before.
Tree-SHA512: f03f618107379edf3efe2a9f3e3677e8f075017ab140a0b4fdc3b8263e6beff148d55256263ab10bc2125ef089ca68e0d8e865beeae176f1eca544e769c976d3
A C-style cast is equivalent to try casting in the following order:
1. const_cast(...)
2. static_cast(...)
3. const_cast(static_cast(...))
4. reinterpret_cast(...)
5. const_cast(reinterpret_cast(...))
By using static_cast<T>(...) explicitly we avoid the possibility
of an unintentional and dangerous reinterpret_cast. Furthermore
static_cast<T>(...) allows for easier grepping of casts.
This was added in order to help OpenNetworkConnection avoid creating a
connection that it would end up aborting. It was necessary because resolving
was done as part of the connection process.
Now that resolving is separated from connecting, this case is detected before
the connection is attempted.
ConnectSocketByName handled resolves as necessary, obscuring the connection
process. With them separated, each can be handled asynchronously.
Also, since proxies must be considered now anyway, go ahead and eliminate the
ConnectSocket wrapper and use ConnectSocketDirectly... directly.
There are a few too many edge-cases here to make this a scripted diff.
The following commits will move a few functions into PeerLogicValidation, where
the local connman instance can be used. This change prepares for that usage.
352d582ba Add vConnect to CConnman::Options (Marko Bencun)
Pull request description:
Split the "-connect" argument parsing out of CConnman and put it into
AppInitMain().
Tree-SHA512: f2d3efc4e2c5808ff98696ea20dd96df599bc472ed5afc9c3eea305d94c36a6ab50c632aa05396c7c34d1917d91b1e7ccd725656ff2631e2a36d9eac477455dc
When running test_bitcoin under Valgrind I found the following issue:
```
$ valgrind src/test/test_bitcoin
...
==10465== Use of uninitialised value of size 8
==10465== at 0x6D09B61: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==10465== by 0x6D0B1BB: std::ostreambuf_iterator<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<unsigned long>(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, unsigned long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==10465== by 0x6D0B36C: std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::do_put(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, unsigned long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==10465== by 0x6D17699: std::ostream& std::ostream::_M_insert<unsigned long>(unsigned long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==10465== by 0x4CAAD7: operator<< (ostream:171)
==10465== by 0x4CAAD7: formatValue<ServiceFlags> (tinyformat.h:345)
==10465== by 0x4CAAD7: void tinyformat::detail::FormatArg::formatImpl<ServiceFlags>(std::ostream&, char const*, char const*, int, void const*) (tinyformat.h:523)
==10465== by 0x1924D4: format (tinyformat.h:510)
==10465== by 0x1924D4: tinyformat::detail::formatImpl(std::ostream&, char const*, tinyformat::detail::FormatArg const*, int) (tinyformat.h:803)
==10465== by 0x553A55: vformat (tinyformat.h:947)
==10465== by 0x553A55: format<ServiceFlags> (tinyformat.h:957)
==10465== by 0x553A55: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<ServiceFlags>(char const*, ServiceFlags const&) (tinyformat.h:966)
==10465== by 0x54C952: getnetworkinfo(JSONRPCRequest const&) (net.cpp:462)
==10465== by 0x28EDB5: CallRPC(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (rpc_tests.cpp:31)
==10465== by 0x293947: rpc_tests::rpc_togglenetwork::test_method() (rpc_tests.cpp:88)
==10465== by 0x2950E5: rpc_tests::rpc_togglenetwork_invoker() (rpc_tests.cpp:84)
==10465== by 0x182496: invoke<void (*)()> (callback.hpp:56)
==10465== by 0x182496: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
...
```
The read of the uninitialized variable nLocalServices is triggered by g_connman->GetLocalServices()
in getnetworkinfo(const JSONRPCRequest& request) (net.cpp:462):
```c++
UniValue getnetworkinfo(const JSONRPCRequest& request)
{
...
if(g_connman)
obj.push_back(Pair("localservices", strprintf("%016x", g_connman->GetLocalServices())));
...
}
```
The reason for the uninitialized nLocalServices is that CConnman::Start(...) is not called
by the tests, and hence the initialization normally performed by CConnman::Start(...) is
not done.
This commit adds a method Init(const Options& connOptions) which is called by both the
constructor and CConnman::Start(...). This method initializes nLocalServices and the other
relevant values from the supplied Options object.
f228b8e remove some unused functions (Marko Bencun)
Pull request description:
Identified with `cppcheck --enable=unusedFunction .`.
- GetSendBufferSize()'s last use removed in
991955e
- SetPort()'s last use removed in
7e195e8
- GetfLargeWorkInvalidChainFound() was introduced in
e3ba0ef and never used
Tree-SHA512: ea8e5498bec981e42e1342c171c37723c2f5e575c7d6c1a524d9c6cd9b332bdd0d84fddf9e14ca011bb49fb82bd037386382c9afc546b3c2231ae548358bd4f4
a2420ae Avoid unnecessary work in SetNetworkActive (João Barbosa)
Pull request description:
This PR adds an early return to avoid unnecessary notifications when the status doesn't change.
Tree-SHA512: 85d05ca6fa36cb581f94bc154d08bd72cd53f6a857173c6fb2f184058f9c0208c4cf5e5d196825a78339902d8f256688eb6793f99abc7be9c7cfac85136180d9
Identified with `cppcheck --enable=unusedFunction .`.
- GetSendBufferSize()'s last use removed in
991955ee81
- SetPort()'s last use removed in
7e195e8459
- GetfLargeWorkInvalidChainFound() was introduced in
e3ba0ef956 and never used
c1be285 chainparams: make supported service bits option explicit (Cory Fields)
d5c7c1c net: use an internal address for fixed seeds (Cory Fields)
6cdc488 net: switch to dummy internal ip for dns seed source (Cory Fields)
6d0bd5b net: do not allow resolving to an internal address (Cory Fields)
7f31762 net: add an internal subnet for representing unresolved hostnames (Cory Fields)
Tree-SHA512: 9bf1042bef546ac3ef0e0d3a9a5555eb21628ff2674a0cf8c6367194b22bfdab477adf452c0e7c56f44e0fb37debc5e14bdb623452e076fb9c492c7702601d7a
Part of a series of changes to clean up the instantiation of connman
by decoupling the command line arguments.
We also now abort with an error when explicit binds are set with
-listen=0.
We currently do two resolves for dns seeds: one for the results, and one to
serve in addrman as the source for those addresses.
There's no requirement that the source hostname resolves to the stored
identifier, only that the mapping is unique. So rather than incurring the
second lookup, combine a private subnet with a hash of the hostname.
The resulting v6 ip is guaranteed not to be publicy routable, and has only a
negligible chance of colliding with a user's internal network (which would be
of no consequence anyway).
This adds the listening address on which incoming connections were received to the
CNode and CNodeStats structures.
The address is reported in `getpeerinfo`.
This can be useful for distinguishing connections received on different listening ports
(e.g. when using a different listening port for Tor hidden service connections)
or different networks.
b6fbfc2 net: only enforce the services required to connect (Cory Fields)
Tree-SHA512: 88943bff63213a734f3c96c45760cadaeb9ba18287c8a20c279851ebaf058a334c969028fb2180f155508e3eea4b838147382e4f2b655e7a9aa098eadc81d53e
Previously if we didn't have any local addresses, GetLocalAddress would return
0.0.0.0 and then we'd swap in a peer's notion of our address in AdvertiseLocal,
but then nServices would never get set.
0f3471f net: make CNode's id private (Cory Fields)
9ff0a51 scripted-diff: net: Use accessor rather than node's id directly (Cory Fields)
e50c33e devtools: add script to verify scriptable changes (Cory Fields)
Tree-SHA512: a0ff50f4e1d38a2b63109b4996546c91b3e02e00d92c0bf04f48792948f78b1f6d9227a15d25c823fd4723a0277fc6a32c2c1287c7abbb7e50fd82ffb0f8d994
d6732d8 [tests] update disconnect_ban.py test case to work with listbanned (John Newbery)
77c54b2 [net] listbanned RPC and QT should show correct banned subnets (John Newbery)
Tree-SHA512: edd0e43377d456260d2697213c2829f8483630f3a668b6707d52605faefa610d951d10e6f22a95eff483cbd14faa8ac9b69fa7d3c0b5735c5f3df23fd71282e0
