Remove version field from GetSerializeSize #28878
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK. Is this next after #28438? Or something else first? |
|
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 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. |
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.
ajtowns
force-pushed
the
202310-getserializesize
branch
from
5410dcf
to
83986f4
Compare
|
Rebased after #28438 was merged.
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.
Taken.
That just needs |
There was a problem hiding this comment.
Some nits in the first commit. Otherwise:
ACK 83986f4 📒
Show signature
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==
| @@ -450,7 +450,7 @@ inline unsigned int GetSizeOfVarInt(I n) | |||
| } | |||
|
|
|||
| template<typename I> | |||
| inline void WriteVarInt(CSizeComputer& os, I n); | |||
| inline void WriteVarInt(SizeComputer& os, I n); | |||
There was a problem hiding this comment.
nit in efa9eb6: inline is implied by template and can be removed.
There was a problem hiding this comment.
(leaving the nits for if it's otherwise necessary to retouch)
| public: | ||
| explicit CSizeComputer(int nVersionIn) : nVersion(nVersionIn) {} | ||
| SizeComputer() {} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?)
| @@ -1099,7 +1098,7 @@ class CSizeComputer | |||
| } | |||
|
|
|||
| template<typename T> | |||
| CSizeComputer& operator<<(const T& obj) | |||
| SizeComputer& operator<<(const T& obj) | |||
| { | |||
| ::Serialize(*this, obj); | |||
| return (*this); | |||
There was a problem hiding this comment.
unrelated nit in the first commit: Can remove the (), which achieve nothing in this context.
| }; | ||
|
|
||
| template<typename I> | ||
| inline void WriteVarInt(CSizeComputer &s, I n) | ||
| inline void WriteVarInt(SizeComputer &s, I n) |
There was a problem hiding this comment.
ACK 83986f4.
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? :)
| WriteCompactSize(s, GetSerializeSizeMany(s.GetVersion(), args...)); | ||
| SizeComputer sizecomp; | ||
| SerializeMany(sizecomp, args...); | ||
| WriteCompactSize(s, sizecomp.size()); | ||
| SerializeMany(s, args...); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄)
Drops the version field from
GetSerializeSize(), simplifying the code in various places. Also dropGetSerializeSizeMany()(as just removing the version parameter could result in silent bugs) and remove unnecessary instances of#include <version.h>.