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

e73d2a8018def940afadb5d699b18f39e882c1fc refactor: remove clientversion include from dbwrapper.h (Cory Fields)
4240a082b81d8ceb7615b1b4ca0d2857382f317b refactor: Use DataStream now that version/type are unused (Cory Fields)
f15f790618d328abd207d55e6291229eb2a8643f 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](https://github.com/theuni/bitcoin/commit/3e3af451652322c92e8e41cf918e69d608ec7c77). 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 e73d2a8018def940afadb5d699b18f39e882c1fc
  ajtowns:
    reACK e73d2a8018def940afadb5d699b18f39e882c1fc

Tree-SHA512: 45b0dd7c2e918493e2ee92a8e35320ad17991cb8908cb811150a96c5fd584ce177c775baeeb8675a602c90b9ba9203b8cefc0a2a0c6a71078b1d9c2b41e1f3ba

8 files changed, 28 insertions, 8 deletions

diff --git a/src/chain.h b/src/chain.h
index 2e1fb37bec..7806720ce9 100644
--- a/src/chain.h
+++ b/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();
-        if (!(s.GetType() & SER_GETHASH)) READWRITE(VARINT_MODE(_nVersion, VarIntMode::NONNEGATIVE_SIGNED));
+        int _nVersion = DUMMY_VERSION;
+        READWRITE(VARINT_MODE(_nVersion, VarIntMode::NONNEGATIVE_SIGNED));
 
         READWRITE(VARINT_MODE(obj.nHeight, VarIntMode::NONNEGATIVE_SIGNED));
         READWRITE(VARINT(obj.nStatus));
diff --git a/src/dbwrapper.h b/src/dbwrapper.h
index eac9594aa1..2f7448e878 100644
--- a/src/dbwrapper.h
+++ b/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&) {
diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
index b23d66ac1d..21132d9305 100644
--- a/src/index/blockfilterindex.cpp
+++ b/src/index/blockfilterindex.cpp
@@ -4,6 +4,7 @@
 
 #include <map>
 
+#include <clientversion.h>
 #include <common/args.h>
 #include <dbwrapper.h>
 #include <hash.h>
diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp
index 2e07a35d0d..0d4de3a53e 100644
--- a/src/index/txindex.cpp
+++ b/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>
diff --git a/src/init.cpp b/src/init.cpp
index 96fec92133..f0847bd4f7 100644
--- a/src/init.cpp
+++ b/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>
diff --git a/src/primitives/block.h b/src/primitives/block.h
index 861d362414..99accfc7dd 100644
--- a/src/primitives/block.h
+++ b/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();
-        if (!(s.GetType() & SER_GETHASH))
-            READWRITE(nVersion);
+        int nVersion = DUMMY_VERSION;
+        READWRITE(nVersion);
         READWRITE(obj.vHave);
     }
 
diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp
index 553bb31ba1..13cb1cc314 100644
--- a/src/test/blockmanager_tests.cpp
+++ b/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>
diff --git a/src/validation.cpp b/src/validation.cpp
index 0b6327ec55..e3a00e4241 100644
--- a/src/validation.cpp
+++ b/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>