Merged
Conversation
+ add debug print
…l communication + merge server connection manager and conection pool classes + merge inbound connection into peer connection class + handle inbound connection messages in peer connection + deprecate server connection manager class + deprecate inbound connection + convert connection pool and rate limiter classes into singletons + centralize connection pool and rate limiter configs into shared state + temporarily disable l2ps + convert peer connection states into integers + update types + skip TLSConnection refactor + remove ConsensusOmniAdapter (unused)
- genesis tx content items order
+ increment connection in connectionpool only
+ increase OMNI_MAX_CONNECTIONS_PER_IP
+ use socket.destroy to close connections
Contributor
WalkthroughDefaults flip TLSNOTARY_DISABLED and MONITORING_DISABLED to true; large OmniProtocol refactor introduces bidirectional PeerConnection, a singleton ConnectionPool, numeric ConnectionState machine, RateLimiter singleton, removes ConsensusOmniAdapter export, and adjusts startup/config wiring and port selection. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Socket as TCP Socket
participant Server as OmniProtocolServer
participant Pool as ConnectionPool
participant Limiter as RateLimiter
participant Peer as PeerConnection
participant Auth as Auth Handler
Socket->>Server: New inbound connection
Server->>Limiter: checkConnection(ipAddress, "OmniProtocolServer")
alt Rate limited
Limiter-->>Server: Blocked/Limited
Server->>Socket: Close connection
else Allowed
Limiter-->>Server: Allowed
Server->>Pool: handleInboundSocket(socket)
Pool->>Peer: Create pending PeerConnection (origin="inbound")
Peer->>Peer: startAuthTimeout()
Peer->>Auth: Receive auth message
Auth-->>Peer: Auth validated
Peer->>Pool: emit "authenticated"
Pool->>Pool: registerAuthenticatedConnection(peerIdentity)
Pool-->>Server: Return authenticated PeerConnection
Server->>Server: emit peer_authenticated
end
sequenceDiagram
autonumber
participant Client as Node code
participant Pool as ConnectionPool
participant Peer as PeerConnection
participant Socket as TCP Socket
participant Remote as Remote Peer
Client->>Pool: acquire(peerIdentity, connectionString)
alt Existing usable connection
Pool-->>Client: Return PeerConnection
else Create outbound
Pool->>Peer: Create PeerConnection (origin="outbound")
Peer->>Socket: connect(connectionString)
Socket->>Remote: TCP handshake
Remote-->>Socket: Connected
Socket-->>Peer: Connected
Peer->>Peer: setState(READY)
Pool->>Pool: track outbound connection
Pool-->>Client: Return PeerConnection
end
Client->>Peer: send(opcode,payload)
Peer->>Socket: write with sequence
Remote->>Socket: respond
Socket->>Peer: data received
Peer->>Peer: route response to in-flight request (cross-connection lookup)
Peer-->>Client: resolve Promise with response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Review Summary by QodoRefactor OmniProtocol to unified bidirectional connection management with numeric state machine
WalkthroughsDescription• Refactor OmniProtocol to support bidirectional TCP connections with unified ConnectionPool • Merge inbound and outbound connection handling into single PeerConnection class • Convert connection states to numeric scale for better state machine validation • Implement cross-connection response routing for multi-connection peer scenarios • Centralize OmniProtocol configuration and singletons (ConnectionPool, RateLimiter) • Fix genesis hash verification and transaction content field ordering • Deprecate ServerConnectionManager and InboundConnection classes Diagramflowchart LR
A["Inbound Socket"] -->|handleInboundSocket| B["ConnectionPool"]
C["Outbound Request"] -->|acquire| B
B -->|creates| D["PeerConnection<br/>bidirectional"]
D -->|numeric states| E["State Machine<br/>-3 to 11"]
D -->|send/receive| F["Message Handler"]
F -->|dispatch| G["Protocol Handler"]
G -->|response| D
H["RateLimiter<br/>singleton"] -->|rate limit| D
I["Shared State<br/>omniConfig"] -->|centralized config| B
File Changes1. src/libs/omniprotocol/transport/PeerConnection.ts
|
Contributor
Code Review by Qodo
1. Missing rate-limit constants
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+24
to
28
| constructor() { | ||
| this.config = structuredClone(getSharedState.omniConfig.rateLimit) | ||
|
|
||
| // Start cleanup timer | ||
| if (this.config.enabled) { |
Contributor
There was a problem hiding this comment.
1. Missing rate-limit constants 🐞 Bug ✓ Correctness
RateLimiter still uses RATE_LIMIT_BLOCK_DURATION_MS and DEFAULT_MANUAL_BLOCK_DURATION_MS but the constants import was removed, leaving unresolved identifiers that will fail compilation or throw at runtime when executed.
Agent Prompt
### Issue description
`RateLimiter` references `RATE_LIMIT_BLOCK_DURATION_MS` and `DEFAULT_MANUAL_BLOCK_DURATION_MS`, but they are not imported anymore. This leaves unresolved identifiers (build break) or a runtime crash if the code path is executed.
### Issue Context
The PR refactored `RateLimiter` to use `getSharedState.omniConfig.rateLimit` for configuration, but the block duration/manual block duration are still expressed via constants.
### Fix Focus Areas
- src/libs/omniprotocol/ratelimit/RateLimiter.ts[8-16]
- src/libs/omniprotocol/ratelimit/RateLimiter.ts[64-76]
- src/libs/omniprotocol/ratelimit/RateLimiter.ts[298-302]
### Suggested fix
Re-introduce the constants import (or inline/move these values into the shared config) so all referenced identifiers are defined, and add a quick unit/tsc check to ensure the file compiles.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+239
to
+244
| // const protocol = options?.protocol ?? "omni" | ||
|
|
||
| // INFO: If is local peer (us), always use httpCall | ||
| if (this.isLocalNode || protocol === "http") { | ||
| return await this.httpCall(request, isAuthenticated) | ||
| } | ||
| // if (this.isLocalNode || protocol === "http") { | ||
| // return await this.httpCall(request, isAuthenticated) | ||
| // } |
Contributor
There was a problem hiding this comment.
2. Protocol option ignored 🐞 Bug ✓ Correctness
Peer.call no longer honors CallOptions.protocol, so callers that explicitly request HTTP (notably Sync) will still attempt OmniProtocol first, changing behavior and potentially breaking sync flows.
Agent Prompt
### Issue description
`Peer.call()` ignores `CallOptions.protocol`, so code paths that deliberately force HTTP (e.g., Sync’s block download) cannot do so anymore.
### Issue Context
`Peer.longCall()` forwards the options through to `call()`, and Sync uses `protocol: "http"` in several places to force HTTP.
### Fix Focus Areas
- src/libs/peer/Peer.ts[233-267]
- src/libs/blockchain/routines/Sync.ts[216-232]
### Suggested fix
Restore a supported protocol switch:
- If `options?.protocol === "http"` then call `httpCall()` directly.
- Keep the Omni attempt as default when protocol is omitted.
Also ensure the local-node short-circuit behavior remains correct (avoid attempting Omni-to-self unless explicitly desired).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+755
to
+764
| const crossConnection = findInFlightRequestAcrossConnections( | ||
| this._peerIdentity, | ||
| header.sequence, | ||
| this, | ||
| ) | ||
| if (crossConnection) { | ||
| const { connection, pending: crossPending } = crossConnection | ||
| clearTimeout(crossPending.timer) | ||
| this.inFlightRequests.delete(header.sequence) | ||
| crossPending.resolve(payload as Buffer) |
Contributor
There was a problem hiding this comment.
3. Wrong in-flight deletion 🐞 Bug ✓ Correctness
When resolving a cross-connection response, PeerConnection deletes the sequence from the current connection’s inFlightRequests instead of the sibling connection that owns the pending request, leaking the pending entry and likely causing timeouts.
Agent Prompt
### Issue description
Cross-connection response routing resolves a pending request found on a sibling connection, but deletes the sequence from the wrong map (`this.inFlightRequests`). This leaves the real pending request in the sibling map and can cause leaked timers/state and timeouts.
### Issue Context
This happens in `PeerConnection.handleMessage()` when `header.opcode === 0xff` and the response arrives on a different socket than the request.
### Fix Focus Areas
- src/libs/omniprotocol/transport/PeerConnection.ts[739-769]
### Suggested fix
When `crossConnection` is found:
- call `connection.inFlightRequests.delete(header.sequence)` (not `this...`).
- consider logging/metrics for this path.
- add a small test that sends on conn A and receives response on conn B and asserts the pending entry is cleared.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+794
to
+813
| // Extract peer identity from auth block for ANY authenticated message | ||
| if ( | ||
| auth && | ||
| auth.identity && | ||
| this._state === ConnectionState.PENDING_AUTH | ||
| ) { | ||
| const newIdentity = "0x" + auth.identity.toString("hex") | ||
| this.setPeerIdentity(newIdentity) | ||
| this.setState(ConnectionState.READY) | ||
|
|
||
| if (this.authTimer) { | ||
| clearTimeout(this.authTimer) | ||
| this.authTimer = null | ||
| } | ||
|
|
||
| this.emit("authenticated", newIdentity) | ||
| log.info( | ||
| `[PeerConnection] ${this._peerIdentity} authenticated via auth block`, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
4. Unverified identity upgrade 🐞 Bug ⛨ Security
Inbound PeerConnection transitions from PENDING_AUTH to READY and emits "authenticated" solely based on auth.identity presence, even though dispatcher signature verification only runs for authRequired handlers, enabling identity spoofing for authRequired:false opcodes and connection-slot exhaustion for arbitrary identities.
Agent Prompt
### Issue description
Inbound connections are upgraded to READY and registered under a claimed identity merely because `auth.identity` exists, without ensuring that identity is cryptographically verified.
### Issue Context
`dispatchOmniMessage()` verifies signatures only for handlers where `descriptor.authRequired` is true; many opcodes are explicitly `authRequired: false`. Today, a peer can send an unauthenticated opcode with a forged `auth.identity`, causing the connection to be re-keyed/registered under a victim identity.
### Fix Focus Areas
- src/libs/omniprotocol/transport/PeerConnection.ts[783-813]
- src/libs/omniprotocol/protocol/dispatcher.ts[28-55]
- src/libs/omniprotocol/protocol/registry.ts[81-129]
- src/libs/omniprotocol/transport/ConnectionPool.ts[118-176]
### Suggested fix
Only emit `"authenticated"` / transition to READY after *successful* signature verification. Options:
1) Require a dedicated auth/hello opcode (authRequired: true) to establish identity for the socket.
2) Or, for the first message on a PENDING_AUTH connection, run SignatureVerifier.verify regardless of `authRequired` and only then set identity.
Also consider rejecting any non-auth opcode while in PENDING_AUTH to avoid ambiguous identity handling.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+217
to
+220
| // Listen for close to clean up rate limiter | ||
| peerConnection.once("close", () => { | ||
| this.rateLimiter.removeConnection(ipAddress) | ||
| }) |
Contributor
There was a problem hiding this comment.
5. Double rate-limit decrement 🐞 Bug ⛨ Security
Inbound connection close decrements RateLimiter connection counts twice (once in OmniProtocolServer and again in ConnectionPool), causing the per-IP connection counter to undercount and allowing more concurrent connections than configured.
Agent Prompt
### Issue description
RateLimiter connection accounting is decremented twice per inbound socket close, breaking enforcement of `maxConnectionsPerIP`.
### Issue Context
ConnectionPool already owns the socket lifecycle and calls `rateLimiter.addConnection()` and `rateLimiter.removeConnection()`.
### Fix Focus Areas
- src/libs/omniprotocol/server/OmniProtocolServer.ts[204-220]
- src/libs/omniprotocol/transport/ConnectionPool.ts[89-140]
- src/libs/omniprotocol/transport/ConnectionPool.ts[182-206]
### Suggested fix
Make only one component responsible for decrementing:
- Prefer: remove OmniProtocolServer’s `peerConnection.once("close", ...)` decrement, and rely on ConnectionPool’s cleanup.
- Ensure error paths don’t call removeConnection unless addConnection actually happened.
Add a small test/metric assertion that opening N sockets then closing one reduces the counter by exactly 1.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
96
to
107
| // Verify server certificate | ||
| if (!this.verifyServerCertificate(socket)) { | ||
| socket.destroy() | ||
| this.setState("ERROR") | ||
| this._setState(ConnectionState.ERROR) | ||
| reject(new Error("Server certificate verification failed")) | ||
| return | ||
| } | ||
|
|
||
| // Store socket | ||
| this.setSocket(socket) | ||
| this.setState("READY") | ||
| this.socket = socket | ||
| this._setState(ConnectionState.READY) | ||
|
|
Contributor
There was a problem hiding this comment.
6. Tls sockets not wired 🐞 Bug ⛯ Reliability
TLSConnection.connect sets this.socket and marks READY but never installs PeerConnection’s socket data/close/error handlers, so TLS connections will not parse inbound frames or resolve pending requests (leading to hangs/timeouts).
Agent Prompt
### Issue description
TLSConnection overrides `connect()` and sets `this.socket = socket`, but does not register the socket handlers that PeerConnection relies on. As a result, TLS-based OmniProtocol traffic won’t be processed.
### Issue Context
`ConnectionFactory` still creates TLSConnection for `tls://` and `tcps://` connection strings, so this path is active when TLS is enabled.
### Fix Focus Areas
- src/libs/omniprotocol/transport/TLSConnection.ts[39-125]
- src/libs/omniprotocol/transport/PeerConnection.ts[235-323]
- src/libs/omniprotocol/transport/ConnectionFactory.ts[24-52]
### Suggested fix
Refactor so TLSConnection can reuse the base handler wiring:
- Expose a protected method in PeerConnection like `protected attachSocket(socket: Socket): void` that sets `this.socket`, installs handlers, and resets timers; call it from both TCP and TLS connects.
- Avoid direct `(this as any)._state` mutation; route through a protected state setter.
- Add an integration test: create a TLSConnection, send a request, and assert the response is received (i.e., 'data' is being processed).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/libs/omniprotocol/server/TLSServer.ts (2)
48-51:⚠️ Potential issue | 🔴 CriticalUpdate this constructor call to the new
RateLimiterAPI.
RateLimiterno longer accepts a config argument in this PR, so this instantiation will fail type-checking here.Minimal compile fix
- this.rateLimiter = new RateLimiter( - this.config.rateLimit ?? { enabled: true }, - ) + this.rateLimiter = RateLimiter.getInstance()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/omniprotocol/server/TLSServer.ts` around lines 48 - 51, The RateLimiter constructor call in TLSServer is using a removed config parameter; update the instantiation so it matches the new API by removing the argument—replace the current new RateLimiter(this.config.rateLimit ?? { enabled: true }) with new RateLimiter() and assign it to this.rateLimiter (referencing the RateLimiter class and the TLSServer constructor / this.rateLimiter field).
53-58:⚠️ Potential issue | 🟠 MajorTLSServer bypasses the unified
ConnectionPoolarchitecture.TLSServer instantiates and manages connections through
ServerConnectionManagerrather than the unifiedConnectionPoolused byOmniProtocolServer. The startup path confirms this: when TLS is enabled (startup.ts:88), a separateTLSServerinstance is created; when disabled (startup.ts:103),OmniProtocolServeris created, which usesConnectionPool.handleInboundSocket()for all inbound connections.This architectural divergence means:
- TLS connections remain invisible to global pool-based limits, statistics, and inflight-request tracking
- ServerConnectionManager is deprecated (see ServerConnectionManager.ts comments) in favor of a unified ConnectionPool
- Pool limits and stats will diverge between TLS and plain TCP connection types
Refactor
TLSServerto useConnectionPooldirectly, similar toOmniProtocolServer, to unify connection management across all inbound connection types.Also applies to: 151-159, 253-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/omniprotocol/server/TLSServer.ts` around lines 53 - 58, TLSServer currently creates a ServerConnectionManager instance; replace that with using the shared ConnectionPool the rest of the server uses (same pattern as OmniProtocolServer) so TLS connections go through ConnectionPool.handleInboundSocket and global limits/stats/rate-limiting apply. Concretely: remove the new ServerConnectionManager({ ... }) creation in TLSServer, accept or obtain the existing ConnectionPool instance (inject it into TLSServer or import the singleton used by OmniProtocolServer), forward config values (maxConnections, connectionTimeout, authTimeout, rateLimiter) into the ConnectionPool configuration if needed, and change the TLS accept/connection handling code to call ConnectionPool.handleInboundSocket(...) instead of any ServerConnectionManager methods; also delete or deprecate any remaining ServerConnectionManager usage in TLSServer (including the other locations referenced) so all inbound sockets unify through ConnectionPool.src/libs/omniprotocol/transport/TLSConnection.ts (2)
47-57:⚠️ Potential issue | 🟠 MajorValidate local TLS files before switching to
CONNECTING.If one of these path checks throws, the instance stays in
CONNECTINGand cannot be retried cleanly.Suggested fix
// Parse connection string const parsed = this.parseConnectionStringForTLS() - this._setState(ConnectionState.CONNECTING) // Validate TLS configuration if (!fs.existsSync(this.tlsConfig.certPath)) { throw new Error(`Certificate not found: ${this.tlsConfig.certPath}`) } if (!fs.existsSync(this.tlsConfig.keyPath)) { throw new Error(`Private key not found: ${this.tlsConfig.keyPath}`) } + + this._setState(ConnectionState.CONNECTING)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/omniprotocol/transport/TLSConnection.ts` around lines 47 - 57, The code sets the connection state to CONNECTING before validating TLS files, so if fs.existsSync checks throw the instance remains stuck; move the TLS file validation (checks of this.tlsConfig.certPath and this.tlsConfig.keyPath, currently after parseConnectionStringForTLS()) to before calling this._setState(ConnectionState.CONNECTING), or alternatively wrap the validation and state transition in a try/catch that resets state (e.g., this._setState(ConnectionState.DISCONNECTED)) on error; update the logic around parseConnectionStringForTLS, this._setState, and the fs.existsSync checks accordingly so the instance never enters CONNECTING unless validation succeeds.
69-78:⚠️ Potential issue | 🟠 MajorThe timeout handler is not owning the live socket.
this.socketis only assigned aftersecureConnect, so a handshake timeout does not actually destroy the pending TLS socket.Suggested fix
- const timeoutTimer = setTimeout(() => { - if (this.socket) { - this.socket.destroy() - } - this._setState(ConnectionState.ERROR) - reject(new Error(`TLS connection timeout after ${timeout}ms`)) - }, timeout) - const tlsOptions: tls.ConnectionOptions = { host: parsed.host, port: parsed.port, key: keyPem, cert: certPem, @@ } const socket = tls.connect(tlsOptions) + this.socket = socket + + const timeoutTimer = setTimeout(() => { + socket.destroy() + this._setState(ConnectionState.ERROR) + reject(new Error(`TLS connection timeout after ${timeout}ms`)) + }, timeout)Also applies to: 91-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/omniprotocol/transport/TLSConnection.ts` around lines 69 - 78, The timeout handler in TLSConnection currently references this.socket which is set only after 'secureConnect', so a handshake timeout won't destroy the pending TLS socket; modify the connection logic in the Promise returned by TLSConnection.connect to capture the created/dialed socket in a local variable (e.g., let pendingSocket = tls.connect(...) or similar) and have the timeoutTimer destroy that pendingSocket (and clear timers) instead of this.socket, update any other timeout blocks (the similar handler around the 'secureConnect' listeners at the 91-106 region) to use the same local pendingSocket and ensure _setState(ConnectionState.ERROR) and reject(...) are still called after destroying the pendingSocket.src/libs/omniprotocol/server/OmniProtocolServer.ts (2)
169-223:⚠️ Potential issue | 🔴 CriticalAccepted sockets never increment the IP connection counter.
This path checks
checkConnection()and removes on"close", but it never callsaddConnection()after handing the socket to the pool.maxConnectionsPerIPtherefore never trips.Suggested fix
try { const peerConnection = this.connectionPool.handleInboundSocket(socket) + this.rateLimiter.addConnection(ipAddress) // Listen for authentication to emit event with peer identity peerConnection.once("authenticated", (peerIdentity: string) => { log.info( `[OmniProtocolServer] Peer authenticated: ${peerIdentity} from ${remoteAddress}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/omniprotocol/server/OmniProtocolServer.ts` around lines 169 - 223, The accepted-socket path calls this.rateLimiter.checkConnection(...) and removes on peerConnection.once("close") but never increments the counter, so call this.rateLimiter.addConnection(ipAddress) immediately after a successful this.connectionPool.handleInboundSocket(socket) returns (before emitting "connection_accepted"); keep the existing peerConnection.once("close", () => { this.rateLimiter.removeConnection(ipAddress) }) to decrement on close; this ensures maxConnectionsPerIP is enforced.
37-51:⚠️ Potential issue | 🟠 MajorThis constructor no longer applies its own connection limits.
config.rateLimitand the newmaxConnectionsPerPeerare stored onthis.config, but neither is forwarded intoRateLimiter.getInstance()/ConnectionPool.getInstance(). The old explicit pool config is commented out, so constructor overrides silently stop working.Also applies to: 53-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/omniprotocol/server/OmniProtocolServer.ts` around lines 37 - 51, The constructor stores rateLimit and maxConnectionsPerPeer on this.config but no longer passes them into the singletons, so restore forwarding of those options by calling RateLimiter.getInstance(this.config.rateLimit) and ConnectionPool.getInstance(this.rateLimiter, { maxConnectionsPerPeer: this.config.maxConnectionsPerPeer, ...otherNeededOptions }) (or equivalent parameter shapes expected by RateLimiter.getInstance and ConnectionPool.getInstance) so the runtime pool/limiter uses the constructor-provided limits; also reconcile the commented-out explicit pool config (around the earlier commented block) and ensure any other options from this.config (like maxConnections) are propagated where required. Use the symbols RateLimiter.getInstance, ConnectionPool.getInstance, this.config.rateLimit, and this.config.maxConnectionsPerPeer to locate and modify the code.src/libs/omniprotocol/ratelimit/RateLimiter.ts (1)
261-265:⚠️ Potential issue | 🟠 MajorA stop/start cycle leaves the singleton without cleanup.
stop()clearscleanupTimer, butgetInstance()returns the same object and never starts cleanup again. After the first stop, expired entries will accumulate forever.One way to restore cleanup on reuse
static getInstance(): RateLimiter { if (!this.instance) { this.instance = new RateLimiter() + } else if ( + this.instance.config.enabled && + !this.instance.cleanupTimer + ) { + this.instance.startCleanup() } return this.instance }Also applies to: 325-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/omniprotocol/ratelimit/RateLimiter.ts` around lines 261 - 265, The singleton's stop() clears cleanupTimer but getInstance() returns the same RateLimiter without re-enabling periodic cleanup, so expired entries pile up; modify getInstance() (or the factory that returns the singleton) to detect when this.cleanupTimer is undefined and call the existing cleanup-starting method (e.g., startCleanup() or whichever method creates cleanupTimer) to re-start the interval on reuse, and also ensure stop() only clears the timer (as it already does) without marking the instance unusable; apply the same restart-on-reuse logic to the other stop/start pair referenced (lines ~325-330) so any call path that returns the singleton will restore its cleanupTimer if missing.
🧹 Nitpick comments (3)
src/libs/peer/Peer.ts (1)
239-244: Commented-out code leaves dead API surface and changes local node behavior.Several concerns with this change:
Dead
protocoloption: TheCallOptions.protocolproperty (lines 17-19) is still defined and documented but now completely unused. Callers passingprotocol: "http"won't get HTTP-first behavior as the interface implies.Local node overhead: Previously,
isLocalNodecalls bypassed OmniProtocol for efficiency. Now they attempt OmniProtocol first, adding latency and a potential failure path before falling back to HTTP. If this is intentional, consider documenting why.Remove commented code: If this behavior change is intentional, delete the commented lines and either remove the
protocoloption fromCallOptionsor restore the routing logic.♻️ Option A: Remove dead code and unused interface property
export interface CallOptions { - /** Protocol selection (default: 'omni' - uses OmniProtocol when enabled, falls back to HTTP) */ - protocol?: "http" | "omni" /** Number of retry attempts (default: 3 for longCall) */ retries?: number /** Milliseconds between retries (default: 1000) */ sleepTime?: number /** Response codes that don't trigger retry (default: []) */ allowedCodes?: number[] }And at lines 239-244:
- // const protocol = options?.protocol ?? "omni" - - // INFO: If is local peer (us), always use httpCall - // if (this.isLocalNode || protocol === "http") { - // return await this.httpCall(request, isAuthenticated) - // }♻️ Option B: Restore local node optimization while keeping OmniProtocol as default
- // const protocol = options?.protocol ?? "omni" + const protocol = options?.protocol ?? "omni" - // INFO: If is local peer (us), always use httpCall - // if (this.isLocalNode || protocol === "http") { - // return await this.httpCall(request, isAuthenticated) - // } + // INFO: If is local peer (us) or explicit HTTP, skip OmniProtocol overhead + if (this.isLocalNode || protocol === "http") { + return await this.httpCall(request, isAuthenticated) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/peer/Peer.ts` around lines 239 - 244, The commented-out routing logic left CallOptions.protocol dead and changed behavior for local peers; restore deterministic routing by: 1) Re-enabling the check in the Peer call path to honor options.protocol and short-circuit to httpCall when protocol === "http"; 2) Ensure Peer.isLocalNode still forces httpCall first (i.e., if (this.isLocalNode || protocol === "http") return await this.httpCall(request, isAuthenticated)); and 3) if you prefer removing the option instead, delete CallOptions.protocol and its docs everywhere and remove related references; key symbols to update are CallOptions.protocol, Peer.isLocalNode, Peer.httpCall and the OmniProtocol fallback logic in the same method.src/utilities/mainLoop.ts (1)
139-189:peerRoutineis now dead code.The only call site at line 60 is commented out, making this entire function unreachable. If the refactor intentionally replaces
peerRoutinewithcheckOfflinePeers, consider removing this function to avoid confusion. Otherwise, if this function is still needed, the call at line 60 should be restored.Note: The old
peerRoutinealso calledcheckOfflinePeers()without await at line 146, which has the same unhandled rejection issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utilities/mainLoop.ts` around lines 139 - 189, peerRoutine is now dead code because its only call site was commented out; either delete peerRoutine to avoid confusion or restore its invocation where it used to be, and in either case fix the unhandled async calls inside it (specifically change checkOfflinePeers() to await checkOfflinePeers() or attach .catch(...) to handle rejections, and decide whether peerGossip() should be awaited or its promise handled). Locate the function peerRoutine and the async helpers checkOfflinePeers and peerGossip to apply the chosen fix (remove the function if unused, or re-enable its caller and ensure all internal async calls are properly awaited/handled).src/libs/omniprotocol/server/ServerConnectionManager.ts (1)
26-26: Keepconnectionsread-only outside this class.A public mutable
Maplets callers bypassremoveConnection(), which skips rate-limit decrements andconnection_removedevents. Expose aReadonlyMapor an accessor instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/omniprotocol/server/ServerConnectionManager.ts` at line 26, The public mutable Map named connections on ServerConnectionManager allows callers to mutate state and bypass removeConnection() (which handles rate-limit decrements and emits the connection_removed event); change the field to a private Map<string, InboundConnection> (or at least protected) and add a read-only accessor that returns a ReadonlyMap<string, InboundConnection> (e.g., a getter get connections(): ReadonlyMap<string, InboundConnection> or getConnections()) so external code can only observe but not modify the Map, and update any internal references to use the private map while keeping removeConnection() responsible for all deletions and side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@run`:
- Around line 10-11: The script currently force-sets TLSNOTARY_DISABLED and
MONITORING_DISABLED which overrides any environment/CLI-provided values; change
the assignments for TLSNOTARY_DISABLED and MONITORING_DISABLED so they only set
defaults when the variables are empty (e.g., use shell parameter expansion or a
test like [ -z "$TLSNOTARY_DISABLED" ] && TLSNOTARY_DISABLED=true), thereby
honoring exported or CLI-provided values while still providing a default.
In `@src/libs/blockchain/transaction.ts`:
- Line 60: The default for from_ed25519_address was changed from null to ""
which breaks null-coalescing and equality checks; restore the original semantics
by reverting the default in the transaction model (from_ed25519_address) back to
null OR update call-sites: change uses of genesisTx.content.from_ed25519_address
?? "0x0" to a falsy check (e.g., use || "0x0") or explicitly treat empty string
as unset, and update the strict comparison in handleIdentityRequest
(tx.content.from_ed25519_address !== sender) to account for "" (e.g., normalize
both sides or treat "" as null) so genesis transactions still fall back to "0x0"
and identity validation behaves as before.
In `@src/libs/l2ps/L2PSHashService.ts`:
- Around line 126-128: L2PSHashService.start() calls
ConnectionPool.getInstance() which currently may exit the process if the
singleton isn't initialized; modify this path to initialize or configure the
singleton instead of relying on prior initialization: change the call site in
L2PSHashService.start() (around the omniEnabled branch) to either call an
initializer on ConnectionPool that creates the singleton with
hash-relay-specific limits or to call a new getOrCreateInstance(...) API on
ConnectionPool that returns the singleton without terminating; alternatively, if
the relay should share the global pool, move the HASH_RELAY_* tuning into the
central Omni configuration and ensure ConnectionPool.initialize(...) is invoked
during app startup so getInstance() no longer exits.
In `@src/libs/omniprotocol/integration/BaseAdapter.ts`:
- Around line 52-55: The adapter currently ignores per-adapter pool settings:
BaseAdapter constructs the shared ConnectionPool via
ConnectionPool.getInstance(RateLimiter.getInstance()) while ConnectionPool
itself uses DEFAULT_OMNIPROTOCOL_CONFIG.pool; update the initialization so the
singleton honors BaseAdapterOptions.config.pool by passing the adapter's pool
config into ConnectionPool.getInstance (or pre-initializing the shared pool from
this.config.pool before any adapters are constructed), or alternatively
remove/validate BaseAdapterOptions.config.pool and throw if callers provide it;
reference BaseAdapter, BaseAdapterOptions.config.pool,
ConnectionPool.getInstance, RateLimiter.getInstance and
DEFAULT_OMNIPROTOCOL_CONFIG.pool when making the change.
In `@src/libs/omniprotocol/ratelimit/RateLimiter.ts`:
- Around line 8-16: The refactor removed the constants
RATE_LIMIT_BLOCK_DURATION_MS and DEFAULT_MANUAL_BLOCK_DURATION_MS but code in
RateLimiter.ts (references around RateLimiter class and its methods such as
where RateLimitConfig/RateLimitEntry/RateLimitResult/RateLimitType are used)
still depends on them; restore those constant definitions near the top of the
file (above imports usage) with their previous values or appropriate defaults so
the references at the noted locations (around lines where block durations are
applied: near 68-75, 180-187, 298-301) compile; ensure the constants are
exported/available in the same module scope and update any comments to match the
chosen durations.
In `@src/libs/omniprotocol/transport/ConnectionPool.ts`:
- Around line 759-762: The current getInstance check calls process.exit(1) on
missing initialization which force-crashes the process; instead, in the
ConnectionPool.getInstance (or equivalent static accessor using this.instance
and rateLimiter) replace the process.exit(1) path with throwing a descriptive
Error (e.g., "Connection Pool not initialized" plus context) so callers can
catch/handle startup ordering failures; ensure the thrown error is documented in
the method contract so callers know to handle it.
- Around line 145-171: The registerAuthenticatedConnection method currently only
enforces per-peer limits; add a global capacity check before accepting an
inbound authenticated connection by computing the total number of usable
connections across this.connections (e.g., flatten or sum usable connections
like existing.filter(...) for all peers) and comparing it against
this.config.maxTotalConnections; if the total is >= maxTotalConnections, reject
the connection the same way the per-peer branch does (call
connection.close().catch(() => {}), remove pending via
this.pendingConnections.delete(tempId) if not already, and
emit("connection_rejected", peerIdentity, "max_total_connections") or a similar
reason) so inbound auths cannot exceed the global pool size.
In `@src/libs/omniprotocol/transport/PeerConnection.ts`:
- Around line 755-764: The code is deleting the settled cross-routed pending
entry from the wrong map; instead of removing from this.inFlightRequests it must
remove the pending from the originating connection's map. In the block where you
handle findInFlightRequestAcrossConnections (and destructure { connection,
pending: crossPending }), clear the crossPending.timer, call
connection.inFlightRequests.delete(header.sequence) (not
this.inFlightRequests.delete), then resolve crossPending
(crossPending.resolve(payload as Buffer)); ensure you still clear the timer and
perform any other cleanup on the originating connection's state.
- Around line 604-605: The code in PeerConnection (method where it checks
this.socket.writable) dereferences this.socket without guarding for null; change
the condition to first verify the socket exists before accessing writable (e.g.,
if (!this.socket || !this.socket.writable) { ... }) and adjust the
error-handling branch to handle the null-socket case gracefully (use this.socket
presence to decide logging or fallback behavior) so no exception is thrown when
socket is gone.
- Around line 794-813: The code in PeerConnection prematurely trusts
auth.identity and promotes ConnectionState.PENDING_AUTH to READY and
setsPeerIdentity using the raw auth bytes before any signature verification;
remove this immediate promotion and stop calling setPeerIdentity,
setState(ConnectionState.READY), clear authTimer, emit("authenticated") and
logging here; instead defer setting the peer identity and transitioning to READY
until the authenticated dispatch/auth handshake path verifies the signature (use
the verified identity returned by that path, then call setPeerIdentity, clear
authTimer, setState(ConnectionState.READY), emit("authenticated",
verifiedIdentity) and log). Ensure the authTimer is still cleared when the
handshake completes or on explicit failure to avoid leaks.
In `@src/libs/omniprotocol/transport/TLSConnection.ts`:
- Around line 127-137: TLSConnection currently bypasses PeerConnection's
validation by setting (this as any)._state directly in TLSConnection._setState;
instead add a protected state-transition helper on PeerConnection (e.g.,
protected transitionState(next: number): void) that runs
ConnectionStateUtils.canTransition(), performs the guarded assignment of the
internal _state and emits "stateChange", and then update TLSConnection._setState
to call super.transitionState(state) (or remove TLSConnection._setState and call
the new protected method directly) so state changes go through PeerConnection's
validation and logging.
In `@src/libs/omniprotocol/transport/types.ts`:
- Around line 37-40: AUTHENTICATED and IDLE share the same numeric value (10)
causing exact-state checks and ConnectionStateUtils.getName(10) to misclassify
IDLE as AUTHENTICATED; update the enum/object entry for IDLE in
src/libs/omniprotocol/transport/types.ts to a unique integer distinct from
AUTHENTICATED and READY (e.g., 12) so IDLE-specific logic, metrics and cleanup
can reliably detect the idle state; ensure any switch/case or comparisons that
reference AUTHENTICATED, READY, or IDLE use the new numeric value consistently.
In `@src/utilities/mainLoop.ts`:
- Around line 60-61: The call to checkOfflinePeers() is fire-and-forget and may
cause an unhandled rejection and leave getSharedState.inPeerRecheckLoop stuck
true; update the caller in mainLoop to handle the promise (either await it or
attach .catch()) and modify checkOfflinePeers (the function that uses
Promise.all and sets getSharedState.inPeerRecheckLoop) to wrap its body in
try/finally so inPeerRecheckLoop is always reset, and ensure any Promise.all
rejections are propagated so the caller's catch can log errors arising from
sayHelloToPeer calls.
In `@src/utilities/sharedState.ts`:
- Around line 79-80: The omniConfig object currently hardcodes
omniConfig.enabled = true; change it to read the centralized flag instead so the
shared state respects the global config: set omniConfig.enabled to
Config.getInstance().omni.enabled (or equivalent getter) where omniConfig (type
OmniServerConfig) is constructed/initialized in sharedState.ts so callers don't
get a different enablement value than the central Config instance.
---
Outside diff comments:
In `@src/libs/omniprotocol/ratelimit/RateLimiter.ts`:
- Around line 261-265: The singleton's stop() clears cleanupTimer but
getInstance() returns the same RateLimiter without re-enabling periodic cleanup,
so expired entries pile up; modify getInstance() (or the factory that returns
the singleton) to detect when this.cleanupTimer is undefined and call the
existing cleanup-starting method (e.g., startCleanup() or whichever method
creates cleanupTimer) to re-start the interval on reuse, and also ensure stop()
only clears the timer (as it already does) without marking the instance
unusable; apply the same restart-on-reuse logic to the other stop/start pair
referenced (lines ~325-330) so any call path that returns the singleton will
restore its cleanupTimer if missing.
In `@src/libs/omniprotocol/server/OmniProtocolServer.ts`:
- Around line 169-223: The accepted-socket path calls
this.rateLimiter.checkConnection(...) and removes on
peerConnection.once("close") but never increments the counter, so call
this.rateLimiter.addConnection(ipAddress) immediately after a successful
this.connectionPool.handleInboundSocket(socket) returns (before emitting
"connection_accepted"); keep the existing peerConnection.once("close", () => {
this.rateLimiter.removeConnection(ipAddress) }) to decrement on close; this
ensures maxConnectionsPerIP is enforced.
- Around line 37-51: The constructor stores rateLimit and maxConnectionsPerPeer
on this.config but no longer passes them into the singletons, so restore
forwarding of those options by calling
RateLimiter.getInstance(this.config.rateLimit) and
ConnectionPool.getInstance(this.rateLimiter, { maxConnectionsPerPeer:
this.config.maxConnectionsPerPeer, ...otherNeededOptions }) (or equivalent
parameter shapes expected by RateLimiter.getInstance and
ConnectionPool.getInstance) so the runtime pool/limiter uses the
constructor-provided limits; also reconcile the commented-out explicit pool
config (around the earlier commented block) and ensure any other options from
this.config (like maxConnections) are propagated where required. Use the symbols
RateLimiter.getInstance, ConnectionPool.getInstance, this.config.rateLimit, and
this.config.maxConnectionsPerPeer to locate and modify the code.
In `@src/libs/omniprotocol/server/TLSServer.ts`:
- Around line 48-51: The RateLimiter constructor call in TLSServer is using a
removed config parameter; update the instantiation so it matches the new API by
removing the argument—replace the current new RateLimiter(this.config.rateLimit
?? { enabled: true }) with new RateLimiter() and assign it to this.rateLimiter
(referencing the RateLimiter class and the TLSServer constructor /
this.rateLimiter field).
- Around line 53-58: TLSServer currently creates a ServerConnectionManager
instance; replace that with using the shared ConnectionPool the rest of the
server uses (same pattern as OmniProtocolServer) so TLS connections go through
ConnectionPool.handleInboundSocket and global limits/stats/rate-limiting apply.
Concretely: remove the new ServerConnectionManager({ ... }) creation in
TLSServer, accept or obtain the existing ConnectionPool instance (inject it into
TLSServer or import the singleton used by OmniProtocolServer), forward config
values (maxConnections, connectionTimeout, authTimeout, rateLimiter) into the
ConnectionPool configuration if needed, and change the TLS accept/connection
handling code to call ConnectionPool.handleInboundSocket(...) instead of any
ServerConnectionManager methods; also delete or deprecate any remaining
ServerConnectionManager usage in TLSServer (including the other locations
referenced) so all inbound sockets unify through ConnectionPool.
In `@src/libs/omniprotocol/transport/TLSConnection.ts`:
- Around line 47-57: The code sets the connection state to CONNECTING before
validating TLS files, so if fs.existsSync checks throw the instance remains
stuck; move the TLS file validation (checks of this.tlsConfig.certPath and
this.tlsConfig.keyPath, currently after parseConnectionStringForTLS()) to before
calling this._setState(ConnectionState.CONNECTING), or alternatively wrap the
validation and state transition in a try/catch that resets state (e.g.,
this._setState(ConnectionState.DISCONNECTED)) on error; update the logic around
parseConnectionStringForTLS, this._setState, and the fs.existsSync checks
accordingly so the instance never enters CONNECTING unless validation succeeds.
- Around line 69-78: The timeout handler in TLSConnection currently references
this.socket which is set only after 'secureConnect', so a handshake timeout
won't destroy the pending TLS socket; modify the connection logic in the Promise
returned by TLSConnection.connect to capture the created/dialed socket in a
local variable (e.g., let pendingSocket = tls.connect(...) or similar) and have
the timeoutTimer destroy that pendingSocket (and clear timers) instead of
this.socket, update any other timeout blocks (the similar handler around the
'secureConnect' listeners at the 91-106 region) to use the same local
pendingSocket and ensure _setState(ConnectionState.ERROR) and reject(...) are
still called after destroying the pendingSocket.
---
Nitpick comments:
In `@src/libs/omniprotocol/server/ServerConnectionManager.ts`:
- Line 26: The public mutable Map named connections on ServerConnectionManager
allows callers to mutate state and bypass removeConnection() (which handles
rate-limit decrements and emits the connection_removed event); change the field
to a private Map<string, InboundConnection> (or at least protected) and add a
read-only accessor that returns a ReadonlyMap<string, InboundConnection> (e.g.,
a getter get connections(): ReadonlyMap<string, InboundConnection> or
getConnections()) so external code can only observe but not modify the Map, and
update any internal references to use the private map while keeping
removeConnection() responsible for all deletions and side effects.
In `@src/libs/peer/Peer.ts`:
- Around line 239-244: The commented-out routing logic left CallOptions.protocol
dead and changed behavior for local peers; restore deterministic routing by: 1)
Re-enabling the check in the Peer call path to honor options.protocol and
short-circuit to httpCall when protocol === "http"; 2) Ensure Peer.isLocalNode
still forces httpCall first (i.e., if (this.isLocalNode || protocol === "http")
return await this.httpCall(request, isAuthenticated)); and 3) if you prefer
removing the option instead, delete CallOptions.protocol and its docs everywhere
and remove related references; key symbols to update are CallOptions.protocol,
Peer.isLocalNode, Peer.httpCall and the OmniProtocol fallback logic in the same
method.
In `@src/utilities/mainLoop.ts`:
- Around line 139-189: peerRoutine is now dead code because its only call site
was commented out; either delete peerRoutine to avoid confusion or restore its
invocation where it used to be, and in either case fix the unhandled async calls
inside it (specifically change checkOfflinePeers() to await checkOfflinePeers()
or attach .catch(...) to handle rejections, and decide whether peerGossip()
should be awaited or its promise handled). Locate the function peerRoutine and
the async helpers checkOfflinePeers and peerGossip to apply the chosen fix
(remove the function if unused, or re-enable its caller and ensure all internal
async calls are properly awaited/handled).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb2e24b6-9c78-4792-a71c-780a6cb874d9
📒 Files selected for processing (23)
runsrc/index.tssrc/libs/blockchain/routines/Sync.tssrc/libs/blockchain/transaction.tssrc/libs/l2ps/L2PSHashService.tssrc/libs/omniprotocol/integration/BaseAdapter.tssrc/libs/omniprotocol/integration/consensusAdapter.tssrc/libs/omniprotocol/integration/index.tssrc/libs/omniprotocol/integration/startup.tssrc/libs/omniprotocol/ratelimit/RateLimiter.tssrc/libs/omniprotocol/server/InboundConnection.tssrc/libs/omniprotocol/server/OmniProtocolServer.tssrc/libs/omniprotocol/server/ServerConnectionManager.tssrc/libs/omniprotocol/server/TLSServer.tssrc/libs/omniprotocol/transport/ConnectionPool.tssrc/libs/omniprotocol/transport/PeerConnection.tssrc/libs/omniprotocol/transport/TLSConnection.tssrc/libs/omniprotocol/transport/types.tssrc/libs/peer/Peer.tssrc/libs/peer/PeerManager.tssrc/libs/peer/routines/peerBootstrap.tssrc/utilities/mainLoop.tssrc/utilities/sharedState.ts
💤 Files with no reviewable changes (1)
- src/libs/peer/routines/peerBootstrap.ts
Comment on lines
+10
to
+11
| TLSNOTARY_DISABLED=true | ||
| MONITORING_DISABLED=true |
Contributor
There was a problem hiding this comment.
Honor operator overrides for these feature defaults.
These assignments hard-force both stacks off for every run. Because they overwrite inherited env values and there is no positive CLI flag, MONITORING_DISABLED=false ./run and TLSNOTARY_DISABLED=false ./run still leave both features disabled.
💡 Suggested fix
-TLSNOTARY_DISABLED=true
-MONITORING_DISABLED=true
+: "${TLSNOTARY_DISABLED:=true}"
+: "${MONITORING_DISABLED:=true}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TLSNOTARY_DISABLED=true | |
| MONITORING_DISABLED=true | |
| : "${TLSNOTARY_DISABLED:=true}" | |
| : "${MONITORING_DISABLED:=true}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@run` around lines 10 - 11, The script currently force-sets TLSNOTARY_DISABLED
and MONITORING_DISABLED which overrides any environment/CLI-provided values;
change the assignments for TLSNOTARY_DISABLED and MONITORING_DISABLED so they
only set defaults when the variables are empty (e.g., use shell parameter
expansion or a test like [ -z "$TLSNOTARY_DISABLED" ] &&
TLSNOTARY_DISABLED=true), thereby honoring exported or CLI-provided values while
still providing a default.
| from_ed25519_address: null, | ||
| type: null, | ||
| from: "", | ||
| from_ed25519_address: "", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of from_ed25519_address with null-coalescing or equality checks
echo "=== Null coalescing patterns (will break) ==="
rg -n 'from_ed25519_address\s*\?\?' --type=ts
echo ""
echo "=== Strict equality checks (may be affected) ==="
rg -n 'from_ed25519_address\s*(===|!==)' --type=ts
echo ""
echo "=== Falsy checks (should work with both null and empty string) ==="
rg -n '(!|if\s*\()\s*\S*from_ed25519_address' --type=tsRepository: kynesyslabs/node
Length of output: 699
🏁 Script executed:
# 1. Read transaction.ts to see the type definition and default value
echo "=== transaction.ts around line 60 ==="
sed -n '50,70p' src/libs/blockchain/transaction.ts
# 2. Read subOperations.ts around line 60
echo ""
echo "=== subOperations.ts around line 60 ==="
sed -n '55,65p' src/libs/blockchain/routines/subOperations.ts
# 3. Read handleIdentityRequest.ts around line 63
echo ""
echo "=== handleIdentityRequest.ts around line 63 ==="
sed -n '55,75p' src/libs/network/routines/transactions/handleIdentityRequest.ts
# 4. Read consensus_rollback_smoke.ts around line 58
echo ""
echo "=== consensus_rollback_smoke.ts around line 58 ==="
sed -n '50,70p' better_testing/loadgen/src/features/consensus/consensus_rollback_smoke.tsRepository: kynesyslabs/node
Length of output: 3209
Breaking change: null → "" default affects downstream null-coalescing operator.
In subOperations.ts:60, the expression genesisTx.content.from_ed25519_address ?? "0x0" will now return "" instead of falling back to "0x0", because empty string is not nullish. Genesis transactions will receive "" instead of the intended "0x0" fallback for unset addresses.
Additionally, in handleIdentityRequest.ts:63, the strict equality check tx.content.from_ed25519_address !== sender now compares against "" instead of null, altering validation semantics.
If intentional, audit all usages of from_ed25519_address to confirm the empty string default is the desired behavior, especially in genesis transaction handling where the "0x0" fallback was previously expected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libs/blockchain/transaction.ts` at line 60, The default for
from_ed25519_address was changed from null to "" which breaks null-coalescing
and equality checks; restore the original semantics by reverting the default in
the transaction model (from_ed25519_address) back to null OR update call-sites:
change uses of genesisTx.content.from_ed25519_address ?? "0x0" to a falsy check
(e.g., use || "0x0") or explicitly treat empty string as unset, and update the
strict comparison in handleIdentityRequest (tx.content.from_ed25519_address !==
sender) to account for "" (e.g., normalize both sides or treat "" as null) so
genesis transactions still fall back to "0x0" and identity validation behaves as
before.
Comment on lines
126
to
128
| if (this.omniEnabled) { | ||
| this.connectionPool = new ConnectionPool({ | ||
| maxTotalConnections: HASH_RELAY_MAX_TOTAL_CONNECTIONS, | ||
| maxConnectionsPerPeer: HASH_RELAY_MAX_CONNECTIONS_PER_PEER, | ||
| idleTimeout: HASH_RELAY_IDLE_TIMEOUT_MS, | ||
| connectTimeout: HASH_RELAY_CONNECT_TIMEOUT_MS, | ||
| authTimeout: HASH_RELAY_AUTH_TIMEOUT_MS, | ||
| }) | ||
| this.connectionPool = ConnectionPool.getInstance() | ||
| log.info("[L2PS Hash Service] OmniProtocol enabled for hash relay") |
Contributor
There was a problem hiding this comment.
ConnectionPool.getInstance() can terminate startup here.
In src/libs/omniprotocol/transport/ConnectionPool.ts (Lines 751-765), the zero-arg getInstance() path exits the process when the singleton has not already been initialized. That makes L2PSHashService.start() order-dependent, and this change also stops applying the HASH_RELAY_* pool tuning that this service used before.
💡 Suggested fix
Initialize the singleton on this path instead of assuming another component already did:
- this.connectionPool = ConnectionPool.getInstance()
+ this.connectionPool = ConnectionPool.getInstance(
+ RateLimiter.getInstance(),
+ )If the relay is meant to share the global pool, move the hash-relay-specific limits into the centralized Omni config before startup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libs/l2ps/L2PSHashService.ts` around lines 126 - 128,
L2PSHashService.start() calls ConnectionPool.getInstance() which currently may
exit the process if the singleton isn't initialized; modify this path to
initialize or configure the singleton instead of relying on prior
initialization: change the call site in L2PSHashService.start() (around the
omniEnabled branch) to either call an initializer on ConnectionPool that creates
the singleton with hash-relay-specific limits or to call a new
getOrCreateInstance(...) API on ConnectionPool that returns the singleton
without terminating; alternatively, if the relay should share the global pool,
move the HASH_RELAY_* tuning into the central Omni configuration and ensure
ConnectionPool.initialize(...) is invoked during app startup so getInstance() no
longer exits.
Comment on lines
+52
to
+55
| // Initialize ConnectionPool with RateLimiter | ||
| this.connectionPool = ConnectionPool.getInstance( | ||
| RateLimiter.getInstance(), | ||
| ) |
Contributor
There was a problem hiding this comment.
This no longer honors options.config.pool.
In src/libs/omniprotocol/transport/ConnectionPool.ts (Lines 751-765), the singleton is initialized from DEFAULT_OMNIPROTOCOL_CONFIG.pool, not from this.config.pool. Any caller that overrides pool size or timeout settings through BaseAdapterOptions.config.pool now gets silently ignored.
Either initialize the shared pool from the adapter's config before constructing adapters, or remove per-adapter pool config from this API so the contract stays honest.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libs/omniprotocol/integration/BaseAdapter.ts` around lines 52 - 55, The
adapter currently ignores per-adapter pool settings: BaseAdapter constructs the
shared ConnectionPool via ConnectionPool.getInstance(RateLimiter.getInstance())
while ConnectionPool itself uses DEFAULT_OMNIPROTOCOL_CONFIG.pool; update the
initialization so the singleton honors BaseAdapterOptions.config.pool by passing
the adapter's pool config into ConnectionPool.getInstance (or pre-initializing
the shared pool from this.config.pool before any adapters are constructed), or
alternatively remove/validate BaseAdapterOptions.config.pool and throw if
callers provide it; reference BaseAdapter, BaseAdapterOptions.config.pool,
ConnectionPool.getInstance, RateLimiter.getInstance and
DEFAULT_OMNIPROTOCOL_CONFIG.pool when making the change.
Comment on lines
+8
to
16
| import { getSharedState } from "@/utilities/sharedState" | ||
| import { | ||
| RateLimitConfig, | ||
| RateLimitEntry, | ||
| RateLimitResult, | ||
| RateLimitType, | ||
| } from "./types" | ||
| import { | ||
| DEFAULT_MAX_CONNECTIONS_PER_IP, | ||
| DEFAULT_MAX_REQUESTS_PER_SECOND_PER_IP, | ||
| DEFAULT_MAX_REQUESTS_PER_SECOND_PER_IDENTITY, | ||
| DEFAULT_RATE_LIMIT_WINDOW_MS, | ||
| DEFAULT_RATE_LIMIT_ENTRY_TTL_MS, | ||
| DEFAULT_RATE_LIMIT_CLEANUP_INTERVAL_MS, | ||
| RATE_LIMIT_BLOCK_DURATION_MS, | ||
| DEFAULT_MANUAL_BLOCK_DURATION_MS, | ||
| } from "../constants" | ||
| import log from "@/utilities/tui/LegacyLoggerAdapter" | ||
|
|
Contributor
There was a problem hiding this comment.
Restore the block-duration definitions before merging.
RATE_LIMIT_BLOCK_DURATION_MS and DEFAULT_MANUAL_BLOCK_DURATION_MS are still used below, but this refactor removed their last visible import/definition. The file will not compile as-is.
Also applies to: 68-75, 180-187, 298-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libs/omniprotocol/ratelimit/RateLimiter.ts` around lines 8 - 16, The
refactor removed the constants RATE_LIMIT_BLOCK_DURATION_MS and
DEFAULT_MANUAL_BLOCK_DURATION_MS but code in RateLimiter.ts (references around
RateLimiter class and its methods such as where
RateLimitConfig/RateLimitEntry/RateLimitResult/RateLimitType are used) still
depends on them; restore those constant definitions near the top of the file
(above imports usage) with their previous values or appropriate defaults so the
references at the noted locations (around lines where block durations are
applied: near 68-75, 180-187, 298-301) compile; ensure the constants are
exported/available in the same module scope and update any comments to match the
chosen durations.
Comment on lines
+794
to
+813
| // Extract peer identity from auth block for ANY authenticated message | ||
| if ( | ||
| auth && | ||
| auth.identity && | ||
| this._state === ConnectionState.PENDING_AUTH | ||
| ) { | ||
| const newIdentity = "0x" + auth.identity.toString("hex") | ||
| this.setPeerIdentity(newIdentity) | ||
| this.setState(ConnectionState.READY) | ||
|
|
||
| if (this.authTimer) { | ||
| clearTimeout(this.authTimer) | ||
| this.authTimer = null | ||
| } | ||
|
|
||
| this.emit("authenticated", newIdentity) | ||
| log.info( | ||
| `[PeerConnection] ${this._peerIdentity} authenticated via auth block`, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Do not trust auth.identity before signature verification.
Line 794-Line 813 promotes PENDING_AUTH to READY and sets peer identity from raw auth bytes before verification. This allows identity spoofing/auth-bypass on first inbound message.
(Reference: src/libs/omniprotocol/protocol/dispatcher.ts:12-76 only verifies signatures for handlers requiring auth.)
🔧 Suggested fix direction
- // Extract peer identity from auth block for ANY authenticated message
- if (
- auth &&
- auth.identity &&
- this._state === ConnectionState.PENDING_AUTH
- ) {
- const newIdentity = "0x" + auth.identity.toString("hex")
- this.setPeerIdentity(newIdentity)
- this.setState(ConnectionState.READY)
-
- if (this.authTimer) {
- clearTimeout(this.authTimer)
- this.authTimer = null
- }
-
- this.emit("authenticated", newIdentity)
- log.info(
- `[PeerConnection] ${this._peerIdentity} authenticated via auth block`,
- )
- }
+ // Do not mark authenticated from unverified auth bytes.
+ // Promote identity/state only after verified auth result from trusted verification flow.Then transition to READY only after verified identity is available from the authenticated dispatch/auth handshake path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libs/omniprotocol/transport/PeerConnection.ts` around lines 794 - 813,
The code in PeerConnection prematurely trusts auth.identity and promotes
ConnectionState.PENDING_AUTH to READY and setsPeerIdentity using the raw auth
bytes before any signature verification; remove this immediate promotion and
stop calling setPeerIdentity, setState(ConnectionState.READY), clear authTimer,
emit("authenticated") and logging here; instead defer setting the peer identity
and transitioning to READY until the authenticated dispatch/auth handshake path
verifies the signature (use the verified identity returned by that path, then
call setPeerIdentity, clear authTimer, setState(ConnectionState.READY),
emit("authenticated", verifiedIdentity) and log). Ensure the authTimer is still
cleared when the handshake completes or on explicit failure to avoid leaks.
Comment on lines
+127
to
+137
| /** | ||
| * Internal setState that accesses parent's protected method | ||
| * @private | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| private _setState(state: number): void { | ||
| // Access the parent's state through casting | ||
| // This is a workaround until TLSConnection is properly refactored | ||
| (this as any)._state = state | ||
| this.emit("stateChange", { to: state }) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for live TLSConnection references"
rg -n -C2 '\bTLSConnection\b' src
echo
echo "Inspecting PeerConnection state API"
nl -ba src/libs/omniprotocol/transport/PeerConnection.ts | sed -n '1,260p'
echo
rg -n -C3 '_state|stateChange|setState|getState|ConnectionState' src/libs/omniprotocol/transportRepository: kynesyslabs/node
Length of output: 9085
🏁 Script executed:
wc -l src/libs/omniprotocol/transport/PeerConnection.tsRepository: kynesyslabs/node
Length of output: 113
🏁 Script executed:
rg -n '_state|setState|stateChange|getState|ConnectionState' src/libs/omniprotocol/transport/PeerConnection.ts -A 2Repository: kynesyslabs/node
Length of output: 4438
Don't bypass PeerConnection's private state with any.
Writing (this as any)._state = state skips ConnectionStateUtils.canTransition() validation, which prevents invalid state transitions and logs warnings. This allows state-dependent cleanup and auth behavior to diverge without detection.
TLSConnection has live call sites (used in ConnectionFactory.createConnection()), but PeerConnection only exposes a public getState() method—setState() is private. Either expose a protected/public state transition method in PeerConnection for subclasses, or fully deprecate and refactor TLSConnection away from the base class.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libs/omniprotocol/transport/TLSConnection.ts` around lines 127 - 137,
TLSConnection currently bypasses PeerConnection's validation by setting (this as
any)._state directly in TLSConnection._setState; instead add a protected
state-transition helper on PeerConnection (e.g., protected transitionState(next:
number): void) that runs ConnectionStateUtils.canTransition(), performs the
guarded assignment of the internal _state and emits "stateChange", and then
update TLSConnection._setState to call super.transitionState(state) (or remove
TLSConnection._setState and call the new protected method directly) so state
changes go through PeerConnection's validation and logging.
Comment on lines
+37
to
+40
| // Usable states (positive 10+) | ||
| AUTHENTICATED: 10, | ||
| READY: 11, | ||
| IDLE: 10, |
Contributor
There was a problem hiding this comment.
Give IDLE a distinct numeric state.
AUTHENTICATED and IDLE both being 10 means exact-state checks cannot tell them apart. That already makes ConnectionStateUtils.getName(10) collapse to AUTHENTICATED, and any idle-specific stats/cleanup will misclassify live authenticated connections.
💡 Suggested fix
// Usable states (positive 10+)
AUTHENTICATED: 10,
READY: 11,
- IDLE: 10,
+ IDLE: 12,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libs/omniprotocol/transport/types.ts` around lines 37 - 40, AUTHENTICATED
and IDLE share the same numeric value (10) causing exact-state checks and
ConnectionStateUtils.getName(10) to misclassify IDLE as AUTHENTICATED; update
the enum/object entry for IDLE in src/libs/omniprotocol/transport/types.ts to a
unique integer distinct from AUTHENTICATED and READY (e.g., 12) so IDLE-specific
logic, metrics and cleanup can reliably detect the idle state; ensure any
switch/case or comparisons that reference AUTHENTICATED, READY, or IDLE use the
new numeric value consistently.
Comment on lines
+60
to
+61
| // await peerRoutine() | ||
| checkOfflinePeers() |
Contributor
There was a problem hiding this comment.
Unhandled promise rejection will crash the process or permanently block peer rechecks.
checkOfflinePeers() is called without await or .catch(). According to the implementation in checkOfflinePeers.ts, if any sayHelloToPeer call fails, the Promise.all will reject with no handler, causing an unhandled promise rejection. Additionally, the inPeerRecheckLoop flag is set to true at the start but only reset at the end—if an error occurs mid-execution, the flag remains true forever, blocking all future peer rechecks.
🐛 Proposed fix: add error handling for fire-and-forget call
// await peerRoutine()
- checkOfflinePeers()
+ checkOfflinePeers().catch(err => {
+ log.error(`[MAIN LOOP] checkOfflinePeers failed: ${err}`)
+ })Additionally, consider wrapping the body of checkOfflinePeers() in a try-finally to ensure inPeerRecheckLoop is always reset:
// In src/libs/peer/routines/checkOfflinePeers.ts
export default async function checkOfflinePeers(): Promise<void> {
if (getSharedState.inPeerRecheckLoop) {
return
}
getSharedState.inPeerRecheckLoop = true
try {
// ... existing logic ...
await Promise.all(checkPromises)
log.info("[PEER RECHECK] Finished checking offline peers")
} finally {
getSharedState.inPeerRecheckLoop = false
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // await peerRoutine() | |
| checkOfflinePeers() | |
| // await peerRoutine() | |
| checkOfflinePeers().catch(err => { | |
| log.error(`[MAIN LOOP] checkOfflinePeers failed: ${err}`) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utilities/mainLoop.ts` around lines 60 - 61, The call to
checkOfflinePeers() is fire-and-forget and may cause an unhandled rejection and
leave getSharedState.inPeerRecheckLoop stuck true; update the caller in mainLoop
to handle the promise (either await it or attach .catch()) and modify
checkOfflinePeers (the function that uses Promise.all and sets
getSharedState.inPeerRecheckLoop) to wrap its body in try/finally so
inPeerRecheckLoop is always reset, and ensure any Promise.all rejections are
propagated so the caller's catch can log errors arising from sayHelloToPeer
calls.
Comment on lines
+79
to
+80
| omniConfig: OmniServerConfig = { | ||
| enabled: true, |
Contributor
There was a problem hiding this comment.
Use the configured Omni enable flag in the shared config.
omniConfig.enabled is hard-coded to true, so it can disagree with Config.getInstance().omni.enabled and re-enable Omni in callers that trust the centralized config.
Suggested fix
- enabled: true,
+ enabled: Config.getInstance().omni.enabled,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| omniConfig: OmniServerConfig = { | |
| enabled: true, | |
| omniConfig: OmniServerConfig = { | |
| enabled: Config.getInstance().omni.enabled, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utilities/sharedState.ts` around lines 79 - 80, The omniConfig object
currently hardcodes omniConfig.enabled = true; change it to read the centralized
flag instead so the shared state respects the global config: set
omniConfig.enabled to Config.getInstance().omni.enabled (or equivalent getter)
where omniConfig (type OmniServerConfig) is constructed/initialized in
sharedState.ts so callers don't get a different enablement value than the
central Config instance.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
474-482:⚠️ Potential issue | 🟠 MajorContradictory error handling: failsafe comment vs. hard exit.
The catch block on lines 474-477 states "Continue without OmniProtocol (failsafe - falls back to HTTP)", but the guard on lines 479-482 will
process.exit(1)ifomniAdapteris not set. IfstartOmniProtocolServerorinitOmniProtocolthrows, the catch block handles it and execution continues to the guard, which then exits—contradicting the failsafe behavior.Either:
- Remove the guard (lines 479-482) to honor the failsafe intent, or
- Move the guard inside the try block and remove the failsafe comment if OmniProtocol is now required.
Option 1: Remove the guard to honor failsafe behavior
} catch (error) { handleError(error, "NETWORK", { source: ErrorSource.OMNI_STARTUP }) // Continue without OmniProtocol (failsafe - falls back to HTTP) } - - if (!getSharedState.omniAdapter) { - log.error("[CORE] Failed to start OmniProtocol server") - process.exit(1) - } } else {Option 2: Make OmniProtocol required when enabled
try { getSharedState.omniConfig.port = indexState.OMNI_PORT const omniServer = await startOmniProtocolServer( getSharedState.omniConfig, ) indexState.omniServer = omniServer log.info(`[CORE] OmniProtocol server started on port ${indexState.OMNI_PORT}`) // REVIEW: Initialize OmniProtocol client adapter for outbound peer communication // Use OMNI_ONLY mode for testing, OMNI_PREFERRED for production gradual rollout const omniMode = (Config.getInstance().omni.mode as | "HTTP_ONLY" | "OMNI_PREFERRED" | "OMNI_ONLY") || "OMNI_ONLY" getSharedState.initOmniProtocol(omniMode) log.info(`[CORE] OmniProtocol client adapter initialized with mode: ${omniMode}`) + + if (!getSharedState.omniAdapter) { + throw new Error("OmniProtocol adapter failed to initialize") + } } catch (error) { handleError(error, "NETWORK", { source: ErrorSource.OMNI_STARTUP }) - // Continue without OmniProtocol (failsafe - falls back to HTTP) + log.error("[CORE] Failed to start OmniProtocol server") + process.exit(1) } - - if (!getSharedState.omniAdapter) { - log.error("[CORE] Failed to start OmniProtocol server") - process.exit(1) - } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 474 - 482, The catch claims we should "Continue without OmniProtocol (failsafe)" but the subsequent guard (checking getSharedState.omniAdapter and calling process.exit(1)) forces a hard exit; fix by making behavior consistent: either remove the post-catch guard block that logs and exits so the app continues without OmniProtocol, or if OmniProtocol must be required, move the guard into the try surrounding startOmniProtocolServer/initOmniProtocol and remove the failsafe comment. Update or remove the process.exit(1) and corresponding log so it matches the chosen behavior (references: startOmniProtocolServer, initOmniProtocol, handleError, and getSharedState.omniAdapter).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 855-858: The global force exit timer set by const forceExitTimeout
= setTimeout(..., 3_000) is too short relative to individual service shutdown
timeouts (e.g., L2PS services calling stop(3000)); update the shutdown budget by
increasing the force exit timeout to a safe value (e.g., 10_000) or
alternatively reduce individual service stop(...) timeouts so their total
worst-case duration fits under the global timer; adjust the constant in the
setTimeout call (forceExitTimeout) and/or the stop(3000) invocations to keep the
global timer larger than the sum of per-service timeouts and ensure all services
have time to clean up.
- Line 260: Restore the original fallback behavior for SERVER_PORT so it uses
cfg.server.rpcPort with cfg.server.serverPort as a fallback (matching the
SIGNALING_SERVER_PORT pattern), i.e., change the assignment of
indexState.SERVER_PORT to use cfg.server.rpcPort || cfg.server.serverPort (or,
if intentional to require RPC_PORT, add a comment/documentation stating that
indexState.SERVER_PORT intentionally uses only cfg.server.rpcPort and that
RPC_PORT must be set); update the assignment near indexState.SERVER_PORT and
mirror the same fallback logic used by SIGNALING_SERVER_PORT for consistency.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 474-482: The catch claims we should "Continue without OmniProtocol
(failsafe)" but the subsequent guard (checking getSharedState.omniAdapter and
calling process.exit(1)) forces a hard exit; fix by making behavior consistent:
either remove the post-catch guard block that logs and exits so the app
continues without OmniProtocol, or if OmniProtocol must be required, move the
guard into the try surrounding startOmniProtocolServer/initOmniProtocol and
remove the failsafe comment. Update or remove the process.exit(1) and
corresponding log so it matches the chosen behavior (references:
startOmniProtocolServer, initOmniProtocol, handleError, and
getSharedState.omniAdapter).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5423170c-2eec-4d84-adfd-62b245ec4995
📒 Files selected for processing (3)
src/config/loader.tssrc/config/types.tssrc/index.ts
💤 Files with no reviewable changes (1)
- src/config/types.ts
| indexState.PG_PORT = cfg.database.port | ||
| // Allow overriding server port through RPC_PORT | ||
| indexState.SERVER_PORT = cfg.server.rpcPort || cfg.server.serverPort | ||
| indexState.SERVER_PORT = cfg.server.rpcPort |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the default value for rpcPort in defaults.ts
rg -n 'rpcPort' src/config/defaults.tsRepository: kynesyslabs/node
Length of output: 82
🏁 Script executed:
# Check the full defaults.ts to understand server config structure
head -50 src/config/defaults.tsRepository: kynesyslabs/node
Length of output: 1281
🏁 Script executed:
# Check if serverPort still exists in defaults
rg -n 'serverPort' src/config/defaults.tsRepository: kynesyslabs/node
Length of output: 217
🏁 Script executed:
# Look at the surrounding context in index.ts around line 260
sed -n '250,270p' src/index.tsRepository: kynesyslabs/node
Length of output: 957
🏁 Script executed:
# Check for any env var defaults for rpcPort
rg -n 'RPC_PORT|rpcPort' src/config/ --type tsRepository: kynesyslabs/node
Length of output: 460
🏁 Script executed:
# Check for any comments or git history about this change
rg -B5 -A5 'SERVER_PORT = cfg.server.rpcPort' src/index.tsRepository: kynesyslabs/node
Length of output: 572
🏁 Script executed:
# Check if RPC_PORT is documented as required
rg -i 'RPC_PORT' README.md .env.example .env src/config/Repository: kynesyslabs/node
Length of output: 250
🏁 Script executed:
# Look for any issue or comment about removing serverPort fallback
rg -i 'serverPort.*fallback|remove.*fallback|intentional' src/Repository: kynesyslabs/node
Length of output: 602
Removed serverPort fallback for SERVER_PORT with inconsistent default behavior.
The assignment changed from cfg.server.rpcPort || cfg.server.serverPort to cfg.server.rpcPort without fallback. Since rpcPort defaults to 0 in src/config/defaults.ts, SERVER_PORT will default to 0, causing the RPC server to bind to a random port unless explicitly configured via the RPC_PORT environment variable.
This is inconsistent with the pattern used for SIGNALING_SERVER_PORT on line 263-265, which includes a fallback: cfg.server.rpcSignalingPort || cfg.server.signalingServerPort.
Clarify whether this behavioral change is intentional and ensure that RPC_PORT is documented as a required configuration if random port binding is not the intended behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` at line 260, Restore the original fallback behavior for
SERVER_PORT so it uses cfg.server.rpcPort with cfg.server.serverPort as a
fallback (matching the SIGNALING_SERVER_PORT pattern), i.e., change the
assignment of indexState.SERVER_PORT to use cfg.server.rpcPort ||
cfg.server.serverPort (or, if intentional to require RPC_PORT, add a
comment/documentation stating that indexState.SERVER_PORT intentionally uses
only cfg.server.rpcPort and that RPC_PORT must be set); update the assignment
near indexState.SERVER_PORT and mirror the same fallback logic used by
SIGNALING_SERVER_PORT for consistency.
Comment on lines
855
to
+858
| const forceExitTimeout = setTimeout(() => { | ||
| log.warning("[CORE] Shutdown timeout exceeded, forcing exit...") | ||
| process.exit(0) | ||
| }, 5_000) | ||
| }, 3_000) |
Contributor
There was a problem hiding this comment.
Force exit timeout (3s) may be too aggressive for cleanup.
The timeout was reduced to 3 seconds, but the shutdown sequence includes multiple services with their own timeouts. For example, L2PS services at lines 882-883 each have a 3-second timeout (stop(3000)), meaning just these two services could take up to 6 seconds if they need the full timeout.
Consider either:
- Increasing the force exit timeout to accommodate the sum of individual service timeouts, or
- Reducing individual service timeouts to fit within the global budget.
Suggested fix: Increase timeout to 10 seconds
const forceExitTimeout = setTimeout(() => {
log.warning("[CORE] Shutdown timeout exceeded, forcing exit...")
process.exit(0)
- }, 3_000)
+ }, 10_000)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const forceExitTimeout = setTimeout(() => { | |
| log.warning("[CORE] Shutdown timeout exceeded, forcing exit...") | |
| process.exit(0) | |
| }, 5_000) | |
| }, 3_000) | |
| const forceExitTimeout = setTimeout(() => { | |
| log.warning("[CORE] Shutdown timeout exceeded, forcing exit...") | |
| process.exit(0) | |
| }, 10_000) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 855 - 858, The global force exit timer set by
const forceExitTimeout = setTimeout(..., 3_000) is too short relative to
individual service shutdown timeouts (e.g., L2PS services calling stop(3000));
update the shutdown budget by increasing the force exit timeout to a safe value
(e.g., 10_000) or alternatively reduce individual service stop(...) timeouts so
their total worst-case duration fits under the global timer; adjust the constant
in the setTimeout call (forceExitTimeout) and/or the stop(3000) invocations to
keep the global timer larger than the sum of per-service timeouts and ensure all
services have time to clean up.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/libs/blockchain/gcr/handleGCR.ts (1)
559-578: Pre-existing:counteris never incremented in rollback logging.The
countervariable is initialized to0but never incremented in the loop, so the debug and info logs will always report0/Nand0 GCREdits rolled backregardless of actual progress.♻️ Suggested fix
- const counter = 0 const results: GCRResult[] = [] - for (const edit of appliedEdits) { + for (let counter = 0; counter < appliedEdits.length; counter++) { + const edit = appliedEdits[counter] log.debug( "[rollback] (" + counter +Or use
forEachwith index, or incrementcounterinside the loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/handleGCR.ts` around lines 559 - 578, The rollback logging uses a non-updated counter so messages always show 0; update the loop over appliedEdits (the for (const edit of appliedEdits) block) to track progress by either replacing the loop with an indexed iteration (e.g., for (let i = 0; i < appliedEdits.length; i++)) or incrementing counter inside the loop after each this.apply(edit, tx, true) call, and then use that counter/index in the log.debug and log.info messages (which reference counter, appliedEdits.length, and tx.hash) so they report the actual number of GCREdits rolled back.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/libs/blockchain/gcr/handleGCR.ts`:
- Around line 559-578: The rollback logging uses a non-updated counter so
messages always show 0; update the loop over appliedEdits (the for (const edit
of appliedEdits) block) to track progress by either replacing the loop with an
indexed iteration (e.g., for (let i = 0; i < appliedEdits.length; i++)) or
incrementing counter inside the loop after each this.apply(edit, tx, true) call,
and then use that counter/index in the log.debug and log.info messages (which
reference counter, appliedEdits.length, and tx.hash) so they report the actual
number of GCREdits rolled back.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 004f5409-bda3-46c0-99ba-582f730a2416
📒 Files selected for processing (2)
runsrc/libs/blockchain/gcr/handleGCR.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- run
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Centralize peer connections and socket management using the
ConnectionPoolclass.Merge inbound and outbound connections into the
PeerConnectionclass.Centralize omni protocol configs using
sharedState.omniConfigSummary by CodeRabbit
New Features
Bug Fixes
Improvements