-
Notifications
You must be signed in to change notification settings - Fork 113
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.
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
Add invalidate block method and invalidated_blocks field to NonFinalizedState #9167
base: main
Are you sure you want to change the base?
Add invalidate block method and invalidated_blocks field to NonFinalizedState #9167
Conversation
…se in vectors.rs. Updates non finalized state to track invalidated_blocks
…date-block-method-to-nonfinalizedstate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for getting this done.
This looks excellent. I left some optional suggestions, but none of them are blockers so feel free to resolve them without making changes if you'd like this to merge as-is.
/// Invalidated blocks and descendants | ||
invalidated_blocks: HashMap<block::Hash, InvalidatedBlockData>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional/Nitpick: Ideally this hash map or its values would be in an Arc
, because Zebra clones the non-finalized state fairly frequently. It shouldn't be possible to update this field without access to the RPC server, so this would be cleanup, not a security issue.
I'm also wondering if this could be simplified to be HashMap<Hash, Arc<Vec<ContextuallyValidatedBlock>>>
where the first item in the value is the explicitly invalidated block?
/// Invalidated blocks and descendants | |
invalidated_blocks: HashMap<block::Hash, InvalidatedBlockData>, | |
/// Blocks that have been invalidated in, and removed from, the non-finalized state | |
/// via the `invalidate` RPC method. | |
invalidated_blocks: Arc<HashMap<block::Hash, InvalidatedBlockData>>, |
|
||
self.update_metrics_for_chains(); | ||
self.update_metrics_bars(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should update the metrics whenever chain_set
is modified.
Optional/Nitpick: This method could drop the chain and return early when the invalidated block is the non-finalized chain root block.
self.update_metrics_for_chains(); | |
self.update_metrics_bars(); | |
} | |
} | |
self.update_metrics_for_chains(); | |
self.update_metrics_bars(); |
/// Data related to an invalidated block including the [`ContextuallyVerifiedBlock`] and | ||
/// descendants. | ||
#[derive(Clone, Debug)] | ||
pub struct InvalidatedBlockData { | ||
/// The block that was invalidated. | ||
pub block: ContextuallyVerifiedBlock, | ||
/// All descendant blocks that were also removed in order by [`block::Height`]. | ||
pub descendants: BTreeMap<block::Height, ContextuallyVerifiedBlock>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a suggestion below for removing this struct altogether, but if we're keeping it, minor documentation update:
/// Data related to an invalidated block including the [`ContextuallyVerifiedBlock`] and | |
/// descendants. | |
#[derive(Clone, Debug)] | |
pub struct InvalidatedBlockData { | |
/// The block that was invalidated. | |
pub block: ContextuallyVerifiedBlock, | |
/// All descendant blocks that were also removed in order by [`block::Height`]. | |
pub descendants: BTreeMap<block::Height, ContextuallyVerifiedBlock>, | |
/// [`ContextuallyVerifiedBlock`]s that were invalidated via the `invalidate` RPC method | |
/// and removed from the non-finalized state, as well as any of their descendants that may | |
/// have also been removed. | |
#[derive(Clone, Debug)] | |
pub struct InvalidatedBlockData { | |
/// The block that was invalidated. | |
pub block: ContextuallyVerifiedBlock, | |
/// All child blocks of the invalidated block that were removed in order by [`block::Height`]. | |
pub descendants: BTreeMap<block::Height, ContextuallyVerifiedBlock>, |
/// Timestamp in which the block was invalidated. | ||
pub timestamp: SystemTime, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the intended purpose of this timestamp?
let block_height = new_chain.height_by_hash(block_hash).unwrap(); | ||
|
||
// Split the new chain at the the `block_hash` and invalidate the block with the | ||
// block_hash along with the block's descendants | ||
let mut invalidated_blocks = new_chain.blocks.split_off(&block_height); | ||
for (_, ctx_block) in invalidated_blocks.iter().rev() { | ||
new_chain.revert_chain_with(ctx_block, chain::RevertPosition::Tip); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick/Optional: Ideally we could move this to a Chain
method, like pop_tip()
but which accepts a block hash and returns a list of ContextuallyValidatedBlock
s.
Motivation
This PR implements a new method invalidate_block to invalidate a block based on the block::Hash along with the blocks descendants. This PR closes the following issue: #8840 , #8841
Specifications & References
Reference: #8634
Solution
Tests
Follow-up Work
PR Author's Checklist
PR Reviewer's Checklist