Skip to content

Conversation

@ndrpp
Copy link
Member

@ndrpp ndrpp commented Dec 19, 2025

Fixes # .

Changes proposed in this PR:

  • pass address body param in http handlers for erc1271 admin validation
  • return an error when pushing config to a node that has running jobs

@ndrpp ndrpp linked an issue Dec 19, 2025 that may be closed by this pull request
Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI automated code review (Gemini 3).

Overall risk: low

Summary:
This pull request introduces the ability to dynamically update the OceanNode's configuration via an admin P2P command. It adds a setConfig method to the OceanNode class and integrates this into the PushConfigHandler. A crucial safety measure has been added to prevent configuration updates if there are active C2D jobs running. Additionally, the address parameter is now passed from HTTP handlers to the P2P admin command handlers for enhanced validation, and the private key is properly hidden in the configuration response. The C2D engines are also re-added after a configuration update to ensure the new settings take effect.

Comments:
• [INFO][other] The setConfig method re-initializes the escrow instance. This is generally safe if the escrow object is designed to be fully recreated from the configuration. Could you confirm that re-initializing it this way, rather than updating its internal properties, is the intended and correct behavior, and doesn't lead to resource leaks or unexpected state changes if it held complex state or external connections?
• [INFO][other] The check for running jobs before updating the configuration is an excellent safety addition. Consider adding a more detailed log message when a configuration update is rejected due to running jobs, indicating which jobs are active (e.g., their IDs or status) to aid in debugging.
• [INFO][other] Calling this.getOceanNode().addC2DEngines() after a config update is critical for the changes to take effect. It's assumed this method handles the lifecycle gracefully, e.g., stopping existing engines if their configuration has changed or is no longer valid, and starting new ones. A comment here clarifying this behavior or confirming that addC2DEngines is idempotent and handles re-initialization properly would be beneficial.

Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI automated code review (Gemini 3).

Overall risk: medium

Summary:
This pull request introduces the capability to dynamically update the OceanNode's configuration via admin endpoints, with a critical safeguard to prevent updates while C2D jobs are running. It also includes necessary changes to pass the address for signature validation in admin HTTP routes and updates various dependencies. The core logic for configuration hot-reloading and re-initializing components like Escrow and C2D engines is implemented.

Comments:
• [INFO][other] Multiple dependency updates are included, primarily minor/patch versions for body-parser, qs, raw-body, and a large number of @esbuild optional development dependencies. These seem low risk as they are mostly security/bug fixes or build-related tools. The addition of buildcheck and cpu-features as optional dependencies also seems to be related to build process improvements.
• [INFO][performance] The new setConfig method is crucial for dynamically updating the node's configuration. It correctly re-initializes the Escrow instance, which is important for reflecting changes to supportedNetworks or claimDurationTimeout. Ensure that all other components relying on the OceanNodeConfig are either re-initialized or aware of the configuration changes upon setConfig being called. The addC2DEngines call in PushConfigHandler seems to address this for C2D, which is good.
• [INFO][security] The check hasRunningJobs to reject config updates when jobs are active is an excellent safety measure. This prevents potential data corruption or unexpected behavior by ensuring the node is in a stable state before applying significant configuration changes.
• [INFO][performance] After updating the configuration via setConfig, the call to this.getOceanNode().addC2DEngines() is critical to ensure that C2D engines are re-initialized with the new configuration. This pattern helps ensure the node properly adapts to the new settings at runtime.
• [INFO][security] Using structuredClone for newConfig and then hiding the private key (responseConfig.keys.privateKey = '[*** HIDDEN CONTENT ***]') is a good security practice to avoid exposing sensitive information in API responses.
• [INFO][security] Passing the address parameter from the HTTP request body to the FetchConfigHandler and PushConfigHandler is a necessary improvement for enabling robust signature validation, especially for smart accounts. This ensures that the caller's identity is correctly verified before processing admin commands.
• [INFO][test] The new test case should reject push config when node has running jobs effectively covers the new safety mechanism. To make this test more robust and deterministic, consider directly mocking getRunningJobs() to return a non-empty array, rather than relying on the current state of the database during test execution, which might vary.
• [INFO][test] Adding await oceanNode.addC2DEngines() before the successful push config test is a good practice. It ensures the environment is properly set up, potentially clearing any residual running jobs and re-initializing engines, making the test more reliable and reflective of real-world scenarios where engines might need to be refreshed after config changes.

Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI automated code review (Gemini 3).

Overall risk: medium

Summary:
This pull request enables dynamic updates to the OceanNode's configuration via admin P2P commands. It introduces a setConfig method to apply changes at runtime and implements a crucial safety check to prevent configuration updates when active C2D jobs are running. Additionally, admin HTTP routes now pass the address parameter for enhanced validation. The PR also includes various dependency updates, primarily for @esbuild platform-specific packages and minor version bumps for core libraries like body-parser, qs, and raw-body.

Comments:
• [INFO][other] There are significant changes in package-lock.json, including the addition of many @esbuild optional dependencies for various platforms, and minor version updates for body-parser, qs, and raw-body. Also, peer: true flags have been added to several existing dependencies. While dependency updates are normal, please ensure that these updates, especially the @esbuild additions, are intentional and have been tested for any unexpected side effects or breaking changes, and that the new peer dependency requirements are met.
• [WARNING][style] The if (this.config) check on this line is redundant, as this.config would have just been assigned and will always be defined. It can be safely removed for cleaner code.

More critically, when setConfig is called, it currently only re-initializes this.escrow. Are there other components within the OceanNode (or any external services managed by the node) that rely on the configuration and would need to be re-initialized, restarted, or updated to fully reflect the new configuration? For example, if supportedNetworks or other network-specific settings change, simply re-initializing escrow might not be enough to ensure consistency across all node operations.
• [WARNING][other] The call to await this.getOceanNode().addC2DEngines() is made after setting the new configuration. The method name addC2DEngines suggests adding new engines. If the intent here is to re-initialize or restart existing C2D engines based on the new configuration, or to clean up existing engines before adding new ones, the method name might be slightly misleading. Could you clarify what addC2DEngines does in this context, especially concerning the lifecycle of any previously running C2D engines? Does it correctly handle stopping old engines and starting new ones with the updated configuration to prevent resource leaks or conflicts?
• [WARNING][bug] In the before hook, the await keyword was removed from oceanNode = OceanNode.getInstance(config, database). This implies that OceanNode.getInstance is now synchronous. Could you confirm if this is an intentional refactoring and if getInstance no longer returns a Promise? If getInstance is still an asynchronous function, the await keyword should be restored here to ensure proper initialization.
• [INFO][test] In the should push config changes and reload node test, await oceanNode.addC2DEngines() is called with the comment // make sure there are no running jobs. This suggests that addC2DEngines has a side effect of clearing or resetting the job queue. If this is its intended behavior in this context, it would be beneficial to either reflect this in the method's name or add a more explicit comment in the addC2DEngines definition itself, explaining this behavior, for better clarity and maintainability.

Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI automated code review (Gemini 3).

Overall risk: medium

Summary:
This pull request introduces significant improvements to the P2P configuration management, particularly for the pushConfig functionality. Key changes include a crucial safety check to prevent configuration updates when C2D jobs are actively running, ensuring node stability. The changes also ensure that new configurations are correctly applied to the running OceanNode instance and that sensitive information like private keys are handled securely in responses. Dependency updates in package-lock.json reflect minor version bumps and the addition of platform-specific esbuild dependencies.

Comments:
• [INFO][other] The package-lock.json has extensive changes, primarily due to dependency updates (e.g., body-parser, qs) and the addition of many @esbuild platform-specific optional packages. Please ensure all dependency updates are intentional and compatible. These changes are typically low risk as long as the underlying library updates are minor/patch versions.
• [INFO][other] The addition of setConfig provides a clear and centralized way to update the node's active configuration. This is a good pattern for managing runtime configuration changes.
• [INFO][security] The hasRunningJobs check is an excellent addition for preventing potentially disruptive configuration updates while compute-to-data jobs are active. This significantly improves the robustness and reliability of the node by preventing mid-job config changes that could lead to unexpected behavior or data corruption.
• [INFO][other] Calling this.getOceanNode().setConfig(newConfig) and await this.getOceanNode().addC2DEngines() ensures that the running node instance and its C2D engines are properly updated with the new configuration, which is critical for the changes to take effect immediately after a successful push.
• [INFO][security] Using structuredClone for responseConfig before hiding the private key is a good practice to ensure that the actual node configuration (which might contain the private key internally) is not directly mutated for the response. This helps maintain a secure separation.
• [INFO][security] Passing the address parameter from the HTTP request to the P2P handlers for both FETCH_CONFIG and PUSH_CONFIG is a good enhancement for administrative command validation, allowing the handler to verify the identity of the requester against a known admin address.
• [INFO][style] The change from await OceanNode.getInstance() to OceanNode.getInstance() suggests getInstance is now synchronous. Please confirm this change is intentional and doesn't hide any asynchronous initialization that should still be awaited. If getInstance's signature changed, it might be worth mentioning in the PR description.
• [INFO][test] Adding a specific test case to reject pushConfig when running jobs exist is excellent. It thoroughly validates the new safety mechanism and increases confidence in the stability of config updates.
• [INFO][test] The addition of logic to stop running jobs before attempting to push a new config in the existing test is a necessary adjustment to accommodate the new validation logic in PushConfigHandler. This ensures the test remains valid and passes consistently under the new, safer conditions.

Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI automated code review (Gemini 3).

Overall risk: low

Summary:
This pull request introduces a critical safety mechanism for updating OceanNode configurations, preventing changes when C2D jobs are actively running. It also ensures that the updated configuration is correctly applied to the running node instance and its C2D engines. Minor dependency updates and a new integration test for the safety feature are also included.

Comments:
• [INFO][other] The setConfig method provides a clear and controlled way to update the node's configuration at runtime. Re-initializing the escrow instance is appropriate here to reflect any changes in supportedNetworks or claimDurationTimeout.
• [INFO][security] The addition of a check for running C2D jobs (hasRunningJobs) before allowing a configuration update is an excellent safety measure. This prevents potential disruption or data corruption of ongoing computations due to configuration changes, significantly improving the robustness of the node's admin functionality.
• [INFO][other] Calling this.getOceanNode().setConfig(newConfig) and await this.getOceanNode().addC2DEngines() after a successful config push is crucial. This ensures that the newly loaded configuration is actually applied to the live OceanNode instance and that C2D engines are properly re-initialized or updated with the new settings.
• [INFO][security] Using structuredClone(newConfig) before setting privateKey to a hidden string ('[*** HIDDEN CONTENT ***]') for the response is a good practice. This ensures that the actual newConfig object used internally by the node retains the sensitive private key, while the response sent over the network is sanitized.
• [INFO][security] Passing the address parameter to the FetchConfigHandler and PushConfigHandler aligns with the commit history's mention of ERC1271 validation for smart accounts. This centralizes authentication/authorization logic within the handlers, which is a good design pattern for security-sensitive operations.
• [INFO][other] The change from await OceanNode.getInstance to OceanNode.getInstance indicates that the instance retrieval is now synchronous, which is consistent with the OceanNode.ts changes. This improves clarity if instance creation no longer involves asynchronous operations.
• [INFO][other] The addition of a new test case (should reject push config when node has running jobs) directly validates the new safety mechanism. This is excellent and confirms the intended behavior of preventing config updates during active C2D operations.
• [INFO][other] Multiple dependency updates, including body-parser, qs, and raw-body, are included. These are minor version bumps, which typically involve bug fixes or minor enhancements and are generally low risk. The addition of various @esbuild/* optional dependencies is common for build tools to support different platforms.

@ndrpp ndrpp marked this pull request as ready for review January 7, 2026 13:08

try {
const hasRunningJobs =
(await this.getOceanNode().getDatabase().c2d.getRunningJobs()).length > 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have this condition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have to restart the engine to apply the new configuration and I thought this would interfere with existing running jobs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested locally and seems I was solving an issue that did not exist, you were right 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node Get/Push config fixes

4 participants