Bug fixes

[WantClue]

Summary by CodeRabbit

• Bug Fixes

  • Improved handling and retry logic for repeated network read timeouts with a warning after consecutive failures.
  • Hardened parsing and validation of incoming protocol messages (including merkle branch and parameter checks) to reject malformed data and avoid crashes.

• Chores

  • Ensure transports are explicitly destroyed during connection shutdowns and reconnection flows to free resources reliably.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 25a9e425-292b-4445-8da2-513c443fbcc9

📥 Commits

Reviewing files that changed from the base of the PR and between 619bfbd and 56e3d47.

📒 Files selected for processing (1)

• components/stratum/stratum_api.c

🚧 Files skipped from review as they are similar to previous changes (1)

• components/stratum/stratum_api.c

---

📝 Walkthrough

Walkthrough

Added timeout counting to transport reads that aborts after 10 consecutive zero-byte reads and frees the JSON RPC buffer; tightened existence/type validation for several mining RPC params (notify, set_difficulty, set_version_mask, set_extranonce); ensured transports are destroyed after close in reconnect/shutdown paths.

Changes

Cohort / File(s)	Summary
JSON RPC read & parsing components/stratum/stratum_api.c	Added timeout_count in STRATUM_V1_receive_jsonrpc_line to retry on zero-byte reads and abort after 10 timeouts (frees global JSON buffer, nulls pointers). Strengthened param existence/type checks and error handling in STRATUM_V1_parse for mining.notify, mining.set_difficulty, mining.set_version_mask, and mining.set_extranonce; added safe cleanup on invalid params.
Transport lifecycle cleanup main/tasks/stratum_task.c	Added explicit esp_transport_destroy() calls after esp_transport_close() in multiple failure/reconnect and shutdown paths (primary heartbeat, connection attempts, and close routine) to release transport resources.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Transport
    participant StratumAPI
    participant JSONBuffer

    Client->>Transport: connect / read()
    Transport-->>StratumAPI: return nbytes, data
    alt nbytes > 0
        StratumAPI->>StratumAPI: timeout_count = 0
        StratumAPI->>JSONBuffer: realloc & append data
        StratumAPI->>StratumAPI: parse JSON (STRATUM_V1_parse)
    else nbytes == 0
        StratumAPI->>StratumAPI: timeout_count++
        alt timeout_count >= 10
            StratumAPI->>JSONBuffer: free & null
            StratumAPI->>Transport: close()
            StratumAPI->>Transport: destroy()
            StratumAPI-->>Client: return NULL / abort
        else
            StratumAPI->>Transport: retry read()
        end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I counted timeouts, nibbled code,

Freed a buffer down the road,
Closed a transport with a hop,
Parsed each branch before I'd stop,
A happy rabbit, fixes done—nonstop.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Bug fixes' is vague and generic, using a non-descriptive term that doesn't convey meaningful information about the specific changes in the changeset.	Use a more specific title that describes the main changes, such as 'Add timeout handling and strengthen JSON-RPC parsing in Stratum API' or 'Improve error handling in Stratum connection and parsing logic'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch bug-fixes

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@components/stratum/stratum_api.c`:
- Around line 382-390: Before any validation failure returns via the existing
cleanup/goto paths in the parsing routine, clear the parsed message state so
stale payloads cannot be reused; specifically, reset message->method (and any
other fields that indicate a populated StratumApiV1Message) to NULL/zero before
each of the early frees/goto done branches (the branches that free new_work and
then goto done, and the similar branches around the merkle/branch/extranonce
parsing). Update the validation blocks that currently free new_work and goto
done (including the other similar exits mentioned) to set message->method = NULL
(or memset the StratumApiV1Message to zero) immediately prior to freeing and
jumping to done so the shared StratumApiV1Message cannot replay stale data.
- Around line 172-176: The loop in stratum_api.c aborts after timeout_count
reaches 10, which conflicts with the longer socket receive timeout used
elsewhere; change the cutoff logic in the function that increments timeout_count
(refer to timeout_count and the branch that currently returns NULL) to compute
the max allowed zero-read iterations from the configured transport timeout
constants (e.g., use TRANSPORT_TIMEOUT_MS or the socket receive timeout constant
instead of hardcoded 10) or introduce a TRANSPORT_MAX_TIMEOUTS constant derived
from the liveness budget so the NULL return aligns with the socket's receive
timeout policy.
- Around line 172-176: Before returning NULL on repeated transport read
timeouts, clear the static framing state so future connections don't append to
stale data: reset json_rpc_buffer (e.g., set first byte to '\0' or zero out the
buffer) and reset its position/index variable (e.g., json_rpc_buffer_pos or any
json_rpc_* length counters) and any related state (timeout_count if appropriate)
in the same function that contains the nbytes/timeout_count timeout branch so
the buffered partial message is discarded prior to returning NULL.
- Around line 381-392: The handlers mining.notify, mining.set_difficulty and
mining.set_extranonce currently assume params is an array and directly
dereference items (e.g., params[5]->valuestring, p0->valueint, p1->valueint);
update each handler (mining.notify, mining.set_difficulty,
mining.set_extranonce) to first validate that the params cJSON pointer is a
cJSON array via cJSON_IsArray(), then use cJSON_GetArrayItem() and check each
returned item for NULL and the expected type with cJSON_IsString() or
cJSON_IsNumber() before reading ->valuestring or ->valueint; add error handling
paths that skip processing or return an error when validation fails to avoid
using uninitialized/invalid data.

🪄 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: e4e3c20d-16b1-4e8c-b060-11a703311378

📥 Commits

Reviewing files that changed from the base of the PR and between 36120db and 8ec1f3a.

📒 Files selected for processing (2)

• components/stratum/stratum_api.c
• main/tasks/stratum_task.c