jnewbery
commented at 9:42 pm on October 23, 2020:
member
CAddrMan has internal consistency checks. Currently, these are only run when the program is compiled with the DEBUG_ADDRMAN option. This option is not enabled on any of our CI builds, and it’s likely that no-one is running them at all.
This PR makes consistency checks a (hidden) runtime option that can be enabled with -checkaddrman, where -checkaddrman=n will result in the consistency checks running every n operations (similar to -checkmempool=n). We set the ratio to 1/100 for our unit tests, and leave it disabled by default for all networks. Additionally, a consistency check failure now asserts, rather than logging and continuing. This matches the behavior of CTxMemPool and TxRequestTracker, where a failed consistency check asserts.
jnewbery renamed this:
addrman: make sanity checks a runtime option
addrman: Make sanity checks a runtime option
on Oct 23, 2020
DrahtBot added the label
P2P
on Oct 23, 2020
DrahtBot added the label
RPC/REST/ZMQ
on Oct 23, 2020
in
src/test/util/setup_common.cpp:177
in
b155d8aedaoutdated
jnewbery
commented at 4:06 pm on October 29, 2020:
I don’t it matters where the line break is, as long as the line isn’t too long.
MarcoFalke removed the label
RPC/REST/ZMQ
on Oct 24, 2020
DrahtBot
commented at 8:59 am on October 24, 2020:
member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#22627 ([addrman] De-duplicate Add() function by amitiuttarwar)
#21878 (Make all networking code mockable by vasild)
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.
practicalswift
commented at 9:03 am on October 24, 2020:
contributor
Concept ACK
Should we run with -checkaddrman in at least one of the Travis jobs?
jnewbery
commented at 10:28 am on October 24, 2020:
member
Should we run with -checkaddrman in at least one of the Traivs jobs?
-checkaddrman is enabled by default for regtest, so is on for all Travis jobs (although our functional tests probably don’t exercise addrman very much)
jnewbery marked this as a draft
on Oct 24, 2020
jnewbery force-pushed
on Oct 24, 2020
jnewbery
commented at 5:11 pm on October 24, 2020:
member
The consistency check fails on one of the unit tests. I’ve converted this PR to draft status while I work out why it’s failing.
practicalswift
commented at 9:56 pm on October 24, 2020:
contributor
-checkaddrman is enabled by default for regtest, so is on for all Travis jobs (although our functional tests probably don’t exercise addrman very much)
Oh, I missed the “by default on regtest” in the PR description. Sorry about that :)
Defaulting to sanity checking in regtest makes perfect sense! Even more Concept ACK :)
jnewbery renamed this:
addrman: Make sanity checks a runtime option
addrman: Make consistency checks a runtime option
on Oct 25, 2020
jnewbery force-pushed
on Oct 25, 2020
jnewbery marked this as ready for review
on Oct 25, 2020
jnewbery
commented at 10:00 am on October 25, 2020:
member
I’ve fixed the failing unit tests.
jnewbery
commented at 2:47 pm on October 25, 2020:
member
This makes some of the functional tests run very slowly. I’ll do some comparisons and maybe disable consistency checks on the slowest tests.
jnewbery
commented at 12:02 pm on October 26, 2020:
member
More generally, I’m not sure that these consistency checks are that useful to enable at runtime/CI rather than just using them as an aid when making changes to addrman internals.
jnewbery
commented at 9:30 am on January 6, 2021:
member
This seems to make rpc_net and p2p_getaddr_caching tests run extremely slowly.
Yes, see my comment at #20233 (comment). I was planning on addressing that after 20228 was merged, probably by disabling the checks for those tests, or perhaps by changing the consistency check configuration to be a ratio, like the mempool consistency checks:
I don’t think there’s any need for this to be based on #20228 – passing a “check_addrman” from init to CConnman seems to work fine, see ajtowns/bitcoin@202101-addrman-check (commits) eg.
Sure, we could continue to pass function calls and (now) ctor parameters through CConnman, but I don’t see why that is desirable. Connman uses addrman, but there’s no reason for it to have an addrman. Addrman is also used by peerman and the RPCs. Connman’s resposibility is to open and maintain connections. Storing and maintaining our map of accessible nodes on the network is a separate responsibility.
There was also some discussion here: http://www.erisian.com.au/bitcoin-core-dev/log-2020-12-16.html#l-585 about being able to manage addrman more directly. Again, in that case it’d be easier if RPC directly held a handle to the addrman instead of passing messages through connman.
More generally, I’m not sure that these consistency checks are that useful to enable at runtime/CI rather than just using them as an aid when making changes to addrman internals.
I think somewhere that they’re definitely useful would be in fuzz testing, to ensure that there’s no way to violate addrman’s invariants. See the usage of txrequest’s sanity check here:
which is called after every loop in the fuzz tester.
ajtowns
commented at 6:24 am on January 7, 2021:
member
Connman uses addrman, but there’s no reason for it to have an addrman.
There’s no point having addrman without a connman outside of unit tests (in which case the test can create an addrman directly), and connman uses addrman. That’s enough reason for connman to be the thing that owns it, and for anything that wants it to access it via connman. I don’t think there’s anything you could do with node.addrman that you couldn’t do equally well with node.connman.GetAddrman().
But more importantly, it’s easy to separate the two questions (should connman own the addrman instance and should debug_addrman be used more often) and judge them independently, so we should.
I think somewhere that they’re definitely useful would be in fuzz testing, […]
Yeah; I agree in principle. But while I think txrequest’s checks are reasonably optimised to be run frequently with modest data sets, I think addrman’s is very heavy weight and only really works for pretty small ones. Maybe it would make sense to have the automatic internal checks (Good() surrounds Good_() by invocations to Check()) only happen via compile-time changes so you can use them when debugging, but still have Check_() compiled unconditionally so that the fuzzer and unit tests call Check() themselves.
Any idea how much this PR currently slows down the fuzzer? Doing the checks irregularly might be enough either way though – perhaps especially if how irregular it is adjust dynamically (100% for the first x checks, 50% for the next x checks, etc?)
Hmm, actually, at least as it stands, perhaps addrman checks should be a regtest-only option, rather than a regtest-by-default option? Aren’t those checks always too heavyweight to run when addrman is populated from mainnet/testnet?
jnewbery
commented at 9:55 am on January 7, 2021:
member
But more importantly, it’s easy to separate the two questions (should connman own the addrman instance and should debug_addrman be used more often) and judge them independently, so we should.
The aim of this PR is to add the ability to run internal consistency checks in addrman. If that can be done without adding more unnecessary coupling between components, then we should do that. There’s no good reason that connman should be forwarding constructor parameters through to addrman, so I don’t want to add that.
This design of both connman and peerman holding a handle to another component is exactly the same as we use for banman, with the added bonus that banman is internally an optional component (although there’s no exposed configuration option to disable it). That design ensures very clean separation between components with no unexpected dependencies creeping in, and no boilerplate forwarding code in connman.
Changing the constuctor parameters is changing the public interface of addrman, so there are two choices:
keep it in connman and update connman’s ctor interface, adding yet more coupling between components
split out addrman and then add the ctor parameter
(2) is more work now, but it’s a net removal of boilerplate code (-24 in #20228), and is a better design for the long term.
MarcoFalke referenced this in commit
1999baac30
on Mar 30, 2021
jnewbery force-pushed
on Mar 30, 2021
jnewbery
commented at 10:54 am on March 30, 2021:
member
Rebased on master. I’m leaving this as draft since I still need to update it to disable addrman consistency checks for certain tests.
jnewbery force-pushed
on Jun 2, 2021
vasild
commented at 11:43 am on July 16, 2021:
member
Concept ACK
jnewbery force-pushed
on Jul 18, 2021
jnewbery force-pushed
on Jul 19, 2021
jnewbery marked this as ready for review
on Jul 19, 2021
jnewbery
commented at 10:49 am on July 19, 2021:
member
Addrman corruption is getting more attention recently, so I’ve rebased on master, with some small changes:
the option is now an integer, where setting -checkaddrman=n will cause the consistency checks to be run every n operations (similar to -checkmempool=n).
the consistency check ratio is set to 100 for unit tests. On my machine, running the addrman_tests took:
with no consistency checks: 0.65s
with consistency check ratio set to 100: 0.71s
with consistency check ratio set to 1: 7.75s
Running consistency checks on 1% of operations seems to give good increased coverage without impacting test runtime too much
consistency checks are disabled by default for all networks, including regtest. That can be changed in a follow-up PR if people think it should be enabled by default for regtest.
jonatack
commented at 11:01 am on July 19, 2021:
member
Concept ACK to facilitate using this check, and as recompiling after defining DEBUG_ADDRMAN is very slow. Perhaps use uint32 rather than int32.
@MarcoFalke I left the cpp file unchanged in case we want to later add some kind of recovery code instead of simply asserting on failure. Changing the if foo return -xx; to assert(!foo) would involve making changes in more places.
@vasild - What’s the advantage of using std::abort? I see lots more examples of assert(false) in the code than std::abort().
assert() calls abort(), so it is the same thing in practice (modulo the “strange” message assertion failed: false). Feel free to ignore or use assert(0) or assert(nullptr).
0 argsman.AddArg("-checkaddrman", strprintf("Do consistency checks on the addrman every <n> operations. Use 0 to disable. (default: %u)", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
This could result in “sometimes fails, sometimes succeeds” tests on CI. A developer should always get the failure if using -checkaddrman=1. If this is undesirable, then if (++counter % m_consistency_check == 0) can be considered to get the checks at deterministic times.
At least here we could use insecure_rand, which should be deterministic if initialized deterministically.
This has the unfortunate effect that the checks are run on the same operations for every run of the unit tests. Having them picked at random means that eventually they’re run at every operation in the unit tests. Taking entropy from insecure_rand at different points would also invalidate the tests that have hard-coded values based on the determinstic insecure_rand.
Leaving this as is for now. The GetRand() call doesn’t seem to result in a performance degradation from my tests: #20233 (comment).
vasild approved
vasild
commented at 4:03 pm on July 19, 2021:
member
ACKa343f9079c30e1dce6f13852ff84bab7dab3fcde
This results in about 8% slowdown of ./src/test/test_bitcoin on my machine (approx 6m10s -> 6m45s).
jnewbery
commented at 5:11 pm on July 19, 2021:
member
This results in about 8% slowdown of ./src/test/test_bitcoin on my machine (approx 6m10s -> 6m45s).
Hmmm, that’s rather worrying. I’d expect the only test for this to have a material impact on would be the addrman tests, which take less than a second to run. Perhaps the calls to GetRand() are expensive enough that even with consistency checks disabled, this has a negative effect on performance.
MarcoFalke
commented at 5:20 pm on July 19, 2021:
member
GetRand seems unneeded wasteful anyway (it calls SeedFast on every call). Have you seen #20233 (review) ?
jnewbery
commented at 5:30 pm on July 19, 2021:
member
GetRand seems unneeded wasteful anyway (it calls SeedFast on every call). Have you seen #20233 (comment) ?
I see it. I’ll try this with insecure_rand tomorrow. Thanks for the tip!
DrahtBot added the label
Needs rebase
on Jul 20, 2021
Seems reasonable. This can be done as a follow-up if desired.
jnewbery
commented at 10:30 am on July 20, 2021:
member
This results in about 8% slowdown of ./src/test/test_bitcoin on my machine (approx 6m10s -> 6m45s).
@vasild how many samples did you take? There’s some variance in the time taken to run the entire test suite, so you’ll need to run it several times to ensure that this isn’t just noise. Here are my results:
Returning immediately from _Check():
0→ time `for i in {1..5}; do ./src/test/test_bitcoin; done`
12[...]
34real 4m1.543s
5user 2m28.929s
6sys 0m25.260s
And calling GetRand(m_consistency_check) in _Check():
0→ time `for i in {1..5}; do ./src/test/test_bitcoin; done`
12[...]
34real 4m0.947s
5user 2m33.306s
6sys 0m21.416s
So for me, there’s essentially no difference.
jnewbery force-pushed
on Jul 20, 2021
DrahtBot removed the label
Needs rebase
on Jul 20, 2021
jnewbery
commented at 9:31 am on July 21, 2021:
member
Added the two commits from PR #22500, to resolve the various suggestions in this thread: #20233 (review).
jnewbery force-pushed
on Jul 21, 2021
jnewbery
commented at 9:41 am on July 21, 2021:
member
@MarcoFalke has just told me that he no longer believes that #22500 is the right approach, so I’ve removed those two commits again.
lsilva01 approved
lsilva01
commented at 11:17 pm on July 21, 2021:
contributor
464 {
465 AssertLockHeld(cs);
466467+ // Run consistency checks 1 in m_consistency_check_ratio times if enabled
468+ if (m_consistency_check_ratio == 0) return 0;
469+ if (GetRand(m_consistency_check_ratio) >= 1) return 0;
in
src/test/addrman_tests.cpp:37
in
21112bd5dfoutdated
33@@ -34,7 +34,7 @@ class CAddrManTest : public CAddrMan
34 //! Ensure that bucket placement is always the same for testing purposes.
35 void MakeDeterministic()
36 {
37- nKey.SetNull();
38+ nKey = uint256{1};
The same MakeDeterministic() also exists in net_tests.cpp and should be changed there too to prevent net_tests/caddrdb_read from failing (though I’m not sure why these tests are there, looks to me like they should be integrated into addrman_tests.cpp or removed if redundant).
Oh good spot! There’s also a deterministic addrman in the fuzzers.
I’ve added a deterministic argument to the CAddrMan ctor (similar to TxRequestTracker) to consolidate these into one place, so that the tests can just ask for a deterministic addrman rather than constructing and addrman and then updating its internals.
I agree that those addrman tests in net_tests.cpp should be moved to addrman_tests.cpp, but that can be done in another PR.
jnewbery force-pushed
on Jul 23, 2021
jnewbery
commented at 10:04 am on July 23, 2021:
member
464 {
465 AssertLockHeld(cs);
466467+ // Run consistency checks 1 in m_consistency_check_ratio times if enabled
468+ if (m_consistency_check_ratio == 0) return 0;
469+ if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return 0;
This has the unfortunate effect that the checks are run on the same operations for every run of the unit tests. Having them picked at random means that eventually they’re run at every operation in the unit tests.
I’ve moved the insecure_rand to the initializer list as suggested.
I’m not going to remove the nKey assignment in this PR. I’d like to remove the Clear() method from CAddrMan in a future PR. The only place it’s used in the product code is if peers.dat deserialization fails, we’ll call Clear(). I think it’s better just to instantiate a new CAddrMan object in that case, since it’s definitely possible to imagine new data being added to CAddrMan and not added to the Clear() method. It wasn’t previously possible to instantiate a new CAddrMan since it was a global initialized before main(), but now that its lifetime is managed to context.node, we can just reset the unique ptr to a new instance.
Two from the PR and two with master and the variance seemed low. However when I run this again five times I see the timing wildly varies between 5min and 8min for both PR and master. Btw that is a debug build, not meant to be a performance benchmark but rather how long would it take on the dev’s machines and on CI (which usually run debug builds).
jnewbery force-pushed
on Jul 26, 2021
jnewbery
commented at 4:00 pm on July 26, 2021:
member
Thanks for the review @vasild! I’ve taken your suggestion of setting insecure_rand in the initializer list.
I’ve also removed the default args for the CAddrMan constructor, since default args are evil and can hide subtle bugs.
In the previous incarnation of this PR which did not pass consistency_checks explicitly: CAddrMan addrman(/* deterministic */ false) it would have equaled to DEFAULT_ADDRMAN_CONSISTENCY_CHECKS.
In the latest incarnation (24515fee60) 0 is passed explicitly.
This contains the assumption that DEFAULT_ADDRMAN_CONSISTENCY_CHECKS is 0 which is true now, but could change in the future.
I think it is good to keep using DEFAULT_ADDRMAN_CONSISTENCY_CHECKS as default value instead of 0. That is, either call CAddrMan addrman(/* deterministic */ false, DEFAULT_ADDRMAN_CONSISTENCY_CHECKS) in all places or restore the default second argument, which defaults to DEFAULT_ADDRMAN_CONSISTENCY_CHECKS.
It’s the other way around. The previous version implicitly assumed that the DEFAULT_ADDRMAN_CONSISTENCY_CHECKS was 0. This line is explicitly saying that we want an addrman without consistency checks.
If we used DEFAULT_ADDRMAN_CONSISTENCY_CHECKS explicitly in the call sites and that value changes in future (unlikely I think, since we don’t want these debug checks enabled in production), then all call sites would have to be checked to make sure that the change in behavior is what is wanted. For example, we certainly wouldn’t want to enable the consistency checks in benchmarks, since it’d totally skew the measurements.
vasild
commented at 1:29 pm on July 27, 2021:
member
ACK0f959391190d973f09ea8ba5d4f34a87e2164040
vasild
commented at 10:54 am on July 30, 2021:
member
Before this PR, it was possible to run all unit and fuzz tests with checks enabled on every step (when compiled with DEBUG_ADDRMAN). One had to recompile, but changing the source code was not necessary.
With this PR, addrman unit tests are run with checks on every 100th step, other unit tests and fuzz tests have the checks disabled. To change that (to enable the checks or get them more often than once per 100), a modification of the source code is necessary on the following lines:
What about replacing those hardcoded 0s (no checks) with a constant DEFAULT_ADDRMAN_CONSISTENCY_CHECKS_IN_TESTS that can be easily changed in order to enable the checks? Or maybe have two constants ..._IN_ADDRMAN_TESTS=100 and ..._IN_TESTS=0.
Or replace the 0s with something like m_args.GetArg("-checkaddrman", 0) so that the checks can be enabled without modifying the source code, e.g.:
What about replacing those hardcoded 0s (no checks) with a constant DEFAULT_ADDRMAN_CONSISTENCY_CHECKS_IN_TESTS that can be easily changed in order to enable the checks? Or maybe have two constants …_IN_ADDRMAN_TESTS=100 and …_IN_TESTS=0.
Or replace the 0s with something like m_args.GetArg("-checkaddrman", 0) so that the checks can be enabled without modifying the source code.
@vasild - This can be done in a follow-up PR if desired. This PR has already gone through multiple rounds of review, so I’d like to settle on a final version of it now.
DrahtBot removed the label
Needs rebase
on Aug 2, 2021
theStack
commented at 8:31 pm on August 2, 2021:
member
Concept ACK
This approach seems to resolve a lot of potential headache caused by the fact that barely anyone ever builds with DEBUG_ADDRMAN (see #21940 (comment) for an example where this caused problems) and we don’t have a CI instance running with it. The only slight drawback I could think of this solution is that the (released) bitcoind binary is a little bit larger compared to the current compile-time flag approach, containing code that the average user will never run. OTOH the fact that with this the tests can trigger regular addrman consistency checks with the default build outweights this by far :)
in
src/addrman.h:126
in
7883160288outdated
121@@ -119,7 +122,7 @@ class CAddrInfo : public CAddress
122 * tried ones) is evicted from it, back to the "new" buckets.
123 * * Bucket selection is based on cryptographic hashing, using a randomly-generated 256-bit key, which should not
124 * be observable by adversaries.
125- * * Several indexes are kept for high performance. Defining DEBUG_ADDRMAN will introduce frequent (and expensive)
126+ * * Several indexes are kept for high performance. Setting m_consistency_check_ratio will introduce (expensive)
127 * consistency checks for the entire data structure.
0- * * Several indexes are kept for high performance. Setting m_consistency_check_ratio will introduce (expensive)
1- * consistency checks for the entire data structure.
2+ * * Several indexes are kept for high performance. Setting m_consistency_check_ratio with the -checkaddrman
3+ configuration option will introduce (expensive) consistency checks for the entire data structure.
Perhaps, but that’s a change in behaviour. Maybe we can do it in a follow up PR.
in
src/bench/addrman.cpp:115
in
7883160288outdated
111@@ -112,10 +112,12 @@ static void AddrManGood(benchmark::Bench& bench)
112 * we want to do the same amount of work in every loop iteration. */
113114 bench.epochs(5).epochIterations(1);
115+ size_t addrman_count = bench.epochs() * bench.epochIterations();
Since we’re already clamping the value, I don’t think it matters.
jonatack
commented at 4:51 pm on August 4, 2021:
member
ACK7883160288226ddc9a682e48647efef051a6286e code review, rebased to current master, and at each commit debug-built with CPPFLAGS="-DDEBUG_ADDRMAN" and ran net_tests, addrman_tests, and ./src/bench/bench_bitcoin -filter=AddrManGood. A number of suggestions below for this PR or follow-ups. Happy to re-ACK if any are taken.
Noting here these follow-ups in the comments that look worthwhile:
jonatack
commented at 4:54 pm on August 4, 2021:
member
In 0d2d0de the commit message states “addrman_tests fail when consistency checks are enabled”…I was unable to reproduce this failure in the preceding commits. Is it still the case, should the message be updated, can the commit be dropped?
amitiuttarwar
commented at 10:39 pm on August 4, 2021:
nit: the variable name addrs makes me think its a vector since that’s a common naming pattern in the codebase, so I’d prefer if this was something like num_addrs.
amitiuttarwar
commented at 11:24 pm on August 4, 2021:
contributor
light code review ACK7883160288
I think its great that, with this PR, the addrman unit tests would run the consistency checks, and it’s easier for developers to manually run or to potentially incorporate into the functional tests.
Add missing const to CAddrMan::Check_()
Also: Always compile the function signature to avoid similar issues in
the future.
ee458d84fc
DrahtBot added the label
Needs rebase
on Aug 5, 2021
[addrman] Add deterministic argument to CAddrMan ctor
Removes the need for tests to update nKey and insecure_rand after constructing
a CAddrMan.
fa9710f62c
[tests] Make deterministic addrman use nKey = 1
addrman_tests fail when consistency checks are enabled, since the tests
set the deterministic test addrman's nKey value to zero, which is an
invalid value. Change this so that deterministic addrman's nKey value is
set to 1.
This requires updating a few tests that are using magic values derived
from nKey being set to 0.
10aac24145
jnewbery force-pushed
on Aug 5, 2021
jnewbery
commented at 4:15 pm on August 5, 2021:
member
DrahtBot removed the label
Needs rebase
on Aug 5, 2021
jnewbery
commented at 5:17 pm on August 5, 2021:
member
In 0d2d0de the commit message states “addrman_tests fail when consistency checks are enabled”…I was unable to reproduce this failure in the preceding commits. Is it still the case, should the message be updated, can the commit be dropped?
@jonatack - I think it’s still true. Try enabling DEBUG_ADDRMAN and running the addrman tests here: https://github.com/jnewbery/bitcoin/tree/2021-08-fail-addrman-tests
The first commit fixes the build so it’s possible to build with DEBUG_ADDRMAN. The second commit aborts if Check() fails, rather than just logging.
Without the Make deterministic addrman use nKey = 1 commit, then when we make addrman checks a runtime option and add the assert, then the tests would fail.
jonatack
commented at 5:48 pm on August 5, 2021:
member
Thanks @jnewbery, I’ll have a look (once the guix build finishes ⏰)
amitiuttarwar
commented at 6:55 pm on August 6, 2021:
contributor
reACK00fd0891ce
jonatack
commented at 10:14 am on August 7, 2021:
member
ACK00fd0891ce799084fdd137fafc0522c27fbfb429 per git range-diff 03826ae 7883160 00fd089 modulo the addrman fuzzer crash
0SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior serialize.h:205:93 in
1fuzz: ./addrman.h:754: void CAddrMan::Check() const: Assertion `false' failed.
2==62677== ERROR: libFuzzer: deadly signal
3 [#0](/bitcoin-bitcoin/0/) 0x55f7ef82d671 in __sanitizer_print_stack_trace (/home/jon/projects/bitcoin/bitcoin/src/test/fuzz/fuzz+0x1000671)
4 [#1](/bitcoin-bitcoin/1/) 0x55f7ef775008 in fuzzer::PrintStackTrace() fuzzer.o
5 [#2](/bitcoin-bitcoin/2/) 0x55f7ef758ef3 in fuzzer::Fuzzer::CrashCallback() fuzzer.o
6 [#3](/bitcoin-bitcoin/3/) 0x7fe5896ab13f (/lib/x86_64-linux-gnu/libpthread.so.0+0x1413f)
7 [#4](/bitcoin-bitcoin/4/) 0x7fe589317ce0 in __libc_signal_restore_set signal/../sysdeps/unix/sysv/linux/internal-signals.h:86:3
8 [#5](/bitcoin-bitcoin/5/) 0x7fe589317ce0 in raise signal/../sysdeps/unix/sysv/linux/raise.c:48:3
9 [#6](/bitcoin-bitcoin/6/) 0x7fe589301536 in abort stdlib/abort.c:79:7
10 [#7](/bitcoin-bitcoin/7/) 0x7fe58930140e in __assert_fail_base assert/assert.c:92:3
11 [#8](/bitcoin-bitcoin/8/) 0x7fe589310661 in __assert_fail assert/assert.c:101:3
12 [#9](/bitcoin-bitcoin/9/) 0x55f7ef86ed17 in CAddrMan::Check() const ./addrman.h:754:13
13 [#10](/bitcoin-bitcoin/10/) 0x55f7ef866d2c in CAddrMan::Select(bool) const ./addrman.h:601:9
14 [#11](/bitcoin-bitcoin/11/) 0x55f7ef8638e1 in addrman_fuzz_target(Span<unsigned char const>) test/fuzz/addrman.cpp:306:26
15 [#12](/bitcoin-bitcoin/12/) 0x55f7ef85b9d5 in void std::__invoke_impl<void, void (*&)(Span<unsigned char const>), Span<unsigned char const> >(std::__invoke_other, void (*&)(Span<unsigned char const>), Span<unsigned char const>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/invoke.h:60:14
16 [#13](/bitcoin-bitcoin/13/) 0x55f7ef85b89d in std::enable_if<is_invocable_r_v<void, void (*&)(Span<unsigned char const>), Span<unsigned char const> >, void>::type std::__invoke_r<void, void (*&)(Span<unsigned char const>), Span<unsigned char const> >(void (*&)(Span<unsigned char const>), Span<unsigned char const>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/invoke.h:110:2
17 [#14](/bitcoin-bitcoin/14/) 0x55f7ef85b608 in std::_Function_handler<void (Span<unsigned char const>), void (*)(Span<unsigned char const>)>::_M_invoke(std::_Any_data const&, Span<unsigned char const>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/std_function.h:291:9
18 [#15](/bitcoin-bitcoin/15/) 0x55f7f078934d in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/std_function.h:622:14
19 [#16](/bitcoin-bitcoin/16/) 0x55f7f0788ff8 in LLVMFuzzerTestOneInput test/fuzz/fuzz.cpp:91:5
20 [#17](/bitcoin-bitcoin/17/) 0x55f7ef75a783 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) fuzzer.o
21 [#18](/bitcoin-bitcoin/18/) 0x55f7ef759c8a in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) fuzzer.o
22 [#19](/bitcoin-bitcoin/19/) 0x55f7ef75c084 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) fuzzer.o
23 [#20](/bitcoin-bitcoin/20/) 0x55f7ef75c299 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) fuzzer.o
24 [#21](/bitcoin-bitcoin/21/) 0x55f7ef74bae7 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) fuzzer.o
25 [#22](/bitcoin-bitcoin/22/) 0x55f7ef7757c2 in main (/home/jon/projects/bitcoin/bitcoin/src/test/fuzz/fuzz+0xf487c2)
26 [#23](/bitcoin-bitcoin/23/) 0x7fe589302d09 in __libc_start_main csu/../csu/libc-start.c:308:16
27 [#24](/bitcoin-bitcoin/24/) 0x55f7ef722969 in _start (/home/jon/projects/bitcoin/bitcoin/src/test/fuzz/fuzz+0xef5969)
2829NOTE: libFuzzer has rudimentary signal handlers.
30 Combine libFuzzer with AddressSanitizer or similar for better crash reports.
31SUMMARY: libFuzzer: deadly signal
32MS: 0 ; base unit: 0000000000000000000000000000000000000000330x0,0x0,0x0,0x0,0x0,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0xe1,0xdc,0xdc,0xdc,0xdc,0xdc,0xdc,0xdc,0x23,0x23,0x23,0xbb,0x1,0x0,0x0,0x23,0x27,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x40,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x0,0x0,0x0,0x0,0x1,0x0,0x0,0x0,0x0,0x0,0xf,0xda,0x3f,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x23,0x23,0x0,0x0,0x0,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0xff,0xff,0xff,0x1f,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0x23,0xf4,0xf4,0xf4,0xf4,0xf4,0xf4,0xf4,0xf4,0xf4,0xb4,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x23,0x23,0x23,0x23,0x23,0x23,0xe1,0xdc,0xdc,0xdc,0xdc,0x32,0xa3,0x1,0x0,0x0,0x0,0x0,0x0,0x0,0x41,0x0,0xff,0x0,0x0,0xff,0x0,0x0,0x2b,
34\x00\x00\x00\x00\x00#############\xe1\xdc\xdc\xdc\xdc\xdc\xdc\xdc###\xbb\x01\x00\x00#'#############@#################\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x0f\xda?\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00##\x00\x00\x00########\xff\xff\xff\x1f####################\xf4\xf4\xf4\xf4\xf4\xf4\xf4\xf4\xf4\xb4\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff######\xe1\xdc\xdc\xdc\xdc2\xa3\x01\x00\x00\x00\x00\x00\x00A\x00\xff\x00\x00\xff\x00\x00+35artifact_prefix='./'; Test unit written to ./crash-d20c926b04d9e4e2657d97d3c5e77bde0e9a9daa
36Base64: AAAAAAAjIyMjIyMjIyMjIyMj4dzc3Nzc3NwjIyO7AQAAIycjIyMjIyMjIyMjIyMjQCMjIyMjIyMjIyMjIyMjIyMjAAAAAAEAAAAAAA/aPwAAAAAAAAAAAAAAIyMAAAAjIyMjIyMjI////x8jIyMjIyMjIyMjIyMjIyMjIyMjI/T09PT09PT09LT///////////////////8jIyMjIyPh3Nzc3DKjAQAAAAAAAEEA/wAA/wAAKw==
Don’t hesitate to reconsider taking the suggestion at #20233 (review).
Without the Make deterministic addrman use nKey = 1 commit, then when we make addrman checks a runtime option and add the assert, then the tests would fail.
Got it. You are right–I dropped that commit and the addrman unit tests fail.
jnewbery force-pushed
on Aug 11, 2021
jnewbery
commented at 11:11 am on August 11, 2021:
member
I’ve disabled the consistency checks again in the addrman fuzz test. I think the problem is that the fuzzer will call Clear(), and subsequent calls to Check() may fail, since Clear() may leave the CAddrMan in a slightly inconsistent state.
[addrman] Make addrman consistency checks a runtime option
Currently addrman consistency checks are a compile time option, and are not
enabled in our CI. It's unlikely anyone is running these consistency checks.
Make them a runtime option instead, where users can enable addrman
consistency checks every n operations (similar to mempool tests). Update
the addrman unit tests to do internal consistency checks every 100
operations (checking on every operations causes the test runtime to
increase by several seconds).
Also assert on a failed addrman consistency check to terminate program
execution.
a4d78546b0
jnewbery force-pushed
on Aug 12, 2021
jonatack
commented at 5:02 pm on August 12, 2021:
member
ACKa4d78546b0858602c60c03fdf8b35ca666ab2e56 per git diff 00fd089 a4d7854, tested by adding logging similar to #22479 and running with -checkaddrman=<n> for various values 0/1/10/100 etc, tested the updated docs with bitcoind -help-debug | grep -A2 "checkaddrman\|checkmempool" and verified rebased on master that compiling with CPPFLAGS="-DDEBUG_ADDRMAN" no longer causes the build to error.
0src/addrman.cpp
1@@ -438,6 +438,7 @@ int CAddrMan::Check_() const
2 // Run consistency checks 1 in m_consistency_check_ratio times if enabled
3 if (m_consistency_check_ratio == 0) return 0;
4 if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return 0;
5+ LogPrintf("Check addrman\n");
0$ ./src/bitcoind -help-debug | grep -A2 "checkaddrman\|checkmempool"
1 -checkaddrman=<n>
2 Run addrman consistency checks every <n> operations. Use 0 to disable.
3 (default: 0)
4--
5 -checkmempool=<n>
6 Run mempool consistency checks every <n> transactions. Use 0 to disable.
7 (default: 0, regtest: 1)
mzumsande
commented at 9:39 pm on August 12, 2021:
member
Code-review ACKa4d78546b0858602c60c03fdf8b35ca666ab2e56
Also did some testing with different values of -checkaddrman and it worked as expected.
in
src/test/addrman_tests.cpp:876
in
10aac24145outdated
Agree with vasild’s idea of introducing constants for the consistency_checks CAddrMan ctor param in unit tests, which can be done in a follow-up: #20233 (comment)
Also left two yocto-nits below regarding unit test assertion style, probably also best picked up in a follow-up.
fanquake
commented at 9:02 am on August 13, 2021:
member
The various nits/changes mentioned can be done in a followup. Either separately, or potentially as part of the Clear() branch.
fanquake referenced this in commit
803ef70fd9
on Aug 13, 2021
mzumsande
commented at 9:11 am on August 13, 2021:
member
I’ve disabled the consistency checks again in the addrman fuzz test. I think the problem is that the fuzzer will call Clear(), and subsequent calls to Check() may fail, since Clear() may leave the CAddrMan in a slightly inconsistent state.
Not sure if this is sufficient. Since #22493, we have a fuzz test that deserializes from random data, and Unserialize() doesn’t seem to be designed to prevent the finer inconsistencies from occurring that Check() checks for. I verified that the fuzz inputs from #22503 and #22519 still crash when running on this PR with consistency_check_ratio=1 (#22504 doesn’t for some reason)
`
fanquake closed this
on Aug 13, 2021
jnewbery deleted the branch
on Aug 13, 2021
sidhujag referenced this in commit
1416e23ca6
on Aug 15, 2021
vasild
commented at 12:24 pm on October 27, 2021:
member
… replace the 0s with something like m_args.GetArg("-checkaddrman", 0) so that the checks can be enabled without modifying the source code.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-06-08 03:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
