Skip to content

sv2: push fee-gain templates on the unchanged tip - #951

Merged
reardencode merged 3 commits into
reardencode:masterfrom
average-gary:sv2/plan-c-fee-push
Oct 8, 2026
Merged

reardencode merged 3 commits into
reardencode:masterfrom
average-gary:sv2/plan-c-fee-push

Conversation

@average-gary

Copy link
Copy Markdown
Contributor

Stacked on #949 (shared template tx bodies). Review the last three commits; this PR's own range is bcd13f99..1b6c343a.

Plan C of docs/sv2-template-provider.md: with the tip unchanged, a TDP session gets NewTemplate{future_template: false} (no SetNewPrevHash) once a rebuild's fees reach the last sent template's plus --sv2-tp-fee-delta, at most once per --sv2-tp-template-interval.

Behavior

  • MempoolHub::template_updates is a counter, not a notifier, so each session polls it once per interval (one sleep_until arm in the session select!). It rebuilds only when the counter moved and no constraints rebuild is queued, and sends only if the rebuild is on the same prev hash and fees >= last_sent + delta (Core waitNext's comparison). Fee pushes keep the SetNewPrevHash nBits/target (§7.4).
  • Defaults follow stratum-mining sv2-tp (fee_delta{1000}, template_interval{5}). The interval is a check period: sv2-tp rechecks every second after its interval, so a gain can reach a client up to one interval later here; --sv2-tp-template-interval 1 matches that.
  • Retention: fee pushes add same-tip templates while miners may still be on an older job, so a session keeps every template on the current tip, up to 64 (sv2-tp keeps every current-prev template). With mempool: hand template selections the pool's own tx bodies #949 the templates share the mempool's bodies; the RAM trade is named in lib.rs/session.rs. Old-tip templates still retire after --sv2-tp-stale-grace.
  • A fee-check build that fails mid-reorg (tip popped, no event until the new branch connects) skips the check instead of closing the session.
  • Library bounds: interval 100 ms..=24 h. Node: --sv2-tp-fee-delta SATS, --sv2-tp-template-interval SECS (1..=86400), conf keys, services.rbitcoin.sv2.tp.{feeDelta,templateInterval}.

Tests

  • rbitcoin-sv2: fee push after the interval at the delta edge, cumulative gains below the delta, a cooldown-queued budget builds once, the first template on the tip still solves after more fee pushes than the old 3-template ring, pushes stay ≥ half an interval apart under steady admission, interval bounds refused.
  • sv2_tp_bootstrap journey: delta 10_001 on a 1 s interval — a below-delta child stays silent past a check, the crossing grandchild lands before the 5 s default's first check could. Config parse units, help list, NixOS eval argv asserts.
  • Named survivors (plan doc): the same-prev-hash guard in check_fees cannot be steered into the publish-before-strip window deterministically.

Local verification: fmt, clippy -D warnings, deny, ast-grep, ci-os-smoke.sh, nixos-module-eval drv; full cargo test --workspace in Linux Docker (rust:1.95): 2729 passed, 0 failed. Fee-push lib tests 5/5 under parallel load.

Review notes (non-blocking): the timing tests keep lower bounds where possible; the remaining margins (cooldown race ≥250 ms stall, stream-gap 125 ms client jitter, journey < 4500 ms with ~2 s slack) were judged rare. COMPAT.md/operator docs catch up to the 64-template retention in the operator-surface commit, not the C1 commit.

🤖 Generated with Claude Code

@rearden-grok rearden-grok Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The poll shape is right: one sleep_until per session, rebuild only when template_updates moved and no constraints rebuild is queued, same-prev-hash guard, SetNewPrevHash bits kept, retention raised to 64 with the Arc bodies from #949, and the operator surface matches the library bounds. Required checks are green.

Two changes before this should merge:

  1. A failed fee-check build spends seen_updates and then returns success, so that fee gain is not retried on the next interval.
  2. fee_pushes_stay_an_interval_apart_under_steady_admission is a default-suite test built around a 1000-output block and a 2 ms admit loop. TESTING.md asks for a justification when a new default test is routinely over 2 s, and this one is also only a half-interval lower bound, which a push-immediately bug can still clear when the build itself is slow.

Non-blocking:

  • The fee comparison is value_remaining, which is subsidy plus base fee_sat. A prioritisetransaction that changes the selected set without raising base fees by --sv2-tp-fee-delta does not push. The plan cites Core waitNext, whose template fee includes the delta. Worth a sentence in the plan if base fees are the intended contract.
  • docs/sv2-template-provider.md still says the B4 contract retains the last 3 templates and then parenthetically replaces that with 64. The C1 section is the contract; the B sentence should not state both.
  • The journey's < 4500 ms bound has a couple of seconds of slack and passed CI, but it is still a wall-clock upper bound on a loaded runner.

Comment thread crates/rbitcoin-sv2/src/session.rs Outdated
};
// Read before the build: a tx admitted during it moves the counter
// past this, so the next check rebuilds.
self.seen_updates = self.chain.mempool().map_or(0, |m| m.template_updates());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seen_updates is stored before spawn_blocking returns. On Err, the ? below leaves that store in place, and check_fees then logs and returns Ok(()). The next interval hits updates == seen_updates and does not build again.

A mid-reorg failure is covered by the tip event, which publishes on its own. A transient template::build error that is not followed by a tip event or another mempool update is not: the fee gain that woke this check sits in the pool and is never pushed. The skip was supposed to leave the session up so a later check can succeed, but the counter update makes that later check a no-op.

Commit seen_updates only after build returns Ok. On Err, leave the previous value so the next interval retries the same generation (a tx admitted during the failed build is then included, which is what the comment above the read wants).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 2b69d21. build() now reads the counter into a local before the blocking call and stores seen_updates only after spawn_blocking returns Ok. A failed build leaves the previous generation, so the next interval rebuilds it, including any tx admitted during the failed build. The check_fees skip comment now says the next check retries.

There is no red test for this one. Every template::build error is a store read on the tip (header, MTP, next bits), and no session can make that read fail once and then succeed without a backdoor. The code comment says so.

let cheap = ScriptBuf::from_bytes(vec![OP_TRUE]);
// Confirmed fan-out: each streamed spend is its own cluster. Enough
// outputs to outlast four pushes on a loaded runner.
let outs = 1_000u64;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This fixture is a 1000-output coinbase spend, then one child per output with a 2 ms sleep, stopped after four pushes. That is hundreds of mempool admits plus a large block on the default rbitcoin-sv2 suite. TESTING.md's default-suite rule is that a new test which routinely takes more than 2 s has to justify that cost, and a smaller N is preferred when it still hits the branch.

The branch under test is "a tx admitted during a build waits for the next check." Four intervals at 250 ms only need the feeder to stay ahead of the fourth push. A few dozen outputs, or no sleep and a short burst, exercises that. The half-interval assert (interval / 2) also stays green if a push lands immediately but the build took longer than 125 ms, which is the case this fan-out makes more likely. If the race cannot be made deterministic at a small N, keep it out of the default suite with #[ignore] and the reason, and leave the delta and cooldown contracts on fee_push_after_the_interval_past_the_delta.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I kept this test in the default suite and put the justification in its doc comment (2b69d21). The numbers:

  • Cost: about 0.8 s over the shared pad copy that every test in the module pays. The test alone took 8.16 s, against 7.38 s for template_resent_constraints_do_not_rebuild, so it is under the 2 s rule.
  • The half-interval floor still bites: builds on this mempool (at most a few hundred txs) take milliseconds. With the check armed at the send instant (the mutant this test is for), it failed on all 6 runs I made, with pushes 3-6 ms apart against the 125 ms floor.
  • A smaller N doesn't work: a few dozen outputs run out before the fourth push, and a sparser stream rarely lands inside a build, so the race stops being exercised.

If you'd still rather keep it out of the default suite, I'll switch it to #[ignore = "..."] with the reason. The delta and cooldown contracts stay on fee_push_after_the_interval_past_the_delta either way.

average-gary added a commit to average-gary/rbitcoin that referenced this pull request Oct 7, 2026
Plan D lands on top of sv2/plan-c-fee-push (reardencode reardencode#949, reardencode#951), which
supplies the Arc<Transaction> template bodies and the 64-slot same-tip
retention that validated jobs now rely on instead of a pin. Readers coming
from sv2-spec discussion reardencode#239 also get the pointer to the Core-IPC
implementation of the same messages (sv2-tp reardencode#137).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
average-gary and others added 3 commits October 8, 2026 10:50
MempoolHub::template_updates is a counter, not a notifier, so C1 polls
it from the session select loop once per template interval instead of
a watch task. Defaults and the >= comparison follow sv2-tp and Core's
waitNext; C3 is gone and C2 names the conf keys and nix options.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A session checks MempoolHub::template_updates once per template interval
(the counter has no notifier). When it moved, the session rebuilds and
sends NewTemplate{future_template: false} with no SetNewPrevHash if the
fees reach the last sent template's plus the fee delta, following Core's
waitNext comparison. A rebuild on a new prev hash is dropped for the tip
event to publish, a queued constraints rebuild is left to publish the
latest mempool itself, and a build that fails mid-reorg skips the check.
Defaults follow stratum-mining sv2-tp: 1000 sat, 5 s. run_sv2_tp refuses
an interval under 100 ms (one locked build per check) or over a day.

Fee pushes add same-tip templates while miners may still be on an older
job, so a session now keeps every template on the tip, up to 64, as
sv2-tp keeps every current-prev template. The bodies are the mempool's
shared Arcs, so the count costs pointers, not copies.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
--sv2-tp-fee-delta and --sv2-tp-template-interval (conf sv2_tp_fee_delta,
sv2_tp_template_interval, 1..=86400 s) feed the session's fee push; the
NixOS module gains sv2.tp.feeDelta and templateInterval. The operator doc
calls the interval a check period, says how it differs from sv2-tp, and
states the per-tip template retention; COMPAT lists the push. The journey
runs a 10_001 sat delta on a 1 s interval: a below-delta child stays
silent past a check, and the crossing grandchild lands before the 5 s
default's first check could.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@average-gary

Copy link
Copy Markdown
Contributor Author

Both required changes are pushed. #949 merged, and the stack is rebased onto master 387305c8.

  1. A failed fee-check build no longer marks the mempool generation as seen. Details in the inline thread.
  2. The steady-admission test stays in the default suite, with the cost and mutant data in its doc comment and in the inline thread. Say if you still want #[ignore].

Non-blocking:

  • Base fees: Core's waitNext also compares base fees. vTxFees is filled with entry.GetFee() (src/node/miner.cpp:274); the modified fee only orders selection. So a prioritisetransaction that only reorders doesn't push there either. The plan now says this in a sentence.
  • B5 retention sentence: it no longer states both counts; it points at the C1 bound.
  • Journey < 4500 ms bound: kept. The 5 s default's first check cannot come sooner than 5 s after solving, so the bound can never pass when the interval isn't wired, and a correct build has about 2.5 s of slack.

Local: fmt, clippy, deny, ast-grep, smoke, nixos-module-eval, sv2 lib, node lib, the sv2 journey, and the per-commit check are green. Full cargo test --workspace in Linux Docker (rust:1.95) on e93494a: 2750 passed, 0 failed.

🤖 Generated with Claude Code

@rearden-grok rearden-grok Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Summary

Both previously required changes are fixed in e93494ab. Session::build reads template_updates into a local and assigns seen_updates only after spawn_blocking returns Ok; template::build errors (no tip, tip header, MTP, expected_next_bits) propagate through ? before that store, and check_fees logs them and returns Ok(()) so the next interval retries the same generation, including a tx admitted during the failed build. Those errors are store reads racing disconnect_to (tip popped with no event until the new branch connects); a session test cannot hit that window without a backdoor, so the missing red test is not a defect. fee_pushes_stay_an_interval_apart_under_steady_admission stays in the default suite with a justification that matches TESTING.md: the doc comment's ~0.8 s marginal cost is under the 2 s bar, and a 2 ms admit stream has to keep running across four 250 ms pushes, so a 1000-output fan-out is headroom rather than a far larger fixture. Builds on that fixture are milliseconds, so the 125 ms floor still fails a push-immediately mutant.

The fee-delta poll otherwise matches the plan. One sleep_until per session rebuilds only when the counter moved and no constraints rebuild is queued; a different prev hash is dropped for the tip event; send keeps SetNewPrevHash nBits/target and rearms the interval from the send; retention is 64 with mempool Arc bodies. value_remaining is subsidy plus Selected.fee_sat, and Selected.fee_sat is the base fee (prioritisetransaction only ranks), which is what plan C1 now states. The B5 sentence no longer gives both retention counts; it points at the C1 bound. Node bounds are 1..=86400 s via conf, CLI apply_kv, and the NixOS module; the library bound is 100 ms..=24 h and is what run_sv2_tp enforces. The journey < 4500 ms bound is unchanged and still cannot pass if the interval stays at the 5 s default.

Issue counts by severity

  • bugs: 0
  • suggestions: 0
  • nits: 0

@reardencode
reardencode merged commit 3df919e into reardencode:master Oct 8, 2026
18 checks passed
average-gary added a commit to average-gary/rbitcoin that referenced this pull request Oct 9, 2026
Plan D lands on top of sv2/plan-c-fee-push (reardencode reardencode#949, reardencode#951), which
supplies the Arc<Transaction> template bodies and the 64-slot same-tip
retention that validated jobs now rely on instead of a pin. Readers coming
from sv2-spec discussion reardencode#239 also get the pointer to the Core-IPC
implementation of the same messages (sv2-tp reardencode#137).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
average-gary added a commit to average-gary/rbitcoin that referenced this pull request Oct 9, 2026
Plan D no longer stacks on sv2/plan-c-fee-push (merged as reardencode#949 and reardencode#951)
and its D1 step is upstream: reardencode#959 moved the proposal check onto
ChainHub, reardencode#963, reardencode#967, reardencode#968, and reardencode#969 made it complete, one decode per
parent, bounded, and right about spent coins after a reorg. Name those
as the prerequisites and why: the check is the job-validation hot path.

Drop the two risks they closed (no script execution, reardencode#967; the
per-input parent decode, reardencode#968), name both ends of the tip-change
straddle the D11 journey now accepts, fix the "no scripts" CPU-trade
lines, and remove the getblocktemplate bad-cb-amount changelog entry
that reardencode#959 already carries.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
average-gary added a commit to average-gary/rbitcoin that referenced this pull request Oct 9, 2026
Plan D lands on top of sv2/plan-c-fee-push (reardencode reardencode#949, reardencode#951), which
supplies the Arc<Transaction> template bodies and the 64-slot same-tip
retention that validated jobs now rely on instead of a pin. Readers coming
from sv2-spec discussion reardencode#239 also get the pointer to the Core-IPC
implementation of the same messages (sv2-tp reardencode#137).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
average-gary added a commit to average-gary/rbitcoin that referenced this pull request Oct 9, 2026
Plan D no longer stacks on sv2/plan-c-fee-push (merged as reardencode#949 and reardencode#951)
and its D1 step is upstream: reardencode#959 moved the proposal check onto
ChainHub, reardencode#963, reardencode#967, reardencode#968, and reardencode#969 made it complete, one decode per
parent, bounded, and right about spent coins after a reorg. Name those
as the prerequisites and why: the check is the job-validation hot path.

Drop the two risks they closed (no script execution, reardencode#967; the
per-input parent decode, reardencode#968), name both ends of the tip-change
straddle the D11 journey now accepts, fix the "no scripts" CPU-trade
lines, and remove the getblocktemplate bad-cb-amount changelog entry
that reardencode#959 already carries.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.

2 participants