#22974 improved the performance of CAddrMan::Good() significantly so that it could be used directly in the fuzz tests, instead of those tests reaching directly into addrman's internal data structures.
This PR continues the work of making the fuzz tests only use CAddrMan's public methods and pulls the Fill() and RandAddr() methods from CAddrManDeterministic into free functions, since they no longer need access to CAddrMan internals.
23 | @@ -23,92 +24,85 @@ void initialize_addrman() 24 | SelectParams(CBaseChainParams::REGTEST); 25 | } 26 | 27 | -class CAddrManDeterministic : public CAddrMan 28 | +/** Generate a random address. Always returns a valid address. */ 29 | +static CNetAddr RandAddr(FuzzedDataProvider& fuzzed_data_provider, FastRandomContext& fast_random_context) 30 | + EXCLUSIVE_LOCKS_REQUIRED(cs)
leftover annotation?
Oops! Fixed now. Thank you.
Rebased
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
137 | - Add_(addr, source, time_penalty); 138 | + return addr; 139 | +} 140 | 141 | - if (n > 0 && mapInfo.size() % n == 0) { 142 | - Good_(addr, false, GetTime());
The new code calls Good() which calls Good_(/*test_before_evict=*/true) whereas the old code passed false. I think this is ok.
ACK 19e2d94ecd599e469e3ff168b456088e12e61f6f
The public Good() will do some extra locking and call Check() in comparison to Good_(). The locking should be insignificant wrt performance and the Check() should be noop due to the used consistency_check_ratio=0. Also, the public Good() will do "test before evict" which could cause some slowdown.
I did measure the timings to fill an addrman with hardcoded n=3 (33% tried), num_sources=50 and num_addresses=500 on both master and this PR. The addrman ends up with about 16k addresses.
master:
13.493194 sec: fill
0.534130 sec: ser
5.664423 sec: deser
1.330460 sec: compare
13.488360 sec: fill
0.529679 sec: ser
5.632100 sec: deser
1.321111 sec: compare
This PR:
14.130295 sec: fill
0.553457 sec: ser
5.809201 sec: deser
1.392843 sec: compare
13.757125 sec: fill
0.556877 sec: ser
5.863635 sec: deser
1.371259 sec: compare
Concept ACK
Don't reach inside the object-under-test to use its random context.
Use a (reference) parameter instead of a data member of
CAddrManDeterministic. This will allow us to make Fill() a free function
in a later commit.
Also remove CAddrManDeterministic.m_fuzzed_data_provider since it's no
longer used.
It doesn't require access to CAddrManDeterministic
Also rename it to FillAddrman and pass an addrman reference as an
argument. Change FillAddrman to only use addrman's public interface methods.
Suggested here: https://github.com/bitcoin/bitcoin/pull/22974#discussion_r711119626
Rebased. Marking ready for review.
SUMMARY: ThreadSanitizer: data race /usr/lib/llvm-12/bin/../include/c++/v1/ios:523:12 in std::__1::ios_base::width() const
concept ACK
SUMMARY: ThreadSanitizer: data race /usr/lib/llvm-12/bin/../include/c++/v1/ios:523:12 in std::__1::ios_base::width() const @MarcoFalke - I don't see how that can be related to this PR. The failure is in
validation_chainstate_tests, which I don't touch here (changes are only in fuzz/addrman.cpp).
utACK 44452110f0fa7cc1bcb941a3c7b5db4b492a7b9c. Agree the failure seems unrelated, looks like some startup race.
Thanks very much @vasild for the benchmarks. Indeed it appears the only functional change here is hitting more locks and test_before_evict is true, good to know those don't slow things down too much.
It appears to me that AddrManDeterministic would no longer be required if AddrMan accepted a reference to a FastRandomContext and grew an equality operator, though I'm not sure there's any need for that.
ACK 44452110f0fa7cc1bcb941a3c7b5db4b492a7b9c
Correct, the tsan failure is unrelated (known bug with boost). Sorry for missing this. Re-run the task, but it can be ignored either way.
ACK 44452110f0fa7cc1bcb941a3c7b5db4b492a7b9c
Reviewed the code and did a few hours of fuzzing addrman_serdeser, and the results look as expected to me (e.g. addrman of various sizes are created for different fuzzing inputs).
Not related to this PR, but the structure of src/test/addrman.cpp is a bit confusing to me, maybe the fuzz target data_stream_addr_man should be moved from the beginning of the file to the other targets.
301 | @@ -307,7 +302,7 @@ FUZZ_TARGET_INIT(addrman_serdeser, initialize_addrman) 302 | 303 | CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION); 304 | 305 | - addr_man1.Fill(); 306 | + FillAddrman(addr_man1, fuzzed_data_provider);
Not related to the changes in this PR, but a general observation on the changed fuzz test:
I just noticed that addrman_serdeser is currently the slowest (>60 minutes, slower almost by a factor of 2 compared to the second slowest one) of fuzz tests when run for the qa asset corpus (CI Link), so it seems to be the current bottleneck for a CI run to finish. I wonder if it this is still in the acceptable range, or whether it would make sense to ease the parameters a bit in a follow-up so that it would fill addrman with less addresses.