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

CExtKey::Unserialize(...) and CExtPubKey::Unserialize(...) throw std::runtime_error instead of the expected std::ios_base::failure.

Context (taken from an e-mail):

When fuzzing some deserialization code I noticed that CExtKey::Unserialize(...) and CExtPubKey::Unserialize(...) throw std::runtime_error instead of the std::ios_base::failure I expected in case of deserialization errors.

I saw that this code was written by you originally: do you remember if there was a particular reason to go with std::runtime_error instead of std::ios_base::failure? If not, do you think it would be safe to change it? :)

Code:

https://github.com/bitcoin/bitcoin/blob/376638afcf945ec43089625d115286594ce0ab16/src/key.h#L174-L183

https://github.com/bitcoin/bitcoin/blob/78dae8caccd82cfbfd76557f1fb7d7557c7b5edb/src/pubkey.h#L240-L249

FWIW the code paths in question are reachable by the fuzzers added in PR #17051 ("tests: Add deserialization fuzzing harnesses").

Friendly ping @jonasschnelli :)

Could you clarify? This is a fuzzing blocker I'd like to get rid of if possible :)

Those messages are not sent over p2p, so fuzzing is nice to have, but not going to harden the protocol layer.

I guess they might be written by the wallet? In that case, you might want to check if the wallet is handling those exceptions properly.

The only use of CExt{Pub,}Key serialisation/deserialisation appears to be in bip32_tests:

https://github.com/bitcoin/bitcoin/blob/f2a094884d987465437a6300daa84de6f39c2322/src/test/bip32_tests.cpp#L122-L136

I cannot find any non-test usage.

Maybe you can remove that logic then?

I'm pretty sure serialization is used by the wallet for the dumpwallet rpc. Also, both are being used in #16463

@achow101 dumpwallet is using EncodeExtKey, no? :)

https://github.com/bitcoin/bitcoin/blob/46d6930f8c7ba7cbcd7d86dd5d0117642fcbc819/src/wallet/rpcdump.cpp#L802-L813

Hmm, so the actual serialization happens in Encode/Decode, not Serialize/Unserialize, and that's what #16463 uses. So in that case, I think it is fine to get rid of Serialize/Unserialize.

@MarcoFalke

Maybe you can remove that logic then?

Since #15814 + #15814 (comment) I'm a bit hesitant to touch dead code :)

Perhaps we can put a "good first issue" tag on this one and let a newcomer take care of the removal.

From a fuzzing perspective I've got my needs covered: if this code isn't used it doesn't matter if invalid deserialisation input makes it throw an unexpected exception type (std::runtime_error instead of the expected std::ios_base::failure). My job here is done :)

Ok, will add "good first issue"

Removing is probably fine. If this will ever be needed, a simple git revert $commit_that_removed_it should restore it.

This was solved with the merge of #17212. Closing. Thanks @theStack! :)