-
Notifications
You must be signed in to change notification settings - Fork 36k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make SER_GETHASH implicit for CHashWriter and SerializeHash #13462
Conversation
bfa47a8
to
d697362
Compare
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
Withdrawing this pending #10785. Will re-open after. |
Good to know, thanks. |
I don't understand "scripted-diff: Drop SER_GETHASH"; it changes |
@sipa |
Ah of course; I missed the This isn't very readable code though. Would you mind keeping |
d697362
to
95d73e0
Compare
@sipa Fair enough, I dropped that commit. Def more straight-forward now. |
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. |
@sipa Nice I'll look into that. |
95d73e0
to
f141484
Compare
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-
d0aab54
to
c05246b
Compare
893628b Drop minor GetSerializeSize template (Ben Woosley) da74db0 Drop unused GetType() from CSizeComputer (Ben Woosley) Pull request description: Based on conversation in bitcoin#13462, it seems the serialization `GetType` has very narrow use/effect. In every case except for `CAddress`, which specifically relates to a network peer's address, not a wallet address etc., the serialized representation of an object is irrespective of its destination / type. This removes the unused `GetType` method from `CSizeComputer` as a step to further narrowing that use. Tree-SHA512: e72b8e9e5160396691e05aeaee3aba5a57935a75bd5005cfcc7fb51c936f3d1728a397f999da5c36696506dd815fafa5c738f3894df8864f25f91f639eba9c3d
893628b Drop minor GetSerializeSize template (Ben Woosley) da74db0 Drop unused GetType() from CSizeComputer (Ben Woosley) Pull request description: Based on conversation in bitcoin#13462, it seems the serialization `GetType` has very narrow use/effect. In every case except for `CAddress`, which specifically relates to a network peer's address, not a wallet address etc., the serialized representation of an object is irrespective of its destination / type. This removes the unused `GetType` method from `CSizeComputer` as a step to further narrowing that use. Tree-SHA512: e72b8e9e5160396691e05aeaee3aba5a57935a75bd5005cfcc7fb51c936f3d1728a397f999da5c36696506dd815fafa5c738f3894df8864f25f91f639eba9c3d
893628b Drop minor GetSerializeSize template (Ben Woosley) da74db0 Drop unused GetType() from CSizeComputer (Ben Woosley) Pull request description: Based on conversation in bitcoin#13462, it seems the serialization `GetType` has very narrow use/effect. In every case except for `CAddress`, which specifically relates to a network peer's address, not a wallet address etc., the serialized representation of an object is irrespective of its destination / type. This removes the unused `GetType` method from `CSizeComputer` as a step to further narrowing that use. Tree-SHA512: e72b8e9e5160396691e05aeaee3aba5a57935a75bd5005cfcc7fb51c936f3d1728a397f999da5c36696506dd815fafa5c738f3894df8864f25f91f639eba9c3d
893628b Drop minor GetSerializeSize template (Ben Woosley) da74db0 Drop unused GetType() from CSizeComputer (Ben Woosley) Pull request description: Based on conversation in bitcoin#13462, it seems the serialization `GetType` has very narrow use/effect. In every case except for `CAddress`, which specifically relates to a network peer's address, not a wallet address etc., the serialized representation of an object is irrespective of its destination / type. This removes the unused `GetType` method from `CSizeComputer` as a step to further narrowing that use. Tree-SHA512: e72b8e9e5160396691e05aeaee3aba5a57935a75bd5005cfcc7fb51c936f3d1728a397f999da5c36696506dd815fafa5c738f3894df8864f25f91f639eba9c3d
893628b Drop minor GetSerializeSize template (Ben Woosley) da74db0 Drop unused GetType() from CSizeComputer (Ben Woosley) Pull request description: Based on conversation in bitcoin#13462, it seems the serialization `GetType` has very narrow use/effect. In every case except for `CAddress`, which specifically relates to a network peer's address, not a wallet address etc., the serialized representation of an object is irrespective of its destination / type. This removes the unused `GetType` method from `CSizeComputer` as a step to further narrowing that use. Tree-SHA512: e72b8e9e5160396691e05aeaee3aba5a57935a75bd5005cfcc7fb51c936f3d1728a397f999da5c36696506dd815fafa5c738f3894df8864f25f91f639eba9c3d
Approach ACK on this simplification, code looks reasonable on first read. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At nearly four years old, this may be the pull that has been open the longest so far.
ACK c05246b 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". |
893628b Drop minor GetSerializeSize template (Ben Woosley) da74db0 Drop unused GetType() from CSizeComputer (Ben Woosley) Pull request description: Based on conversation in bitcoin#13462, it seems the serialization `GetType` has very narrow use/effect. In every case except for `CAddress`, which specifically relates to a network peer's address, not a wallet address etc., the serialized representation of an object is irrespective of its destination / type. This removes the unused `GetType` method from `CSizeComputer` as a step to further narrowing that use. Tree-SHA512: e72b8e9e5160396691e05aeaee3aba5a57935a75bd5005cfcc7fb51c936f3d1728a397f999da5c36696506dd815fafa5c738f3894df8864f25f91f639eba9c3d
Not sure on this. I think it might be cleaner to remove ser-type and ser-version completely. See #19477 (comment) |
See #25331 |
Can this be closed now? |
Closing in favor of #25331 Thanks, yours is cleaner, but for those brackets. ;) |
Most invocations of
CHashWriter
useSER_GETHASH
and version 0, this allows for eliding those values,and removesSER_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.