feat: add logs websocket subscription for live smart-contract events - #98
feat: add logs websocket subscription for live smart-contract events#98Test0rMaik wants to merge 11 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds the ChangesLogs event flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SaveBlock
participant LogsAndEventsProcessor
participant EventQueue
participant WebSocketHub
participant Client
SaveBlock->>LogsAndEventsProcessor: PrepareLogsForDB(transaction logs)
LogsAndEventsProcessor-->>SaveBlock: prepared log entries
SaveBlock->>EventQueue: enqueue LOGS event
EventQueue->>WebSocketHub: deliver LOGS event
WebSocketHub->>Client: send matching address-scoped log
🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@network/api/websocket/routes_test.go`:
- Line 226: Replace the hardcoded time.Sleep in the websocket subscription test
with deterministic synchronization using a channel or sync primitive that
signals once the handler has registered klv1contract. Wait for that readiness
signal before sending or dequeuing the LOGS event, preserving the existing test
assertions and timeout behavior.
In `@websocket/websocket_test.go`:
- Around line 837-847: The LOGS tests use timing-based synchronization and must
switch to deterministic channel or sync-primitive coordination. In
websocket/websocket_test.go lines 837-847, update
TestStartServer_LogsEvent_NoSubscribers to wait for an ordered sentinel event or
completion hook proving the queued LOGS event was consumed before teardown. In
network/api/websocket/routes_test.go lines 205-241, wait for subscription
registration via a deterministic readiness signal before queuing the LOGS event;
remove hardcoded sleeps at both sites.
- Line 531: Update the test around hub.HandleClientInsertion to capture and
assert its returned error, ensuring subscription limit or validation failures
are reported at the insertion call instead of being discarded.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2acfbb19-df32-4645-8aab-ed7018ccb753
📒 Files selected for processing (7)
indexer/events.goindexer/eventsProcessor.goindexer/eventsProcessor_test.gonetwork/api/websocket/routes_test.gowebsocket/README.mdwebsocket/websocket.gowebsocket/websocket_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
network/api/websocket/routes_test.goindexer/events.goindexer/eventsProcessor_test.goindexer/eventsProcessor.gowebsocket/websocket_test.gowebsocket/websocket.go
network/**
⚙️ CodeRabbit configuration file
network/**: Peer-to-peer networking layer. - Check for proper input validation on all received messages - Verify rate limiting and DoS protection mechanisms - Ensure connection handling is goroutine-safe - Look for potential message amplification attacks - Verify TLS/authentication on peer connections
Files:
network/api/websocket/routes_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
network/api/websocket/routes_test.goindexer/eventsProcessor_test.gowebsocket/websocket_test.go
🧠 Learnings (2)
📚 Learning: 2026-04-21T20:12:22.959Z
Learnt from: phcarneirobc
Repo: klever-io/klever-go PR: 38
File: indexer/eventsProcessor.go:188-211
Timestamp: 2026-04-21T20:12:22.959Z
Learning: In Go structs that are JSON-marshaled, if a field is a `bool` and has the `json:"...,omitempty"` tag, then leaving that field at its zero value (`false`) is functionally equivalent (in the resulting JSON) to explicitly setting `Foundation: false`. Reviewers should not flag struct literals that omit such `bool` fields as an inconsistency; they will serialize identically because `omitempty` suppresses `false` values.
Applied to files:
network/api/websocket/routes_test.goindexer/events.goindexer/eventsProcessor_test.goindexer/eventsProcessor.gowebsocket/websocket_test.gowebsocket/websocket.go
📚 Learning: 2026-05-23T22:52:58.065Z
Learnt from: fbsobreira
Repo: klever-io/klever-go PR: 65
File: data/blockchain/blockchain.go:170-172
Timestamp: 2026-05-23T22:52:58.065Z
Learning: In Go, the pattern `append([]byte(nil), src...)` should be treated as preserving nil identity when `src` is a nil `[]byte`: spreading a nil slice contributes zero variadic arguments, so `append` performs no allocation and returns the original nil destination slice unchanged (i.e., result is nil, not an empty non-nil slice). Do not flag this as an incorrect empty-slice conversion; it intentionally maintains `nil`.
Applied to files:
network/api/websocket/routes_test.goindexer/events.goindexer/eventsProcessor_test.goindexer/eventsProcessor.gowebsocket/websocket_test.gowebsocket/websocket.go
🔇 Additional comments (7)
indexer/events.go (1)
27-27: LGTM!Also applies to: 64-65, 81-82
indexer/eventsProcessor.go (1)
13-24: LGTM!Also applies to: 33-43, 58-60, 106-106, 168-186
indexer/eventsProcessor_test.go (1)
272-272: LGTM!Also applies to: 442-493, 495-522, 523-542, 543-560
websocket/websocket.go (1)
33-33: LGTM!Also applies to: 150-151, 173-193, 333-348, 373-415, 595-643
websocket/websocket_test.go (1)
502-510: LGTM!Also applies to: 537-543, 583-607, 816-835, 849-885
network/api/websocket/routes_test.go (1)
15-15: LGTM!Also applies to: 103-103
websocket/README.md (1)
16-16: LGTM!Also applies to: 41-52, 71-96, 227-228, 267-269
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@network/api/websocket/routes_test.go`:
- Line 238: Update TestSubscribeTopics_LogsEventDelivery and the other deadline
setup calls to assert SetReadDeadline succeeds with require.NoError before each
corresponding ReadJSON. Replace ignored deadline-setting results while
preserving the existing timeout durations and read sequence.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a05116c3-e481-4fa7-a2cd-5b010e23f9a7
📒 Files selected for processing (2)
network/api/websocket/routes_test.gowebsocket/websocket_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
network/api/websocket/routes_test.gowebsocket/websocket_test.go
network/**
⚙️ CodeRabbit configuration file
network/**: Peer-to-peer networking layer. - Check for proper input validation on all received messages - Verify rate limiting and DoS protection mechanisms - Ensure connection handling is goroutine-safe - Look for potential message amplification attacks - Verify TLS/authentication on peer connections
Files:
network/api/websocket/routes_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
network/api/websocket/routes_test.gowebsocket/websocket_test.go
🧠 Learnings (2)
📚 Learning: 2026-04-21T20:12:22.959Z
Learnt from: phcarneirobc
Repo: klever-io/klever-go PR: 38
File: indexer/eventsProcessor.go:188-211
Timestamp: 2026-04-21T20:12:22.959Z
Learning: In Go structs that are JSON-marshaled, if a field is a `bool` and has the `json:"...,omitempty"` tag, then leaving that field at its zero value (`false`) is functionally equivalent (in the resulting JSON) to explicitly setting `Foundation: false`. Reviewers should not flag struct literals that omit such `bool` fields as an inconsistency; they will serialize identically because `omitempty` suppresses `false` values.
Applied to files:
network/api/websocket/routes_test.gowebsocket/websocket_test.go
📚 Learning: 2026-05-23T22:52:58.065Z
Learnt from: fbsobreira
Repo: klever-io/klever-go PR: 65
File: data/blockchain/blockchain.go:170-172
Timestamp: 2026-05-23T22:52:58.065Z
Learning: In Go, the pattern `append([]byte(nil), src...)` should be treated as preserving nil identity when `src` is a nil `[]byte`: spreading a nil slice contributes zero variadic arguments, so `append` performs no allocation and returns the original nil destination slice unchanged (i.e., result is nil, not an empty non-nil slice). Do not flag this as an incorrect empty-slice conversion; it intentionally maintains `nil`.
Applied to files:
network/api/websocket/routes_test.gowebsocket/websocket_test.go
🔇 Additional comments (2)
network/api/websocket/routes_test.go (1)
15-15: LGTM!Also applies to: 103-103, 205-237, 239-251, 253-259
websocket/websocket_test.go (1)
502-511: LGTM!Also applies to: 531-541, 543-544, 584-608, 817-836, 838-859, 861-897
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
network/api/websocket/routes_test.go (1)
245-257: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert the originating transaction hash.
The test creates a log without
IDand only checksTypeandAddress, so a regression that drops the transaction hash can pass. PopulateLogs.IDand assert the websocket envelope’s hash field afterReadJSON.Proposed test strengthening
Message: []*wsdata.Logs{ - {Address: "klv1contract", Events: []*wsdata.Event{{Identifier: "transfer"}}}, + {ID: "tx-hash", Address: "klv1contract", Events: []*wsdata.Event{{Identifier: "transfer"}}}, }, } @@ assert.Equal(t, indexer.LOGS, received.Type) assert.Equal(t, "klv1contract", received.Address) + assert.Equal(t, "tx-hash", received.Hash)As per path instructions, tests should cover the relevant delivery contract; the PR objective requires preserving the originating transaction hash.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@network/api/websocket/routes_test.go` around lines 245 - 257, Strengthen the websocket log delivery test around the queued indexer.Event by populating the Logs.ID with a known transaction hash, then after ReadJSON assert the received socket.Send hash field matches it, while preserving the existing Type and Address assertions.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@network/api/websocket/routes_test.go`:
- Around line 245-257: Strengthen the websocket log delivery test around the
queued indexer.Event by populating the Logs.ID with a known transaction hash,
then after ReadJSON assert the received socket.Send hash field matches it, while
preserving the existing Type and Address assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b3afebfe-0bfa-45f5-a713-0939102fd316
📒 Files selected for processing (1)
network/api/websocket/routes_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
network/api/websocket/routes_test.go
network/**
⚙️ CodeRabbit configuration file
network/**: Peer-to-peer networking layer. - Check for proper input validation on all received messages - Verify rate limiting and DoS protection mechanisms - Ensure connection handling is goroutine-safe - Look for potential message amplification attacks - Verify TLS/authentication on peer connections
Files:
network/api/websocket/routes_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
network/api/websocket/routes_test.go
🧠 Learnings (2)
📚 Learning: 2026-04-21T20:12:22.959Z
Learnt from: phcarneirobc
Repo: klever-io/klever-go PR: 38
File: indexer/eventsProcessor.go:188-211
Timestamp: 2026-04-21T20:12:22.959Z
Learning: In Go structs that are JSON-marshaled, if a field is a `bool` and has the `json:"...,omitempty"` tag, then leaving that field at its zero value (`false`) is functionally equivalent (in the resulting JSON) to explicitly setting `Foundation: false`. Reviewers should not flag struct literals that omit such `bool` fields as an inconsistency; they will serialize identically because `omitempty` suppresses `false` values.
Applied to files:
network/api/websocket/routes_test.go
📚 Learning: 2026-05-23T22:52:58.065Z
Learnt from: fbsobreira
Repo: klever-io/klever-go PR: 65
File: data/blockchain/blockchain.go:170-172
Timestamp: 2026-05-23T22:52:58.065Z
Learning: In Go, the pattern `append([]byte(nil), src...)` should be treated as preserving nil identity when `src` is a nil `[]byte`: spreading a nil slice contributes zero variadic arguments, so `append` performs no allocation and returns the original nil destination slice unchanged (i.e., result is nil, not an empty non-nil slice). Do not flag this as an incorrect empty-slice conversion; it intentionally maintains `nil`.
Applied to files:
network/api/websocket/routes_test.go
🔇 Additional comments (1)
network/api/websocket/routes_test.go (1)
15-15: LGTM!Also applies to: 103-103, 238-238, 252-252
974c6f6 to
4166230
Compare
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@network/api/websocket/routes_test.go`:
- Around line 205-209: Update TestSubscribeTopics_LogsEventDelivery to wait for
the hub.StartServer goroutine to exit after cancellation, using a
synchronization mechanism such as a done channel or wait group. Ensure cleanup
joins the goroutine before the test returns so it cannot consume events
published by subsequent tests.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 76585daf-a6e9-4ade-8911-2030a00a47d4
📒 Files selected for processing (7)
indexer/events.goindexer/eventsProcessor.goindexer/eventsProcessor_test.gonetwork/api/websocket/routes_test.gowebsocket/README.mdwebsocket/websocket.gowebsocket/websocket_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
indexer/eventsProcessor.goindexer/events.gonetwork/api/websocket/routes_test.goindexer/eventsProcessor_test.gowebsocket/websocket_test.gowebsocket/websocket.go
network/**
⚙️ CodeRabbit configuration file
network/**: Peer-to-peer networking layer. - Check for proper input validation on all received messages - Verify rate limiting and DoS protection mechanisms - Ensure connection handling is goroutine-safe - Look for potential message amplification attacks - Verify TLS/authentication on peer connections
Files:
network/api/websocket/routes_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
network/api/websocket/routes_test.goindexer/eventsProcessor_test.gowebsocket/websocket_test.go
🧠 Learnings (2)
📚 Learning: 2026-04-21T20:12:22.959Z
Learnt from: phcarneirobc
Repo: klever-io/klever-go PR: 38
File: indexer/eventsProcessor.go:188-211
Timestamp: 2026-04-21T20:12:22.959Z
Learning: In Go structs that are JSON-marshaled, if a field is a `bool` and has the `json:"...,omitempty"` tag, then leaving that field at its zero value (`false`) is functionally equivalent (in the resulting JSON) to explicitly setting `Foundation: false`. Reviewers should not flag struct literals that omit such `bool` fields as an inconsistency; they will serialize identically because `omitempty` suppresses `false` values.
Applied to files:
indexer/eventsProcessor.goindexer/events.gonetwork/api/websocket/routes_test.goindexer/eventsProcessor_test.gowebsocket/websocket_test.gowebsocket/websocket.go
📚 Learning: 2026-05-23T22:52:58.065Z
Learnt from: fbsobreira
Repo: klever-io/klever-go PR: 65
File: data/blockchain/blockchain.go:170-172
Timestamp: 2026-05-23T22:52:58.065Z
Learning: In Go, the pattern `append([]byte(nil), src...)` should be treated as preserving nil identity when `src` is a nil `[]byte`: spreading a nil slice contributes zero variadic arguments, so `append` performs no allocation and returns the original nil destination slice unchanged (i.e., result is nil, not an empty non-nil slice). Do not flag this as an incorrect empty-slice conversion; it intentionally maintains `nil`.
Applied to files:
indexer/eventsProcessor.goindexer/events.gonetwork/api/websocket/routes_test.goindexer/eventsProcessor_test.gowebsocket/websocket_test.gowebsocket/websocket.go
🔇 Additional comments (7)
indexer/events.go (1)
27-27: LGTM!Also applies to: 64-65, 81-82
indexer/eventsProcessor.go (1)
13-24: LGTM!Also applies to: 33-44, 58-60, 106-106, 168-186
indexer/eventsProcessor_test.go (1)
272-272: LGTM!Also applies to: 442-561
websocket/websocket.go (1)
33-33: LGTM!Also applies to: 150-151, 173-193, 333-344, 373-415, 595-636
websocket/websocket_test.go (1)
502-510: LGTM!Also applies to: 531-544, 584-608, 817-897
network/api/websocket/routes_test.go (1)
15-15: LGTM!Also applies to: 103-103, 219-258
websocket/README.md (1)
16-16: LGTM!Also applies to: 41-52, 71-96, 227-228, 267-269
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
network/api/websocket/routes_test.go (1)
252-265: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover transaction-hash propagation in the integration test.
The
wsdata.Logsfixture leavesIDempty, whilewebsocket/websocket.gousesentry.IDas the outgoing envelope hash. This test would still pass if that contract regressed. Populate a known transaction ID and assert the corresponding hash in the receivedsocket.Sendor payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@network/api/websocket/routes_test.go` around lines 252 - 265, Update the logs integration test fixture in the EventQueue send to include a known transaction ID, then assert that the received socket.Send hash or payload contains the same ID. Keep the existing event type and address assertions unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@network/api/websocket/routes_test.go`:
- Around line 252-265: Update the logs integration test fixture in the
EventQueue send to include a known transaction ID, then assert that the received
socket.Send hash or payload contains the same ID. Keep the existing event type
and address assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 62cdf59f-2ca3-4bad-b5a6-4262e3e88021
📒 Files selected for processing (1)
network/api/websocket/routes_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
network/api/websocket/routes_test.go
network/**
⚙️ CodeRabbit configuration file
network/**: Peer-to-peer networking layer. - Check for proper input validation on all received messages - Verify rate limiting and DoS protection mechanisms - Ensure connection handling is goroutine-safe - Look for potential message amplification attacks - Verify TLS/authentication on peer connections
Files:
network/api/websocket/routes_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
network/api/websocket/routes_test.go
🧠 Learnings (2)
📚 Learning: 2026-04-21T20:12:22.959Z
Learnt from: phcarneirobc
Repo: klever-io/klever-go PR: 38
File: indexer/eventsProcessor.go:188-211
Timestamp: 2026-04-21T20:12:22.959Z
Learning: In Go structs that are JSON-marshaled, if a field is a `bool` and has the `json:"...,omitempty"` tag, then leaving that field at its zero value (`false`) is functionally equivalent (in the resulting JSON) to explicitly setting `Foundation: false`. Reviewers should not flag struct literals that omit such `bool` fields as an inconsistency; they will serialize identically because `omitempty` suppresses `false` values.
Applied to files:
network/api/websocket/routes_test.go
📚 Learning: 2026-05-23T22:52:58.065Z
Learnt from: fbsobreira
Repo: klever-io/klever-go PR: 65
File: data/blockchain/blockchain.go:170-172
Timestamp: 2026-05-23T22:52:58.065Z
Learning: In Go, the pattern `append([]byte(nil), src...)` should be treated as preserving nil identity when `src` is a nil `[]byte`: spreading a nil slice contributes zero variadic arguments, so `append` performs no allocation and returns the original nil destination slice unchanged (i.e., result is nil, not an empty non-nil slice). Do not flag this as an incorrect empty-slice conversion; it intentionally maintains `nil`.
Applied to files:
network/api/websocket/routes_test.go
🔇 Additional comments (1)
network/api/websocket/routes_test.go (1)
15-15: LGTM!Also applies to: 103-103, 208-216
fbsobreira
left a comment
fbsobreira
left a comment
There was a problem hiding this comment.
Requesting changes for three blocking findings from the detailed inline comments already on this PR, plus one strongly recommended follow-up.
Must fix before merge
-
ES logs index template gap (logsProcessor.go:155) —
PrepareLogsForDBalso feeds the Elasticsearchlogs-*index, so the newstatus/resultCode/isSystemLogfields (and the now-populatedcontractId) reach production documents with no template entry. ES will dynamic-mapstatus/resultCodeas analyzed text, and that wrong mapping is permanently baked into the index — unfixable without a full reindex. Please declare the new fields in bothindexer/templates/noKibana/logs.goandindexer/templates/withKibana/logs.go, and flag thecontractId0→real-value change for downstream consumers. -
Unbounded per-log-entry mirror POSTs (websocket.go:219) — with
postConnectionURLconfigured,handleLogsEventspawns one goroutine + HTTP POST per log entry with no concurrency cap; SC-heavy blocks against a slow mirror pile up thousands of in-flight POSTs and exhaust file descriptors. Please batch the block's logs into a single POST or capasyncPostconcurrency. -
Data race on shared
*data.Transactionpointers (eventsProcessor.go:105) — the websocket hubjson.Marshals the same transaction pointers the ES worker mutates (ExtractDataFromLogssetstx.HasLogs/tx.HasOperations). Run the mutation before enqueueing to the hub, or hand the hub copies; as-is this fails under-raceand is undefined behavior.
Strongly recommended
- Log conversion cost on the block-commit path (eventsProcessor.go:185, duplicate-conversion note) —
PrepareLogsForDBruns synchronously on block commit for every block withUseEventQueueon, even with zero logs subscribers, and runs a second time inprepareAndIndexLogson indexer nodes. Gating the conversion on subscriber/mirror presence and sharing the convertedlogsDBviaPreparedBlockData(as this PR already does forTxsMap) fixes both.
The remaining inline comments (positional bools vs userOptions, LOGS-only subscriber pre-check, comment verbosity) are non-blocking polish.
c118726
a9e7667 to
c118726
Compare
| var txsMap map[string]*data.Transaction | ||
| if prepared != nil { | ||
| txsMap = prepared.TxsMap | ||
| if indexerEnabled { |
There was a problem hiding this comment.
[correctness — confirmed] New: gating ExtractDataFromLogs on indexerEnabled makes the websocket payload depend on whether Elasticsearch is enabled — including this PR's own new logs.status/resultCode
ExtractDataFromLogs is not a pure computation; it writes to the shared *data.Transaction objects: tx.HasLogs (logsevents/logsProcessor.go:61), tx.HasOperations (:111), and tx.Status (logsevents/informativeLogs.go:39-45). Both websocket consumers read those fields after this line:
dispatchTransactionEvents→ hub →json.Marshal(tx), emittinghasLogs/hasOperations/status.dispatchLogEvents→PrepareLogsForDB→prepareLogsForDBcopiestx.Status/tx.ResultCodeinto the newLogs.Status/Logs.ResultCode.
So on a node running the websocket feed with the indexer disabled — the natural deployment for a public event feed — ExtractDataFromLogs never runs: every user_transactions event omits hasLogs/hasOperations, and every logs event reports pre-log-processing status/resultCode. The same block on a node that also has ES enabled emits hasLogs: true and the post-mutation status. Two nodes on the same chain, divergent feed payloads for the same block, decided by an unrelated config flag.
That matters especially because status/resultCode were added so a subscriber can tell a log from a reverted transaction apart from a committed one — which is exactly the distinction that silently stops working on a ws-only node.
On develop this side effect lived only on the elastic worker, so it was racy on ws+ES and absent on ws-only. The fix made ws+ES deterministic but left ws-only deterministically different rather than making the two agree. Gating on prepared != nil instead (i.e. run it whenever the websocket path will consume prepared.Txs), while keeping the LogsResults hand-off to ES conditional, would make both configurations emit the same payload.
Two related notes while you are in here:
- Status casing is inconsistent.
informativeLogswrites"SUCCESS"/"Fail"(transaction.Transaction_SUCCESS.String()), whileBuildTransactionwrites"success"/"fail"(indexer/common.go:428-431). Sincestatusis now akeywordin the logs mapping, those are distinct terms — an exact-match query for one silently misses the other. informativeLogsProcessorkeys off the event identifier, and the KVM sets awriteLog-hook event's identifier to the executing contract function's name (kvm/vmhost/contexts/output.go:274-276). A contract with an endpoint literally namedsignalErrortherefore getstx.Statusrewritten to"Fail"on success. That quirk pre-exists in ES, but the reordering newly propagates it, deterministically, into the live websocket feed.
There was a problem hiding this comment.
Fixed in 6946bf2 — good catch, and you named the exact right seam. ExtractDataFromLogs now runs whenever prepared != nil (not just indexerEnabled); only the subsequent hand-off to the elastic worker (args.Prepared = prepared) stays gated on indexerEnabled. Added TestEventsProcessor_SaveBlock_LogsPayloadConsistentWithoutIndexer on a ws-only processor asserting the writeLog-driven Status override survives without an indexer configured — mutation-tested against the old if indexerEnabled gate to confirm it fails there.
One thing your finding surfaced that I then had to fix in a follow-up (4238e76): making this unconditional meant ws-only nodes now also reach scDeploysProcessor/scInvocationsProcessor, which do pubKeyConverter.Encode on contract-controlled event topics — expensive on malformed input, previously only reachable when ES was enabled. Split logsAndEventsProcessor's processor list into full vs. lightweight (informative-only) sets; ExtractDataFromLogs now takes full bool, passed as indexerEnabled — so the payload-parity fix doesn't also cost ws-only nodes the scDeploy/scInvocation work they never needed. New test TestExtractDataFromLogs_LightweightModeSkipsExpensiveProcessors asserts zero Encode calls and correct HasOperations/Status parity in lightweight mode.
The status-casing (SUCCESS/Fail vs success/fail) and signalError-identifier-quirk notes are real but pre-existing and orthogonal to this PR (both live in informativeLogsProcessor/BuildTransaction, untouched here) — flagging for a separate follow-up rather than fixing inline, since this PR's reordering just makes the existing quirk deterministically visible rather than introducing it.
| // watches an address with LOGS accepted. Wired into indexer.LogsSubscriberChecker (see | ||
| // network/api/api.go) so the block-commit goroutine can skip the log-conversion cost | ||
| // entirely for a block that nobody would receive it for. | ||
| func (h *SocketHub) HasLogsSubscriberOrMirror() bool { |
There was a problem hiding this comment.
[correctness — confirmed] New: the block-commit goroutine now blocks on the hub mutex, which handleClientDelete holds across a socket close and a full-map scan
HasLogsSubscriberOrMirror takes h.mu.RLock(), and it is now called synchronously from dispatchLogEvents on the block-commit goroutine for every block with logs. The same mutex is taken exclusively by handleClientDelete (:621-641), which holds it across c.close() — conn.Close() (a syscall), close(c.out), and a drain of a 500-entry buffered channel — and across a scan of the entire addressSubscription outer map. With defaultMaxAddressesPerClient = 50000, that map can hold hundreds of thousands of keys.
Failure scenario: a client connects, subscribes to 50,000 addresses (five subscribe calls at the 10,000-per-call cap), then disconnects, in a loop. Each disconnect takes the write lock for an O(total-addresses) walk plus a socket teardown, and because sync.RWMutex blocks newly arriving readers once a writer is waiting, the commit goroutine's HasLogsSubscriberOrMirror() parks behind the whole queue of disconnects.
This is the same "a client cheaply inflates block-commit cost by holding many address subscriptions" surface that e4417bb4's own commit message set out to close. The counter removed the scan the reader paid, but the reader now waits on a writer doing the identical scan plus I/O. Before this PR the block-commit goroutine never touched the hub's mutex at all, so the coupling is new.
Either make the check lock-free (an atomic.Int64, or a bool flipped under the existing lock and read atomically), or move c.close() out from under h.mu.
There was a problem hiding this comment.
Fixed in 6946bf2 — you named the exact mechanism. logsSubscriberCount is now atomic.Int64; HasLogsSubscriberOrMirror reads it with .Load() and takes no lock at all, so it can never queue behind handleClientDelete's write lock regardless of how long that holds h.mu (socket close + map scan). The four mutation sites still run inside the existing h.mu.Lock() critical section (no new race there), just using .Add/.Store instead of ++/--/=.
Added TestHasLogsSubscriberOrMirror_NeverBlocksOnHubMutex, which holds h.mu.Lock() for the whole test and asserts the checker still returns from another goroutine within a second — mutation-tested by reverting to the RLock version and confirming it hangs/times out.
| // synchronously on the commit goroutine and reused by the elastic worker instead of | ||
| // recomputing them on its own goroutine — see eventsProcessor.SaveBlock. | ||
| LogsResults *PreparedLogsResults | ||
| LogsDB []*Logs |
There was a problem hiding this comment.
[efficiency — plausible] New: caching LogsDB pins every queued block's hex-expanded logs in the elastic work-item backlog
Caching the conversion on PreparedBlockData extends its lifetime from "one elastic worker iteration" to "however long the work item sits in chanWorkItems", which is indexerCacheSize deep — 100 by default (config/node/external.yaml:3, indexer/dataDispatcher.go:41).
Failure scenario: Elasticsearch stalls, dataDispatcher.doWork retries with backoff up to 5 minutes, the queue fills, and 100 blocks' worth of converted logs — topics and data hex-encoded, so roughly 2× the raw bytes — stay pinned in memory on top of the already-retained Txs/TxsMap/Altered. It only bites when websocket + ES are both enabled and a LOGS subscriber exists, which is the intended production configuration.
Not necessarily worth changing — the CPU saving is real and this is a memory-for-CPU trade you may well want. But it is worth making deliberately rather than as a side effect, and the retention change is not mentioned anywhere. If it does concern you, clearing prepared.LogsDB once the elastic worker has consumed it would cap the exposure to the in-flight block.
There was a problem hiding this comment.
Took your suggested remediation in 4238e76: p.LogsDB = nil; p.LogsResults = nil now happens after doBulkRequests succeeds, not before. Two independent audit agents I ran before pushing caught that my first attempt cleared them before the bulk request could fail — since dataDispatcher.doWork retries a failed work item on the same PreparedBlockData, that would have forced the retry to recompute ExtractDataFromLogs on the elastic worker goroutine, reintroducing the exact race this whole round of fixes exists to prevent. Added TestElasticProcessor_SaveTransactions_KeepsCachedLogsOnBulkRequestError (fields must survive an error) alongside the existing clear-on-success test; mutation-tested both orderings.
fbsobreira
left a comment
fbsobreira
left a comment
There was a problem hiding this comment.
Re-reviewed at 32134b73. The previous round was addressed well — all eight of my earlier threads are resolved, and I have withdrawn one of them as my own false positive. But verification surfaced two new confirmed issues introduced by the fixes, so I am not clearing the block yet.
Previous round: verified
| Finding | Outcome |
|---|---|
| tx-pointer data race | Fixed — ordering + hand-off, not copying (the safer choice here). Also covers a writer my comment missed: informativeLogs.go writing tx.Status. |
| ES logs index template | Fixed in code — both templates now declare every field with correct types, and differ only in the package line. See the deployment caveat below. |
| Conversion with zero subscribers | Fixed — LogsSubscriberChecker gate, O(1). ES correctly falls back when the gate skipped conversion, so indexing is unaffected. |
| Double conversion per block | Fixed — rides PreparedBlockData.LogsDB. |
| Positional bools | Fixed — userOptions throughout. |
| LOGS-only subscriber pre-check | Fixed and generalized — dispatchToAddress now serves all four address-scoped handlers with one lock and lookup. |
| Verbose comments | Addressed for every in-scope site. I also owe a correction: websocket/config.go and dropWarner are base code, not this PR — my citation was wrong. |
| Unbounded mirror POSTs | Withdrawn — my error. asyncPost never spawned goroutines; it is a non-blocking send into a 1000-slot queue drained by 8 fixed workers (#97, predating this PR), and fan-out is per-transaction, not per-event. |
I also audited the new logsSubscriberCount specifically for the failure mode that would matter most — under-counting, which would silently starve a real subscriber. Every increment and decrement is balanced and guarded, all under h.mu; it cannot go negative or under-count. go test -race ./websocket/... ./indexer/... ./network/api/websocket/... passes locally, as does go build ./....
New: please fix before merge
-
Websocket payload now depends on whether Elasticsearch is enabled (eventsProcessor.go:106).
ExtractDataFromLogsmutatestx.HasLogs/HasOperations/Status, and both websocket consumers read those fields afterwards — but the call sits underif indexerEnabled. On a ws-only node (the natural public-feed deployment)hasLogsis always absent andlogs.status/resultCodereport pre-log-processing values, so this PR's own new fields silently stop distinguishing reverted from committed logs. Gating onprepared != nilinstead, with the ES hand-off staying conditional, makes both configurations agree. Two related warts noted in the comment:statuscasing differs between the two writers ("SUCCESS"vs"success") which matters now that it is akeyword, and a contract endpoint namedsignalErrordeterministically forcesStatus: "Fail"on success. -
The block-commit goroutine now blocks on the hub mutex (websocket.go:273).
HasLogsSubscriberOrMirrortakesh.mu.RLock()on the commit path, andhandleClientDeleteholds that same lock exclusively across a socket close and an O(total-addresses) map walk. Under Go's writer-preferringRWMutex, a subscribe-many/disconnect loop parks block commit behind those teardowns. This re-opens, from the writer side, the exact surfacee4417bb4set out to close — before this PR the commit goroutine never touched the hub mutex at all. Anatomiccounter, or movingc.close()out from underh.mu, closes it. -
LogsDBretention in the elastic backlog (prepared.go:14) — plausible, not confirmed, and possibly a trade you want. Flagging so it is deliberate rather than incidental.
Two things that are not code changes
- The ES mapping needs a one-time manual step before rollout.
CheckAndCreateTemplatereturns early when the template already exists, andlogsIndexrollover is disabled, so the corrected template is never PUT to a cluster that has run this indexer. Without aPUT logs-000001/_mappingaddingstatus/resultCodeaskeywordandisSystemLogasbooleanbefore the new binary starts writing, the first document still bakes ES's dynamictextmapping into the live index — the original harm, just not reachable through code. Adding fields to an existing mapping is legal; changing them afterwards is not, which is why the ordering matters. - CI red is not this PR's fault. The only failure is
TestConsensus_InsertDupTransaction, the known issue-106 flake. Its fix (e48c2c24, #119) landed on develop after this branch's baseb77a13b3. A rebase onto current develop should turn CI green and is worth doing regardless.
32134b7 to
5310ce8
Compare
|
Round 6 fixes pushed (60db05d), all three new findings addressed — replied inline on each thread. Operational note, surfacing per your request so it doesn't get lost in a resolved thread: the ES index-template fix ( Also, for the record since it's not obvious from the diff alone: the |
Adds a new address-scoped /subscribe topic, "logs", so clients can receive smart-contract execution logs/events live instead of only via the Elasticsearch indexer. Mirrors the existing accounts subscription exactly: same addressSubscription fan-out mechanism (userOptions gains a third acceptLogs flag alongside acceptAccount/ acceptTransaction), same address-count/length hardening caps (inherited automatically via containsAddressScoped), same non-blocking EventQueue dispatch path. On the producer side, eventsProcessor.SaveBlock now also converts each block's SC logs (already collected in TransactionsPool.Logs) via the same logsevents.PrepareLogsForDB converter already used to index them into Elasticsearch, and dispatches one LOGS event per block; the hub fans each entry out by its (top-level) contract address. Purely additive to the wire protocol — existing blocks/ transactions/accounts/user_transactions subscribers are unaffected. v1 filters by the top-level contract address only (matching what's already indexed today); a cross-contract call's inner-contract events are attributed to the outer contract, not the inner one. Documented as a known limitation in README.md alongside the other named non-goals (no per-event/topic filtering, no historical replay). Reviewed via a two-agent correctness + security audit pass; no findings beyond a pre-existing, bounded, non-issue duplication of log-prep work when both Elasticsearch and the websocket hub are enabled for the same block.
Replace two time.Sleep-based waits in tests added for the logs subscription with real synchronization, per this repo's own .coderabbit.yaml path-instruction banning hardcoded sleeps for test sync. TestStartServer_LogsEvent_NoSubscribers: rather than dropping the sleep outright (which would race the queued event against shutdown and could let it go unprocessed most runs, silently losing coverage rather than just gaining determinism), push a second, subscribed BLOCKS event afterward and await its delivery — env.queue is a single channel drained in order by StartServer's single-threaded loop, so observing the second event's delivery proves the first was already run through handleLogsEvent without panicking. TestSubscribeTopics_LogsEventDelivery: the initial /subscribe handshake has no success ack on the wire, which is what made the original sleep a guess. Switched to subscribing via the dynamic `subscribe` method instead, which does send a "subscribed" ack only after HandleClientInsertion completes under the hub's lock — waiting on that ack (already a real protocol feature, no test-only hook needed) before pushing the LOGS event is a genuine readiness signal. Also: TestHandleClientInsertion_AddressTypes now asserts HandleClientInsertion's returned error instead of discarding it.
TestSubscribeTopics_LogsEventDelivery discarded the return value of two conn.SetReadDeadline calls. Wrapped both in require.NoError so a failed deadline setup fails the test clearly instead of silently leaving ReadJSON without its intended timeout. Five other pre-existing SetReadDeadline calls elsewhere in this file (unrelated tests, not touched this session) are left as-is.
TestSubscribeTopics_LogsEventDelivery started hub.StartServer(ctx) in a bare goroutine with only defer cancel() — no wait for it to actually exit. indexer.EventQueue is a package-level global not swapped per test in this external test package, so a StartServer goroutine that outlives its test could consume an event meant for (or race with) a subsequent test in the same file. Now waits on a done channel in t.Cleanup after cancelling, guaranteeing the goroutine has returned before the next test runs. A pre-existing sibling test (TestSubscribeTopics_BlockEventDelivery) has the identical bare-goroutine pattern and predates this PR — left untouched, not flagged by the reviewer, out of scope here. Audited; no findings. -race -count=20 clean.
…e fix - dispatchLogEvents no longer lives inside the `prepared != nil` guard: it didn't actually depend on prepared, so a tx-prep failure was silently dropping a whole block's LOGS from the feed while BLOCKS still shipped. - Thread ContractID/IsSystemLog through PrepareLogsForDB via new LogHandler.GetContractID()/EventHandler.GetIsSystemLog() interface methods (both already exist as proto-generated getters on the concrete types). - Add Status/ResultCode to data.Logs so a subscriber can tell a log from a reverted/failed transaction apart from one that actually committed. - Skip marshal+mirror-post in handleLogsEvent when there's no subscriber for an entry's address and no HTTP mirror configured — LOGS volume per block can be far higher than ACCOUNTS/TRANSACTIONS. - Simplify NewEventType to delegate to NewEventTypeStrict instead of duplicating the switch. - Replace hard-to-read `i = 10` drain loops in tests with drainAllEvents/ findEventType helpers; strengthen log-event test assertions; add a regression test for the dropped-log fix (mutation-tested against the original bug). - Port the already-merged keepalive atomic-timing fix (pingPeriod/pongWait torn-read race) onto this branch, which was cut before that fix merged to develop — found via `go test -race` while validating this round.
…rsion waste, schema drift, userOptions cleanup - Fix a genuine data race: SaveTransactions (async elastic worker) used to call ExtractDataFromLogs itself, mutating tx.HasLogs/tx.HasOperations on the same *data.Transaction pointers the websocket hub concurrently json.Marshals via dispatchTransactionEvents. Now run synchronously on the commit goroutine whenever both websocket and Elasticsearch indexing are enabled, before either consumer can touch prepared.Txs, and reused via PreparedBlockData.LogsResults instead of being recomputed. - Dedupe PrepareLogsForDB the same way (PreparedBlockData.LogsDB) when both paths need it. - Confirmed the "unbounded goroutine + HTTP POST per log entry" finding was already resolved by rebasing onto develop, which picked up klever-io#97's bounded postWorkers/postQueue pool — handleLogsEvent already routes through it. - Add explicit ES index-template mappings for the new logs fields (caller/contractId/status/resultCode, events.order/isSystemLog) so they don't fall back to ambiguous dynamic mapping. - Skip the LOGS conversion entirely on the commit goroutine when nobody would receive it: new indexer.LogsSubscriberChecker hook, wired from the websocket hub's HasLogsSubscriberOrMirror. - Replace applyEventTypes/addAddressSubscriptions/removeClientFromAddress's positional bool triplets with a single userOptions value. - Generalize the "skip marshal when nobody's listening" gate (previously LOGS-only) into one dispatchToAddress helper shared by ACCOUNTS, LOGS, and tx sender/receipts, replacing the old hasAddressSubscriber+ notifyAddressSubscribers pair's separate lock/lookup with a single snapshot. - Trim several multi-line narrative comments down to their essential why.
…e, mirrorConfigured predicate - HasLogsSubscriberOrMirror scanned the full addressSubscription map synchronously on the block-commit goroutine for every block with logs — a client could cheaply inflate that cost by holding many address subscriptions without ever accepting LOGS (consensus-timing DoS surface). Replaced with an incrementally-maintained logsSubscriberCount, kept in sync by addAddressSubscriptions/removeClientFromAddress/ handleClientDelete/deleteAll, making the check O(1). - mirrorConfigured was postConnectionURL != "" || postConnectionAPIKey != "", but the mirror is only actually enabled when postQueue != nil (URL only, per NewHub's own stated design) — an API-key-without-URL misconfig silently defeated the "skip when nobody's listening" optimization forever. Now set directly from postQueue != nil. Found by parallel correctness+security audit agents before push.
…tive Audit flagged this as a coverage gap (the shipped code already guards it correctly via existing.acceptLogs in removeClientFromAddress) — a negative count would silently suppress LOGS dispatch for a later genuine subscriber. Also fixes a stale test-name reference in a comment.
…-free subscriber gate, log cache retention - Fix payload divergence between ws-only and ws+ES nodes: ExtractDataFromLogs (sets tx.HasLogs/HasOperations/Status) was gated on indexerEnabled, so a ws-only node emitted different TRANSACTIONS/USER_TRANSACTIONS/LOGS payloads than a ws+ES node for the identical block. Now runs whenever the websocket path will consume prepared.Txs; only the hand-off to the elastic worker stays conditional on indexerEnabled. - Fix a new DoS-adjacent finding in last round's own fix: HasLogsSubscriberOrMirror took h.mu.RLock(), and it runs synchronously on the block-commit goroutine — handleClientDelete holds h.mu.Lock() across a socket close and a full map scan, so a burst of client disconnects could park the commit goroutine behind that queue (RWMutex favors a pending writer over new readers). logsSubscriberCount is now an atomic.Int64, read lock-free. - Cap PreparedBlockData.LogsDB/LogsResults retention: clear both once the elastic worker has consumed them, instead of leaving them pinned in the work-item queue (up to indexerCacheSize deep) for no further benefit. - Strengthen the LogsSubscriberChecker gate's regression test with a call-counting wrapper — the previous version only checked no event was dispatched / a stash field stayed nil, which a slightly-later gate could still pass while paying the conversion cost anyway. - README: note contractId's constant-to-real-ID change and mirror delivery being best-effort (sheds under sustained load, doesn't backpressure). Also: rebased onto develop, which picked up fbsobreira's own fix for the flaky TestConsensus_InsertDupTransaction (klever-io#119) causing our CI's spurious test-job failures.
…success race, expensive-processor exposure on ws-only nodes Two independent audit agents (correctness + security) reviewed the round-6 commit before push and both found the same real regression, plus security found one more: - elasticProcessor.SaveTransactions cleared PreparedBlockData.LogsDB/ LogsResults before doBulkRequests could fail. dataDispatcher.doWork retries a failed work item on the same PreparedBlockData; with the fields already nil, the retry would recompute ExtractDataFromLogs on the elastic worker goroutine — reintroducing the exact tx-pointer race round 5 fixed. Moved the clear to after doBulkRequests succeeds. - Making ExtractDataFromLogs unconditional (round 6's payload-parity fix) newly exposed ws-only nodes to its scDeploy/scInvocation processors, which decode contract-controlled event topics as addresses via pubKeyConverter.Encode — expensive on malformed input (stack trace + hex logging in the error path), previously reachable only when Elasticsearch was enabled. Split logsAndEventsProcessor's processor list into full (scDeploy/scInvocation/informative) and lightweight (informative only, the only one needed for websocket payload parity) sets; ExtractDataFromLogs takes a new `full bool` parameter, true only when Elasticsearch will actually consume ScDeploys/AlteredSCs. Both fixes mutation-tested (temporarily reverted, confirmed the new test fails, restored).
…cises the parity path The audit that reviewed 4238e76 caught that txs[0].Hash didn't match the hex-encoded lookup key ExtractDataFromLogs uses, so the test's informativeLogsProcessor parity assertions were never actually exercised (silently vacuous). Fixed the keying and added a writeLog event so the test covers both the ScDeploys/ Encode-skipping property and the HasOperations/Status parity property in one place. Mutation-tested (emptied the lightweight processor list, confirmed the new assertions fail, restored).
60db05d to
0d71d56
Compare
3 participants
Summary
/subscribetopic,logs, so clients can receive smart-contract execution logs/events live instead of only via the Elasticsearch indexer.accountssubscription exactly: sameaddressSubscriptionfan-out mechanism (userOptionsgains a thirdacceptLogsflag alongsideacceptAccount/acceptTransaction), same address-count/length hardening caps (inherited automatically viacontainsAddressScoped), same non-blockingEventQueuedispatch path.HandleClientInsertion,HandleClientRemoval,applyEventTypes,addAddressSubscriptions,removeClientFromAddresswere all extended in parallel across all three flags.eventsProcessor.SaveBlocknow also converts each block's SC logs (already collected inTransactionsPool.Logs) via the samelogsevents.PrepareLogsForDBconverter already used to index them into Elasticsearch, and dispatches oneLOGSevent per block; the hub fans each entry out by its (top-level) contract address.blocks/transactions/accounts/user_transactionssubscribers are unaffected.hashfield is populated with the originating transaction hash (already computed by the shared converter) so a subscriber can correlate a log back to its transaction.v1 scope / known limitation: filters by the top-level contract address only (matching what's already indexed to Elasticsearch today) — a cross-contract call's inner-contract events are attributed to the outer contract, not the inner one. Documented in
websocket/README.mdalongside other named non-goals for this version (no per-event/topic filtering, no historical replay/backfill).Reviewed with a two-agent correctness + security audit pass against the diff; no findings beyond a pre-existing, bounded, non-issue duplication of log-prep work when both Elasticsearch and the websocket hub are enabled for the same block (each subsystem already builds its own independent view — same trade-off pattern used elsewhere in this file).
Test plan
go build ./...go vet ./indexer/... ./websocket/... ./network/api/websocket/...gofmt -lclean on changed Go filesgo test ./indexer/... ./websocket/... ./network/api/websocket/... -race— all pass, including new coverage:indexer: log-event dispatch, no-op on empty/nil logs, skipped when disabled,NewEventsProcessorconstructs the shared logs converterwebsocket: dispatch/fan-out by contract address (including a multi-entry, multi-client isolation test), partial/full unsubscribe across all three address-scoped flags,containsAddressScopedparity, message marshalingnetwork/api/websocket: HTTP-layer handshake acceptslogs, full end-to-end delivery over a real websocket connectionwebsocket/README.mdupdated: new subscription type, payload shape, known limitations, and the subscribe/unsubscribe param tablesSummary
logssubscription type for live smart-contract execution logs/events by extending the existing address subscription model with a per-addressacceptLogsflag and corresponding subscribe/unsubscribe lifecycle handling.LOGSwebsocket events (viaPrepareLogsForDB) and dispatch them when the websocket/event-queue delivery path is enabled.LOGSdelivery is routed by top-level contract address and carries the originating transaction hash in the message envelope.logs: allowed subscribed types, address-scoped filtering, payload marshaling/validation (skip nil/empty-address log entries), and defensive delivery behavior (no delivery when there are no log subscribers; no log dispatch when the relevant pool is nil; correct separation across different subscribed addresses).logs(indexer.LOGS) and adds/updates targeted unit + integration tests to verify correct routing, gating, and subscription state handling forLOGS.Blockchain-critical impact & stability/data integrity
acceptLogs, and invalid payload entries (e.g., nil/empty address) are filtered out before sending.LOGSdispatch is skipped when websocket/event-queue delivery is disabled, and additional tests ensure other event types (e.g.,BLOCKS) continue to be delivered even when the logs pool is nil.NewEventsProcessorfails fast if the logs/events processor cannot be constructed; a follow-up test cleanup change ensures the logs-delivery server goroutine exits after cancellation to avoid races or unintended event consumption during subsequent tests.