test: add addnode connection limit test to rpc

Bortlesboat commented at 3:41 pm on March 16, 2026: none

Add test_addnode_connection_limit to NetTest in rpc_net.py, replacing the standalone p2p_addnode_limit.py.

The test restarts node 0 with -maxconnections=0 to verify that addnode connections bypass the general connection limit and are independently capped at MAX_ADDNODE_CONNECTIONS (8). Nodes 2-10 serve as addnode targets (9 targets, only 8 can connect). The test verifies the cap is enforced, then removes one peer and confirms the previously-blocked node takes the freed slot.

Added as a separate test method rather than inline in test_addnode_getaddednodeinfo because the limit test requires different node configuration (no proxy, -maxconnections=0), necessitating a restart_node call.

Partially addresses #28635. This PR adds coverage for the MAX_ADDNODE_CONNECTIONS limit only; it does not cover the double-addnode / ThreadOpenAddedConnections crash case.

DrahtBot added the label Tests on Mar 16, 2026

DrahtBot commented at 3:42 pm 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
Concept ACK	kevkevinpal

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34162 (net: Avoid undershooting in GetAddressesUnsafe by fjahr)
#27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)

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.

kevkevinpal commented at 7:33 pm on March 16, 2026: contributor

Concept ACK ee55ce3

I think it makes sense to add this test to test_addnode_getaddednodeinfo in test/functional/rpc_net.py instead of creating a new file for it.

We could hit the limit before self.log.info("Check that node can be removed") and then start to remove nodes

kevkevinpal commented at 7:36 pm on March 16, 2026: contributor

It might make sense to use the -maxconnections=<num> node setting in the test aswell incase this were to ever change then the test might fail aswell

Bortlesboat commented at 7:43 pm on March 16, 2026: none

Thanks for the review! Good points on both — I’ll consolidate this into test_addnode_getaddednodeinfo in rpc_net.py and add explicit -maxconnections to make the test self-contained. Will push an updated version.

Bortlesboat force-pushed on Mar 16, 2026

Bortlesboat renamed this:
~~test: add functional test for addnode connection limit~~
test: add addnode connection limit test to rpc_net.py
on Mar 16, 2026

in test/functional/rpc_net.py:600 in 5a888dd6be

596@@ -586,6 +597,51 @@ def check_getrawaddrman_entries(expected):
597         self.log.debug("Test that getrawaddrman contains information about newly added addresses in each addrman table")
598         check_getrawaddrman_entries(expected)
599 
600+    def count_manual(self, node):

chriszeng1010 commented at 5:23 am on March 18, 2026:

Nit; If count_manual only used in test_addnode_connection_limit, then we should add it as a nested function.

Bortlesboat commented at 2:26 pm on March 18, 2026:

Done, moved to nested function.

in test/functional/rpc_net.py:606 in 5a888dd6be

601+        return sum(1 for p in node.getpeerinfo() if p['connection_type'] == 'manual')
602+
603+    def test_addnode_connection_limit(self):
604+        self.log.info("Test addnode connection limit (MAX_ADDNODE_CONNECTIONS=8)")
605+        # Must match MAX_ADDNODE_CONNECTIONS in src/net.h
606+        MAX_ADDNODE = 8

brunoerg commented at 1:03 pm on March 18, 2026:

nit: perhaps we could name it as MAX_ADDNODE_CONNECTIONS as well.

Bortlesboat commented at 2:26 pm on March 18, 2026:

Good catch, fixed.

in test/functional/rpc_net.py:629 in 5a888dd6be

624+        self.log.info(f"Wait for {MAX_ADDNODE} manual connections to establish")
625+        self.wait_until(lambda: self.count_manual(node) >= MAX_ADDNODE, timeout=120)
626+        assert_equal(self.count_manual(node), MAX_ADDNODE)
627+
628+        self.log.info("Verify exactly one target node was NOT connected")
629+        inbound_counts = [len(self.nodes[i].getpeerinfo()) for i in range(2, 2 + targets)]

brunoerg commented at 1:08 pm on March 18, 2026:

Instead of verifying only the number of connections to know whether one of them was not connected, you could also check that the last node you tried to connect to was the peer that was not connected.

Bortlesboat commented at 2:27 pm on March 18, 2026:

Added — the test now asserts the last-added target’s port is specifically not in the connected set. Since all targets are localhost and ThreadOpenAddedConnections iterates in insertion order with 500ms between attempts, the 9th node is deterministically blocked by the semaphore.

in test/functional/rpc_net.py:621 in 5a888dd6be outdated

616+        assert_equal(node.getaddednodeinfo(), [])
617+        assert_equal(self.count_manual(node), 0)
618+
619+        self.log.info(f"Add {targets} nodes to the addnode list")
620+        for i in range(2, 2 + targets):
621+            node.addnode(node=f"127.0.0.1:{p2p_port(i)}", command='add')

brunoerg commented at 1:09 pm on March 18, 2026:

nit: Perhaps we could also test it using the -addnode flag?

Bortlesboat commented at 2:35 pm on March 18, 2026:

Good idea. The -addnode flag feeds into the same m_added_nodes and goes through ThreadOpenAddedConnections, so the limit enforcement is identical — but covering the flag parsing path separately makes sense. I’ll add that in a follow-up.

Bortlesboat force-pushed on Mar 18, 2026

Bortlesboat commented at 2:26 pm on March 18, 2026: none

Updated per feedback:

count_manual moved to nested function inside the test
Renamed MAX_ADDNODE to MAX_ADDNODE_CONNECTIONS to match src/net.h
Added assertion that the last-added target is specifically the unconnected peer (since ThreadOpenAddedConnections iterates in insertion order, the 9th node is deterministically the one blocked by the semaphore on localhost)
Fixed sync_all() calls to sync_blocks(self.nodes[:2]) since nodes 2-10 have no peers

DrahtBot added the label CI failed on Mar 20, 2026

Bortlesboat force-pushed on Mar 22, 2026

test: add addnode connection limit test to rpc_net.py

Add test_addnode_connection_limit to verify that MAX_ADDNODE_CONNECTIONS
(8) is enforced when using the addnode RPC. The test adds 9 targets,
confirms exactly 8 connect and identifies the blocked peer, then frees
a slot and verifies the blocked peer connects.

Closes #28635

d20e3f4a17

Bortlesboat force-pushed on Mar 22, 2026

DrahtBot removed the label CI failed on Mar 24, 2026

naiyoma commented at 2:52 pm on March 27, 2026: contributor

I’ve looked at the test briefly. I think it partially addresses the issue by testing MAX_ADDNODE_CONNECTIONS.

However, it doesn’t cover the crash described here:

I was able to reproduce it by calling addnode twice and having the test suite wait a little. This also required modifying ThreadOpenAddedConnections to wait 1 second instead of a full minute (see Sjors@bc3995c).

There’s also a suggestion to use a unit test:[ #28635 (comment)](/bitcoin-bitcoin/28635/#issuecomment-1757283488) This would allow us to call ThreadOpenAddedConnections().

Or am I missing something?

Also, max_addnode_connections could be tested in a unit test together with the crash scenario, which would be faster than a functional test.

It’s okay to keep this as a functional test if it’s preferred over a unit test. Consider editing the description to clarify that it only partially addresses the issue.

Bortlesboat commented at 6:02 pm on March 28, 2026: none

You’re right, this PR only covers the MAX_ADDNODE_CONNECTIONS part of #28635. I left out the double-addnode / ThreadOpenAddedConnections crash case because that looks like a separate failure mode and probably wants a different test, likely unit rather than functional. I’ve updated the description to make that scope explicit.

test: add addnode connection limit test to rpc_net.py #34833

Reviews

Conflicts