Stratum improv#28
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR enhances Stratum pool configuration by adding a compile-time option to conditionally send ChangesStratum URL Handling & Job Queue Management
Sequence DiagramsequenceDiagram
actor User as User
participant UI as Pool Component UI
participant HTTP as HTTP Server
participant NVS as NVS Storage
participant Stratum as Stratum Task
participant Pool as Stratum Pool Server
User->>UI: Paste full URL (e.g., stratum+tls://pool.com:3333)
UI->>UI: onUrlChange() detects protocol, extracts port, strips prefix
UI->>HTTP: updateSystem() sends normalized URL + extracted TLS + Port
HTTP->>HTTP: save_stratum_url() parses URL, verifies port, detects TLS scheme
HTTP->>NVS: Write normalized URL, port, TLS mode
NVS-->>HTTP: Stored
Stratum->>NVS: Read stratumURL, stratumPort, stratumTLS
Stratum->>Stratum: resolve_stratum_address() calls getaddrinfo()
Stratum->>Stratum: Select AF_INET/AF_INET6, populate sockaddr, set scope_id if needed
Stratum->>Pool: Connect using resolved host_ip
Pool-->>Stratum: Connected (TCP or TLS)
alt CONFIG_STRATUM_SUGGEST_DIFFICULTY enabled and STRATUM_DIFFICULTY > 0
Stratum->>Pool: Send mining.suggest_difficulty
end
Pool-->>Stratum: Confirm/Response
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
components/stratum/stratum_api.c (1)
145-175:⚠️ Potential issue | 🟠 MajorRemove the busy-wait concern; the real issue is the missing timeout/retry limit when
nbytes == 0.When
esp_transport_readreturns 0 (timeout after 5 seconds with no data), the loop continues and blocks for another 5 seconds. While this is not a busy-wait sinceesp_transport_readblocks, the function has no maximum retry limit. If the peer closes the connection gracefully or stops sending data, the loop waits indefinitely in 5-second intervals without attempting reconnection.The proposed fix to track consecutive timeouts and return
NULLafter a threshold (triggering reconnection logic in the caller) is reasonable for defensive programming, though it should be verified whether this behavior represents a regression from previous implementation or is expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/stratum/stratum_api.c` around lines 145 - 175, The loop that reads into json_rpc_buffer via esp_transport_read currently treats nbytes==0 as a benign repeat and can block forever in TRANSPORT_TIMEOUT_MS intervals; modify the read loop in the function that uses recv_buffer/json_rpc_buffer and esp_transport_read to count consecutive zero-byte timeouts, free json_rpc_buffer and return NULL after a small configurable threshold (e.g., 3) to trigger reconnection, reset the counter to 0 on any nbytes>0, and keep the existing error handling for negative nbytes; ensure realloc_json_buffer is still called only when nbytes>0 and include the timeout-counter variable scoped to the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@components/stratum/stratum_api.c`:
- Around line 145-175: The loop that reads into json_rpc_buffer via
esp_transport_read currently treats nbytes==0 as a benign repeat and can block
forever in TRANSPORT_TIMEOUT_MS intervals; modify the read loop in the function
that uses recv_buffer/json_rpc_buffer and esp_transport_read to count
consecutive zero-byte timeouts, free json_rpc_buffer and return NULL after a
small configurable threshold (e.g., 3) to trigger reconnection, reset the
counter to 0 on any nbytes>0, and keep the existing error handling for negative
nbytes; ensure realloc_json_buffer is still called only when nbytes>0 and
include the timeout-counter variable scoped to the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08acb5e1-aa06-432c-a719-a1a3733c746d
📒 Files selected for processing (6)
components/stratum/stratum_api.cmain/Kconfig.projbuildmain/http_server/forge-os/src/app/components/pool/pool.component.htmlmain/http_server/forge-os/src/app/components/pool/pool.component.tsmain/http_server/http_server.cmain/tasks/stratum_task.c
- Adjusted QUEUE_LOW_WATER_MARK to 1 for more responsive job generation. - Implemented abandonment handling in create_jobs_task to clear ASIC jobs when work is abandoned. - Replaced direct access to queue counts with queue_count function for better encapsulation. - Added has_active_jobs function to check for active jobs in valid_jobs array. - Enhanced cleanQueue logic to consider active jobs before clearing queues. - Introduced queue_dequeue_if_full function to handle dequeuing only when the queue is full. - Updated stratum_task to utilize new queue management functions for improved clarity and maintainability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
main/tasks/create_jobs_task.c (1)
35-42: 💤 Low valueConsider extracting duplicate abandonment handling into a helper function.
The abandon_work handling logic at lines 35-42 and 68-75 is nearly identical. Extracting this into a static helper would reduce duplication and ensure both paths stay in sync.
♻️ Suggested refactor
+static void handle_abandon_work(GlobalState *GLOBAL_STATE) +{ + if (GLOBAL_STATE->abandon_work == 1) { + GLOBAL_STATE->abandon_work = 0; + pthread_mutex_lock(&GLOBAL_STATE->valid_jobs_lock); + ASIC_jobs_queue_clear(&GLOBAL_STATE->ASIC_jobs_queue); + pthread_mutex_unlock(&GLOBAL_STATE->valid_jobs_lock); + xSemaphoreGive(GLOBAL_STATE->ASIC_TASK_MODULE.semaphore); + } +}Then replace both blocks with
handle_abandon_work(GLOBAL_STATE);Also applies to: 68-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/tasks/create_jobs_task.c` around lines 35 - 42, Extract the duplicated abandon handling into a static helper (e.g., static void handle_abandon_work(GlobalState *state) or static void handle_abandon_work(void) if using GLOBAL_STATE directly) and move the logic that checks/clears state->abandon_work, resets it to 0, locks/unlocks state->valid_jobs_lock, calls ASIC_jobs_queue_clear(&state->ASIC_jobs_queue) and xSemaphoreGive(state->ASIC_TASK_MODULE.semaphore) into that helper; then replace both duplicated blocks (the ones referencing GLOBAL_STATE->abandon_work, pthread_mutex_lock/unlock, ASIC_jobs_queue_clear, and xSemaphoreGive) with a single call to handle_abandon_work(GLOBAL_STATE) (or handle_abandon_work()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/tasks/stratum_task.c`:
- Around line 206-220: The loops in has_active_jobs() and stratum_reset_uid()
currently step by 4 and only examine every 4th slot of GLOBAL_STATE->valid_jobs;
update both loops (in the functions has_active_jobs and stratum_reset_uid) to
iterate over every index by changing the loop increment from i = i + 4 to i++ so
all 128 uint8_t slots (indices 0..127) are checked under the existing
pthread_mutex_lock sections.
In `@main/work_queue.c`:
- Around line 50-68: The function queue_dequeue_if_full lacks a NULL check for
the out-parameter work and will dereference *work causing a crash if NULL is
passed; add a guard at the start of queue_dequeue_if_full (e.g., if (work ==
NULL) { return false; } or assert) before locking or before any dereference,
then proceed with the existing logic that reads from queue->buffer[queue->head],
updates queue->head and queue->count, signals queue->not_full and returns
dequeued; ensure you return false when work is NULL to preserve behavior and
avoid modifying the queue.
---
Nitpick comments:
In `@main/tasks/create_jobs_task.c`:
- Around line 35-42: Extract the duplicated abandon handling into a static
helper (e.g., static void handle_abandon_work(GlobalState *state) or static void
handle_abandon_work(void) if using GLOBAL_STATE directly) and move the logic
that checks/clears state->abandon_work, resets it to 0, locks/unlocks
state->valid_jobs_lock, calls ASIC_jobs_queue_clear(&state->ASIC_jobs_queue) and
xSemaphoreGive(state->ASIC_TASK_MODULE.semaphore) into that helper; then replace
both duplicated blocks (the ones referencing GLOBAL_STATE->abandon_work,
pthread_mutex_lock/unlock, ASIC_jobs_queue_clear, and xSemaphoreGive) with a
single call to handle_abandon_work(GLOBAL_STATE) (or handle_abandon_work()).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5037140-7dc0-42e8-8056-e0084e14bb05
⛔ Files ignored due to path filters (1)
main/http_server/forge-os/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
main/tasks/create_jobs_task.cmain/tasks/stratum_task.cmain/work_queue.cmain/work_queue.h
Comment on lines
+50
to
+68
| bool queue_dequeue_if_full(work_queue *queue, void **work) | ||
| { | ||
| bool dequeued = false; | ||
|
|
||
| pthread_mutex_lock(&queue->lock); | ||
|
|
||
| if (queue->count == QUEUE_SIZE) | ||
| { | ||
| *work = queue->buffer[queue->head]; | ||
| queue->head = (queue->head + 1) % QUEUE_SIZE; | ||
| queue->count--; | ||
| pthread_cond_signal(&queue->not_full); | ||
| dequeued = true; | ||
| } | ||
|
|
||
| pthread_mutex_unlock(&queue->lock); | ||
|
|
||
| return dequeued; | ||
| } |
Contributor
There was a problem hiding this comment.
Missing NULL check for work parameter.
If a caller passes NULL for the work out-parameter, dereferencing *work on line 58 will crash. Add a guard or document that work must not be NULL.
🛡️ Proposed fix
bool queue_dequeue_if_full(work_queue *queue, void **work)
{
+ if (queue == NULL || work == NULL) {
+ return false;
+ }
+
bool dequeued = false;
pthread_mutex_lock(&queue->lock);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bool queue_dequeue_if_full(work_queue *queue, void **work) | |
| { | |
| bool dequeued = false; | |
| pthread_mutex_lock(&queue->lock); | |
| if (queue->count == QUEUE_SIZE) | |
| { | |
| *work = queue->buffer[queue->head]; | |
| queue->head = (queue->head + 1) % QUEUE_SIZE; | |
| queue->count--; | |
| pthread_cond_signal(&queue->not_full); | |
| dequeued = true; | |
| } | |
| pthread_mutex_unlock(&queue->lock); | |
| return dequeued; | |
| } | |
| bool queue_dequeue_if_full(work_queue *queue, void **work) | |
| { | |
| if (queue == NULL || work == NULL) { | |
| return false; | |
| } | |
| bool dequeued = false; | |
| pthread_mutex_lock(&queue->lock); | |
| if (queue->count == QUEUE_SIZE) | |
| { | |
| *work = queue->buffer[queue->head]; | |
| queue->head = (queue->head + 1) % QUEUE_SIZE; | |
| queue->count--; | |
| pthread_cond_signal(&queue->not_full); | |
| dequeued = true; | |
| } | |
| pthread_mutex_unlock(&queue->lock); | |
| return dequeued; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/work_queue.c` around lines 50 - 68, The function queue_dequeue_if_full
lacks a NULL check for the out-parameter work and will dereference *work causing
a crash if NULL is passed; add a guard at the start of queue_dequeue_if_full
(e.g., if (work == NULL) { return false; } or assert) before locking or before
any dereference, then proceed with the existing logic that reads from
queue->buffer[queue->head], updates queue->head and queue->count, signals
queue->not_full and returns dequeued; ensure you return false when work is NULL
to preserve behavior and avoid modifying the queue.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
improvs connects with certain pools
Summary by CodeRabbit
New Features
Improvements