Most invocations of CHashWriter use SER_GETHASH and version 0, this allows for eliding those values, ~and removes SER_GETHASH as a type, because it functions simply as “has no external destination” in practice.~
~SerializeHash basically existed due to the overhead of CHashWriter construction, now that that is minimized, it’s unnecessary.~
11@@ -12,7 +12,7 @@
12
13 uint256 CBlockHeader::GetHash() const
14 {
15- return SerializeHash(*this);
16+ return (CHashWriter(PROTOCOL_VERSION) << *this).GetHash();
SerializeHash nVersion default value.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
GetUTXOStats codepaths, decouple from coinstatsindex by dongcarl)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.
!(s.GetType() & SER_GETHASH) to s.GetType(). That means that what used to be the branch for DISK/NETWORK will end up being run just for DISK, and what used to be the branch for GETHASH will end up being run for NETWORK and GETHASH.
SER_NETWORK is (1 << 0) i.e. 0x01, SER_DISK is (1 << 1) i.e. 0x02, so both will evaluate to true, while with 0 used where SER_GETHASH once was, it will evaluate to false. Note also while they’re defined as flags all uses of SER_GETHASH are singular.
https://github.com/bitcoin/bitcoin/pull/13462/files#diff-1fc2d3d7edc00ab8ea29eb1ca30cdcbbR163
Ah of course; I missed the (1 << ...) around it.
This isn’t very readable code though. Would you mind keeping SER_GETHASH as a constant (perhaps just defined as 0), and explicit comparisons with it (rather than bit masking). That should still be a nice simplification.
121@@ -122,8 +122,8 @@ class CHashWriter
122 const int nVersion;
123 public:
124
125- CHashWriter() : CHashWriter(SER_GETHASH, 0) {}
126- CHashWriter(int nVersionIn) : CHashWriter(SER_GETHASH, nVersionIn) {}
127+ CHashWriter() : CHashWriter(0, 0) {}
Reviewing calls to GetType after these changes, it seems like there are only 2 calls that affect the output, both testing GetType for SER_DISK in CAddress::SerializationOp. Is it possible to remove those? If so we could get right of GetType entirely, which seems like a big win.
If not, it seems like SER_NETWORK would not cause negative effects, but it would be a bit misleading for those cases where the serialization is not propagated to the network.
SER_NETWORK - it’s equivalent to 0 now as only SER_DISK has conditional behavior associated with it.
121@@ -122,6 +122,8 @@ class CHashWriter
122 const int nVersion;
123 public:
124
125+ CHashWriter() : CHashWriter(0, 0) {}
126+ CHashWriter(int nVersionIn) : CHashWriter(0, nVersionIn) {}
explicit? :-)
122@@ -123,6 +123,8 @@ class CHashWriter
123 const int nVersion;
124 public:
125
126+ CHashWriter() : CHashWriter(SER_GETHASH, 0) {}
127+ CHashWriter(int nVersionIn) : CHashWriter(SER_GETHASH, nVersionIn) {}
explicit?
This has been open for two (almost three) years ! we probably should get to either merging it if it is worth doing, or decide not to, but not keep @Empact rebasing it forever.
Edit: personally I’m not 100% convinced, I mean, yes the arguments can be elided now in some cases but does this make the code clearer than being explicit?
Hi @laanwj - I took 2020 (and then some) off, but it’s good to be back at it. As you note, this is old work and may not be worthwhile but here are my thoughts:
In these contexts, SER_GETHASH is redundant/noise. It’s a matter of interpretation whether CHashWriter(0) is more informative than CHashWriter - I tend to think it does not convey information so should be elided for readability.
It’s a bit striking when you consider that of 39 references to SER_GETHASH in master, 30 of them are content-free/noise. This removes those. The original PR was to remove SER_GETHASH entirely, but I walked that back to this more modest change.
As the bulk of the changes are in the form of a scripted-diff covering the specific cases, I look at this as a low-risk improvement to readability - refinement/readability being important to the long-term health of any software project.
If the answer to this is “meh,” though, fair enough.
Personally I don’t have any strong opinions about this PR but I do have strong opinions about another thing: very glad to have you back @Empact! Warm welcome back as a contributor! :)
I took 2020 (and then some) off, but it’s good to be back at it.
All callers either relied on the default value of SER_GETHASH or provided
SER_GETHASH.
-BEGIN VERIFY SCRIPT-
sed -i -E "s/CHashWriter\(SER_GETHASH, 0\)(.[^{])/CHashWriter()\1/" $(git grep -l 'CHashWriter')
sed -i -E "s/CHashWriter (.+)\(SER_GETHASH, 0\)/CHashWriter \1/" $(git grep -l 'CHashWriter')
sed -i -E "s/CHashWriter (.+)\(SER_GETHASH, /CHashWriter \1(/" $(git grep -l 'CHashWriter')
-END VERIFY SCRIPT-
At nearly four years old, this may be the pull that has been open the longest so far.
ACK c05246bf00bf8ffa5e73ed739a5001dc3639830c code review, clean rebase to master, clean debug build, ran unit tests
Incidentally, @jonatack do you happen to know the situation with Cirrus-CI? I’d like to retry this build, but I don’t have the option to do so on the site.
Restarted.
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
