refactor: Remove unused CExt{Pub,}Key (de)serialization methods #17212
Conversation
The serialization/deserialization methods for the classes CExtKey and CExtPubKey were only used in the BIP32 unit tests, where the relevant parts are removed as well.
|
ACK 5b44a75 -- -60 LOC diff looks correct :) |
|
ACK 5b44a75. |
|
I'd like some more context here: Were they ever used? Why were they added? Should they be used? (I vaguely remember some of these methods were added in the BIP32 pull with the idea they'd be used for future functionality) |
|
More context in #6215, in particular #6215 (comment). |
|
unsigned ACK 5b44a75 Seems fine to remove them after 4 years of never being used |
|
@theStack Thanks for taking on this. Always nice to get rid of dead code that is unlikely to be used in the future. If you want to find more dead code you might want to look at coverage after running the unit tests, the functional tests, the benchmarks and say a full IBD with coverage . Code paths not covered by such a run could then be investigated manually to see if it is genuinely dead code or just hard-to-reach code. Additionally, you can find a subset of all unused functions in #16967 (see "Functions that are both unused and untested" and "Functions that are unused outside of the testing code"). |
|
ACK 5b44a75 Ran test, grepped for any callers and also checked doxygen, which does not show any callers for the removed methods. |
…ization methods 5b44a75 refactor: Remove unused CExt{Pub,}Key (de)serialization methods (Sebastian Falbesoner) Pull request description: As pointed out in issue bitcoin#17130, the serialization/deserialization methods for the classes `CExtKey` and `CExtPubKey` are only used in the BIP32 unit tests and hence can be removed (see comments bitcoin#17130 (comment), bitcoin#17130 (comment) and bitcoin#17130 (comment)). ACKs for top commit: practicalswift: ACK 5b44a75 -- -60 LOC diff looks correct :) promag: ACK 5b44a75. MarcoFalke: unsigned ACK 5b44a75 fjahr: ACK 5b44a75 jonatack: Light ACK 5b44a75. Built, ran tests and bitcoind. `git blame` shows most of the last changes are from commit 90604f1 in 2015 to add bip32 pubkey serialization. Tree-SHA512: 6887573b76b9e54e117a076557407b6f7908719b2202fb9eea498522baf9f30198b3f78b87a62efcd17ad1ab0886196f099239992ce7cbbaee79979ffe9e5f2c
Summary: The serialization/deserialization methods for the classes CExtKey and CExtPubKey were only used in the BIP32 unit tests, where the relevant parts are removed as well. Backport of Bitcoin Core [[bitcoin/bitcoin#17212 | PR17212]] Test Plan: `ninja check` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7553
theStack
deleted the
20191021-refactor-remove_unused_cextkey_and_cextpubkey_serialization
branch
…ization methods 5b44a75 refactor: Remove unused CExt{Pub,}Key (de)serialization methods (Sebastian Falbesoner) Pull request description: As pointed out in issue bitcoin#17130, the serialization/deserialization methods for the classes `CExtKey` and `CExtPubKey` are only used in the BIP32 unit tests and hence can be removed (see comments bitcoin#17130 (comment), bitcoin#17130 (comment) and bitcoin#17130 (comment)). ACKs for top commit: practicalswift: ACK 5b44a75 -- -60 LOC diff looks correct :) promag: ACK 5b44a75. MarcoFalke: unsigned ACK 5b44a75 fjahr: ACK 5b44a75 jonatack: Light ACK 5b44a75. Built, ran tests and bitcoind. `git blame` shows most of the last changes are from commit 90604f1 in 2015 to add bip32 pubkey serialization. Tree-SHA512: 6887573b76b9e54e117a076557407b6f7908719b2202fb9eea498522baf9f30198b3f78b87a62efcd17ad1ab0886196f099239992ce7cbbaee79979ffe9e5f2c
8 participants
As pointed out in issue #17130, the serialization/deserialization methods for the classes
CExtKeyandCExtPubKeyare only used in the BIP32 unit tests and hence can be removed (see comments #17130 (comment), #17130 (comment) and #17130 (comment)).