Skip to content

Skip malformed live chain keys #443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
samsamtrum wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Skip malformed live chain keys #443

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 41 additions & 12 deletions crates/storage/src/store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,10 +469,14 @@ fn encode_live_chain_key(slot: u64, root: &H256) -> Vec<u8> {
}

/// Decode a LiveChain key from bytes.
fn decode_live_chain_key(bytes: &[u8]) -> (u64, H256) {
fn decode_live_chain_key(bytes: &[u8]) -> Option<(u64, H256)> {
if bytes.len() != 40 {
return None;
}

let slot = u64::from_be_bytes(bytes[..8].try_into().expect("valid slot bytes"));
let root = H256::from_slice(&bytes[8..]);
(slot, root)
Some((slot, root))
Comment on lines +472 to +479

@greptile-apps greptile-apps Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 No diagnostic when malformed keys are encountered

Malformed keys are silently skipped across all three read paths (get_live_chain, max_live_chain_slot, get_block_roots) and silently deleted in prune_live_chain. If corrupt data reaches the store due to a bug in a write path, there is no observable signal in logs or metrics. A tracing::warn! (or equivalent) inside decode_live_chain_key on the None branch would surface the anomaly without changing observable behavior for callers.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/storage/src/store.rs
Line: 472-479

Comment:
**No diagnostic when malformed keys are encountered**

Malformed keys are silently skipped across all three read paths (`get_live_chain`, `max_live_chain_slot`, `get_block_roots`) and silently deleted in `prune_live_chain`. If corrupt data reaches the store due to a bug in a write path, there is no observable signal in logs or metrics. A `tracing::warn!` (or equivalent) inside `decode_live_chain_key` on the `None` branch would surface the anomaly without changing observable behavior for callers.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

}

/// Fork choice store backed by a pluggable storage backend.
Expand Down Expand Up @@ -819,10 +823,10 @@ impl Store {
view.prefix_iterator(Table::LiveChain, &[])
.expect("iterator")
.filter_map(|res| res.ok())
.map(|(k, v)| {
let (slot, root) = decode_live_chain_key(&k);
.filter_map(|(k, v)| {
let (slot, root) = decode_live_chain_key(&k)?;
let parent_root = H256::from_ssz_bytes(&v).expect("valid parent_root");
(root, (slot, parent_root))
Some((root, (slot, parent_root)))
})
.collect()
}
Expand All @@ -833,7 +837,7 @@ impl Store {
view.prefix_iterator(Table::LiveChain, &[])
.expect("iterator")
.filter_map(Result::ok)
.map(|(key, _)| decode_live_chain_key(&key).0)
.filter_map(|(key, _)| decode_live_chain_key(&key).map(|(slot, _)| slot))
.max()
}

Expand All @@ -845,10 +849,7 @@ impl Store {
view.prefix_iterator(Table::LiveChain, &[])
.expect("iterator")
.filter_map(|res| res.ok())
.map(|(k, _)| {
let (_, root) = decode_live_chain_key(&k);
root
})
.filter_map(|(k, _)| decode_live_chain_key(&k).map(|(_, root)| root))
.collect()
}

Expand All @@ -868,8 +869,9 @@ impl Store {
.expect("iterator")
.filter_map(|res| res.ok())
.take_while(|(k, _)| {
let (slot, _) = decode_live_chain_key(k);
slot < finalized_slot
decode_live_chain_key(k)
.map(|(slot, _)| slot < finalized_slot)
.unwrap_or(true)
})
Comment on lines 871 to 875

@greptile-apps greptile-apps Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 unwrap_or(true) silently deletes malformed keys during any prune

When decode_live_chain_key returns None, unwrap_or(true) passes the predicate, so malformed keys are included in keys_to_delete and deleted — even if finalized_slot is 0 or the key sorts well after the finalization boundary. The PR description says read paths "skip" malformed entries, but in prune_live_chain they are always deleted. This is probably fine (garbage-collecting corrupt keys), but a comment clarifying the intent would help future readers understand why true (rather than false) is the fallback here.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/storage/src/store.rs
Line: 871-875

Comment:
**`unwrap_or(true)` silently deletes malformed keys during any prune**

When `decode_live_chain_key` returns `None`, `unwrap_or(true)` passes the predicate, so malformed keys are included in `keys_to_delete` and deleted — even if `finalized_slot` is 0 or the key sorts well after the finalization boundary. The PR description says read paths "skip" malformed entries, but in `prune_live_chain` they are always deleted. This is probably fine (garbage-collecting corrupt keys), but a comment clarifying the intent would help future readers understand why `true` (rather than `false`) is the fallback here.

How can I resolve this? If you propose a fix, please make it concise.

.map(|(k, _)| k.to_vec())
.collect();
Expand Down Expand Up @@ -1420,6 +1422,33 @@ mod tests {
use super::*;
use crate::backend::InMemoryBackend;

#[test]
fn live_chain_queries_skip_malformed_keys() {
let backend = Arc::new(InMemoryBackend::new());
let store = Store::test_store_with_backend(backend.clone());
let root = root(1);
let parent = H256::ZERO;

let mut batch = backend.begin_write().expect("write batch");
batch
.put_batch(
Table::LiveChain,
vec![
(vec![1, 2, 3], parent.to_ssz()),
(encode_live_chain_key(42, &root), parent.to_ssz()),
],
)
.expect("put live chain");
batch.commit().expect("commit");

assert_eq!(store.max_live_chain_slot(), Some(42));
assert_eq!(store.get_block_roots(), HashSet::from([root]));

let live_chain = store.get_live_chain();
assert_eq!(live_chain.len(), 1);
assert_eq!(live_chain.get(&root), Some(&(42, parent)));
}

/// Insert a block header (and dummy body + signature) for a given root and slot.
fn insert_header(backend: &dyn StorageBackend, root: H256, slot: u64) {
let header = BlockHeader {
Expand Down