feat: Lock-free apply_block refactor - #2345
Conversation
…ckfree-store-state
…ckfree-store-state
…ckfree-store-state
| @@ -0,0 +1,419 @@ | |||
| use std::collections::HashSet; | |||
There was a problem hiding this comment.
Much of the diffs here are a move of impl but partial / involving a split to other files. The only changes should be w.r.t BlockNumber -> ScopedBlockNumber and ScopedBlockRange.
| conn, | ||
| note_commitments.as_slice(), | ||
| up_to_block, | ||
| ) |
There was a problem hiding this comment.
This up_to_block bound may technically not be required but I think it might still be a good idea to use it for any path that involves multiple DB calls - some of which are block bound sensitive. If anything, to catch / not be effected by desync-like bugs we would not expect.
…ckfree-store-state
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Some comments; but the size of the PR does somewhat force our hand towards merging this and fixing things in post.
A more staggered/stacked approach could have been:
- snapshot API + one or two example impls
- more impls
- lastly update apply block
I do think we can do better once we have sqlite added; but this is a decent stepping stone.
There was a problem hiding this comment.
Does recovery still have a place in a multi-validator world? I guess if they each resign each block?
There was a problem hiding this comment.
Current impl is maybe one piece of what we would need. Lets followup #2427
There was a problem hiding this comment.
Should we even offer this API outside of a view?
There was a problem hiding this comment.
Yea it seems wrong at first glance but is actually the right thing to do. StateViews are intended to be short-lived because they hold StateSnapshots which we want to release ASAP. The committed / proven tips are for long-held stream use cases. And they don't involve the rest of data in StateSnapshot at all.
Updated comment to elaborate on that.
| let (current_block_height, store_inputs) = state | ||
| .with_view(async |view| { |
There was a problem hiding this comment.
Why do we use with_state as a closure?
There was a problem hiding this comment.
Do you mean with_view? Mainly so that a set of view calls are ensured to use the same snapshot (because state.view() could return a different snapshot different calls).
| /// Store write capabilities consumed by the full-node sync loop. | ||
| /// | ||
| /// Must be provided in full-node mode and omitted in sequencer mode. The RPC service itself | ||
| /// only ever reads through `state`; these are passed through untouched to the sync tasks. |
There was a problem hiding this comment.
Then this should probably form part of the enum?
There was a problem hiding this comment.
Yea it might be a little bit involved so will do it as a followup #2428
| /// Runs a read operation over a view pinned at the current chain tip, dropping the view — and | ||
| /// releasing its snapshot generation — as soon as the operation completes. | ||
| /// | ||
| /// This is the required form whenever multiple reads must observe the *same* snapshot: the | ||
| /// closure's view is one consistent generation, whereas consecutive [`Self::view`] calls may | ||
| /// straddle a commit. The typical case is pairing a query with [`StateView::tip`] so a | ||
| /// response reports exactly the height it was served at. |
There was a problem hiding this comment.
tbh I don't quite understand the need for view and with_view. Why can't the caller hold the view for as many calls as they want?
There was a problem hiding this comment.
Both are intended to be used so that the lifetime of the StateView (and underlying Arc<StateSnapshot>) are as short as possible.
So for example
// Bad
let view = state.view();
let tip = view.tip();
// more unrelated instructions...
// scope finally ends
}
// Good
let tip = state.view().tip();And
// Bad
let view = state.view();
let x = view.get_accounts();
let y = view.compute_something(x);
// more unrelated instructions...
// scope finally ends
}
// Good
// Snapshot tightly scoped
state.with_view(...);
// unrelated instructions don't affect lifetime of snapshotThere was a problem hiding this comment.
I understand in general, but because we offer both APIs there isn't really any safety in that; its an improvement in cases where we should otherwise manually drop.
But I don't know of any such cases? I would expect a view to live for 99% of an RPC request's lifespan.
| /// moving the closure body into `block_in_place`. | ||
| fn with_inner_read_blocking<R>(&self, f: impl FnOnce(&StateSnapshot) -> R) -> R { | ||
| let span = Span::current(); | ||
| tokio::task::block_in_place(|| span.in_scope(|| f(&self.snapshot))) |
There was a problem hiding this comment.
There are very few places on should use block_in_place - ideally never / only when manually starting a new async runtime.
Can we make this spawn block instead?
There was a problem hiding this comment.
@kkovaacs I believe you originally implemented with_inner_read_blocking let us know your thoughts. The 'static lifetime bound of spawn_blocking might be an issue here.
|
|
||
| // The DB is committed at this point, so the prepared mutations must be applied and any | ||
| // failure to do so aborts the process. | ||
| let snapshot = tokio::task::block_in_place(|| { |
There was a problem hiding this comment.
Prefer spawn blocking
There was a problem hiding this comment.
We can't move self into spawn_blocking (requires 'static lifetime). Also I think block_in_place is fine given this is all called serially by one task.
| .unwrap_or_else(|error| { | ||
| Self::abort_after_post_commit_failure("nullifier tree", &error) | ||
| }); |
There was a problem hiding this comment.
Are we aborting because the apply should always succeed?
Why not just panic?
There was a problem hiding this comment.
The idea is to exit the entire application instantly rather than panic this task and have upstream join handle it. Unsure if that has real world value however.
There was a problem hiding this comment.
Yes but why do we want to abort at all? Why not panic and log? If we abort we get zero telemetry.
…ckfree-store-state
igamigo
left a comment
There was a problem hiding this comment.
Not a full review (hopefully by tomorrow morning), but so far it's looking good. Left just one comment for now
| .reader() | ||
| .expect("nullifier tree snapshot creation should not fail"), | ||
| account_tree: self.account_tree.reader(), | ||
| blockchain: self.blockchain.clone(), |
There was a problem hiding this comment.
Is this fine to do? I haven't done the math but wouldn't this self.blockchain grow into the hundreds of MBs fairly quickly (weeks/months)? Not sure there is an easy way to work around this though. This might have been accounted for already
Summary
Closes #1539.
Closes #1853.
Closes #2414.
Makes the store's block-write path lock-free for readers, and makes the resulting read-consistency rules type-enforced. Reads previously contended on
RwLocks over the in-memory trees (and blocked duringapply_block's DB-commit window); they now load an immutable snapshot viaArcSwapand are never blocked by writes. All reads flow through a request-scopedStateView, so a query combining tree and DB data at different chain heights is no longer expressible.Why:
apply_block(oneshot handshakes between the DB task and the in-memory update).StateViewmakes the scoping structural instead.How:
Lock-free write path (
state/writer/)WriteWorkertask owns the mutable nullifier tree, account tree, blockchain MMR, and account-state forest, processing blocks serially from an mpsc channel — no locks. In-flight writes always complete; shutdown is only observed between requests.StateSnapshot(trees backed by read-only RocksDB snapshot views) and publishes it atomically viaArcSwap, so readers keep a consistent frozen view while the next block commits.Db::apply_blockis now a plain transaction — the oneshotallow_acquire/acquire_donesynchronization is removed.Write capabilities (
state/lifecycle.rs)LoadedState::startspawns the worker and returns the read-onlyArc<State>plus non-cloneableBlockWriter/ProofWritercapabilities and aWriterTaskhandle, statically limiting each write path to one task. The capabilities expose no read access; tasks that read and write getArc<State>alongside their capability.BlockWriter::stopdrains and joins the worker so tree storage is released deterministically before the data directory is re-opened or deleted (used byrecoverand stress-test seeding).Type-enforced reads (
state/view/)StateView, pinned to one snapshot per request (State::view()). DB queries are scoped by the view's tip internally; callers cannot supply their own. RocksDB-backed trees are only reachable throughblock_in_placehelpers; snapshot fields are only visible inside the view module.Dbqueries require view-issued proof types (ScopedBlockNum/ScopedBlockRange), constructible only by aStateViewafter validating the bound against its tip — extending the enforcement to the DB boundary itself.range.end() <= tipthemselves via a newRangeBeyondTiperror (sameInvalidArgumentresponse as before); the RPC layer'srange_bounds_checkis deleted and pagination'schain_tipis now the tip the query actually ran against.get_account, the block producer'sget_tx_inputs);sync_chain_mmrclamps the proven tip to the view's tip.Live tips (
state/tip.rs)State::committed_tip()/proven_tip()read the watch channels their writers publish to (mirroringsubscribe_committed_tip/subscribe_proven_tip); theFinalityenum is removed. The committed tip is published after the snapshot, so it never reports a block a fresh view cannot serve.Observability:
SnapshotGuardtracks live snapshot generations and lifetimes; warns when a snapshot outlives 10s or more than 4 generations are pinned (a leaked/slow reader pins a RocksDB snapshot).Supporting changes: read-only
reader()views forAccountStateForest/AccountTreeWithHistory(relaxed toBackendReader/SmtStorageReaderbounds);statemodule restructured intoview/(read endpoints) andwriter/(worker + capabilities); new tracing field names allowlisted.Changelog