Skip to content

Truncated Display impl for ExecutionBlockHash#9108

Merged
mergify[bot] merged 4 commits intosigp:unstablefrom
chong-he:shorter-info-log
Apr 9, 2026
Merged

Truncated Display impl for ExecutionBlockHash#9108
mergify[bot] merged 4 commits intosigp:unstablefrom
chong-he:shorter-info-log

Conversation

@chong-he
Copy link
Copy Markdown
Member

@chong-he chong-he commented Apr 8, 2026

Issue Addressed

Proposed Changes

The intention is to only modify the INFO logs that's emitted regularly to reduce the verbosity. But I understand that this change will affect other display in the logs too that uses the ExecutionBlockHash display. So would love some feedbacks about the change.

Additional Info

Before:

Apr 08 10:42:17.000 INFO Synced peers: "30", exec_hash: "0x34b19d1c1e27a6da19e0cd1e399aa6be25e79844558a90ad725a3d7f7555a957 (unverified)", finalized_root: 0x7d486be7f3979e50952e45f3d55b52bb0467e7defad79aa3ad9443d8da092749, finalized_epoch: 439560, epoch: 439562, block: "0x8135c79dc63ec70bdf890b1040282ac8d12743d10a44858990d0024c2726189c", slot: 14066009

After:

Apr 08 11:17:17.000 INFO Synced peers: "37", exec_hash: "0xf9d6…4778 (unverified)", finalized_root: 0x2e01…5ff9, finalized_epoch: 439566, epoch: 439568, block: "0xf7c5…afee", slot: 14066184

@chong-he chong-he added ready-for-review The code is ready for review low-hanging-fruit Easy to resolve, get it before someone else does! UX-and-logs labels Apr 8, 2026
}
}

impl fmt::Display for ExecutionBlockHash {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moving this here because this is just under impl fmt::Debug for ExecutionBlockHash so I thought it would be cleaner this way

@macladson macladson self-requested a review April 8, 2026 13:12
Copy link
Copy Markdown
Member

@macladson macladson left a comment

Choose a reason for hiding this comment

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

I like the idea of this PR and the new Display impl is good, but I think the implementation is a bit overzealous. I think this PR would be better as a targeted "truncated Display impl for ExecutionBlockHash" PR.

Comment thread beacon_node/client/src/notifier.rs Outdated
Comment thread beacon_node/client/src/notifier.rs Outdated
@macladson macladson added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Apr 8, 2026
@chong-he
Copy link
Copy Markdown
Member Author

chong-he commented Apr 9, 2026

If I understand you correctly, we keep others logs unchanged and just changed the impl for ExecutionBlockHash, so the final log looks like this:

Apr 09 16:58:29.000 INFO Synced peers: "19", exec_hash: "0x2388…9dde (unverified)", finalized_root: 0x65945b2482f5e1f42158a94c167d6a7fb727a55ddf4ec123f9ae2220c590d132, finalized_epoch: 439844, epoch: 439846, block: "0x22783cac5c0e8da4bac41a32f56aa4eacdc8d22b3d508001db05991e5df0d4c5", slot: 14075090

i.e., only the exec_hash is truncated, and not the block_root and finalized_root

head_root is of type FixedBytes<32>, which is from the external crate alloy-primitives.
finalized_root is of type Hash256, which is defined as: pub type Hash256 = alloy_primitives::B256 which is also from alloy-primitives.
So we keep these unchanged

@chong-he chong-he changed the title Shorter info log Truncated Display impl for ExecutionBlockHash Apr 9, 2026
@chong-he chong-he added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Apr 9, 2026
Copy link
Copy Markdown
Member

@macladson macladson left a comment

Choose a reason for hiding this comment

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

Exactly, this looks good now!
Note that this is only a partial solution to #6689.
We need to come up with a solution which works for alloy_primitives::B256 but that should be a separate PR

@macladson macladson added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Apr 9, 2026
@mergify mergify Bot added the queued label Apr 9, 2026
@mergify
Copy link
Copy Markdown

mergify Bot commented Apr 9, 2026

Merge Queue Status

This pull request spent 29 minutes 40 seconds in the queue, including 27 minutes 31 seconds running CI.

Required conditions to merge

mergify Bot added a commit that referenced this pull request Apr 9, 2026
@mergify mergify Bot merged commit c615210 into sigp:unstable Apr 9, 2026
39 checks passed
@mergify mergify Bot removed the queued label Apr 9, 2026
@chong-he chong-he deleted the shorter-info-log branch April 9, 2026 12:17
mergify Bot pushed a commit that referenced this pull request Apr 28, 2026
#6689


  Inspired by the initial implementation of #9108, credit to @chong-he.
This adds an extension trait to `Hash256` and add a `short` method to provide smaller formatted hashes for logging.


Co-Authored-By: Mac L <mjladson@pm.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

low-hanging-fruit Easy to resolve, get it before someone else does! ready-for-merge This PR is ready to merge. UX-and-logs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants