pull Checkpoints out of #2289 #2457
Conversation
|
Looks like a sloppy specialization of a DAG. Have you looked at: |
I've just had a look, and I don't know if using it would be less code. Maybe I'd be able to copy and paste some generic DAG code for trunk() and collect(), but would that be better? |
Codecov Report
@@ Coverage Diff @@
## master #2457 +/- ##
========================================
Coverage ? 67.5%
========================================
Files ? 111
Lines ? 20089
Branches ? 0
========================================
Hits ? 13575
Misses ? 6514
Partials ? 0 |
| } | ||
| /// given a maximum depth, find the highest parent shared by all | ||
| /// forks | ||
| pub fn trunk(&self, num: usize) -> Option<u64> { |
There was a problem hiding this comment.
why not a method that does
/// returns the trunks, and an inverse dag
pub fn invert(&self) -> (HashSet<u64>, HashMap<u64, HashSet<u64>>) {
let mut trunks = HashSet::new();
let mut idag = HashMap::new();
for (k,(_,v)) in &self.checkpoints {
trunks.remove(k);
trunks.insert(*v);
idag.entry(*v).or_insert(HashSet::new()).insert(*k);
}
(trunks, idag)
}
There was a problem hiding this comment.
and then? probably what you've written is more along the lines of what you want for bank_state?
|
It's not clear what you're trying to do here (not integrated into anything, no issue referenced, and there's no PR description). You're adding what appears to be a fairly generic library awkwardly using the same name as the existing Checkpoint trait and specialized for |
new trait name |
rob-solana
force-pushed
the
checkpoints
branch
from
489f395 to
2461aeb
Compare
| checkpoints.store(4, 0, 2); | ||
|
|
||
| let _trunks: Vec<_> = checkpoints.invert().0.into_iter().collect(); | ||
| // TODO: returns [0, 2] ?? |
There was a problem hiding this comment.
@aeyakovenko is this the expected behavior?
| checkpoints.store(40, 0, 20); | ||
|
|
||
| let _trunks: Vec<_> = checkpoints.invert().0.into_iter().collect(); | ||
| // TODO: returns [0, 2, 10, 20, 1] ?? |
There was a problem hiding this comment.
@aeyakovenko is this the expected behavior? I don't understand how 1 is now part of trunks
|
updated description, tests |
* update test for shrink removing zero lamports * clean up comment
Problem
current Bank checkpoint code supports only a single stack of checkpoints, cannot support 2 simultaneous forks
Summary of Changes
implement Checkpoints, a directed acyclic graph, uses u64 (probably block index) as key for a checkpoint