Conversation
Implements TLSNotary (MPC-TLS) support in the Demos node for verifiable HTTPS attestation proofs without compromising user privacy. Key changes: - Add libs/tlsn/ with pre-built Rust library (libtlsn_notary.so) - Create src/features/tlsnotary/ feature module: - ffi.ts: Bun FFI bindings to Rust library - TLSNotaryService.ts: Service wrapper with lifecycle management - routes.ts: HTTP API endpoints (/tlsnotary/health, /info, /verify) - index.ts: Feature entry point with exports - Integrate with node startup in src/index.ts: - Add indexState properties for configuration - Initialize service in main() with failsafe pattern - Add graceful shutdown cleanup - Register routes in src/libs/network/server_rpc.ts Configuration via environment variables: - TLSNOTARY_ENABLED: Enable/disable (default: false) - TLSNOTARY_PORT: WebSocket port (default: 7047) - TLSNOTARY_SIGNING_KEY: 32-byte hex secp256k1 key (required if enabled) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add tlsnotary.getInfo nodeCall handler for SDK auto-configuration - Implement signing key resolution with priority: ENV > file > auto-generate - Add .tlsnotary-key to gitignore for secure key storage - Update .env.example with TLSNotary configuration variables 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ution - Replace all console.log/warn/error with log.info/warning/error - Fix TypeScript error: use exposedUrl instead of non-existent host property - Parse URL hostname from exposedUrl for WebSocket notary URLs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add TLSN LogCategory type for TLSNotary HTTPS attestation operations - Add tag mappings: TLSNOTARY, TLSNotary, TLSN, NOTARY, ATTESTATION - Add TLSN tab (key: =) to TUI, move CMD to backslash key 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use 'as any' casts for FFI pointer parameters to match the reference
implementation in demos_tlsnotary/node/ts/TLSNotary.ts. This resolves
8 TypeScript errors where 'number' was not assignable to 'Pointer'.
The bun:ffi Pointer type is branded (number & { __pointer__: null })
but at runtime, pointers are plain numbers. The 'as any' pattern is
the pragmatic solution used by the upstream reference implementation.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The TLSN and CMD categories were defined in LogCategory type and tagCategories.ts but were missing from the ALL_CATEGORIES array that initializes the per-category ring buffers. This caused logs for these categories to not be stored/displayed in their tabs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Extended NodeInfo interface with optional tlsnotary field (enabled, port, running) - Added TLSN status indicator on line 5 of header with visual indicators: - 🔐 icon + green "✓ :port" when running - 🔐 icon + red "✗ STOPPED" when not running - Updated index.ts to push TLSNotary info to TUI after initialization - Fixed lint error: wrapped case block lexical declaration in braces 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@noble/ed25519 v3.x has TypeScript type issues with sha512Sync configuration. Switched to node-forge's forge.pki.ed25519.sign/verify which is consistent with the SDK's Cryptography module and doesn't require SHA-512 configuration. This fixes "hashes.sha512 not set" error during Ed25519 signing/verification. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When OmniProtocol wraps RPC calls in NODE_CALL, the hello_peer method was falling through to manageNodeCall which lacks a hello_peer case, resulting in "Received unknown message" warnings. Added hello_peer routing in handleNodeCall (control.ts) to forward the request to manageHelloPeer with the authenticated peer identity from the OmniProtocol auth block. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Added environment variables for TLSNotary debugging: - TLSNOTARY_FATAL: Make errors fatal (exit on failure) for debugging - TLSNOTARY_DEBUG: Enable verbose debug logging Also added: - Port collision warning when both OmniProtocol and TLSNotary are enabled - More detailed error logging during initialization and start - Fatal mode support in index.ts for early exit on TLSNotary failure This helps diagnose issues like the "WebSocket upgrade failed" error which can occur when OmniProtocol tries to connect to TLSNotary port. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add TLSNOTARY_PROXY environment variable that enables a TCP proxy to intercept and log all incoming data before forwarding to the Rust server. This helps debug what's connecting to the TLSNotary port. When enabled: - Rust server runs on port+1 (internal) - Node.js proxy listens on configured port - All incoming data is logged (text + hex preview) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ation Adds WebSocket proxy spawning system for domain-specific TLS attestation: - portAllocator.ts: Port pool management (55000-57000) with sequential allocation and recycling - proxyManager.ts: Proxy lifecycle management with: - wstcp binary auto-installation - Activity monitoring with 30s idle timeout - Lazy cleanup on subsequent requests - Status reporting and manual kill controls - requestTLSNproxy nodeCall: SDK endpoint for dynamic proxy requests - SDK_INTEGRATION.md: Integration documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Token Manager: - In-memory token store for paid attestation access - Domain-locked tokens with 30min expiry, 3 retries - Full lifecycle: pending → active → completed → stored - Periodic cleanup of expired tokens Proxy Manager Fixes: - Better error detection: catch wstcp panics from stderr - Detect "Address already in use" errors properly - Port availability: use actual socket binding test instead of lsof 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
PR Compliance Guide 🔍(Compliance updated until commit 2f0ea12)Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label Previous compliance checksCompliance check up to commit bff37bb
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Caution Review failedThe pull request is closed. WalkthroughAdds a multi-part TLSNotary integration: dual-mode service (FFI & Docker), FFI bindings, proxy manager and port allocator, token lifecycle and GCR wiring, NodeCall and HTTP APIs, wstcp proxy orchestration, CLI/docker helpers, Ed25519 → node-forge migration, and payload-size hardening. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NodeCall
participant TokenManager
participant ProxyManager
participant PortAllocator
participant Wstcp as wstcp_Process
Client->>NodeCall: requestTLSNproxy(targetUrl, auth)
NodeCall->>TokenManager: extractDomain(targetUrl)
TokenManager-->>NodeCall: domain
NodeCall->>TokenManager: createToken(owner, domain)
TokenManager-->>NodeCall: tokenId (PENDING)
NodeCall->>ProxyManager: requestProxy(targetUrl)
ProxyManager->>ProxyManager: extractDomainAndPort(targetUrl)
ProxyManager->>PortAllocator: allocatePort()
PortAllocator-->>ProxyManager: port
ProxyManager->>ProxyManager: ensureWstcp()
ProxyManager->>Wstcp: spawn wstcp --listen <port> → targetDomain
Wstcp-->>ProxyManager: proxyId, websocketUrl
ProxyManager-->>NodeCall: { websocketProxyUrl, proxyId, expiresIn }
NodeCall->>TokenManager: consumeRetry(tokenId, proxyId)
TokenManager-->>NodeCall: token (ACTIVE, retriesLeft--)
NodeCall-->>Client: { tokenId, websocketProxyUrl, expiresIn, proxyId }
sequenceDiagram
participant Client
participant NativeHandler as NodeCall
participant TokenManager
participant GCRRoutines
participant DB
Client->>NativeHandler: tlsn_request(targetUrl)
NativeHandler->>TokenManager: extractDomain(targetUrl)
NativeHandler->>TokenManager: createToken(owner, domain)
TokenManager-->>NativeHandler: tokenId
NativeHandler->>NativeHandler: burn fee (1 DEM)
Client->>NativeHandler: tlsn_store(tokenId, proof)
NativeHandler->>TokenManager: validateToken(tokenId, owner, targetUrl)
TokenManager-->>NativeHandler: valid
NativeHandler->>NativeHandler: compute storage fee
NativeHandler->>NativeHandler: burn storage fee
NativeHandler->>GCRRoutines: apply(tlsnotary edit with proof)
GCRRoutines->>DB: insert GCRTLSNotary entity
DB-->>GCRRoutines: OK
GCRRoutines-->>NativeHandler: success
NativeHandler->>TokenManager: markStored(tokenId)
TokenManager-->>NativeHandler: token (STORED)
NativeHandler-->>Client: { success, tokenId, txHash }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
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 |
Drop buffered data when an oversized message payload is detected to release memory holding attacker-controlled bytes and prevent repeated processing of the invalid frame. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace Math.random()-based UUID generation with cryptographically secure crypto.randomUUID() to prevent predictable proxy IDs that could be exploited by attackers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Dynamically determine WebSocket protocol (ws:// or wss://) based on the origin URL's protocol. This prevents mixed-content errors for clients connecting from HTTPS pages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add proxyServer property to store the debug proxy server instance. Close any previous instance before creating a new one, and properly close it during stop() to prevent resource leaks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
Clear line before rendering TLSNotary status to prevent stale characters from remaining when status changes or becomes disabled. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @src/features/tlsnotary/proxyManager.ts:
- Around line 107-120: getTLSNotaryState currently assigns the function
reference getSharedState instead of calling it; change the assignment to call
getSharedState() so sharedState is the returned object, then access/initialize
sharedState.tlsnotary as shown; also fix the other occurrence mentioned (line
199) that uses getSharedState without parentheses so it calls getSharedState()
there too; ensure existing uses like initPortPool(), proxies: new Map<string,
ProxyInfo>(), and any logging remain unchanged.
- Around line 126-139: The ensureWstcp function lacks a timeout and a pre-flight
check for cargo; update ensureWstcp to first verify cargo is available by
calling execAsync("which cargo") (similar to the existing "which wstcp" check)
and if missing log a clear message before attempting install, then call
execAsync("cargo install wstcp", { timeout: SPAWN_TIMEOUT_MS || 60000 }) to
guard against hanging installs and surface timeout errors; ensure the install
catch includes the full error info in the thrown Error and add a note to
INSTALL.md that either wstcp or cargo must be present prior to deployment.
In @src/libs/blockchain/gcr/gcr_routines/handleNativeOperations.ts:
- Around line 89-101: The tlsn_store validation uses break which silences
failures and returns an empty edits array; change these to throw explicit errors
so the transaction fails consistently like tlsn_request does: in the tlsn_store
handling within handleNativeOperations.ts replace the break paths after "if
(!token)", "if (token.owner !== tx.content.from)", and "if (token.status !==
TokenStatus.COMPLETED && token.status !== TokenStatus.ACTIVE)" with thrown Error
instances (include informative messages referencing tokenId, token.owner,
tx.content.from, and token.status) so callers receive a clear failure instead of
continuing with an empty edits result.
In @src/libs/blockchain/gcr/gcr_routines/hashGCR.ts:
- Line 84: The proofTimestamp field is converted with String(r.proofTimestamp)
which can call Date.toString() and produce timezone-dependent output; replace
that conversion with a deterministic ISO 8601 string (use
r.proofTimestamp.toISOString() if it's a Date, or coerce/construct a Date and
call .toISOString() otherwise) so it matches the deterministic format used for
createdAt and ensures consistent hashing in the hashGCR routine.
♻️ Duplicate comments (5)
src/utilities/tui/TUIManager.ts (1)
108-109: Backslash key accessibility concern persists.The CMD tab remains mapped to the backslash key
\, which was previously flagged as having accessibility issues on international keyboard layouts (e.g., requires AltGr on European layouts, different positions on UK vs US keyboards). This concern from the previous review still applies.Based on past review comments, consider using a more universally accessible key or making the keybinding configurable.
src/features/tlsnotary/ffi.ts (1)
213-246: Memory management is correct.The implementation properly stores strong references before calling
ptr():
- Line 226:
this._signingKey = this.config.signingKey- Line 237:
this._configBuffer = new Uint8Array(configBuffer)This ensures the garbage collector won't reclaim the buffers while native code holds pointers to them.
src/features/tlsnotary/TLSNotaryService.ts (3)
339-377: process.exit(1) calls prevent graceful shutdown.Lines 342, 351, and 373 contain
process.exit(1)calls that bypass cleanup handlers and prevent graceful shutdown. This matches the concern raised in previous reviews.Instead of calling
process.exit(1), throw errors and let the application entry point decide how to handle fatal conditions. This allows cleanup handlers (e.g., stopping servers, closing connections, releasing resources) to run before process termination.🔎 Recommended approach
Define a typed fatal error:
class TLSNotaryFatalError extends Error { constructor(message: string) { super(message) this.name = 'TLSNotaryFatalError' } }Replace process.exit calls with throws:
} else { const error = new Error("Signing key required for FFI mode") if (fatal) { log.error("[TLSNotary] FATAL: " + error.message) - process.exit(1) + throw new TLSNotaryFatalError(error.message) } throw error }Handle at application entry point:
try { await service.initialize() } catch (e) { if (e instanceof TLSNotaryFatalError) { process.exit(1) } throw e }
402-443: Docker connectivity check with process.exit issue.Line 439 contains another
process.exit(1)call with the same graceful shutdown concern.Line 411 correctly uses
AbortSignal.timeout(5000)to prevent the fetch from hanging indefinitely, which is good defensive programming.Apply the same error-throwing pattern as suggested for
initializeFFIMode.
454-496: FFI startup with process.exit issues.Lines 458 and 492 contain
process.exit(1)calls that should be replaced with error throwing for graceful shutdown.The debug logging (lines 466-467) helpfully documents that TLSNotary only accepts WebSocket connections via HTTP GET, which will help developers troubleshoot connection issues.
🧹 Nitpick comments (2)
src/features/tlsnotary/proxyManager.ts (1)
370-394: Consider adding periodic cleanup in addition to lazy cleanup.The current implementation only cleans up stale proxies when a new request arrives. If the system is idle for extended periods, stale proxies will remain running and consume resources.
Consider adding a periodic cleanup timer (e.g., every 5 minutes) in addition to the lazy cleanup, especially if the proxy manager will be long-lived:
// In getTLSNotaryState or a separate init function let cleanupInterval: NodeJS.Timeout | null = null export function startPeriodicCleanup(intervalMs = 300000) { // 5 minutes if (cleanupInterval) return cleanupInterval = setInterval(() => { cleanupStaleProxies() }, intervalMs) } export function stopPeriodicCleanup() { if (cleanupInterval) { clearInterval(cleanupInterval) cleanupInterval = null } }src/features/tlsnotary/TLSNotaryService.ts (1)
503-571: Debug proxy implementation is functional but could be more robust.The proxy serves its purpose for debugging, but consider these optional improvements:
- Lines 539, 544: No error handling if
write()fails (e.g., backpressure, closed socket)- Lines 548-556: Error handlers destroy both sockets immediately, which might drop in-flight data
- No handling of the 'drain' event for flow control
For a production-quality proxy, consider:
- Check
write()return values and handle backpressure withdrainevents- Use
end()instead ofdestroy()for graceful shutdown when possible- Add connection timeout handling
Given this is a debug-only feature (controlled by
TLSNOTARY_PROXY), the current implementation is acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
src/features/tlsnotary/TLSNotaryService.tssrc/features/tlsnotary/ffi.tssrc/features/tlsnotary/portAllocator.tssrc/features/tlsnotary/proxyManager.tssrc/libs/blockchain/gcr/gcr_routines/handleNativeOperations.tssrc/libs/blockchain/gcr/gcr_routines/hashGCR.tssrc/libs/omniprotocol/protocol/handlers/control.tssrc/libs/omniprotocol/transport/MessageFramer.tssrc/utilities/tui/TUIManager.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/libs/omniprotocol/protocol/handlers/control.ts
- src/features/tlsnotary/portAllocator.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/libs/blockchain/gcr/gcr_routines/hashGCR.ts (1)
src/libs/crypto/hashing.ts (1)
Hashing(15-26)
src/features/tlsnotary/proxyManager.ts (3)
src/features/tlsnotary/portAllocator.ts (5)
PortPoolState(28-32)initPortPool(38-44)releasePort(119-128)PORT_CONFIG(17-23)allocatePort(80-112)src/utilities/sharedState.ts (1)
getSharedState(338-340)src/utilities/tui/CategorizedLogger.ts (1)
log(345-376)
🔇 Additional comments (21)
src/libs/blockchain/gcr/gcr_routines/hashGCR.ts (3)
5-5: LGTM: Import added for TLSNotary integration.The import of
GCRTLSNotaryis correctly added to support the new TLSNotary proofs hashing functionality.
76-86: Excellent normalization approach for deterministic hashing.The explicit field mapping with fixed order ensures deterministic
JSON.stringifyoutput across all nodes, which is critical for consensus. This approach correctly avoids JavaScript object property ordering issues that could break GCR integrity verification.Note: Ensure the timestamp conversion issue (Line 84) is addressed for full determinism.
108-113: LGTM: TLSNotary hash correctly integrated into GCR.The TLSNotary table hash is properly included in the combined GCR integrity hash, following the same pattern as existing tables. The integration maintains the deterministic ordering required for consensus.
src/utilities/tui/TUIManager.ts (3)
27-32: LGTM! Well-structured TLSNotary status extension.The optional
tlsnotaryfield is properly typed and provides the necessary information for status display in the TUI header. The use of optional property syntax ensures backward compatibility.
523-529: LGTM! Input handling correctly maps to tab indices.The key bindings for TLSN (tab index 11) and CMD (tab index 12) are consistent with the TABS array definition. The logic is correct.
1083-1094: TLSN category is properly integrated into the logging system.Verification confirms that the
TLSNcategory is fully defined in theLogCategorytype, included in theALL_CATEGORIESarray, and properly mapped inTAG_TO_CATEGORYwith multiple tag variants (TLSNOTARY, TLSNotary, TLSN, NOTARY, ATTESTATION). The TLSNotary status rendering logic at lines 1083-1094 is correct and uses safe optional chaining.src/libs/omniprotocol/transport/MessageFramer.ts (3)
36-37: LGTM! Solid DoS mitigation.The 16 MB payload limit is a reasonable upper bound that prevents memory exhaustion attacks while accommodating legitimate large messages.
202-209: Excellent security hardening.The validation placement is optimal—it rejects oversized payloads before buffering or extraction. Clearing the buffer on detection is the correct choice: once a malicious header is seen, all buffered data is suspect and the connection should be terminated.
284-287: Good defensive check on the egress path.Validating payload size before encoding catches bugs early and maintains symmetry with the ingress validation, ensuring both directions respect the same size limits.
src/features/tlsnotary/proxyManager.ts (2)
288-358: Startup monitoring logic is well-structured.The Promise-based startup monitoring with timeout, panic detection, and success signal detection is thorough. The cleanup function properly removes listeners to prevent leaks.
416-514: Request proxy flow is well-designed.The implementation follows a logical flow: validate inputs → cleanup stale proxies → reuse existing → spawn new with retries → proper error handling. The retry logic with port release on failure is correct.
src/features/tlsnotary/ffi.ts (3)
166-207: GC hazard successfully addressed.The addition of
_signingKeyand_configBufferfields (lines 173-174) properly addresses the previous review concern about garbage collection. These strong references ensure the buffers remain live while the native code holds pointers to them.The constructor also includes good defensive validation of the signing key length.
294-372: Attestation verification is well-implemented.Key strengths:
- Line 303: Empty buffer check prevents bun:ffi errors with zero-length buffers
- Try/finally block ensures native memory is freed even if parsing fails (line 370)
- Proper null pointer checks for optional C strings
447-465: Resource cleanup follows correct order.The destroy method properly sequences cleanup:
- Stop server if running (lines 450-454)
- Destroy native handle (line 457)
- Clear buffer references (lines 461-462)
- Reset state flags
This ensures native resources are released before the GC can reclaim the buffers.
src/features/tlsnotary/TLSNotaryService.ts (2)
81-120: Signing key resolution is well-designed.The priority-based resolution (ENV → file → auto-generate) provides good flexibility for different deployment scenarios. Line 113 correctly sets restrictive file permissions (
0o600) for the auto-generated key file, protecting the sensitive cryptographic material.
577-635: Shutdown logic is properly structured.The lifecycle management correctly handles both modes:
- Docker mode: Marks service as stopped without controlling the external container (lines 584-589, 622-625)
- FFI mode: Stops server, closes proxy if active, and destroys FFI instance (lines 593-609, 628-632)
The cleanup order is appropriate: stop server → close proxy → destroy FFI → clear references.
src/libs/blockchain/gcr/gcr_routines/handleNativeOperations.ts (5)
6-11: LGTM - TLSNotary imports and pricing constants are well-defined.The pricing constants are clearly documented. Consider making these configurable via environment variables in the future if pricing needs to change without code deployment.
26-51: LGTM -sendcase properly refactored with block scoping.The block scoping with
constdeclarations addresses the previous review feedback correctly.
52-80: LGTM -tlsn_requestcase addresses previous review concerns.The refactoring correctly:
- Uses block scoping with
constdeclarations.- Throws on invalid URL, preventing fee burn for invalid requests.
- Defers token creation to
processNativeSideEffects()to avoid duplicates.This addresses the atomicity concern from the previous review.
150-155: LGTM - Default case correctly handles unknown operations.The previous exhaustive check issue has been properly addressed. The cast to
INativePayloadis appropriate since TypeScript doesn't narrow the type through partial switch coverage.
130-139: The flagged code appears to be incomplete/unused and not part of the active codebase flow.The
HandleNativeOperationsclass in this file is never imported or called anywhere in the codebase. Native operations are actually processed through themanageNativemodule andHandleGCR.apply()method in the consensus flow (PoRBFT.ts→applyGCREditsFromMergedMempool). The file shows a "TODO Implement this" comment and appears to be work-in-progress code from a refactoring effort rather than production code.While the Date.now() concern would be valid if this code were executed, it does not apply to the current active system. Remove or properly integrate this file if it's intended for future use, but the current concern does not represent a live issue.
Likely an incorrect or invalid review comment.
Replace silent break statements with throw new Error() for token validation failures to ensure deterministic transaction rejection across all nodes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…precision loss - Add TypeORM transformer to handle bigint<->string conversion - Convert timestamp to string when saving to entity - Prevents JavaScript number precision loss for large timestamps 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Derive ws/wss scheme from node's exposedUrl protocol instead of hardcoding wss://, preventing mixed-content errors when node is accessed via HTTP. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace hardcoded tab indices with TABS.findIndex() for TLSN and CMD tabs to make navigation more robust and maintainable. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Wrap proxyServer.close() and proxyServer.listen() in Promises with error event handlers to ensure proper async execution and prevent unhandled exceptions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Wrap tlsn_notary_stop_server call in try/finally to ensure serverRunning is always set to false even if the FFI call throws. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add SPAWN_TIMEOUT_MS timeout to port availability check to prevent indefinite hangs. Also wrap error handler server.close in try/finally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/libs/blockchain/gcr/gcr_routines/GCRTLSNotaryRoutines.ts (4)
46-48: Consider including error details in log messages.The error handling logs the error object directly. Consider extracting
error.messageanderror.stackfor more structured logging.🔎 Suggested improvement
} catch (error) { - log.error(`[TLSNotary] Failed to rollback proof: ${error}`) + const errorMessage = error instanceof Error ? error.message : String(error) + log.error(`[TLSNotary] Failed to rollback proof for token ${tlsnEdit.data.tokenId}: ${errorMessage}`, error instanceof Error ? error.stack : undefined) return { success: false, message: "Failed to rollback TLSNotary proof" } }
83-85: Consider including error details in log messages.Similar to the rollback error handling, consider extracting structured error information.
🔎 Suggested improvement
} catch (error) { - log.error(`[TLSNotary] Failed to store proof: ${error}`) + const errorMessage = error instanceof Error ? error.message : String(error) + log.error(`[TLSNotary] Failed to store proof for token ${tlsnEdit.data.tokenId}: ${errorMessage}`, error instanceof Error ? error.stack : undefined) return { success: false, message: "Failed to store TLSNotary proof" } }
112-129: Add pagination to prevent memory issues with large result sets.Both
getProofsByOwnerandgetProofsByDomaincould return unbounded result sets if a single owner or domain accumulates many proofs over time. This could lead to memory exhaustion and poor performance.Consider adding pagination parameters (e.g.,
limitandoffset, or cursor-based pagination).🔎 Suggested pagination implementation
/** * Get all proofs for an owner * @param owner - The account address * @param gcrTLSNotaryRepository - TypeORM repository + * @param options - Optional pagination { limit, offset } */ static async getProofsByOwner( owner: string, gcrTLSNotaryRepository: Repository<GCRTLSNotary>, + options?: { limit?: number; offset?: number }, ): Promise<GCRTLSNotary[]> { - return gcrTLSNotaryRepository.findBy({ owner }) + const query = gcrTLSNotaryRepository + .createQueryBuilder("proof") + .where("proof.owner = :owner", { owner }) + + if (options?.limit) { + query.take(options.limit) + } + if (options?.offset) { + query.skip(options.offset) + } + + return query.getMany() } /** * Get all proofs for a domain * @param domain - The domain to look up * @param gcrTLSNotaryRepository - TypeORM repository + * @param options - Optional pagination { limit, offset } */ static async getProofsByDomain( domain: string, gcrTLSNotaryRepository: Repository<GCRTLSNotary>, + options?: { limit?: number; offset?: number }, ): Promise<GCRTLSNotary[]> { - return gcrTLSNotaryRepository.findBy({ domain }) + const query = gcrTLSNotaryRepository + .createQueryBuilder("proof") + .where("proof.domain = :domain", { domain }) + + if (options?.limit) { + query.take(options.limit) + } + if (options?.offset) { + query.skip(options.offset) + } + + return query.getMany() }
57-87: The PRIMARY KEY constraint ontokenIdalready prevents duplicate entries.The
GCRTLSNotaryentity hastokenIddeclared as@PrimaryColumn, which enforces uniqueness at the database level. This mitigates the actual risk of duplicate entries being stored.However, the application-level duplicate check (lines 57-64) is still vulnerable to a time-of-check-to-time-of-use race condition. Two concurrent requests could both pass the check and attempt to save. When the second request reaches the database, it will fail with a constraint violation error, which is caught and returned as a generic "Failed to store TLSNotary proof" message.
Consider either removing the redundant application-level check or using atomic database operations (e.g., upsert) if you want to avoid the race condition at the application level as well.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
package.jsonsrc/features/tlsnotary/TLSNotaryService.tssrc/features/tlsnotary/ffi.tssrc/features/tlsnotary/portAllocator.tssrc/libs/blockchain/gcr/gcr_routines/GCRTLSNotaryRoutines.tssrc/libs/blockchain/gcr/gcr_routines/handleNativeOperations.tssrc/libs/network/manageNodeCall.tssrc/model/entities/GCRv2/GCR_TLSNotary.tssrc/utilities/tui/TUIManager.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utilities/tui/TUIManager.ts
- src/model/entities/GCRv2/GCR_TLSNotary.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/libs/blockchain/gcr/gcr_routines/GCRTLSNotaryRoutines.ts (1)
src/libs/blockchain/gcr/handleGCR.ts (1)
GCRResult(78-82)
src/libs/network/manageNodeCall.ts (5)
src/features/tlsnotary/tokenManager.ts (5)
validateToken(158-205)consumeRetry(214-233)getToken(281-284)getTokenByTxHash(292-300)getTokenStats(327-349)src/features/tlsnotary/proxyManager.ts (1)
requestProxy(416-514)src/features/tlsnotary/TLSNotaryService.ts (1)
getTLSNotaryService(817-822)src/features/tlsnotary/index.ts (1)
getTLSNotaryService(63-63)src/utilities/sharedState.ts (1)
getSharedState(338-340)
src/features/tlsnotary/portAllocator.ts (1)
src/utilities/tui/CategorizedLogger.ts (1)
log(345-376)
src/libs/blockchain/gcr/gcr_routines/handleNativeOperations.ts (1)
src/features/tlsnotary/tokenManager.ts (3)
extractDomain(98-105)getToken(281-284)markStored(261-273)
src/features/tlsnotary/TLSNotaryService.ts (1)
src/features/tlsnotary/ffi.ts (4)
NotaryHealthStatus(51-62)TLSNotaryFFI(166-483)NotaryConfig(21-28)VerificationResult(33-46)
🔇 Additional comments (27)
package.json (2)
93-93: Critical security upgrade: fixes three CVEs in earlier versions.The bump to
1.3.3addresses three significant vulnerabilities in versions <1.3.2:
- CVE-2025-12816: ASN.1 validator bypass (signature/certificate verification)
- CVE-2025-66030: ASN.1 OID integer truncation bypass
- CVE-2025-66031: Uncontrolled recursion DoS
Version
1.3.3has no reported security advisories and is safe to deploy.
62-62: Verify SDK upgrade works with the breaking changes in 2.7.10.This bump crosses two minor versions with confirmed breaking changes: DemosWebAuth decoupling, updated method signatures for transaction signing and payload preparation, and API surface changes to
demos.connectand related helpers. While the specific methods likeDemosTransactions.signandWallet.transferdon't appear in the visible codebase, the extensive SDK usage across the project means the upgrade must be tested to ensure compatibility.Confirm that existing tests pass with the new SDK version before merging.
src/features/tlsnotary/portAllocator.ts (4)
1-23: LGTM! Module structure and configuration are well-defined.The module-level
netimport correctly addresses the previous review feedback. Constants are clearly defined with appropriate timeouts and limits.
25-44: LGTM! Pool initialization is straightforward.The interface and initialization function are clean and correctly set up the pool state.
51-85: LGTM! Port availability check is robust with timeout protection.The implementation correctly addresses previous review feedback:
- Module-level
netimportsettledflag prevents double-resolution- Timeout ensures the promise always resolves within 5 seconds
- Error and timeout handlers use try/finally to guarantee
finish()is calledThe close callback at line 80 doesn't explicitly handle errors, but the timeout mechanism provides adequate protection against hangs.
93-164: LGTM! Port allocation, release, and statistics are correctly implemented.The allocation strategy (recycled-first, then sequential) is sound. The
includescheck on line 136 is O(n) but acceptable given the small size of the recycled pool in typical usage.src/libs/network/manageNodeCall.ts (4)
457-534: LGTM! Token-based proxy request is well-validated.The endpoint correctly:
- Validates all required parameters (tokenId, owner, targetUrl)
- Enforces HTTPS for TLS attestation
- Validates token ownership and domain lock
- Avoids consuming retries on internal errors
- Links proxy to token only on success
Dynamic imports prevent circular dependencies.
536-595: LGTM! Service discovery endpoint correctly derives WebSocket scheme.The endpoint properly:
- Checks service availability (503 if not running)
- Derives WebSocket scheme from
exposedUrlto avoid mixed-content issues- Provides fallback defaults
- Returns notary and proxy URLs for SDK auto-configuration
597-646: LGTM! Token lookup supports both tokenId and txHash.The endpoint correctly:
- Supports flexible lookup by either identifier
- Validates at least one parameter is provided
- Returns appropriate token fields
- Uses correct status codes
648-663: LGTM! Token statistics endpoint is straightforward.Simple monitoring endpoint with proper error handling.
src/libs/blockchain/gcr/gcr_routines/handleNativeOperations.ts (5)
1-11: LGTM! Imports and pricing constants are well-defined.TLSNotary pricing is simple (1 DEM = 1 unit) with clear per-KB storage fees.
26-51: LGTM! Send case now uses block scoping.The case is properly wrapped in a block with
constdeclarations, addressing previous review feedback about variable leakage.
52-80: LGTM! URL validation now throws for deterministic failure.The case correctly:
- Uses block scoping with
constdeclarations- Throws on invalid URL (addressing previous review feedback)
- Burns the request fee
- Documents that token creation is a native side-effect
82-148: LGTM! Token validation now uses throw for consistency.The case correctly addresses previous review feedback:
- Block scoping with
constdeclarations- Throws on validation failures (token not found, owner mismatch, wrong status)
- Proper proof size calculation handling both string and Uint8Array
- Storage fee formula is clear: base + (KB × per-KB fee)
The proof size overflow concern from past review is unlikely with current constants (1 DEM/KB would require unrealistic proof sizes).
150-155: LGTM! Default case now correctly handles unknown operations.The exhaustive check issue from previous review has been resolved. The safe cast with explanatory comment is appropriate.
src/features/tlsnotary/ffi.ts (5)
1-89: LGTM! FFI interfaces and library path resolution are well-structured.The platform-specific library path resolution correctly handles macOS, Windows, and Linux with appropriate file extensions.
94-175: LGTM! FFI symbols are properly defined.The class fields
_signingKeyand_configBuffer(lines 173-174) correctly address the previous review concern about GC hazards by maintaining strong references to buffers passed to native code.
181-246: LGTM! Constructor correctly prevents GC hazards.The implementation properly addresses the previous review concern:
_signingKeyis assigned at line 226 beforeptr()call (line 227)_configBufferis assigned at line 237 beforeptr()call (line 238)This ensures buffers remain live while native code uses the pointers.
253-372: LGTM! Server lifecycle and verification are properly managed.Key correctness aspects:
- State checks prevent double-start/stop
- Empty attestation check (line 303) prevents FFI issues
- Memory cleanup in
finallyblock (line 370) ensures no leaks
380-483: LGTM! Resource cleanup is complete.The
destroymethod properly:
- Stops server in try-finally (lines 450-456)
- Destroys native handle
- Clears buffer references (lines 464-465) after native cleanup
This completes the GC protection lifecycle.
src/features/tlsnotary/TLSNotaryService.ts (7)
1-146: LGTM! Service configuration and key resolution are well-designed.The signing key resolution priority (ENV → file → auto-generate) is clear and well-documented. File permissions (0o600, line 113) are appropriately restrictive.
169-197: LGTM! Environment configuration correctly handles mode-specific requirements.Docker mode is the default (line 177), and signing key validation only applies to FFI mode (lines 181-187), which is appropriate.
229-296: LGTM! Service initialization correctly delegates to mode-specific handlers.The dual-mode architecture is clean, with appropriate delegation based on the operational mode.
323-377: Verify fatal mode behavior aligns with operational requirements.The
process.exit(1)calls (lines 342, 351, 372) whenTLSNOTARY_FATAL=trueprevent graceful shutdown, as noted in previous review feedback. While this may be intentional for debugging, consider whether:
- Fatal mode is documented as a debug-only feature
- Production deployments should never enable
TLSNOTARY_FATAL=true- Throwing typed errors (allowing the caller to decide on exit) would provide better separation of concerns
When
fatal=false, errors are thrown normally, which is appropriate.Alternative approach if graceful shutdown is required
class TLSNotaryFatalError extends Error { constructor(message: string) { super(message) this.name = 'TLSNotaryFatalError' } } // In FFI init: if (!signingKeyBytes) { const error = new TLSNotaryFatalError("Signing key required for FFI mode") log.error("[TLSNotary] FATAL: " + error.message) if (fatal) { throw error // Let caller handle process.exit } throw error } // At application entry point: try { await service.initialize() } catch (e) { if (e instanceof TLSNotaryFatalError && isTLSNotaryFatal()) { process.exit(1) } // ... handle other errors }
383-578: LGTM! Dual-mode startup is well-implemented.Docker mode correctly verifies container accessibility via HTTP
/infoendpoint. FFI mode includes an optional debug proxy (lines 503-578) that intercepts and logs data before forwarding—useful for troubleshooting but should be disabled in production.The
process.exit(1)calls follow the same fatal-mode pattern discussed in the previous segment.
584-725: LGTM! Lifecycle management and mode-specific accessors are correct.The service properly handles:
- Docker mode leaving container lifecycle to external orchestration
- FFI mode managing native resources
- Verification unsupported in Docker mode (client-side verification used instead, line 659)
- Public key retrieval from cached Docker value or FFI
730-846: LGTM! Status reporting and singleton management are straightforward.The singleton pattern is correctly implemented with lazy initialization and proper cleanup.
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
|
- INVALID_URL returns 400 (Bad Request) - PORT_EXHAUSTED returns 503 (Service Unavailable) - WSTCP_NOT_AVAILABLE/PROXY_SPAWN_FAILED return 500 (Internal Error) This improves API ergonomics by distinguishing client errors from server errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
PR Type
Enhancement, Tests
Description
TLSNotary Integration: Comprehensive support for TLS attestation with dual-mode execution (FFI and Docker), including service lifecycle management, proxy management with port pooling, and token-based access control
OmniProtocol Implementation: Complete binary protocol stack with TCP transport, message framing, connection pooling, TLS security, and rate limiting for peer-to-peer communication
Protocol Handlers: Full handler implementations for control, transaction, sync, consensus, and GCR operations with binary serialization/deserialization
Handler Registry: Centralized opcode registry supporting 60+ handlers with validation and HTTP fallback
Test Coverage: Comprehensive test suites for OmniProtocol handlers, transaction operations, consensus, and GCR functionality with fixture-based testing
Blockchain Integration: TLSNotary support in GCR with native side-effects processing for
tlsn_requestoperations and token creationLogging Improvements: Enhanced logger with async I/O, persistent stream caching, new categories (TLSN, CMD), and structured logging throughout codebase
Adapter Pattern: Base adapter class for OmniProtocol client integration with migration mode support (HTTP_ONLY, OMNI_PREFERRED, OMNI_ONLY)
RPC Endpoints: New TLSNotary RPC endpoints for proxy requests, token management, and service information
Diagram Walkthrough
File Walkthrough
4 files
handlers.test.ts
OmniProtocol handlers comprehensive test suitetests/omniprotocol/handlers.test.ts
lines of test cases
sync, block sync, and GCR operations
for real-world scenarios
serialization
consensus.test.ts
Consensus protocol round-trip encoding teststests/omniprotocol/consensus.test.ts
(proposeBlockHash, setValidatorPhase, greenlight)
serialization
transaction.test.ts
Transaction opcode serialization test suitetests/omniprotocol/transaction.test.ts
opcodes (0x10, 0x11, 0x12, 0x15, 0x16)
broadcast requests/responses
nested structures
validation errors
gcr.test.ts
GCR operations and JSON envelope serialization teststests/omniprotocol/gcr.test.ts
JSON envelope serialization
validateReferral, getAccountByIdentity, and getTopAccounts operations
error responses
data structures
27 files
TLSNotaryService.ts
TLSNotary service with FFI and Docker mode supportsrc/features/tlsnotary/TLSNotaryService.ts
Docker)
with environment-based configuration
verification
file > auto-generate
proxyManager.ts
TLSNotary WebSocket proxy manager with port poolingsrc/features/tlsnotary/proxyManager.ts
lazy cleanup
of idle proxies
proxy cleanup
and localhost
gcr.ts
GCR protocol handlers for identity and credential operationssrc/libs/omniprotocol/protocol/handlers/gcr.ts
OmniProtocol opcodes
referral validation, and account lookup
and encodes responses
messages
ffi.ts
TLSNotary Rust FFI bindings with bun:ffi integrationsrc/features/tlsnotary/ffi.ts
platform-specific library loading
TLSNotaryFFIclass for server lifecycle (start/stop),attestation verification, and public key retrieval
conversion
initialization and operations
control.ts
Control protocol message serialization implementationsrc/libs/omniprotocol/serialization/control.ts
node call, and related control protocol messages
node call payloads
encoders/decoders for binary protocol handling
metadata
PeerConnection.ts
Peer TCP connection management with state machinesrc/libs/omniprotocol/transport/PeerConnection.ts
machine (UNINITIALIZED → CONNECTING → AUTHENTICATING → READY →
IDLE_PENDING → CLOSING → CLOSED)
handling, and in-flight request tracking
Ed25519 signing
CategorizedLogger.ts
Logger enhancements with async I/O and new categoriessrc/utilities/tui/CategorizedLogger.ts
TLSNfor TLSNotary operations andCMDfor command execution
fs.promisesfor better performance
WriteStreamcaching for log files instead ofrepeated
appendFilecallssetImmediatescheduling forimproved performance
sync.ts
Data synchronization protocol message serializationsrc/libs/omniprotocol/serialization/sync.ts
transaction synchronization protocol messages
block metadata
and block sync operations
block information
ConnectionPool.ts
Connection pool for peer node managementsrc/libs/omniprotocol/transport/ConnectionPool.ts
nodes
and lazy connection creation
connection reuse
unauthenticated requests with connection lifecycle management
consensus.ts
Consensus protocol message serializationsrc/libs/omniprotocol/serialization/consensus.ts
(block hash proposals, validator phases, greenlight)
greenlight, and related consensus operations
validation data and signatures
registry.ts
OmniProtocol opcode handler registrysrc/libs/omniprotocol/protocol/registry.ts
across control, transaction, sync, consensus, and GCR operations
HandlerDescriptorinterface with opcode, name, authrequirements, and handler function
registration
getHandler()function for opcode lookup with fallback HTTPhandler support
handleGCR.ts
TLSNotary integration and native transaction side-effectssrc/libs/blockchain/gcr/handleGCR.ts
GCRTLSNotaryRoutinesfor attestationproof storage
processNativeSideEffects()method to handle token creationfor
tlsn_requestoperations during mempool entrysmartContract,storageProgram,escrow, andtlsnotaryconsole.log/console.errorcalls with structured logging vialogutilityvalidation
tokenManager.ts
TLSNotary attestation token manager implementationsrc/features/tlsnotary/tokenManager.ts
attestation access
retries
completed/stored)
index.ts
OmniProtocol and TLSNotary service integration in main entry pointsrc/index.ts
configuration
graceful shutdown
log-levelcommand-line parameter for dynamic log level adjustmentTLSServer.ts
TLS-enabled OmniProtocol server with rate limitingsrc/libs/omniprotocol/server/TLSServer.ts
and fingerprint verification
MessageFramer.ts
OmniProtocol message framing and serialization layersrc/libs/omniprotocol/transport/MessageFramer.ts
header and checksum validation
manageNodeCall.ts
TLSNotary RPC endpoints and logging improvementssrc/libs/network/manageNodeCall.ts
requestTLSNproxy,tlsnotary.getInfo,tlsnotary.getToken,tlsnotary.getTokenStatslogmoduleRateLimiter.ts
Rate limiting implementation with sliding window algorithmsrc/libs/omniprotocol/ratelimit/RateLimiter.ts
identity-based limits
blocking
consensus.ts
OmniProtocol consensus operation handlerssrc/libs/omniprotocol/protocol/handlers/consensus.ts
(proposeBlockHash, setValidatorPhase, greenlight, etc.)
encoding/decoding
queries
InboundConnection.ts
Inbound connection handler for OmniProtocol serversrc/libs/omniprotocol/server/InboundConnection.ts
dispatching
hello_peer)
sync.ts
OmniProtocol blockchain synchronization handlerssrc/libs/omniprotocol/protocol/handlers/sync.ts
operations
queries
consensusAdapter.ts
OmniProtocol consensus adapter with HTTP fallbacksrc/libs/omniprotocol/integration/consensusAdapter.ts
binary-efficient communication
methods
LegacyLoggerAdapter.ts
Logger adapter improvements for flexible type handlingsrc/utilities/tui/LegacyLoggerAdapter.ts
stringify()helper function to handle any value type in logging(matches console.log behavior)
unknowntype instead ofstringwarn()alias forwarning()method for compatibilitytransaction.ts
OmniProtocol transaction operation handlerssrc/libs/omniprotocol/protocol/handlers/transaction.ts
(execute, broadcast, confirm)
encoding
BaseAdapter.ts
Base adapter class for OmniProtocol client integrationsrc/libs/omniprotocol/integration/BaseAdapter.ts
manageConsensusRoutines.ts
Consensus routines logging improvementssrc/libs/network/manageConsensusRoutines.ts
logmoduleindex.ts
TLS module index and exportssrc/libs/omniprotocol/tls/index.ts
1 files
getTransactions.ts
Replace console.log with structured loggingsrc/libs/network/routines/nodecalls/getTransactions.ts
console.logwithlog.debugfor consistent logging101 files
Summary by CodeRabbit
New Features
Bug Fixes / Security
Chores
✏️ Tip: You can customize this high-level summary in your review settings.