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

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

2. Remove unused GetType() from OverrideStream, CVectorWriter, SpanReader

   GetType() is never called, so it is completely unused and can be
   removed.

   fa4a9c0f43
3. 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
4. 
   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.

5. DrahtBot added the label Refactoring on Sep 19, 2023
6. 
   theuni commented at 3:22 pm on September 19, 2023: member

   Concept ACK.
7. 
   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.
8. maflcko force-pushed on Sep 19, 2023
9. maflcko renamed this:
   refactor: Remove SER_GETHASH type
   refactor: Remove SER_GETHASH, hard-code client version in CKeyPool serialize
   on Sep 19, 2023
10. 
    maflcko commented at 3:58 pm on September 19, 2023: member

    Changed the commit title and PR title. Is it good enough?
11. 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
12. maflcko force-pushed on Sep 19, 2023
13. 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{}
14. 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.

15. kevkevinpal approved
16. 
    kevkevinpal commented at 2:58 am on September 21, 2023: contributor

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

    Concept ACK
18. 
    ajtowns commented at 2:00 am on September 28, 2023: contributor

    ACK fac29a0ab19fda457b55d7a0a37c5cd3d9680f82
19. fanquake merged this on Oct 2, 2023
20. fanquake closed this on Oct 2, 2023

    ---

21. maflcko deleted the branch on Oct 3, 2023
22. bitcoin locked this on Oct 2, 2024

Contributors
maflcko DrahtBot theuni kevkevinpal stickies-v ajtowns

Labels
Refactoring