refactor: Remove unused CExt{Pub,}Key (de)serialization methods #17212

pull theStack wants to merge 1 commits into bitcoin:master from theStack:20191021-refactor-remove_unused_cextkey_and_cextpubkey_serialization changing 3 files +0 −60

theStack commented at 10:15 PM on October 21, 2019: member

As pointed out in issue #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 #17130 (comment), #17130 (comment) and #17130 (comment)).

refactor: Remove unused CExt{Pub,}Key (de)serialization methods

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.

5b44a75493

fanquake added the label Refactoring on Oct 21, 2019
practicalswift commented at 6:02 AM on October 22, 2019: contributor

ACK 5b44a75493a1a098404d5e21dc384e74eae1892e -- -60 LOC diff looks correct :)
promag commented at 10:11 AM on October 22, 2019: member

ACK 5b44a75493a1a098404d5e21dc384e74eae1892e.
laanwj commented at 11:19 AM on October 22, 2019: member

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)
fanquake added the label Waiting for author on Oct 22, 2019
jonatack commented at 11:58 AM on October 22, 2019: member

Light ACK 5b44a75493a1a098404d5e21dc384e74eae1892e. Built, ran tests and bitcoind. git blame shows most of the last changes are from commit 90604f16af63ec066d6561337f476ccd8acec326 in 2015 to add bip32 pubkey serialization.
jonatack commented at 12:03 PM on October 22, 2019: member

More context in #6215, in particular #6215 (comment).
MarcoFalke removed the label Waiting for author on Oct 22, 2019
MarcoFalke commented at 12:20 PM on October 22, 2019: member

unsigned ACK 5b44a75493a1a098404d5e21dc384e74eae1892e

Seems fine to remove them after 4 years of never being used
practicalswift commented at 3:38 PM on October 22, 2019: contributor

@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").
fjahr commented at 3:50 PM on October 24, 2019: member

ACK 5b44a75

Ran test, grepped for any callers and also checked doxygen, which does not show any callers for the removed methods.
MarcoFalke referenced this in commit 773026044f on Oct 24, 2019
MarcoFalke merged this on Oct 24, 2019
MarcoFalke closed this on Oct 24, 2019
jasonbcox referenced this in commit 231b37f3ea on Sep 25, 2020
theStack deleted the branch on Dec 1, 2020
PastaPastaPasta referenced this in commit ae4e42b17d on Sep 11, 2021
PastaPastaPasta referenced this in commit d8515fa191 on Sep 11, 2021
PastaPastaPasta referenced this in commit 63e9dec6b3 on Sep 12, 2021
PastaPastaPasta referenced this in commit 69ba1dc285 on Sep 12, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#17130 CExtKey::Unserialize and CExtPubKey::Unserialize throw std::runtime_error instead of the expected std::ios_base::failure