ea4c9fd4ab test: Cover eviction by timeout in addrman_evictionworks (Martin Zumsande)
4f1bb467b5 test: Add test for multiplicity in addrman new tables (Martin Zumsande)
e880bb7836 test: Add test for updating addrman entries (Martin Zumsande)
f02eee8c87 test: introduce utility function to retrieve an addrman (Martin Zumsande)
f0e5efb824 test: Remove unused AddrManTest class (Martin Zumsande)
b696d7870b test: Remove tests for internal helper functions (Martin Zumsande)
0538520091 test: use AddrMan instead of AddrManTest where possible (Martin Zumsande)
1c65d427bb test: Inline SimConnFail function (Martin Zumsande)
5b7aac34f2 test: delete unused GetBucketAndEntry function (Amiti Uttarwar)
2ba1e74e59 test: Update addrman_serialization unit test to use AddrMan's interface (Amiti Uttarwar)
dad5f76021 addrman: Introduce a test-only function to lookup addresses (Amiti Uttarwar)
Pull request description:
This PR (joint work with Amiti Uttarwar) changes the addrman unit tests such that they only use the public `AddrMan` interface:
This has the advantage that the tests are less implementation-dependent, i.e. it would be possible to rewrite the internal addrman implementation (as drafted [here](https://github.com/sipa/bitcoin/tree/202106_multiindex_addrman) for using a multiindex) without having to adjust the tests.
This includes the following steps:
* Adding a test-only function `FindAddressEntry()` to the public addrman interface which returns info about an address in addrman (e.g. bucket, position, whethe the address is in new or tried). Obviously we want to do this sparingly, but I think a single test-only function is ok (which could also be useful elsewhere, e.g. in fuzz tests).
* Removal of the `AddrManTest` subclass which would reach into AddrMan's internals, using `AddrMan` instead
* Removal of tests for internal helper functions that are not publicly exposed (these are still tested indirectly via the public functions calling them).
* Additional tests for previously untested features such as multiplicity in the new tables, that can be tested with the help of `FindAddressEntry()`.
All in all, this PR increases the unit test coverage of AddrMan by a bit.
ACKs for top commit:
jnewbery:
ACK ea4c9fd4ab
josibake:
reACK ea4c9fd4ab
Tree-SHA512: c2d4ec8bdc62ffd6055ddcd37dea85ec08c76889e9e417e8d7c62a96cf68a8bcbe8c67bec3344d91fa7d3c499f6d9f810962da1dddd38e70966186b10b8ab447
The logic of these functions is already covered by existing unit tests
using publicly exposed functions of the interface.
Therefore, removing them does not decrease test coverage.
Switches to AddrMan for tests that use no features of AddrManTest.
Also removes unusued AddrManTest variables
Co-Authored-By: Amiti Uttarwar <amiti@uttarwar.org>
826e12b010 test: call VerifyLoadedChainstate during ChainTestingSetup (James O'Beirne)
Pull request description:
for additional coverage and similarity to actual init process.
Followup to #23280.
ACKs for top commit:
dongcarl:
Code Review ACK 826e12b010
ryanofsky:
Code review ACK 826e12b010
Tree-SHA512: a4e7fd25e5d7a08b1e154ae6daf67c3048260a2684b0e569b544dd826693b7b969db9923b191e499cb8d8d0a2a73eb9330ff45909313145a9abb6052eb8c3ad9
bf4f817135 refactor: addrman_select test (josibake)
5a64dc018c refactor: addrman_evictionworks test (josibake)
e281fccd8a refactor: addrman_noevict test (josibake)
8bdd9240d4 refactor: addrman_selecttriedcollisions test (josibake)
Pull request description:
As a follow-up to #23713 , this PR refactors the remaining tests in `src/tests/addrman_tests.cpp` to use the output from `AddrMan::Good()` where appropriate.
ACKs for top commit:
naumenkogs:
ACK bf4f817135
mzumsande:
Code Review ACK bf4f817135
Tree-SHA512: 93cc127aecff42c1c174daa04911af7e3460a5c40ddf96952fe4a6ab86fa1ff22d66724326abb709008d7f9f79c26c55c6d62753c40059c9ac60f869507ec913
c44c20108f p2p, refactor: drop unused DNSLookupFn param in LookupSubnet() (Vasil Dimov)
f0c9e68080 p2p, refactor: tidy up LookupSubNet() (Jon Atack)
Pull request description:
This pull originally resolved a code `TO-DO`, as well as fixing different param names between the function declaration and definition, updating the function to current style standards, clearer variable naming, and improving the Doxygen documentation.
Following the merge of #17160, it now does the non-`TODO` changes and also now drops an unused param to simplify the function.
ACKs for top commit:
dunxen:
ACK c44c201
vasild:
ACK c44c20108f
shaavan:
crACK c44c20108f
Tree-SHA512: 55f64c7f403819dec84f4da06e63db50f7c0601a2d9a1ec196fda667c220ec6f5ad2a3c95e0e02275da9f6da6b984275d1dc10e19ed82005c5e13da5c5ecab02
edd0313ae7 test: Improve "invalid_command" subtest in system_tests for Windows (Hennadii Stepanov)
fb1b0590af test: Fix "non-zero exit code" subtest in system_tests for Windows (Hennadii Stepanov)
0aad33db64 test: Fix "false" subtest in system_tests for Windows (Hennadii Stepanov)
507c009c1e test: Fix "echo" subtest in the system_tests for Windows (Hennadii Stepanov)
Pull request description:
An attempt to fix bitcoin/bitcoin#23775.
With this PR on Windows 10 Pro 21H1 (build 19043.1348):
```
C:\Users\hebasto\bitcoin>src\test_bitcoin.exe --run_test=system_tests/run_command
Running 1 test case...
*** No errors detected
C:\Users\hebasto\bitcoin>src\test_bitcoin.exe
Running 482 test cases...
*** No errors detected
```
ACKs for top commit:
sipsorcery:
tACK edd0313ae7
Tru3Nrg:
tACK edd0313ae7
Tree-SHA512: 66a4f2372858011ff862b71c6530bedb8bc731b18595636fac9affc9189d9320f212c68b62498f2b57ee7a07f59e842dbec085b76a7419791d1a06c8e80e7744
4d0ac72f3a [fuzz] Add fuzzing harness to compare both implementations of ChaCha20 (stratospher)
65ef93203c [fuzz] Add D. J. Bernstein's implementation of ChaCha20 (stratospher)
Pull request description:
This PR compares Bitcoin Core's implementation of ChaCha20 with D. J. Bernstein's in order to find implementation discrepancies if any.
ACKs for top commit:
laanwj:
Code review ACK 4d0ac72f3a
Tree-SHA512: f826144b4db61b9cbdd7efaaca8fa9cbb899953065bc8a26820a566303b2ab6a17431e7c114635789f0a63fbe3b65cb0bf2ab85baf882803a5ee172af4881544
Test for collisions and duplicates directly with `Good()`.
If an entry to tried is a duplicate, `Good()` will return false
but `SelectTriedCollision()` will be empty (assuming there were no prior
collisions). If there is a collision, `Good()` will retun false
and `SelectTriedCollision()` will return a value.
Check the response from `Good()` wherever it is called.
Previously, the test was using `size()` (incorrect for checking tried)
and `SelectTriedCollision()` to determine if a collision happened.
Check `Good()` directly when adding addresses.
Previously, test would check `size()`, which is incorrect.
Check that duplicates are also handled by checking the
output from `SelectTriedCollision()` when `Good()` returns
false.
fad943821e scripted-diff: Rename touched member variables (MarcoFalke)
fa663a4c0d Use mockable time for peer connection time (MarcoFalke)
fad7ead146 refactor: Use type-safe std::chrono in net (MarcoFalke)
Pull request description:
Benefits:
* Type-safe
* Mockable
* Allows to revert a temporary test workaround
ACKs for top commit:
naumenkogs:
ACK fad943821e
shaavan:
ACK fad943821e
Tree-SHA512: af9bdfc695ab727b100c6810a7289d29b02b0ea9fa4fee9cc1f3eeefb52c8c465ea2734bae0c1c63b3b0d6264ba2c493268bc970ef6916570eb166de77829d82
faa6c3d44c net: Drop only invalid entries when reading banlist.json (MarcoFalke)
Pull request description:
All entries will be dropped when there is at least one invalid one in `banlist.json`. Fix this by only dropping invalid ones.
Also suggested in https://github.com/bitcoin/bitcoin/pull/20966#issuecomment-861150204
ACKs for top commit:
laanwj:
Re-ACK faa6c3d44c
Tree-SHA512: 5a58e7f1dcabf78d0c65d8c6d5d997063af1efeaa50ca7730fc00056fda7e0061b6f7a38907ea045fe667c9f61d392e01e556b425a95e6b126e3c41cd33deb83
2283b9cd1e test: add tests for LoadBlockIndex when using multiple chainstates (James O'Beirne)
0fd599a51a validation: have LoadBlockIndex account for snapshot use (James O'Beirne)
d0c6e61f5d validation: don't modify genesis during snapshot load (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Currently, `BlockManager::LoadBlockIndex` adds all blocks that have downloaded transactions to the active chain state's `setBlockIndexCandidates` set, ignoring the background chain state.
This PR changes ChainstateManager::LoadBlockIndex to update `setBlockIndexCandidates` in the background chain, not just the active chain. In the active chain, the same blocks are added as before. In the background chain, only blocks that have actually been validated, not blocks marked assumed-valid are added so the background chain will continue to download and validate assumed-valid blocks.
ACKs for top commit:
MarcoFalke:
Concept ACK 2283b9cd1e🤽
Sjors:
utACK 2283b9cd1e
Tree-SHA512: 7c9a80802df4722d85d12b78d2e7f628ac5f11cb8be66913d5c3230339bd1220c6723805509d4460826a17d1dc04b0ae172eb7d09ac0ea5dc5e41d77975cbd5e
caac999ff0 refactor: remove dependence on AddrManTest (josibake)
f961c477b5 refactor: check Good() in tried_collisions test (josibake)
207f1c825c refactor: make AddrMan::Good return bool (josibake)
Pull request description:
Previously, the `addrman_tried_collisions` test behaved in the following way:
1. add an address to addrman
2. attempt to move the new address to the tried table (using `AddrMan.Good()`)
3. verify that `num_addrs` matched `size()` to check for collisions in the new table
`AddrMan.size()`, however, returns the number of unique address in addrman, regardless of whether they are in new or tried. This means the test would still pass for addresses where a collision did occur in the tried table. After 3 collisions in the tried table, there would eventually be a collision in the new table when trying to add a new address, which was then detected by checking `num_addrs - collisions == size()`.
While the collision in the new table was caused by a collision in the tried table, the test is misleading as it's not directly testing for collisions in the tried table and misses 3 collisions before identifying a collision in the new table.
### solution
To more directly test the tried table, I refactored `AddrMan::Good()` to return a boolean after successfully adding an address to the tried table. This makes the test much cleaner by first adding an address to new, calling `Good` to move it to the tried table, and checking if it was successful or not. It is worth noting there are other reasons, aside from collisions, which will cause `Good` to return false. That being said, this is an improvement over the previous testing methodology.
Additionally, having `Good()` return a boolean is useful outside of testing as it allows the caller to handle the case where `Good` is unable to move the entry to the tried table (e.g a063647413/src/rpc/net.cpp (L945)).
### followup
As a follow up to this PR, I plan to look at the following places `Good()` is called and see if it makes sense to handle the case where it is unable to add an entry to tried:
* a063647413/src/rpc/net.cpp (L945)
* a063647413/src/net.cpp (L2067)
* a063647413/src/net_processing.cpp (L2708)
ACKs for top commit:
jnewbery:
utACK caac999ff0
mzumsande:
Code review ACK caac999ff0
Tree-SHA512: f328896b1f095e8d2581fcdbddce46fc0491731a0440c6fff01081fa5696cfb896dbbe1d183eda2c100f19aa111e1f8b096ef93582197edc6b791de563a58f99
Rather than try to infer a collision by checking `AddrMan::size`,
check whether or not moving to the tried table was successful by
checking the output from `AddrMan::Good`
fa19bab90a fuzz: Rework FillNode (MarcoFalke)
fae6e31df7 refactor: Set fSuccessfullyConnected in FillNode (MarcoFalke)
fa3583f856 fuzz: Avoid negative NodeId in ConsumeNode (MarcoFalke)
Pull request description:
Currently `FillNode` is a bit clumsy because it directly modifies memory of `CNode`. This gets in the way of moving that memory to `Peer`. Also, it isn't particularly consistent. See for example https://github.com/bitcoin/bitcoin/pull/21160#discussion_r739206139 .
Fix all issues by sending a `version`/`verack` in `FillNode` and let net_processing figure out the internal details.
ACKs for top commit:
jnewbery:
Strong concept ACK and light code review ACK fa19bab90a
Tree-SHA512: 33261d857c3fa6d5d39d742624009a29178ad5a15eb3fd062da741affa5a4854fd45ed20d59a6bba2fb068cf7b39cad6f95b2910be7cb6afdc27cd7917955b67
Avoid modifying the genesis block index entry during snapshot load. This
is because, in a future change that fixes LoadBlockIndex for UTXO
snapshots, we detect block index entries that are reliant on
assumed-valid ancestors and treat them specially.
Since the genesis block doesn't have BLOCK_VALID_SCRIPTS, it would be
erroneously marked BLOCK_ASSUMED_VALID during snapshot load if we didn't
skip it here. This would cause a "setBlockIndexCandidates() empty"
assertion to be tripped since all block index entries would be marked
assume-valid due to genesis, which is never re-validated.
There's probably no good reason to modify the genesis block index entry
during snapshot load anyway...
fa72dd314f fuzz: Move ISO8601 to one place (MarcoFalke)
Pull request description:
Seems confusing to split this to two places.
Also fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=42178
ACKs for top commit:
fanquake:
ACK fa72dd314f
Tree-SHA512: 637b0671078848ea417fdf66b92715602040fad34d4ca5f7b843a519a1cfeebe5d992a79a399deba39926905125681d66ab0dc05f66f79a26f3bf555e12fb0ba
