Add the following improvements to the fuzz target for CCrypter:
Add roundtrip assertions: Add verification logic by decrypting the resulting ciphertext from a successful call to Encrypt() or EncryptSecret() and verifying that the output matches the original plaintext.
Cover untested methods: Extend the target to explicitly test CCrypter::SetKey() and DecryptSecret().
Add verification logic to the `crypter` fuzz target by decrypting
the resulting ciphertext from a successful call to `Encrypt()` or
`EncryptSecret()` and verifying that the output matches the
original plaintext.
This ensures that the encryption/decryption cycle preserves data
correctness, rather than just checking for execution crashes.
Extend the `crypter` fuzz target to cover `CCrypter::SetKey()` and
`DecryptSecret()`.
<!--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/34111.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | brunoerg |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Concept NACK
We already have 100% coverage for wallet/crypter, see: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/crypter.cpp.gcov.html.
Also, we don't need to cover DecryptSecret directly if this is already exercised by DecryptKey, same idea for SetKey. For reference, see #32118 which fixed this target and removed a direct call to DecryptSecret.
Concept NACK
We already have 100% coverage for
wallet/crypter, see: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/crypter.cpp.gcov.html.Also, we don't need to cover
DecryptSecretdirectly if this is already exercised byDecryptKey, same idea forSetKey. For reference, see #32118 which fixed this target and removed a direct call toDecryptSecret.
Makes sense. But shouldn't making the encryption tests roundtrip better help in future regression catching?
Closing the PR for now.