Remove version field from GetSerializeSize #28878

ajtowns commented at 2:41 AM on November 15, 2023: contributor

Drops the version field from GetSerializeSize(), simplifying the code in various places. Also drop GetSerializeSizeMany() (as just removing the version parameter could result in silent bugs) and remove unnecessary instances of #include <version.h>.

DrahtBot commented at 2:41 AM on November 15, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, theuni

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:

#28451 (refactor: Remove unused SER_DISK, SER_NETWORK, SER_GETHASH by maflcko)
#27006 (reduce cs_main scope, guard block index 'nFile' under a local mutex by furszy)

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.

DrahtBot added the label Needs rebase on Nov 15, 2023

theuni commented at 4:31 PM on November 15, 2023: member

Concept ACK.

Is this next after #28438? Or something else first?

theuni commented at 6:19 PM on November 15, 2023: member

Playing around locally, I was also able to drop the include in:

$ git diff --stat
 src/bitcoin-util.cpp            | 1 -
 src/coins.cpp                   | 1 -
 src/core_read.cpp               | 1 -
 src/kernel/coinstats.cpp        | 1 -
 src/rest.cpp                    | 1 -
 src/script/bitcoinconsensus.cpp | 1 -
 src/zmq/zmqpublishnotifier.cpp  | 1 -

(I didn't mess with the tests, maybe could drop some there too).

Also worth pointing out: libbitcoinkernel now only requires version.h for signet.cpp, which I assume will go away in one of these follow-ups.

Edit: Not that the header dependency matters at all for libbitcoinkernel. It's just indicative imo that we're moving the right direction if it ends up un-tethered.

serialize: Drop nVersion from [C]SizeComputer

Protocol version is no longer needed to work out the serialized size
of objects so drop that information from CSizeComputer and rename the
class to SizeComputer.

efa9eb6d7c

serialize: drop GetSerializeSizeMany bf574a7501

serialize: Drop useless version param from GetSerializeSize() 1410d300df

Convert some CDataStream to DataStream c7b61fd61b

Include version.h in fewer places 83986f464c

ajtowns force-pushed on Nov 16, 2023

ajtowns commented at 1:48 AM on November 16, 2023: contributor

Rebased after #28438 was merged.

Is this next after #28438? Or something else first?

Either this or #28451 seem good to go; I think they conflict due to the "Convert some CDataStream.." commit here though; shouldn't be a hard rebase in either case though, I think.

Playing around locally, I was also able to drop the include in:

Taken.

Also worth pointing out: libbitcoinkernel now only requires version.h for signet.cpp, which I assume will go away in one of these follow-ups.

That just needs SpanReader and CVectorWriter to be updated to not need a version.

ajtowns added the label Refactoring on Nov 16, 2023

ajtowns marked this as ready for review on Nov 16, 2023

DrahtBot removed the label Needs rebase on Nov 16, 2023

in src/serialize.h:453 in efa9eb6d7c outdated

 449 | @@ -450,7 +450,7 @@ inline unsigned int GetSizeOfVarInt(I n)
 450 |  }
 451 |  
 452 |  template<typename I>
 453 | -inline void WriteVarInt(CSizeComputer& os, I n);
 454 | +inline void WriteVarInt(SizeComputer& os, I n);

maflcko commented at 2:30 PM on November 16, 2023:

nit in efa9eb6d7c8012fe4ed85699d81c8fe5dd18da1e: inline is implied by template and can be removed.

ajtowns commented at 2:33 AM on November 17, 2023:

(leaving the nits for if it's otherwise necessary to retouch)

in src/serialize.h:1087 in efa9eb6d7c outdated

1087 |      size_t nSize{0};
1088 |  
1089 | -    const int nVersion;
1090 |  public:
1091 | -    explicit CSizeComputer(int nVersionIn) : nVersion(nVersionIn) {}
1092 | +    SizeComputer() {}

maflcko commented at 2:31 PM on November 16, 2023:

Why remove the explicit? Shouldn't matter in this context, but when in doubt, I'd prefer to keep it, unless there is a reason to remove it?

ajtowns commented at 2:32 AM on November 17, 2023:

explicit is good for constructors that take a single argument to prevent accidental conversion, but seems useless for constructors that don't take an argument. (The real question is why leave a constructor there at all?)

in src/serialize.h:1104 in efa9eb6d7c outdated

1097 | @@ -1099,7 +1098,7 @@ class CSizeComputer
1098 |      }
1099 |  
1100 |      template<typename T>
1101 | -    CSizeComputer& operator<<(const T& obj)
1102 | +    SizeComputer& operator<<(const T& obj)
1103 |      {
1104 |          ::Serialize(*this, obj);
1105 |          return (*this);

maflcko commented at 2:33 PM on November 16, 2023:

unrelated nit in the first commit: Can remove the (), which achieve nothing in this context.

in src/serialize.h:1113 in efa9eb6d7c outdated

1111 | -    int GetVersion() const { return nVersion; }
1112 |  };
1113 |  
1114 |  template<typename I>
1115 | -inline void WriteVarInt(CSizeComputer &s, I n)
1116 | +inline void WriteVarInt(SizeComputer &s, I n)

maflcko commented at 2:34 PM on November 16, 2023:

Same (inline)

maflcko approved

maflcko commented at 3:09 PM on November 16, 2023: member

Some nits in the first commit. Otherwise:

ACK 83986f464c59a6517f790a960a72574e167f3f72 📒

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 83986f464c59a6517f790a960a72574e167f3f72 📒
u/O7R7516VMRG5DuyOAXpyIHkMFCYUDfQH4PmkXGtkYoDV483ta0wAcinIAZ/Fijsk9h85VNKL9CG4+pCBLwAA==

</details>

DrahtBot requested review from theuni on Nov 16, 2023

in src/psbt.h:95 in bf574a7501 outdated

  88 | @@ -89,7 +89,9 @@ struct PSBTProprietary
  89 |  template<typename Stream, typename... X>
  90 |  void SerializeToVector(Stream& s, const X&... args)
  91 |  {
  92 | -    WriteCompactSize(s, GetSerializeSizeMany(s.GetVersion(), args...));
  93 | +    SizeComputer sizecomp;
  94 | +    SerializeMany(sizecomp, args...);
  95 | +    WriteCompactSize(s, sizecomp.size());
  96 |      SerializeMany(s, args...);

theuni commented at 4:57 PM on November 16, 2023:

Unrelated to this PR, but I think we could make this much more efficient by adding a serializer override that keeps a running tally of serialized size? That way size is calculated as a side-effect of the actual serialization and prepended to the stream, rather than running all the way through twice.

No idea if this path is hot enough to bother messing with that.

ajtowns commented at 2:29 AM on November 17, 2023:

A running tally doesn't help -- we need to write the size of the serialized data before we serialize it, so this would mean caching the serialization until we've finished first which would likely be a bigger waste of memory than running things through the size computation first is a waste of cpu. (I've looked at this at least twice with exactly the same idea already :smile:)

theuni approved

theuni commented at 5:12 PM on November 16, 2023: member

ACK 83986f464c59a6517f790a960a72574e167f3f72.

I started working on a patch to remove clientversion.h where it's no longer necessary after these changes, but it's just more work than it's worth for this.

Can we just agree to do a (client)version.h include cleanup at some point when the dust has settled on all this? :)

fanquake merged this on Nov 17, 2023

fanquake closed this on Nov 17, 2023

bitcoin locked this on Dec 3, 2024