Hard-code version number value for CBlockLocator and CDiskBlockIndex

theuni commented at 3:58 pm on September 7, 2023: member

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. 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.

DrahtBot commented at 3:58 pm on September 7, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, ajtowns
Stale ACK	sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28438 (Use serialization parameters for CTransaction by ajtowns)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

theuni commented at 3:59 pm on September 7, 2023: member

Ping @sipa @MarcoFalke

sipa commented at 4:41 pm on September 7, 2023: member

utACK c1aa0af69eee05a9fad1eabd45662fc2143a3e0c

ajtowns commented at 7:38 pm on September 7, 2023: contributor

utACK c1aa0af69eee05a9fad1eabd45662fc2143a3e0c

in src/chain.h:396 in c1aa0af69e outdated

387@@ -388,6 +388,13 @@ const CBlockIndex* LastCommonAncestor(const CBlockIndex* pa, const CBlockIndex*
388 /** Used to marshal pointers into hashes for db storage. */
389 class CDiskBlockIndex : public CBlockIndex
390 {
391+    /** Historically CBlockLocator's version field has been written to disk
392+     * streams as the client version, but the value has never been used.
393+     *
394+     * Hard-code to the highest client version ever written.
395+     * SerParams can be used if the field requires any meaning in the future.
396+    **/

TheCharlatan commented at 9:02 am on September 8, 2023:

Nitty nit: My clang-format-diff wants this to align with the stars in the line above.

TheCharlatan approved

TheCharlatan commented at 9:16 am on September 8, 2023: contributor

ACK c1aa0af69eee05a9fad1eabd45662fc2143a3e0c

Remove version/hashing options from CBlockLocator/CDiskBlockIndex f15f790618

refactor: Use DataStream now that version/type are unused 4240a082b8

refactor: remove clientversion include from dbwrapper.h e73d2a8018

theuni force-pushed on Sep 8, 2023

theuni commented at 1:45 pm on September 8, 2023: member

Resolved formatting nit.

Reviewers can confirm locally that the diff from before amounts only to whitespace: git diff -w --ignore-blank-lines c1aa0af69eee05a9fad1eabd45662fc2143a3e0c..e73d2a8018def940afadb5d699b18f39e882c1fc

TheCharlatan commented at 1:52 pm on September 8, 2023: contributor

Re-ACK e73d2a8018def940afadb5d699b18f39e882c1fc

DrahtBot requested review from sipa on Sep 8, 2023

DrahtBot requested review from ajtowns on Sep 8, 2023

ajtowns commented at 4:07 am on September 9, 2023: contributor

reACK e73d2a8018def940afadb5d699b18f39e882c1fc

DrahtBot removed review request from ajtowns on Sep 9, 2023

fanquake merged this on Sep 9, 2023

fanquake closed this on Sep 9, 2023

in src/dbwrapper.h:169 in e73d2a8018

165@@ -167,7 +166,7 @@ class CDBIterator
166 
167     template<typename V> bool GetValue(V& value) {
168         try {
169-            CDataStream ssValue{GetValueImpl(), SER_DISK, CLIENT_VERSION};
170+            DataStream ssValue{GetValueImpl()};

TheCharlatan commented at 11:38 am on September 9, 2023:

Just saw that this did replace the CDataStream with a DataStream here in CDBIterator and CDBWrapper, but it is still using a CDataStream for CDBBatch. Should that be replaced too, as I’ve done here: https://github.com/TheCharlatan/bitcoin/commit/95405b8a4f3816277c3dbf075c8da013226e6cec?

maflcko commented at 12:13 pm on September 11, 2023:

Yes, if it compiles, then it should be fine to replace.

theuni commented at 1:34 pm on September 11, 2023:

Yes please, thanks for catching this. IIRC I couldn’t do this with my original approach of converting to SerParams, but forgot about it after switching. As @MarcoFalke says, this must be correct if it compiles.

maflcko commented at 12:59 pm on September 11, 2023: member

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.

I think it would have been easier to review if SER_GETHASH was removed. It will need to be removed anyway, and it is already unused, so removing it should be correct, if it compiles.

Concept ACK either way

maflcko commented at 4:08 pm on September 11, 2023: member

Fixed in #28451, I guess.

Frank-GER referenced this in commit d042ef9f09 on Sep 19, 2023

fanquake referenced this in commit 48b8910d12 on Oct 2, 2023

bitcoin locked this on Sep 10, 2024

Hard-code version number value for CBlockLocator and CDiskBlockIndex #28428

Reviews

Conflicts