Add proto_array fork choice #804
Conversation
paulhauner
added
ready-for-review
| /// If some balances are found, they are removed from the cache. | ||
| pub fn get(&mut self, block_root: Hash256) -> Option<Vec<u64>> { | ||
| let i = self.position(block_root)?; | ||
| Some(self.items.remove(i).balances) |
There was a problem hiding this comment.
I was surprised by the remove here, let me see if I have the reasoning right.
Whenever we add a block to fork choice, we call CheckpointManager::process_state, which attempts to fetch the balances from the cache for that block's state's current_justified_checkpoint. In the best case, this current justified checkpoint (CJC) is the same as the CJC for the last block processed, so we remove its entry from the cache (a hit), and then re-add an equivalent entry when we call BalancesCache::process_state. So the only time balances should actually need to be loaded from an on-disk state is on the epoch boundary where the CJC usually updates, and will not be found in the cache?
There was a problem hiding this comment.
I'm not sure I follow your description, so perhaps I can respond with my own:
- Every time we call
process_statewith a state that is from the first block in the epoch, we cache it's balances. - Whenever we process a block that has a new higher CJC, we try the cache for the balances associated with
CJC.root.- Since the cache has a size of
4, if there are no forks we should get a cache hit as long as there haven't been 4 epochs since justification. If this is not the case (or there are forks washing out the cache) then we load from an on-disk cache.
- Since the cache has a size of
WRT to the remove, I think you're right that we might end up doing unnecessary on-disk reads when we don't update current. I'll fix this up in the v0.10.1 update -- in it's current state I don't think it will produce errors, just inefficiency.
There was a problem hiding this comment.
Ok, I agree with your assessment. I'll leave this convo unresolved so we have a pointer back to it for v0.10.1
michaelsproul
added
waiting-on-author
|
Alrighty, I think we're all good here :) Let see if tests pass. |
Issue Addressed
NA
Proposed Changes
ReducedTreeand replaces it withProtoArray.ProtoArraystruct as JSON.Notes
Spec
v0.10.1introduces changes to fork choice that affect theCheckpointManager. I have not included these changes here yet, they will require a refactor ofCheckpointManager. So, when you're reviewing that section, remember that it will soon be refactored (this is why it doesn't have tests).TODO: