fSuccessfullyConnected=true on nodes added through AddTestNode(). NodeFullyConnected() gates ForEachNode() on that flag, making its callback unreachable in the current harness.
Set fSuccessfullyConnected=true before AddTestNode() to simulate a node that has completed the version handshake.
Without this, NodeFullyConnected() filters out every fuzz-constructed node, making ForEachNode's callback unreachable (0/1.13M branch hits from my end).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | sedited, maflcko, marcofleon |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
88@@ -89,6 +89,8 @@ FUZZ_TARGET(connman, .init = initialize_connman)
89
90 LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100) {
91 CNode& p2p_node{*ConsumeNodeAsUniquePtr(fuzzed_data_provider).release()};
92+ // Simulate post-handshake state.
93+ p2p_node.fSuccessfullyConnected = true;
Yes I considered both states, but I think the current behaviour already covers the false case. The false path of NodeFullyConnected is exercised through DisconnectNode, which sets fDisconnect = true.
Also, assuming we are to model the real situation in production, fSuccessfullyConnected is only ever set to true on VERACK, it’s a one-way transition.
Crypt-iQ has a similar suggestion above. I think the false path is already covered through
DisconnectNode, and always setting true maximises coverage of the paths that were actually unreachable.
In my view, setting it deterministically to true guarantees those paths are always reachable, rather than only being explored in subsets of executions, which also reduces coverage efficiency for this harness.
Concept ACK
Kind of related: Is there a reason why NodeFullyConnected is a static member function? Seem like it should be a const member function instead.
Kind of related: Is there a reason why
NodeFullyConnectedis a static member function? Seem like it should be a const member function instead.
As far as I can tell, it was introduced as a static helper in commit 12752af0cc, likely because it doesn’t depend on any instance state and only inspects the CNode.
There’s a meta discussion in theUni’s net split roadmap (#33958), which plans to remove ForNode/ForEachNode entirely once PeerManager iterates Peer objects instead of CNodes.
Could you provide a fuzz coverage report before and after this change? It would help reviewers assess whether the change affects which code paths are being exercised by the existing fuzz harness.
Coverage reports before and after the change:
* [Before](https://frankomosh.github.io/fuzz-coverage/connman-fSuccessfullyConnected/before/index.html) * [After](https://frankomosh.github.io/fuzz-coverage/connman-fSuccessfullyConnected/after/index.html)
Not sure what this shows. It shows that a single line is now covered on top.
Not sure which one it is, and how this would be relevant.
Can you clarify which line it is and why this is relevant?
Maybe run llvm cov show and then diff (c.f. https://github.com/bitcoin/bitcoin/blob/52e8c1ce32a24d94b8fe06fa5e17bf8a176e50f0/contrib/devtools/deterministic-fuzz-coverage/src/main.rs#L128-L204)
Not sure what this shows. It shows that a single line is now covered on top.
Not sure which one it is, and how this would be relevant.
Can you clarify which line it is and why this is relevant?
Maybe run llvm cov show and then diff (c.f.
diff cov_before.txt cov_after.txt filtered to NodeFullyConnected/ForEachNode/func(node):
ForEachNode — net.h:1270-1271
0 - 1270| 1.13M| if (NodeFullyConnected(node))
1- | Branch (1270:17): [True: 0, False: 1.13M]
2- 1271| 0| func(node);
3+ 1270| 1.11M| if (NodeFullyConnected(node))
4+ | Branch (1270:17): [True: 1.11M, False: 80]
5+ 1271| 1.11M| func(node);
ForNode — net.cpp:4131
0 - 4131| 39| return found != nullptr && NodeFullyConnected(found) && func(found);
1- ^0 ^0
2+ 4131| 37| return found != nullptr && NodeFullyConnected(found) && func(found);
3+ ^2 ^2
The ForEachNode callback (func(node)) was dead. NodeFullyConnected always returned false because fSuccessfullyConnected was not set on fuzz constructed nodes. After this proposal, the callback is reached 1.11M times. ForNode’s callback also gains minimal coverage (^2) from the rare case where a fuzzed NodeId matches an inserted node.
lgtm ACK 685a44c60122a8e9bf69076bdab117dd55469373
Seems fine, and thanks for the show|diff. It claims to call func, when it previously was not called. I guess this means the empty lambda is now called:
0src/test/fuzz/connman.cpp: connman.ForEachNode([](auto) {});
As well as this lambda:
0src/test/fuzz/connman.cpp: (void)connman.ForNode(fuzzed_data_provider.ConsumeIntegral<NodeId>(), [&](auto) { return fuzzed_data_provider.ConsumeBool(); });
The first lambda has no return type and no side-effect and the second one has neither the return type checked nor its side-effect.
I guess it is fine to merge, but this is just a minimal coverage increase.
ACK 685a44c60122a8e9bf69076bdab117dd55469373
I think it’s fine just being set to true. Both branches of NodeFullyConnected end up getting executed.
