2ea62cae483b764e30f61c06d8ac65755bbd864c 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 2ea62cae483b764e30f61c06d8ac65755bbd864c
Tree-SHA512: c9c03c09eefeacec28ea199cc3f697b0a98723f2f849f7a8115edc43791f8165e296e0e25a82f0b5a4a781a7de38c8954b48bf74c714eba02cdc21f7460673e5
faca73000fa8975c28f6be8be01957c1ae94ea62 ci: Install fixed version of clang-format for linters (MarcoFalke)
fa4695da4c69646b58a8fa0b6b30146bb234deb8 build: Sort Makefile.am after renaming file (MarcoFalke)
cccc2784a3bb10fa8e43be7e68207cafb12bd915 scripted-diff: Move ui_interface to the node lib (MarcoFalke)
fa72ca6a9d90d66012765b0043fd819698b94ba8 qt: Remove unused includes (MarcoFalke)
fac96e6450d595fe67168cb7afa7692da6cc9973 wallet: Do not include server symbols (MarcoFalke)
fa0f6c58c1c6d10f04c4e65a424cc51ebca50a8c Revert "Fix link error with --enable-debug" (MarcoFalke)
Pull request description:
This reverts a hacky workaround from commit b83cc0f, which only happens to work due to compiler optimizations. Then, it actually fixes the linker error.
The underlying problem is that the wallet includes symbols from the server (ui_interface), which usually results in linker failures. Though, in this specific case the linker failures have not been observed (unless `-O0`) because our compilers were smart enough to strip unused symbols.
Fix the underlying problem by creating a new header-only with the needed symbol and move ui_interface to node to clarify that this is part of libbitcoin_server.
ACKs for top commit:
Sjors:
ACK faca730
laanwj:
ACK faca73000fa8975c28f6be8be01957c1ae94ea62
hebasto:
re-ACK faca73000fa8975c28f6be8be01957c1ae94ea62, since the [previous](https://github.com/bitcoin/bitcoin/pull/19331#pullrequestreview-434420539) review:
Tree-SHA512: e9731f249425aaea50b6db5fc7622e10078cf006721bb87989cac190a2ff224412f6f8a7dd83efd018835302337611f5839e29e15bef366047ed591cef58dfb4
a512925e19a70d7f6b80ac530a169f45ffaafa1c [doc] Release notes (Amiti Uttarwar)
50f94b34a33c954f6e207f509c93d33267a5c3e2 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar)
df091b9b509f0b10e4315c0bfa2da0cc0c31c22f [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar)
395acfa83a5436790c1a722a5609ac9d48df235f [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar)
49c10a9ca40967d28ae16dfea9cccc6f3a6624a1 [log] Add connection type to log statement (Amiti Uttarwar)
Pull request description:
After #19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field.
This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string.
Suggested by sdaftuar- https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468001604 & https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468018093
ACKs for top commit:
jnewbery:
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
sipa:
utACK a512925e19a70d7f6b80ac530a169f45ffaafa1c
guggero:
Tested and code review ACK a512925e.
MarcoFalke:
cr ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c 🌇
promag:
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e
01e283068b9e6214f2d77a2f772a4244ebfe2274 [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar)
bc5d65b3ca41eebb1738fdda4451d1466e77772e [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar)
2f2e13b6c2c8741ca9d825eaaef736ede484bc85 [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar)
7f7b83deb2427599c129f4ff581d4d045461e459 [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar)
35839e963bf61d2da0d12f5b8cea74ac0e0fbd7b [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar)
4972c21b671ff73f13a1b5053338b6abbdb471b5 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar)
60156f5fc40d56bb532278f16ce632c5a8b8035e [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar)
7b322df6296609570e368e5f326979279041c11f [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar)
14923422b08ac4b21b35c426bf0e1b9e7c97983b [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar)
49efac5cae7333c6700d9b737d09fae0f3f4d7fa [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar)
d3698b5ee309cf0f0cdfb286d6b30a256d7deae5 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar)
46578c03e92a55925308363ccdad04dcfc820d96 [doc] Describe different connection types (Amiti Uttarwar)
442abae2bac7bff85886143df01e14215532b974 [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar)
af59feb05235ecb85ec9d75b09c66e71268c9889 [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar)
e1bc29812ddf1d946bc5acca406a7ed2dca064a6 [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar)
0e52a659a2de915fc3dce37fc8fac39be1c8b6fa [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar)
1521c47438537e192230486dffcec0228a53878d [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar)
26304b4100201754fb32440bec3e3b78cd3f0e6d [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar)
3f1b7140e95d0f8f958cb35f31c3d964c57e484d 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 01e283068b9e6214f2d77a2f772a4244ebfe2274
laanwj:
Code review ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall
vasild:
ACK 01e283068
sdaftuar:
ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274.
fanquake:
ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274 - 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 01e283068b9e6214f2d77a2f772a4244ebfe2274
Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15
## Issue being fixed or feature implemented
Dead-code, useless conditions can be potential source of bug.
## What was done?
See each particular commit.
This particular commit "fix: check ptr in assert before usage" fixes
potential UB - `assert` is better than UB.
All other commits are not fixing any real issue, just to tidy-up code a
bit or to shut a potential warning.
## How Has This Been Tested?
Run unit/functional tests.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
Impossible to drop it completelly right now because:
- net doesn't know any details about chain - can't check status of fork
- the functional test feature_maxuploadtarget.py assume block size 1Mb
- DIP0001 can't be activated from regtest early block2 because big txes are
not allowed after DIP0001
refactor: drop global variable fDIP0001ActiveAtTip - attempt 2
105941b726c078642e785ecb7b6834ba814381b0 net: use stronger AddLocal() for our I2P address (Vasil Dimov)
Pull request description:
There are two issues:
### 1. Our I2P address not added to local addresses.
* `externalip=` is used with an IPv4 address (this sets automatically `discover=0`)
* No `discover=1` is used
* `i2psam=` is used
* No `externalip=` is used for our I2P address
* `listenonion=1 torcontrol=` are used
In this case `AddLocal(LOCAL_MANUAL)` [is used](94f83534e4/src/torcontrol.cpp (L354)) for our `.onion` address and `AddLocal(LOCAL_BIND)` [for our](94f83534e4/src/net.cpp (L2247)) `.b32.i2p` address, the latter being [ignored](94f83534e4/src/net.cpp (L232-L233)) due to `discover=0`.
### 2. Our I2P address removed from local addresses even if specified with `externalip=` on I2P proxy restart.
* `externalip=` is used with our I2P address (this sets automatically `discover=0`)
* No `discover=1` is used
* `i2psam=` is used
In this case, initially `externalip=` causes our I2P address to be [added](94f83534e4/src/init.cpp (L1266)) with `AddLocal(LOCAL_MANUAL)` which overrides `discover=0` and works as expected. However, if later the I2P proxy is shut down [we do](94f83534e4/src/net.cpp (L2234)) `RemoveLocal()` in order to stop advertising our I2P address (since we have lost I2P connectivity). When the I2P proxy is started and we reconnect to it, restoring the I2P connectivity, [we do](94f83534e4/src/net.cpp (L2247)) `AddLocal(LOCAL_BIND)` which does nothing due to `discover=0`.
To resolve those two issues, use `AddLocal(LOCAL_MANUAL)` for I2P which is also what we do with Tor.
ACKs for top commit:
laanwj:
Code review ACK 105941b726c078642e785ecb7b6834ba814381b0
Tree-SHA512: 0c9daf6116b8d9c34ad7e6e9bbff6e8106e94e4394a815d7ae19287aea22a8c7c4e093c8dd8c58a4a1b1412b2575a9b42b8a93672c8d17f11c24508c534506c7
36fb036d25e2a3016b36873456e5a9e6251ffef8 p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT (Jon Atack)
4e0d5788ba5771c81bc0ff2e6523cf9accddae46 test: add net permissions noban/download unit test coverage (Jon Atack)
dde69f20a01acca64ac21cb13993c6e4f8709f23 p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() (Jon Atack)
Pull request description:
This is a bugfix follow-up to #16248 and #19191 that was noticed in #21506. Both v0.21 and master are affected.
Since #19191, noban is a multi-flag that implies download, so the conditional in `CConnman::Bind()` using a bitwise AND on noban will return the same result for both the noban status and the download status. This means that download peers are incorrectly not being added to local addresses because they are mistakenly seen as noban peers.
The second commit adds unit test coverage to illustrate and test the noban/download relationship and the `NetPermissions` operations involving them.
The final commit adds documentation and disallows calling `NetPermissions::ClearFlag()` with any second param other than `NetPermissionFlags` "implicit" -- per current usage in the codebase -- because `ClearFlag()` should not be called with any second param that is a subflag of a multiflag, e.g. "relay" or "download," as that would leave the result in an invalid state corresponding to none of the existing NetPermissionFlags. Thanks to Vasil Dimov for noticing this.
ACKs for top commit:
theStack:
re-ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8 ☕
vasild:
ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8
hebasto:
ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8, I have reviewed the code and it looks OK, I agree it can be merged.
kallewoof:
Code review ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8
Tree-SHA512: 5fbc7ddbf31d06b35bf238f4d77ef311e6b6ef2e1bb9893f32f889c1a0f65774a3710dcb21d94317fe6166df9334a9f2d42630809e7fe8cbd797dd6f6fc49491
2a39ccf1334ef3c48c6f9969a0fc916b9e10aae1 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 2a39ccf1334ef3c48c6f9969a0fc916b9e10aae1
Tree-SHA512: fb5ac07d9cd5d006182b52857b289a9926362a2f1bfa4f7f1c78a088670e2ccf39ca28214781df82e8de3909fa3e69685fe1124a7e3ead758575839f5f2277a9
83da576f4416c64b5d520819208a722b2273739a net: use CMessageHeader::HEADER_SIZE, add missing include (Jon Atack)
Pull request description:
as suggested 16 months ago by Gleb Naumenko in https://github.com/bitcoin/bitcoin/pull/15197#issuecomment-456181865.
`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 in the file with it and adds the explicit include header.
Co-authored by: Gleb Naumenko <naumenko.gs@gmail.com>
ACKs for top commit:
naumenkogs:
utACK 83da576
practicalswift:
ACK 83da576f4416c64b5d520819208a722b2273739a -- patch looks correct
theStack:
ACK 83da576f4416c64b5d520819208a722b2273739a -- verified that its just magic number elimination refactoring and additionally checked that all tests pass 👍
Tree-SHA512: 5b915483bca4ea162c259865a1b615d73b88a1b1db3f82db05f770d10b8a42494d948f5b21badbcce2d9efa5915b8cbb6af83073867c23d2f152c0d35ac37b96
fa74e726c414f5f7a1e63126a69463491f66e0ec refactor: Make FEELER_SLEEP_WINDOW type safe (std::chrono) (MacroFake)
fa3b3cb9b5d944d34b1d5ac3e102ac333482a475 Expose underlying clock in CThreadInterrupt (MacroFake)
Pull request description:
This gets rid of the `value*1000` manual conversion.
ACKs for top commit:
naumenkogs:
utACK fa74e726c414f5f7a1e63126a69463491f66e0ec
dergoegge:
Code review ACK fa74e726c414f5f7a1e63126a69463491f66e0ec
Tree-SHA512: 90409c05c25f0dd2f1c4dead78f707ebfd78b7d84ea4db9fcefd9c4958a1a3338ac657cd9e99eb8b47d52d4485fa3c947dce4ee1559fb56ae65878685e1ed9a3
## Issue being fixed or feature implemented
`GetAdjustedTime()` can be manipulated by our peers, we should avoid
using it for our internal data structures/logic.
## What was done?
Use `GetTime<T>()` instead, fix some includes while at it.
## How Has This Been Tested?
run tests, run a node
## Breaking Changes
should be none
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
includes:
- 2a2182c387f607cd8284f33890bd285a81077b7f
- 77c507358bda9bd6c496f33e0f4418c0603bb08d
completion of partial merge done in dash#4164 as 56f1b2d (blocker
bitcoin#17938 was merged in dash#5246 as 00802bb)
## Motivation
As highlighted in https://github.com/dashpay/dash-issues/issues/52,
decoupling of `CFlatDB`-interacting components from managers of objects
like `CGovernanceManager` and `CSporkManager` is a key task for
achieving deglobalization of Dash-specific components.
The design of `CFlatDB` as a flat database agent relies on hooking into
the object's state its meant to load and store, using its
(de)serialization routines and other miscellaneous functions (notably,
without defining an interface) to achieve those ends. This approach was
taken predominantly for components that want a single-file cache.
Because of the method it uses to hook into the object (templates and the
use of temporary objects), it explicitly prevented passing arguments
into the object constructor, an explicit requirement for storing
references to other components during construction. This, in turn,
created an explicit dependency on those same components being available
in the global context, which would block the backport of bitcoin#21866,
a requirement for future backports meant to achieve parity in
`assumeutxo` support.
The design of these objects made no separation between persistent (i.e.
cached) and ephemeral (i.e. generated/fetched during initialization or
state transitions) data and the design of `CFlatDB` attempts to "clean"
the database by breaching this separation and attempting to access this
ephemeral data.
This might be acceptable if it is contained within the manager itself,
like `CSporkManager`'s `CheckAndRemove()` but is utterly unacceptable
when it relies on other managers (that, as a reminder, are only
accessible through the global state because of restrictions caused by
existing design), like `CGovernanceManager`'s `UpdateCachesAndClean()`.
This pull request aims to separate the `CFlatDB`-interacting portions of
these managers into a struct, with `CFlatDB` interacting only with this
struct, while the manager inherits the struct and manages
load/store/update of the database through the `CFlatDB` instance
initialized within its scope, though the instance only has knowledge of
what is exposed through the limited parent struct.
## Additional information
* As regards to existing behaviour, `CFlatDB` is written entirely as a
header as it relies on templates to specialize itself for the object it
hooks into. Attempting to split the logic and function definitions into
separate files will require you to explicitly define template
specializations, which is tedious.
* `m_db` is defined as a pointer as you cannot instantiate a
forward-declared template (see [this Stack Overflow
answer](https://stackoverflow.com/a/12797282) for more information),
which is done when defined as a member in the object scope.
* The conditional cache flush predicating on RPC _not_ being in the
warm-up state has been replaced with unconditional flushing of the
database on object destruction (@UdjinM6, is this acceptable?)
## TODOs
This is a list of things that aren't within the scope of this pull
request but should be addressed in subsequent pull requests
* [ ] Definition of an interface that `CFlatDB` stores are expected to
implement
* [ ] Lock annotations for all potential uses of members protected by
the `cs` mutex in each manager object and store
* [ ] Additional comments documenting what each function and member does
* [ ] Deglobalization of affected managers
---------
Co-authored-by: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com>
31b136e5802e1b1e5f9a9589736afe0652f34da2 Don't declare de facto const reference variables as non-const (practicalswift)
1c65c075ee4c7f98d9c1fac5ed7576b96374d4e9 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 31b136e5802e1b1e5f9a9589736afe0652f34da2
jonatack:
ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2
theStack:
ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2 ❄️
Tree-SHA512: f58f8f00744219426874379e9f3e9331132b9b48e954d24f3a85cbb858fdcc98009ed42ef7e7b4619ae8af9fc240a6d8bfc1c438db2e97b0ecd722a80dcfeffe
378aedc45248cea82d9a3e6dc1038d6828008a76 [net] Add cs_vSend lock annotations (John Newbery)
673254515a2f97e53dd8c7335c836b083ba7e31a [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 378aedc45248cea82d9a3e6dc1038d6828008a76
theStack:
re-ACK 378aedc45248cea82d9a3e6dc1038d6828008a76
MarcoFalke:
review ACK 378aedc45248cea82d9a3e6dc1038d6828008a76 🔌
Tree-SHA512: e9cd6c472b7e1479120c1bf2d1c640cf6d18c7d589a5f9b7dfc4875e5790adaab403a7a1b945a47e79e7249a614b8583270e4549f89b22e8a9edb2e4818b0d07
3e68efa615968e0c9d68a7f197c7852478f6be78 [net] Move checks from GetLocalAddrForPeer to caller (John Newbery)
d21d2b264cd77c027a06f68289cf4c3f177d1ed0 [net] Change AdvertiseLocal to GetLocalAddrForPeer (John Newbery)
Pull request description:
This is the first part of #21186. It slightly disentangles addr handling in net/net_processing by making it explicit that net_processing is responsible for pushing addr records into `vAddrToSend`.
ACKs for top commit:
MarcoFalke:
re-ACK 3e68efa615968e0c9d68a7f197c7852478f6be78 🍅
Tree-SHA512: 9af50c41f5a977e2e277f24a589db38e2980b353401def5e74b108ac5f493d9b5d6b1b8bf15323a4d66321495f04bc271450fcef7aa7d1c095f051a4f8e9b15f
010eed3ce03cf4fc622a48f40fc4d589383f7a44 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 010eed3ce03cf4fc622a48f40fc4d589383f7a44
Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
fabecce71909c984504c21fa05f91d5f1b471e8c net: Treat raw message bytes as uint8_t (MarcoFalke)
Pull request description:
Using `uint8_t` from the beginning when messages are `recv`ed has two style benefits:
* The signedness is clear from reading the code, as it does not depend on the architecture
* When passing the bytes on, the need for static signedness casts is dropped, making the code a bit less verbose and more coherent
ACKs for top commit:
laanwj:
Code review ACK fabecce71909c984504c21fa05f91d5f1b471e8c
theStack:
Code Review ACK fabecce71909c984504c21fa05f91d5f1b471e8c
jonatack:
Tested ACK fabecce71909c984504c21fa05f91d5f1b471e8c
Tree-SHA512: e6d9803c78633fde3304faf592afa961ff9462a7912d1da97a24720265274aa10ab4168d71b6ec2756b7448dd42585321afee0e5c889e705be778ce9a330d145
fa5ed3b4ca609426b2622cad235e107d33db7b30 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 fa5ed3b4ca609426b2622cad235e107d33db7b30 code review, rebased to current master 12a1c3ad1a43634, debug build, unit tests, ran bitcoind/-netinfo/getpeerinfo
theStack:
ACK fa5ed3b4ca609426b2622cad235e107d33db7b30
Tree-SHA512: 89bf111323148d6e6e50185ad20ab39f73ab3a58a27e46319e3a08bcf5dcf9d6aa84faff0fd6afb90cb892ac2f557a237c144560986063bc736a69ace353ab9d
9d09132be4ff99f98ca905c342347d5f35f13350 CConnman: initialise at declaration rather than in Start() (Anthony Towns)
Pull request description:
Ensure nMaxOutboundTotalBytesSentInCycle and nMaxOutboundCycleStartTime are initialized even if CConnman::Start() is not called. Prevents failures in test/fuzz/connman when run under valgrind.
ACKs for top commit:
practicalswift:
ACK 9d09132be4ff99f98ca905c342347d5f35f13350: patch looks correct!
MarcoFalke:
review ACK 9d09132be4ff99f98ca905c342347d5f35f13350 , checked that we call Start only once and in the same scope where connman is constructed (AppInitMain) 💸
jnewbery:
Code review ACK 9d09132be4
Tree-SHA512: 1c6c893e8c616a91947a8cc295b0ba508af3ecfcdcd94cdc5f95d808cc93c6d1a71fd24dcc194dc583854e9889fb522ca8523043367fb0263370fbcab08c6aaa
2f97c1180b7eba2c48683a553ca166eba84e14c6 doc: Remove TODO 'exclude peers with download permission' (Douglas Chimento)
Pull request description:
Following from PR https://github.com/bitcoin/bitcoin/pull/23109
The [TODO](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L2872) is no longer necessary.
Removing it to prevent future confusion.
Top commit has no ACKs.
Tree-SHA512: c2f4c3eae951d13d623e1b4bd9315804ec33473e501367f89edae80fa446674edc71549e145f058fe7126b8588790dc895d6ea3dfb4347ceeca61f5e5f2f95cc
5e7e4c9f6e57f5333bd17a20b0c85a78d032998e refactor: replace RecursiveMutex g_maplocalhost_mutex with Mutex (w0xlt)
a7da1409bc9f614009f76c1bfc55f029ff1265e4 scripted-diff: rename cs_mapLocalHost -> g_maplocalhost_mutex (w0xlt)
Pull request description:
This PR is related to #19303 and gets rid of the `RecursiveMutex cs_mapLocalHost`.
ACKs for top commit:
shaavan:
ACK 5e7e4c9f6e57f5333bd17a20b0c85a78d032998e
theStack:
ACK 5e7e4c9f6e57f5333bd17a20b0c85a78d032998e
hebasto:
ACK 5e7e4c9f6e57f5333bd17a20b0c85a78d032998e, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 961171e346fe385e16db9830115a8096f4ca2499bbea11a08c02ca808638dfb63c434ab9d66392c71e85be6352c8a2b6a0054b5a61aaabd28d71581fed5beae7
e09c701e0110350f78366fb837308c086b6503c0 scripted-diff: Bump copyright of files changed in 2020 (MarcoFalke)
6cbe6209646db8914b87bf6edbc18c6031a16f1e scripted-diff: Replace CCriticalSection with RecursiveMutex (MarcoFalke)
Pull request description:
`RecursiveMutex` better clarifies that the mutex is recursive, see also the standard library naming: https://en.cppreference.com/w/cpp/thread/recursive_mutex
For that reason, and to avoid different people asking me the same question repeatedly (e.g. https://github.com/bitcoin/bitcoin/pull/15932#pullrequestreview-339175124 ), remove the outdated alias `CCriticalSection` with a scripted-diff
## Issue being fixed or feature implemented
This refactoring is a follow-up changes to backport bitcoin#17164 (PR
#5314)
These changes are reduce difference in implementation for our code and
bitcoin's
## What was done?
Removed a flag m_block_relay_peer. Instead I call IsAddrRelayPeer() that
has same information now.
It changes logic introduced in #4888 due to dash-specific code.
## How Has This Been Tested?
Run unit/functional tests.
## Breaking Changes
No breaking changes
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
b6d2183858975abc961207c125c15791e531edcc Minor refactoring to remove implied m_addr_relay_peer. (User)
a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2 added asserts to check m_addr_known when it's used (User)
090b75c14be6b9ba2efe38a17d141c6e6af575cb 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
