perf(rpc): serve getblocktemplate from a precomputed block template - #11371
perf(rpc): serve getblocktemplate from a precomputed block template#11371upbqdn wants to merge 6 commits into
Conversation
And one more auto-invalidated finding. Analyzed four files, diff |
|
The red That test asserts zallet's visible mature-coinbase count is within 5 of the tip. It fails with a different count on every run: 34 on zallet-interop-request run 33470577537 hours before this branch existed, then 52 and 44 on the two runs here. In each case the preceding assertion — |
Every `getblocktemplate` call read the state and the mempool, ran ZIP-317 transaction selection, and built a coinbase transaction, which runs a shielded proof when the miner address has a shielded component. Miners short-poll far more often than the chain tip or the mempool change, so each call paid that cost again. `RpcImpl::spawn_block_template_updater()` spawns a task that keeps a template for the current tip ready, and `getblocktemplate` serves it without touching the state or the mempool. The task publishes a coinbase-only template as soon as the tip changes, then replaces it with one that contains mempool transactions, and refreshes it every `MEMPOOL_LONG_POLL_INTERVAL` seconds. A precomputed template can be a few seconds behind the mempool, which costs the miner the fees of the transactions that arrived in the meantime. It is never behind the chain: the RPC ignores a template whose previous block hash isn't the current tip, and builds one itself instead. Handlers whose miner parameters were overridden after cloning, as `generatetoaddress` and `generate` do, don't share the precomputed template or the coinbase cache, so they never serve a coinbase built for another address or another set of coinbase data.
…g poll Each `getblocktemplate` long-poll iteration precomputed the coinbase for the next tip on a blocking thread, so concurrent long polls each ran their own shielded proof, and every 5-second iteration started another one. The block template updater task already builds that coinbase once per tip and shares it, so the long-poll wait now serves the precomputed template on a tip change, and falls back to building a template from the state and the mempool. This also drops `BlockTemplateResponse::new_internal()`'s precomputed-coinbase parameter, which no caller passes now, leaving the coinbase cache as the only way a template reuses a coinbase. Closes #10747.
…e tip channel The state's write task publishes a committed block to the read state before it updates the chain tip channel, so between those two sends `ReadRequest::Tip` and `ReadRequest::ChainInfo` report the new tip while `latest_chain_tip` still reports its parent. `getblocktemplate` validated the precomputed template against the channel, so in that interval it served a template for a chain Zebra had already extended, and every share a miner computed on it was wasted. Validate against `ReadRequest::Tip` instead, which reads the same non-finalized state channel that `ReadRequest::ChainInfo` builds templates from, so the two cannot disagree about the tip. The read is skipped when no template has been published, so nodes without the updater task don't pay for it.
5216041 to
ae626ac
Compare
|
Pushed a third commit: the tip check now reads The state's write task publishes a committed block to the read state before it updates the chain tip channel (
The first commit's changelog entry is reworded to match: a call validates the tip against the state rather than skipping state reads entirely. |
conradoplg
left a comment
There was a problem hiding this comment.
Some Claude findings, haven't fully verified them:
- Long polling degenerates into an unthrottled busy loop (high)
zebra-rpc/src/methods.rs:2566 and :2723 serve templates from the precompute cache; zebra-rpc/src/methods.rs:2576-2657 builds them from a fresh state+mempool read. Long-poll "has anything changed?" is now
evaluated against two independent sources that never agree, so it oscillates:
- Client long-polls with id S (from the fallback path). Top-of-function check: cache holds P, P != S → returns P immediately.
- Client long-polls with P. Top check: P == P → None (methods.rs:1075) → falls into the loop → fresh mempool read yields S'; S' != P → breaks and returns immediately.
- Repeat, with no wait on either side.
The long poll ID is (tip_height, tip_hash, max_time, mempool tx checksum). max_time is median_time_past + 90min (zebra-state/src/service/read/difficulty.rs:230), constant per tip, so the only differing
component is the mempool set — and the precomputed snapshot is up to MEMPOOL_LONG_POLL_INTERVAL = 5s old by construction (precompute.rs:252). So every mempool arrival opens a spin window that lasts until the
next 5-second refresh. Each spin iteration costs a Tip read, a ChainInfo read, a full mempool::Request::FullTransactions, ZIP-317 selection, and Merkle/auth root computation — plus a fresh shielded-coinbase
proof each time the fee total changes (the fallback's fee total won't match any entry the updater cached).
Before this branch both sides read the same fresh mempool, so the second poll converged and slept 5s. This is a new regression.
Concrete consequence for the internal miner (zebrad/src/components/miner.rs:282-320), which is exactly this long-polling client: it alternates between two templates with different Merkle roots, so
template_sender.send_if_modified reports a change every iteration, cancel_fn (miner.rs:439) cancels the equihash solver every iteration, and it mines nothing while spinning and logging "mining with an updated
block template" at full speed.
Fix direction: pick one change detector. When template_cache is live, implement the long-poll wait on the watch channel itself (receiver.changed(), alongside tip-change and max-time), instead of falling
through to a path that rebuilds from the state and mempool.
- The updater task is unsupervised (medium)
zebrad/src/commands/start.rs:490 spawns the task, and :866-868 aborts it at shutdown — but unlike every other ongoing task it is never added to the select! at :748-846, so its handle is never joined. If run()
panics or returns early (precompute.rs:213, :249 return silently when the tip channel closes), nothing logs it and nothing restarts it.
The failure is silent but not harmless: TemplateCache::is_empty() (precompute.rs:76) stays false because the last template is still there, so every subsequent getblocktemplate call still enters wait_for_tip,
and after the next tip change it burns the full NEW_TIP_TIMEOUT of 1 second (precompute.rs:49) before falling back — permanently, on every request. Either put the handle in the supervision select! like the
other ongoing tasks, or have run() clear the cache on exit so is_empty() short-circuits.
- Unconditional background shielded proving (medium)
run() refreshes every 5 seconds forever (precompute.rs:252) whenever mining.miner_address is set. Each refresh calls new_internal, which misses the coinbase cache whenever the mempool fee total moved and
re-runs the Sapling/Orchard proof (get_block_template.rs:333-341) — the code's own comments put that at "seconds". That is close to a permanently busy core.
It runs regardless of whether anyone is mining: spawn_block_template_updater is called at start.rs:490 before the config.rpc.listen_addr.is_some() check at :493, so a node with a configured miner address and
no RPC listener and no internal miner still proves continuously. Previously this work was strictly on demand. Consider gating the spawn on an RPC listener or the internal miner being enabled, and/or backing
off the refresh when no getblocktemplate call has arrived recently.
- Mempool failures are masked (low)
If the updater's mempool build keeps failing, build() returns Err, gets logged at debug (precompute.rs:216), and the cache keeps the last template. The RPC serves that instead of surfacing the error the
fallback path would have returned. Staleness is bounded by the inter-block interval (the next tip change makes wait_for_tip time out and fall back), so this is minor — but a repeated failure deserves more than
debug!, since it silently costs miners the fees of everything in the mempool.
|
Thanks — 1 was real and the worst of it. All four addressed in 1. Long polling degenerates into an unthrottled busy loop. Confirmed, and your fix direction is what I took: long polling now waits on the cache it is served from (published template, tip change, or Regression test 2. The updater task is unsupervised. Confirmed; its handle is now in the supervision 3. Unconditional background shielded proving. Two halves. The gating half is fixed: the spawn now requires an RPC listener or the internal miner, so a node with a miner address and neither no longer proves anything. The refresh half is fixed in the stacked #11374, which replaces the five second timer with mempool change notifications plus a thirty second backstop — an idle node rebuilds 6x less often, and those rebuilds hit the coinbase cache because the fee total is unchanged, so they do not re-prove. 4. Mempool failures are masked. Now warns when a failing spell starts and logs the recovery, staying at The |
|
Force-pushed: the long-poll fix had a bug of its own, and writing a sharper test for it is what found it. My first version cloned the chain tip receiver inside the wait loop, so The test asserts the absence of that too. "Did not return" is satisfied by a spin as well as by a park, so it now also measures the work: it samples the state-tip read count across two windows and requires no growth while waiting. Against the re-cloned receiver that assertion fails with 37,129 tip reads in 250 ms, each of which would have carried a mempool selection and a coinbase build in production. Against the original fall-through it fails as before. New hashes: |
1bffc8a to
756e51b
Compare
756e51b to
73536c3
Compare
…d ID source Serving `getblocktemplate` from the precomputed cache while long polling fell through to a fresh state and mempool read left two independent long poll ID sources, and they never agree once the mempool moves: the cached snapshot is older than the fresh one by construction. A client alternated between them with no wait on either side. It long polls with the ID the fallback gave it, the cache holds a different template and returns immediately; it long polls with that one, the cache matches so the call falls through, the fresh read derives a third ID and returns immediately. Each answer also went backwards, handing the client the older mempool snapshot. For the internal miner, which is exactly this client, every answer cancels the equihash solver, so it mines nothing while spinning at full speed. Long polling now waits on the cache it is served from: a published template, a chain tip change, or `max_time`. The synchronous path stays for the cases where the cache can't serve, which is a node without an updater task, or one whose task hasn't caught up with a tip change yet.
…r a consumer The updater task was spawned but never joined, unlike every other ongoing task, so an early exit or a panic was silent. That is not harmless: the cache keeps the last template, so `is_empty()` stays false and every later `getblocktemplate` call waits out `NEW_TIP_TIMEOUT` after each tip change, permanently. Put its handle in the supervision `select!` alongside the others. It also ran whenever a miner address was configured, including on a node with no RPC server and no internal miner, where nothing can call `getblocktemplate` at all. Only spawn it when one of those two can ask for a template.
A build that keeps failing is logged at `debug!` while the RPC serves the last template, so miners silently lose the fees of every transaction that arrived since it broke. Warn when a failing spell starts, and log the recovery, while staying at `debug!` for the repeats: the retry delay is one second, so warning on every attempt would bury the rest of the log.
73536c3 to
42bd34b
Compare
|
Force-pushed once more, The race in the fix itself. The cache subscription was taken at the point of waiting, after the call had already read the template, so a template published in between was marked seen and skipped. The other wake conditions are a chain tip change and Changelog. The Also correcting something I wrote earlier in this thread: I said each of the 37,129 spinning iterations would carry a mempool selection and a coinbase build. It would not — the spin stays inside the cache path, so each iteration is a state tip read and an ID comparison. The busy loop was real, its cost was state traffic. |
Motivation
Closes #11370. Closes #10747.
Solution
A background task keeps a block template for the current chain tip ready, and
getblocktemplateserves it instead of reading the state and the mempool, selecting transactions, and building a
coinbase transaction on the request path. Zebrad starts the task when
mining.miner_addressisconfigured; the RPC falls back to building a template itself when no task is running.
The task publishes a coinbase-only template as soon as the chain tip changes, so miners get work
on the new tip immediately, then replaces it with one carrying mempool transactions, and refreshes
that every
MEMPOOL_LONG_POLL_INTERVAL. A precomputed template can therefore be a few secondsbehind the mempool, costing the miner the fees of the transactions that arrived in the meantime.
It is never behind the chain: the RPC ignores a template whose previous block hash is not the
current tip.
That shared coinbase also removes the per-request precompute from the long-poll wait (#10747),
where each outstanding long poll ran its own shielded coinbase proof, once per 5-second iteration.
Handlers whose miner parameters were overridden after cloning —
generateandgeneratetoaddress— share neither the template nor the coinbase cache, so they never serve a coinbase built for
another address or another set of coinbase data.
Tests
getblocktemplate_precomputedasserts the two properties that make this safe to serve. It startsthe updater task against mock services, then stops those mocks answering: a response still
arrives, which is only possible without reading the state or the mempool. It then advances the
mock chain tip and requires the next response to extend the new tip hash — that assertion fails
if the staleness check is removed, which is the bug that would hand miners work on a chain Zebra
has already seen a block for.
getblocktemplate_long_poll_returns_submit_old_false_on_new_tipcovers the rewritten long-pollwait end to end: it keeps waiting while the template is valid, returns within the fast-path bound
after
generatemoves the tip, reportssubmit_old: false, and validates the returned templateas a block proposal under every advertised time source.
Specifications & References
Part of #11310, which also tracks the other
getblocktemplateperformance work discussed there.Follow-up Work
The remaining ports named in #11310 are untouched. The per-call cost that is left is response
serialization, which is proportional to the template's transaction data.
AI Disclosure
PR Checklist
type(scope): description