fuzz: addrman, avoid ConsumeDeserializable when possible
#27918
Conversation
`ConsumeDeserializable` may return `std::nullopt`, prefer to call specific functions such as `ConsumeService`and `ConsumeNetAddr` which always return a value.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
lgtm In theory one could use the nullopt state to break a loop or exit early (for example with a different exit code, c.f. #27552), but given that this fuzz target checks |
| @@ -326,4 +307,4 @@ FUZZ_TARGET_INIT(addrman_serdeser, initialize_addrman) | |||
| data_stream << addr_man1; | |||
| data_stream >> addr_man2; | |||
| assert(addr_man1 == addr_man2); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
It'd be good to avoid making unrelated changes like this (deleting the file ending newline).
…n possible 025fda0 fuzz: addrman, avoid `ConsumeDeserializable` when possible (brunoerg) Pull request description: Using specific functions like `ConsumeService`, `ConsumeAddress` and `ConsumeNetAddr` may be more effective than using `ConsumeDeserializable`. They always return some value while `ConsumeDeserializable` may return `std::nullopt`. E.g.: In this part of the code, if `op_net_addr` is `std::nullopt`, we basically generated the addresses (if so) unnecessarily, because we won't be able to use them: ```cpp std::vector<CAddress> addresses; LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) { const std::optional<CAddress> opt_address = ConsumeDeserializable<CAddress>(fuzzed_data_provider); if (!opt_address) { break; } addresses.push_back(*opt_address); } const std::optional<CNetAddr> opt_net_addr = ConsumeDeserializable<CNetAddr>(fuzzed_data_provider); if (opt_net_addr) { addr_man.Add(addresses, *opt_net_addr, std::chrono::seconds{ConsumeTime(fuzzed_data_provider, 0, 100000000)}); } ``` Also, if we are not calling `Add` effectively, it would also be affect other functions that may "depend" on it. ACKs for top commit: dergoegge: Code review ACK 025fda0 Tree-SHA512: 02450bec0b084c15ba0cd1cbdfbac067c8fea4ccf27be0c86d54e020f029a6c749a16d8e0558f9d6d35a7ca9db8916f180c872f09474702b5591129e9be0d192
Using specific functions like
ConsumeService,ConsumeAddressandConsumeNetAddrmay be more effective than usingConsumeDeserializable. They always return some value whileConsumeDeserializablemay returnstd::nullopt.E.g.: In this part of the code, if
op_net_addrisstd::nullopt, we basically generated the addresses (if so) unnecessarily, because we won't be able to use them:Also, if we are not calling
Addeffectively, it would also be affect other functions that may "depend" on it.