test: add unit tests for stratumserver module by iho · Pull Request #3886 · mimblewimble/grin

iho · 2026-07-09T07:21:53Z

Summary

Expand stratumserver unit tests from 8 (RPC serde only) to 34, covering the main testable units without consensus or full PoW mining.
Adds coverage for:
- RpcError constructors and JSON conversion
- parse_params success/failure paths (LoginParams, SubmitParams)
- WorkersList add/login/remove/re-login, stats updates, broadcast vs unicast, network stats, unknown-worker error paths
- Handler RPC routing: keepalive, unknown method, login, status, getjobtemplate (synced + syncing), stale/invalid/missing-params/invalid-edge_bits submit, build_block_template, last_seen

Addresses #3210.

Behavior fix (found via these tests, not just a test change): RpcError::internal_error() used a positive JSON-RPC error code (32603). Per the JSON-RPC 2.0 spec (and api/src/json_rpc.rs, which already uses the correct value), this is now -32603. This changes the wire response a stratum client receives on an internal error from {"code": 32603, ...} to {"code": -32603, ...}.

RPC routing tests share a single lazily-initialized test chain (via OnceLock) instead of opening one LMDB env per test, to avoid intermittent Invalid argument failures under default test-runner parallelism.

Test plan

cargo test -p grin_servers --lib mining::stratumserver::tests (34 passed, default parallelism; also verified with --test-threads=16)
CI on this PR

Not covered in this PR (possible follow-ups): valid share/block submit with real PoW, TCP accept-loop integration tests. See also the review comment about a pre-existing panic on out-of-range worker_id in handle_rpc_requests.

Expand stratumserver coverage beyond RPC serde helpers to exercise WorkersList lifecycle, parse_params, RpcError constructors, and Handler RPC routing (login, keepalive, status, getjobtemplate, submit stale). Addresses mimblewimble#3210.

- Use JSON-RPC -32603 for RpcError::internal_error (match api/json_rpc) - Apply cargo fmt on the login invalid-params assertion - Store test chain DBs under target/tmp/ to avoid repo-root clutter

…ors, re-login test - Share a single lazily-initialized test chain across RPC routing tests instead of opening one LMDB env per test; avoids the intermittent "Invalid argument" failures seen under default test parallelism. - test_last_seen_updates: seed last_seen to UNIX_EPOCH and assert strict '>' instead of '>=' after a sleep, so a no-op update would fail the test. - Add cheap submit error coverage: missing params (-32600) and an edge_bits value that is neither primary nor secondary PoW (-32502), neither requiring a mined proof. - Add a re-login test documenting that logging in again replaces the previous login/agent rather than being rejected. - Add a WorkersList-level test for get_stats on an unknown worker id (-32603), in place of exercising it through handle_rpc_requests, which panics on an out-of-range worker_id before reaching handle_status (see PR comment).

iho · 2026-07-11T15:27:26Z

Pushed a follow-up commit addressing the remaining open comments:

Shared a single lazily-initialized test chain (OnceLock) across the RPC routing tests instead of opening one LMDB env per test, to fix the intermittent Invalid argument failures under default parallelism. None of these tests reach chain::Chain::process_block, so the chain is read-only from their perspective and safe to share.
test_last_seen_updates now seeds last_seen to UNIX_EPOCH and asserts strict > instead of >= after a sleep, so a no-op update would actually fail the test.
Added the two cheap submit error cases: missing params (-32600) and an edge_bits value that's neither primary nor secondary PoW (-32502), neither requiring a mined proof.
Added a re-login test documenting that logging in again replaces the previous login/agent rather than being rejected.
Also updated the PR description to call out the -32603 fix as a wire-visible behavior change.

One thing I could not implement as suggested — found a real bug while trying: the request to add a status test with an unknown worker ID, asserting the -32603 response, doesn't work as written. handle_rpc_requests unconditionally calls self.workers.last_seen(worker_id) before dispatching to handle_status. last_seen → update_stats does stratum_stats.worker_stats[worker_id], an unchecked Vec index, whereas get_stats (which handle_status uses) does a checked .get(worker_id). So calling handle_rpc_requests with an out-of-range worker_id panics at the last_seen step with index out of bounds, rather than returning the graceful -32603 RpcError that handle_status alone would produce.

I confirmed this with a throwaway test (handler.handle_rpc_requests(rpc_request("status", None), 999) on a handler with no workers added) — it panicked with index out of bounds: the len is 0 but the index is 999 at the update_stats call site, rather than returning an error response.

Since fixing that unchecked indexing is a production-code behavior change beyond this test-only PR's scope, I left it alone here and instead added test_workers_list_get_stats_missing_worker, which exercises WorkersList::get_stats's -32603 path directly (mirroring the existing test_workers_list_login_missing_worker pattern) without going through the panicking handle_rpc_requests path.

Happy to open a separate PR/issue for the panic if that's useful — a connected worker's worker_id is normally always in range, so this likely isn't reachable in practice today, but it's a footgun for any future caller of handle_rpc_requests (or update_stats/last_seen) with an id that isn't guaranteed to be currently registered.

wiesche89

The behavioral coverage is useful and the earlier review points are addressed. The test module still feels broader and more heavily documented than the surrounding code, though. Could we trim tests that only mirror constructor field assignments, inline single-use helpers, and keep comments for non obvious behavior and setup constraints?

wiesche89 · 2026-07-12T07:25:32Z

+		let _ = fs::remove_dir_all(dir_name);
+	}
+
+	/// Path under `target/tmp/` so interrupted tests do not leave dirs in the repo root.


These helpers are only used by shared_test_chain. Could we inline them there to keep the test setup smaller?

wiesche89 · 2026-07-12T07:26:41Z

+		tx
+	}
+
+	/// A single chain instance shared by every RPC routing test below.


This comment is quite long. Could we shorten it and just note that write-path tests need their own chain?

wiesche89 · 2026-07-12T07:28:55Z

+	}
+
+	// ----------------------------------------
+	// RpcRequest / RpcResponse serde (existing)


“Existing” is review context rather than useful source documentation. Could we drop it?

wiesche89 · 2026-07-12T07:29:38Z

+		workers
+			.login(id0, "alice".to_string(), "agent-a".to_string())
+			.unwrap();
+		// Logging in again (e.g. after a reconnect) replaces the previous


This is a second login for the same worker, not a reconnect, which would create a new worker ID. Could we drop the reconnect example?

Drop constructor-mirror tests for State and Worker, inline the single-use chain dir helpers into shared_test_chain, shorten its doc comment, and remove review-context wording from comments.

test: add unit tests for stratumserver module

091bef5

Expand stratumserver coverage beyond RPC serde helpers to exercise WorkersList lifecycle, parse_params, RpcError constructors, and Handler RPC routing (login, keepalive, status, getjobtemplate, submit stale). Addresses mimblewimble#3210.

iho mentioned this pull request Jul 9, 2026

Implement missing unit tests for stratumserver #3210

Open

wiesche89 self-requested a review July 9, 2026 20:35

wiesche89 reviewed Jul 9, 2026

View reviewed changes

Comment thread servers/src/mining/stratumserver.rs Outdated

Comment thread servers/src/mining/stratumserver.rs Outdated

Comment thread servers/src/mining/stratumserver.rs Outdated

Address review: fix internal error code, fmt, test dirs

4cb8e6c

- Use JSON-RPC -32603 for RpcError::internal_error (match api/json_rpc) - Apply cargo fmt on the login invalid-params assertion - Store test chain DBs under target/tmp/ to avoid repo-root clutter

wiesche89 reviewed Jul 11, 2026

View reviewed changes

iho requested a review from wiesche89 July 11, 2026 15:39

wiesche89 reviewed Jul 12, 2026

View reviewed changes

Address review: trim test module

788923d

Drop constructor-mirror tests for State and Worker, inline the single-use chain dir helpers into shared_test_chain, shorten its doc comment, and remove review-context wording from comments.

iho requested a review from wiesche89 July 12, 2026 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: add unit tests for stratumserver module#3886

test: add unit tests for stratumserver module#3886
iho wants to merge 4 commits into
mimblewimble:stagingfrom
iho:test/stratumserver-unit-tests

iho commented Jul 9, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iho commented Jul 11, 2026

Uh oh!

wiesche89 left a comment

Uh oh!

wiesche89 Jul 12, 2026

Uh oh!

wiesche89 Jul 12, 2026

Uh oh!

wiesche89 Jul 12, 2026

Uh oh!

wiesche89 Jul 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

iho commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iho commented Jul 11, 2026

Uh oh!

wiesche89 left a comment

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iho commented Jul 9, 2026 •

edited

Loading