Skip to content
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

Log imported block info post merge #4310

Merged
merged 5 commits into from
Aug 26, 2022

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Aug 25, 2022

After the switch to PoS, info of the imported blocks are not more logged, this PR re-add the log when a block is imported via the NewPayload engine RPC call, and also update the info logged to remove ommers and add base fee.

Sample of the message:
Imported #12,862,033 / 5 tx / 439,018 (1.5%) gas / base fee 7 wei / (0xb1b6d51e6382a1c88b42344f53d5321ee63bec0226fae502876af55e7109f431) in 0.028s.

Signed-off-by: Fabio Di Fabio fabio.difabio@consensys.net

PR description

Fixed Issue(s)

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

Removed ommers and added base fee.

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 force-pushed the log-imported-block-info-new-payload branch from 907a39d to ed9676a Compare August 25, 2022 13:21
Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

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

Other than the question about peer count LGTM, will approve once that has a good answer.

private void logImportedBlockInfo(final Block block, final double timeInS) {
LOG.info(
String.format(
"Imported #%,d / %d tx / %,d (%01.1f%%) gas / base fee %s / (%s) in %01.3fs.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could we put basefee between tx and gas? We are dropping ommers and we could just re-use the position for basefee.

Copy link
Contributor

Choose a reason for hiding this comment

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

Question: is dropping peer count intentional? Does the ELs no longer gossip? If there is still gossip I think we want peer counts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Moved the base fee where ommes where
  2. I was thinking on logging peers count in another way, but not yet sure how, so it make sense to readd them, since there is still gossip

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 merged commit c321a85 into hyperledger:main Aug 26, 2022
@fab-10 fab-10 deleted the log-imported-block-info-new-payload branch August 26, 2022 08:40
@macfarla macfarla added the 22.7.2 label Sep 4, 2022
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Sep 7, 2022
Removed ommers and added base fee.

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Sep 7, 2022
Removed ommers and added base fee.

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Sep 7, 2022
Removed ommers and added base fee.

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: garyschulte <garyschulte@gmail.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
Removed ommers and added base fee.

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants