65332b1178c75e1f83415bad24918996a1524866 [addrman] Remove RemoveInvalid() (John Newbery)
Pull request description:
PRs #22179 and #22112 (EDIT: later reverted in #22497) added hotfix code to addrman to remove invalid addresses and mutate the ports of I2P entries after entering into addrman. Those hotfixes included at least two addrman data corruption bugs:
- #22467 (Assertion `nRndPos1 < vRandom.size() && nRndPos2 < vRandom.size()' failed)
- #22470 (Changing I2P ports in addrman may wronly skip some entries from "new" buckets)
Hotfixing addrman is inherently dangerous. There are many members that have implicit assumptions on each others' state, and mutating those directly can lead to violating addrman's internal invariants.
Instead of trying to hotfix addrman, just don't insert any invalid addresses. For now, those are addresses which fail `CNetAddr::IsValid()`.
ACKs for top commit:
sipa:
utACK 65332b1178c75e1f83415bad24918996a1524866. I tried to reason through scenarios that could introduce inconsistencies with this code, but can't find any.
fanquake:
ACK 65332b1178c75e1f83415bad24918996a1524866 - Skipping the addition of invalid addresses (this code was initially added for Tor addrs) rather than adding all the invalids then removing them all when finishing unserializing seems like an improvement. Especially if it can be achieved with less code.
Tree-SHA512: 023113764cb475572f15da7bf9824b62b79e10a7e359af2eee59017df354348d2aeed88de0fd4ad7a9f89a0dad10827f99d70af6f1cb20abb0eca2714689c8d7
87651795d8622d354f8e3c481eb868d9433b841c fuzz: check that ser+unser produces the same AddrMan (Vasil Dimov)
6408b24517f3418e2a408071b4c2ce26571f3167 fuzz: move init code to the CAddrManDeterministic constructor (Vasil Dimov)
Pull request description:
Add a fuzz test that fills addrman with a pile of randomly generated addresses, serializes it to a stream, unserializes the stream to another addrman object and compares the two.
Some discussion of this already happened at https://github.com/jnewbery/bitcoin/pull/18.
ACKs for top commit:
practicalswift:
cr ACK 87651795d8622d354f8e3c481eb868d9433b841c
jonatack:
ACK 87651795d8622d354f8e3c481eb868d9433b841c rebased to current master, reviewed, fuzz build, ran `FUZZ=addrman_serdeser src/test/fuzz/fuzz`
Tree-SHA512: 7eda79279f14f2649840bf752e575d7b02cbaad541f74f7254855ebd4a32da988f042d78aa9228983350283bb74dd0c71f51f04c0846889c3ba2f19f01a0c303
a92485b2c250fd18f55d22aa32722bf52ab32bfe addrman: use unordered_map instead of map (Vasil Dimov)
Pull request description:
`CAddrMan` uses `std::map` internally even though it does not require
that the map's elements are sorted. `std::map`'s access time is
`O(log(map size))`. `std::unordered_map` is more suitable as it has a
`O(1)` access time.
This patch lowers the execution times of `CAddrMan`'s methods as follows
(as per `src/bench/addrman.cpp`):
```
AddrMan::Add(): -3.5%
AddrMan::GetAddr(): -76%
AddrMan::Good(): -0.38%
AddrMan::Select(): -45%
```
ACKs for top commit:
jonatack:
ACK a92485b2c250fd18f55d22aa32722bf52ab32bfe
achow101:
ACK a92485b2c250fd18f55d22aa32722bf52ab32bfe
hebasto:
re-ACK a92485b2c250fd18f55d22aa32722bf52ab32bfe, only suggested changes and rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/18722#pullrequestreview-666663681) review.
Tree-SHA512: d82959a00e6bd68a6c4c5a265dd08849e6602ac3231293b7a3a3b7bf82ab1d3ba77f8ca682919c15c5d601b13e468b8836fcf19595248116635f7a50d02ed603
f036dfbb692c4d44d0f59194d089ed0aa1096347 [addrman] Remove unused test_before_evict argument from Good() (John Newbery)
Pull request description:
This has never been used in the public interface method since it was
introduced in #9037.
ACKs for top commit:
lsilva01:
Tested ACK f036dfbb69 on Ubuntu 20.04.
theStack:
Code-review ACK f036dfbb692c4d44d0f59194d089ed0aa1096347
Tree-SHA512: 98145d9596b4ae1f354cfa561be1a54c6b8057c920e0ac3d4c1d42c9326b2dad2d44320f4171bb701d97088b216760cca8017b84c8b5dcd2b1dc8f158f28066d
fae108ceb53f61d7338ba205873623ede3c1d3be Fix incorrect whitespace in addrman (MarcoFalke)
fa32024d51c098441623e246f304a80f011e29d1 Add missing GUARDED_BY to CAddrMan::insecure_rand (MarcoFalke)
fab755b77f88873f01cbd988051de7ad3f0150de fuzz: Actually use const addrman (MarcoFalke)
fae0c79351ce34186249d44af0c5c9c7521f4b6c refactor: Mark CAddrMan::GetAddr const (MarcoFalke)
fa02934c8c9d290ea4d12683e8680c70967a4d3a refactor: Mark CAddrMan::Select const (MarcoFalke)
Pull request description:
To clarify that a call to this only changes the random state and nothing else.
ACKs for top commit:
jnewbery:
Code review ACK fae108ceb53f61d7338ba205873623ede3c1d3be
theStack:
re-ACK fae108ceb53f61d7338ba205873623ede3c1d3be 🍦
Tree-SHA512: 3ffb211d4715cc3daeb3bfcdb3fcc6b108ca96df5fa565510436fac0e8da86c93b30c9c4aad0563e27d84f615fcd729481072009a4e2360c8b3d40787ab6506a
## Issue being fixed or feature implemented
Some headers include other heavy headers, such as `logging.h`,
`tinyformat.h`, `iostream`. These headers are heavy and increase
compilation time on scale of whole project drastically because can be
used in many other headers.
## What was done?
Moved many heavy includes from headers to cpp files to optimize
compilation time.
In some places added forward declarations if it is reasonable.
As side effect removed 2 circular dependencies:
```
"llmq/debug -> llmq/dkgsessionhandler -> llmq/debug"
"llmq/debug -> llmq/dkgsessionhandler -> llmq/dkgsession -> llmq/debug"
```
## How Has This Been Tested?
Run build 2 times before refactoring and after refactoring: `make clean
&& sleep 10s; time make -j18`
Before refactoring:
```
real 5m37,826s
user 77m12,075s
sys 6m20,547s
real 5m32,626s
user 76m51,143s
sys 6m24,511s
```
After refactoring:
```
real 5m18,509s
user 73m32,133s
sys 6m21,590s
real 5m14,466s
user 73m20,942s
sys 6m17,868s
```
~5% of improvement for compilation time. That's not huge, but that's
worth to get merged
There're several more refactorings TODO but better to do them later by
backports:
- bitcoin/bitcoin#27636
- bitcoin/bitcoin#26286
- bitcoin/bitcoin#27238
- and maybe this one: bitcoin/bitcoin#28200
## 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
ae98aec9c0521cdcec76459c8200bd45ff6a1485 refactor: Make CAddrMan::cs non-recursive (Hennadii Stepanov)
f5d1c7fac70f424114dae3be270fdc31589a8c34 Add AssertLockHeld to CAddrMan private functions (Hennadii Stepanov)
5ef1d0b6982f05f70ff2164ab9af1ac1d2f97f5d Add thread safety annotations to CAddrMan public functions (Hennadii Stepanov)
b138973a8b4bbe061ad97011f278a21e08ea79e6 refactor: Avoid recursive locking in CAddrMan::Clear (Hennadii Stepanov)
f79a664314b88941c1a2796623e846d0a5916c06 refactor: Apply consistent pattern for CAddrMan::Check usage (Hennadii Stepanov)
187b7d2bb36e6de9cd960378021ebe690619a2ef refactor: Avoid recursive locking in CAddrMan::Check (Hennadii Stepanov)
f77d9c79aa41dab4285e95c9432cc6d853be67a3 refactor: Fix CAddrMan::Check style (Hennadii Stepanov)
06703973c758c2c5d0ff916993aa7055f609d2d7 Make CAddrMan::Check private (Hennadii Stepanov)
efc6fac951e75ba913350bb470c3d4e6a4e284b9 refactor: Avoid recursive locking in CAddrMan::size (Hennadii Stepanov)
2da95545ea42f925dbc7703e42e9356908a8c83e test: Drop excessive locking in CAddrManTest::SimConnFail (Hennadii Stepanov)
Pull request description:
This PR replaces `RecursiveMutex CAddrMan::cs` with `Mutex CAddrMan::cs`.
All of the related code branches are covered by appropriate lock assertions to insure that the mutex locking policy has not been changed by accident.
Related to #19303.
Based on #22025, and first three commits belong to it.
ACKs for top commit:
vasild:
ACK ae98aec9c0521cdcec76459c8200bd45ff6a1485
Tree-SHA512: c3a2d3d955a75befd7e497a802b8c10730e393be9111ca263ad0464d32fae6c7edf9bd173ffb6bc9bb61c4b39073a74eba12979d47f26b0b7b4a861d100942df
036d7eadf5dd0e06e0734a5d78dbe28f4bfaa07f doc: Correct description of CAddrMan::Create() (Amiti Uttarwar)
318176aff1ded36d1fbc5977f288ac3bac1d8712 doc: Update high-level addrman description (Martin Zumsande)
Pull request description:
The high-level description of `addrman` has outdated information with respect to the eviction behavior, both for the New and Tried tables (at least since #5941) - this has confused me in the past.
This PR corrects this and also adds basic info about the bucket size and position.
ACKs for top commit:
amitiuttarwar:
reACK 036d7eadf5dd0e06e0734a5d78dbe28f4bfaa07f
jnewbery:
ACK 036d7eadf5dd0e06e0734a5d78dbe28f4bfaa07f
Tree-SHA512: 3f0635d765f5e580a1fae31187742a833cef66ef2286d40eeb28f2253521260038e16e5f1a65741464a2ddfdbeb5c0f1bc38bf73841e600639033d59c3c534e4
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
816f29eab296ebec2da8f8606ad618609e3ba228 addrman: detect on-disk corrupted nNew and nTried during unserialization (Vasil Dimov)
Pull request description:
Negative `nNew` or `nTried` are not possible during normal operation.
So, if we read such values during unserialize, report addrman
corruption.
Fixes https://github.com/bitcoin/bitcoin/issues/22450
ACKs for top commit:
MarcoFalke:
cr ACK 816f29eab296ebec2da8f8606ad618609e3ba228
jonatack:
ACK 816f29eab296ebec2da8f8606ad618609e3ba228
lsilva01:
Code Review ACK 816f29eab2. This change provides a more accurate description of the error.
Tree-SHA512: 01bdd72d2d86a0ef770a319fee995fd1e147b24a8db84ddb8cd121688e7f94fed73fddc0084758e7183c4f8d08e971f0b1b224f5adb10928a5aa4dbbc8709d74
fa2510d5c1cdf9c2cd5cc9887302ced4378c7202 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
e414486d56b9f06af7aeb07ce13e3c3780c2b69b Do not permit copying FastRandomContexts (Pieter Wuille)
022cf47dd7ef8f46e32a184e84f94d1e9f3a495c Simplify testing RNG code (Pieter Wuille)
fd3e7973ffaaa15ed32e5aeadcb02956849b8fc7 Make unit tests use the insecure_rand_ctx exclusively (Pieter Wuille)
8d98d426116f0178612f14d1874d331042c4c4b7 Bugfix: randbytes should seed when needed (non reachable issue) (Pieter Wuille)
273d02580aa736b7ccea8fce51d90541665fdbd1 Use a FastRandomContext in LimitOrphanTxSize (Pieter Wuille)
3db746beb407f7cdd9cd6a605a195bef1254b4c0 Introduce a Shuffle for FastRandomContext and use it in wallet and coinselection (Pieter Wuille)
8098379be5465f598220e1d6174fc57c56f9da42 Use a local FastRandomContext in a few more places in net (Pieter Wuille)
9695f31d7544778853aa373f0aeed629fa68d85e Make addrman use its local RNG exclusively (Pieter Wuille)
Pull request description:
This improves a few minor issues with the RNG code:
* Avoid calling `GetRand*()` functions (which currently invoke OpenSSL, later may switch to using our own RNG pool) inside loops in addrman, networking code, `KnapsackSolver`, and `LimitOrphanSize`
* Fix a currently unreachable bug in `FastRandomContext::randbytes`.
* Make a number of simplifications to the unit tests' randomness code (some tests unnecessarily used their own RNG or the OpenSSL one, instead of using the unit test specific `insecure_rand_ctx`).
* As a precaution, make it illegal to copy a `FastRandomContext`.
Tree-SHA512: 084c70b533ea68ca7adc0186c39f0b3e0a5c0ae43a12c37286e5d42086e056a8cd026dde61b12c0a296dc80f87fdc87fe303b9e8e6161b460ac2086cf7615f9d
4676a4fb5be0f6ef0b3f71c1f4361c20f7cb0e0b [addrman] Don't repeat "Bucketing method was updated" log multiple times (John Newbery)
436292367c1d737cf73bd985293539500d1206f5 [addrman] Improve serialization comments (John Newbery)
ac3547eddd8a7d67b4103508f30d5d02a9c1f148 [addrman] Improve variable naming/code style of touched code. (John Newbery)
a5c9b04959f443372400f9a736c6eaf5502284a1 [addrman] Don't rebucket new table entries unnecessarily (John Newbery)
8062d928ce5c495c1b6ecd18e4b30c12da822d90 [addrman] Rename asmap version to asmap checksum (John Newbery)
009b8e0fdf3bfb11668edacced5d8b70726d5d0e [addrman] Improve variable naming/code style of touched code. (John Newbery)
b4c5fda417dd9ff8bf9fe24a87d384a649e3730d [addrman] Fix new table bucketing during unserialization (John Newbery)
Pull request description:
This fixes three issues in addrman unserialization.
1. An addrman entry can appear in up to 8 new table buckets. We store this entry->bucket indexing during shutdown so that on restart we can restore the entries to their correct buckets. Commit ec45646de9e62b3d42c85716bfeb06d8f2b507dc broke the deserialization code so that each entry could only be put in up to one new bucket.
2. Unserialization may result in an entry appearing in a 9th bucket. If the entry already appears in 8 buckets don't try to place it in another bucket.
3. We unnecessarily rebucket when reading a peers.dat with file version 1. Don't do that.
ACKs for top commit:
vasild:
ACK 4676a4fb5be0f6ef0b3f71c1f4361c20f7cb0e0b
glozow:
re-ACK 4676a4fb5b, changes were a rename, comments, and removing repeat-logging.
naumenkogs:
ACK 4676a4f
laanwj:
Code review ACK 4676a4fb5be0f6ef0b3f71c1f4361c20f7cb0e0b
dhruv:
ACK 4676a4fb5be0f6ef0b3f71c1f4361c20f7cb0e0b
ryanofsky:
Code review ACK 4676a4fb5be0f6ef0b3f71c1f4361c20f7cb0e0b. I'm not previously familiar with this code but all the changes here do make sense and seem like improvements. Left some notes and comments, but they aren't important so feel to ignore.
Tree-SHA512: b228984f6dec5910be23c3740ae20258da33bcf66ceb7edb10e5a53163450f743bab349e47f09808b7e8d40f27143119ec3e0981d7e678aa494d8559a1c99c23
20e6ea259b222b10f066f22695a5f56c52071f63 [addrman] Improve collision logging and address nits (Suhas Daftuar)
f71fdda3bc2e7acd2a8b74e882364866b8b0f55b [addrman] Ensure collisions eventually get resolved (Suhas Daftuar)
4991e3c813c9848d3b3957ea3ad433f02fca9e81 [net] feeler connections can be made to outbound peers in same netgroup (Suhas Daftuar)
4d834018e368c3481a5421891395f64aa9002185 [addrman] Improve tried table collision logging (Suhas Daftuar)
Pull request description:
The restriction on outbound peers sharing the same network group is not intended to apply to feeler connections, so fix this.
This fixes an issue where a tried table collision with an entry to a netgroup we already have an outbound connection to could cause feelers to stop working, because the tried collision buffer (`m_tried_collisions`) would never be drained.
Also, ensure that all entries don't linger in `m_tried_collisions` by evicting an old entry if its collisions is unresolved after 40 minutes.
Tree-SHA512: 553fe2b01b82cd7f0f62f90c6781e373455a45b254e3bec085b5e6b16690aa9f3938e8c50e7136f19dafa250ed4578a26227d944b76daf9ce4ef0c75802389b6
