Merge bitcoin/bitcoin#28428: Hard-code version number value for CBloc…

…kLocator and CDiskBlockIndex

e73d2a8 refactor: remove clientversion include from dbwrapper.h (Cory Fields)
4240a08 refactor: Use DataStream now that version/type are unused (Cory Fields)
f15f790 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](theuni/bitcoin@3e3af45). 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 e73d2a8
  ajtowns:
    reACK e73d2a8

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 @@ class CDiskBlockIndex : public CBlockIndex
  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));

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 @@ class CDBIterator
 
  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 @@ class CDBWrapper
  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 @@ class CBlock : public CBlockHeader
  */
 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);
  }
 

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>