Repository navigation
Give each IBD thread one role. - #1233
rearden-grok[bot] wants to merge 8 commits into
Conversation
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
The thread-role split holds on the paths this PR actually rewired. IBD Class C barriers stay ordered under tip_commit, heavy v2 and compact tip-announce CPU leaves the blocking pool, and light JSON stays on the caller. One startup change does not. A datadir that already has a durable scripthash head enters Tip before catch-up, and rbtc-sh-wb is not started until catch-up returns, so every new block's scripthash records sit in RAM for the whole sync.
Disposition: comment, not an approval.
The scripthash write-behind gap should be fixed before merge. No other happy-path product bug showed up in the offload, flush, or encode paths.
Issue counts by severity
- bugs: 1
- suggestions: 0
- nits: 0
1 bug, 0 suggestions, 0 nits
| .await | ||
| .map_err(|e| NodeError::Config(format!("tip entry: {e}")))?; | ||
| sh_writebehind = | ||
| sh_writebehind_at_tip_entry(Arc::clone(&node.hub.query), Arc::clone(&shutdown.flag), { |
There was a problem hiding this comment.
[bug] apply_startup_index_mode puts a --sh-index node with a durable scripthash head into Tip before run_ibd_or_skip (line 1951). Confirm then calls enqueue_sh_pending, and note_confirmed_tip_while releases those heights, but sh_writebehind_at_tip_entry runs only after catch-up completes. Nothing drains sh.pending or ram_head during that sync. The queue is unbounded, the on-disk index does not advance, and a long restart catch-up holds every new block's scripthash records until IBD returns. Direct catch-up does not enqueue, so this is only the durable-head path. A crash is rebuilt by recover_sh_writebehind, which has the same undrained window.
Suggestion: Start rbtc-sh-wb as soon as startup has entered Tip with shindex on. Keep the post-materialize start for the Direct path, where the appender must stay down until enter_tip_mode finishes.
There was a problem hiding this comment.
A durable scripthash head is already in Tip before catch-up, and confirm enqueues those records while the chain syncs. rbtc-sh-wb now starts at that startup tip entry, so the queue drains during catch-up. Direct catch-up still waits until materialize enters Tip. A second start does not spawn another thread.
A write-behind IO error still calls Shutdown::fail, so the process exits non-zero.
Fixed in ff0174f.
There was a problem hiding this comment.
Re-checked at ff0174f. The durable head still starts rbtc-sh-wb before catch-up, and the second call does not spawn another thread. This finding stays fixed.
There was a problem hiding this comment.
Re-checked at 34122b4. The durable head still enters Tip before catch-up, and the later start still returns when that slot is already filled.
The catch-up header thread was unnamed, so ps showed the process name. It is now one ibd-header thread for the catch-up and is joined when IBD stops.
The scripthash appender used to start before IBD and wake through Direct catch-up. It now starts only after enter_tip_mode returns in Tip with --sh-index on. Materialize runs on the blocking pool, not a reactor worker.
Each write batch used to start its own thread. The confirm engine now keeps one thread, and the next commit waits for the in-flight barrier. Tip mode still flushes that barrier on tip-accept. A failed barrier still fails later commits until restart.
Heavy v2 encode, compact short ids after the body is in hand, and JSON of at least 4 KiB leave the blocking pool. Shorter encodes stay on the caller.
Confirm enqueues while that head is already in Tip. The appender now starts at startup. Direct catch-up still waits until materialize enters Tip, and a second start does not spawn another thread.
6c65883 to
ff0174f
Compare
There was a problem hiding this comment.
1 bug, 2 suggestions, 0 nits
Prior finding, scripthash write-behind during durable-head catch-up: fixed. ensure_sh_writebehind (run.rs:626) starts rbtc-sh-wb after startup has entered Tip and before run_ibd_or_skip. The later call (run.rs:838) returns when that slot is already filled. Both closures call Shutdown::fail.
The pull request text still says write-behind starts only after tip entry. The changelog and the code start a durable head before catch-up.
Disposition: comment, not an approval. The script-worker panic should block merge.
| state: Arc::clone(&state), | ||
| value: None, | ||
| }; | ||
| spawn_detached(move || { |
There was a problem hiding this comment.
[bug] run_cpu runs the guest inside spawn_detached, and steal_or_job calls that job with no catch_unwind. A panic drops CpuFinish, so the awaiter sees None, and then the panic leaves the rbtc-scripts-* loop. workers() starts those threads once and never replaces a dead one. run_held already catches panics. After that, encode_json_body turns the dropped reply into a 200 body of {}, and cmpct_from_parts_cpu flattens it to Ok(None), so the tip path sends headers. v2 encode already returns NetError::Protocol("encode task join failed"). On a one-worker pool every later run_cpu stays pending and script waves stop.
Suggestion: Catch the guest panic inside the worker, the same way run_held does, and leave the thread in the loop. Keep None for a dropped reply, but do not publish that as HTTP 200 {} or as a missing compact body. Compact should use the existing join-failure error.
There was a problem hiding this comment.
A guest panic is caught inside run_cpu, the same way run_held catches a script chunk. That worker stays in the pool, and the awaiter still gets None. encode_json_body now returns an error, so Esplora answers 500 instead of a 200 with {}. Compact short ids return the same cmpct relay join failed protocol error as a failed blocking join, so the tip path does not fall through to headers. Fixed in 34cda69.
There was a problem hiding this comment.
Re-checked at 34122b4. A guest panic is still caught inside run_cpu, and the callers still turn a dropped join into an error.
| if !json_is_cpu_heavy(v) { | ||
| return serde_json::to_vec(v).unwrap_or_else(|_| b"{}".to_vec()); | ||
| } | ||
| let v = v.clone(); |
There was a problem hiding this comment.
[suggestion] encode_json_body clones the Value on the awaiting task before to_vec runs on rbtc-scripts-*. For Esplora that await is ensure_json on a tokio worker, after spawn_join has released the blocking thread. gbt_json still calls Value::to_string inside the spawn_join closure, and ensure_json sees a finished body, so a large template never hops.
Suggestion: Move the Value into the script-pool job instead of cloning it on the waiter. Point block-template JSON at the same heavy path without dropping its cache-control header.
There was a problem hiding this comment.
encode_json_body now takes an Arc and moves it into the script job, so the waiting task does not clone the value. Electrum hands over the reply it already owns. Esplora keeps the heavy body in that Arc and ensure_json moves it across. GET /block-template uses the same handoff and still sends cache-control: no-store. A body under 4 KiB still encodes on the caller. Fixed in 2592225.
There was a problem hiding this comment.
Re-checked at 34122b4. Heavy JSON still moves into the script job, and block templates still go through json_reply_shared.
|
|
||
| /// Positional pread (page cache / disk). Preferred for Class A `tx.body`. | ||
| pub fn pread_at(&self, offset: u64, buf: &mut [u8]) -> Result<(), StoreError> { | ||
| rbitcoin_primitives::assert_not_script_worker("store read"); |
There was a problem hiding this comment.
[suggestion] assert_not_script_worker runs only from pread_at and write_at_pwrite. pread_exact calls pread_all directly, and the io_uring batch path submits reads without this check, so those store reads do not panic on rbtc-scripts-*. script_worker_cannot_pread only calls read_at, which is the guarded wrapper.
Suggestion: Put the assert on the pread/pwrite syscall wrappers and the uring submit path, and point the test at one path that does not go through pread_at.
There was a problem hiding this comment.
The guard now sits on the pread and pwrite wrappers, and on io_uring submit. pread_exact and pwrite_all panic on a script worker, and so does a pool-backend submit. The checks that used to live only on pread_at and write_at_pwrite are gone, so there is one path. The test calls pread_exact, pwrite_all, and submit. Fixed in 34122b4.
There was a problem hiding this comment.
Re-checked at 34122b4. The script-worker store checks are still on the pread and pwrite wrappers and on the uring submit paths.
A dropped reply stays None. JSON encode fails instead of publishing {}, and compact short ids return the relay join error instead of falling through to headers.
Block templates use that same handoff and still send cache-control: no-store.
pread_exact, pwrite_all, and an io_uring submit hit the same guard as pread_at.
There was a problem hiding this comment.
0 bugs, 0 suggestions, 0 nits
Prior findings at 34122b4c3:
-
Durable scripthash head undrained during catch-up: fixed.
apply_startup_index_mode(run.rs:345) enters Tip (run.rs:1999) beforeensure_sh_writebehind(run.rs:626). The post-catch-up call (run.rs:838) returns when the slot is already filled (run.rs:2564). Both closures callShutdown::fail. Direct, cancelled, and failed materialize stay down until afterenter_tip_mode. -
run_cpuguest panic: fixed. The guest runs insidecatch_unwind(script_pool.rs:796). A dropped reply isErrfromencode_json_body(json_cpu.rs:83); Esplora returns 500 (server.rs:189). Compact returnsNetError::Protocol(peer.rs:5225) andon_tip_eventuses?(peer.rs:1483). -
Heavy JSON cloned on the waiter, and block templates encoded inside
spawn_join: fixed.encode_json_bodytakesArc<Value>and moves it into the script job (json_cpu.rs:75).gbt_jsonusesjson_reply_shared(handlers.rs:847) and still setscache-control: no-store(handlers.rs:849). Under 4 KiB encodes on the caller. -
Script-worker store assert missed
pread_exactand io_uring submit: fixed. The checks are onhandle_pread_allandhandle_pwrite_all(file.rs:234,file.rs:204) and on uringsubmit,submit_and_wait_one, anduring_final_harvest(uring_session.rs:584,:515,:935).
The pull request text still says write-behind starts only after tip entry. The changelog and the code start a durable head before catch-up.
Disposition: comment, not an approval. No new bug should block merge.
The header catch-up thread is named
ibd-headerand joined when catch-up ends.rbtc-sh-wbstarts after tip entry, and only when--sh-indexis on and the index is in Tip. Direct catch-up, including a cancelled or failed scripthash materialize, does not start it.IBD Class C barriers share one
rbtc-tip-flushthread for the confirm engine. Tip mode still flushes that barrier on the tip-accept thread. A failed barrier still fails later commits until restart.Heavy pure CPU leaves the blocking pool for
rbtc-scripts-*: v2 encode whenencode_is_cpu_heavy, compact short ids after the body is in hand, and Electrum or Esplora JSON of at least 4 KiB. Shorter encodes stay on the caller. Block and header decode stay on the blocking pool. The blocking-pool cap staysmax(N, 4).