Merge bitcoin/bitcoin#28428: Hard-code version number value for CBlockLocator and CDiskBlockIndex

e73d2a8018 refactor: remove clientversion include from dbwrapper.h (Cory Fields)
4240a082b8 refactor: Use DataStream now that version/type are unused (Cory Fields)
f15f790618 Remove version/hashing options from CBlockLocator/CDiskBlockIndex (Cory Fields)

Pull request description:

  This is also a much simpler replacement for #28327.

  There are version fields in `CBlockLocator` and `CDiskBlockIndex` that have always been written but discarded when read.

  I intended to convert them to use SerParams as introduced by #25284, which [ended up looking like this](3e3af45165). However because we don't currently have any definition of what a hash value would mean for either one of those, and we've never assigned the version field any meaning, I think it's better to just not worry about them.

  If we ever need to assign meaning in the future, we can introduce `SerParams` as was done for `CAddress`.

  As for the dummy values chosen:

  `CDiskBlockIndex::DUMMY_VERSION` was easy as the highest ever client version, and I don't expect any objection there.

  `CBlockLocator::DUMMY_VERSION` is hard-coded to the higest _PROTOCOL_ version ever used. This is to avoid a sudden bump that would be visible on the network if CLIENT_VERSION were used instead. In the future, if we ever need to use the value, we can discard anything in the CLIENT_VERSION range (for a few years as needed), as it's quite a bit higher.

  While reviewing, I suggest looking at the throwaway `SerParams` commit above as it shows where the call-sites are. I believe that should be enough to convince one's self that hashing is never used.

ACKs for top commit:
  TheCharlatan:
    Re-ACK e73d2a8018
  ajtowns:
    reACK e73d2a8018

Tree-SHA512: 45b0dd7c2e918493e2ee92a8e35320ad17991cb8908cb811150a96c5fd584ce177c775baeeb8675a602c90b9ba9203b8cefc0a2a0c6a71078b1d9c2b41e1f3ba

src/chain.h

@@ -388,6 +388,14 @@ const CBlockIndex* LastCommonAncestor(const CBlockIndex* pa, const CBlockIndex*
 /** Used to marshal pointers into hashes for db storage. */
 class CDiskBlockIndex : public CBlockIndex
 {
+    /** Historically CBlockLocator's version field has been written to disk
+     * streams as the client version, but the value has never been used.
+     *
+     * Hard-code to the highest client version ever written.
+     * SerParams can be used if the field requires any meaning in the future.
+     **/
+    static constexpr int DUMMY_VERSION = 259900;
+
 public:
     uint256 hashPrev;
 
@@ -404,8 +412,8 @@ public:
     SERIALIZE_METHODS(CDiskBlockIndex, obj)
     {
         LOCK(::cs_main);
-        int _nVersion = s.GetVersion();
+        int _nVersion = DUMMY_VERSION;
-        if (!(s.GetType() & SER_GETHASH)) READWRITE(VARINT_MODE(_nVersion, VarIntMode::NONNEGATIVE_SIGNED));
+        READWRITE(VARINT_MODE(_nVersion, VarIntMode::NONNEGATIVE_SIGNED));
 
         READWRITE(VARINT_MODE(obj.nHeight, VarIntMode::NONNEGATIVE_SIGNED));
         READWRITE(VARINT(obj.nStatus));

src/dbwrapper.h

@@ -6,7 +6,6 @@
 #define BITCOIN_DBWRAPPER_H
 
 #include <attributes.h>
-#include <clientversion.h>
 #include <serialize.h>
 #include <span.h>
 #include <streams.h>
@@ -167,7 +166,7 @@ public:
 
     template<typename V> bool GetValue(V& value) {
         try {
-            CDataStream ssValue{GetValueImpl(), SER_DISK, CLIENT_VERSION};
+            DataStream ssValue{GetValueImpl()};
             ssValue.Xor(dbwrapper_private::GetObfuscateKey(parent));
             ssValue >> value;
         } catch (const std::exception&) {
@@ -229,7 +228,7 @@ public:
             return false;
         }
         try {
-            CDataStream ssValue{MakeByteSpan(*strValue), SER_DISK, CLIENT_VERSION};
+            DataStream ssValue{MakeByteSpan(*strValue)};
             ssValue.Xor(obfuscate_key);
             ssValue >> value;
         } catch (const std::exception&) {

src/index/blockfilterindex.cpp

@@ -4,6 +4,7 @@
 
 #include <map>
 
+#include <clientversion.h>
 #include <common/args.h>
 #include <dbwrapper.h>
 #include <hash.h>

src/index/txindex.cpp

@@ -4,6 +4,7 @@
 
 #include <index/txindex.h>
 
+#include <clientversion.h>
 #include <common/args.h>
 #include <index/disktxpos.h>
 #include <logging.h>

src/init.cpp

@@ -19,6 +19,7 @@
 #include <chain.h>
 #include <chainparams.h>
 #include <chainparamsbase.h>
+#include <clientversion.h>
 #include <common/args.h>
 #include <common/system.h>
 #include <consensus/amount.h>

src/primitives/block.h

@@ -118,6 +118,15 @@ public:
  */
 struct CBlockLocator
 {
+    /** Historically CBlockLocator's version field has been written to network
+     * streams as the negotiated protocol version and to disk streams as the
+     * client version, but the value has never been used.
+     *
+     * Hard-code to the highest protocol version ever written to a network stream.
+     * SerParams can be used if the field requires any meaning in the future,
+     **/
+    static constexpr int DUMMY_VERSION = 70016;
+
     std::vector<uint256> vHave;
 
     CBlockLocator() {}
@@ -126,9 +135,8 @@ struct CBlockLocator
 
     SERIALIZE_METHODS(CBlockLocator, obj)
     {
-        int nVersion = s.GetVersion();
+        int nVersion = DUMMY_VERSION;
-        if (!(s.GetType() & SER_GETHASH))
+        READWRITE(nVersion);
-            READWRITE(nVersion);
         READWRITE(obj.vHave);
     }
 

src/test/blockmanager_tests.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <chainparams.h>
+#include <clientversion.h>
 #include <node/blockstorage.h>
 #include <node/context.h>
 #include <node/kernel_notifications.h>

src/validation.cpp

@@ -11,6 +11,7 @@
 #include <arith_uint256.h>
 #include <chain.h>
 #include <checkqueue.h>
+#include <clientversion.h>
 #include <consensus/amount.h>
 #include <consensus/consensus.h>
 #include <consensus/merkle.h>