Add execution blockNumber to the notifier log - #5297
Merged
Conversation
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
dapplion
reviewed
Mar 23, 2023
dapplion
left a comment
dapplion
left a comment
Contributor
There was a problem hiding this comment.
Not strongly opposed to this but the status log is already pretty long. Maybe we should add some guidelines so the status log does not grow forever. i.e. let's try to formalize a bit the purpose of this log and what information makes it in, and what does not. I can imagine that with new forks more and more data would be appealing to be displayed here
| const protoNodeSszType = new ContainerType( | ||
| { | ||
| executionPayloadBlockHash: stringType, | ||
| executionPayloadNumber: ssz.UintNum64, |
Contributor
There was a problem hiding this comment.
Is this API standardized?
Contributor
Author
There was a problem hiding this comment.
ummm ... i don't think so, double checked
Contributor
Author
right!, may be we can further truncate the hash of head exec-block since number + first 4 hash bytes are good enough to identify where execution head is: so like
We can also apply same principal for beacon head so: May be we can also drop exec-block bit from finalized and let it remain unchanged, so final log could look like: |
Contributor
|
Yes definitely better not to add in the finalized, so this log looks good
|
Contributor
Author
how about this: Mar-24 11:09:05.434[] info: Syncing - 10 minutes left - 6.32 slots/s - head: 6065535 (clock: 6069343) 0x6c82…486d exec-block: valid(16893200 0xadfe…) - finalized: 0x14bd…d8d6:189545 - peers: 7
Mar-24 11:09:17.473[] info: Syncing - 9.8 minutes left - 6.35 slots/s - head: 6065599 (clock: 6069344) 0xb659…7109 exec-block: valid(16893264 0xf32f…) - finalized: 0xb551…b203:189548 - peers: 9
Mar-24 11:16:29.187[] info: Syncing - 3.4 minutes left - 4.77 slots/s - head: 6068415 (clock-965) 0xeddf…e97b exec-block: valid(16896048 0x5d16…) - finalized: 0xd7ba…8386:189636 - peers: 9
valid(16896681 0xc9a6…9b3a) - finalized: 0x495d…f884:189656 - peers: 10
Mar-24 11:18:53.033[] info: Syncing - 42 seconds left - 5.77 slots/s - head: 6069151 (clock-241) 0xdb52…77b7 exec-block: valid(16896777 0x0c24…) - finalized: 0x6777…d015:189659 - peers: 17
Mar-24 11:19:17.031[] info: Syncing - 18 seconds left - 6.55 slots/s - head: 6069279 (clock-115) 0xd6bb…4ecd exec-block: valid(16896905 0x6599…) - finalized: 0x4658…51b6:189663 - peers: 11
Mar-24 11:19:29.047[] info: Syncing - 7.8 seconds left - 6.67 slots/s - head: 6069343 (clock-52) 0xf111…4552 exec-block: valid(16896968 0x6a16…) - finalized: 0x4e1e…3499:189665 - peers: 14
Mar-24 11:19:35.675[network] info: Subscribed gossip core topics
Mar-24 11:37:29.001[] info: Synced - head: 6069482 0xa9fd…ddab exec-block: valid(16897109 0x4951…) - finalized: 0xec9e…9cc1:189669 - peers: 27
Mar-24 11:37:17.000[] info: Synced - head: 6069482 (clock-1) 0x88b2…fb8c exec-block: valid(16897108 0x90c9…) - finalized: 0xec9e…9cc1:189669 - peers: 31
Mar-24 11:37:29.001[] info: Synced - head: 6069484 0xa9fd…ddab exec-block: valid(16897109 0x4951…) - finalized: 0xec9e…9cc1:189669 - peers: 27Explanation:
|
Member
|
I think it no matter what the format is, we should include in our docs what each thing in the log means. I was used to the |
Contributor
Author
added |
g11tech
enabled auto-merge (squash)
March 24, 2023 18:27
dapplion
disabled auto-merge
March 28, 2023 01:43
dapplion
previously approved these changes
Mar 28, 2023
dapplion
left a comment
dapplion
left a comment
Contributor
There was a problem hiding this comment.
Disabled auto-merging to allow others to 👍 before merging
Member
|
I had a chat with @wemeetagain yesterday about this PR, specifically about the logging which can be further shortened in our opinion to reduce any redundancy of information. If we can find the most concise way to explain something that is very clear without additional context and definitions in docs, we've hit the jackpot. At least for the slot numbers and identifying where the head is, it could be shortened to: |
Contributor
Author
|
Problem with slot as clock in the front is that when syncing from far away, this is confusing to others: as slot and head are both very generic , also clock slot info as the first thing isn't the concern of the users: What is relevant to them are:
current clock info is only relevant w.r.t. the local head |
Member
So for syncing, you're right and that a lot of this information may not even be required. Maybe we can rethink something for We currently see something like: info: Syncing - 2.8 hours left - 3.19 slots/s - slot: 5314216 (skipped 32552) - head: 5281664 0x14d0…8776
info: Syncing - 2.8 hours left - 3.22 slots/s - slot: 5314217 (skipped 32490) - head: 5281727 0x1d24…d906
info: Syncing - 2.7 hours left - 3.32 slots/s - slot: 5314218 (skipped 32459) - head: 5281759 0x0646…8590And with the above idea we will see: info: Syncing - 2.8 hours left - 3.19 slots/s - slot: 5314216 - head: (slot -32552) 0x14d0…8776
info: Syncing - 2.8 hours left - 3.22 slots/s - slot: 5314217 - head: (slot -32490) 0x1d24…d906
info: Syncing - 2.7 hours left - 3.32 slots/s - slot: 5314218 - head: (slot -32459) 0x0646…8590
info: Syncing - 2.8 hours left - 3.19 slots/s - head: 5281664 0x14d0…8776
info: Syncing - 2.8 hours left - 3.22 slots/s - head: 5281727 0x1d24…d906
info: Syncing - 2.7 hours left - 3.32 slots/s - head: 5281759 0x0646…8590 |
wemeetagain
added
the
meta-discussion
g11tech
force-pushed
the
g11tech/notify-execution-blocknum
branch
from
April 20, 2023 15:42
166fc52 to
80eeb95
Compare
Contributor
Author
|
Ok this is how the sync logs will look r.n. |
g11tech
enabled auto-merge (squash)
April 20, 2023 15:53
wemeetagain
approved these changes
Apr 20, 2023
g11tech
added a commit
that referenced
this pull request
Apr 24, 2023
Member
|
🎉 This PR is included in v1.8.0 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add execution blockNumber to the notifier log
We only display execution head's blockHash while execution block number is also an important info that people from EL land (ethereumjs specifically) have asked for in the log.
result:
