test_bitcoin and caddrdb_read failures #8069

Current master:

Running 194 test cases...
test/net_tests.cpp:94: error in "caddrdb_read": check addrman1.size() == 3 failed
test/net_tests.cpp:105: error in "caddrdb_read": check addrman2.size() == 3 failed

*** 2 failures detected in test suite "Bitcoin Test Suite"

@EthanHeilman ?

I extended a debugging:

diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index 6debf6a..64afbad 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -57,6 +57,8 @@ CDataStream AddrmanToStream(CAddrManSerializationMock& addrman)
     ssPeersIn << addrman;
     std::string str = ssPeersIn.str();
     vector<unsigned char> vchData(str.begin(), str.end());
+    cerr << "DBG: vchData.size() = " << vchData.size();
+    cerr << ", vchData = " << HexStr(vchData) << "\n";
     return CDataStream(vchData, SER_DISK, CLIENT_VERSION);
 }

@@ -89,6 +91,10 @@ BOOST_AUTO_TEST_CASE(caddrdb_read)
         exceptionThrown = true;
     }

+    if (addrman1.size() != 3)
+        cerr << "addrman1.size() == " << addrman1.size() << "\n";
+    if (exceptionThrown)
+        cerr << "exceptionThrown!\n";
     BOOST_CHECK(addrman1.size() == 3);
     BOOST_CHECK(exceptionThrown == false);

@@ -99,6 +105,8 @@ BOOST_AUTO_TEST_CASE(caddrdb_read)
     CAddrDB adb;
     BOOST_CHECK(addrman2.size() == 0);
     adb.Read(addrman2, ssPeers2);
+    if (addrman2.size() != 3)
+        cerr << "addrman2.size() == " << addrman2.size();
     BOOST_CHECK(addrman2.size() == 3);
 }

And to reproduce the problem:

for i in {1..100}; do ./test_bitcoin --run_test=net_tests || break ; done

It looks like the serialization of CAddrMan is fragile...

The nNew inside serialized format is 3 vs 2 in such simple case. Why?

Hmm, in the fail case, after the third entry added to the addrmanUncorrupted (https://github.com/bitcoin/bitcoin/blob/master/src/test/net_tests.cpp#L76), the size of addrmanUncorrupted is still two! So it is not serialization related!

This is new test added in the last merge commit #7696.

I can reproduce the problem in plain master with addrman_simple:

for i in {1..100}; do ./test_bitcoin --run_test=addrman_tests || break; done
...
Running 11 test cases...
test/addrman_tests.cpp:87: error in "addrman_simple": check addrman.size() == 2 failed

*** 1 failure detected in test suite "Bitcoin Test Suite"

@paveljanik Looking at this now.

In regards to the net_tests failing: Addrman will evict an existing addr if it is in the same position as an existing addr. This position is determined via a hash function and a randomly picked seed (nKey). This seed is serialized and deserialized so if the Addrman which is serialized to the disk has a randomly chosen seed this will cause non-determinism down the road when it is deserialized and tested.

For testing purposes I created a deterministic subclass of Addrman to prevent exactly these problems but it was not used in this test. I have created a pull request which resolves this issue: #8070

In regards to the addrman_tests failing: Unlike the net_tests failure I'm unable to replicate addrman_tests failures and addrman_tests does not have the same issue as net_tests. I ran your script several times without a failure.

for i in {1..100}; do ./test_bitcoin --run_test=addrman_tests || break; done
Running 11 test cases...
...
*** No errors detected

Now testing for i in {1..10000}; will post results when it finished. What system are you running?

Ran addrman_tests 10000 times no failures on my side:

for i in {1..10000}; do echo $i;./test_bitcoin --run_test=addrman_tests || break; done
...
*** No errors detected
9999
Running 11 test cases...
*** No errors detected
10000
Running 11 test cases...
*** No errors detected

@EthanHeilman The same for me now. Strange. Will investigate.

Maybe it was a false alarm when I was debugging the net_tests problem - I could have removed MakeDeterministic() calls there...

I can't reproduce addrman_tests issue on plain master.

But anyway, the original problem is fixed by #8070. Thanks!

Fixed by #8070, closing.