refactor: Remove SER_GETHASH, hard-code client version in CKeyPool serialize #28508

pull maflcko wants to merge 3 commits into bitcoin:master from maflcko:2309-no-gethash- changing 19 files +63 −95

maflcko commented at 2:43 PM on September 19, 2023: member

Removes a bunch of redundant, dead or duplicate code.

Uses the idea from and finishes the idea #28428 by theuni
Remove unused GetType() from OverrideStream, CVectorWriter, SpanReader
GetType() is never called, so it is completely unused and can be removed.
fa4a9c0f43

Remove CHashWriter type

The type is only ever set, but never read via GetType(), so remove it.
Also, remove SerializeHash to avoid silent merge conflicts and use the
already existing GetHash() boilerplate consistently.

fa72f09d6f

DrahtBot commented at 2:43 PM on September 19, 2023: contributor


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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK kevkevinpal, ajtowns

Concept ACK theuni, stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #28438 (Use serialization parameters for CTransaction by ajtowns)
- #28107 (util: Type-safe transaction identifiers by dergoegge)
- #21283 (Implement BIP 370 PSBTv2 by achow101)
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.
DrahtBot added the label Refactoring on Sep 19, 2023
theuni commented at 3:22 PM on September 19, 2023: member

Concept ACK.
theuni commented at 3:24 PM on September 19, 2023: member

Mind splitting up the last commit? The version number change needs a bit of scrutiny imo, but it's hidden inside an innocuous commit message.
maflcko force-pushed on Sep 19, 2023
maflcko renamed this:
~~refactor: Remove SER_GETHASH type~~
refactor: Remove SER_GETHASH, hard-code client version in CKeyPool serialize
on Sep 19, 2023
maflcko commented at 3:58 PM on September 19, 2023: member

Changed the commit title and PR title. Is it good enough?
Remove SER_GETHASH, hard-code client version in CKeyPool serialize
It was never set, so it can be removed along with any code reading it.
fac29a0ab1
maflcko force-pushed on Sep 19, 2023
in src/primitives/transaction.cpp:83 in fa72f09d6f outdated
```
  81 |  {
  82 |      if (!HasWitness()) {
  83 |          return hash;
  84 |      }
  85 | -    return SerializeHash(*this, SER_GETHASH, 0);
  86 | +    return (CHashWriter{0} << *this).GetHash();
```
theuni commented at 5:34 PM on September 19, 2023:

CHashWriter{0} is pretty ugly. I assume this will be going away though as part of one of the other refactors going on?

maflcko commented at 9:22 PM on September 19, 2023:

Yes, it will become HashWriter{}
in src/wallet/scriptpubkeyman.h:126 in fac29a0ab1
```
 122 | @@ -123,20 +123,14 @@ class CKeyPool
 123 |      template<typename Stream>
 124 |      void Serialize(Stream& s) const
 125 |      {
 126 | -        int nVersion = s.GetVersion();
 127 | -        if (!(s.GetType() & SER_GETHASH)) {
 128 | -            s << nVersion;
 129 | -        }
 130 | +        s << int{259900}; // Unused field, writes the highest client version ever written
```
kevkevinpal commented at 2:56 AM on September 21, 2023:

nit: in the comment does "highest client version ever written" mean that this is the highest client version that can be written or that we've ever seen written?

Suggestion "Unused field, writes the highest possible client version"

feel free to ignore this nit aswell!

maflcko commented at 9:20 AM on September 21, 2023:

It is the current client version written. It should be possible to confirm this by adding an std::cout << nVersion << "\n"; here.

The client version is only increased, so it is also the highest.

In any case, the value doesn't matter, so I am happy to replace it with anything else.
kevkevinpal approved
kevkevinpal commented at 2:58 AM on September 21, 2023: contributor

added one nit but otherwise ACK fac29a0
stickies-v commented at 9:22 AM on September 27, 2023: contributor

Concept ACK
ajtowns commented at 2:00 AM on September 28, 2023: contributor

ACK fac29a0ab19fda457b55d7a0a37c5cd3d9680f82
fanquake merged this on Oct 2, 2023
fanquake closed this on Oct 2, 2023
maflcko deleted the branch on Oct 3, 2023
bitcoin locked this on Oct 2, 2024