feat: Implement commit all and revert all for world state checkpoints#21532
feat: Implement commit all and revert all for world state checkpoints#21532PhilWindle wants to merge 4 commits intomerge-train/spartanfrom
Conversation
|
⏳ Run #1 — Session completed (3m) Reviewed #21532 — 23 files, 829 additions. LGTM. No bugs found. The depth-aware checkpoint commit/revert design is clean, correctly fixes the per-tx revert destroying caller checkpoints, and has thorough test coverage across C++ and TS. Full review: https://gist.github.com/AztecBot/7c1effe73ef529c1f0ede41684b6f1af |
spalladino
left a comment
There was a problem hiding this comment.
I feel I'm missing some understanding of how things worked beforehand. Left a few questions that may help me follow along.
| if (target_depth == 0 || target_depth > journals_.size()) { | ||
| throw std::runtime_error("Invalid depth for revert_to_depth"); | ||
| } | ||
| while (journals_.size() >= target_depth) { | ||
| revert(); | ||
| } |
There was a problem hiding this comment.
I'm probably missing something, but shouldn't we allow a revert_to_depth(0) that reverts everything? Same for commit?
There was a problem hiding this comment.
Rightly or wrongly, it's inclusive. So revert_to_depth(1) reverts everything.
| cache.commit_to_depth(1); | ||
| EXPECT_EQ(cache.depth(), 0u); |
There was a problem hiding this comment.
I see, so a commit-all or revert-all is to 1. I'd've expected that commit_to/revert_to(N) means the depth after running that is N.
| async revert(): Promise<void> { | ||
| if (this.completed) { | ||
| return; | ||
| } | ||
|
|
||
| await this.fork.revertCheckpoint(); | ||
| this.completed = true; | ||
| } | ||
|
|
||
| /** | ||
| * Reverts all checkpoints at or above this checkpoint's depth (inclusive), | ||
| * destroying this checkpoint and any nested checkpoints created on top of it, | ||
| * while preserving any checkpoints created by callers below our depth. | ||
| */ | ||
| async revertToCheckpoint(): Promise<void> { | ||
| if (this.completed) { | ||
| return; | ||
| } | ||
|
|
||
| await this.fork.revertAllCheckpointsTo(this.depth); | ||
| this.completed = true; | ||
| } |
There was a problem hiding this comment.
I'm missing something here: what's the difference between these two methods? Doesn't reverting a checkpoint also revert all checkpoint built in top of it? In which scenario wouldn't we want that?
There was a problem hiding this comment.
The original intention of this class was a simple, almost RAII like wrapper around a single checkpoint. So the expected flow was little more than
function processFunction() {
const cp = ForkCheckpoint.new();
try {
// Execute code that might call processFunction() again
} catch () {
cp.revert();
} finally {
cp.commit();
}
}
The AVM never actually uses this. It's used in one place in public_processor for setting up the base checkpoint for the transaction. If all succeeds (or errors gracefully), then we call commit/revert for that one final checkpoint, either applying or removing that tx's side effects. If we error ungracefully, the AVM will have lost track of checkpoints and we just have to hit the big revertToCheckpoint button deleting every modification the AVM made ready for the next transaction.
|
Claude seems to like it! |
Co-authored-by: Santiago Palladino <santiago@aztecprotocol.com>
Summary
commitAllCheckpointsTo(depth)andrevertAllCheckpointsTo(depth)to the world state checkpoint system. These revert/commit all checkpoints at or above the given depth (inclusive), preserving any checkpoints created by callers below that depth.createCheckpoint()now returns the depth of the newly created checkpoint, threading it through the full C++ async callback chain (cache → tree store → append-only tree → world state → NAPI → TypeScript).ForkCheckpointstores its depth and exposesrevertToCheckpoint()which encapsulates the revert-to-depth pattern, replacing the previousrevertAllCheckpoints()+markCompleted()two-step.revertToCheckpoint()on tx timeout/panic, so per-tx reverts no longer destroy checkpoints created by callers (e.g.,CheckpointBuilder).Changes
C++ (barretenberg)
ContentAddressedCache:checkpoint()returns depth, newcommit_to_depth()/revert_to_depth()methodsCachedContentAddressedTreeStore: passes through depth-aware operationsContentAddressedAppendOnlyTree:CheckpointCallbacknow receivesTypedResponse<CheckpointResponse>with depthWorldState:checkpoint()returns depth,commit_all_checkpoints_to/revert_all_checkpoints_totake required depthForkIdWithDepthRequest/CheckpointDepthResponsemessage typesTypeScript
MerkleTreeCheckpointOperationsinterface:createCheckpoint()returnsPromise<number>, depth is required oncommitAllCheckpointsTo/revertAllCheckpointsToMerkleTreesFacade: passes depth through native message channelForkCheckpoint: stores depth, newrevertToCheckpoint()methodPublicProcessor: usescheckpoint.revertToCheckpoint()on error pathsTests
commit_to_depth,revert_to_depth, edge casesTest plan
crypto_content_addressed_cache_tests)crypto_content_addressed_append_only_tree_tests)native_world_state.test.tspassesfork_checkpoint.test.tspassespublic_processor.test.tspassestimeout_race.test.tspasses