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

[practicalswift]

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:

bitcoin/src/key.h

Lines 174 to 183 in 376638a

	template <typename Stream>
	void Unserialize(Stream& s)
	{
	unsigned int len = ::ReadCompactSize(s);
	unsigned char code[BIP32_EXTKEY_SIZE];
	if (len != BIP32_EXTKEY_SIZE)
	throw std::runtime_error("Invalid extended key size\n");
	s.read((char *)&code[0], len);
	Decode(code);
	}

bitcoin/src/pubkey.h

Lines 240 to 249 in 78dae8c

	template <typename Stream>
	void Unserialize(Stream& s)
	{
	unsigned int len = ::ReadCompactSize(s);
	unsigned char code[BIP32_EXTKEY_SIZE];
	if (len != BIP32_EXTKEY_SIZE)
	throw std::runtime_error("Invalid extended key size\n");
	s.read((char *)&code[0], len);
	Decode(code);
	}

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

[practicalswift]

Friendly ping @jonasschnelli :)

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

[maflcko]

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.

[practicalswift]

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

bitcoin/src/test/bip32_tests.cpp

Lines 122 to 136 in f2a0948

	CDataStream ssPub(SER_DISK, CLIENT_VERSION);
	ssPub << pubkeyNew;
	BOOST_CHECK(ssPub.size() == 75);
	CDataStream ssPriv(SER_DISK, CLIENT_VERSION);
	ssPriv << keyNew;
	BOOST_CHECK(ssPriv.size() == 75);
	CExtPubKey pubCheck;
	CExtKey privCheck;
	ssPub >> pubCheck;
	ssPriv >> privCheck;
	BOOST_CHECK(pubCheck == pubkeyNew);
	BOOST_CHECK(privCheck == keyNew);

I cannot find any non-test usage.

[maflcko]

Maybe you can remove that logic then?

[achow101]

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

[practicalswift]

@achow101 dumpwallet is using EncodeExtKey, no? :)

bitcoin/src/wallet/rpcdump.cpp

Lines 802 to 813 in 46d6930

	// add the base58check encoded extended master if the wallet uses HD
	CKeyID seed_id = pwallet->GetHDChain().seed_id;
	if (!seed_id.IsNull())
	{
	CKey seed;
	if (pwallet->GetKey(seed_id, seed)) {
	CExtKey masterKey;
	masterKey.SetSeed(seed.begin(), seed.size());
	file << "# extended private masterkey: " << EncodeExtKey(masterKey) << "\n\n";
	}
	}

[achow101]

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.

[practicalswift]

@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 :)

[maflcko]

Ok, will add "good first issue"

[maflcko]

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

[practicalswift]

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