Skip to content

Migrate translator to shared synchronous primitive - #530

Draft
bit-aloo wants to merge 1 commit into
stratum-mining:mainfrom
bit-aloo:2026-05-26-migrate-translator-to-shared-map
Draft

Migrate translator to shared synchronous primitive#530
bit-aloo wants to merge 1 commit into
stratum-mining:mainfrom
bit-aloo:2026-05-26-migrate-translator-to-shared-map

Conversation

@bit-aloo

@bit-aloo bit-aloo commented May 26, 2026

Copy link
Copy Markdown
Member

closes: #601

@bit-aloo
bit-aloo force-pushed the 2026-05-26-migrate-translator-to-shared-map branch 3 times, most recently from f086369 to 4f9d12a Compare May 27, 2026 12:28
Comment on lines +200 to +201
.with(|data| data.authorized_worker_name == *name)
.unwrap();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't propagate error from Sv1Server downstream message handler, considering it doesn't support TproxyError type.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved forward with UnknownId variant for now, we can extend error enum in Sv1 crate later to include some poison related variant.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not tackling that now, by doing the changes on sv1_api?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would require adding an error variant to sv1_api, which I can do. But it wouldn't really be specific to the library we would basically be adding a poison variant just because our downstream crate needs it.

Initially, I was thinking about having an associated error type, similar to what we do in handlers. But I need to think about it a bit more and consider how the APIs around this might evolve.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe I can have an universal error mapping in sv1 error enum. Maybe that could do the trick.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocked by: #664, I will pull this PR back to draft and wait for the assoc error type PR to get merged.

@bit-aloo
bit-aloo force-pushed the 2026-05-26-migrate-translator-to-shared-map branch from 4f9d12a to 26c7b24 Compare May 27, 2026 12:56
@bit-aloo
bit-aloo force-pushed the 2026-05-26-migrate-translator-to-shared-map branch 2 times, most recently from cd0fdd1 to 2ec31b8 Compare July 21, 2026 12:27
@bit-aloo
bit-aloo marked this pull request as ready for review July 21, 2026 16:30
@GitGab19

Copy link
Copy Markdown
Member

Is this aiming to close #601?

@bit-aloo

Copy link
Copy Markdown
Member Author

Is this aiming to close #601?

yup updated the description

@GitGab19

Copy link
Copy Markdown
Member

Can you solve conflicts?

@bit-aloo
bit-aloo force-pushed the 2026-05-26-migrate-translator-to-shared-map branch from a8e04fe to dcb1833 Compare July 28, 2026 15:16
@bit-aloo

Copy link
Copy Markdown
Member Author

Can you solve conflicts?

Should be good now

@GitGab19

Copy link
Copy Markdown
Member

Preliminary clanker findings:

## Issues

| # | Severity | Finding |
|---|----------|---------|
| 1 | 🔴 High | `difficulty_manager.rs`: a failed `set_difficulty` send to **one** downstream is now `TproxyError::shutdown` → cancels the global token → **whole proxy exits**. Trigger is a routine race: miner disconnects between sender clone and `send().await`. Was log-and-continue before. |
| 2 | 🔴 High | Per-downstream `hash_rate_to_target` failures (3 sites) and `try_vardiff` errors now also escalate to global shutdown; previously `continue`/ignored. One pathological miner can take down the proxy. |
| 3 | 🟡 Medium | `vardiff_future` select branch: if `handle_error_action` ever returns `Continue`, the completed future is re-polled → panic. Currently unreachable only because all escaping errors are shutdown-kind and `biased;` breaks first on cancelled tokens. Set `vardiff_enabled = false` after an error. |
| 4 | 🟢 Low | Keepalive path: `get_mut` → `with_mut_or_default` can re-create a `valid_sv1_jobs` entry for a dead channel (never cleaned up → small leak). Use `with_mut`. |
| 5 | ⚪ Nit | `Error::UnexpectedMessage("poisoned downstream data lock")` misuses a protocol error variant (×8 — extract a helper); `contains_key` + `with_mut` TOCTOU pairs in `mining_message_handler.rs` (benign today, fragile); `handle_downstream_message` now does 3–4 map lookups per SV1 message; vardiff iteration holds shard *write* guards during computation (cheap, no awaits — acceptable). |

@bit-aloo
bit-aloo force-pushed the 2026-05-26-migrate-translator-to-shared-map branch 2 times, most recently from 043ddf7 to 2f6c38b Compare July 29, 2026 10:02
@bit-aloo
bit-aloo force-pushed the 2026-05-26-migrate-translator-to-shared-map branch from 2f6c38b to b12b08f Compare July 31, 2026 11:10
Comment on lines +73 to +75
// It's safe to unwrap hashrate because we know that
// the downstream has a hashrate (we are
// doing vardiff)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you know that we are doing vardiff? Vardiff can be disabled from the config.

@GitGab19

GitGab19 commented Aug 3, 2026

Copy link
Copy Markdown
Member

Second pass of clanker review:

Review — updated revision (b12b08f)

The update addresses all substantive issues from my previous review. Two remaining
requests, plus nits:

1. Latent panic: completed vardiff/keepalive futures can be re-polled

spawn_vardiff_loop / spawn_job_keepalive_loop only exit by returning Err, so
when their select branch fires the future is complete. The vardiff_enabled = false
disarm from the previous revision is gone, so if handle_error_action ever returns
LoopControl::Continue, the next iteration re-polls the completed future →
panic ("async fn resumed after completion"), killing the task without cleanup().

Today this is unreachable — biased; ordering covers the token-cancelled paths, and
all errors escaping these loops happen to be Break-kind — but nothing enforces that.
A future Log/Disconnect-kind error propagating out of either loop becomes a
process-killing panic.

Suggestion: on Continue, re-arm with vardiff_future.set(self.clone().spawn_vardiff_loop())
(also restores the service instead of leaving it dead). At minimum, comment the
Break-only invariant on both loops.

2. sv1_api error handling in downstream_message_handler.rs — fix at the source

IsServer (sv1_api crate) hardcodes its error type to sv1_api::Error — unlike the
SV2 handler traits, there is no associated error type — which is what forces the
Error::UnexpectedMessage("poisoned downstream data lock") workaround repeated ~10×
(a poisoned lock is an internal failure, not a protocol error; the variant is only
"least wrong"). Error::UnknownID for "downstream not registered" is correct and
stays as is.

Rather than shipping the workaround, let's fix this properly now:

  • Open a PR against stratum-core giving IsServer an associated
    type Error: From<sv1_api::Error> (mirroring the SV2 handler traits). An additive
    Error::Internal(String) variant would also work, but since the enum is not
    #[non_exhaustive] either change is breaking — if we're bumping anyway, the
    associated type is the right spend and makes sv1 consistent with sv2.
  • Bump the dependency here (a git dependency on the stratum-core branch is fine
    while the release is pending) and have these handlers return TproxyError
    directly, with TproxyErrorKind::PoisonLock / DownstreamNotFound and proper
    actions — deleting the UnexpectedMessage mapping entirely.
  • Independently: hoist the two panics out of lock closures
    (VersionRollingParams::new(...).expect(...) in handle_configure,
    Notify::try_from(...).expect(...) in downstream.rs). A panic inside a closure
    is the realistic way these locks get poisoned in the first place.

If the sv1_api change lands, the duplicated .map_err closures and the
with_registered_downstream_sv1 / with_registered_downstream split collapse
naturally.

Nits (non-blocking)

  • The "first group channel" lookup via for_each + get_or_insert (3× in
    mining_message_handler.rs) scans the whole map with no early exit and silently
    picks an arbitrary entry if the single-group-channel assumption is ever violated.
    Consider a first_key() helper + warn! when len() > 1.
  • The nested with_mut(...).ok_or(...)?? chains in the group-channel branches are
    correct but dense; extracting per-channel bodies into named methods would help.

Tests

No coverage for the new error plumbing. Most valuable: force handle_vardiff_updates
to fail and assert clean shutdown (would also catch a re-poll regression), and pin
the "share dropped with Ok(()) when downstream disconnects mid-submission" behavior
this revision just fixed.


As I asked here I would prefer to make the changes on sv1_api in order to proceed in a clean way with this PR, wdyt @bit-aloo ?

@bit-aloo
bit-aloo marked this pull request as draft August 4, 2026 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor tProxy to reduce nested locking

2 participants