feat(v2): EpochStakes parsing & zero-copy SnapshotMetadata - #1702
feat(v2): EpochStakes parsing & zero-copy SnapshotMetadata#1702kprotty wants to merge 13 commits into
Conversation
kprotty
changed the title
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (81.72%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage.
🚀 New features to boost your workflow:
|
moves away from RuntimeMetadata being delta changes of already decoded snapshot metadata. Instead, it stores the snapshot fba & parsed Manifest/StatusCache in a region directly (SnapshotMetadata) and shares that with accounts_db and replay. Rooted writes that region to a fixed sized section in the disk after the journal for persistence
kprotty
changed the title
yewman
left a comment
yewman
left a comment
There was a problem hiding this comment.
Rooted.loadSnapshot / loadExisting persist the wrong 15 bytes
The blob write assumes [Manifest][StatusCache][FBA memory] are contiguous in SnapshotMetadata, but the extern-struct layout has 15 bytes between &status_cache and &memory (7 bytes of padding + the 8-byte memory_len field). Verified with zig 0.15.2 on this branch:
offsetOf(manifest) = 8
offsetOf(status_cache) = 9856
offsetOf(memory_len) = 9864
offsetOf(memory) = 9872
hdr_size = @sizeOf(Manifest) + @sizeOf(StatusCache) = 9849
distance &manifest → &memory = 9864 // gap = 15
Consequences of writing unpadded = hdr_size + memory_len bytes from &manifest and reading them back the same way:
- Bytes intended as FBA
[0..15)are actually 7 bytes of struct padding + thememory_lenfield. On reloadmemory_lenis silently overwritten with the write-time value. - FBA bytes
[memory_len-15 .. memory_len)are never written. On reload they come back mmap-zeroed. AnyRelativeSlicepayload that landed there reads zeros. - The
disk_bytes > total_capacityguard uses the same wrong formula, so it also rejects some valid loads with 15 bytes to spare.
Fix by writing/reading the two segments separately, and add a comptime assert so this can't drift again:
comptime std.debug.assert(
@offsetOf(SnapshotMetadata, "memory") - @offsetOf(SnapshotMetadata, "manifest")
== @sizeOf(Manifest) + @sizeOf(StatusCache),
);That assert fails on today's layout (9864 != 9849), which is exactly the bug. The existing unit test never round-trips through the rooted DB so CI is green today — worth adding a small loadSnapshot → loadExisting roundtrip that checks a RelativeSlice read after reload.
There was a problem hiding this comment.
Pull request overview
This PR updates the v2 snapshot ingestion pipeline to parse additional snapshot metadata (EpochStakes / stake-related fields) and to pass snapshot-derived metadata to replay via a new zero-copy SnapshotMetadata shared-memory blob, replacing the prior RuntimeMetadata ring-buffer based handoff.
Changes:
- Introduces
lib.snapshot.SnapshotMetadata(Manifest + StatusCache + trailing FBA memory) and rewires accounts_db → replay to use it as the readiness barrier and metadata carrier. - Extends
solana/snapshot.zigparsing to includeBankFieldsstake-related data andExtraFields.versioned_epoch_stakes, and makes Manifest/StatusCache extern + relative-offset based. - Persists the metadata blob in the rooted DB journal area and simplifies replay bootstrap to read blockhashes/block_id directly from the Manifest.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| v2/tests/replay/main.zig | Updates replay test harness to allocate/init SnapshotMetadata with a trailing FBA region. |
| v2/services/replay.zig | Switches bootstrap input from RuntimeMetadata to SnapshotMetadata and reads blockhashes from Manifest. |
| v2/services/accounts_db.zig | Removes SnapshotIter usage at service layer; delegates snapshot parsing to Rooted using SnapshotMetadata. |
| v2/lib/solana/snapshot.zig | Adds RelativeSlice/RelativeOffset, makes metadata structs extern, and adds stake + versioned epoch stakes parsing. |
| v2/lib/solana/inflation.zig | Adds Inflation model with extern pow for consensus-sensitive calculations. |
| v2/lib/solana/epoch_schedule.zig | Adjusts EpochSchedule layout/field alignment for snapshot deserialization. |
| v2/lib/solana.zig | Exposes Inflation from the solana module. |
| v2/lib/snapshot.zig | Adds SnapshotMetadata shared-memory container and slot readiness barrier. |
| v2/lib/ipc/ring.zig | Adds nextBlocking() helper to ring iterator API. |
| v2/lib/accounts_db/rooted.zig | Persists/restores metadata blob in rooted DB and rewires snapshot load path to populate SnapshotMetadata. |
| v2/lib/accounts_db.zig | Removes RuntimeMetadata definition (replaced by SnapshotMetadata). |
| v2/init/services.zig | Rewires service specs to pass *SnapshotMetadata instead of *RuntimeMetadata. |
| v2/init/main.zig | Allocates SnapshotMetadata region sized for trailing FBA; adds config knob for metadata memory. |
| v2/config/example.zon | Adds .metadata memory sizing for accounts_db. |
| v2/build.zig | Sets code_model = .medium for accounts_db service module. |
Comments suppressed due to low confidence (2)
v2/lib/solana/snapshot.zig:43
RelativeSlice.sliceConstusesself.len(au32) directly as a slice bound. Cast tousizeexplicitly for correctness/clarity.
pub fn sliceConst(self: Self, base: [*]const u8) []const T {
const raw = base + self.offset;
std.debug.assert(@intFromPtr(raw) % @alignOf(T) == 0);
const ptr: [*]const T = @ptrCast(@alignCast(raw));
return ptr[0..self.len];
}
v2/lib/solana/snapshot.zig:61
RelativeOffset.fromhas the same underflow risk asRelativeSlice.fromSlice: it subtracts pointers before assertingptr >= base. Add an order assertion before subtraction to prevent bogus non-zero offsets.
pub fn from(base: [*]const u8, ptr: *const T) Self {
const o = @intFromPtr(ptr) - @intFromPtr(base);
std.debug.assert(o != 0);
std.debug.assert(o <= std.math.maxInt(u32));
return .{ .offset = @intCast(o) };
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// A single-pointer variant of `RelativeSlice`. `offset == 0` is the null sentinel; | ||
| /// callers must guarantee no valid allocation lands at offset 0 (see `Rooted.loadSnapshot` | ||
| /// which reserves the first byte of its FBA for exactly this purpose). |
| pub const StatusCache = extern struct { | ||
| /// Placeholder — the deserialized contents are currently discarded. Kept as an | ||
| /// extern struct so it can live inline in `SnapshotMetadata` and be persisted | ||
| /// alongside the `Manifest` in the rooted DB blob. | ||
| _reserved: u8 = 0, | ||
|
|
||
| pub fn read(fba: *std.heap.FixedBufferAllocator, r: anytype) !StatusCache { |
There was a problem hiding this comment.
It would be better to make memory_len align(1) instead of tuning StatusCache to how its stored in its parent.
But in this case, it's fine to persist padding bytes. As long as those padding byte sizes dont change.
| const manifest = &snapshot_metadata.manifest; | ||
| const bhq = &manifest.bank_fields.blockhash_queue; | ||
| const hashes = bhq.hashes[0..bhq.hashes_count]; | ||
|
|
There was a problem hiding this comment.
the u32 doesn't matter, but using getHashes makes sense
| // Persist the Manifest + StatusCache + max FBA bytes | ||
| // contiguous blob right after the journal block. Padded to block_size | ||
| // so account sectors start block-aligned. | ||
| { | ||
| const metadata = snapshot_metadata.getSerializable(); | ||
| const padded = std.mem.alignForward(u64, metadata.len, block_size); | ||
| if (padded > std.math.maxInt(u32)) return error.ManifestTooLarge; | ||
| self.journal.metadata_bytes = @intCast(padded); | ||
|
|
||
| // write unpadded | ||
| var r = std.Io.Reader.fixed(metadata); | ||
| try self.queueWrite(.from(logger), metadata.len, &r); | ||
|
|
||
| // write padding if any | ||
| const pad_len = padded - metadata.len; | ||
| if (pad_len > 0) try self.queueWrite(.from(logger), pad_len, struct { | ||
| pub fn readSliceAll(_: @This(), b: []u8) !void { | ||
| @memset(b, 0); | ||
| } | ||
| }{}); | ||
| } |
There was a problem hiding this comment.
Intentional to (naively) rewrite it on root without shifting over sectors on size change
| var epoch_schedule: EpochSchedule = undefined; | ||
| try r.readSliceAll(std.mem.asBytes(&epoch_schedule)); | ||
|
|
||
| var inflation: Inflation = undefined; | ||
| try r.readSliceAll(std.mem.asBytes(&inflation)); |
| // versioned_epoch_stakes: NullOnEof(Vec({ epoch: u64, value: union(enum(u32)) { current: ... } })) | ||
| var versioned_epoch_stakes: RelativeSlice(VersionedEpochStakes) = .{}; | ||
| { | ||
| const outer_len = readInt(u64, r) catch |err| switch (err) { | ||
| const len = readInt(u64, r) catch |err| switch (err) { | ||
| error.EndOfStream => 0, | ||
| else => |e| return e, | ||
| }; | ||
| for (0..outer_len) |_| { | ||
| try r.discardAll( | ||
| 8 + // epoch: u64 | ||
| 4, // union tag: u32 (enum(u32), always 'current') | ||
| ); | ||
|
|
||
| // current.epoch_stakes: Stakes(StakeDelegationWithStake) | ||
| // vote_accounts: VoteAccounts | ||
| try discardVoteAccounts(r); | ||
|
|
||
| // stake_delegations: HashMap(Pubkey, { delegation: Delegation, credits_observed: u64 }) | ||
| const stake_del_len = try readInt(u64, r); | ||
| try r.discardAll(stake_del_len * (32 + // key: Pubkey | ||
| 32 + // delegation.voter_pubkey: Pubkey | ||
| 8 + // delegation.stake: u64 | ||
| 8 + // delegation.activation_epoch: Epoch | ||
| 8 + // delegation.deactivation_epoch: Epoch | ||
| 8 + // delegation.warmup_cooldown_rate: f64 | ||
| 8 // credits_observed: u64 | ||
| )); | ||
|
|
||
| try r.discardAll( | ||
| 8 + // stakes.unused: u64 | ||
| 8, // stakes.epoch: Epoch | ||
| ); | ||
|
|
||
| // stake_history: Vec({ epoch: Epoch, effective: u64, activating: u64, deactivating: u64 }) | ||
| const sh_len = try readInt(u64, r); | ||
| try r.discardAll(sh_len * (8 + 8 + 8 + 8)); | ||
|
|
||
| // current.total_stake: u64 | ||
| try r.discardAll(8); | ||
|
|
||
| // current.node_id_to_vote_accounts: HashMap(Pubkey, { vote_accounts: Vec(Pubkey), total_stake: u64 }) | ||
| const nv_len = try readInt(u64, r); | ||
| for (0..nv_len) |_| { | ||
| // key: Pubkey | ||
| try r.discardAll(32); | ||
| // value.vote_accounts: Vec(Pubkey) | ||
| const va_len = try readInt(u64, r); | ||
| try r.discardAll( | ||
| va_len * 32 + // vote_accounts: []Pubkey | ||
| 8, // total_stake: u64 | ||
| ); | ||
| } | ||
|
|
||
| // current.epoch_authorized_voters: HashMap(Pubkey, Pubkey) | ||
| const eav_len = try readInt(u64, r); | ||
| try r.discardAll(eav_len * (32 + // key: Pubkey | ||
| 32 // value: Pubkey | ||
| )); | ||
| const slice = try fba.allocator().alloc(VersionedEpochStakes, len); | ||
| for (slice) |*versioned_epoch_stake| { | ||
| versioned_epoch_stake.* = try .read(fba, r); | ||
| } | ||
|
|
||
| versioned_epoch_stakes = .fromSlice(fba.buffer.ptr, slice); | ||
| } |
| /// use that. For reference: | ||
| /// - `std.math.pow`: pow(0.85, 4.019250798563942) -> 7.805634650110366e-2 | ||
| /// - glibc/musl: pow(0.85, 4.019250798563942) -> 7.805634650110367e-2 | ||
| extern fn pow(f64, f64) f64; |
There was a problem hiding this comment.
If it fails to link, we'll get a compile error so that is fine. It currently works since we have dependencies like zstd that link to libc anyways.
dnut
left a comment
dnut
left a comment
There was a problem hiding this comment.
here are some some initial thoughts. still working my way through snapshot.zig
some of the copilot comments seem valid
| @@ -1,3 +1,5 @@ | |||
| // TODO: move this into core | |||
There was a problem hiding this comment.
what's core?
also it would be nice if any TODOs we merge to main to have a github issue number as in TODO(1234):
There was a problem hiding this comment.
shared/core folder root. v2/lib/solana still has its own duplicates of types from there
| pub const SnapshotMetadata = extern struct { | ||
| slot: std.atomic.Value(u64), | ||
|
|
||
| manifest: lib.solana.snapshot.Manifest, | ||
| status_cache: lib.solana.snapshot.StatusCache, | ||
|
|
||
| memory_len: usize, | ||
| memory: [0]u8 align(16), // VLA for [0..memory_len] | ||
|
|
||
| // 0 may be a valid slot, so use something that will never be reached. | ||
| const invalid_slot = std.math.maxInt(Slot); | ||
|
|
||
| pub fn init(self: *SnapshotMetadata, memory_len: usize) void { | ||
| self.slot = .init(invalid_slot); | ||
| self.memory_len = memory_len; | ||
| } | ||
|
|
||
| /// Gets a slice of the serializable memory of the SnapshotMetadata | ||
| pub fn getSerializable(self: *SnapshotMetadata) []u8 { | ||
| // skip the slot & return all bytes from manifest onwards | ||
| comptime std.debug.assert(@offsetOf(SnapshotMetadata, "slot") == 0); | ||
| comptime std.debug.assert(@offsetOf(SnapshotMetadata, "manifest") == 8); | ||
| const header_size = @sizeOf(SnapshotMetadata) - 8; | ||
| return @as([*]u8, @ptrCast(&self.manifest))[0 .. header_size + self.memory_len]; | ||
| } | ||
|
|
||
| /// Returns the base pointer used to resolve `RelativeSlice`/`RelativeOffset` | ||
| /// values inside `manifest` / `status_cache`. | ||
| pub fn getMemory(self: *SnapshotMetadata) []u8 { | ||
| return self.memory[0..].ptr[0..self.memory_len]; | ||
| } | ||
|
|
||
| /// Unblocks all getSlotBlocking() callers with the given slot value. | ||
| /// Can be called only once. | ||
| /// Should also only call after all other SnapshotMetadata fields are populated. | ||
| pub fn populateSlot(self: *SnapshotMetadata, slot: Slot) void { | ||
| std.debug.assert(slot != invalid_slot); | ||
| std.debug.assert(self.slot.swap(slot, .release) == invalid_slot); | ||
| } | ||
|
|
||
| pub fn getSlotBlocking(self: *SnapshotMetadata, runner: lib.runner.Connection) !Slot { | ||
| while (true) { | ||
| const slot = self.slot.load(.acquire); | ||
| if (slot != invalid_slot) { | ||
| try runner.activity.signalActive(); | ||
| return slot; | ||
| } | ||
| try runner.activity.signalIdleSpinning(); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Moving away from ring buffers has made this struct easier to use, but it's still a bit esoteric. You have to know that you're supposed to read getSlotBlocking before reading other fields. It's an unusual design that's not clear from looking at the code and it requires reading the type's documentation to understand how to use it correctly. Personally I prefer when the code is more self explanatory. What do you think about something like this:
pub const SnapshotMetadata = extern struct {
slot: u64,
manifest: lib.solana.snapshot.Manifest,
status_cache: lib.solana.snapshot.StatusCache,
memory_len: usize,
memory: [0]u8 align(16), // VLA for [0..memory_len]
pub fn init(self: *SnapshotMetadata, memory_len: usize) void {
self.memory_len = memory_len;
}
/// Gets a slice of the serializable memory of the SnapshotMetadata
pub fn getSerializable(self: *SnapshotMetadata) []u8 {
// skip the slot & return all bytes from manifest onwards
comptime std.debug.assert(@offsetOf(SnapshotMetadata, "slot") == 0);
comptime std.debug.assert(@offsetOf(SnapshotMetadata, "manifest") == 8);
const header_size = @sizeOf(SnapshotMetadata) - 8;
return @as([*]u8, @ptrCast(&self.manifest))[0 .. header_size + self.memory_len];
}
/// Returns the base pointer used to resolve `RelativeSlice`/`RelativeOffset`
/// values inside `manifest` / `status_cache`.
pub fn getMemory(self: *SnapshotMetadata) []u8 {
return self.memory[0..].ptr[0..self.memory_len];
}
};
/// Stored in the region
pub const SharedSnapshotMetadata = extern struct {
is_ready: std.atomic.Value(bool),
/// not safe to read until is_ready is true
private: SnapshotMetadata,
/// Unblocks all getBlocking() callers.
/// Can be called only once.
/// Should also only call after all other SnapshotMetadata fields are populated.
pub fn finalize(self: *SharedSnapshotMetadata) void {
std.debug.assert(self.is_ready.swap(true, .release) == false);
}
pub fn getBlocking(
self: *SharedSnapshotMetadata,
runner: lib.runner.Connection,
) !*SnapshotMetadata {
while (true) {
if (self.is_ready.load(.acquire)) {
try runner.activity.signalActive();
return &self.private;
}
try runner.activity.signalIdleSpinning();
}
}
};
| pub fn isNull(self: Self) bool { | ||
| return self.offset == 0; |
There was a problem hiding this comment.
Why does every type need to be nullable? I'd rather express this in the concrete type by using an optional type as T if necessary.
There was a problem hiding this comment.
RelativeOffset is stored in Manifest/StatusCache and must be extern/region compatible. Zig's optionals (like slices) dont have a stable ABI
| vote_accounts: RelativeSlice(Pubkey), // acc.data contains stake for the voter | ||
| stake_accounts: RelativeSlice(Pubkey), // acc.data contains voter pubkey + Delegation |
There was a problem hiding this comment.
Your intent is for replay's bootstrap process to fetch all these accounts from accountsdb before starting replay? Avoiding that would require sending 8 extra bytes per vote account, and 64 extra bytes per stake account. Is that a problem?
StakeDelegation {
voter_pubkey: Pubkey, // 32 which vote account this stake delegates to
stake: u64, // 8 delegated lamports
activation_epoch: Epoch, // 8 warmup/cooldown math needs both epochs
deactivation_epoch: Epoch, // 8
credits_observed: u64, // 8 reward = (voter_credits_now − credits_observed) * pts
}
There was a problem hiding this comment.
Yes. It'll have to fetch them from AccountsDB to verify that the data in the snapshot manifest matches the accounts on disk: https://github.com/anza-xyz/agave/blob/master/runtime/src/stakes.rs#L344
This struct just then contains what to fetch, where we'll use the account data directly instead of verifying fields that the snapshot manifest presented. Should we still verify those fields?
| node_to_vote_accounts: RelativeSlice(NodeToVoterEntry), | ||
| epoch_authorized_voters: RelativeSlice(AuthToVoterEntry), |
There was a problem hiding this comment.
Do we actually need these two fields for anything?
There was a problem hiding this comment.
Add
BankFields.stakesandExtraFields.VersionedEpochStakesparsing to snapshot.zig. Also simplifies how that parsed data is communicated to replay.