test: make v2transport arg in addconnection mandatory and few cleanups

stratospher commented at 2:48 pm on January 31, 2024: contributor

make v2transport argument in addconnection regression-testing only RPC mandatory. #24748 (review)
- previously it was an optional arg with default false value.
- only place this RPC is used is in the functional tests where we always pass the appropriate v2transport option to the RPC anyways. (and that too just for python dummy peer(P2PInterface) and bitcoind(TestNode) interactions)
rename v2_handshake() to _on_data_v2_handshake() #24748 (review)
more compact return statement in wait_for_reconnect() #24748 (review)
assertion to check that empty version packets are received from TestNode.

DrahtBot commented at 2:48 pm on January 31, 2024: 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.

Type	Reviewers
ACK	theStack, mzumsande, glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

fanquake renamed this:
~~[test] make v2transport arg in addconnection mandatory and few cleanups~~
test: make v2transport arg in addconnection mandatory and few cleanups
on Jan 31, 2024

DrahtBot added the label Tests on Jan 31, 2024

DrahtBot added the label CI failed on Jan 31, 2024

[test] make v2transport arg in addconnection mandatory and few cleanups

`TestNode::add_outbound_p2p_connection()` is the only place where
addconnection test-only RPC is used. here, we always pass the
appropriate v2transport option to addconnection RPC.

currently the v2transport option for addconnection RPC is optional.
so simply make the v2transport option mandatory instead.

e7fd70f4b6

stratospher force-pushed on Jan 31, 2024

theStack commented at 6:01 pm on January 31, 2024: contributor

Concept ACK

Another possible alternative for addconnection’s third parameter handling would be to take as default whatever is set by -v2transport, like we already do it for addnode (i.e. bool use_v2transport = self.MaybeArg<bool>(2).value_or(node_v2transport);).

DrahtBot removed the label CI failed on Jan 31, 2024

kristapsk commented at 7:38 pm on January 31, 2024: contributor

make v2transport argument in addconnection RPC mandatory.

This will break compatibility for third party software / scripts written against previous behaviour where there was no such mandatory argument. What are benefits?

mzumsande commented at 7:44 pm on January 31, 2024: contributor

This will break compatibility for third party software / scripts written against previous behaviour where there was no such mandatory argument. What are benefits?

addconnection is a debug-only command that is only used by our functional test framework (to make “fake” automatic outbound connections). It has never been announced to the public, is only enabled for regtest and I don’t see why any third party would ever need to use it. Did you maybe confuse it with the widely used addnode rpc?

kristapsk commented at 7:49 pm on January 31, 2024: contributor

Ok, didn’t notice it is regtest only. Probably help addconnection should give error instead of usage information if not running against regtest.

With v26.0.0 you get this against mainnet:

 0bitcoin@odroid:~$ bitcoin-cli help | grep addconnection
 1bitcoin@odroid:~$ bitcoin-cli help addconnection       
 2addconnection "address" "connection_type"
 3
 4Open an outbound connection to a specified node. This RPC is for testing only.
 5
 6Arguments:
 71. address            (string, required) The IP address and port to attempt connecting to.
 82. connection_type    (string, required) Type of connection to open ("outbound-full-relay", "block-relay-only", "addr-fetch" or "feeler").
 9
10Result:
11{                               (json object)
12  "address" : "str",            (string) Address of newly added connection.
13  "connection_type" : "str"     (string) Type of connection opened.
14}
15
16Examples:
17> bitcoin-cli addconnection "192.168.0.6:8333" "outbound-full-relay"
18> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "addconnection", "params": ["192.168.0.6:8333" "outbound-full-relay"]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
19
20bitcoin@odroid:~$ bitcoin-cli addconnection "192.168.0.6:8333" "outbound-full-relay"
21error code: -1
22error message:
23addconnection is for regression testing (-regtest mode) only.

stratospher commented at 8:00 am on February 2, 2024: contributor

Another possible alternative for addconnection’s third parameter handling would be to take as default whatever is set by -v2transport, like we already do it for addnode (i.e. bool use_v2transport = self.MaybeArg(2).value_or(node_v2transport);). @theStack, i went with the mandatory third parameter approach because:

this can be used only for interaction between python dummy peer(P2PInterface) and bitcoind(TestNode) on only regtest anyways.
we’re indirectly passing whatever is set by -v2transport (using use_v2transport) into addconnection RPC in this commit in #29358.

open to the optional-argument alternative if there are better reasons for doing that approach!

Ok, didn’t notice it is regtest only. Probably help addconnection should give error instead of usage information if not running against regtest. @kristapsk, makes sense. it’s mentioned in the help that it’s for testing only though and there are other regtest-only RPCs too! So i feel it’s out of scope for this PR.

theStack approved

theStack commented at 9:43 am on February 2, 2024: contributor

Code-review ACK e7fd70f4b6b163f4ad5b25b4da7fa79899245235

Another possible alternative for addconnection’s third parameter handling would be to take as default whatever is set by -v2transport, like we already do it for addnode (i.e. bool use_v2transport = self.MaybeArg(2).value_or(node_v2transport);).

@theStack, i went with the mandatory third parameter approach because:

this can be used only for interaction between python dummy peer(P2PInterface) and bitcoind(TestNode) on only regtest anyways.

we’re indirectly passing whatever is set by -v2transport (using use_v2transport) into addconnection RPC in this commit in test: use v2 everywhere for P2PConnection if –v2transport is enabled #29358.

open to the optional-argument alternative if there are better reasons for doing that approach!

The thinking behind my suggestion was that it could be easier for functional tests that call addconnection directly, if third parameter doesn’t have to be set explicitly. But it seems there is only one of these cases anyway (in feature_anchors.py), so I think the mandatory third parameter approach is fine.

in test/functional/feature_anchors.py:102 in e7fd70f4b6

 98@@ -99,7 +99,7 @@ def run_test(self):
 99         self.restart_node(0, extra_args=[f"-onion={onion_conf.addr[0]}:{onion_conf.addr[1]}"])
100 
101         self.log.info("Add 256-bit-address block-relay-only connections to node")
102-        self.nodes[0].addconnection(ONION_ADDR, 'block-relay-only')
103+        self.nodes[0].addconnection(ONION_ADDR, 'block-relay-only', v2transport=False)

mzumsande commented at 7:15 pm on February 5, 2024:

Unrelated to this PR, but it could be changed from False to self.options.v2transport in the future (e.g. in #29358)

mzumsande commented at 11:16 pm on February 6, 2024:

well, after looking what the tests does it doesn’t matter either way, since we don’t actually make a real connection here…

mzumsande commented at 7:43 pm on February 5, 2024: contributor

Code Review ACK e7fd70f4b6b163f4ad5b25b4da7fa79899245235

Nit, only if you need to re-push: The sentence “TestNode::add_outbound_p2p_connection() is the only place where addconnection test-only RPC is used.” in the commit message is not strictly true because of the other spot in feature_anchors.py.

maflcko added this to the milestone 27.0 on Feb 6, 2024

glozow commented at 11:01 am on February 6, 2024: member

ACK e7fd70f4b6

glozow merged this on Feb 6, 2024

glozow closed this on Feb 6, 2024

test: make v2transport arg in addconnection mandatory and few cleanups #29356

Code Coverage

Reviews