Merge bitcoin/bitcoin#21969: refactor: Switch serialize to uint8_t (Bundle 1/2)

MarcoFalke · MarcoFalke · commit 5cf92c32d1ac · 2021-06-01T09:01:29.000+02:00
ffff0d0 refactor: Switch serialize to uint8_t (1/n) (MarcoFalke) Pull request description: Replace `char` -> `uint8_t` in serialization where a sign doesn't make sense (char might be signed/unsigned). ACKs for top commit: practicalswift: cr ACK ffff0d0: patch looks correct and commit hash is ffffresh (was bbbbadass) kristapsk: ACK ffff0d0 Tree-SHA512: cda682280c21d37cc3a6abd62569732079b31d18df3f157aa28bed80bd6f9f29a7db5c133b1f57b3a8f8d5ba181a76e473763c6e26a2df6d9244813f56f893ee
diff --git a/src/addrman.cpp b/src/addrman.cpp
@@ -36,7 +36,7 @@ int CAddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const std:
 
 int CAddrInfo::GetBucketPosition(const uint256 &nKey, bool fNew, int nBucket) const
 {
-    uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << (fNew ? 'N' : 'K') << nBucket << GetKey()).GetCheapHash();
+    uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << (fNew ? uint8_t{'N'} : uint8_t{'K'}) << nBucket << GetKey()).GetCheapHash();
     return hash1 % ADDRMAN_BUCKET_SIZE;
 }
 
@@ -652,7 +652,7 @@ std::vector<bool> CAddrMan::DecodeAsmap(fs::path path)
     int length = ftell(filestr);
     LogPrintf("Opened asmap file %s (%d bytes) from disk\n", path, length);
     fseek(filestr, 0, SEEK_SET);
-    char cur_byte;
+    uint8_t cur_byte;
     for (int i = 0; i < length; ++i) {
         file >> cur_byte;
         for (int bit = 0; bit < 8; ++bit) {
diff --git a/src/net.cpp b/src/net.cpp
@@ -3048,7 +3048,7 @@ void CaptureMessage(const CAddress& addr, const std::string& msg_type, const Spa
     ser_writedata64(f, now.count());
     f.write(msg_type.data(), msg_type.length());
     for (auto i = msg_type.length(); i < CMessageHeader::COMMAND_SIZE; ++i) {
-        f << '\0';
+        f << uint8_t{'\0'};
     }
     uint32_t size = data.size();
     ser_writedata32(f, size);
diff --git a/src/serialize.h b/src/serialize.h
@@ -226,12 +226,8 @@ template<typename Stream, int N> inline void Unserialize(Stream& s, char (&a)[N]
 template<typename Stream, int N> inline void Unserialize(Stream& s, unsigned char (&a)[N]) { s.read(CharCast(a), N); }
 template<typename Stream> inline void Unserialize(Stream& s, Span<unsigned char>& span) { s.read(CharCast(span.data()), span.size()); }
 
-template<typename Stream> inline void Serialize(Stream& s, bool a)    { char f=a; ser_writedata8(s, f); }
-template<typename Stream> inline void Unserialize(Stream& s, bool& a) { char f=ser_readdata8(s); a=f; }
-
-
-
-
+template <typename Stream> inline void Serialize(Stream& s, bool a) { uint8_t f = a; ser_writedata8(s, f); }
+template <typename Stream> inline void Unserialize(Stream& s, bool& a) { uint8_t f = ser_readdata8(s); a = f; }
 
 
 /**
diff --git a/src/test/dbwrapper_tests.cpp b/src/test/dbwrapper_tests.cpp
@@ -28,7 +28,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper)
     for (const bool obfuscate : {false, true}) {
         fs::path ph = m_args.GetDataDirBase() / (obfuscate ? "dbwrapper_obfuscate_true" : "dbwrapper_obfuscate_false");
         CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate);
-        char key = 'k';
+        uint8_t key{'k'};
         uint256 in = InsecureRand256();
         uint256 res;
 
@@ -88,21 +88,21 @@ BOOST_AUTO_TEST_CASE(dbwrapper_basic_data)
         BOOST_CHECK_EQUAL(res.ToString(), in_utxo.ToString());
 
         //Simulate last block file number - "l"
-        char key_last_blockfile_number = 'l';
+        uint8_t key_last_blockfile_number{'l'};
         uint32_t lastblockfilenumber = InsecureRand32();
         BOOST_CHECK(dbw.Write(key_last_blockfile_number, lastblockfilenumber));
         BOOST_CHECK(dbw.Read(key_last_blockfile_number, res_uint_32));
         BOOST_CHECK_EQUAL(lastblockfilenumber, res_uint_32);
 
         //Simulate Is Reindexing - "R"
-        char key_IsReindexing = 'R';
+        uint8_t key_IsReindexing{'R'};
         bool isInReindexing = InsecureRandBool();
         BOOST_CHECK(dbw.Write(key_IsReindexing, isInReindexing));
         BOOST_CHECK(dbw.Read(key_IsReindexing, res_bool));
         BOOST_CHECK_EQUAL(isInReindexing, res_bool);
 
         //Simulate last block hash up to which UXTO covers - 'B'
-        char key_lastblockhash_uxto = 'B';
+        uint8_t key_lastblockhash_uxto{'B'};
         uint256 lastblock_hash = InsecureRand256();
         BOOST_CHECK(dbw.Write(key_lastblockhash_uxto, lastblock_hash));
         BOOST_CHECK(dbw.Read(key_lastblockhash_uxto, res));
@@ -129,11 +129,11 @@ BOOST_AUTO_TEST_CASE(dbwrapper_batch)
         fs::path ph = m_args.GetDataDirBase() / (obfuscate ? "dbwrapper_batch_obfuscate_true" : "dbwrapper_batch_obfuscate_false");
         CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate);
 
-        char key = 'i';
+        uint8_t key{'i'};
         uint256 in = InsecureRand256();
-        char key2 = 'j';
+        uint8_t key2{'j'};
         uint256 in2 = InsecureRand256();
-        char key3 = 'k';
+        uint8_t key3{'k'};
         uint256 in3 = InsecureRand256();
 
         uint256 res;
@@ -166,10 +166,10 @@ BOOST_AUTO_TEST_CASE(dbwrapper_iterator)
         CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate);
 
         // The two keys are intentionally chosen for ordering
-        char key = 'j';
+        uint8_t key{'j'};
         uint256 in = InsecureRand256();
         BOOST_CHECK(dbw.Write(key, in));
-        char key2 = 'k';
+        uint8_t key2{'k'};
         uint256 in2 = InsecureRand256();
         BOOST_CHECK(dbw.Write(key2, in2));
 
@@ -178,7 +178,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_iterator)
         // Be sure to seek past the obfuscation key (if it exists)
         it->Seek(key);
 
-        char key_res;
+        uint8_t key_res;
         uint256 val_res;
 
         BOOST_REQUIRE(it->GetKey(key_res));
@@ -207,7 +207,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate)
 
     // Set up a non-obfuscated wrapper to write some initial data.
     std::unique_ptr<CDBWrapper> dbw = std::make_unique<CDBWrapper>(ph, (1 << 10), false, false, false);
-    char key = 'k';
+    uint8_t key{'k'};
     uint256 in = InsecureRand256();
     uint256 res;
 
@@ -248,7 +248,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex)
 
     // Set up a non-obfuscated wrapper to write some initial data.
     std::unique_ptr<CDBWrapper> dbw = std::make_unique<CDBWrapper>(ph, (1 << 10), false, false, false);
-    char key = 'k';
+    uint8_t key{'k'};
     uint256 in = InsecureRand256();
     uint256 res;
 
@@ -334,15 +334,15 @@ struct StringContentsSerializer {
     void Serialize(Stream& s) const
     {
         for (size_t i = 0; i < str.size(); i++) {
-            s << str[i];
+            s << uint8_t(str[i]);
         }
     }
 
     template<typename Stream>
     void Unserialize(Stream& s)
     {
         str.clear();
-        char c = 0;
+        uint8_t c{0};
         while (true) {
             try {
                 s >> c;
diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp
@@ -24,7 +24,7 @@ class CSerializeMethodsTestSingle
     CTransactionRef txval;
 public:
     CSerializeMethodsTestSingle() = default;
-    CSerializeMethodsTestSingle(int intvalin, bool boolvalin, std::string stringvalin, const char* charstrvalin, const CTransactionRef& txvalin) : intval(intvalin), boolval(boolvalin), stringval(std::move(stringvalin)), txval(txvalin)
+    CSerializeMethodsTestSingle(int intvalin, bool boolvalin, std::string stringvalin, const uint8_t* charstrvalin, const CTransactionRef& txvalin) : intval(intvalin), boolval(boolvalin), stringval(std::move(stringvalin)), txval(txvalin)
     {
         memcpy(charstrval, charstrvalin, sizeof(charstrval));
     }
@@ -70,8 +70,8 @@ BOOST_AUTO_TEST_CASE(sizes)
     BOOST_CHECK_EQUAL(sizeof(uint32_t), GetSerializeSize(uint32_t(0), 0));
     BOOST_CHECK_EQUAL(sizeof(int64_t), GetSerializeSize(int64_t(0), 0));
     BOOST_CHECK_EQUAL(sizeof(uint64_t), GetSerializeSize(uint64_t(0), 0));
-    // Bool is serialized as char
-    BOOST_CHECK_EQUAL(sizeof(char), GetSerializeSize(bool(0), 0));
+    // Bool is serialized as uint8_t
+    BOOST_CHECK_EQUAL(sizeof(uint8_t), GetSerializeSize(bool(0), 0));
 
     // Sanity-check GetSerializeSize and c++ type matching
     BOOST_CHECK_EQUAL(GetSerializeSize(char(0), 0), 1U);
@@ -263,7 +263,7 @@ BOOST_AUTO_TEST_CASE(class_methods)
     int intval(100);
     bool boolval(true);
     std::string stringval("testing");
-    const char charstrval[16] = "testing charstr";
+    const uint8_t charstrval[16]{"testing charstr"};
     CMutableTransaction txval;
     CTransactionRef tx_ref{MakeTransactionRef(txval)};
     CSerializeMethodsTestSingle methodtest1(intval, boolval, stringval, charstrval, tx_ref);
diff --git a/src/txdb.cpp b/src/txdb.cpp
@@ -16,22 +16,22 @@
 
 #include <stdint.h>
 
-static const char DB_COIN = 'C';
-static const char DB_COINS = 'c';
-static const char DB_BLOCK_FILES = 'f';
-static const char DB_BLOCK_INDEX = 'b';
+static constexpr uint8_t DB_COIN{'C'};
+static constexpr uint8_t DB_COINS{'c'};
+static constexpr uint8_t DB_BLOCK_FILES{'f'};
+static constexpr uint8_t DB_BLOCK_INDEX{'b'};
 
-static const char DB_BEST_BLOCK = 'B';
-static const char DB_HEAD_BLOCKS = 'H';
-static const char DB_FLAG = 'F';
-static const char DB_REINDEX_FLAG = 'R';
-static const char DB_LAST_BLOCK = 'l';
+static constexpr uint8_t DB_BEST_BLOCK{'B'};
+static constexpr uint8_t DB_HEAD_BLOCKS{'H'};
+static constexpr uint8_t DB_FLAG{'F'};
+static constexpr uint8_t DB_REINDEX_FLAG{'R'};
+static constexpr uint8_t DB_LAST_BLOCK{'l'};
 
 namespace {
 
 struct CoinEntry {
     COutPoint* outpoint;
-    char key;
+    uint8_t key;
     explicit CoinEntry(const COutPoint* ptr) : outpoint(const_cast<COutPoint*>(ptr)), key(DB_COIN)  {}
 
     SERIALIZE_METHODS(CoinEntry, obj) { READWRITE(obj.key, obj.outpoint->hash, VARINT(obj.outpoint->n)); }
@@ -143,7 +143,7 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) {
 
 size_t CCoinsViewDB::EstimateSize() const
 {
-    return m_db->EstimateSize(DB_COIN, (char)(DB_COIN+1));
+    return m_db->EstimateSize(DB_COIN, uint8_t(DB_COIN + 1));
 }
 
 CBlockTreeDB::CBlockTreeDB(size_t nCacheSize, bool fMemory, bool fWipe) : CDBWrapper(gArgs.GetDataDirNet() / "blocks" / "index", nCacheSize, fMemory, fWipe) {
@@ -155,7 +155,7 @@ bool CBlockTreeDB::ReadBlockFileInfo(int nFile, CBlockFileInfo &info) {
 
 bool CBlockTreeDB::WriteReindexing(bool fReindexing) {
     if (fReindexing)
-        return Write(DB_REINDEX_FLAG, '1');
+        return Write(DB_REINDEX_FLAG, uint8_t{'1'});
     else
         return Erase(DB_REINDEX_FLAG);
 }
@@ -235,14 +235,14 @@ bool CBlockTreeDB::WriteBatchSync(const std::vector<std::pair<int, const CBlockF
 }
 
 bool CBlockTreeDB::WriteFlag(const std::string &name, bool fValue) {
-    return Write(std::make_pair(DB_FLAG, name), fValue ? '1' : '0');
+    return Write(std::make_pair(DB_FLAG, name), fValue ? uint8_t{'1'} : uint8_t{'0'});
 }
 
 bool CBlockTreeDB::ReadFlag(const std::string &name, bool &fValue) {
-    char ch;
+    uint8_t ch;
     if (!Read(std::make_pair(DB_FLAG, name), ch))
         return false;
-    fValue = ch == '1';
+    fValue = ch == uint8_t{'1'};
     return true;
 }
 
@@ -255,7 +255,7 @@ bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams,
     // Load m_block_index
     while (pcursor->Valid()) {
         if (ShutdownRequested()) return false;
-        std::pair<char, uint256> key;
+        std::pair<uint8_t, uint256> key;
         if (pcursor->GetKey(key) && key.first == DB_BLOCK_INDEX) {
             CDiskBlockIndex diskindex;
             if (pcursor->GetValue(diskindex)) {
diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h
@@ -204,8 +204,8 @@ class CWalletTx
             mapValueCopy["timesmart"] = strprintf("%u", nTimeSmart);
         }
 
-        std::vector<char> dummy_vector1; //!< Used to be vMerkleBranch
-        std::vector<char> dummy_vector2; //!< Used to be vtxPrev
+        std::vector<uint8_t> dummy_vector1; //!< Used to be vMerkleBranch
+        std::vector<uint8_t> dummy_vector2; //!< Used to be vtxPrev
         bool dummy_bool = false; //!< Used to be fSpent
         uint256 serializedHash = isAbandoned() ? ABANDON_HASH : m_confirm.hashBlock;
         int serializedIndex = isAbandoned() || isConflicted() ? -1 : m_confirm.nIndex;
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
@@ -155,7 +155,7 @@ bool WalletBatch::WriteWatchOnly(const CScript &dest, const CKeyMetadata& keyMet
     if (!WriteIC(std::make_pair(DBKeys::WATCHMETA, dest), keyMeta)) {
         return false;
     }
-    return WriteIC(std::make_pair(DBKeys::WATCHS, dest), '1');
+    return WriteIC(std::make_pair(DBKeys::WATCHS, dest), uint8_t{'1'});
 }
 
 bool WalletBatch::EraseWatchOnly(const CScript &dest)
@@ -308,8 +308,8 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
                 {
                     if (!ssValue.empty())
                     {
-                        char fTmp;
-                        char fUnused;
+                        uint8_t fTmp;
+                        uint8_t fUnused;
                         std::string unused_string;
                         ssValue >> fTmp >> fUnused >> unused_string;
                         strErr = strprintf("LoadWallet() upgrading tx ver=%d %d %s",
@@ -336,7 +336,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
             wss.nWatchKeys++;
             CScript script;
             ssKey >> script;
-            char fYes;
+            uint8_t fYes;
             ssValue >> fYes;
             if (fYes == '1') {
                 pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadWatchOnly(script);

Original file line number	Diff line number	Diff line change
`@@ -3048,7 +3048,7 @@ void CaptureMessage(const CAddress& addr, const std::string& msg_type, const Spa`
`3048`	`3048`	`ser_writedata64(f, now.count());`
`3049`	`3049`	`f.write(msg_type.data(), msg_type.length());`
`3050`	`3050`	`for (auto i = msg_type.length(); i < CMessageHeader::COMMAND_SIZE; ++i) {`
`3051`		`- f << '\0';`
	`3051`	`+ f << uint8_t{'\0'};`
`3052`	`3052`	`}`
`3053`	`3053`	`uint32_t size = data.size();`
`3054`	`3054`	`ser_writedata32(f, size);`