connect_nodes - account for test networks

[mplsgrant]

Issue

Sometimes connect_nodes produces an error:

2024-06-24T23:39:30+0000 - INFO -     tank_index = int(peer['addr'].split('-')[2])  # NETWORK-tank-INDEX-service
2024-06-24T23:39:30+0000 - INFO -                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2024-06-24T23:39:30+0000 - INFO - ValueError: invalid literal for int() with base 10: 'vqyr11r'

Cause

connect_nodes does not handle test networks properly. Specifically, it handles this dns format:

NETWORK-tank-INDEX-service

But not the "test" format:

NETWORK-test-TEST-tank-INDEX-service

Solution

Modify connect_nodes to handle the "test" format, and update the test coverage to account for this.

[mplsgrant]

Updated with simplified tank index extraction logic.

I also took a moment to pull clarify the connect dag scenario by using a function to make assertions. This makes it easier to add new assertions such as for nodes 8 and 9.

[pinheadmz]

Looking at this again, I think the entire connect_dag scenario should just be a test, or can be copied into dag_connection_test. It is written very specifically for one graph file and is full of assert statements. Its not like the other scenarios that are written more generically and can apply to a broad range of graphs.

[mplsgrant]

@pinheadmz

I think the entire connect_dag scenario should just be a test, or can be copied into dag_connection_test.

I like this idea, but I don't think we have easy access to WarnetTestFramework from TestBase.

[pinheadmz]

I like this idea, but I don't think we have easy access to WarnetTestFramework from TestBase.

oh right i forgot we were going back and forth about using a scenario just for testing, hmmmmmm. It still feels anti-pattern to have the asserts in a scenario instead of a test, but then it also will be silly to split up the connect_nodes calls and the asserts into two separate files.

One way this might make more sense, and im not married to this but lets just jam on it a sec:

• move the asserts into the test, leaving the connect_dag scenario very simple, just makes connections
• move the connect_dag file into test/data
• in the test, call warcli scenarios run_file <path>

this makes a bit more sense to me because first of all i dont think we cover that particular rpc in any test, and second of all it demonstrates using a "custom" scenario that is written for a specific graph and not intended for general applications

[mplsgrant]

I like your thinking about splitting connect_dag.py away from scenarios and into some other area that we have designated for testing. I think calling warcli scenario on a specific file in the testing area is probably a good way to do that.

In terms of specifics:

move the connect_dag file into test/data

I'm fine with test/data. However, I think it would be even better to make a folder called "warnet_test_framework_files" or "tank_tests" or similar. That way, this area would be where we place test files that specifically use the test framework/tank features and are not simply files containing plain data.

move the asserts into the test, leaving the connect_dag scenario very simple, just makes connections

I like keeping the assert_connection in connect_dag.py because assert_connection is tied really closely to the Tanks. Being tied to the Tanks means to me that we are not testing bitcoin nodes as such, but we are actually testing the interplay between bitcoin nodes and their respective tanks.

edit: Just to clarify, I also don't think we can move the asserts into the TestBase test b/c of the need to work with Tanks directly.

in the test, call warcli scenarios run_file <path>

This sounds like it will work and I like it.

[mplsgrant]

@pinheadmz I moved the scenario file and incorporated run-file as discussed.

[pinheadmz]

👍 OKIE DOKE!

[mplsgrant]

@pinheadmz I updated the wording on that final log entry.

LGTM