Conversation
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com> # Conflicts: # src/common/abi/INodesManager.json # src/config/networks.py
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR expands the node manager “operator service” from eligibility polling into a full validator registration flow via oracles + NodesManager contract, and exposes deposit/funding amount helpers as public APIs.
Changes:
- Promotes validator amount calculation helpers from private to public (
get_deposits_amounts,get_funding_amounts) and updates callers/tests. - Implements NodesManager registration flow: build oracle approval requests, aggregate oracle signatures, and submit
registerValidatorstx. - Adds a new
node_manager_startCLI with keystore/wallet configuration and introduces node_manager unit tests.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/validators/tasks.py | Renames internal helpers to public functions and updates internal usage. |
| src/validators/tests/test_tasks.py | Updates tests to call the new public helper names. |
| src/node_manager/typings.py | Adds request/approval dataclasses for oracle registration flow. |
| src/node_manager/oracles.py | Implements polling/aggregation of oracle approvals for registration. |
| src/node_manager/register_validators.py | Adds tx submission logic for NodesManager registerValidators. |
| src/node_manager/tasks.py | Extends task to register validators after eligibility + gas checks. |
| src/node_manager/startup_check.py | Renames parameter to operator_address for clarity. |
| src/commands/node_manager_start.py | Adds a CLI entrypoint and config wiring for the node manager service. |
| src/node_manager/tests/test_tasks.py | Adds unit tests for NodeManagerTask registration flow (with mocks). |
| src/node_manager/tests/test_register_validators.py | Adds unit tests for tx submission behavior and failure paths. |
| src/node_manager/tests/test_oracles.py | Adds unit tests for parsing and approval aggregation logic. |
| src/node_manager/tests/test_oracles_http.py | Adds HTTP tests for polling eligible operators and collecting approvals. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| approvals_min_interval = 1 | ||
| rate_limiter = RateLimiter(approvals_min_interval) | ||
|
|
||
| while True: | ||
| await rate_limiter.ensure_interval() | ||
|
|
There was a problem hiding this comment.
RateLimiter.ensure_interval() currently does asyncio.sleep(self.min_interval - elapsed) without clamping, which raises ValueError when the previous loop iteration took longer than min_interval (a common case here due to network I/O). Since this code relies on RateLimiter, await rate_limiter.ensure_interval() can crash poll_registration_approval. Consider avoiding RateLimiter here or adding a local clamp (sleep only when remaining > 0).
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
src/node_manager/oracles.py
Outdated
|
|
||
| def _parse_registration_response(data: dict) -> NodeManagerRegistrationApproval: | ||
| """Parse oracle response containing both keeper and NM signatures.""" | ||
| keeper_params = data['keeperParams'] |
There was a problem hiding this comment.
| keeper_params = data['keeperParams'] | |
| keeper_params = data['keeper_params'] |
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/node_manager/oracles.py:1
poll_registration_approvalloops indefinitely without any explicit cancellation/interrupt integration or adaptive backoff on repeated failures. This can block shutdowns (until task cancellation is handled elsewhere) and can continuously hammer oracles at a 1s interval during outages. Consider explicitly handlingasyncio.CancelledError/stop conditions and adding a backoff strategy or a max retry window.
import asyncio
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _sign_deadline(deadline: int) -> HexStr: | ||
| """EIP-191 personal_sign of the deadline timestamp.""" | ||
| message = encode_defunct(text=str(deadline)) | ||
| return HexStr(wallet.sign_message(message).signature.hex()) |
There was a problem hiding this comment.
The manager signature only covers deadline, which does not bind the signature to the specific approval request fields (operator, registry_root, validator_index, public_keys, amounts, etc.). This makes the signature replayable for arbitrary requests sharing the same deadline. Sign a deterministic payload that includes all security-relevant request fields (and ideally chain id / domain separation) so the oracles can verify the signature is for this exact request.
|
|
||
| while True: | ||
| await rate_limiter.ensure_interval() | ||
|
|
||
| current_registry_root = await validators_registry_contract.get_registry_root() | ||
| if current_registry_root != validators_registry_root: | ||
| validators_registry_root = current_registry_root | ||
| oracles_request = None | ||
|
|
||
| current_timestamp = get_current_timestamp() | ||
| if oracles_request is None or deadline is None or deadline <= current_timestamp: | ||
| deadline = current_timestamp + protocol_config.signature_validity_period | ||
|
|
||
| oracles_request = await create_approval_request( | ||
| protocol_config=protocol_config, | ||
| keystore=keystore, | ||
| validators=validators, | ||
| registry_root=current_registry_root, | ||
| deadline=deadline, | ||
| operator_address=operator_address, | ||
| ) | ||
|
|
||
| try: | ||
| raw_approvals = await send_registration_requests(protocol_config, oracles_request) | ||
| oracles_approval = process_registration_approvals( | ||
| raw_approvals, protocol_config.validators_threshold | ||
| ) | ||
| return oracles_request, oracles_approval | ||
| except NotEnoughOracleApprovalsError as e: | ||
| logger.error( | ||
| 'Not enough oracle approvals for community vault registration: %d.' | ||
| ' Threshold is %d.', | ||
| e.num_votes, | ||
| e.threshold, | ||
| ) | ||
| except InvalidOraclesRequestError: | ||
| logger.error('All oracles failed to respond for community vault registration') |
There was a problem hiding this comment.
poll_registration_approval loops indefinitely without any explicit cancellation/interrupt integration or adaptive backoff on repeated failures. This can block shutdowns (until task cancellation is handled elsewhere) and can continuously hammer oracles at a 1s interval during outages. Consider explicitly handling asyncio.CancelledError/stop conditions and adding a backoff strategy or a max retry window.
| while True: | |
| await rate_limiter.ensure_interval() | |
| current_registry_root = await validators_registry_contract.get_registry_root() | |
| if current_registry_root != validators_registry_root: | |
| validators_registry_root = current_registry_root | |
| oracles_request = None | |
| current_timestamp = get_current_timestamp() | |
| if oracles_request is None or deadline is None or deadline <= current_timestamp: | |
| deadline = current_timestamp + protocol_config.signature_validity_period | |
| oracles_request = await create_approval_request( | |
| protocol_config=protocol_config, | |
| keystore=keystore, | |
| validators=validators, | |
| registry_root=current_registry_root, | |
| deadline=deadline, | |
| operator_address=operator_address, | |
| ) | |
| try: | |
| raw_approvals = await send_registration_requests(protocol_config, oracles_request) | |
| oracles_approval = process_registration_approvals( | |
| raw_approvals, protocol_config.validators_threshold | |
| ) | |
| return oracles_request, oracles_approval | |
| except NotEnoughOracleApprovalsError as e: | |
| logger.error( | |
| 'Not enough oracle approvals for community vault registration: %d.' | |
| ' Threshold is %d.', | |
| e.num_votes, | |
| e.threshold, | |
| ) | |
| except InvalidOraclesRequestError: | |
| logger.error('All oracles failed to respond for community vault registration') | |
| failure_count = 0 | |
| try: | |
| while True: | |
| await rate_limiter.ensure_interval() | |
| current_registry_root = await validators_registry_contract.get_registry_root() | |
| if current_registry_root != validators_registry_root: | |
| validators_registry_root = current_registry_root | |
| oracles_request = None | |
| current_timestamp = get_current_timestamp() | |
| if oracles_request is None or deadline is None or deadline <= current_timestamp: | |
| deadline = current_timestamp + protocol_config.signature_validity_period | |
| oracles_request = await create_approval_request( | |
| protocol_config=protocol_config, | |
| keystore=keystore, | |
| validators=validators, | |
| registry_root=current_registry_root, | |
| deadline=deadline, | |
| operator_address=operator_address, | |
| ) | |
| try: | |
| raw_approvals = await send_registration_requests(protocol_config, oracles_request) | |
| oracles_approval = process_registration_approvals( | |
| raw_approvals, protocol_config.validators_threshold | |
| ) | |
| # Reset failure counter on success | |
| failure_count = 0 | |
| return oracles_request, oracles_approval | |
| except NotEnoughOracleApprovalsError as e: | |
| failure_count += 1 | |
| logger.error( | |
| 'Not enough oracle approvals for community vault registration: %d.' | |
| ' Threshold is %d.', | |
| e.num_votes, | |
| e.threshold, | |
| ) | |
| except InvalidOraclesRequestError: | |
| failure_count += 1 | |
| logger.error('All oracles failed to respond for community vault registration') | |
| # Apply exponential backoff on repeated failures, capped to 60 seconds | |
| backoff_delay = min(approvals_min_interval * (2 ** min(failure_count, 5)), 60) | |
| logger.debug( | |
| 'Backing off for %s seconds before retrying registration approval polling', | |
| backoff_delay, | |
| ) | |
| await asyncio.sleep(backoff_delay) | |
| except asyncio.CancelledError: | |
| logger.info('Registration approval polling cancelled') | |
| raise |
| envvar='MAX_VALIDATOR_BALANCE_GWEI', | ||
| help=f'The maximum validator balance in Gwei. ' | ||
| f'Default is {NETWORKS[MAINNET].MAX_VALIDATOR_BALANCE_GWEI} Gwei', | ||
| callback=validate_max_validator_balance_gwei, |
There was a problem hiding this comment.
The CLI help states a default value, but the option does not set a Click default=..., so max_validator_balance_gwei will be None when omitted. If settings.configure(...) applies None literally, this can override the intended network default and break V2 deposit splitting. Set an explicit Click default (preferably derived from the selected network), or avoid passing the field to settings.configure when the option is unset.
| envvar='MAX_VALIDATOR_BALANCE_GWEI', | |
| help=f'The maximum validator balance in Gwei. ' | |
| f'Default is {NETWORKS[MAINNET].MAX_VALIDATOR_BALANCE_GWEI} Gwei', | |
| callback=validate_max_validator_balance_gwei, | |
| envvar='MAX_VALIDATOR_BALANCE_GWEI', | |
| default=NETWORKS[MAINNET].MAX_VALIDATOR_BALANCE_GWEI, | |
| help=f'The maximum validator balance in Gwei. ' | |
| f'Default is {NETWORKS[MAINNET].MAX_VALIDATOR_BALANCE_GWEI} Gwei', | |
| callback=validate_max_validator_balance_gwei, | |
| show_default=True, |
| max_validator_balance_gwei=( | ||
| Gwei(max_validator_balance_gwei) if max_validator_balance_gwei else None | ||
| ), |
There was a problem hiding this comment.
The CLI help states a default value, but the option does not set a Click default=..., so max_validator_balance_gwei will be None when omitted. If settings.configure(...) applies None literally, this can override the intended network default and break V2 deposit splitting. Set an explicit Click default (preferably derived from the selected network), or avoid passing the field to settings.configure when the option is unset.
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
src/node_manager/oracles.py
Outdated
|
|
||
| def _parse_registration_response(data: dict) -> NodeManagerRegistrationApproval: | ||
| """Parse oracle response containing both keeper and NM signatures.""" | ||
| keeper_params = data['keeperParams'] |
Signed-off-by: cyc60 <avsysoev60@gmail.com>
|
|
||
|
|
||
| def _parse_registration_response(data: dict) -> NodeManagerRegistrationApproval: | ||
| """Parse oracle response containing both keeper and NM signatures.""" |
There was a problem hiding this comment.
Replace NM to Nodes Manager in docstrings
No description provided.