test: add peer connection unit test coverage

jonatack commented at 11:48 pm on October 28, 2023: contributor

This PR is based on #28155 and adds unit test coverage for each change in it (i.e. tests that fail on master), as well as some missing unit test coverage in general for CConnman::AddNode(), ConnectNode(), GetAddedNodeInfo(), ThreadOpenAddedConnections(), and RPC addnode – see coverage check at https://corecheck.dev/bitcoin/bitcoin/pulls/28749. I’m building on these initial unit tests for writing coverage for #28248.

A version of this branch based on current master instead of #28155 is here.

Suggest the following to reviewers for running the tests locally:

0./src/test/test_bitcoin -t net_peer_connection_tests -l all -- -printtoconsole=1

or

0./src/test/test_bitcoin -t net_peer_connection_tests -l message -- -printtoconsole=1

net/rpc: prevents connecting to an address we are already connected to

The current `addnode` rpc command has some edge cases in where it is possible to
connect to the same node twice by combining ip and address requests. This can happen under two situations:

The two commands are run one right after each other, in which case they will be processed
under the same loop in `CConnman::ThreadOpenAddedConnections` without refreshing `vInfo`, so both
will go trough. An example of this would be:

```
bitcoin-cli addnode "localhost:port" add

```

A node is added by IP using `addnode "add"` while the other is added by name using
`addnode "onetry"` with an address that resolves to multiple IPs. In this case, we currently
only check one of the resolved IPs (picked at random), instead of all the resolved ones, meaning
this will only probabilistically fail/succeed. An example of this would be:

```
bitcoin-cli addnode "127.0.0.1:port" add
[...]
bitcoin-cli addnode "localhost:port" onetry
```

Both cases can be fixed by iterating over all resolved addresses in `CConnman::ConnectNode` instead
of picking one at random

6453b47dc1

rpc: Prevents adding the same ip more than once when formatted differently

Currently it is possible to add the same node twice when formatting IPs in
different, yet equivalent, manner. This applies to both ipv4 and ipv6, e.g:

127.0.0.1 = 127.1 | [::1] = [0:0:0:0:0:0:0:1]

`addnode` will accept both and display both as connected (given they translate to
the same IP). This will not result in multiple connections to the same node, but
will report redundant info when querying `getaddednodeinfo` and populate `m_added_nodes`
with redundant data.

This can be avoided performing comparing the contents of `m_added_addr` and the address
to be added as `CServices` instead of as strings.

e477fa665d

net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request

`CConnman::GetAddedNodeInfo` is used both to get a list of addresses to manually connect to
in `CConnman::ThreadOpenAddedConnections`, and to report about manually added connections in
`getaddednodeinfo`. In both cases, all addresses added to `m_added_nodes` are returned, however
the nodes we are already connected to are only relevant to the latter, in the former they are
actively discarded.

Parametrizes `CConnman::GetAddedNodeInfo` so we can ask for only addresses we are not connected to,
to avoid passing useless information around.

1cfc171720

DrahtBot commented at 11:48 pm on October 28, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28690 (build: Introduce internal kernel library by TheCharlatan)
#28248 (p2p: peer connection bug fixes by jonatack)
#28155 (net: improves addnode / m_added_nodes logic by sr-gi)
#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)
#23352 (test: Extend stale_tip_peer_management test by maflcko)

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.

DrahtBot added the label Tests on Oct 28, 2023

DrahtBot added the label CI failed on Oct 29, 2023

jonatack force-pushed on Oct 29, 2023

DrahtBot removed the label CI failed on Oct 29, 2023

jonatack force-pushed on Oct 29, 2023

Allow unit tests to access additional CConnman members

that are otherwise private:
- CConnman::m_nodes
- CConnman::ConnectNodes()
- CConnman::AlreadyConnectedToAddress()

and update the #include headers per iwyu.

458d3753cb

jonatack force-pushed on Oct 29, 2023

jonatack commented at 8:39 pm on October 30, 2023: contributor

Closing, as these unit tests were pulled into #28155.

jonatack closed this on Oct 30, 2023

jonatack reopened this on Oct 31, 2023

Create net_peer_connection unit tests

for initial partial unit test coverage of these CConnman class methods:

- AddNode()
- ConnectNode()
- GetAddedNodeInfo()
- AlreadyConnectedToAddress()
- ThreadOpenAddedConnections()

and of the GetAddedNodeInfo() call in RPC addnode.

ad31935da7

jonatack force-pushed on Oct 31, 2023

jonatack closed this on Oct 31, 2023

jonatack commented at 5:46 pm on October 31, 2023: contributor

Re-opened to push an update and re-closed after it was cherry-picked into #28155.

fanquake commented at 5:48 pm on October 31, 2023: member

In future, there is no need to reopen and reclose pull requests, for someone to cherry pick a commit out of a branch. Someone can just fetch your remote after you’ve pushed changes, and cherry-pick what they like.

jonatack commented at 5:56 pm on October 31, 2023: contributor

Yes. It was to allow verifying the CI was green, if desired, prior to cherry-picking.

test: add peer connection unit test coverage #28749

Code Coverage

Reviews

Conflicts