fuzz: addrman, avoid ConsumeDeserializable when possible#27918
fuzz: addrman, avoid ConsumeDeserializable when possible#27918fanquake merged 1 commit intobitcoin:masterfrom
ConsumeDeserializable when possible#27918Conversation
`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 |
| data_stream >> addr_man2; | ||
| assert(addr_man1 == addr_man2); | ||
| } | ||
| } No newline at end of file |
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
5 participants
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.