Repository navigation
store: Class C flush from live slots, one writer, lock-free gets - #1217
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
92b648d to
b088775
Compare
coreyphillips
left a comment
coreyphillips
left a comment
There was a problem hiding this comment.
Two independent reviews.
worth doing, does not block
- store_byte reintroduces deprecated fetch_update and fails to build on 1.99 (crates/rbitcoin-store/src/chain.rs:219).
StrongTxTable::store_byte(crates/rbitcoin-store/src/chain.rs:219) callsAtomicU64::fetch_update. Master's 8c1e825 renamed every atomicfetch_updatetotry_updateahead of Rust 1.99. That commit is in this PR's base, which has nofetch_updateleft. With rustc 1.99.1 and the workspacewarnings = "deny", rbitcoin-store fails to compile: "use of deprecated methodfetch_update: renamed totry_update". I hit this building the crate from a scratch directory. The pinned 1.95.0 builds it, so CI passes. Usetry_update. - IBD coordinator blocks on the write thread while holding
shared(crates/rbitcoin-query/src/lib.rs:2450-2468). An archived-body clear now makes the IBD coordinator wait for the confirm-write thread's whole backlog while it holds the shared state lock. The path:apply_confirm_eventsruns on the IBD tokio task underHeldShared. A mutated soft reject reachesapply_soft_wire_reject, which callsQuery::clear_archived_body(crates/rbitcoin-query/src/lib.rs:2450). That call now blocks twice. FirstSyncSender::sendon the write channel, which blocks when the queue is full. Thenreply_rx.recv(). The reply comes only after the write thread finishes every batch queued ahead of the clear (the channel is FIFO, up tocaps.write), plus its drain group and a tip-flush join. While it waits, the header thread cannot takeshared, and a tokio worker sits parked on std channels, which docs/concurrency.md rules out. Before this PR the clear was a direct store op plus one fsync. I did not run this. I found no deadlock: the event channel is unbounded, and the write thread never takesshared. Suggestion: send the clear without waiting for the reply and log the result on the write thread, or releasesharedbefore the call. - Truncated slots reappear when an array grows across a gap (crates/rbitcoin-store/src/array_table.rs:463-470). A sparse extension after truncation restores discarded values. I reproduced
[10,11,12,13], truncate to length 1, then set slot 3 to 30: the result is[10,11,12,30]instead of[10,0,0,30].store_ramretains the truncated slots and never clears the gap. Current confirmation callers extend contiguously, so I found no production path reaching this case. - Failure-injection tests require a device absent on macOS (crates/rbitcoin-store/tests/dev_full.rs:19-20). The new failure-injection tests panic on macOS because
/dev/fullis absent. I ranfailed_write_keeps_the_range_dirtyon Darwin and reproducedENOENTat this unwrap. Itscfg(unix)guard also enables it on macOS, and the strong-bit failure test uses the same helper.
the reviewers disagree, your call
- strong_tx keeps about twice its body resident after growth (crates/rbitcoin-store/src/array_table.rs:128-156). Objection: Real, but non-blocking rather than blocking: Growth retains every previous buffer, and open rounds capacity to a power of two. The logical-length meter and the documented ~177 MiB therefore understate retained memory, though I consider this bounded regression non-blocking.
- ClassCWrite does not enforce a single writer; two writers lose stores (crates/rbitcoin-store/src/array_table.rs:18-35). Objection: Real, but blocking rather than non-blocking: Safe callers can mint independent tokens, and the counter does not serialize writers. Concurrent growth can publish competing images and reconstruct owning boxes from the same raw pointer, making this a memory-safety issue despite current production serialization.
- Concurrent writer capabilities can discard successful stores (crates/rbitcoin-store/src/array_table.rs:150-155). Objection: Real, but non-blocking rather than blocking: Real:
ClassCWrite::new()is public and mintable on any thread, so two writers'seq_beginincrements pair up to an even counter and concurrentgrowpublishes last-wins, discarding the other writer's slot stores (the orphan leaks rather than double-frees, but two Boxes do owncur). Every production mint site runs underlock_tip_commitor single-threaded startup, apart from the inline clear fallback after confirm-write detaches at IBD teardown, so there is no current production trigger; a compare_exchange even-to-odd inseq_beginwould close it cheaply.
| pub(crate) fn grow(&self, need: usize) { | ||
| let cur = self.ptr.load(Ordering::Relaxed); | ||
| // SAFETY: `cur` is null or a box this writer published. The copy finishes | ||
| // before `from_raw` moves that box onto the new buffer. | ||
| let old = unsafe { cur.as_ref().map(|buf| &buf.slots[..]) }; | ||
| let cap = old.map_or(0, |slots| slots.len()); | ||
| if old.is_some() && need <= cap { | ||
| return; | ||
| } | ||
| let new_cap = if cap == 0 { | ||
| need.max(1).next_power_of_two() | ||
| } else { | ||
| cap.saturating_mul(2).max(need) | ||
| }; | ||
| let slots: Box<[AtomicU64]> = (0..new_cap) | ||
| .map(|i| { | ||
| let v = old | ||
| .and_then(|s| s.get(i)) | ||
| .map_or(0, |a| a.load(Ordering::Relaxed)); | ||
| AtomicU64::new(v) | ||
| }) | ||
| .collect(); | ||
| let prev = (!cur.is_null()).then(|| { | ||
| // SAFETY: `cur` came from `Box::into_raw` on this writer. | ||
| unsafe { Box::from_raw(cur) } | ||
| }); | ||
| let buf = Box::into_raw(Box::new(Buf { slots, prev })); | ||
| self.ptr.store(buf, Ordering::Release); | ||
| } |
There was a problem hiding this comment.
the reviewers disagree, your call: strong_tx keeps about twice its body resident after growth
The in-RAM bit image now costs roughly 2x its body in RSS. The memory doc row this PR edits still says ~177 MiB. RamSlots::grow (crates/rbitcoin-store/src/array_table.rs:128-156) writes every word of the new buffer, so the whole buffer is resident. It also keeps every retired buffer on prev until the table drops. StrongTxTable::open calls grow on a null buffer, which rounds the size up to the next power of two. How I confirmed it: I ran the same probe against b0e2df9 and against HEAD (pinned 1.95.0, release, scratch crate under /tmp). The probe grows strong_tx to 2^30 bits (127 MiB body) with set_strong_range in 65536-bit steps, then reads RSS with ps. base: body=127 MiB rss_after=132 MiB HEAD: body=127 MiB rss_after=258 MiB On mainnet that works out to about 512 MiB after a fresh IBD (256 MiB current plus 256 MiB retired). After a restart it is 256 MiB, because open rounds up. The doc row (docs/ibd-memory.md:176) still says ~177 MiB. class_c_l2= still reports body bytes from l2_resident_bytes, so the extra lands in residual=, which the same doc tells operators is allocator residue. Possible fixes: size the buffer exactly at open. Free retired buffers once no reader can hold them, or at least make the doc row and the meter report the real cost.
Objection: Real, but non-blocking rather than blocking: Growth retains every previous buffer, and open rounds capacity to a power of two. The logical-length meter and the documented ~177 MiB therefore understate retained memory, though I consider this bounded regression non-blocking.
There was a problem hiding this comment.
strong_tx reserves the 256 MiB cap once and commits the used prefix. A grow does not keep a retired copy of that image. Slot tables still keep the previous buffer. class_c_l2= stays the logical body. The memory row now says RSS is that prefix rounded up to pages.
| /// One Class C writer (`!Send` + `!Sync`). A shared [`crate::Store`] cannot carry | ||
| /// it onto another thread. Confirm-write, tip-accept, and startup each mint one | ||
| /// on the storing thread. The flush thread reads live slots and does not take it. | ||
| #[derive(Default)] | ||
| pub struct ClassCWrite(PhantomData<*const ()>); | ||
|
|
||
| impl ClassCWrite { | ||
| /// Mint a writer capability on the calling thread. | ||
| pub fn new() -> Self { | ||
| Self(PhantomData) | ||
| } | ||
|
|
||
| /// Same as [`Self::new`]. Tests are not the confirm-write thread. | ||
| #[doc(hidden)] | ||
| pub fn for_test() -> Self { | ||
| Self::new() | ||
| } | ||
| } |
There was a problem hiding this comment.
the reviewers disagree, your call: ClassCWrite does not enforce a single writer; two writers lose stores
The token's doc says "One Class C writer", but ClassCWrite::new() is a safe public function. !Send stops a token from moving between threads. It does not stop a second thread from minting its own. With two writers the design breaks in three ways. Both grow calls Box::from_raw the same cur. The two odd increments leave the counter even while stores are in flight. Stores into a retired buffer are lost. Probe through the public API: 4 threads, each with its own ClassCWrite::new(), call ArrayTable::set on interleaved indices 0..8192 of one table. 860 of 2000 rounds lost stores. Base serialized these stores with the RwLock. I found no production path that does this today. Every runtime mint site runs under lock_tip_commit or at startup. That same convention is the only thing keeping strong_tx's flush safe now that flush_dirty_between skips flush_lock for byte bodies (array_table.rs:524). Two concurrent StrongTxTable::flush calls would clear each other's flushing flag mid-copy, and the second would then mark as clean stores it never copied. Suggestion: mint the token from the tip_commit guard, or add a debug owner-thread check in begin_store. Keep the flush lock; it is uncontended.
Objection: Real, but blocking rather than non-blocking: Safe callers can mint independent tokens, and the counter does not serialize writers. Concurrent growth can publish competing images and reconstruct owning boxes from the same raw pointer, making this a memory-safety issue despite current production serialization.
There was a problem hiding this comment.
ClassCWrite still cannot move to another thread, and a second thread can still mint its own. The counter now moves even to odd with a compare-exchange, so the second store waits. Both SeqCst fences stay. The existing sequence model passed with that exchange. A loom model of the wait itself exceeds loom's branch cap, so that case is not in the model job.
| fn store_byte(words: &[AtomicU64], index: usize, value: u8) { | ||
| let w = &words[index / 8]; | ||
| let shift = (index % 8) * 8; | ||
| let _ = w.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |cur| { |
There was a problem hiding this comment.
worth doing, does not block: store_byte reintroduces deprecated fetch_update and fails to build on 1.99
StrongTxTable::store_byte (crates/rbitcoin-store/src/chain.rs:219) calls AtomicU64::fetch_update. Master's 8c1e825 renamed every atomic fetch_update to try_update ahead of Rust 1.99. That commit is in this PR's base, which has no fetch_update left. With rustc 1.99.1 and the workspace warnings = "deny", rbitcoin-store fails to compile: "use of deprecated method fetch_update: renamed to try_update". I hit this building the crate from a scratch directory. The pinned 1.95.0 builds it, so CI passes. Use try_update.
There was a problem hiding this comment.
store_byte calls try_update. The product compiler stays 1.95. The rust-forward CI job runs cargo check on 1.99.0 beside pread. Loom 0.7 still names the method fetch_update, so the model build uses that name only under cfg(loom).
| pub fn clear_archived_body(&self, hash: &[u8; 32]) -> Result<bool, QueryError> { | ||
| let tx = self | ||
| .confirm_write | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()) | ||
| .clone(); | ||
| if let Some(tx) = tx { | ||
| let (reply_tx, reply_rx) = std::sync::mpsc::channel(); | ||
| tx.send(crate::ConfirmWriteMsg::ClearArchived { | ||
| hash: *hash, | ||
| reply: reply_tx, | ||
| }) | ||
| .map_err(|_| StoreError::Cancelled("confirm write stopped"))?; | ||
| return reply_rx | ||
| .recv() | ||
| .map_err(|_| StoreError::Cancelled("confirm write stopped"))?; | ||
| } | ||
| self.clear_archived_body_here(hash) | ||
| } |
There was a problem hiding this comment.
worth doing, does not block: IBD coordinator blocks on the write thread while holding shared
An archived-body clear now makes the IBD coordinator wait for the confirm-write thread's whole backlog while it holds the shared state lock. The path: apply_confirm_events runs on the IBD tokio task under HeldShared. A mutated soft reject reaches apply_soft_wire_reject, which calls Query::clear_archived_body (crates/rbitcoin-query/src/lib.rs:2450). That call now blocks twice. First SyncSender::send on the write channel, which blocks when the queue is full. Then reply_rx.recv(). The reply comes only after the write thread finishes every batch queued ahead of the clear (the channel is FIFO, up to caps.write), plus its drain group and a tip-flush join. While it waits, the header thread cannot take shared, and a tokio worker sits parked on std channels, which docs/concurrency.md rules out. Before this PR the clear was a direct store op plus one fsync. I did not run this. I found no deadlock: the event channel is unbounded, and the write thread never takes shared. Suggestion: send the clear without waiting for the reply and log the result on the write thread, or release shared before the call.
There was a problem hiding this comment.
The coordinator drops shared while it waits for the confirm-write thread to clear the archived body. After the reply it takes the lock again and marks that hash missing only when the hash still occupies the height.
| let prev = (!cur.is_null()).then(|| { | ||
| // SAFETY: `cur` came from `Box::into_raw` on this writer. | ||
| unsafe { Box::from_raw(cur) } | ||
| }); | ||
| let buf = Box::into_raw(Box::new(Buf { slots, prev })); | ||
| self.ptr.store(buf, Ordering::Release); |
There was a problem hiding this comment.
the reviewers disagree, your call: Concurrent writer capabilities can discard successful stores
Two safe stores can both succeed while one value disappears. Each thread can construct its own ClassCWrite, and seq_begin does not reject an already odd counter. I reproduced concurrent stores at slots 131071 and 262143: both returned Ok, but the first slot read back as zero. The previous implementation retained both values. Concurrent growers can also create two Box owners for the same cur here.
Objection: Real, but non-blocking rather than blocking: Real: ClassCWrite::new() is public and mintable on any thread, so two writers' seq_begin increments pair up to an even counter and concurrent grow publishes last-wins, discarding the other writer's slot stores (the orphan leaks rather than double-frees, but two Boxes do own cur). Every production mint site runs under lock_tip_commit or single-threaded startup, apart from the inline clear fallback after confirm-write detaches at IBD teardown, so there is no current production trigger; a compare_exchange even-to-odd in seq_begin would close it cheaply.
There was a problem hiding this comment.
The even-to-odd exchange is what keeps the second writer out of the image. strong_tx also no longer grows by taking ownership of the current buffer, so two growers cannot free the same allocation.
| fn store_ram(&self, min_idx: u64, max_idx: u64, pairs: &[(u64, u64)]) { | ||
| self.slots.grow(max_idx as usize + 1); | ||
| let slots = self.slots.slots(Ordering::Relaxed); | ||
| for &(index, value) in pairs { | ||
| self.file | ||
| .write_at(Self::offset(index), &value.to_le_bytes())?; | ||
| slots[index as usize].store(value, Ordering::Release); | ||
| } | ||
| Ok(()) | ||
| let len = self.len.load(Ordering::Relaxed).max(max_idx + 1); | ||
| self.len.store(len, Ordering::Relaxed); |
There was a problem hiding this comment.
worth doing, does not block: Truncated slots reappear when an array grows across a gap
A sparse extension after truncation restores discarded values. I reproduced [10,11,12,13], truncate to length 1, then set slot 3 to 30: the result is [10,11,12,30] instead of [10,0,0,30]. store_ram retains the truncated slots and never clears the gap. Current confirmation callers extend contiguously, so I found no production path reaching this case.
There was a problem hiding this comment.
set and set_many return corrupt when an index above the length is missing, so a truncate cannot republish those bytes. Header ranges use set_zero_gap, which stores zeros for the slots the call does not name.
| .open("/dev/full") | ||
| .unwrap(); |
There was a problem hiding this comment.
worth doing, does not block: Failure-injection tests require a device absent on macOS
The new failure-injection tests panic on macOS because /dev/full is absent. I ran failed_write_keeps_the_range_dirty on Darwin and reproduced ENOENT at this unwrap. Its cfg(unix) guard also enables it on macOS, and the strong-bit failure test uses the same helper.
There was a problem hiding this comment.
The /dev/full tests are cfg(target_os = "linux").
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 bugs, 0 suggestions, 1 nit.
Style
The master RwLock is the easier read. A get and a set exclude each other, and the flush copies under the lock and drops it before pwrite. dirty_lo and the dirty epoch are a second handshake, so a later flush change can update one and leave the other. PR 1214 keeps that lock and that epoch, and checks the file against the writer's history. A later edit has to keep the snapshot, the epoch, and dirty_lo in step, or the model and the table diverge.
This PR has one order: the sequence counter, flushing, and ClassCWrite minted on the storing thread. Flush, growth, and the script-pool claim each have one place to change (seq_arm / FlushGuard / store_ram, the prev chain, claim_chunk). There is no snapshot and no dirty epoch left beside it. It is harder to read. The edits that still compile are deleting either SeqCst fence (loom 0.7 does not order a bare SeqCst access), minting a second ClassCWrite on another thread (!Send does not stop a second new), and a model that writes through poke_live_slot instead of set.
The lock is the simpler style. This counter is the more maintainable one: the next change to flush or growth updates one protocol. Nothing in this pass is a bug that 1214's lock avoids.
Disposition
Code scanning 757 and 758: the latest instance of each is fixed on merge commit 1053dbc4. There is no instance for b088775da. The pin is unchanged and matches master ci.yml. The nit is the missing comment, not a second report of the impostor rule.
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| persist-credentials: false | ||
| - uses: dtolnay/rust-toolchain@035fceb3c12fb0c6a9461b52775e16473133e591 # 1.95.0 |
There was a problem hiding this comment.
[nit] model.yml pins dtolnay/rust-toolchain at 035fceb3, the same commit as the pread job in ci.yml. That job's comment says a5f673d0 still names toolchain 1.95.0 but is no longer on the 1.95.0 branch, so zizmor reports it as an impostor, and 035fceb3 is the branch tip. This line has no such comment.
Suggestion: Copy those two comment lines from ci.yml onto this step.
There was a problem hiding this comment.
The dtolnay/rust-toolchain step is gone from model.yml. rustc comes from rust-toolchain.toml.
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| persist-credentials: false | ||
| - uses: dtolnay/rust-toolchain@035fceb3c12fb0c6a9461b52775e16473133e591 # 1.95.0 |
There was a problem hiding this comment.
nit: can remove dtolnay/rust-toolchain altogether, like in #1223
31abf0e to
49381b5
Compare
| workspaces: ". -> target/model-${{ matrix.engine }}" | ||
| - name: model-check ${{ matrix.engine }} | ||
| if: github.event_name != 'schedule' | ||
| run: ./scripts/model-check.sh ${{ matrix.engine }} |
There was a problem hiding this comment.
[suggestion] Every pull request runs ./scripts/model-check.sh with LOOM_MAX_PREEMPTIONS unset. seq::len_belongs_to_the_slot_and_a_header_pair_does_not_mix calls check_at_most(2), and an unset env becomes bound 2, so that test is the ~15 minute step (job 121898809276: 929.96s) while flush and utxo finish in the same second they start. The nightly cron sets LOOM_MAX_PREEMPTIONS=4, and check_limited takes min(bound, cap), so the sequence model stays at 2. Nightly cannot make that test longer.
Suggestion: Keep the sub-second models (flush, utxo, script-pool claim) on the pull-request job, and run the sequence model only on the nightly schedule. Moving it does not raise coverage while check_at_most(2) stays. Raising that cap is the change TESTING.md refuses, because a higher bound still would not exhaust an aarch64 release reorder of the odd advance. Do not put a spinning two-writer case back on the default model job.
| | loom | Class C sequence counter (`get_sample`, `get_range`) | a new length arrives with the slot from before that store, or a header first/count pair mixes two generations | | ||
| | loom | Class C flush exclusion (`set`, `flush_dirty_between`) | a store returns Ok, the flush marks the table clean, and the slot it sampled is the one left on disk | | ||
|
|
||
| A `Relaxed` counter, or publishing a length before its slot stores, fails the sequence-counter model. One counter covers a header first/count pair. Header pairs take a relaxed odd step and a release fence before the column stores. Array and `strong_tx` stores take a sequentially consistent odd step, then that same release fence. The reader's second counter load sits behind an acquire fence. That model stays at preemption bound 2: the schedule is already millions of executions, so the nightly bound does not raise it. The bound does not explore every aarch64 reorder of a release on the odd advance, so the fence order is part of the protocol and not something this model is claimed to exhaust. It also does not flush, so it does not cover exclusion. |
There was a problem hiding this comment.
[nit] claim_odd is a SeqCst compare-exchange for header pairs as well as array and strong_tx stores. This paragraph still says header pairs take a relaxed odd step.
Suggestion: Say header pairs take the same sequentially consistent odd step, then the release fence. Leave the bound-2 sentence; the nightly job does not raise this model.
| - **In-RAM Class C gets take no reader-writer lock.** One sequence counter | ||
| covers a store. The odd bit is sequentially consistent, a release fence | ||
| runs before the slot stores, and the even advance publishes them. Header | ||
| pairs, which have no flush flag, still take a relaxed odd step and that |
There was a problem hiding this comment.
[nit] The same sentence says header pairs still take a relaxed odd step. seq_odd calls claim_odd, which exchanges with Ordering::SeqCst.
Suggestion: Describe that odd step as sequentially consistent, matching the array and strong_tx stores. The following release fence is the part that differs from seq_begin only in that header pairs have no flush flag.
2562b20 to
c6021b3
Compare
There was a problem hiding this comment.
Summary
The follow-up fixes hold. claim_odd is a SeqCst compare-exchange, both SeqCst fences stay, strong_tx growth commits one reservation, the archived-body clear joins on the write thread and skips the post-clear mark when that hash no longer occupies the height, and the two sequence closures call the real set, get_sample, put_range, and get_range inside the loom closure. (Fk(50), 4) and (Fk(10), 9) still panic; None, (Fk(10), 4), and (Fk(50), 9) still pass. Tip-flush joins still precede the production Class C stores, the even publish is still the release step, and the debug hold does not run in release or in tests that did not arm it. changelog.d/class-c-snapshot.md still says header pairs take a relaxed odd step.
Issue counts by severity
- bugs: 0
- suggestions: 0
- nits: 1
| - **In-RAM Class C gets take no reader-writer lock.** One sequence counter | ||
| covers a store. The odd bit is sequentially consistent, a release fence | ||
| runs before the slot stores, and the even advance publishes them. Header | ||
| pairs, which have no flush flag, still take a relaxed odd step and that |
There was a problem hiding this comment.
[nit] The unreleased note says header pairs take a relaxed odd step. seq_odd calls claim_odd, and that exchange is SeqCst on both success and failure. The release fence after it is the publish, not a relaxed increment.
Suggestion: Say the header-pair odd step is the same SeqCst compare-exchange, with no flush flag, followed by the release fence.
c6021b3 to
e8e948a
Compare
There was a problem hiding this comment.
Summary
The branch regresses release IBD coordination and fd-only Class C reads. In-RAM gets, the sequence-counter flush exclusion, and the script-pool claim order do not. Wave::claim_chunk still release-publishes next after the in_wave increment, and claim_holds_in_wave_before_next_advances drives that function, so publishing next first still lets is_complete free the wave under the claim. The header-pair odd step in changelog.d/class-c-snapshot.md is the SeqCst compare-exchange, then the release fence.
Disposition: checked the prior review threads against this head, the Class C odd/flush handshake, header-pair get_range, strong-bit flush, confirm-write joins, and get_fk_by_txid_batch_cancellable (unchanged). PR 1214's production fixes sit on the dirty-epoch ArrayTable: a shrinking flush that also rewrites a dirty prefix, an fd-only get that treats a truncate race as absent, a dirty mark that must observe the clean reset, and a rewrite that still publishes the logical length. This branch does not take that model. It does call set_logical_len after a dirty flush, which is the rewrite-path length publish 1214 added. The script-pool arm order is this branch's model; 1214 does not change the script pool.
Issue counts by severity
- bugs: 3
- suggestions: 0
- nits: 0
| was_occupant: st.height_to_hash.get(&height) == Some(&hash), | ||
| }; | ||
| if query.is_some() && rbitcoin_net::chain::reject_is_mutated(err) { | ||
| if rbitcoin_store::coord_shared_lock_held() { |
There was a problem hiding this comment.
[bug] apply_soft_wire_reject defers a mutated archived-body clear only when coord_shared_lock_held() is true. That function is hard-wired to false without debug_assertions, and CoordSharedLock::enter only sets the thread-local under the same cfg. Release take_confirm_events therefore calls Query::clear_archived_body while release_archived_clears still holds shared. The clear blocks on the confirm-write queue and on the in-flight tip flush. The header thread cannot take shared for that wait, and the coordinator task is parked on a std channel. Debug builds and debug_assert in clear_archived_body do not run this path.
Suggestion: Remember the coordinator hold in release builds, or always return the deferred clear from take_confirm_events and send it only after shared is dropped. Do not branch the production wait on the debug counter.
| offset += n as u64; | ||
| remaining -= n as u64; | ||
| } | ||
| self.len.store(new_len, Ordering::Release); |
There was a problem hiding this comment.
[bug] Fd-only ArrayTable::set publishes len before the payload pwrite. write_slots zero-fills the new span, stores new_len, then writes the pairs. A concurrent get_sample that observes the new length reads the zero fill and returns 0. The previous set wrote the value, then stored len. In-RAM store_ram still stores the slots before len. An append of the next slot is the case that used to be ordered and no longer is. Tables over the in-RAM cap, or a lowered RBITCOIN_CLASS_C_INRAM_MAX_MB, take this path. Fd-only stores still take no odd guard, so nothing else hides the window.
Suggestion: Write the pair bytes, then store len, as the old single-slot append did.
| self.mark_dirty_index(index); | ||
| return Ok(()); | ||
| let mut buf = [0u8; 8]; | ||
| self.file.read_at(Self::offset(index), &mut buf)?; |
There was a problem hiding this comment.
[bug] get_sample on an fd-only table loads len once, then read_at. truncate stores the shorter length and then set_logical_len shrinks published_len. A get that sampled the old length calls pread_at after that shrink and returns Corrupt("pread past logical end") instead of the absent (0, len) the in-RAM path returns. Pull request 1214 made this race read as absent.
Suggestion: When the read is past the published end, reload len and return (0, len) if index is past it.
The dirty-epoch mark is gone. A flush records the unstored index and the length, pwrites those live slots in fixed scratch chunks, and clears the mark only after that persist succeeds. A store that starts in that window is refused; the next flush writes it. A same-length rewrite still publishes the length. A failed write leaves the range dirty.
ClassCWrite is not Send or Sync. Confirm-write, tip-accept, and startup mint it on the thread that stores. Archived-body clear is an item on the confirm-write channel: that thread joins the in-flight tip flush, then clears. Header range puts on that path wait for the same join, including when a later phase rejects the batch, so a retry still sees header_txs. Class A body append still overlaps the flush. The write thread polls the channel because Query keeps a sender so a clear can arrive. A blocking recv never observed disconnect at shutdown.
The writer takes an odd step, a release fence, then the slot stores. The even advance publishes them. A sequentially consistent fence keeps a store out of the flush it races. strong_tx bits live in the same slot image. Growth keeps the previous buffer until the table drops. An fd-only store holds no odd guard.
The loom and shuttle harness follows phlip9's #1214: cfg-gated model tests, a separate target directory, and the same pull-request, push, dispatch, and nightly jobs. This pull request is an alternative to that ArrayTable model. Those scenarios stay on #1214. The models here are the script-pool claim order, the idle turn, the UTXO view stamp, the Class C sequence counter, and Class C flush exclusion. A script-pool claim release-publishes next after in_wave.
Drop the pinned toolchain action from the loom and shuttle job. The runner rustup installs the compiler named in rust-toolchain.toml.
fetch_update is a denied deprecation on 1.99. try_update is the same call on 1.95 and on 1.99. The rust-forward job checks that channel beside pread, after qc and test. Product rustc stays 1.95.
Those tests open /dev/full. macOS has no such device, so the helper and both tests compile only on Linux.
set and set_many return corrupt when an index above the length is missing, so a truncate cannot republish bytes the length no longer names. Header ranges use set_zero_gap, which stores zeros for a header that was stamped and then rejected.
A mutated reject still waits for the confirm-write thread to clear the body. That wait no longer holds the coordinator state lock. After the reply, the hash is marked missing only when it still occupies the height.
strong_tx reserves the 256 MiB cap once and commits the used prefix. A grow does not allocate a second buffer. Slot tables still keep the previous buffer until the table is dropped. The logical body meter is unchanged.
Two threads can each mint ClassCWrite. The sequence counter now moves even to odd with a compare-exchange, so the second store waits. Both SeqCst fences stay. The strong-bit flush takes the same flush lock as the slot tables. Loom cannot run two waiters on that lock, so the overlap check is a normal thread test.
The Class A commit helper stored header_txs while the previous IBD tip flush could still be excluding stores. pread keeps that flush in the window, and the next block's put then returns Rejected. Class A append still overlaps the flush. The put waits on the same tip join as confirm-write. A debug hold pauses the tip flush after it excludes stores, so the test observes the flag.
rustc 1.99 denies a safe extern close because that name is a runtime symbol. The stderr test now calls the unsafe declaration.
One closure ran a four-slot store and two header puts in the same schedule, so bound 2 took about 15 minutes. A slot set against one get_sample, and one header put_range against a pair stored before the threads start, still fail a relaxed even publish.
Header pairs call the same compare-exchange as a slot store, and they have no flush flag. The release fence after that exchange publishes the pair.
The classifier returns the clear and does not send it. The coordinator drops its state lock, then the caller sends the clear and applies the reject. The test entry points send the same way.
An fd-only extension pwrites the new slots, then the file length, then len. A zero gap is only the unnamed slots, and it does not publish the file end. An index already inside len stays a direct write. Loom branches before the length store, and the pwrite is not a loom access, so the model yields once len covers the new slots. The append under test is the next index of a pre-sized file: an empty create is in RAM, and index 0 of that file is an overwrite.
The get loads len, reads, then loads len again. A slot past that second length is empty. A past-end pread is empty only when the length no longer covers the index. Any other error is unchanged. Loom branches before the length load and the pread is not a loom access, so the model yields after len covers the index and before the read.
ac6fd9f to
edb87fb
Compare
A zero gap pwrites zeros across the new span, then the named slots. Walking each hole was a second write of the same bytes, and that helper was uncovered. The append still publishes the file length after those bytes. A unit test reads the value, the holes, and the slot that was already in range.
The pread suite counts every task named rbtc-tip-flush. Another query's barrier in that process made the count two, or left one after drop. The worker records its own live count and Linux tid. The test checks that thread's name and that drop joins it.
There was a problem hiding this comment.
Summary
Disposition: comment.
Class C gets no longer take a reader lock. In-RAM confirmed, header_txs, and strong_tx publish through an even/odd counter; a table past the RAM cap pwrites new bytes, then the file length, then len. IBD runs that barrier on one rbtc-tip-flush thread, and the next tip_commit taker joins it before another Class C store. That protocol matches docs/concurrency.md and docs/crash-recovery.md on the production path, where stores hold tip_commit and the flush is started after those stores return. The remaining risk is on the store API, outside those callers: a shrink flush marks the image clean without rewriting a dirty prefix below the new length, and a header column whose seq_begin returns Rejected after its pair partner committed leaves a torn (first, count).
Issue counts by severity
- bugs: 0
- suggestions: 0
- nits: 0
Checked, not filed
ArrayTable::flush_dirty_between skips the slot rewrite when len is below disk_len, then FlushGuard marks the table clean. truncate marks the prefix dirty. Disconnect only shortens confirmed and does not store into the kept prefix after the previous flush, so the bytes already on disk are the kept slots. A caller that stores below the new length and then truncates before flush would persist the old prefix.
HeaderTxTable::put_range holds the pair counter across both columns. Each column's begin_store can still return Rejected on its own. After that error the pair counter is even, so get_range can accept a new first with the old count. Confirm joins the in-flight tip flush before the next Class C store, so both columns see flushing == false.
Class C images no longer keep a dirty epoch or a second copy of the dirty range. A flush reads the live slots after it excludes new stores. A store that committed before that exclusion is in this flush. A failed persist leaves that range dirty.
ClassCWriteis notSend. Confirm-write, tip-accept, and startup mint it on the thread that stores. Archived-body clear runs onibd-confirm-writeafter the in-flight tip flush joins. Header range puts on that path wait for the same join, including when a later phase rejects the batch. Class A body append still overlaps the flush. Class A device sync stays onrbtc-spend-syncabout every 10 minutes. Class C still flushes once per confirm batch.In-RAM gets of
confirmed,header_txs, andstrong_txtake no reader-writer lock. The writer adds one to a sequence counter, stores the slots, and adds one again.header_txsreads first and count under one counter. A column load acquires that store, then the counter is checked again. A get retries when the sampled length does not fit the buffer. A flush does not bump the counter. Growth keeps the previous buffer until the table drops.Four commits on
store/class-c-snapshot.Loom checks the script-pool claim order, the UTXO view stamp, and that counter. Shuttle checks the idle turn. They are not required checks. The index-release order is not a model: it needs a
Queryfull ofstdlocks. The sequence-counter model stays at preemption bound 2.The loom and shuttle harness follows phlip9's #1214: cfg-gated model tests, a separate target directory, and the same pull-request and nightly jobs. This pull request is an alternative to that ArrayTable model. Those scenarios stay on #1214.
Branch
store/class-c-snapshot.