Repository navigation
store: drop the process-local scripthash head cache - #1207
Conversation
|
@coreyphillips could use your review on this |
coreyphillips
left a comment
There was a problem hiding this comment.
Two independent reviews.
nits
- Stale doc comment and names left over from the head cache (crates/rbitcoin-store/src/scripthash.rs:1341).
head_valueis still documented as "(process-cache seed / disconnect refresh)". Both of those callers are gone, and only tests call it now. Input_create_batch_append,missingandseen_missnow hold every key in the batch, so the names suggest a cache that no longer exists. - Changelog omits the
ibd: perfowned field removal (changelog.d/drop-sh-head-cache.md:3). The fragment saystip: perfno longer reportssh_heads.ser_process_ownedalso droppedsh_headsfrom theownedobject ofibd: perf, and the fragment does not mention that. Anyone running jq on that field would get null without warning.
the reviewers disagree, your call
- Class A backfill no longer serialized against the SH write-behind appender (crates/rbitcoin-query/src/catchup.rs:391). Objection: Real, but blocking rather than non-blocking: A forced rebuild with a durable head stays in Tip, allowing write-behind to overlap the unlocked tail backfill. The allocator lock does not cover head lookup and publication, so overlapping appends can lose creates or corrupt chains.
| self.store | ||
| .scripthash | ||
| .put_create_batch_append(&recs, &mut heads)?; | ||
| self.store.scripthash.put_create_batch_append(&recs)?; |
There was a problem hiding this comment.
the reviewers disagree, your call: Class A backfill no longer serialized against the SH write-behind appender
The sh.heads mutex this PR deletes was the only lock shared by the two callers of put_create_batch_append. That function corrupts a key's chain when two calls on the same key overlap. Before, apply_sh_job_inner (connect.rs:755) and backfill_sh_creates_from_class_a (catchup.rs:391) both held sh.heads around the append. The backfill held it across every chunk. Now the write-behind holds only sh.appender, and the backfill takes no lock. docs/concurrency.md says the backfill runs while Direct, so rbtc-sh-wb no-ops. That holds on the usual paths. It does not hold for RBITCOIN_SH_FORCE_REBUILD=1 on a datadir with a durable head. enter_tip_mode enters Tip at run.rs:2531, then finalize_sh_unsorted_cancellable wipes, materializes, and runs the tail backfill at catchup.rs:315 in Tip mode. That backfill only runs because blocks connected during materialize, and rbtc-sh-wb is applying those blocks' SH jobs. To check the store side, I built a scratch binary in /tmp against this HEAD. It runs two barrier-synced threads on one ScriptHashTable::create_tiny, both appending to the same scripthash: rust for i in 0..2000 { bar.wait(); t.put_create_batch_append(&[ScriptHashRecord::from_fk(sh, Fk(2 * i + off))]) } // off = 1 on thread A, 2 on thread B All 10 trials broke. Appends returned Corrupt errors: "page append create_fk not strictly increasing", "slab append ...", "pread past logical end", and "fk stream null first fk". Chains ended with 3 to about 1800 entries while entry_count() was near 3900. A Corrupt on the write-behind side halts the process. On the backfill side it fails finalize. I did not run the force-rebuild node path end to end, so this is a traced exposure, not a reproduced one. The materialize step on that path was already unserialized before this PR. Taking self.sh.appender in backfill_sh_creates_from_class_a restores the old serialization and matches the documented single Class B appender.
Objection: Real, but blocking rather than non-blocking: A forced rebuild with a durable head stays in Tip, allowing write-behind to overlap the unlocked tail backfill. The allocator lock does not cover head lookup and publication, so overlapping appends can lose creates or corrupt chains.
There was a problem hiding this comment.
The head map was the mutex this tail and apply_sh_job_inner both held. The write-behind apply still takes sh.appender. This tail did not.
rbtc-sh-wb starts before enter_tip_mode. A durable head flips to Tip, then finalizes. RBITCOIN_SH_FORCE_REBUILD wipes on that path, and a create that lands during the pack is appended here while write-behind can apply the same scripthash. Two overlapping put_create_batch_append calls on one scripthash corrupt the chain. Direct backfill still runs while write-behind is idle, because that thread applies only in Tip.
backfill_sh_creates_from_class_a now holds sh.appender for the chunk append and drops it before the table sync, so the two appends take turns. class_a_tail_backfill_waits_on_sh_appender holds the mutex and checks the tail stays blocked, then checks the lagging create is in the store.
| @@ -0,0 +1,3 @@ | |||
| Removed | |||
|
|
|||
| - **Scripthash head cache.** Tip append locates each scripthash head from disk for that batch. The process no longer keeps 65,536 head values between blocks, and `tip: perf` no longer reports `sh_heads`. | |||
There was a problem hiding this comment.
nit: Changelog omits the ibd: perf owned field removal
The fragment says tip: perf no longer reports sh_heads. ser_process_owned also dropped sh_heads from the owned object of ibd: perf, and the fragment does not mention that. Anyone running jq on that field would get null without warning.
There was a problem hiding this comment.
The fragment now says tip: perf and the owned object on ibd: perf both dropped sh_heads.
A second batch must locate the head from disk. The caller map is filled today.
Tip append locates each head from disk for that batch. A retained hit still re-probed the head to learn its home, so the 65536-entry map did not skip I/O. tip: perf no longer reports sh_heads.
Two overlapping put_create_batch_append calls on one scripthash corrupt the chain. The Class A tail and the write-behind worker both append, so the tail has to wait on sh.appender. This fails while the tail appends without that lock.
The removed head map was the mutex both the tail backfill and rbtc-sh-wb held around put_create_batch_append. A forced rebuild enters Tip before that tail, so the two can append one scripthash at once and corrupt the chain. The tail now takes sh.appender for the chunk append and drops it before the table sync. The store entries are the pin that the lagging create landed.
Every key in the batch is located from disk, so the old miss names and the head_value cache note no longer match. tip: perf and the owned object on ibd: perf both dropped sh_heads.
5c0f176 to
42cfa05
Compare
|
The batch locate loop is named keys and seen. head_value reads the durable head. |
|
@coreyphillips review with improvements plz |
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after the branch was rewritten, so every earlier point was verified again from scratch. Nothing blocking remains.
addressed
- Class A backfill no longer serialized against the SH write-behind appender (crates/rbitcoin-query/src/catchup.rs:391). Fixed:
backfill_sh_creates_from_class_anow takesself.sh.appenderaround the whole chunk loop (crates/rbitcoin-query/src/catchup.rs:389). That is the same mutexapply_sh_job_innertakes at connect.rs:725, and it is released before the flush.class_a_tail_backfill_waits_on_sh_appenderfails at the test-first commit 8a0efe6 (panics at catchup.rs:413) and passes at HEAD, along with the other 12 catchup tests. I also checked lock order: disconnect takes sh.appender and then the bf appender, the tail takes only sh.appender, so there is no cycle. - Stale doc comment and names left over from the head cache (crates/rbitcoin-store/src/scripthash.rs:1341). Fixed:
head_valuenow reads "Durable head value for a key." (crates/rbitcoin-store/src/scripthash.rs:1341). The batch locate loop useskeysandseen(scripthash.rs:1593-1594). A repo-wide grep forsh_heads,SH_HEADS_CAP,sh.heads, and "process-cache" finds nothing outside the changelog. - Changelog omits the
ibd: perfowned field removal (changelog.d/drop-sh-head-cache.md:3). Fixed: changelog.d/drop-sh-head-cache.md:3 now says that bothtip: perfand theownedobject onibd: perfno longer reportsh_heads. That matches the removals inser_process_ownedandTipPerfLog(crates/rbitcoin-ibd/src/ibd/perf_log.rs).
Tip append kept up to 65,536
ShHeadValues inQuery.sh.headsand reported that length assh_headson everytip: perfline. Once the map filled, the gauge sat at 65536.A hit skipped the miss-path
locate_head, then calledkey_home, which islocate_headagain. Repeat keys and new keys both probed the head once per batch. The map did not save I/O, and it stays empty through Direct IBD because script-hash apply starts at tip finalize.put_create_batch_appendnow locates heads for that batch and drops them. Disconnect and the Class A tail backfill no longer refresh a process cache.tip: perfand the IBD owned-size JSON no longer includesh_heads.append_second_batch_locates_head_from_diskappends the same scripthash in two calls and checks both creates.page_append_preserves_prefix_and_orderstill checks a later batch reuses the slab.