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.

Concept ACK

Should we run with -checkaddrman in at least one of the Travis jobs?

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)

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.

-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 :)

I've fixed the failing unit tests.

This makes some of the functional tests run very slowly. I'll do some comparisons and maybe disable consistency checks on the slowest tests.

Marking as draft until #20228 is merged.

This seems to make rpc_net and p2p_getaddr_caching tests run extremely slowly:

rpc_net.py                                       | ✓ Passed  | 1372 s
p2p_getaddr_caching.py                           | ✓ Passed  | 1367 s

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 https://github.com/ajtowns/bitcoin/commits/202101-addrman-check eg.

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.

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.

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?

Rebased and addressed review comments from @jonatack and @amitiuttarwar

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.

Thanks @jnewbery, I'll have a look (once the guix build finishes ⏰)

reACK 00fd089

ACK 00fd089 per git range-diff 03826ae 7883160 00fd089 modulo the addrman fuzzer crash

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior serialize.h:205:93 in 
fuzz: ./addrman.h:754: void CAddrMan::Check() const: Assertion `false' failed.
==62677== ERROR: libFuzzer: deadly signal
    #0 0x55f7ef82d671 in __sanitizer_print_stack_trace (/home/jon/projects/bitcoin/bitcoin/src/test/fuzz/fuzz+0x1000671)
    #1 0x55f7ef775008 in fuzzer::PrintStackTrace() fuzzer.o
    #2 0x55f7ef758ef3 in fuzzer::Fuzzer::CrashCallback() fuzzer.o
    #3 0x7fe5896ab13f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1413f)
    #4 0x7fe589317ce0 in __libc_signal_restore_set signal/../sysdeps/unix/sysv/linux/internal-signals.h:86:3
    #5 0x7fe589317ce0 in raise signal/../sysdeps/unix/sysv/linux/raise.c:48:3
    #6 0x7fe589301536 in abort stdlib/abort.c:79:7
    #7 0x7fe58930140e in __assert_fail_base assert/assert.c:92:3
    #8 0x7fe589310661 in __assert_fail assert/assert.c:101:3
    #9 0x55f7ef86ed17 in CAddrMan::Check() const ./addrman.h:754:13
    #10 0x55f7ef866d2c in CAddrMan::Select(bool) const ./addrman.h:601:9
    #11 0x55f7ef8638e1 in addrman_fuzz_target(Span<unsigned char const>) test/fuzz/addrman.cpp:306:26
    #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
    #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
    #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
    #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
    #16 0x55f7f0788ff8 in LLVMFuzzerTestOneInput test/fuzz/fuzz.cpp:91:5
    #17 0x55f7ef75a783 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) fuzzer.o
    #18 0x55f7ef759c8a in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) fuzzer.o
    #19 0x55f7ef75c084 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) fuzzer.o
    #20 0x55f7ef75c299 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) fuzzer.o
    #21 0x55f7ef74bae7 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) fuzzer.o
    #22 0x55f7ef7757c2 in main (/home/jon/projects/bitcoin/bitcoin/src/test/fuzz/fuzz+0xf487c2)
    #23 0x7fe589302d09 in __libc_start_main csu/../csu/libc-start.c:308:16
    #24 0x55f7ef722969 in _start (/home/jon/projects/bitcoin/bitcoin/src/test/fuzz/fuzz+0xef5969)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x0,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,
\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+
artifact_prefix='./'; Test unit written to ./crash-d20c926b04d9e4e2657d97d3c5e77bde0e9a9daa
Base64: AAAAAAAjIyMjIyMjIyMjIyMj4dzc3Nzc3NwjIyO7AQAAIycjIyMjIyMjIyMjIyMjQCMjIyMjIyMjIyMjIyMjIyMjAAAAAAEAAAAAAA/aPwAAAAAAAAAAAAAAIyMAAAAjIyMjIyMjI////x8jIyMjIyMjIyMjIyMjIyMjIyMjI/T09PT09PT09LT///////////////////8jIyMjIyPh3Nzc3DKjAQAAAAAAAEEA/wAA/wAAKw==

Don't hesitate to reconsider taking the suggestion at #20233 (comment).

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.

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.

I have a branch that removes Clear() at https://github.com/jnewbery/bitcoin/tree/2021-08-remove-addrman-clear, which I'll open after this is merged, and then re-enable consistency checks in the fuzzer.

ACK a4d7854 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.

src/addrman.cpp
@@ -438,6 +438,7 @@ int CAddrMan::Check_() const
     // Run consistency checks 1 in m_consistency_check_ratio times if enabled
     if (m_consistency_check_ratio == 0) return 0;
     if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return 0;
+    LogPrintf("Check addrman\n");

$ ./src/bitcoind -help-debug | grep -A2 "checkaddrman\|checkmempool"
  -checkaddrman=<n>
       Run addrman consistency checks every <n> operations. Use 0 to disable.
       (default: 0)
--
  -checkmempool=<n>
       Run mempool consistency checks every <n> transactions. Use 0 to disable.
       (default: 0, regtest: 1)

Code-review ACK a4d7854
Also did some testing with different values of -checkaddrman and it worked as expected.

The various nits/changes mentioned can be done in a followup. Either separately, or potentially as part of the Clear() branch.

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.

I have a branch that removes Clear() at https://github.com/jnewbery/bitcoin/tree/2021-08-remove-addrman-clear, which I'll open after this is merged, and then re-enable consistency checks in the fuzzer.

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)
`

... 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 ...

Done in #23373.