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

stratospher · 2024-01-31T14:48:10Z

make v2transport argument in addconnection regression-testing only RPC mandatory. test/BIP324: functional tests for v2 P2P encryption #24748 (comment)
- 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() test/BIP324: functional tests for v2 P2P encryption #24748 (comment)
more compact return statement in wait_for_reconnect() test/BIP324: functional tests for v2 P2P encryption #24748 (comment)
assertion to check that empty version packets are received from TestNode.

DrahtBot · 2024-01-31T14:48:13Z

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.

`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.

theStack · 2024-01-31T18:01:02Z

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);).

kristapsk · 2024-01-31T19:38:14Z

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 · 2024-01-31T19:44:12Z

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 · 2024-01-31T19:49:00Z

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:

bitcoin@odroid:~$ bitcoin-cli help | grep addconnection
bitcoin@odroid:~$ bitcoin-cli help addconnection       
addconnection "address" "connection_type"

Open an outbound connection to a specified node. This RPC is for testing only.

Arguments:
1. address            (string, required) The IP address and port to attempt connecting to.
2. connection_type    (string, required) Type of connection to open ("outbound-full-relay", "block-relay-only", "addr-fetch" or "feeler").

Result:
{                               (json object)
  "address" : "str",            (string) Address of newly added connection.
  "connection_type" : "str"     (string) Type of connection opened.
}

Examples:
> bitcoin-cli addconnection "192.168.0.6:8333" "outbound-full-relay"
> 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/

bitcoin@odroid:~$ bitcoin-cli addconnection "192.168.0.6:8333" "outbound-full-relay"
error code: -1
error message:
addconnection is for regression testing (-regtest mode) only.

stratospher · 2024-02-02T08:00:22Z

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!

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

Code-review ACK e7fd70f

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.

mzumsande

Code Review ACK e7fd70f

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.

mzumsande · 2024-02-05T19:15:48Z

test/functional/feature_anchors.py

@@ -99,7 +99,7 @@ def run_test(self):
        self.restart_node(0, extra_args=[f"-onion={onion_conf.addr[0]}:{onion_conf.addr[1]}"])

        self.log.info("Add 256-bit-address block-relay-only connections to node")
-        self.nodes[0].addconnection(ONION_ADDR, 'block-relay-only')
+        self.nodes[0].addconnection(ONION_ADDR, 'block-relay-only', v2transport=False)


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

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

glozow · 2024-02-06T11:01:39Z

ACK e7fd70f

…and few cleanups

, bitcoin#29353, bitcoin#29452, bitcoin#29483, bitcoin#30545, bitcoin#31383 (BIP324 backports: part 5) c6f23a7 merge bitcoin#31383: Add missing node.setmocktime(self.mocktime) to p2p_ibd_stalling.py (Kittywhiskers Van Gogh) 9072a10 merge bitcoin#30545: fix intermittent failures in feature_proxy.py (Kittywhiskers Van Gogh) 7e2d435 merge bitcoin#29483: add --v1transport option, add --v2transport to a CI task (Kittywhiskers Van Gogh) 7e59a96 merge bitcoin#29452: document that BIP324 on by default (Kittywhiskers Van Gogh) 0f3b5e0 merge bitcoin#29353: adhere to typical VERSION message protocol flow (Kittywhiskers Van Gogh) dfdddfd rpc: enable `v2transport` for `masternode connect` when capable (Kittywhiskers Van Gogh) 3c16361 merge bitcoin#29356: make v2transport arg in addconnection mandatory and few cleanups (Kittywhiskers Van Gogh) c53cd93 merge bitcoin#29347: enable v2transport by default (Kittywhiskers Van Gogh) 7dcf561 merge bitcoin#27452: cover addrv2 anchors by adding TorV3 to CAddress in messages.py (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * When backporting [bitcoin#27452](bitcoin#27452) in `feature_anchors.py`, `P2P_SERVICES` (`NODE_NETWORK | NODE_HEADERS_COMPRESSED`) has been replaced with `NODE_NETWORK` as the former evaluates to a value greater than `256` (specifically `2049`), which causes test failure. The replacement value is acceptable as `NODE_NETWORK` is the desired service flag expected by Dash Core ([source](https://github.com/dashpay/dash/blob/779e4295ad253bf909a7c45ec02743e580abc61b/src/protocol.cpp#L249-L254)). <details> <summary>Test failure:</summary> ``` dash@89afd55ae77e:/src/dash$ ./test/functional/feature_anchors.py 2024-12-14T12:31:22.244000Z TestFramework (INFO): PRNG seed is: 8365703189892653614 2024-12-14T12:31:22.244000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_j0ya02yu 2024-12-14T12:31:22.776000Z TestFramework (INFO): When node starts, check if anchors.dat doesn't exist 2024-12-14T12:31:22.776000Z TestFramework (INFO): Add 2 block-relay-only connections to node 2024-12-14T12:31:24.781000Z TestFramework (INFO): Add 5 inbound connections to node 2024-12-14T12:31:29.843000Z TestFramework (INFO): Check node connections 2024-12-14T12:31:30.848000Z TestFramework (INFO): Check the addresses in anchors.dat 2024-12-14T12:31:30.848000Z TestFramework (INFO): Perturb anchors.dat to test it doesn't throw an error during initialization 2024-12-14T12:31:31.356000Z TestFramework (INFO): When node starts, check if anchors.dat doesn't exist anymore 2024-12-14T12:31:31.357000Z TestFramework (INFO): Ensure addrv2 support 2024-12-14T12:31:32.364000Z TestFramework (INFO): Add 256-bit-address block-relay-only connections to node 2024-12-14T12:31:33.368000Z TestFramework (INFO): Check for addrv2 addresses in anchors.dat 2024-12-14T12:31:33.369000Z TestFramework (ERROR): Unexpected exception caught during testing Traceback (most recent call last): File "/src/dash/test/functional/test_framework/test_framework.py", line 161, in main self.run_test() File "/src/dash/./test/functional/feature_anchors.py", line 135, in run_test new_data[services_index] = P2P_SERVICES ValueError: byte must be in range(0, 256) 2024-12-14T12:31:33.870000Z TestFramework (INFO): Stopping nodes 2024-12-14T12:31:33.871000Z TestFramework (WARNING): Not cleaning up dir /tmp/dash_func_test_j0ya02yu 2024-12-14T12:31:33.871000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/dash_func_test_j0ya02yu/test_framework.log ``` </details> ## Breaking Changes None expected. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK c6f23a7 PastaPastaPasta: utACK c6f23a7; Tree-SHA512: ca134432d000d521827a20c75c03913421fe52a31fda1cbf632e0b207c31728406feb090469d592d8e2fd8d64298faa2752ff703de79f737a62c276c6a231097

fanquake changed the title ~~[test] make v2transport arg in addconnection mandatory and few cleanups~~ test: make v2transport arg in addconnection mandatory and few cleanups Jan 31, 2024

DrahtBot added the Tests label Jan 31, 2024

stratospher mentioned this pull request Jan 31, 2024

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

DrahtBot added the CI failed label Jan 31, 2024

stratospher force-pushed the 24748-followups branch from 5dc0f28 to e7fd70f Compare January 31, 2024 17:08

DrahtBot removed the CI failed label Jan 31, 2024

theStack approved these changes Feb 2, 2024

View reviewed changes

mzumsande reviewed Feb 5, 2024

View reviewed changes

mzumsande mentioned this pull request Feb 5, 2024

test: use v2 everywhere for P2PConnection if --v2transport is enabled #29358

Merged

maflcko added this to the 27.0 milestone Feb 6, 2024

glozow merged commit 4de8455 into bitcoin:master Feb 6, 2024
16 checks passed

stratospher deleted the 24748-followups branch July 17, 2024 06:39

kwvg added a commit to kwvg/dash that referenced this pull request Dec 14, 2024

merge bitcoin#29356: make v2transport arg in addconnection mandatory …

3c16361

…and few cleanups

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stratospher commented Jan 31, 2024 •

edited

Loading

DrahtBot commented Jan 31, 2024 •

edited

Loading

theStack commented Jan 31, 2024

kristapsk commented Jan 31, 2024

mzumsande commented Jan 31, 2024

kristapsk commented Jan 31, 2024

stratospher commented Feb 2, 2024

theStack left a comment

mzumsande left a comment

mzumsande Feb 5, 2024

mzumsande Feb 6, 2024 •

edited

Loading

glozow commented Feb 6, 2024

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

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

Conversation

stratospher commented Jan 31, 2024 • edited Loading

DrahtBot commented Jan 31, 2024 • edited Loading

Code Coverage

Reviews

theStack commented Jan 31, 2024

kristapsk commented Jan 31, 2024

mzumsande commented Jan 31, 2024

kristapsk commented Jan 31, 2024

stratospher commented Feb 2, 2024

theStack left a comment

Choose a reason for hiding this comment

mzumsande left a comment

Choose a reason for hiding this comment

mzumsande Feb 5, 2024

Choose a reason for hiding this comment

mzumsande Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

glozow commented Feb 6, 2024

stratospher commented Jan 31, 2024 •

edited

Loading

DrahtBot commented Jan 31, 2024 •

edited

Loading

mzumsande Feb 6, 2024 •

edited

Loading