Repository: Chia-Network/chia-blockchain
Pull request: Small improvements to trusted peer check to include IPv6 addresses and add tests
Refactor function `is_trusted_peer` to a standalone easily testable function. Leverage the existing `is_localhost` function to extend coverage to IPv6 local address for the trusted peer check

Added tests for the new standalone function

Problem statement:
[Bug] Wallet does not trust local node over IPv6
### What happened?

If you configure `self-hostname` to be `::1`, or something that resolves to the IPv6 loopback address (for example, `ip6-localhost`) in `config.yaml`, and configure the wallet to use this as the full node peer, the wallet will connect to the node, however, the node is not trusted. You can also configure `wallet:full_node_peer:host` to be `::1` - this also is not trusted

Note that practically this may not be that common because typically the default setting of `localhost` resolves to the IPv4 loopback address and it seems quite difficult to avoid using IPv4 loopback on most systems. However, you can manually configure it in a variety of ways and it should probably work


### Version

1.7.0

### What platform are you using?

Windows

### What ui mode are you using?

GUI

### Relevant log output

```shell
N/A
```

Hints from issue discussion:

Commit message: refactor is_trusted_peer

Patch to review:
diff --git a/chia/server/server.py b/chia/server/server.py
index 766201956c7f..5ca5eb90fdbd 100644
--- a/chia/server/server.py
+++ b/chia/server/server.py
@@ -36,7 +36,7 @@
 from chia.types.peer_info import PeerInfo
 from chia.util.errors import Err, ProtocolError
 from chia.util.ints import uint16
-from chia.util.network import WebServer, is_in_network, is_localhost
+from chia.util.network import WebServer, is_in_network, is_localhost, is_trusted_peer
 from chia.util.ssl_check import verify_ssl_certs_and_keys
 
 max_message_size = 50 * 1024 * 1024  # 50MB
@@ -681,14 +681,12 @@ def accept_inbound_connections(self, node_type: NodeType) -> bool:
         return True
 
     def is_trusted_peer(self, peer: WSChiaConnection, trusted_peers: Dict[str, Any]) -> bool:
-        if trusted_peers is None:
-            return False
-        if not self.config.get("testing", False) and peer.peer_host == "127.0.0.1":
-            return True
-        if peer.peer_node_id.hex() not in trusted_peers:
-            return False
-
-        return True
+        return is_trusted_peer(
+            host=peer.peer_host,
+            node_id=peer.peer_node_id,
+            trusted_peers=trusted_peers,
+            testing=self.config.get("testing", False),
+        )
 
     def set_capabilities(self, capabilities: List[Tuple[uint16, str]]) -> None:
         self._local_capabilities_for_handshake = capabilities
diff --git a/chia/util/network.py b/chia/util/network.py
index 3a349dd2eda2..7361086e3926 100644
--- a/chia/util/network.py
+++ b/chia/util/network.py
@@ -113,6 +113,13 @@ def is_localhost(peer_host: str) -> bool:
     return peer_host == "127.0.0.1" or peer_host == "localhost" or peer_host == "::1" or peer_host == "0:0:0:0:0:0:0:1"
 
 
+def is_trusted_peer(host: str, node_id: bytes32, trusted_peers: Dict[str, Any], testing: bool = False) -> bool:
+    if testing or is_localhost(host) or node_id.hex() in trusted_peers:
+        return True
+
+    return False
+
+
 def class_for_type(type: NodeType) -> Any:
     if type is NodeType.FULL_NODE:
         from chia.full_node.full_node_api import FullNodeAPI
diff --git a/chia/wallet/wallet_node.py b/chia/wallet/wallet_node.py
index b5995888d643..cdfd77bea3c5 100644
--- a/chia/wallet/wallet_node.py
+++ b/chia/wallet/wallet_node.py
@@ -937,7 +937,7 @@ async def is_peer_synced(
         return latest_timestamp
 
     def is_trusted(self, peer) -> bool:
-        return self.server.is_trusted_peer(peer, self.config["trusted_peers"])
+        return self.server.is_trusted_peer(peer, self.config.get("trusted_peers", {}))
 
     def add_state_to_race_cache(self, header_hash: bytes32, height: uint32, coin_state: CoinState) -> None:
         # Clears old state that is no longer relevant
diff --git a/tests/util/test_trusted_peer.py b/tests/util/test_trusted_peer.py
new file mode 100644
index 000000000000..aec425c91311
--- /dev/null
+++ b/tests/util/test_trusted_peer.py
@@ -0,0 +1,28 @@
+from __future__ import annotations
+
+from typing import Any, Dict
+
+import pytest
+
+from chia.types.blockchain_format.sized_bytes import bytes32
+from chia.util.network import is_trusted_peer
+
+
+@pytest.mark.parametrize(
+    "host,node_id,trusted_peers,testing,result",
+    [
+        ("::1", 0, {}, False, True),
+        ("::1", bytes32(b"d" * 32), {bytes32(b"a" * 32).hex(): "0"}, False, True),
+        ("127.0.0.1", 0, {}, False, True),
+        ("localhost", 0, {}, False, True),
+        ("0:0:0:0:0:0:0:1", 0, {}, False, True),
+        ("2000:1000::1234:abcd", 0, {}, True, True),  # testing=True
+        ("10.11.12.13", bytes32(b"d" * 32), {bytes32(b"a" * 32).hex(): "0"}, False, False),
+        ("10.11.12.13", bytes32(b"d" * 32), {bytes32(b"d" * 32).hex(): "0"}, False, True),
+        ("10.11.12.13", bytes32(b"d" * 32), {}, False, False),
+    ],
+)
+def test_is_trusted_peer(
+    host: str, node_id: bytes32, trusted_peers: Dict[str, Any], testing: bool, result: bool
+) -> None:
+    assert is_trusted_peer(host=host, node_id=node_id, trusted_peers=trusted_peers, testing=testing) == result


Code review comments:
diff_hunk: @@ -113,6 +113,13 @@ def is_localhost(peer_host: str) -> bool:
     return peer_host == "127.0.0.1" or peer_host == "localhost" or peer_host == "::1" or peer_host == "0:0:0:0:0:0:0:1"
 
 
+def is_trusted_peer(host: str, node_id: bytes32, trusted_peers: Dict[str, Any], testing: bool = False) -> bool:
+    if testing or is_localhost(host) or node_id.hex() in trusted_peers:
path: chia/util/network.py
text: @user1:
```suggestion
    if not testing and is_localhost(host) or node_id.hex() in trusted_peers:
```

You changed the behaviour here it seems? With this we would just always return `True` if `testing` is `True`. 



@author:
You are correct, I had this backward I think. It seems "testing" in this case means that the code should **not** trust localhost. 

@user1:
Yeah its just to not have all nodes be trusted nodes during tests but still allow to see localhost as trusted in production from my understanding.

@author:
I think I fixed this