Skip to content

Conversation

@dapplion
Copy link
Collaborator

@dapplion dapplion commented Apr 9, 2024

Issue Addressed

When dabbling in #5534, I noticed some code paths that should be covered by logging

Proposed Changes

Add some extra logs in block lookup

Additional Info

Noticed that SyncMessage::UnknownBlockHashFromAttestation only checks synced_and_connected whereas SyncMessage::UnknownParentBlob checks also is_execution_engine_online. Is this by design or a typo?

@realbigsean
Copy link
Member

Noticed that SyncMessage::UnknownBlockHashFromAttestation only checks synced_and_connected whereas SyncMessage::UnknownParentBlob checks also is_execution_engine_online. Is this by design or a typo?

Looks like the synced method includes the EE check

    fn synced(&mut self) -> bool {
        self.network_globals().sync_state.read().is_synced()
            && self.network.is_execution_engine_online()
    }

Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Love logging improvements

@realbigsean
Copy link
Member

@Mergifyio queue

@mergify
Copy link

mergify bot commented Apr 9, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at b74da14

mergify bot added a commit that referenced this pull request Apr 9, 2024
@mergify mergify bot merged commit b74da14 into sigp:unstable Apr 9, 2024
@dapplion dapplion deleted the sync-logging branch January 24, 2025 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants