Make SER_GETHASH implicit for CHashWriter and SerializeHash #13462

Empact · 2018-06-13T20:42:39Z

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.~~

src/primitives/block.cpp

DrahtBot · 2018-06-14T20:10:06Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24410 ([kernel 2a/n] Split hashing/index GetUTXOStats codepaths, decouple from coinstatsindex by dongcarl)
#24058 (BIP-322 basic support by kallewoof)

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.

Empact · 2018-06-14T21:06:35Z

Withdrawing this pending #10785. Will re-open after.

sipa · 2018-06-14T21:35:51Z

@Empact #10785 is low priority, and will probably take a while. Don't let it stop you.

Empact · 2018-06-14T21:48:57Z

Good to know, thanks.

sipa · 2018-06-14T22:15:04Z

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.

Empact · 2018-06-14T23:23:09Z

@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

sipa · 2018-06-14T23:27:52Z

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.

Empact · 2018-06-15T00:13:43Z

@sipa Fair enough, I dropped that commit. Def more straight-forward now.

sipa · 2018-06-15T00:24:12Z

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.

Empact · 2018-06-15T00:25:12Z

@sipa Nice I'll look into that.

src/hash.h

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-

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

jonatack · 2022-05-18T05:44:05Z

Approach ACK on this simplification, code looks reasonable on first read.

jonatack

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

Empact · 2022-05-21T18:13:53Z

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.

hebasto · 2022-05-21T18:20:09Z

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.

DrahtBot · 2022-05-24T13:41:21Z

🐙 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

maflcko · 2022-06-06T13:05:41Z

Not sure on this. I think it might be cleaner to remove ser-type and ser-version completely. See #19477 (comment)

maflcko · 2022-06-10T10:04:19Z

See #25331

maflcko · 2022-07-20T17:27:42Z

Can this be closed now?

Empact · 2022-07-22T07:18:26Z

Closing in favor of #25331

Thanks, yours is cleaner, but for those brackets. ;)

Empact force-pushed the serialize-hash-type branch 2 times, most recently from bfa47a8 to d697362 Compare June 13, 2018 20:52

Empact changed the title ~~scripted-diff: Simplify common case of CHashWriter and drop SER_GETHASH~~ scripted-diff: Simplify common case of CHashWriter and drop SER_GETHASH & SerializeHash Jun 13, 2018

Empact commented Jun 13, 2018

View reviewed changes

src/primitives/block.cpp Outdated Show resolved Hide resolved

meshcollider added the Refactoring label Jun 13, 2018

Empact closed this Jun 14, 2018

Empact reopened this Jun 14, 2018

Empact closed this Jun 14, 2018

Empact reopened this Jun 14, 2018

Empact force-pushed the serialize-hash-type branch 3 times, most recently from d697362 to 95d73e0 Compare June 15, 2018 00:11

Empact changed the title ~~scripted-diff: Simplify common case of CHashWriter and drop SER_GETHASH & SerializeHash~~ scripted-diff: Simplify common case of CHashWriter and drop SerializeHash Jun 15, 2018

Empact force-pushed the serialize-hash-type branch from 95d73e0 to f141484 Compare June 17, 2018 21:56

sipa reviewed Jun 26, 2018

View reviewed changes

src/hash.h Outdated Show resolved Hide resolved

Empact mentioned this pull request Jun 28, 2018

Drop unused GetType() from CSizeComputer #13558

Merged

Empact changed the title ~~scripted-diff: Simplify common case of CHashWriter and drop SerializeHash~~ Simplify common case of CHashWriter and drop SerializeHash Jun 28, 2018

DrahtBot mentioned this pull request Jul 31, 2018

[wallet] Kill accounts #13825

Closed

DrahtBot closed this Aug 10, 2018

DrahtBot reopened this Aug 10, 2018

This was referenced Mar 1, 2022

net: Encapsulate asmap in NetGroupManager #22910

Merged

addrman: treat Tor/I2P/CJDNS as a single group #22563

Closed

DrahtBot added the Needs rebase label Apr 22, 2022

Empact added 4 commits April 22, 2022 15:48

Drop nType argument from SerializeHash

f387b6d

All callers either relied on the default value of SER_GETHASH or provided SER_GETHASH.

Add convenience constructors for CHashWriter

f923975

Include serialize.h wherever SER_GETHASH is referenced

c05246b

Empact force-pushed the serialize-hash-type branch from d0aab54 to c05246b Compare April 22, 2022 20:48

DrahtBot removed the Needs rebase label Apr 22, 2022

DrahtBot mentioned this pull request Apr 29, 2022

[kernel 2a/n] Split hashing/index GetUTXOStats codepaths, decouple from coinstatsindex #24410

Merged

1 task

jonatack approved these changes May 18, 2022

View reviewed changes

DrahtBot added the Needs rebase label May 24, 2022

Empact closed this Jul 22, 2022

Empact deleted the serialize-hash-type branch July 22, 2022 07:18

Empact restored the serialize-hash-type branch November 18, 2022 20:45

bitcoin locked and limited conversation to collaborators Nov 18, 2023

Make SER_GETHASH implicit for CHashWriter and SerializeHash #13462

Make SER_GETHASH implicit for CHashWriter and SerializeHash #13462

Uh oh!

Conversation

Empact commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Empact commented Jun 14, 2018

Uh oh!

sipa commented Jun 14, 2018

Uh oh!

Empact commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jun 14, 2018

Uh oh!

Empact commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Jun 15, 2018

Uh oh!

sipa commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Jun 15, 2018

Uh oh!

Uh oh!

jonatack commented May 18, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Empact commented May 21, 2022

Uh oh!

hebasto commented May 21, 2022

Uh oh!

DrahtBot commented May 24, 2022

Uh oh!

maflcko commented Jun 6, 2022

Uh oh!

maflcko commented Jun 10, 2022

Uh oh!

maflcko commented Jul 20, 2022

Uh oh!

Empact commented Jul 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Empact commented Jun 13, 2018 •

edited

Loading

DrahtBot commented Jun 14, 2018 •

edited

Loading

Empact commented Jun 14, 2018 •

edited

Loading

Empact commented Jun 14, 2018 •

edited

Loading

sipa commented Jun 14, 2018 •

edited

Loading

sipa commented Jun 15, 2018 •

edited

Loading