-
Notifications
You must be signed in to change notification settings - Fork 19
LibP2P V3 #1125
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?
LibP2P V3 #1125
Conversation
# Conflicts: # package-lock.json
alexcos20
left a comment
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.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This pull request updates the libp2p dependency from version 1 to version 3, along with several related @libp2p and @chainsafe/libp2p packages. This is a significant upgrade that required adjusting the codebase to the new libp2p API, particularly concerning private key handling, peer ID generation, and node configuration. Several previously asynchronous functions related to configuration and key generation have been refactored to be synchronous due to changes in the underlying libp2p API. The package-lock.json has extensive changes reflecting these dependency updates. Overall, the changes seem to correctly adapt to the new libp2p paradigm, but warrant thorough testing.
Comments:
• [INFO][other] Major dependency upgrade to libp2p v3 and related packages. This is a significant change and should be thoroughly tested to ensure no regressions or unexpected behavior.
• [INFO][style] The change from config.keys.privateKey to config.keys.privateKey.raw is consistently applied across the codebase. This indicates an API change in how the raw private key is accessed from the OceanNodeKeys object after the libp2p upgrade. This seems correct given the new key structure.
• [WARNING][other] The libp2p constructor now expects privateKey instead of peerId. This is a notable API change. Could you confirm that the privateKey object passed here is the correct type and structure expected by libp2p v3 for initialization? It was previously passed peerId: config.keys.peerId, which would include the private key internally, but now it's directly privateKey: config.keys.privateKey.
• [WARNING][bug] The circuitRelayTransport configuration was changed from circuitRelayTransport({ discoverRelays: config.p2pConfig.circuitRelays }) to just circuitRelayTransport(). Does libp2p v3 handle relay discovery differently, or is there a new way to pass this configuration? If not explicitly configured, it might impact connectivity and NAT traversal for nodes behind restrictive firewalls.
• [INFO][other] The getPeerIdFromPrivateKey function has been changed from async to synchronous, and now uses privateKeyFromRaw and peerIdFromPrivateKey from the new libp2p API. This is a good simplification if the operations are indeed synchronous now. Please ensure no blocking I/O or heavy computation is inadvertently introduced in a synchronous context.
• [INFO][other] Similarly, buildMergedConfig and getConfiguration have been updated to be synchronous. This is a logical consequence of getPeerIdFromPrivateKey no longer being async. Ensure that any operations within these functions that could be asynchronous (e.g., file system access, network calls) are properly handled or are confirmed to be synchronous.
• [INFO][style] The use of privateKey.raw in crypt.ts is consistent with the changes made in src/components/P2P/index.ts and src/utils/config/builder.ts. This confirms the new structure of the private key object obtained from the updated libp2p/crypto library.
alexcos20
left a comment
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.
AI automated code review (Gemini 3).
Overall risk: high
Summary:
This pull request performs a major upgrade of the libp2p library from v1 to v3, along with its associated sub-modules and @chainsafe dependencies. This upgrade necessitated significant changes to the P2P component's initialization, peer discovery, connection management, and cryptographic key handling due to breaking API changes in libp2p v3. Custom dial logic has been introduced to replace the deprecated autodialer functionality. Additionally, changes were made to how private and public keys are accessed (now typically via a .raw property on key objects) across several core utility files and the P2P component itself.
Comments:
• [INFO][other] This PR updates libp2p and its related dependencies to major new versions (v3). This is a significant change. It would be beneficial to include a high-level overview in the PR description of the motivation for this upgrade (e.g., new features, performance improvements, security fixes, or deprecation of old APIs) and any known implications.
• [INFO][other] The new custom dial logic on peer:discovery is a critical change. It's important to ensure this logic correctly manages connections based on minConnections and maxConnections and handles various network scenarios (e.g., peers becoming unreachable, network partitions, etc.) as reliably as (or better than) the old libp2p autodialer. Are there specific tests added or updated to cover the behavior of this custom dialer?
• [WARNING][bug] The change from config.keys.privateKey to config.keys.privateKey.raw is consistently applied across several files. This indicates a breaking change in the underlying crypto library's key object structure. This is fine as long as config.keys.privateKey is indeed an object now containing the raw bytes in a .raw property. Confirm this assumption.
• [INFO][style] Renaming connectionEncryption to connectionEncrypters reflects the new libp2p v3 API. This is a good adaptation to the library's changes.
• [INFO][other] Several connectionManager options were removed. The comment mentions relay discovery is now automatic through the network's RandomWalk component. Can you confirm if all removed options (e.g., autoDialPeerRetryThreshold, autoDialConcurrency, autoDialInterval) are either handled internally by libp2p v3's new architecture or are no longer relevant for the desired behavior?
• [INFO][other] The libp2p constructor now expects privateKey: config.keys.privateKey instead of peerId: config.keys.peerId. This implies libp2p v3 derives the PeerId internally from the private key. This is a significant change in how the libp2p node is initialized and should be verified against the new libp2p documentation.
• [INFO][other] The option runOnTransientConnection has been replaced with runOnLimitedConnection in the _libp2p.dial options. This seems like a direct rename in the libp2p v3 API. Confirm this maps to the same intended behavior or if there are subtle differences in 'limited' versus 'transient' connections.
• [INFO][other] The getPeerIdFromPrivateKey function has been converted from async to synchronous, and its internal logic updated to use privateKeyFromRaw and peerIdFromPrivateKey. This simplifies the configuration loading process. Ensure there are no performance implications for synchronous key generation, although for a single key, this is unlikely.
alexcos20
left a comment
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.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This pull request upgrades the core Libp2p library from version 1 to version 3, along with its associated packages. This is a significant update, introducing several breaking API changes that have been addressed across the P2P component, key management, and cryptographic utilities. Key changes include adapting to the new Libp2p instantiation process (using privateKey directly), implementing custom peer dialing logic (as the auto-dialer was removed in v3), updating key access patterns to use .raw property, and adjusting connection manager configurations. The PR also includes changes to make buildMergedConfig and getConfiguration synchronous, reflecting the synchronous nature of the new key generation functions.
Comments:
• [INFO][other] Major version bumps for Libp2p and its related packages (@chainsafe/libp2p-noise, @libp2p/*) indicate significant breaking changes. It's good to see these updates, but it implies a thorough review of all dependent logic is necessary, which appears to have been done in the other files.
• [INFO][performance] The introduction of custom dial logic (if (currentConnections < minConnections || currentConnections < maxConnections)) to replace the removed auto-dialer is a crucial change. This logic needs to be robustly tested to ensure reliable peer discovery and connection management, especially under various network conditions (e.g., high churn, many peers). It would be beneficial to have clear documentation or comments around the expected behavior and edge cases of this custom logic.
Consider adding a P2P_LOGGER.debug message when a peer is not dialed, to help debug potential connectivity issues.
• [INFO][other] The change from config.keys.privateKey to config.keys.privateKey.raw reflects an API change in the libp2p/crypto library where the raw bytes of the private key are now accessed via the .raw property. This is correctly applied here and in other files (downloadHandler.ts, feesHandler.ts, crypt.ts). This consistency is good.
• [INFO][other] Instantiating libp2p with privateKey: config.keys.privateKey instead of peerId: config.keys.peerId is a significant change in Libp2p v3. This implies that Libp2p now derives the PeerId directly from the provided PrivateKey during initialization. This is a more secure and robust way to handle identity but requires verification that the PeerId derived is consistent with expectations and that all components relying on peerId still function correctly.
• [WARNING][performance] Several connectionManager options (minConnections, autoDialPeerRetryThreshold, autoDialConcurrency, autoDialInterval) have been removed. The commit message indicates the autodialer was removed. Can we confirm if the new default behavior for these aspects is suitable for Ocean Node's requirements, or if there are equivalent new configurations for fine-tuning connection management in LibP2P v3?
• [INFO][style] Changing runOnTransientConnection to runOnLimitedConnection is a direct API name update. This seems to be a minor renaming, but it's worth double-checking the documentation for libp2p v3 to ensure there are no subtle semantic differences in this flag's behavior.
• [INFO][other] The imports for key and peer ID generation have been updated to privateKeyFromRaw and peerIdFromPrivateKey, which aligns with the new libp2p/crypto and @libp2p/peer-id API. This is a correct adaptation to the v3 changes.
• [INFO][style] The assignment privateKey: key instead of (key as any)._key improves type safety and readability, as key is now the PrivateKey instance itself (which is what libp2p expects). This is a good cleanup.
• [INFO][other] The buildMergedConfig and getConfiguration functions are now synchronous. This simplifies the code path since getPeerIdFromPrivateKey no longer requires awaiting. Ensure that any callers of these functions are aware of this change if they were previously handling Promises.
alexcos20
left a comment
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.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This pull request upgrades the libp2p dependency to version 3.x across the Ocean Node project. This is a significant update, requiring extensive changes to the P2P component initialization, configuration, and key handling to align with the new libp2p API. Key adaptations include implementing custom peer dialing logic (as the autodialer was removed), updating private/public key object access (.raw property), and adjusting libp2p constructor options such as transports and connection manager settings. The getPeerIdFromPrivateKey function has also been refactored to be synchronous and use the updated crypto APIs.
Comments:
• [INFO][other] Major version bumps for libp2p and its related modules (@chainsafe/libp2p-noise, @libp2p/autonat, etc.) indicate potentially breaking changes. While the code adaptations address known API changes, it's crucial to ensure comprehensive testing of the P2P network's stability, connectivity, and performance under various conditions, especially in a production environment. The extensive changes in package-lock.json reinforce the breadth of this dependency update.
• [WARNING][bug] The custom dial logic introduced replaces the removed libp2p autodialer. While this attempts to maintain connectivity, the current implementation is quite basic. Consider adding:
- Backoff/Retry Strategy: For failed dials, a more sophisticated exponential backoff or retry mechanism could prevent constant retries on unreachable peers.
- Peer Scoring/Prioritization: For larger networks, you might want to prioritize dialing certain types of peers or those with better observed latency/reliability.
- Connection Throttling: While
maxParallelDialsexists, ensure this custom logic doesn't inadvertently overload the dialing mechanism if many peers are discovered simultaneously.
Thorough testing for network resilience and connection stability is highly recommended for this new logic.
• [INFO][other] The change from config.keys.privateKey to config.keys.privateKey.raw is observed in several places (src/components/P2P/index.ts, src/components/core/handler/downloadHandler.ts, src/components/core/utils/feesHandler.ts). This is a critical change reflecting the new structure of libp2p/crypto's PrivateKey object. Confirming that the .raw property consistently provides the expected byte array for cryptographic operations (e.g., ethCrypto.decryptWithPrivateKey) is essential to prevent subtle crypto bugs or security issues.
• [INFO][other] The circuitRelayTransport no longer accepting discoverRelays as an option and relying on RandomWalk for discovery is a significant internal change. It would be beneficial to explicitly confirm in the libp2p v3 migration guides or documentation that RandomWalk (or its equivalent) adequately covers the previous relay discovery functionality to ensure network reachability is not negatively impacted.
• [INFO][other] The option runOnTransientConnection has been renamed to runOnLimitedConnection. Please confirm that the semantic meaning and behavior of this option remain identical in libp2p v3 to avoid any unintended side effects on how dials are handled for transient or limited connections.
• [INFO][refactor] The getPeerIdFromPrivateKey function has been converted from async to synchronous, and its internal logic updated to use privateKeyFromRaw and peerIdFromPrivateKey from the new libp2p crypto modules. This is a good adaptation to the new API. Ensure that the privateKey and publicKey fields of the OceanNodeKeys object are correctly populated and their types are consistent with their usage across the application (e.g., in Buffer.from(...).toString('hex') conversions).
alexcos20
left a comment
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.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This pull request performs a major upgrade of the libp2p library and its related dependencies to version 3. This involves significant API changes across the P2P component and key management utilities. Custom peer dialing logic has been introduced to replace deprecated automatic dialing functionality. Key handling for private and public keys has been updated to reflect the new libp2p/crypto API, requiring access to .raw properties for raw key material.
Comments:
• [INFO][other] The addition of custom dial logic (lines 162-176) is a necessary adaptation due to the removal of the autodialer in libp2p v3. While this is a good immediate solution, consider adding more robust error handling or logging for dial failures, especially if connection stability becomes an issue in production. Also, ensure the minConnections and maxConnections logic aligns with the desired network topology and resilience goals.
• [INFO][style] The change from config.keys.privateKey to config.keys.privateKey.raw (and similar for publicKey) reflects the new API for libp2p/crypto keys. This is correctly applied here and in other files (downloadHandler.ts, feesHandler.ts, builder.ts). It would be beneficial to ensure internal documentation or typings (if applicable) clearly reflect that config.keys.privateKey now stores a Libp2pPrivateKey object and not just the raw bytes.
• [INFO][performance] Several connectionManager options like autoDialPeerRetryThreshold, autoDialConcurrency, and autoDialInterval have been removed with the libp2p v3 upgrade. Please verify that the default behaviors in libp2p v3 are suitable for our needs, or if custom logic needs to be introduced to replicate any critical functionality previously provided by these options, beyond the basic peer:discovery dialing.
• [INFO][style] The option runOnTransientConnection has been renamed to runOnLimitedConnection. This is a straightforward API change, but it's good to double-check the libp2p documentation to confirm there are no semantic differences or new considerations with the runOnLimitedConnection flag.
• [INFO][style] The getPeerIdFromPrivateKey function has been converted from async to synchronous, which is a good simplification given the new libp2p/crypto APIs. This change is also correctly reflected in buildMergedConfig and getConfiguration. This consistency is good.
• [WARNING][other] Upgrading libp2p and its associated modules to new major versions (e.g., libp2p from ^1.8.0 to ^3.1.2) indicates significant breaking changes. While the code has been updated to reflect known API changes, there might be subtle behavioral changes or regressions not immediately apparent. Extensive integration and system tests for the P2P networking layer are crucial to ensure stability and functionality post-upgrade.
alexcos20
left a comment
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.
AI automated code review (Gemini 3).
Overall risk: high
Summary:
This pull request performs a major upgrade of the libp2p library and its associated dependencies to their latest major versions (v3 and above). This extensive update required significant refactoring across the P2P component, core handlers, and configuration builder to adapt to the new libp2p APIs. Key changes include updated package versions, revised peer ID and key handling, adjustments to libp2p configuration options (transports, connection management, encryption), and the implementation of custom peer dialing logic to replace the removed autodialer. The async nature of certain key generation and configuration functions has also been updated to synchronous where applicable in the new libp2p API.
Comments:
• [WARNING][other] Multiple libp2p and @chainsafe packages have been updated to major new versions. This indicates potential breaking changes in APIs, which are indeed reflected in the other file changes. Thorough testing, especially integration and system tests, is critical to ensure no regressions or unexpected behaviors are introduced. A detailed changelog review for each updated dependency would be beneficial to understand all potential impacts.
• [WARNING][performance] The new custom dial logic replaces the autodialer removed in libp2p v3. While necessary, it's crucial to ensure this custom logic is robust enough to handle network churn, varying peer availability, and efficient connection management. Special attention should be paid to the minConnections and maxConnections logic, and how quickly the node establishes connections to maintain network health, especially under load or initial startup. Performance implications of frequent dialing should be monitored.
• [INFO][style] Accessing config.keys.privateKey.raw is a breaking API change for how private keys are handled in the new @libp2p/crypto versions. This change is propagated consistently, which is good. Ensure that config.keys.privateKey always contains the expected PrivateKey object with a raw property.
• [INFO][other] The circuitRelayTransport no longer takes discoverRelays as an explicit option. The comment states "relay discovery is now automatic through the network's RandomWalk component." This implies that the RandomWalk service (or similar) must be correctly configured and enabled for circuit relay functionality to work as expected. Please confirm this dependency is explicitly set up and functioning.
• [WARNING][other] The connectionManager options minConnections, autoDialPeerRetryThreshold, autoDialConcurrency, and autoDialInterval have been removed. It is important to confirm how these behaviors are now managed by the new libp2p version. Are there new equivalent configurations, or does the default behavior of the new libp2p suffice? The custom dialing logic partially addresses minConnections, but other aspects might be implicitly handled or require a different approach.
• [INFO][style] Similar to private keys, public keys now require accessing the .raw property (peerId.publicKey.raw). This is a consistent API change across the updated libp2p libraries.
• [INFO][style] The option runOnTransientConnection has been renamed to runOnLimitedConnection. This is a minor API change but should be noted.
• [INFO][style] The getPeerIdFromPrivateKey function has been converted from async to synchronous. This is a significant change in how peer IDs are generated and should be verified for correctness, especially considering potential blocking operations if not truly synchronous. The new import from @libp2p/crypto/keys (privateKeyFromRaw) and @libp2p/peer-id (peerIdFromPrivateKey) suggests a direct API call that is synchronous.
• [WARNING][security] Cryptographic operations using privateKey.raw are critical. While consistent with the libp2p API changes, any modification to crypto-related code should be thoroughly reviewed by a security expert to ensure there are no subtle vulnerabilities or incorrect usages introduced by the new key object structure. Ensure that privateKey.raw consistently provides the expected raw byte array required by crypto.createCipheriv and eciesjs.PrivateKey.
alexcos20
left a comment
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.
AI automated code review (Gemini 3).
Overall risk: high
Summary:
This pull request performs a major upgrade of the libp2p library from version 1 to version 3, along with corresponding updates to related @libp2p/* packages. This involves significant refactoring of the P2P component, including a custom dialer implementation replacing the removed libp2p autodialer, updates to connection and transport configurations, and changes to stream handling for P2P communication. The PR also updates key and peer ID generation logic to align with the new libp2p/crypto API and adjusts the OceanNodeKeys structure. Several areas now use @ts-ignore due to type incompatibilities with the new libp2p versions.
Comments:
• [WARNING][other] Major version bumps for libp2p and its sub-packages (@chainsafe/libp2p-noise, @libp2p/bootstrap, etc.) indicate potentially breaking API changes. Ensure all integrations fully adhere to the new libp2p v3 API specifications. This is the root cause of many changes in the PR.
• [INFO][style] Changing _libp2p type from any to Libp2p is a good step for type safety. However, the numerous @ts-ignore comments later in the file suggest some remaining type compatibility issues that should ideally be resolved or properly documented.
• [WARNING][bug] The previous libp2p autodialer has been replaced with custom dial logic. This is a critical component for network stability and peer discovery. Thorough testing is required to ensure this custom implementation correctly handles all edge cases (e.g., transient connections, peer disconnections, retries, connection limits, and network partitioning) as effectively as, or better than, the original autodialer.
• [INFO][style] Accessing config.keys.privateKey.raw for _privateKey is consistent with the OceanNodeKeys structure change. Ensure this raw property access is uniformly applied wherever the private key is used to avoid unexpected behavior.
• [INFO][other] The removal of the discoverRelays option for circuitRelayTransport and the note about 'automatic relay discovery through RandomWalk' imply a change in relay handling. If users relied on explicit relay configuration, this might be a breaking change or require documentation updates on how to configure relays in the new setup.
• [WARNING][bug] The createLibp2p options now use privateKey directly instead of peerId. This is a fundamental change in node instantiation. Verify that this correctly configures the libp2p instance and that peer ID derivation from the private key works as expected.
• [WARNING][style] The use of @ts-ignore for libp2pInternal.components suggests type issues when accessing internal libp2p components (addressManager, transportManager). While it might work, this can hide potential runtime errors if the internal structure of libp2p changes in future patch versions. Consider adding more specific type declarations or opening an issue with the libp2p library for better type support.
• [WARNING][bug] The _getPeerInfo() function now explicitly returns { ...peer, publicKey: pubKey }. This custom publicKey property might conflict with or override libp2p's native Peer object structure and its publicKey property, potentially leading to inconsistencies or unexpected behavior in modules expecting the standard Peer object structure. Verify that this custom property doesn't break any libp2p internal operations or external integrations that rely on Peer objects.
• [WARNING][performance] Converting x.multiaddr and peerData.multiaddrs[index] to string and then back to multiaddr() via multiaddr(value.toString()) might introduce unnecessary overhead or potential parsing errors if the string representation is not always canonical. Ideally, if x.multiaddr is already a Multiaddr object, it should be used directly without conversion, or multiaddr.decode should be used if a Buffer is provided.
• [WARNING][bug] The dial method now takes a peerId instead of a list of multiaddrs. This is a significant change in the dialing API. Ensure this change is correctly handled in all calling contexts and that the peer discovery mechanism provides reliable peerIds for dialing. The custom dialer logic from lines 162-177 should also reflect this new API.
• [ERROR][bug] Stream handling has been completely refactored from it-pipe to a new EventTarget-like API (stream.send, stream.close()). This is a critical change. The @ts-ignore for response.stream due to 'libp2p v3 Stream type differs from Node.js Stream' highlights a potential for type mismatches and runtime issues. Extensive testing of all P2P stream-based communications (e.g., data transfer, request/response patterns) is crucial to ensure correctness and stability. The sink function also needs to be compatible with the new stream type.
• [WARNING][style] Similar to other instances, the @ts-ignore for contentRouting.provide and contentRouting.findProviders (line 834) indicates a type discrepancy. It's important to understand why these ignores are necessary and if a more robust typing approach can be adopted or if this points to potential API misuse under the hood.
• [WARNING][bug] The getPeerIdFromPrivateKey function has been changed from async to synchronous and now uses privateKeyFromRaw and peerIdFromPrivateKey from new libp2p/crypto imports. This is a breaking change for any consumer that previously awaited this function. Additionally, the OceanNodeKeys return structure has been updated (privateKey and publicKey properties now return different types/shapes). All consumers of getConfiguration().keys need to be reviewed to ensure they correctly handle the new PrivateKey object and publicKey.raw property.
alexcos20
left a comment
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.
AI automated code review (Gemini 3).
Overall risk: high
Summary:
This pull request undertakes a significant upgrade of the core libp2p networking library from version 1 to version 3. This involves extensive changes across the codebase, including updating package dependencies, refactoring P2P communication patterns (stream handling for sending and receiving data), adapting key generation and cryptographic operations to the new libp2p API, and modifying internal command routing and HTTP response mechanisms. A custom autodialer logic has been implemented to replace removed libp2p features, which is critical for network stability.
Comments:
• [WARNING][other] The PR involves major version bumps for libp2p and its related @libp2p/* dependencies. This is a substantial upgrade, indicating potential breaking API changes and behavioral differences. Thorough testing of all P2P communication paths and functionalities is crucial to ensure no regressions or unexpected behavior.
• [INFO][performance] The handleDirectProtocolCommand now collects the entire response stream into a Uint8Array before returning. For commands that might return very large payloads and are processed locally (not via P2P), this could increase memory consumption by buffering the entire data in memory. Consider if true streaming is necessary for local command handling, or if the current approach is acceptable given expected payload sizes.
• [INFO][bug] The DDO decryption process now expects response.data from p2pNode.sendTo to contain the full decrypted DDO. The previous implementation had explicit checks for isBinaryContent and different stream processing. While streamToUint8Array is generic, confirm that the reconstructed Uint8Array always represents a valid JSON string for DDOs, especially for various content types and sizes.
• [WARNING][architecture] The P2P communication protocol has been significantly altered: the first chunk sent over the stream is now strictly the JSON status, and subsequent chunks form the data payload. This custom wire protocol needs strong documentation and careful implementation across all interacting nodes to ensure interoperability and prevent issues like partial data reads or misinterpretation of chunks. Thorough integration testing is crucial for this new interaction pattern.
• [BUG][performance] A custom autodialer logic has been implemented in _onPeerDiscovery to manage connections based on minConnections and maxConnections, replacing features removed in libp2p v3. This logic is essential for network stability. Please ensure extensive testing covers scenarios like peer churn, network partitions, and connection limits. The current logic only dials if existingConnections.length === 0, which might be too conservative; it should ideally re-dial if the total number of connections is below minConnections regardless of specific peer connections, or if a previously connected peer becomes unreachable.
• [INFO][performance] Several connectionManager options (e.g., minConnections, autoDialPeerRetryThreshold, autoDialConcurrency, autoDialInterval) have been removed from the libp2p configuration. While some functionality might be replaced by custom logic, confirm that their removal does not negatively impact overall network behavior or lead to regressions, especially regarding connection resilience and peer discovery efficiency in various network topologies.
• [WARNING][style] The use of @ts-ignore for multiaddr conversion in getPeerMultiaddrs suggests a type incompatibility between the libp2p v3 types and the project's internal Multiaddr usage. It is preferable to resolve the underlying type issue by aligning the imported Multiaddr type or adjusting type definitions to remove the need for ts-ignore.
• [HIGH][bug] The sendTo method has undergone a complete refactor of its data exchange part. It now sends the message, closes the sender stream, and then reads the response stream sequentially (first chunk for status, subsequent for data). This synchronous-like stream interaction is a fundamental change from previous it-pipe usage with a sink. Robust error handling (e.g., remote peer closing prematurely, partial data reads) and performance validation for various payload sizes and network conditions are critical here.
• [WARNING][bug] The key generation (getPeerIdFromPrivateKey) and configuration building (buildMergedConfig, getConfiguration) have been updated to use synchronous libp2p v3 key APIs (privateKeyFromRaw, peerIdFromPrivateKey). This is a fundamental change in node identity management. Verify that existing private keys can still be correctly loaded and new keys are generated and used securely and consistently throughout the node's lifecycle. The change to access .raw on private and public keys must be consistent everywhere.
alexcos20
left a comment
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.
AI automated code review (Gemini 3).
Overall risk: high
Summary:
This pull request performs a significant upgrade of the core libp2p networking library to version 3, along with adapting all dependent components and communication patterns within the Ocean Node. The changes include updating package.json dependencies, refactoring P2P stream handling from it-pipe based sinks to a direct stream.send() and for await...of model, modifying libp2p node configuration, implementing custom peer dialing logic, and updating cryptographic key handling to align with the new library APIs. Multiple files across src/, src/components/, and src/utils/ have been updated to reflect these changes, including OceanNode.ts, BaseProcessor.ts, handleProtocolCommands.ts, index.ts, ddoHandler.ts, downloadHandler.ts, p2p.ts, feesHandler.ts, commands.ts, builder.ts, crypt.ts, and util.ts (with a new streamToUint8Array helper).
Comments:
• [INFO][other] The dependency bumps to @libp2p/* and @chainsafe/libp2p-* packages indicate a major upgrade to libp2p v3. This is the root cause of many downstream changes in the PR. This is a significant version jump, so thorough testing (unit, integration, and system tests) is crucial to ensure all networking functionalities remain stable and compatible.
• [WARNING][bug] In handleDirectProtocolCommand, if response.stream is null, the data variable will be undefined, which will trigger the if (!data) block and return a 500 error: No data received. This might be too aggressive if a command legitimately returns a 200 status with no payload (e.g., a simple acknowledgement). Consider returning { status: response.status, data: undefined } if response.stream is null but response.status.httpStatus is 200, to correctly reflect a successful but content-less response.
• [INFO][style] The ReadableString import from ./components/P2P/handleProtocolCommands.js is no longer used within src/OceanNode.ts after the refactoring of handleDirectProtocolCommand. It can be removed from this file.
• [INFO][other] Several connectionManager options (autoDialPeerRetryThreshold, autoDialConcurrency, autoDialInterval) have been removed from the libp2p configuration. This implies that libp2p v3 handles these aspects differently or they are no longer exposed as direct configuration. It's important to confirm that the default behavior or new mechanisms in libp2p v3 adequately replace the functionality of these removed options to avoid any regressions in peer discovery or connection stability.
• [WARNING][other] The use of @ts-ignore for this._libp2p.contentRouting.provide and findProviders indicates a potential type mismatch or missing type definitions for libp2p v3. It's advisable to investigate if updated type definitions are available or if a more type-safe approach can be used to avoid suppressing TypeScript checks.
• [INFO][other] The sendTo method's new implementation correctly reflects libp2p v3's stream communication model where status and data are sent as distinct chunks. The client calling sendTo must be aware of this protocol to correctly parse the first chunk as JSON status and subsequent chunks as raw data. This change fundamentally alters how remote peers will process responses.
• [INFO][other] The functions getPeerIdFromPrivateKey, buildMergedConfig, and getConfiguration have been changed from async to synchronous. This is a significant architectural simplification if key generation and config loading no longer involve asynchronous I/O. However, it implicitly assumes that all underlying operations are indeed synchronous. If any future dependencies or operations within these functions become asynchronous, they will need to be re-evaluated and potentially reverted to async.
• [INFO][other] The await keyword was removed from this.getOceanNode().getP2PNode().getNetworkingStats(). The getNetworkingStats() method in src/components/P2P/index.ts is no longer async and directly returns its properties. This change aligns with the new method signature and is correct.
• [INFO][other] The directCommandRoute has been substantially refactored to align with the new P2P stream handling. The custom writeResponsePayload helper and the use of streamToUint8Array are good additions for clarity and consistency. This improved handling of binary and string content based on content-type header is a robust change.
# Conflicts: # package-lock.json # package.json
Fixes #1104
Changes proposed in this PR: