Verify snapshot slot deltas

[brooksprumo]

Problem

The snapshot slot deltas are not really verified before reconstructing the status cache.

There's not a ton of verification to do here without needing to add really full-blown verification, but there is some low hanging fruit.

Summary of Changes

Verify the snapshot slot deltas before reconstructing the status cache.

PR Tasks

• verify slot lower bound
• check for duplicate slots

meta issue: #7167

[ryoqun]

👍

[ryoqun]

could you also verify lower bound of *slot with picking a valid offset after reading the relevant bank code?

[brooksprumo]

could you also verify lower bound of *slot with picking a valid offset after reading the relevant bank code?

I need to step away from my computer now; I did look through the status cache code and only saw the limits on the total number of entries (status_cache::MAX_CACHE_ENTRIES), but did not see a minimum slot check. This makes sense to me, as slots are not guaranteed to be rooted, right? Is there a minimum slot check somewhere else that you were thinking of and could point me to? TIA

[ryoqun]

Is there a minimum slot check somewhere else that you were thinking of and could point me to?

@brooksprumo (sorry for delayed review...)

hmm, there should be none atm. that's the reason snapshot code isn't well guarded in general. and you're helping remedying the situation, sounds cool. :)

come to think of it, you can compare against the slot history sysvar. For sample, see adjust_lockouts_after_replay (btw, one of my proud robust code is here, lol. fyi, it has unfixed really special case insignificant bug still...):

solana/core/src/consensus.rs

Line 1162 in ad0acaa

for slot_in_tower in slots_in_tower.iter().rev() {

Referencing sysvars for snapshot sanitization is the general direction, which I'd like to see. And, putting more deta onto chain as new sysvars as well. so that we have moar trusted (voted) on chain data for internal bank state sanitization. maybe eventually all. :)

Using the slot history sysvar, you can compute precise block height, thus lower bound in terms of slot number by looping over the sysvar MAX_CACHE_ENTRIES times.

Also, you can now validate whether the given slot delta for a slot is actually for a skipped block or not. And more, you can detect any missing slot delta which should exist actually according to the slot history sysvar.

did not see a minimum slot check. This makes sense to me, as slots are not guaranteed to be rooted, right?

hmm, there is no minimum slot check for lack of manpower. and it should be unrelated to another fact that slots aren't rooted always. What links these things together in your mind?

[brooksprumo]

you can compare against the slot history sysvar.

Ah, very cool! Thanks for the pointer.

Referencing sysvars for snapshot sanitization is the general direction, which I'd like to see. And, putting more deta onto chain as new sysvars as well. so that we have moar trusted (voted) on chain data for internal bank state sanitization. maybe eventually all. :)

Love this idea!

Using the slot history sysvar, you can compute precise block height, thus lower bound in terms of slot number by looping over the sysvar MAX_CACHE_ENTRIES times.

Also, you can now validate whether the given slot delta for a slot is actually for a skipped block or not. And more, you can detect any missing slot delta which should exist actually according to the slot history sysvar.

I've pushed a commit that attempts to do this. Thanks for the suggestion!

did not see a minimum slot check. This makes sense to me, as slots are not guaranteed to be rooted, right?

hmm, there is no minimum slot check for lack of manpower. and it should be unrelated to another fact that slots aren't rooted always. What links these things together in your mind?

This was me saying that the SlotHistory may have entries for slots [10, 11, 12, 13, 18, 19, 20], because slots 14-17 were either never produced or were on a bad fork. Same for slot deltas. So the lowest slot number in the slot deltas cannot be calculated simply as bank.slot() - MAX_CACHE_ENTRIES.

[ryoqun]

@brooksprumo sorry for late review... i added some improvement suggestions. let's make status cache robust together. :)

[brooksprumo]

@ryoqun Ok! This PR is ready for another review.

[brooksprumo]

Had to rebase onto newer master due to serde rust security issue. Should be ready for another review!

[ryoqun]

lgtm

please sanity-check that mainnet-beta vanilla snapshot doesn't vomit verification error. also, please do the same for another snapshot which is re-created from solana-ledger-tool.

[brooksprumo]

please sanity-check that mainnet-beta vanilla snapshot doesn't vomit verification error. also, please do the same for another snapshot which is re-created from solana-ledger-tool.

• sanity check that mainnet-beta vanilla snapshot doesn't vomit verification error
• sanity check that a snapshot created with solana-ledger-tool doesn't vomit verification error

Done! Verification was successful for both a freshly downloaded mnb snapshot, and one created with ledger-tool.