-
Notifications
You must be signed in to change notification settings - Fork 182
Refactor: Replace magic numbers with named constants and enums for clarity and maintainability #917
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4a36d6e
8793667
547bbf1
9d51952
029dcfc
21dd467
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
dataclass, | ||
field, | ||
) | ||
from enum import Flag, auto | ||
|
||
from libp2p.peer.peerinfo import ( | ||
PeerInfo, | ||
|
@@ -18,38 +19,127 @@ | |
RelayLimits, | ||
) | ||
|
||
DEFAULT_MIN_RELAYS = 3 | ||
DEFAULT_MAX_RELAYS = 20 | ||
DEFAULT_DISCOVERY_INTERVAL = 300 # seconds | ||
DEFAULT_RESERVATION_TTL = 3600 # seconds | ||
DEFAULT_MAX_CIRCUIT_DURATION = 3600 # seconds | ||
DEFAULT_MAX_CIRCUIT_BYTES = 1024 * 1024 * 1024 # 1GB | ||
|
||
DEFAULT_MAX_CIRCUIT_CONNS = 8 | ||
DEFAULT_MAX_RESERVATIONS = 4 | ||
|
||
MAX_RESERVATIONS_PER_IP = 8 | ||
MAX_CIRCUITS_PER_IP = 16 | ||
RESERVATION_RATE_PER_IP = 4 # per minute | ||
CIRCUIT_RATE_PER_IP = 8 # per minute | ||
MAX_CIRCUITS_TOTAL = 64 | ||
MAX_RESERVATIONS_TOTAL = 32 | ||
MAX_BANDWIDTH_PER_CIRCUIT = 1024 * 1024 # 1MB/s | ||
MAX_BANDWIDTH_TOTAL = 10 * 1024 * 1024 # 10MB/s | ||
|
||
MIN_RELAY_SCORE = 0.5 | ||
MAX_RELAY_LATENCY = 1.0 # seconds | ||
ENABLE_AUTO_RELAY = True | ||
AUTO_RELAY_TIMEOUT = 30 # seconds | ||
MAX_AUTO_RELAY_ATTEMPTS = 3 | ||
RESERVATION_REFRESH_THRESHOLD = 0.8 # Refresh at 80% of TTL | ||
MAX_CONCURRENT_RESERVATIONS = 2 | ||
|
||
# Timeout constants for different components | ||
DEFAULT_DISCOVERY_STREAM_TIMEOUT = 10 # seconds | ||
DEFAULT_PEER_PROTOCOL_TIMEOUT = 5 # seconds | ||
DEFAULT_PROTOCOL_READ_TIMEOUT = 15 # seconds | ||
DEFAULT_PROTOCOL_WRITE_TIMEOUT = 15 # seconds | ||
DEFAULT_PROTOCOL_CLOSE_TIMEOUT = 10 # seconds | ||
DEFAULT_DCUTR_READ_TIMEOUT = 30 # seconds | ||
DEFAULT_DCUTR_WRITE_TIMEOUT = 30 # seconds | ||
DEFAULT_DIAL_TIMEOUT = 10 # seconds | ||
|
||
|
||
@dataclass | ||
class TimeoutConfig: | ||
"""Timeout configuration for different Circuit Relay v2 components.""" | ||
|
||
# Discovery timeouts | ||
discovery_stream_timeout: int = DEFAULT_DISCOVERY_STREAM_TIMEOUT | ||
peer_protocol_timeout: int = DEFAULT_PEER_PROTOCOL_TIMEOUT | ||
|
||
# Core protocol timeouts | ||
protocol_read_timeout: int = DEFAULT_PROTOCOL_READ_TIMEOUT | ||
protocol_write_timeout: int = DEFAULT_PROTOCOL_WRITE_TIMEOUT | ||
protocol_close_timeout: int = DEFAULT_PROTOCOL_CLOSE_TIMEOUT | ||
|
||
# DCUtR timeouts | ||
dcutr_read_timeout: int = DEFAULT_DCUTR_READ_TIMEOUT | ||
dcutr_write_timeout: int = DEFAULT_DCUTR_WRITE_TIMEOUT | ||
dial_timeout: int = DEFAULT_DIAL_TIMEOUT | ||
|
||
|
||
# Relay roles enum | ||
class RelayRole(Flag): | ||
""" | ||
Bit-flag enum that captures the three possible relay capabilities. | ||
|
||
A node can combine multiple roles using bit-wise OR, for example:: | ||
|
||
RelayRole.HOP | RelayRole.STOP | ||
""" | ||
|
||
HOP = auto() # Act as a relay for others ("hop") | ||
STOP = auto() # Accept relayed connections ("stop") | ||
CLIENT = auto() # Dial through existing relays ("client") | ||
|
||
|
||
@dataclass | ||
class RelayConfig: | ||
"""Configuration for Circuit Relay v2.""" | ||
|
||
# Role configuration | ||
enable_hop: bool = False # Whether to act as a relay (hop) | ||
enable_stop: bool = True # Whether to accept relayed connections (stop) | ||
enable_client: bool = True # Whether to use relays for dialing | ||
# Role configuration (bit-flags) | ||
roles: RelayRole = RelayRole.STOP | RelayRole.CLIENT | ||
|
||
# Resource limits | ||
limits: RelayLimits | None = None | ||
|
||
# Discovery configuration | ||
bootstrap_relays: list[PeerInfo] = field(default_factory=list) | ||
min_relays: int = 3 | ||
max_relays: int = 20 | ||
discovery_interval: int = 300 # seconds | ||
min_relays: int = DEFAULT_MIN_RELAYS | ||
max_relays: int = DEFAULT_MAX_RELAYS | ||
discovery_interval: int = DEFAULT_DISCOVERY_INTERVAL | ||
|
||
# Connection configuration | ||
reservation_ttl: int = 3600 # seconds | ||
max_circuit_duration: int = 3600 # seconds | ||
max_circuit_bytes: int = 1024 * 1024 * 1024 # 1GB | ||
reservation_ttl: int = DEFAULT_RESERVATION_TTL | ||
max_circuit_duration: int = DEFAULT_MAX_CIRCUIT_DURATION | ||
max_circuit_bytes: int = DEFAULT_MAX_CIRCUIT_BYTES | ||
Comment on lines
+98
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be better to make an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yupp @lla-dane , having a look, I think that is a great idea. |
||
|
||
# Timeout configuration | ||
timeouts: TimeoutConfig = field(default_factory=TimeoutConfig) | ||
|
||
# --------------------------------------------------------------------- | ||
# Backwards-compat boolean helpers. Existing code that still accesses | ||
# ``cfg.enable_hop, cfg.enable_stop, cfg.enable_client`` will continue to work. | ||
# --------------------------------------------------------------------- | ||
|
||
@property | ||
def enable_hop(self) -> bool: # pragma: no cover – helper | ||
return bool(self.roles & RelayRole.HOP) | ||
|
||
@property | ||
def enable_stop(self) -> bool: # pragma: no cover – helper | ||
return bool(self.roles & RelayRole.STOP) | ||
|
||
@property | ||
def enable_client(self) -> bool: # pragma: no cover – helper | ||
return bool(self.roles & RelayRole.CLIENT) | ||
|
||
def __post_init__(self) -> None: | ||
"""Initialize default values.""" | ||
if self.limits is None: | ||
self.limits = RelayLimits( | ||
duration=self.max_circuit_duration, | ||
data=self.max_circuit_bytes, | ||
max_circuit_conns=8, | ||
max_reservations=4, | ||
max_circuit_conns=DEFAULT_MAX_CIRCUIT_CONNS, | ||
max_reservations=DEFAULT_MAX_RESERVATIONS, | ||
) | ||
|
||
|
||
|
@@ -58,35 +148,35 @@ class HopConfig: | |
"""Configuration specific to relay (hop) nodes.""" | ||
|
||
# Resource limits per IP | ||
max_reservations_per_ip: int = 8 | ||
max_circuits_per_ip: int = 16 | ||
max_reservations_per_ip: int = MAX_RESERVATIONS_PER_IP | ||
max_circuits_per_ip: int = MAX_CIRCUITS_PER_IP | ||
|
||
# Rate limiting | ||
reservation_rate_per_ip: int = 4 # per minute | ||
circuit_rate_per_ip: int = 8 # per minute | ||
reservation_rate_per_ip: int = RESERVATION_RATE_PER_IP | ||
circuit_rate_per_ip: int = CIRCUIT_RATE_PER_IP | ||
|
||
# Resource quotas | ||
max_circuits_total: int = 64 | ||
max_reservations_total: int = 32 | ||
max_circuits_total: int = MAX_CIRCUITS_TOTAL | ||
max_reservations_total: int = MAX_RESERVATIONS_TOTAL | ||
|
||
# Bandwidth limits | ||
max_bandwidth_per_circuit: int = 1024 * 1024 # 1MB/s | ||
max_bandwidth_total: int = 10 * 1024 * 1024 # 10MB/s | ||
max_bandwidth_per_circuit: int = MAX_BANDWIDTH_PER_CIRCUIT | ||
max_bandwidth_total: int = MAX_BANDWIDTH_TOTAL | ||
|
||
|
||
@dataclass | ||
class ClientConfig: | ||
"""Configuration specific to relay clients.""" | ||
|
||
# Relay selection | ||
min_relay_score: float = 0.5 | ||
max_relay_latency: float = 1.0 # seconds | ||
min_relay_score: float = MIN_RELAY_SCORE | ||
max_relay_latency: float = MAX_RELAY_LATENCY | ||
|
||
# Auto-relay settings | ||
enable_auto_relay: bool = True | ||
auto_relay_timeout: int = 30 # seconds | ||
max_auto_relay_attempts: int = 3 | ||
enable_auto_relay: bool = ENABLE_AUTO_RELAY | ||
auto_relay_timeout: int = AUTO_RELAY_TIMEOUT | ||
max_auto_relay_attempts: int = MAX_AUTO_RELAY_ATTEMPTS | ||
|
||
# Reservation management | ||
reservation_refresh_threshold: float = 0.8 # Refresh at 80% of TTL | ||
max_concurrent_reservations: int = 2 | ||
reservation_refresh_threshold: float = RESERVATION_REFRESH_THRESHOLD | ||
max_concurrent_reservations: int = MAX_CONCURRENT_RESERVATIONS |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,11 @@ | |
from libp2p.peer.peerinfo import ( | ||
PeerInfo, | ||
) | ||
from libp2p.relay.circuit_v2.config import ( | ||
DEFAULT_DCUTR_READ_TIMEOUT, | ||
DEFAULT_DCUTR_WRITE_TIMEOUT, | ||
DEFAULT_DIAL_TIMEOUT, | ||
) | ||
from libp2p.relay.circuit_v2.nat import ( | ||
ReachabilityChecker, | ||
) | ||
|
@@ -47,11 +52,7 @@ | |
# Maximum message size for DCUtR (4KiB as per spec) | ||
MAX_MESSAGE_SIZE = 4 * 1024 | ||
|
||
# Timeouts | ||
STREAM_READ_TIMEOUT = 30 # seconds | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is being changed from 30 to 15. Are you sure that's appropriate here? Should be mentioned in a newsfragment if it is to be changed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed this as well, there are two STREAM_READ_TIMEOUTS, whose value differed in dcutr & protocol files, so I have now defined them locally in the respective files in the new commit, should fix. Thanks! |
||
STREAM_WRITE_TIMEOUT = 30 # seconds | ||
DIAL_TIMEOUT = 10 # seconds | ||
|
||
# DCUtR protocol constants | ||
# Maximum number of hole punch attempts per peer | ||
MAX_HOLE_PUNCH_ATTEMPTS = 5 | ||
|
||
|
@@ -70,18 +71,33 @@ class DCUtRProtocol(Service): | |
hole punching, after they have established an initial connection through a relay. | ||
""" | ||
|
||
def __init__(self, host: IHost): | ||
def __init__( | ||
self, | ||
host: IHost, | ||
read_timeout: int = DEFAULT_DCUTR_READ_TIMEOUT, | ||
write_timeout: int = DEFAULT_DCUTR_WRITE_TIMEOUT, | ||
dial_timeout: int = DEFAULT_DIAL_TIMEOUT, | ||
): | ||
""" | ||
Initialize the DCUtR protocol. | ||
|
||
Parameters | ||
---------- | ||
host : IHost | ||
The libp2p host this protocol is running on | ||
read_timeout : int | ||
Timeout for stream read operations, in seconds | ||
write_timeout : int | ||
Timeout for stream write operations, in seconds | ||
dial_timeout : int | ||
Timeout for dial operations, in seconds | ||
|
||
""" | ||
super().__init__() | ||
self.host = host | ||
self.read_timeout = read_timeout | ||
self.write_timeout = write_timeout | ||
self.dial_timeout = dial_timeout | ||
self.event_started = trio.Event() | ||
self._hole_punch_attempts: dict[ID, int] = {} | ||
self._direct_connections: set[ID] = set() | ||
|
@@ -161,7 +177,7 @@ async def _handle_dcutr_stream(self, stream: INetStream) -> None: | |
|
||
try: | ||
# Read the CONNECT message | ||
with trio.fail_after(STREAM_READ_TIMEOUT): | ||
with trio.fail_after(self.read_timeout): | ||
msg_bytes = await stream.read(MAX_MESSAGE_SIZE) | ||
|
||
# Parse the message | ||
|
@@ -196,7 +212,7 @@ async def _handle_dcutr_stream(self, stream: INetStream) -> None: | |
response.type = HolePunch.CONNECT | ||
response.ObsAddrs.extend(our_addrs) | ||
|
||
with trio.fail_after(STREAM_WRITE_TIMEOUT): | ||
with trio.fail_after(self.write_timeout): | ||
await stream.write(response.SerializeToString()) | ||
|
||
logger.debug( | ||
|
@@ -206,7 +222,7 @@ async def _handle_dcutr_stream(self, stream: INetStream) -> None: | |
) | ||
|
||
# Wait for SYNC message | ||
with trio.fail_after(STREAM_READ_TIMEOUT): | ||
with trio.fail_after(self.read_timeout): | ||
sync_bytes = await stream.read(MAX_MESSAGE_SIZE) | ||
|
||
# Parse the SYNC message | ||
|
@@ -300,7 +316,7 @@ async def initiate_hole_punch(self, peer_id: ID) -> bool: | |
connect_msg.ObsAddrs.extend(our_addrs) | ||
|
||
start_time = time.time() | ||
with trio.fail_after(STREAM_WRITE_TIMEOUT): | ||
with trio.fail_after(self.write_timeout): | ||
await stream.write(connect_msg.SerializeToString()) | ||
|
||
logger.debug( | ||
|
@@ -310,7 +326,7 @@ async def initiate_hole_punch(self, peer_id: ID) -> bool: | |
) | ||
|
||
# Receive the peer's CONNECT message | ||
with trio.fail_after(STREAM_READ_TIMEOUT): | ||
with trio.fail_after(self.read_timeout): | ||
resp_bytes = await stream.read(MAX_MESSAGE_SIZE) | ||
|
||
# Calculate RTT | ||
|
@@ -349,7 +365,7 @@ async def initiate_hole_punch(self, peer_id: ID) -> bool: | |
sync_msg = HolePunch() | ||
sync_msg.type = HolePunch.SYNC | ||
|
||
with trio.fail_after(STREAM_WRITE_TIMEOUT): | ||
with trio.fail_after(self.write_timeout): | ||
await stream.write(sync_msg.SerializeToString()) | ||
|
||
logger.debug("Sent SYNC message to %s", peer_id) | ||
|
@@ -468,7 +484,7 @@ async def _dial_peer(self, peer_id: ID, addr: Multiaddr) -> None: | |
peer_info = PeerInfo(peer_id, [addr]) | ||
|
||
# Try to connect with timeout | ||
with trio.fail_after(DIAL_TIMEOUT): | ||
with trio.fail_after(self.dial_timeout): | ||
await self.host.connect(peer_info) | ||
|
||
logger.info("Successfully connected to %s at %s", peer_id, addr) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @pacrob , thanks for the review, I realised this line was removed accidently and I have added it back.