-
Notifications
You must be signed in to change notification settings - Fork 5
Publish rollback messages #444
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
Conversation
| ChainEvent::RollForward { header, body } => { | ||
| self.block_sink.announce_roll_forward(header, body).await?; | ||
| self.published_blocks += 1; | ||
| if self.published_blocks.is_multiple_of(100) { |
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.
This might be a silly question, but why do we log that a specific block was published every 100 blocks?
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.
Two reasons:
UpstreamChainFetcherhad the same line- it's been useful for me, for observing signs of life
| }; | ||
| let hash = Hash::try_from(hash).unwrap_or_default(); | ||
| if let Some(slot_blocks) = self.blocks.get(&slot) | ||
| && let Some(header) = slot_blocks.header(hash) |
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.
I didn't know you could let chain like this!
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's a feature that's less than a year old, my favorite from Rust 2024 edition
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.
Yay!
| } | ||
|
|
||
| #[test] | ||
| fn should_gracefully_switch_to_new_chain_at_older_head() { |
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.
Great tests!!
whankinsiv
left a comment
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.
Looks great! Really nice tests.
Description
Update the PeerNetworkInterface module to produce rollback messages whenever it rolls back. We need this for custom indexers, which should be able to handle rollbacks immediately without waiting for a roll forward.
The logic changes were relatively involved:
This also fixes some other chain-sync bugs:
Related Issue(s)
Completes #421
How was this tested?
Almost entirely through unit tests, though with some manual tests.
Checklist
Impact / Side effects
The node can actually emit rollback messages now, meaning they propagate downstream.
Reviewer notes / Areas to focus
The most involved method is
switch_head_to_peer, which gets called whenever we change which peer we're following. It has to gracefully handle rolling our chain back to that peer's head, and if we don't know where that peer's head is yet it sets a flag to call it again when we know more.