partial Merge bitcoin#18216: test, build: Enable -Werror=sign-compare

6853727 build: Enable -Werror=sign-compare (Ben Woosley)
eac6a30 refactor: Rework asmap Interpret to avoid ptrdiff_t (Ben Woosley)
df37377 test: Fix outstanding -Wsign-compare errors (Ben Woosley)

Pull request description:

  Disallowing sign-comparison mismatches can help to prevent the introduction of overflow and interpretation bugs.

  In this case, ~all~ most existing violations are in the tests, and most simply required annotating the literal as unsigned for comparison.

  This was previously prevented by violations in leveldb which were fixed upstream and merged in bitcoin#17398. You can test that by building this branch against: 22d1118 vs 75fb37c

ACKs for top commit:
  fjahr:
    re-ACK 6853727
  practicalswift:
    ACK 6853727

Tree-SHA512: 14b5daa38c496fb51548feb30fb4dd179e6f76a8d355f52bc8e2a18f2f9340f0bc98dcf36d8b3d6521045d013891c3103749a4eda88ceef00202a6a0cf93f73c

configure.ac

@@ -392,6 +392,7 @@ if test "x$enable_werror" = "xyes"; then
   AX_CHECK_COMPILE_FLAG([-Werror=date-time],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=date-time"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Werror=return-type],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=return-type"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Werror=conditional-uninitialized],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=conditional-uninitialized"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Werror=sign-compare],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=sign-compare"],,[[$CXXFLAG_WERROR]])
   dnl -Wsuggest-override is broken with GCC before 9.2
   dnl https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78010
   AX_CHECK_COMPILE_FLAG([-Werror=suggest-override],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=suggest-override"],,[[$CXXFLAG_WERROR]],
@@ -411,6 +412,7 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
   AX_CHECK_COMPILE_FLAG([-Wunused-variable],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-variable"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wdate-time],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdate-time"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wconditional-uninitialized],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wconditional-uninitialized"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wsign-compare],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsign-compare"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wsuggest-override],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsuggest-override"],,[[$CXXFLAG_WERROR]],
                         [AC_LANG_SOURCE([[struct A { virtual void f(); }; struct B : A { void f() final; };]])])
 

src/test/blockfilter_index_tests.cpp

@@ -53,8 +53,8 @@ static bool CheckFilterLookups(BlockFilterIndex& filter_index, const CBlockIndex
     BOOST_CHECK(filter_index.LookupFilterHashRange(block_index->nHeight, block_index,
                                                    filter_hashes));
 
-    BOOST_CHECK_EQUAL(filters.size(), 1);
-    BOOST_CHECK_EQUAL(filter_hashes.size(), 1);
+    BOOST_CHECK_EQUAL(filters.size(), 1U);
+    BOOST_CHECK_EQUAL(filter_hashes.size(), 1U);
 
     BOOST_CHECK_EQUAL(filter.GetHash(), expected_filter.GetHash());
     BOOST_CHECK_EQUAL(filter_header, expected_filter.ComputeHeader(last_header));
@@ -255,8 +255,9 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, TestChain100Setup)
     BOOST_CHECK(filter_index.LookupFilterRange(0, tip, filters));
     BOOST_CHECK(filter_index.LookupFilterHashRange(0, tip, filter_hashes));
 
-    BOOST_CHECK_EQUAL(filters.size(), tip->nHeight + 1);
-    BOOST_CHECK_EQUAL(filter_hashes.size(), tip->nHeight + 1);
+    assert(tip->nHeight >= 0);
+    BOOST_CHECK_EQUAL(filters.size(), tip->nHeight + 1U);
+    BOOST_CHECK_EQUAL(filter_hashes.size(), tip->nHeight + 1U);
 
     filters.clear();
     filter_hashes.clear();

src/test/blockfilter_tests.cpp

@@ -42,14 +42,14 @@ BOOST_AUTO_TEST_CASE(gcsfilter_test)
 BOOST_AUTO_TEST_CASE(gcsfilter_default_constructor)
 {
     GCSFilter filter;
-    BOOST_CHECK_EQUAL(filter.GetN(), 0);
-    BOOST_CHECK_EQUAL(filter.GetEncoded().size(), 1);
+    BOOST_CHECK_EQUAL(filter.GetN(), 0U);
+    BOOST_CHECK_EQUAL(filter.GetEncoded().size(), 1U);
 
     const GCSFilter::Params& params = filter.GetParams();
-    BOOST_CHECK_EQUAL(params.m_siphash_k0, 0);
-    BOOST_CHECK_EQUAL(params.m_siphash_k1, 0);
+    BOOST_CHECK_EQUAL(params.m_siphash_k0, 0U);
+    BOOST_CHECK_EQUAL(params.m_siphash_k1, 0U);
     BOOST_CHECK_EQUAL(params.m_P, 0);
-    BOOST_CHECK_EQUAL(params.m_M, 1);
+    BOOST_CHECK_EQUAL(params.m_M, 1U);
 }
 
 BOOST_AUTO_TEST_CASE(blockfilter_basic_test)

src/test/bloom_tests.cpp

@@ -589,7 +589,7 @@ BOOST_AUTO_TEST_CASE(rolling_bloom)
             ++nHits;
     }
     // Expect about 100 hits
-    BOOST_CHECK_EQUAL(nHits, 75);
+    BOOST_CHECK_EQUAL(nHits, 75U);
 
     BOOST_CHECK(rb1.contains(data[DATASIZE-1]));
     rb1.reset();
@@ -617,7 +617,7 @@ BOOST_AUTO_TEST_CASE(rolling_bloom)
             ++nHits;
     }
     // Expect about 5 false positives
-    BOOST_CHECK_EQUAL(nHits, 6);
+    BOOST_CHECK_EQUAL(nHits, 6U);
 
     // last-1000-entry, 0.01% false positive:
     CRollingBloomFilter rb2(1000, 0.001);

src/test/flatfile_tests.cpp

@@ -90,16 +90,16 @@ BOOST_AUTO_TEST_CASE(flatfile_allocate)
 
     bool out_of_space;
 
-    BOOST_CHECK_EQUAL(seq.Allocate(FlatFilePos(0, 0), 1, out_of_space), 100);
-    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 0))), 100);
+    BOOST_CHECK_EQUAL(seq.Allocate(FlatFilePos(0, 0), 1, out_of_space), 100U);
+    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 0))), 100U);
     BOOST_CHECK(!out_of_space);
 
-    BOOST_CHECK_EQUAL(seq.Allocate(FlatFilePos(0, 99), 1, out_of_space), 0);
-    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 99))), 100);
+    BOOST_CHECK_EQUAL(seq.Allocate(FlatFilePos(0, 99), 1, out_of_space), 0U);
+    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 99))), 100U);
     BOOST_CHECK(!out_of_space);
 
-    BOOST_CHECK_EQUAL(seq.Allocate(FlatFilePos(0, 99), 2, out_of_space), 101);
-    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 99))), 200);
+    BOOST_CHECK_EQUAL(seq.Allocate(FlatFilePos(0, 99), 2, out_of_space), 101U);
+    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 99))), 200U);
     BOOST_CHECK(!out_of_space);
 }
 
@@ -113,11 +113,11 @@ BOOST_AUTO_TEST_CASE(flatfile_flush)
 
     // Flush without finalize should not truncate file.
     seq.Flush(FlatFilePos(0, 1));
-    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 1))), 100);
+    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 1))), 100U);
 
     // Flush with finalize should truncate file.
     seq.Flush(FlatFilePos(0, 1), true);
-    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 1))), 1);
+    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 1))), 1U);
 }
 
 BOOST_AUTO_TEST_SUITE_END()

src/test/netbase_tests.cpp

@@ -520,7 +520,7 @@ BOOST_AUTO_TEST_CASE(netpermissions_test)
     BOOST_CHECK(NetWhitelistPermissions::TryParse("bloom,forcerelay,noban,relay,mempool@1.2.3.4/32", whitelistPermissions, error));
 
     const auto strings = NetPermissions::ToStrings(PF_ALL);
-    BOOST_CHECK_EQUAL(strings.size(), 5);
+    BOOST_CHECK_EQUAL(strings.size(), 5U);
     BOOST_CHECK(std::find(strings.begin(), strings.end(), "bloomfilter") != strings.end());
     BOOST_CHECK(std::find(strings.begin(), strings.end(), "forcerelay") != strings.end());
     BOOST_CHECK(std::find(strings.begin(), strings.end(), "relay") != strings.end());

src/test/random_tests.cpp

@@ -128,7 +128,7 @@ BOOST_AUTO_TEST_CASE(shuffle_stat_test)
     }
     BOOST_CHECK(chi_score > 58.1411); // 99.9999% confidence interval
     BOOST_CHECK(chi_score < 210.275);
-    BOOST_CHECK_EQUAL(sum, 12000);
+    BOOST_CHECK_EQUAL(sum, 12000U);
 }
 
 BOOST_AUTO_TEST_SUITE_END()

src/test/streams_tests.cpp

@@ -74,28 +74,28 @@ BOOST_AUTO_TEST_CASE(streams_vector_reader)
     std::vector<unsigned char> vch = {1, 255, 3, 4, 5, 6};
 
     VectorReader reader(SER_NETWORK, INIT_PROTO_VERSION, vch, 0);
-    BOOST_CHECK_EQUAL(reader.size(), 6);
+    BOOST_CHECK_EQUAL(reader.size(), 6U);
     BOOST_CHECK(!reader.empty());
 
     // Read a single byte as an unsigned char.
     unsigned char a;
     reader >> a;
     BOOST_CHECK_EQUAL(a, 1);
-    BOOST_CHECK_EQUAL(reader.size(), 5);
+    BOOST_CHECK_EQUAL(reader.size(), 5U);
     BOOST_CHECK(!reader.empty());
 
     // Read a single byte as a signed char.
     signed char b;
     reader >> b;
     BOOST_CHECK_EQUAL(b, -1);
-    BOOST_CHECK_EQUAL(reader.size(), 4);
+    BOOST_CHECK_EQUAL(reader.size(), 4U);
     BOOST_CHECK(!reader.empty());
 
     // Read a 4 bytes as an unsigned int.
     unsigned int c;
     reader >> c;
-    BOOST_CHECK_EQUAL(c, 100992003); // 3,4,5,6 in little-endian base-256
-    BOOST_CHECK_EQUAL(reader.size(), 0);
+    BOOST_CHECK_EQUAL(c, 100992003U); // 3,4,5,6 in little-endian base-256
+    BOOST_CHECK_EQUAL(reader.size(), 0U);
     BOOST_CHECK(reader.empty());
 
     // Reading after end of byte vector throws an error.
@@ -106,7 +106,7 @@ BOOST_AUTO_TEST_CASE(streams_vector_reader)
     VectorReader new_reader(SER_NETWORK, INIT_PROTO_VERSION, vch, 0);
     new_reader >> d;
     BOOST_CHECK_EQUAL(d, 67370753); // 1,255,3,4 in little-endian base-256
-    BOOST_CHECK_EQUAL(new_reader.size(), 2);
+    BOOST_CHECK_EQUAL(new_reader.size(), 2U);
     BOOST_CHECK(!new_reader.empty());
 
     // Reading after end of byte vector throws an error even if the reader is
@@ -149,14 +149,14 @@ BOOST_AUTO_TEST_CASE(bitstream_reader_writer)
     BOOST_CHECK_EQUAL(serialized_int2, (uint16_t)0x1072); // NOTE: Serialized as LE
 
     BitStreamReader<CDataStream> bit_reader(data_copy);
-    BOOST_CHECK_EQUAL(bit_reader.Read(1), 0);
-    BOOST_CHECK_EQUAL(bit_reader.Read(2), 2);
-    BOOST_CHECK_EQUAL(bit_reader.Read(3), 6);
-    BOOST_CHECK_EQUAL(bit_reader.Read(4), 11);
-    BOOST_CHECK_EQUAL(bit_reader.Read(5), 1);
-    BOOST_CHECK_EQUAL(bit_reader.Read(6), 32);
-    BOOST_CHECK_EQUAL(bit_reader.Read(7), 7);
-    BOOST_CHECK_EQUAL(bit_reader.Read(16), 30497);
+    BOOST_CHECK_EQUAL(bit_reader.Read(1), 0U);
+    BOOST_CHECK_EQUAL(bit_reader.Read(2), 2U);
+    BOOST_CHECK_EQUAL(bit_reader.Read(3), 6U);
+    BOOST_CHECK_EQUAL(bit_reader.Read(4), 11U);
+    BOOST_CHECK_EQUAL(bit_reader.Read(5), 1U);
+    BOOST_CHECK_EQUAL(bit_reader.Read(6), 32U);
+    BOOST_CHECK_EQUAL(bit_reader.Read(7), 7U);
+    BOOST_CHECK_EQUAL(bit_reader.Read(16), 30497U);
     BOOST_CHECK_THROW(bit_reader.Read(8), std::ios_base::failure);
 }
 
@@ -249,7 +249,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
     BOOST_CHECK_EQUAL(i, 1);
 
     // After reading bytes 0 and 1, we're positioned at 2.
-    BOOST_CHECK_EQUAL(bf.GetPos(), 2);
+    BOOST_CHECK_EQUAL(bf.GetPos(), 2U);
 
     // Rewind to offset 0, ok (within the 10 byte window).
     BOOST_CHECK(bf.SetPos(0));
@@ -276,18 +276,18 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
     // The default argument removes the limit completely.
     BOOST_CHECK(bf.SetLimit());
     // The read position should still be at 3 (no change).
-    BOOST_CHECK_EQUAL(bf.GetPos(), 3);
+    BOOST_CHECK_EQUAL(bf.GetPos(), 3U);
 
     // Read from current offset, 3, forward until position 10.
     for (uint8_t j = 3; j < 10; ++j) {
         bf >> i;
         BOOST_CHECK_EQUAL(i, j);
     }
-    BOOST_CHECK_EQUAL(bf.GetPos(), 10);
+    BOOST_CHECK_EQUAL(bf.GetPos(), 10U);
 
     // We're guaranteed (just barely) to be able to rewind to zero.
     BOOST_CHECK(bf.SetPos(0));
-    BOOST_CHECK_EQUAL(bf.GetPos(), 0);
+    BOOST_CHECK_EQUAL(bf.GetPos(), 0U);
     bf >> i;
     BOOST_CHECK_EQUAL(i, 0);
 
@@ -297,12 +297,12 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
     BOOST_CHECK(bf.SetPos(10));
     bf >> i;
     BOOST_CHECK_EQUAL(i, 10);
-    BOOST_CHECK_EQUAL(bf.GetPos(), 11);
+    BOOST_CHECK_EQUAL(bf.GetPos(), 11U);
 
     // Now it's only guaranteed that we can rewind to offset 1
     // (current read position, 11, minus rewind amount, 10).
     BOOST_CHECK(bf.SetPos(1));
-    BOOST_CHECK_EQUAL(bf.GetPos(), 1);
+    BOOST_CHECK_EQUAL(bf.GetPos(), 1U);
     bf >> i;
     BOOST_CHECK_EQUAL(i, 1);
 
@@ -316,7 +316,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
             BOOST_CHECK_EQUAL(a[j], 11 + j);
         }
     }
-    BOOST_CHECK_EQUAL(bf.GetPos(), 40);
+    BOOST_CHECK_EQUAL(bf.GetPos(), 40U);
 
     // We've read the entire file, the next read should throw.
     try {
@@ -330,11 +330,11 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
     BOOST_CHECK(bf.eof());
 
     // Still at offset 40, we can go back 10, to 30.
-    BOOST_CHECK_EQUAL(bf.GetPos(), 40);
+    BOOST_CHECK_EQUAL(bf.GetPos(), 40U);
     BOOST_CHECK(bf.SetPos(30));
     bf >> i;
     BOOST_CHECK_EQUAL(i, 30);
-    BOOST_CHECK_EQUAL(bf.GetPos(), 31);
+    BOOST_CHECK_EQUAL(bf.GetPos(), 31U);
 
     // We're too far to rewind to position zero.
     BOOST_CHECK(!bf.SetPos(0));

src/test/util_tests.cpp

@@ -1246,24 +1246,24 @@ BOOST_AUTO_TEST_CASE(test_tracked_vector)
     BOOST_CHECK(t3.origin == &t3);
 
     auto v1 = Vector(t1);
-    BOOST_CHECK_EQUAL(v1.size(), 1);
+    BOOST_CHECK_EQUAL(v1.size(), 1U);
     BOOST_CHECK(v1[0].origin == &t1);
     BOOST_CHECK_EQUAL(v1[0].copies, 1);
 
     auto v2 = Vector(std::move(t2));
-    BOOST_CHECK_EQUAL(v2.size(), 1);
+    BOOST_CHECK_EQUAL(v2.size(), 1U);
     BOOST_CHECK(v2[0].origin == &t2);
     BOOST_CHECK_EQUAL(v2[0].copies, 0);
 
     auto v3 = Vector(t1, std::move(t2));
-    BOOST_CHECK_EQUAL(v3.size(), 2);
+    BOOST_CHECK_EQUAL(v3.size(), 2U);
     BOOST_CHECK(v3[0].origin == &t1);
     BOOST_CHECK(v3[1].origin == &t2);
     BOOST_CHECK_EQUAL(v3[0].copies, 1);
     BOOST_CHECK_EQUAL(v3[1].copies, 0);
 
     auto v4 = Vector(std::move(v3[0]), v3[1], std::move(t3));
-    BOOST_CHECK_EQUAL(v4.size(), 3);
+    BOOST_CHECK_EQUAL(v4.size(), 3U);
     BOOST_CHECK(v4[0].origin == &t1);
     BOOST_CHECK(v4[1].origin == &t2);
     BOOST_CHECK(v4[2].origin == &t3);
@@ -1272,7 +1272,7 @@ BOOST_AUTO_TEST_CASE(test_tracked_vector)
     BOOST_CHECK_EQUAL(v4[2].copies, 0);
 
     auto v5 = Cat(v1, v4);
-    BOOST_CHECK_EQUAL(v5.size(), 4);
+    BOOST_CHECK_EQUAL(v5.size(), 4U);
     BOOST_CHECK(v5[0].origin == &t1);
     BOOST_CHECK(v5[1].origin == &t1);
     BOOST_CHECK(v5[2].origin == &t2);
@@ -1283,7 +1283,7 @@ BOOST_AUTO_TEST_CASE(test_tracked_vector)
     BOOST_CHECK_EQUAL(v5[3].copies, 1);
 
     auto v6 = Cat(std::move(v1), v3);
-    BOOST_CHECK_EQUAL(v6.size(), 3);
+    BOOST_CHECK_EQUAL(v6.size(), 3U);
     BOOST_CHECK(v6[0].origin == &t1);
     BOOST_CHECK(v6[1].origin == &t1);
     BOOST_CHECK(v6[2].origin == &t2);
@@ -1292,7 +1292,7 @@ BOOST_AUTO_TEST_CASE(test_tracked_vector)
     BOOST_CHECK_EQUAL(v6[2].copies, 1);
 
     auto v7 = Cat(v2, std::move(v4));
-    BOOST_CHECK_EQUAL(v7.size(), 4);
+    BOOST_CHECK_EQUAL(v7.size(), 4U);
     BOOST_CHECK(v7[0].origin == &t2);
     BOOST_CHECK(v7[1].origin == &t1);
     BOOST_CHECK(v7[2].origin == &t2);
@@ -1303,7 +1303,7 @@ BOOST_AUTO_TEST_CASE(test_tracked_vector)
     BOOST_CHECK_EQUAL(v7[3].copies, 0);
 
     auto v8 = Cat(std::move(v2), std::move(v3));
-    BOOST_CHECK_EQUAL(v8.size(), 3);
+    BOOST_CHECK_EQUAL(v8.size(), 3U);
     BOOST_CHECK(v8[0].origin == &t2);
     BOOST_CHECK(v8[1].origin == &t1);
     BOOST_CHECK(v8[2].origin == &t2);

src/test/util_threadnames_tests.cpp

@@ -61,7 +61,7 @@ BOOST_AUTO_TEST_CASE(util_threadnames_test_rename_threaded)
 
     std::set<std::string> names = RenameEnMasse(100);
 
-    BOOST_CHECK_EQUAL(names.size(), 100);
+    BOOST_CHECK_EQUAL(names.size(), 100U);
 
     // Names "test_thread.[n]" should exist for n = [0, 99]
     for (int i = 0; i < 100; ++i) {

src/wallet/test/wallet_tests.cpp

@@ -866,9 +866,9 @@ BOOST_FIXTURE_TEST_CASE(select_coins_grouped_by_addresses, ListCoinsTestingSetup
     {
         LOCK(wallet->cs_wallet);
         const auto& conflicts = wallet->GetConflicts(tx2->GetHash());
-        BOOST_CHECK_EQUAL(conflicts.size(), 2);
-        BOOST_CHECK_EQUAL(conflicts.count(tx1->GetHash()), 1);
-        BOOST_CHECK_EQUAL(conflicts.count(tx2->GetHash()), 1);
+        BOOST_CHECK_EQUAL(conflicts.size(), 2U);
+        BOOST_CHECK_EQUAL(conflicts.count(tx1->GetHash()), 1U);
+        BOOST_CHECK_EQUAL(conflicts.count(tx2->GetHash()), 1U);
     }
 
     // Committed tx is the one that should be marked as "conflicting".