Make it possible to override from the command line (without recompiling)
the addrman check ratio in the common `TestingSetup::m_node::addrman`
(used by all unit tests) instead of hardcoding it to 0:
```
test_bitcoin --run_test="transaction_tests/tx_valid" -- -checkaddrman=1
```
In addrman unit tests, make it possible to override the check ratio from
the command line, without recompiling:
```
test_bitcoin --run_test="addrman_tests/*" -- -checkaddrman=1
```
Also, make the arguments of the constructor of `AddrManTest` the
same as the arguments of `AddrMan`.
Retrieve the command line arguments from the fuzzer and save them for
later retrieval by `BasicTestingSetup` so that we gain extra flexibility
of passing any config options on the test command line, e.g.:
```
FUZZ=addrman ./src/test/fuzz/fuzz --checkaddrman=5
```
A fuzz test should call `MakeNoLogFileContext<>()` in its initialize
function in order to invoke the constructor of `BasicTestingSetup`,
which sets `gArgs`.
Retrieve the command line arguments from boost and pass them to
`BasicTestingSetup` so that we gain extra flexibility of passing any
config options on the test command line, e.g.:
```
test_bitcoin -- -printtoconsole=1 -checkaddrman=5
```
e5b6aef61221b621ad77b5f075a16897e08835bf Move CBlockFileInfo::ToString method where class is declared (Russell Yanofsky)
f7086fd8ff084ab0dd656d75b7485e59263bdfd8 Add src/wallet/* code to wallet:: namespace (Russell Yanofsky)
90fc8b089d591cabff60ee829a33f96c37fd27ba Add src/node/* code to node:: namespace (Russell Yanofsky)
Pull request description:
There are no code changes, this is just adding `namespace` and `using` declarations and `node::` or `wallet::` qualifiers in some places.
Motivations for this change are:
- To make it easier to see when node and wallet code is being accessed places where it shouldn't be. For example if GUI code is accessing node and wallet internals or if wallet and node code are referencing each other.
- To make source code organization clearer ([#15732](https://github.com/bitcoin/bitcoin/issues/15732)), being able to know that `wallet::` code is in `src/wallet/`, `node::` code is in `src/node/`, `init::` code is in `src/init/`, `util::` code is in `src/util/`, etc.
Reviewing with `git log -p -n1 -U0 --word-diff-regex=.` can be helpful to verify this is only updating declarations, not changing code.
ACKs for top commit:
achow101:
ACK e5b6aef61221b621ad77b5f075a16897e08835bf
MarcoFalke:
Concept ACK e5b6aef61221b621ad77b5f075a16897e08835bf 🍨
Tree-SHA512: 3797745c90246794e2d55a2ee6e8b0ad5c811e4e03a242d3fdfeb68032f8787f0d48ed4097f6b7730f540220c0af99ef423cd9dbe7f76b2ec12e769a757a2c8d
efab28b06bfaa50c41337e84136cb58437e7ba00 Add FastRange32 function and use it throughout the codebase (Pieter Wuille)
96ecd6fa3e0f53c3a25eb7c328220b819f8dde03 scripted-diff: rename MapIntoRange to FastRange64 (Pieter Wuille)
c6d15c45d971fb25551b7a66a2615e3f0bee999b [moveonly] Move MapIntoRange() to separate util/fastrange.h (Pieter Wuille)
Pull request description:
Several places in the codebase use the fast range mapping technique described in https://lemire.me/blog/2016/06/27/a-fast-alternative-to-the-modulo-reduction/, some for 32-bit ranges, some for 64-bit ones.
Move all of these to `util/fastrange.h`, and give them a consistent name.
ACKs for top commit:
Sjors:
ACK efab28b06bfaa50c41337e84136cb58437e7ba00
shaavan:
reACK efab28b06bfaa50c41337e84136cb58437e7ba00
MarcoFalke:
review ACK efab28b06bfaa50c41337e84136cb58437e7ba00 🍸
Tree-SHA512: 3190a25ef21d17f0ab2afcd9b8d5a1813fdfac0d93996878017e84ff62eee412c823d6149ae8e92cfc3214458641e83ace4b022b4a0fe0679f78dbaee21c6227
fa68a6c2fc6754c160e0f98007785602201b3c47 scripted-diff: Rename touched member variables (MarcoFalke)
facd3df21f344dd84e5f28862056700c1fded17c Make blockstorage globals private members of BlockManager (MarcoFalke)
faa8c2d7d75f8d9782709e73e00e35851e233392 doc: Clarify nPruneAfterHeight for signet (MarcoFalke)
fad381b2f8e1beb18f748fbeb820e63545b9b0fd test: Load genesis block to allow flush (MarcoFalke)
fab262174b96854d2df5bee7da578990c9e9cb1e Move blockstorage-related unload to BlockManager::Unload (MarcoFalke)
fa467f3913918701c765f9bc754203b4591b894f move-only: Create WriteBlockIndexDB helper (MarcoFalke)
fa88cfd3f9896d5b56ea6c111a23f90a79253c18 Move functions to BlockManager (MarcoFalke)
Pull request description:
Globals aren't too nice because they hide dependencies, also they make testing harder.
Fix that by removing some.
ACKs for top commit:
Sjors:
ACK fa68a6c2fc6754c160e0f98007785602201b3c47
ryanofsky:
Code review ACK fa68a6c2fc6754c160e0f98007785602201b3c47. Nice changes!
Tree-SHA512: 6abc5929a5e43a05e238276721d46a64a44f23dca18c2caa9775437a32351d6815d88b88757254686421531d0df13861bbd3a202e13a3192798d87a96abef65d
e3544c864e3e56867de25b8db7b012d58b378050 init: Use clang-tidy named args syntax (Carl Dong)
3401630417d994b53ff3a89db2ea759ab1ec6f0f style-only: Rename *Chainstate return values (Carl Dong)
1dd582782d3c182aa952f23ec577f6a0a8672e7b docs: Make LoadChainstate comment more accurate (Carl Dong)
6b83576388e205116a0ebc67b9949f309eea1207 node/chainstate: Use MAX_FUTURE_BLOCK_TIME (Carl Dong)
Pull request description:
There are 2 proposed fixups in discussions in #23280 which I have not implemented:
1. An overhaul to return types and an option type for the two `*Chainstate` functions: https://github.com/bitcoin/bitcoin/pull/23280#issuecomment-984149564
- The change reintroduces stringy return types and is quite involved. It could be discussed in a separate PR.
2. Passing in the unix time to `VerifyChainstate` instead of a callback to get the time: https://github.com/bitcoin/bitcoin/pull/23280#discussion_r765051533
- I'm not sure it matters much whether it's a callback or just the actual unix time. Also, I think `VerifyDB` can take quite a while, and I don't want to impose that the function have to "run quickly" in order to have it be correct.
If reviewers feel strongly about either of the two fixups listed above, please feel free to open a PR based on mine and I'll close this one!
ACKs for top commit:
ryanofsky:
Code review ACK e3544c864e3e56867de25b8db7b012d58b378050
MarcoFalke:
ACK e3544c864e3e56867de25b8db7b012d58b378050 🐸
Tree-SHA512: dd1de0265b6785eef306e724b678ce03d7c54ea9f4b5ea0ccd7af59cce2ea3aba73fd4af0c15e2dca9265807dc4075f9afa2ec103672677b6638b1a4fc090904
This is needed to turn globals into member variables. Otherwise, this
will lead to issues:
runtime error: reference binding to null pointer of type 'CBlockFileInfo'
#0 in std::vector<CBlockFileInfo, std::allocator<CBlockFileInfo> >::operator[](unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:1046:2
#1 in BlockManager::FlushBlockFile(bool, bool) src/node/blockstorage.cpp:540:47
#2 in CChainState::FlushStateToDisk(BlockValidationState&, FlushStateMode, int) src/validation.cpp:2262:28
#3 in CChainState::ResizeCoinsCaches(unsigned long, unsigned long) src/validation.cpp:4414:15
#4 in validation_chainstate_tests::validation_chainstate_resize_caches::test_method() src/test/validation_chainstate_tests.cpp:66:12
6bf6e9fd9dece67878595a5f3361851c25833c49 net: change CreateNodeFromAcceptedSocket() to take Sock (Vasil Dimov)
9e3cbfca7c9efa620c0cce73503772805cc1fa82 net: use Sock in CConnman::ListenSocket (Vasil Dimov)
f8bd13f85ae5404adef23a52719d804a5c36b1e8 net: add new method Sock::Accept() that wraps accept() (Vasil Dimov)
Pull request description:
_This is a piece of https://github.com/bitcoin/bitcoin/pull/21878, chopped off to ease review._
Introduce an `accept(2)` wrapper `Sock::Accept()` and extend the usage of `Sock` in `CConnman::ListenSocket` and `CreateNodeFromAcceptedSocket()`.
ACKs for top commit:
laanwj:
Code review ACK 6bf6e9fd9dece67878595a5f3361851c25833c49
jamesob:
ACK 6bf6e9fd9dece67878595a5f3361851c25833c49 ([`jamesob/ackr/21879.2.vasild.wrap_accept_and_extend_u`](https://github.com/jamesob/bitcoin/tree/ackr/21879.2.vasild.wrap_accept_and_extend_u))
jonatack:
ACK 6bf6e9fd9dece67878595a5f3361851c25833c49 per `git range-diff ea989de 976f6e8 6bf6e9f` -- only change since my last review was `s/listen_socket.socket/listen_socket.sock->Get()/` in `src/net.cpp: CConnman::SocketHandlerListening()` -- re-read the code changes, rebase/debug build/ran units following my previous full review (https://github.com/bitcoin/bitcoin/pull/21879#pullrequestreview-761251278)
w0xlt:
tACK 6bf6e9f
Tree-SHA512: dc6d1acc4f255f1f7e8cf6dd74e97975cf3d5959e9fc2e689f74812ac3526d5ee8b6a32eca605925d10a4f7b6ff1ce5e900344311e587d19786b48c54d021b64
fa996c58e8a31ebe610d186cef408b6dd3b385a8 refactor: Avoid integer overflow in ApplyStats when activating snapshot (MarcoFalke)
fac01888d17423d6c23a9ce15d98fc88fb34e3cc Move AdditionOverflow to util, Add CheckedAdd with unit tests (MarcoFalke)
fa526d8fb6ab8f2678a30d4536aa9c45218f5269 Add dev doc to CCoinsStats::m_hash_type and make it const (MarcoFalke)
faff051560552d4405896e01920a18f698155a56 style: Remove unused whitespace (MarcoFalke)
Pull request description:
A snapshot contains the utxo set, including the out value. To activate the snapshot, the hash needs to be calculated. As a side-effect, the total amount in the snapshot is calculated (as the sum of all out values), but never used. Instead of running into an integer overflow in an unused result, don't calculate the result in the first place.
Other code paths (using the active utxo set) can not run into an integer overflow, since the active utxo set is valid.
Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39716
ACKs for top commit:
shaavan:
reACK fa996c58e8a31ebe610d186cef408b6dd3b385a8
vasild:
ACK fa996c58e8a31ebe610d186cef408b6dd3b385a8
Tree-SHA512: 4f207f634841f6f634fd02ae1e5907e343fd767524fd0e8149aa99fa9a1834fe50167d14874834d45236e9c325d567925f28129bacb7d80be29cf22277a16a14
ea4c9fd4ab9aaa2e8f2c2e38a75c9f05d0bfc866 test: Cover eviction by timeout in addrman_evictionworks (Martin Zumsande)
4f1bb467b556ec93c9b8f758783fda4d050da491 test: Add test for multiplicity in addrman new tables (Martin Zumsande)
e880bb7836dab2018049390884220177c6db9b92 test: Add test for updating addrman entries (Martin Zumsande)
f02eee8c8784dfc8db80a21ab6508f7c99298255 test: introduce utility function to retrieve an addrman (Martin Zumsande)
f0e5efb82493f7a14580335ce719d5be81c8713e test: Remove unused AddrManTest class (Martin Zumsande)
b696d7870b29232057600df5ddd8351888253b95 test: Remove tests for internal helper functions (Martin Zumsande)
0538520091bf2982a029a0298835400f5afbdc15 test: use AddrMan instead of AddrManTest where possible (Martin Zumsande)
1c65d427bbf61bb558cf7e18f7aff99b19f68508 test: Inline SimConnFail function (Martin Zumsande)
5b7aac34f2363822c3a1cfafda8ffc9528905058 test: delete unused GetBucketAndEntry function (Amiti Uttarwar)
2ba1e74e59a325ca6cb140757067dd5e0c7c249b test: Update addrman_serialization unit test to use AddrMan's interface (Amiti Uttarwar)
dad5f760211df314d650999e0a76edb0151b4fe1 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 ea4c9fd4ab9aaa2e8f2c2e38a75c9f05d0bfc866
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>
826e12b010eda4238f9e8cd875e8915a405bed0d 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 826e12b010eda4238f9e8cd875e8915a405bed0d
ryanofsky:
Code review ACK 826e12b010eda4238f9e8cd875e8915a405bed0d
Tree-SHA512: a4e7fd25e5d7a08b1e154ae6daf67c3048260a2684b0e569b544dd826693b7b969db9923b191e499cb8d8d0a2a73eb9330ff45909313145a9abb6052eb8c3ad9
bf4f8171352e9b384b42c91da61dfc9c3ca89ed8 refactor: addrman_select test (josibake)
5a64dc018c04ce16202a8e58ce92d2657c0b1806 refactor: addrman_evictionworks test (josibake)
e281fccd8a80d7cd48c3b17d58fd4a8915e1e965 refactor: addrman_noevict test (josibake)
8bdd9240d4310aafa1332159355f106a8fcfc5c9 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 bf4f8171352e9b384b42c91da61dfc9c3ca89ed8
mzumsande:
Code Review ACK bf4f8171352e9b384b42c91da61dfc9c3ca89ed8
Tree-SHA512: 93cc127aecff42c1c174daa04911af7e3460a5c40ddf96952fe4a6ab86fa1ff22d66724326abb709008d7f9f79c26c55c6d62753c40059c9ac60f869507ec913
c44c20108f7b7314f59f034110789385a24db280 p2p, refactor: drop unused DNSLookupFn param in LookupSubnet() (Vasil Dimov)
f0c9e68080432c1ab11b14e571b8dfb7cfe727f8 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 c44c20108f7b7314f59f034110789385a24db280
shaavan:
crACK c44c20108f7b7314f59f034110789385a24db280
Tree-SHA512: 55f64c7f403819dec84f4da06e63db50f7c0601a2d9a1ec196fda667c220ec6f5ad2a3c95e0e02275da9f6da6b984275d1dc10e19ed82005c5e13da5c5ecab02
edd0313ae7c94420642081c9172e349080bb9335 test: Improve "invalid_command" subtest in system_tests for Windows (Hennadii Stepanov)
fb1b0590af138e317803893d2cab9dc887f33c5b test: Fix "non-zero exit code" subtest in system_tests for Windows (Hennadii Stepanov)
0aad33db6410ed36fa0f4b96245cacbae7897d2e test: Fix "false" subtest in system_tests for Windows (Hennadii Stepanov)
507c009c1ee68a4c3ad100f765bf854307d5bf39 test: Fix "echo" subtest in the system_tests for Windows (Hennadii Stepanov)
Pull request description:
An attempt to fixbitcoin/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 edd0313ae7c94420642081c9172e349080bb9335
Tru3Nrg:
tACK edd0313ae7
Tree-SHA512: 66a4f2372858011ff862b71c6530bedb8bc731b18595636fac9affc9189d9320f212c68b62498f2b57ee7a07f59e842dbec085b76a7419791d1a06c8e80e7744
4d0ac72f3ae78e3c6a0d5dc4f7e809583abd0546 [fuzz] Add fuzzing harness to compare both implementations of ChaCha20 (stratospher)
65ef93203cc6a977c8e96f07cb9155f46faf5004 [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 4d0ac72f3ae78e3c6a0d5dc4f7e809583abd0546
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.
fad943821e35d0eb2143061e718f0193e12a4c71 scripted-diff: Rename touched member variables (MarcoFalke)
fa663a4c0d20487ed3f7a93e1c2ca9932b05f5a8 Use mockable time for peer connection time (MarcoFalke)
fad7ead146a152f46b25ce6623e05cbb1dbc8cca 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 fad943821e35d0eb2143061e718f0193e12a4c71
shaavan:
ACK fad943821e35d0eb2143061e718f0193e12a4c71
Tree-SHA512: af9bdfc695ab727b100c6810a7289d29b02b0ea9fa4fee9cc1f3eeefb52c8c465ea2734bae0c1c63b3b0d6264ba2c493268bc970ef6916570eb166de77829d82
faa6c3d44c861c0486c1369e1d098b7645ab07cd 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 faa6c3d44c861c0486c1369e1d098b7645ab07cd
Tree-SHA512: 5a58e7f1dcabf78d0c65d8c6d5d997063af1efeaa50ca7730fc00056fda7e0061b6f7a38907ea045fe667c9f61d392e01e556b425a95e6b126e3c41cd33deb83
2283b9cd1ee0fbd1e8ebc61673b1fe7596199a24 test: add tests for LoadBlockIndex when using multiple chainstates (James O'Beirne)
0fd599a51a700c028d6f7ed8067d2d9f6e6a04a5 validation: have LoadBlockIndex account for snapshot use (James O'Beirne)
d0c6e61f5dd3b6af818459d9d03b7ba316c5a3f7 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 2283b9cd1ee0fbd1e8ebc61673b1fe7596199a24 🤽
Sjors:
utACK 2283b9cd1ee0fbd1e8ebc61673b1fe7596199a24
Tree-SHA512: 7c9a80802df4722d85d12b78d2e7f628ac5f11cb8be66913d5c3230339bd1220c6723805509d4460826a17d1dc04b0ae172eb7d09ac0ea5dc5e41d77975cbd5e
