Skip to content

Commit f8dbda4

Browse files
dappliondapplion
committed
Add more details to errors
1 parent 8931141 commit f8dbda4

File tree

2 files changed

+97
-42
lines changed

2 files changed

+97
-42
lines changed

‎beacon_node/beacon_chain/src/migrate.rs‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -524,7 +524,7 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
524524
}
525525

526526
StateSummariesDAG::new_from_v22(state_summaries)
527-
.map_err(|e| PruningError::SummariesDagError("creating StateSumariesDAG", e))?
527+
.map_err(|e| PruningError::SummariesDagError("new StateSumariesDAG", e))?
528528
};
529529

530530
// To debug faulty trees log if we unexpectedly have more than one root. These trees may not
@@ -546,9 +546,7 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
546546
std::iter::once(new_finalized_state_root).chain(
547547
state_summaries_dag
548548
.descendants_of(&new_finalized_state_root)
549-
.map_err(|e| {
550-
PruningError::SummariesDagError("state summaries descendants_of", e)
551-
})?,
549+
.map_err(|e| PruningError::SummariesDagError("descendants of", e))?,
552550
),
553551
);
554552

@@ -565,17 +563,15 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
565563
// should never error, we just constructed
566564
// finalized_and_descendant_state_roots_of_finalized_checkpoint from the
567565
// state_summaries_dag
568-
.map_err(|e| {
569-
PruningError::SummariesDagError("state summaries blocks of descendant", e)
570-
})?
566+
.map_err(|e| PruningError::SummariesDagError("blocks of descendant", e))?
571567
.into_iter()
572568
.map(|(block_root, _)| block_root),
573569
);
574570

575571
// Note: ancestors_of includes the finalized state root
576572
let newly_finalized_state_summaries = state_summaries_dag
577573
.ancestors_of(new_finalized_state_root)
578-
.map_err(|e| PruningError::SummariesDagError("state summaries ancestors_of", e))?;
574+
.map_err(|e| PruningError::SummariesDagError("ancestors of", e))?;
579575
let newly_finalized_state_roots = newly_finalized_state_summaries
580576
.iter()
581577
.map(|(root, _)| *root)
@@ -589,9 +585,7 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
589585
// Note: ancestors_of includes the finalized block
590586
let newly_finalized_blocks = state_summaries_dag
591587
.blocks_of_states(newly_finalized_state_roots.iter())
592-
.map_err(|e| {
593-
PruningError::SummariesDagError("state summaries blocks of newly finalized", e)
594-
})?;
588+
.map_err(|e| PruningError::SummariesDagError("blocks of newly finalized", e))?;
595589

596590
// We don't know which blocks are shared among abandoned chains, so we buffer and delete
597591
// everything in one fell swoop.

‎beacon_node/beacon_chain/src/summaries_dag.rs‎

Lines changed: 92 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use itertools::Itertools;
22
use std::{
33
cmp::Ordering,
4-
collections::{BTreeMap, HashMap},
4+
collections::{btree_map::Entry, BTreeMap, HashMap},
55
};
66
use types::{Hash256, Slot};
77

@@ -33,18 +33,41 @@ pub struct StateSummariesDAG {
3333

3434
#[derive(Debug)]
3535
pub enum Error {
36+
DuplicateStateSummary {
37+
block_root: Hash256,
38+
existing_state_summary: Box<(Slot, Hash256)>,
39+
new_state_summary: (Slot, Hash256),
40+
},
3641
MissingStateSummary(Hash256),
37-
MissingStateSummaryByBlockRoot(Hash256),
38-
MissingStateSummaryAtSlot(Hash256, Slot),
39-
MissingChildBlockRoot(Hash256),
42+
MissingStateSummaryByBlockRoot {
43+
state_root: Hash256,
44+
latest_block_root: Hash256,
45+
},
46+
StateSummariesNotContiguous {
47+
state_root: Hash256,
48+
state_slot: Slot,
49+
latest_block_root: Hash256,
50+
parent_block_root: Box<Hash256>,
51+
parent_block_latest_state_summary: Box<Option<(Slot, Hash256)>>,
52+
},
4053
MissingChildStateRoot(Hash256),
41-
MissingBlock(Hash256),
42-
RequestedSlotAboveSummary(Hash256, Slot),
43-
RootUnknownPreviousStateRoot(Hash256, Slot),
54+
RequestedSlotAboveSummary {
55+
starting_state_root: Hash256,
56+
ancestor_slot: Slot,
57+
state_root: Hash256,
58+
state_slot: Slot,
59+
},
60+
RootUnknownPreviousStateRoot(Slot, Hash256),
61+
RootUnknownAncestorStateRoot {
62+
starting_state_root: Hash256,
63+
ancestor_slot: Slot,
64+
root_state_root: Hash256,
65+
root_state_slot: Slot,
66+
},
4467
}
4568

4669
impl StateSummariesDAG {
47-
pub fn new(state_summaries: Vec<(Hash256, DAGStateSummary)>) -> Self {
70+
pub fn new(state_summaries: Vec<(Hash256, DAGStateSummary)>) -> Result<Self, Error> {
4871
// Group them by latest block root, and sorted state slot
4972
let mut state_summaries_by_state_root = HashMap::new();
5073
let mut state_summaries_by_block_root = HashMap::<_, BTreeMap<_, _>>::new();
@@ -55,8 +78,19 @@ impl StateSummariesDAG {
5578
.entry(summary.latest_block_root)
5679
.or_default();
5780

58-
// TODO(hdiff): error if existing
59-
summaries.insert(summary.slot, (state_root, summary));
81+
// Sanity check to ensure no duplicate summaries for the tuple (block_root, state_slot)
82+
match summaries.entry(summary.slot) {
83+
Entry::Vacant(entry) => {
84+
entry.insert((state_root, summary));
85+
}
86+
Entry::Occupied(existing) => {
87+
return Err(Error::DuplicateStateSummary {
88+
block_root: summary.latest_block_root,
89+
existing_state_summary: (summary.slot, state_root).into(),
90+
new_state_summary: (*existing.key(), existing.get().0),
91+
})
92+
}
93+
}
6094

6195
state_summaries_by_state_root.insert(state_root, summary);
6296

@@ -68,11 +102,11 @@ impl StateSummariesDAG {
68102
child_state_roots.entry(state_root).or_default();
69103
}
70104

71-
Self {
105+
Ok(Self {
72106
state_summaries_by_state_root,
73107
state_summaries_by_block_root,
74108
child_state_roots,
75-
}
109+
})
76110
}
77111

78112
/// Computes a DAG from a sequence of state summaries, including their parent block
@@ -92,8 +126,19 @@ impl StateSummariesDAG {
92126
.entry(summary.latest_block_root)
93127
.or_default();
94128

95-
// TODO(hdiff): error if existing
96-
summaries.insert(summary.slot, (state_root, summary));
129+
// Sanity check to ensure no duplicate summaries for the tuple (block_root, state_slot)
130+
match summaries.entry(summary.slot) {
131+
Entry::Vacant(entry) => {
132+
entry.insert((state_root, summary));
133+
}
134+
Entry::Occupied(existing) => {
135+
return Err(Error::DuplicateStateSummary {
136+
block_root: summary.latest_block_root,
137+
existing_state_summary: (summary.slot, *state_root).into(),
138+
new_state_summary: (*existing.key(), *existing.get().0),
139+
})
140+
}
141+
}
97142
}
98143

99144
let state_summaries = state_summaries_v22
@@ -109,9 +154,10 @@ impl StateSummariesDAG {
109154
.get(&summary.latest_block_root)
110155
// Should never error: we construct the HashMap here and must have at least
111156
// one entry per block root
112-
.ok_or(Error::MissingStateSummaryByBlockRoot(
113-
summary.latest_block_root,
114-
))?;
157+
.ok_or(Error::MissingStateSummaryByBlockRoot {
158+
state_root: *state_root,
159+
latest_block_root: summary.latest_block_root,
160+
})?;
115161
if let Some((state_root, _)) = same_block_root_summaries.get(&previous_slot) {
116162
// Skipped slot: block root at previous slot is the same as latest block root.
117163
**state_root
@@ -123,12 +169,19 @@ impl StateSummariesDAG {
123169
{
124170
*parent_block_summaries
125171
.get(&previous_slot)
126-
// Should never error: summaries are continuous, so if there's an
172+
// Should never error: summaries are contiguous, so if there's an
127173
// entry it must contain at least one summary at the previous slot.
128-
.ok_or(Error::MissingStateSummaryAtSlot(
129-
parent_block_root,
130-
previous_slot,
131-
))?
174+
.ok_or(Error::StateSummariesNotContiguous {
175+
state_root: *state_root,
176+
state_slot: summary.slot,
177+
latest_block_root: summary.latest_block_root,
178+
parent_block_root: parent_block_root.into(),
179+
parent_block_latest_state_summary: parent_block_summaries
180+
.iter()
181+
.max_by(|a, b| a.0.cmp(b.0))
182+
.map(|(slot, (state_root, _))| (*slot, **state_root))
183+
.into(),
184+
})?
132185
.0
133186
} else {
134187
// We don't know of any summary with this parent block root. We'll
@@ -153,7 +206,7 @@ impl StateSummariesDAG {
153206
})
154207
.collect::<Result<Vec<_>, _>>()?;
155208

156-
Ok(Self::new(state_summaries))
209+
Self::new(state_summaries)
157210
}
158211

159212
// Returns all non-unique latest block roots of a given set of states
@@ -219,8 +272,8 @@ impl StateSummariesDAG {
219272
.ok_or(Error::MissingStateSummary(state_root))?;
220273
if summary.previous_state_root == Hash256::ZERO {
221274
Err(Error::RootUnknownPreviousStateRoot(
222-
state_root,
223275
summary.slot,
276+
state_root,
224277
))
225278
} else {
226279
Ok(summary.previous_state_root)
@@ -229,9 +282,10 @@ impl StateSummariesDAG {
229282

230283
pub fn ancestor_state_root_at_slot(
231284
&self,
232-
mut state_root: Hash256,
285+
starting_state_root: Hash256,
233286
ancestor_slot: Slot,
234287
) -> Result<Hash256, Error> {
288+
let mut state_root = starting_state_root;
235289
// Walk backwards until we reach the state at `ancestor_slot`.
236290
loop {
237291
let summary = self
@@ -242,17 +296,24 @@ impl StateSummariesDAG {
242296
// Assumes all summaries are contiguous
243297
match summary.slot.cmp(&ancestor_slot) {
244298
Ordering::Less => {
245-
return Err(Error::RequestedSlotAboveSummary(state_root, ancestor_slot))
299+
return Err(Error::RequestedSlotAboveSummary {
300+
starting_state_root,
301+
ancestor_slot,
302+
state_root,
303+
state_slot: summary.slot,
304+
})
246305
}
247306
Ordering::Equal => {
248307
return Ok(state_root);
249308
}
250309
Ordering::Greater => {
251310
if summary.previous_state_root == Hash256::ZERO {
252-
return Err(Error::RootUnknownPreviousStateRoot(
253-
state_root,
254-
summary.slot,
255-
));
311+
return Err(Error::RootUnknownAncestorStateRoot {
312+
starting_state_root,
313+
ancestor_slot,
314+
root_state_root: state_root,
315+
root_state_slot: summary.slot,
316+
});
256317
} else {
257318
state_root = summary.previous_state_root;
258319
}
@@ -313,7 +374,7 @@ mod tests {
313374
fn assert_previous_state_root_is_zero(dag: &StateSummariesDAG, root: Hash256) {
314375
assert!(matches!(
315376
dag.previous_state_root(root).unwrap_err(),
316-
Error::RootUnknownPreviousStateRoot(..)
377+
Error::RootUnknownPreviousStateRoot { .. }
317378
));
318379
}
319380

0 commit comments

Comments
 (0)