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();
Note this was the only call that relied on the SerializeHash nVersion default value.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
Good to know, thanks.
I don't understand "scripted-diff: Drop SER_GETHASH"; it changes !(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.
@sipa 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.
Thinking a bit more about this, I think you can actually go further. I believe none of the serializers which have conditionals that mention SER_GETHASH (CDiskBlockIndex, CBlockLocator, CAddress, CKeyPool, CWalletKey, CAccountingEntry, CAccount) are ever actually invoked with SER_GETHASH as nType, so we could literally delete SER_GETHASH entire and all conditions that test for it. SerializeHash could then just use SER_NETWORK afaict.
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) {}
Can you use SER_NETWORK instead here? 0 is not a valid serialization type, even if it's not actually used anywhere.
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.
Switched to 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) {}
This should be explicit? :-)
In case the SER_GETHASH removal was complicating things, I reoriented this around simply making SER_GETHASH implicit in the obviously hash-specific contexts.
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) {}
Should be explicit?
Indeed, fixed.
Rebased, refined, how about another look?
Rebased
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-
Approach ACK on this simplification, code looks reasonable on first read.
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.
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.
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
Not sure on this. I think it might be cleaner to remove ser-type and ser-version completely. See #19477 (comment)
Can this be closed now?