fix(pool_sv2): validate extended channel opens before minting an extranonce prefix - #746
Merged
Merged
Conversation
…anonce prefix The extended path minted an extranonce prefix and only then checked the client's `user_identity`, so every rejected `OpenExtendedMiningChannel` burnt a prefix permanently. The allocator is `server_id || counter` over `POOL_ALLOCATION_BYTES`, two of which are the fixed server id, leaving a 16-bit counter — 65,535 prefixes for the life of the process, and none are ever handed back. An unauthenticated client looping opens with a garbage identity therefore exhausted the space and stopped every honest miner from opening a channel until `pool_sv2` restarted. The safe order already existed in the same file: the standard path runs `PayoutMode::try_from` first and `next_prefix_standard` second. This makes the extended path match, via `validate_and_allocate_extended`, which also hoists the nominal-hashrate and max-target checks — the other two client-controlled fields that can reject an open, and previously the same leak by a different message field. `ExtendedChannel::new_for_pool` keeps its own branches as a backstop. Exhaustion also gets its own `error_code`. Every allocator error collapsed into `min-extranonce-size-too-large`, which blames the client's requested size and points whoever investigates away from the real cause. Closes #744
19 tasks
defenwycke
added a commit
that referenced
this pull request
Aug 23, 2026
Seven commits since v1.11.26, none of which change payout behaviour — the no-vote gate at 964_100 is untouched and still fires on schedule. - #740 mesh: bind timestamp and message type into the envelope signature, and persist per-sender replay floors (H-11). ⚠ SHIPS DORMANT — `MESH_ENVELOPE_V2_HEIGHT = u64::MAX`. This release is what makes every node able to VERIFY a v2 envelope; arming is a later, separate change, and arming it before the whole fleet is on this binary partitions the mesh. - #742 authenticate the share webhook and verify its PoW at ingest (#603). The `[share_webhook] secret` is already staged on all eight nodes, inert until this binary lands. - #741 address convergence replies to their requester, and count the frames the outbound queue sheds (#647). - #745 refuse a ghost-pool deploy to a node whose pool_sv2 cannot sign share batches. - #746 validate extended channel opens before minting an extranonce prefix (#744) — a remote client with a garbage identity could exhaust the prefix space. - #749 remove the assumeUTXO fast-sync path entirely (#748). Onboarding no longer depends on one node serving a 9 GB snapshot from a hardcoded IP. - #743 make the regtest integration tests actually run instead of passing by skipping. Both lockfiles refreshed — `fuzz/` is a separate workspace that `cargo update --workspace` at the root does not reach.
defenwycke
added a commit
that referenced
this pull request
Aug 28, 2026
…tranonce prefixes Opening the channel on every `mining.subscribe` was wrong in a way the pool already had a comment about. The pool's extended allocator is `server_id || counter` with a two-byte server id, leaving a 16-BIT counter — 65,535 prefixes for the lifetime of the process, never handed back (#746). Measured on vm1: `sri-pool` up 28.5h, counter at `0x1be3` = 7,139, so ~250/h and roughly ten days of uptime before every channel open starts failing. Two problems with opening unconditionally: - Subscribe-only probes (~15% of connections) would each burn a prefix despite never mining, shortening that window for no benefit. - #746 deliberately moved allocation AFTER validation so an UNAUTHENTICATED client cannot burn the space. A subscribe arrives before any authorize, so opening on it immediately hands that capability straight back — a drive-by prober could exhaust the pool for every honest miner. That is a security regression, not a capacity one. So the open is debounced by `SUBSCRIBE_OPEN_DEBOUNCE` (300ms), sized from measured client behaviour rather than guessed: a pipelining miner's authorize follows its subscribe by ~12ms and a marketplace capability probe disconnects ~10-30ms in. Neither reaches the timer, so the pipelining path is byte-identical to today and probes cost nothing. Only a client still holding the connection open — the serialising shape this exists to serve — opens early, and it waits the channel-open round trip (~50-300ms) instead of the old 1500ms, for a real extranonce instead of an unusable one. Tests: 62 translator, 35 pool, clippy clean. Claude-Session: https://claude.ai/code/session_01ResUgwsjEwGZ99vZn4hu4R
defenwycke
added a commit
that referenced
this pull request
Aug 28, 2026
) * fix(pool_sv2): let a provisional channel's TLV carry the payout address A serialising SV1 client — proxies and rented-hashrate marketplaces — waits for the `mining.subscribe` RESPONSE before it will authorise. That response has to carry the real, channel-allocated extranonce, and only the pool can mint one, so the channel must open before `mining.authorize` arrives and its identity cannot hold the miner's address. The address then has to travel per share in the Worker-Specific Hashrate Tracking TLV. This is the pool half of that, and it ships first: a translator that opens channels provisionally is only safe against a pool that already understands them. Rolled the other way round, `build_webhook_user_identity` would splice a worker onto the sentinel and credit the address portion — `sri` — to nobody, which is how #447 misattributed ~395 shares in July. - `PROVISIONAL_CHANNEL_IDENTITY = "sri/donate/provisional"`. It has to be a shape `PayoutMode::try_from` already parses or the channel open is rejected outright, and three segments so it cannot collide with a miner authorising as plain `sri/donate`. Pinned by a test. - `build_webhook_user_identity` returns `Option<String>` and decides from the CHANNEL identity, never by inspecting the TLV. A worker name may legitimately contain a dot (`addr.farm1.rig1` yields `farm1.rig1`, #481), so "the TLV looks dotted" does not imply it holds an address — keying on that would silently reassign `farm1` as a payout target. There is a test for exactly that shape. - A provisional channel whose TLV is absent, empty, or a bare worker resolves to `None` and the share is not credited. Splicing would have produced `sri/donate/provisional.rig1` — an address of `sri` — which looks entirely normal in the logs while paying nobody. `attributable` now folds in that resolution. - The block path stays deliberately ungated: a block is always reported, falling back to the raw channel identity so the record still points somewhere traceable. Behaviour for every channel opened on `mining.authorize` is unchanged — the address still comes from the channel and the worker from the TLV. Claude-Session: https://claude.ai/code/session_01ResUgwsjEwGZ99vZn4hu4R * fix(translator): open the channel on subscribe so rented hashrate gets a real extranonce A serialising SV1 client waits for the `mining.subscribe` RESPONSE before it will authorise. The channel opens on authorize, so those clients deadlock, and the 1.5s fallback answers subscribe with an 8-byte all-zero PLACEHOLDER extranonce while the real prefix is 12 bytes. That placeholder is not merely approximate, it is unusable. The coinbase declares a 91-byte scriptSig ending in `OP_PUSHBYTES_20`, reserving exactly 20 bytes: real: en1 12 + en2 8 = 20 ✅ placeholder: en1 8 + en2 8 = 16 ❌ 4 bytes short so the client builds a coinbase shorter than its own length byte — malformed transaction, wrong merkle root, every share invalid. `mining.set_extranonce` cannot rescue it: that is an OPTIONAL extension, and these clients never send `mining.extranonce.subscribe` (0 occurrences in 24h of fleet logs, 0 on the wire). Behind `open_channel_on_subscribe`, the channel opens at subscribe under `PROVISIONAL_CHANNEL_IDENTITY`, so the subscribe response carries the real prefix first time, for pipelining and serialising miners alike. The address then cannot live in the channel identity, so it travels per share in the worker TLV, which now carries the full `<address>.<worker>` in that mode. The pool half (562225d) uses it verbatim and fails closed if it is missing. - `tlv_compatible_username` REFUSES rather than truncates. A cut identity is not a shorter identity: it mangles the payout address, or collapses two workers sharing a prefix onto one `miner_id`. Its private 32-byte cap was a third copy of a limit owned by `extensions_sv2::MAX_USER_IDENTITY_LENGTH`; two copies of that constant drifting is what sent shares out with no TLV at all and misattributed ~395 of them under #447. It now defers to the single owner. - `PROVISIONAL_CHANNEL_IDENTITY` lives in `extensions_sv2` alongside the extension it belongs to, imported by both binaries. Duplicating it is the same failure mode as the length constant, so it gets one owner from the start. - `channel_open_requested` guards the open. `channel_id` cannot: it is set only when `OpenExtendedMiningChannelSuccess` returns, so a pipelining miner's authorize lands mid-flight and would burn a second upstream channel. ⛔ ROLLOUT GATE, default OFF, so this binary is safe to roll at any time. `pool_sv2` must understand provisional channels on EVERY node before it is switched on anywhere; rolled the other way round the pool credits `sri` — nobody. Roll `pool_sv2` fleet-wide, then flip the flag. Remove the flag once the fleet is on it. The blockers that shelved #447 are both resolved on `main`: the duplicated `MAX_USER_IDENTITY_LENGTH` (#422) and `set_difficulty` never reaching the wire (#455, fixed by #575). Tests: 62 translator, 35 pool, clippy clean. Not yet exercised end-to-end against a live pool — the handshake needs a canary with the flag on. Claude-Session: https://claude.ai/code/session_01ResUgwsjEwGZ99vZn4hu4R * style: rustfmt the pool attribution change Claude-Session: https://claude.ai/code/session_01ResUgwsjEwGZ99vZn4hu4R * fix(translator): debounce the subscribe-open so probes cannot burn extranonce prefixes Opening the channel on every `mining.subscribe` was wrong in a way the pool already had a comment about. The pool's extended allocator is `server_id || counter` with a two-byte server id, leaving a 16-BIT counter — 65,535 prefixes for the lifetime of the process, never handed back (#746). Measured on vm1: `sri-pool` up 28.5h, counter at `0x1be3` = 7,139, so ~250/h and roughly ten days of uptime before every channel open starts failing. Two problems with opening unconditionally: - Subscribe-only probes (~15% of connections) would each burn a prefix despite never mining, shortening that window for no benefit. - #746 deliberately moved allocation AFTER validation so an UNAUTHENTICATED client cannot burn the space. A subscribe arrives before any authorize, so opening on it immediately hands that capability straight back — a drive-by prober could exhaust the pool for every honest miner. That is a security regression, not a capacity one. So the open is debounced by `SUBSCRIBE_OPEN_DEBOUNCE` (300ms), sized from measured client behaviour rather than guessed: a pipelining miner's authorize follows its subscribe by ~12ms and a marketplace capability probe disconnects ~10-30ms in. Neither reaches the timer, so the pipelining path is byte-identical to today and probes cost nothing. Only a client still holding the connection open — the serialising shape this exists to serve — opens early, and it waits the channel-open round trip (~50-300ms) instead of the old 1500ms, for a real extranonce instead of an unusable one. Tests: 62 translator, 35 pool, clippy clean. Claude-Session: https://claude.ai/code/session_01ResUgwsjEwGZ99vZn4hu4R
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
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.
Closes #744.
The bug, re-verified
Confirmed against
mainbefore touching anything, and the numbers were measured rather than assumed:bins/pool-sv2/src/lib/channel_manager/mining_message_handler.rsminted the extranonce prefix (next_prefix_extended) and only then ranPayoutMode::try_from(user_identity). On rejection it returnedOpenMiningChannelErrorand the prefix was gone for good.ChannelManager::newbuilds the extended allocator withrange_1 = 0..POOL_ALLOCATION_BYTES(4 bytes) and a static prefix ofserver_id.to_be_bytes()—server_idis au16, so two of the four bytes are fixed and the counter is 16 bits. The new testextended_prefix_space_is_only_sixteen_bitsdrains a real factory and asserts the count: 65,535 prefixes for the life of the process.is_client_authorizedreturnsOk(true)unconditionally, so the loop needs no credentials.One correction to the issue's reading of the allocator:
next_prefix_extendedchecksrequired_lenbefore it increments, somin-extranonce-size-too-largewas never the leaking branch. Every leak came from a check performed after the call.The fix
Both call orders now live in one place,
validate_and_allocate_extended, which the extended path calls: identity first, then the client-controlled hashrate/target checks, then — last — the allocation. This is the order the standard path has always used (PayoutMode::try_fromat the top,next_prefix_standardafter it).Reordering rather than reclaiming, as the issue asks: a reclaim leaves a window and has to be right on every error branch, whereas nothing is allocated to reclaim if the request never gets that far.
One extension beyond the issue, called out deliberately
Reordering the identity check alone would have left the same denial of service reachable through a different field.
ExtendedChannel::new_for_poolrejects a channel onnominal_hash_rate(any negative value —hash_rate_to_targetreturnsNegativeInput) and onmax_target, and both of those run after the prefix is minted, for exactly the same cost to the attacker. A fix that closed only the identity path would have been cosmetic, so those two checks are hoisted ahead of the allocation as well. They duplicate the constructor's first two checks on purpose — the constructor cannot run before the prefix exists, because it takes the prefix — and the constructor keeps its own branches as a backstop.The constructor's remaining rejections (
ExtranoncePrefixTooLarge,ScriptSigSizeTooLarge) are determined by the pool's own configuration and constants, not by anything the client sends, so they are not an attacker lever.Error code
Every allocator failure previously collapsed into
min-extranonce-size-too-large, which blames the client's requested size and points an investigation away from the real cause. Exhaustion now returnsextranonce-space-exhausted.On the spec:
OpenMiningChannel.Error.error_codeis a free-form human-readableStr0255, and the mining spec enumerates onlyunknown-userandmax-target-out-of-rangeas examples. Roles are expected to extend that set, and this pool already sendsinvalid-user-identity,invalid-nominal-hashrate,min-extranonce-size-too-largeandstandard-channels-not-supported-for-custom-work. The new code follows the same kebab-case convention and fitsStr0255.Tests, and the mutations that prove they can fail
Five tests in
extranonce_allocation_tests, all built on a factory with the exact geometryChannelManager::newuses:extended_prefix_space_is_only_sixteen_bitsrejected_identities_never_consume_a_prefixrejected_hashrate_and_target_never_consume_a_prefixexhaustion_and_oversized_request_report_different_codesthe_extended_allocator_has_exactly_one_call_siteEvery test was mutated and confirmed to fail:
rejected_identities_never_consume_a_prefixFAILED,rejected_hashrate_and_target_never_consume_a_prefixFAILEDMaxValueReachedmapped back tomin-extranonce-size-too-largeexhaustion_and_oversized_request_report_different_codesFAILED,extended_prefix_space_is_only_sixteen_bitsFAILEDrejected_hashrate_and_target_never_consume_a_prefixFAILEDnext_prefix_extendedcall added to the handlerthe_extended_allocator_has_exactly_one_call_siteFAILEDThe last one exists because the four runtime tests exercise the helper, not the handler, and re-inlining an allocation ahead of the validation is precisely how the bug was written the first time. Building a
ChannelManagerin a unit test needs a live downstream connection, a template and a prev-hash, so the call site is asserted against the source instead.cargo test -p pool_sv2 --lib: 29 passed, 0 failed.cargo clippy -p pool_sv2 --tests -- -D warningsandRUSTDOCFLAGS="-D warnings" cargo doc -p pool_sv2 --no-depsboth clean.Known residual, not fixed here
If a client opens an extended channel before the pool has received its first template, the prefix is still minted before the
last_future_template/last_new_prev_hashguards reject it. That path disconnects the client and only exists in a pre-template startup window, so it is not an attacker lever, but the standard path checks both at the top and the extended path could be brought into line separately.