fuzz: set fSuccessfullyConnected in connman harness #34830

pull frankomosh wants to merge 1 commits into bitcoin:master from frankomosh:fuzz-connman-set-connected-state changing 1 files +2 −0

frankomosh commented at 8:58 AM on March 16, 2026: contributor

The connman fuzz harness never sets 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.

fuzz: set fSuccessfullyConnected in connman harness

Without this, NodeFullyConnected() filters out every fuzz-constructed node, making ForEachNode's callback unreachable (0/1.13M branch hits from my end).

685a44c601

DrahtBot added the label Fuzzing on Mar 16, 2026
DrahtBot commented at 8:58 AM on March 16, 2026: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK sedited, maflcko, marcofleon

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
in src/test/fuzz/connman.cpp:93 in 685a44c601
```
  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;
```
sedited commented at 11:22 AM on March 16, 2026:

Could there be value in consuming a bool for this (maybe once outside the limited while)?

Crypt-iQ commented at 3:50 PM on March 16, 2026:

Could sometimes set to false?

frankomosh commented at 5:39 AM on March 17, 2026:

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.

frankomosh commented at 5:50 AM on March 17, 2026:

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.
dergoegge commented at 12:31 PM on March 16, 2026: member

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.
frankomosh commented at 3:28 PM on March 16, 2026: contributor
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
- After
sedited commented at 9:33 PM on March 16, 2026: contributor

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.
frankomosh commented at 6:20 AM on March 17, 2026: contributor

Kind of related: Is there a reason why NodeFullyConnected is 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.
sedited approved
sedited commented at 8:03 AM on March 17, 2026: contributor

ACK 685a44c60122a8e9bf69076bdab117dd55469373
fanquake requested review from marcofleon on Mar 17, 2026
maflcko commented at 9:55 AM on March 17, 2026: member
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)
frankomosh commented at 11:55 AM on March 17, 2026: contributor
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
```
  - 1270|  1.13M|            if (NodeFullyConnected(node))
-   |  Branch (1270:17): [True: 0, False: 1.13M]
-  1271|      0|                func(node);
+ 1270|  1.11M|            if (NodeFullyConnected(node))
+   |  Branch (1270:17): [True: 1.11M, False: 80]
+  1271|  1.11M|                func(node);
```
ForNode — net.cpp:4131
```
  -  4131|     39|    return found != nullptr && NodeFullyConnected(found) && func(found);
-                                              ^0                          ^0
+  4131|     37|    return found != nullptr && NodeFullyConnected(found) && func(found);
+                                              ^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.
maflcko commented at 12:51 PM on March 17, 2026: member
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:
```
src/test/fuzz/connman.cpp:                connman.ForEachNode([](auto) {});
```
As well as this lambda:
```
src/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.
marcofleon commented at 3:28 PM on March 17, 2026: contributor

ACK 685a44c60122a8e9bf69076bdab117dd55469373

I think it's fine just being set to true. Both branches of NodeFullyConnected end up getting executed.
fanquake merged this on Mar 17, 2026
fanquake closed this on Mar 17, 2026
frankomosh deleted the branch on Mar 17, 2026
CyberNFT commented at 4:59 PM on March 18, 2026: none

👍🏻