Add support for lists of peers in the config

[felixbrucker]

Purpose:

Currently it is possible to connect to a single peer via the peer info in the config.yaml, for example:

full_node_peer:
  host: *self_hostname
  port: 8444

It is currently not possible to connect to multiple predefined peers, one has to use the cli/rpc to connect to additional peers after startup.

This PR adds support for a list of peers in the config.yaml, the key name just gets a s added, for example:

full_node_peers:
  - host: *self_hostname
    port: 8444
  - host: some.other.host
    port: 8444

Note: This is currently only supported for farmer and full node peers, as imho those are the only ones where multiple peers make sense at the moment.

Current Behavior:

Connecting to multiple predefined peers is not possible.

New Behavior:

When a singular peer info is present in the config it is used as is, same as before, it just gets wrapped into a set with it being the only member. In case the peer list should be present as well both get merged together.
When no singular peer info is present, the peer list is retrieved with a default of an empty set.

Testing Notes:

I added tests for the new get_unresolved_peer_infos() function. Its usages in the start_<service>.py files are not currently tested as im unsure on how to best test those as they use globals and are generally hard to work with (async_main() that is). If there is a recommended way to test those please let me know and i'll happily add tests for those as well.

It's also important to note that i did not change the configure cli, it will continue to write to the singular entry in the config. I added support for farmer and full node node types. In the config there are some more peer infos (harvester, wallet, timelord, introducer) which currently are either unused or rely on being a singular value to set service wide values (enable_private_networks).

[aqk]

This looks like a good idea, and low risk. Thanks, Felix, and good luck with this PR.

[xearl4]

I'd love to see this. I run several nodes which I want to keep in a fully connected mesh with each other. Currently, I achieve this with the peer whitelist and a cronjob that connects a node manually to the desired peers via the CLI. Being able to get rid of that cronjob and directly have this in the config would be great.

Second use-case: since another of Felix' recent PRs was merged, I run my farmer processes connected to two full nodes, for redundancy. Have to manually connect to the second full node, though. And if the farmer loses connection to the secondary node, it does not automatically re-connect (which it does for the primary full node peer from the config) -- so, same cronjob to stay connected to the secondary.

[altendky]

I agree that this feature addition makes sense and is valuable. Thanks Felix for both suggesting it and providing the implementation! Bonus, while reviewing this I found we needed to fix some mutable default parameter values. #16472

---

When a singular peer info is present in the config it is used as is, same as before, it just gets wrapped into a list with it being the only member. In this case the peer list should it be present as well gets ignored. When no singular peer info is present, the peer list is retrieved with a default of an empty list.

Is there a particular reason for this behavior? The first thing that popped into my head was to have the singular peer entry just treated as if it were another entry in the plural peers list. I was thinking that leaving the singular peer entry at all is mostly a backwards compatibility feature (which is good, thanks).

---

For the tests, maybe look at uses o @datacases, a small @pytest.mark.parametrize wrapper we use some, to provide a single parametrized test with the many cases.

[felixbrucker]

Is there a particular reason for this behavior? The first thing that popped into my head was to have the singular peer entry just treated as if it were another entry in the plural peers list. I was thinking that leaving the singular peer entry at all is mostly a backwards compatibility feature (which is good, thanks).

No, it would probably make sense to merge them in this case yes 👍

[felixbrucker]

For the tests, maybe look at uses o @datacases, a small @pytest.mark.parametrize wrapper we use some, to provide a single parametrized test with the many cases.

This is only for the tests i wrote, right? Together with the change to assert a set of unresolved peer infos this will work nicely

Do you have suggestions regarding testing the async_main()s?

[altendky]

For the tests, maybe look at uses o @datacases, a small @pytest.mark.parametrize wrapper we use some, to provide a single parametrized test with the many cases.

This is only for the tests i wrote, right? Together with the change to assert a set of unresolved peer infos this will work nicely

Yes, that comment is just about the pattern for coding the tests.

Do you have suggestions regarding testing the async_main()s?

If you really want to go for this, you could setup a temporary root (we have a fixture), edit the config (we have helper functions and patterns) then setup the services you want it to connect (we have fixtures), call the async_main() (probably in a separate task) and make sure it connects to the desired peers (presumably we can look at some objects directly and not have to parse logs etc). I won't require you achieve coverage for this PR to get merged, but if you want to then I can try to piece together some reference links and think through some of the details. Note that if you do this right now there's room for some issues related to #16472 potentially. They may or may not show up, I didn't look in detail.

[felixbrucker]

Yeah, testing the async_main()s feels like a lot of pain, i actually tried something similar to what you described, but it didn't work properly (the async_main uses the DEFAULT_ROOT, and i couldn't overwrite it)

i'll omit tests for those for now 👍

[altendky]

Yep, plenty of corners and we might need to adjust some params etc.

[altendky]

I think it would be good to have this new configuration in the initial-config.yaml file. I think we can remove the existing singular case and add the peer listed there as the only entry in the new plural list.

Once we have this ready, I'll create an ephemeral testnet to make sure everything still launches ok. This will be a partial stop gap for the lack of top level integration test coverage.

[felixbrucker]

I think we can remove the existing singular case and add the peer listed there as the only entry in the new plural list.

Seems like some tests are failing, because the configure command assumes the key is present

What should be its behaviour when only a list is present?

[altendky]

It reads to me like that code should be updated to set the plural list value for the if case and remove both for the else case. Maybe use the pop method with a default of None instead of del to avoid failures when either key is not there.

[felixbrucker]

set the plural list value

which entry in the list would you update, the first?

for the if case and remove both for the else case.

Sadly im not following here, why would we remove anything?

[altendky]

At a glance, this is what I was thinking.

diff --git a/chia/simulator/setup_services.py b/chia/simulator/setup_services.py
index 799134941..6582b7ee4 100644
--- a/chia/simulator/setup_services.py
+++ b/chia/simulator/setup_services.py
@@ -309,11 +309,15 @@ async def setup_wallet_node(
             service_config["introducer_peer"] = None
 
         if full_node_port is not None:
-            service_config["full_node_peer"] = {}
-            service_config["full_node_peer"]["host"] = self_hostname
-            service_config["full_node_peer"]["port"] = full_node_port
+            service_config["full_node_peers"] = [
+                {
+                    "host": self_hostname,
+                    "port": full_node_port,
+                },
+            ]
         else:
-            del service_config["full_node_peer"]
+            service_config.pop("full_node_peer", None)
+            service_config.pop("full_node_peers", None)
 
         service = create_wallet_service(
             local_bt.root_path,
@@ -413,11 +417,16 @@ async def setup_farmer(
     service_config["rpc_port"] = uint16(0)
     config_pool["xch_target_address"] = encode_puzzle_hash(b_tools.pool_ph, "xch")
 
-    if full_node_port:
-        service_config["full_node_peer"]["host"] = self_hostname
-        service_config["full_node_peer"]["port"] = full_node_port
+    if full_node_port is not None:
+        service_config["full_node_peers"] = [
+            {
+                "host": self_hostname,
+                "port": full_node_port,
+            },
+        ]
     else:
-        del service_config["full_node_peer"]
+        service_config.pop("full_node_peer", None)
+        service_config.pop("full_node_peers", None)
 
     service = create_farmer_service(
         root_path,

[felixbrucker]

Oh you are talking about the simulator, yeah, that looks good

i was talking about the configure cmd used on the cli, which currently will break as well

im unsure how this command should work with a list, as we probably cant uniquely identify the "self" peer

[altendky]

I'm going to chat around about this. Maybe ping me tomorrow if I haven't gotten an answer to you here.

[emlowe]

I think this might make some sense for wallet as well if you want to "hardcode" some nodes for a wallet to use.

I believe you are referring to chia configure --set-fullnode-port. I might make it work as follows:
Change the port in full_node_peer if present, otherwise, change the port for the first entry in full_node_peers.

It's seems mostly a testing option, and I don't know how much it's really used.

[altendky]

Chris pointed out --set-farmer-peer.

[felixbrucker]

I think this might make some sense for wallet as well if you want to "hardcode" some nodes for a wallet to use.

That is already done yeah 👍

I believe you are referring to chia configure --set-fullnode-port. I might make it work as follows:
Change the port in full_node_peer if present, otherwise, change the port for the first entry in full_node_peers.

Yeah, setting the first entries port was my first thought as well, as that is very likely the self peer in most cases

Chris pointed out --set-farmer-peer.

I'd probably handle it the same here, for the full node it just gets updated same as before as that did not change, and for the harvester its the first entry as well

[felixbrucker]

Rebased to current main to hopefully get rid of test flakes

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[emlowe]

Coverage diff exemption

Much thanks for the PR