Skip to content

channels_sv2: enforce share nTime bounds, bound seen_shares, and fix job-ID/chain-tip/coinbase validation gaps (plus template_distribution_sv2 docs fix)) - #2303

Open
plebhash wants to merge 7 commits into
stratum-mining:mainfrom
plebhash:2026-08-17-channels-sv2-hardening
Open

channels_sv2: enforce share nTime bounds, bound seen_shares, and fix job-ID/chain-tip/coinbase validation gaps (plus template_distribution_sv2 docs fix))#2303
plebhash wants to merge 7 commits into
stratum-mining:mainfrom
plebhash:2026-08-17-channels-sv2-hardening

Conversation

@plebhash

@plebhash plebhash commented Aug 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

@plebhash
plebhash force-pushed the 2026-08-17-channels-sv2-hardening branch 4 times, most recently from ba027e4 to 0f52489 Compare August 18, 2026 15:16
@plebhash plebhash changed the title channels_sv2: enforce share nTime bounds, bound seen_shares, and fix job-ID/chain-tip/coinbase validation gaps channels_sv2: enforce share nTime bounds, bound seen_shares, and fix job-ID/chain-tip/coinbase validation gaps (plus template_distribution_sv2 docs fix)) Aug 18, 2026
@plebhash
plebhash force-pushed the 2026-08-17-channels-sv2-hardening branch 2 times, most recently from cd3ccb6 to 0a508c6 Compare August 18, 2026 17:57
@plebhash
channels-sv2: reject semantically non-coinbase txs in merkle_root_fro…
36d2297 
…m_path

A syntactically valid transaction whose input spends a non-null outpoint
was accepted as a coinbase and yielded a Merkle root, letting a peer
supply an ordinary transaction as coinbase prefix/suffix and have miners
work on a job whose first transaction cannot be a valid block coinbase.

Enforce the coinbase invariant (exactly one input spending the null
outpoint) via Transaction::is_coinbase before deriving the txid. All
five callers already handle the None return.

Closes stratum-mining#2300
@plebhash
channels-sv2: make JobIdFactory::next wrap explicitly on u32 overflow
d916f4c 
next() incremented with unchecked arithmetic, so its overflow behavior
depended on build flags: after u32::MAX allocations it would panic with
overflow checks enabled (terminating the embedding server task) or wrap
silently in ordinary release builds.

Make the wrap explicit and documented: IDs restart from 0 after
u32::MAX, identically across build profiles. Reusing an ID after 2^32
allocations is safe because job stores only retain recent jobs (future
jobs are consumed on activation, past/stale jobs are flushed on every
chain-tip transition), so no store can still track the previous holder
of a wrapped ID by the time it comes around again.

Closes stratum-mining#2302
@plebhash
template-distribution: correct SetNewPrevHash docs on future template…
365924d 
… ordering

The rustdoc omitted the Template Distribution Protocol's requirement that
a Template Provider send at least one future NewTemplate before every
SetNewPrevHash, and contradicted itself on how binding template_id is:
the struct doc said it *should* be set *if* a future template had
previously been sent, while the field doc said it *must* be identical to
a previously sent NewTemplate.

The conditional reading makes a SetNewPrevHash with no preceding future
template look like a normal flow. It is not: spec section 7.3 puts the
ordering requirement on the upstream, so the absence of a queued future
template signals a non-conforming peer.

State the precondition, describe what template_id selects among the
queued future templates, and note the direction of the requirement so
the conditional phrasing cannot be read as making template_id optional.

Docs only, no API or behavior change. Related to stratum-mining#2301.
@plebhash
channels-sv2: recover from SetNewPrevHash with no queued future template
fdea81c 
The Template Distribution Protocol requires a Template Provider to send
at least one future NewTemplate before every SetNewPrevHash, so a
SetNewPrevHash arriving with no future job queued means the peer broke
the protocol. Group and standard server channels responded by returning
TemplateIdNotFound and never recording the new chain tip, which leaves
the channel worse off than the violation itself: the chain-tip fields
are self-contained and do not depend on template_id resolving, so
discarding them buys nothing.

The two resulting states are both unrecoverable. On an established
channel the tip stays on the previous block, so every later non-future
NewTemplate builds jobs committed to a dead prev_hash and validate_share
keeps crediting work that can never form a block. On a channel that
never received a tip, the next non-future NewTemplate fails with
ChainTipNotSet. TemplateIdNotFound also carries no error code, so the
documented recommendation of shutdown applies and an embedding pool may
terminate a shared process over one peer's misbehavior.

Mirror the extended channel, which already recovers here: with no future
jobs queued, standard demotes the active job, marks past jobs stale,
clears the target mappings and seen shares, and updates the chain tip;
group drops the active job outright (it never validates shares against
stored jobs, so routing it through the stale set would only pin its
retired extranonce prefix for an extra tip) and updates the chain tip.
TemplateIdNotFound remains for the case where future jobs exist but none
matches.

Closes stratum-mining#2301
@plebhash
channels-sv2: enforce each job's own min_ntime in client share valida…
6d9de36 
…tion

Client channels validated share nTime only against the chain tip's
min_ntime, but an immediately-active job carries its own min_ntime,
which may be later than the tip's minimum. A share in the gap
(chain_tip.min_ntime <= ntime < job.min_ntime) was hashed and returned
as Valid/BlockFound, letting a proxy/JDC forward out-of-window work
upstream or surface a bogus block candidate.

Enforce the per-job bound alongside the chain-tip bound on both standard
and extended client channels; for jobs activated from the future queue
the two are equal, so the effective bound is max(tip, job) either way.

Also repairs the coinbase_tx_suffix test vector in
test_share_validation_ntime_below_min_ntime, which carried one extra
zero byte: the corrupt suffix made merkle-root computation fail, so the
test passed through the same Invalid error for the wrong reason.

Closes stratum-mining#2298
@plebhash
channels-sv2: enforce an nTime upper bound in validate_share (all cha…
c6a2ef6 
…nnel types)

No validate_share implementation enforced any upper bound on share
nTime: values as large as u32::MAX were hashed and returned as
Valid/BlockFound whenever they met the target, corrupting block
accounting with headers bitcoind rejects (consensus caps block
timestamps at ~2h in the future).

Introduce MAX_FUTURE_BLOCK_TIME (mirroring Bitcoin Core's constant) and
reject shares with ntime > chain_tip.min_ntime() + MAX_FUTURE_BLOCK_TIME
(saturating) on all four channel types. The bound is anchored at
chain-tip receipt since the crate is no_std and has no clock: it is
deliberately looser than the spec's elapsed-time window but strictly
enforceable; embedding applications with a time source can additionally
enforce the spec-exact window. Rationale and edge cases are documented
on the constant.

Three server-standard tests carried pre-mined shares whose ntime sat ~4h
past the tip; their chain tips are re-anchored within the consensus
window (the header hash does not depend on the tip timestamp, so the
pre-mined vectors stay valid).

Closes stratum-mining#2299
@plebhash
channels-sv2: bound the accepted-share dedup cache (seen_shares)
4bbde98 
seen_shares inserted every distinct accepted share hash and was only
cleared on chain-tip transitions, whose timing the peer controls (a
zero-hashrate channel at maximum target makes essentially every unique
header pass validation), so memory grew linearly at message speed for
the lifetime of a tip on both server and client channels.

A plain evict-oldest cap is not safe on the server: evicting a hash
re-admits its duplicate, re-enabling the replay/accounting fraud the set
exists to prevent. The two sides therefore get different bounds:

- server: a budget derived from the channel's pool-configured
  expected_share_per_minute (x WORST_CASE_TIP_MINUTES x
  SEEN_SHARES_MARGIN, clamped into [MIN_SEEN_SHARES_CAP,
  MAX_SEEN_SHARES_CAP] so absurd configuration can neither disable dedup
  nor unbound memory). The budget is a hard limit: once full,
  validate_share returns the new
  ShareValidationError::SeenSharesBudgetExhausted. It carries no error
  code because no SubmitShares.Error code fits, so per that enum's
  contract the embedding application should disconnect the client --
  closing the abusing channel, not the process. Deriving the budget is
  worthwhile here because the limit is load-bearing: it must stay
  unreachable for any honestly configured rate, and a flat cap that is
  safe at 1 share/min is reachable within an hour of tip lifetime at
  100 shares/min.

- client: a flat MAX_SEEN_SHARES of 4096 hashes (128 KB) with
  oldest-first eviction, held in a single VecDeque. The bound has to be
  affordable on the smallest supported device, since the client module
  is documented as no_std/embedded-capable and an adversarial upstream
  advertising a trivial target can drive the cache to its bound at
  message speed. A client cache also does not need a whole tip's worth
  of history: it catches a share source re-sending work it already sent,
  which arrives within seconds, so 4096 covers ~11h of a typical
  6 shares/min channel. No derivation from target/hashrate: those inputs
  are upstream-controlled, so a derived bound cannot constrain a hostile
  upstream, and eviction is locally harmless (a replayed evicted hash
  double-counts one local statistic; nothing is paid out and nothing
  reaches upstream that it won't independently dedup).

Closes stratum-mining#2297
@plebhash
plebhash force-pushed the 2026-08-17-channels-sv2-hardening branch from 0a508c6 to 4bbde98 Compare August 18, 2026 18:29
@plebhash plebhash mentioned this pull request Aug 18, 2026
Draft
@plebhash
plebhash marked this pull request as ready for review August 18, 2026 18:32
@plebhash

plebhash commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

ready for review

@plebhash plebhash changed the title channels_sv2: enforce share nTime bounds, bound seen_shares, and fix job-ID/chain-tip/coinbase validation gaps (plus template_distribution_sv2 docs fix)) channels_sv2: enforce share nTime bounds, bound seen_shares, and fix job-ID/chain-tip/coinbase validation gaps (plus template_distribution_sv2 docs fix)) Aug 18, 2026
@bit-aloo

bit-aloo commented Aug 19, 2026

Copy link
Copy Markdown
Member

clankers findings:

Findings

  1. Wrong constant in the client-side doc — sv2/channels-sv2/src/lib.rs:88
    /// Client channels bound seen_shares at [MAX_SEEN_SHARES_CAP] outright and evict oldest-first;
    Clients bound at client::MAX_SEEN_SHARES (4 096 ≈ 128 KB), not MAX_SEEN_SHARES_CAP (1 << 20 ≈ 32 MB). This contradicts the doc six lines above on MAX_SEEN_SHARES_CAP itself ("Client channels do not use
    this: they bound their cache at the far smaller client::MAX_SEEN_SHARES") and inverts the embedded-affordability argument the whole client design rests on. It's a live rustdoc link, so a reader
    following it lands on the wrong number.
  1. The protocol violation is now completely silent — server/standard.rs:576, server/group.rs:339
    Both no-future-jobs branches return Ok(()). The commit message argues (correctly) that the tip is usable and rejecting it wedges the channel — but the previous TemplateIdNotFound was also the only
    signal the embedding pool got that the TP is non-conforming. Now there is no return value, no log, nothing. tracing is already imported in both files (use tracing::debug), so a warn! in that branch
    costs nothing and preserves the observability the error return used to carry.
  1. 6d9de36's fix stops at the client; server extended has the same gap
    Server extended jobs built from templates get min_ntime = chain_tip.min_ntime() (factory.rs:259, :352), so no gap there — but custom jobs take the client-supplied value verbatim (factory.rs:535,
    Sv2OptionOwned::new(Some(set_custom_mining_job.min_ntime))), and server/extended.rs::validate_share checks only the chain-tip bound (:824, :833), never job.min_ntime. A share in [chain_tip.min_ntime,
    custom_job.min_ntime) is accepted and credited against a window the downstream JDC explicitly declared. Lower impact than the client case (nothing consensus-invalid results), but it's the same class,
    and the series otherwise applies its bounds uniformly across all four channel types.
  1. Both new bounds assume mainnet block cadence
    MAX_FUTURE_BLOCK_TIME is anchored at chain_tip.min_ntime() and WORST_CASE_TIP_MINUTES = 600 assumes a tip turns over well inside 10h. On regtest/CI or any harness where the tip doesn't advance, a
    long-lived tip both tightens the nTime window below consensus and lets an honest channel walk into SeenSharesBudgetExhausted (7 200 shares at 6/min ≈ 20h). The MAX_FUTURE_BLOCK_TIME doc already concedes
    the aging-tip edge; the seen_shares_budget doc frames hitting the budget as unambiguously "a protocol-abuse signal", which is only true under mainnet-like cadence. Worth one sentence there.
  1. Nits
  • server/standard.rs:2699 — comment says 1 share/min -> 1200 hashes; the actual budget is 4 096, because MIN_SEEN_SHARES_CAP clamps it. The test uses seen_shares_budget() so it passes regardless, but
    the comment misstates the mechanism it's demonstrating.
  • lib.rs:58 — MIN_SEEN_SHARES_CAP is documented as "4 096 hashes ≈ 128 KB" using payload math, while seen_shares_budget two constants later deliberately uses measured HashSet math ("rounds to a power of
    two at ~7/8 load"). By that same measure this one is ~270 KB (8 192 slots × 33 B). Pick one convention.
  • client/share_accounting.rs:112 — "at this size a linear scan costs less than a hash lookup would (random hashes diverge in the first byte)". A 4 096-entry scan is ~two orders of magnitude slower than
    a SipHash lookup; early divergence makes each comparison cheap, not the scan. It doesn't matter in practice (two scans per share, tens of µs, on a client), and the memory half of the rationale —
    storing each hash once instead of set-plus-order-queue — is sound on its own. Just drop the CPU claim.

bit-aloo

This comment was marked as off-topic.

Sign in to view

bit-aloo
bit-aloo requested changes Aug 19, 2026
View reviewed changes

@bit-aloo bit-aloo left a comment

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.

.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment