Skip to content

fix(translator): make worker identity TLV optional and fix unwrap panic#300

Closed
warioishere wants to merge 1 commit intostratum-mining:mainfrom
warioishere:fix/tproxy-optional-worker-identity-tlv-and-unwrap-panic
Closed

fix(translator): make worker identity TLV optional and fix unwrap panic#300
warioishere wants to merge 1 commit intostratum-mining:mainfrom
warioishere:fix/tproxy-optional-worker-identity-tlv-and-unwrap-panic

Conversation

@warioishere
Copy link

Summary

  • Add enable_worker_identity_tlv config option (default: true) to control whether the UserIdentity TLV is attached to SubmitSharesExtended messages
  • Replace .unwrap() with .ok() on UserIdentity::new() to prevent panics when user_identity exceeds the 32-byte TLV limit
  • When disabled, the translator skips TLV encoding, matching current JD-client behavior

Context

The translator panics when user_identity is a Bitcoin address (42-64 bytes) because the UserIdentity TLV has a hard 32-byte limit in extensions-sv2. This is relevant for solo mining setups where the SV2 server parses the user_identity from OpenExtendedMiningChannel to substitute the coinbase payout address. The address is correctly passed via Str0255 (up to 255 bytes) in OpenExtendedMiningChannel, but the TLV encoding on share submissions crashes due to the 32-byte constraint.

Setting enable_worker_identity_tlv = false allows using Bitcoin addresses as user_identity without crashing, at the cost of losing per-worker hashrate attribution in the TLV. The JD-client already behaves this way (it does not attach UserIdentity TLV to shares).

Closes #295

Test plan

  • cargo check passes
  • All 20 unit tests pass (cargo test -p translator_sv2)
  • Manual test: connect SV1 miner with Bitcoin address as user_identity and enable_worker_identity_tlv = false — no crash
  • Manual test: connect SV1 miner with short username as user_identity and enable_worker_identity_tlv = true — TLV attached as before
  • Verify existing configs without the new field still work (serde default = true)

Add `enable_worker_identity_tlv` config option (default: true) to
control whether UserIdentity TLV is attached to SubmitSharesExtended
messages. When disabled, the translator skips TLV encoding, matching
JD-client behavior.

Also replace .unwrap() with .ok() on UserIdentity::new() to prevent
panics when user_identity exceeds the 32-byte TLV limit (e.g. when
using Bitcoin addresses as user_identity).

Closes stratum-mining#295
@warioishere warioishere force-pushed the fix/tproxy-optional-worker-identity-tlv-and-unwrap-panic branch from 93fe7df to c48597e Compare February 27, 2026 16:42
@warioishere
Copy link
Author

Question: TLV logic seems inverted?

While working on this fix, we noticed the original TLV logic sends the UserIdentity TLV in non-aggregated mode but skips it in aggregated mode:

let tlv_fields = if is_non_aggregated() {
    // builds and attaches UserIdentity TLV
} else {
    None
};

This seems backwards to us:

  • Non-aggregated mode: each miner gets its own upstream channel with its own user_identity from OpenExtendedMiningChannel. The upstream already knows which worker submitted which share via the channel_id. The TLV is redundant here.

  • Aggregated mode: all miners share one upstream channel. The upstream has no way to distinguish individual workers from the channel_id alone. This is where the TLV would actually be useful for per-worker hashrate tracking.

Is there a reason the TLV is sent in non-aggregated mode specifically? Or should the condition be is_aggregated() instead of is_non_aggregated()?

This PR preserves the original condition (just adds the config flag and fixes the panic), but wanted to flag this for clarification.

@average-gary
Copy link
Contributor

Related: #276
Also: #197

@warioishere
Copy link
Author

closed as unnecessary and in favor of #276

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.

Translator panics on share submission when user_identity is a Bitcoin address

2 participants