DeserializeHDKeypaths() was writing into the original hd_keypaths map instead of deserialized_hd_keypaths. As a result the latter was always empty and the round-trip assertion following was trivially true, so the serialize/deserialize round-trip wasn't actually being exercised.
That bug was introduced with the commit introducing the fuzz target (commit f898ef65c947776750e49d050633f830546bbdc6, #18994).
`DeserializeHDKeypaths()` was writing into the original `hd_keypaths`
map instead of `deserialized_hd_keypaths`. As a result the latter was
always empty and the round-trip assertion following was trivially true,
so the serialize/deserialize round-trip wasn't actually being exercised.
That bug was introduced with the commit introducing the fuzz target
(commit f898ef65c947776750e49d050633f830546bbdc6, #18994).
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35481.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | sedited |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
ACK 5deb053a75fb0529618a2980d9b6a6ba9928d039
66 | @@ -67,7 +67,7 @@ FUZZ_TARGET(script_sign, .init = initialize_script_sign) 67 | } 68 | std::map<CPubKey, KeyOriginInfo> deserialized_hd_keypaths; 69 | try { 70 | - DeserializeHDKeypaths(serialized, key, hd_keypaths); 71 | + DeserializeHDKeypaths(serialized, key, deserialized_hd_keypaths); 72 | } catch (const std::ios_base::failure&) {
do we still need the try/catch? Doesn't that kinda' defeat the purpose of a fuzzer?
afaict the key can still have a bad size here, which triggers a try/catch, no?
shouldn't we guard for that specifically and abort in other cases? Otherwise what's the point of this call, to have fake coverage?
This is how we handle stream and serialization failures in all the fuzz tests. I think that is fine to be honest. Code calling this function has to handle this exception in the same manner with the highest-level exception type. We could match on the various debug strings for them, but I'm not sure if we really gain much soundness from that.
What do we gain from calling it as such (besides fake code coverage), i.e. calling the method and silencing every potentially useful outcome?
I understand the cases when a generator method throws - but it should terminate execution if it wasn't able to create a new useful value, e.g.: https://github.com/bitcoin/bitcoin/blob/c01c7f068c5b67329794bac9354eb112111815ee/src/test/fuzz/rpc.cpp#L54-L58
In other cases we're validating the failure reason, e.g. https://github.com/bitcoin/bitcoin/blob/c01c7f068c5b67329794bac9354eb112111815ee/src/test/fuzz/rpc.cpp#L389-L403