refactor: Remove SER_GETHASH, hard-code client version in CKeyPool serialize#28508
refactor: Remove SER_GETHASH, hard-code client version in CKeyPool serialize#28508fanquake merged 3 commits intobitcoin:masterfrom
Conversation
GetType() is never called, so it is completely unused and can be removed.
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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
|
Concept ACK. |
|
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
the
2309-no-gethash-
branch
from
fa8bb5e to
fab2583
Compare
maflcko
changed the title
|
Changed the commit title and PR title. Is it good enough? |
It was never set, so it can be removed along with any code reading it.
maflcko
force-pushed
the
2309-no-gethash-
branch
from
fab2583 to
fac29a0
Compare
kevkevinpal
left a comment
kevkevinpal
left a comment
There was a problem hiding this comment.
added one nit but otherwise ACK fac29a0
| if (!(s.GetType() & SER_GETHASH)) { | ||
| s << nVersion; | ||
| } | ||
| s << int{259900}; // Unused field, writes the highest client version ever written |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
|
Concept ACK |
|
ACK fac29a0 |
7 participants
Removes a bunch of redundant, dead or duplicate code.
Uses the idea from and finishes the idea #28428 by theuni