Skip to content

Standardise log content in AbstractEngineNewPayload and AbstractEngineForkchoiceUpdated#8568

Merged
ahamlat merged 4 commits intobesu-eth:mainfrom
Wetitpig-cross-chain:shortLog
Apr 30, 2025
Merged

Standardise log content in AbstractEngineNewPayload and AbstractEngineForkchoiceUpdated#8568
ahamlat merged 4 commits intobesu-eth:mainfrom
Wetitpig-cross-chain:shortLog

Conversation

@Wetitpig
Copy link
Copy Markdown
Contributor

@Wetitpig Wetitpig commented Apr 19, 2025 •
edited
Loading

PR description

Further to PR #7961 the logs are now rather long (183 characters), requiring font size 9 to fit on one line within a Ubuntu console at its maximum size.

Moreover, the labels are now standardised to be appended instead of prepended to values, and block hash labels are standardised without the word blockHash

printf 'Imported #21,293,789  (0xdcec.....503fa)|  212 tx| 16 ws| 2 blobs| base fee  13.78 gwei| gas used  19,512,289 ( 65.0%%)| exec time 0.249s| mgas/s  78.36| Parallel txs 36.8%%| peers: 25' | wc -c
183

This PR attempts to trim down log size.

printf 'Imported #22,300,103  (a8e1d.....b63b5)|   60 tx ( 65.0%% parallel)| 16 ws| 0 blobs| 355.69 mwei base|  4,087,850 ( 11.4%%) gas used| 0.083s exec|  49.25 Mgas/s|  7 peers' | wc -c
168

Sample log lines:

2025-04-19 19:52:31.676  INFO  AbstractEngineNewPayload - Imported #22,302,866  (744cf.....44361)|  184 tx ( 67.4% parallel)| 16 ws| 0 blobs| 314.58 mwei base| 35,919,031 (100.0%) gas used| 0.677s exec|  53.06 Mgas/s|  1 peers
2025-04-19 19:52:32.974  INFO  AbstractEngineNewPayload - Imported #22,302,867  (9a223.....88346)|  158 tx ( 66.5% parallel)| 16 ws| 5 blobs| 353.88 mwei base| 13,312,777 ( 37.0%) gas used| 0.350s exec|  38.04 Mgas/s|  1 peers
2025-04-19 19:52:33.667  INFO  AbstractEngineForkchoiceUpdated - FCU(VALID) | head: 9a223.....88346 | safe: 1c3ef.....57c2b | finalized: c290e.....c0239
2025-04-19 19:52:34.032  INFO  AbstractEngineNewPayload - Imported #22,302,868  (2f362.....7ab40)|  121 tx ( 67.8% parallel)| 16 ws| 3 blobs| 342.39 mwei base|  7,766,981 ( 21.6%) gas used| 0.347s exec|  22.38 Mgas/s|  1 peers
2025-04-19 19:52:36.378  INFO  AbstractEngineNewPayload - Imported #22,302,869  (569bf.....051bc)|   94 tx ( 75.5% parallel)| 16 ws| 6 blobs| 318.06 mwei base| 22,556,476 ( 62.7%) gas used| 0.601s exec|  37.53 Mgas/s|  1 peers
2025-04-19 19:52:37.892  INFO  AbstractEngineNewPayload - Imported #22,302,870  (7f815.....8933f)|  178 tx ( 62.9% parallel)| 16 ws| 3 blobs| 328.13 mwei base| 15,380,751 ( 42.7%) gas used| 0.453s exec|  33.95 Mgas/s|  1 peers
2025-04-19 19:52:38.595  INFO  AbstractEngineNewPayload - Imported #22,302,871  (2473d.....1771f)|   16 tx ( 56.3% parallel)| 16 ws| 0 blobs| 322.16 mwei base|  2,711,125 (  7.5%) gas used| 0.060s exec|  45.19 Mgas/s|  1 peers
2025-04-19 19:52:50.415  INFO  AbstractEngineNewPayload - Imported #22,302,872  (9d115.....a758b)|  283 tx ( 56.9% parallel)| 16 ws| 0 blobs| 287.95 mwei base| 28,756,687 ( 80.0%) gas used| 0.544s exec|  52.86 Mgas/s|  1 peers

Further trims would be most welcome.

Fork choice update block hashes are also listed in ascending order of finality (head → safe → finalized)

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • spotless: ./gradlew spotlessApply
  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@matkt matkt requested a review from ahamlat April 23, 2025 12:59
ahamlat
ahamlat requested changes Apr 30, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahamlat ahamlat 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 general suggestion, I asked few modifications in regard to what is scheduled for Fusaka.

Comment thread .../hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java Outdated
double mgasPerSec = (timeInS != 0) ? block.getHeader().getGasUsed() / (timeInS * 1_000_000) : 0;
message.append(
"|%2d blobs| base fee %s| gas used %,11d (%5.1f%%)| exec time %01.3fs| mgas/s %6.2f");
"| %d blobs| %s base| %,10d (%5.1f%%) gas used| %01.3fs exec| %6.2f Mgas/s| %2d peers");
Copy link
Copy Markdown
Contributor

@ahamlat ahamlat Apr 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

With peerdas scheduled for Fusaka, blob count is expected to exceed 10, so we should stick with at least two-digit formatting to maintain consistent log output.

Copy link
Copy Markdown
Contributor

@ahamlat ahamlat Apr 30, 2025

Choose a reason for hiding this comment

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

The same for gas used, we want to support more gas limit starting from fusaka, we are discussing about 100 and 150 million gas limit in the next months. So we still need to have 11 characters for gas used (9 digits and two commas).

Copy link
Copy Markdown
Contributor

@ahamlat ahamlat Apr 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I suggest to replace base with bfee, because base doesn't reflect the base fee and exec with time

Copy link
Copy Markdown
Contributor

@ahamlat ahamlat Apr 30, 2025

Choose a reason for hiding this comment

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

We can also replace gas used with gas to reduce more

Copy link
Copy Markdown
Contributor Author

@Wetitpig Wetitpig Apr 30, 2025

Choose a reason for hiding this comment

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

We can also replace gas used with gas to reduce more

Replacing gas used with gas may cause ambiguity as in gas used vs gas limit. 🤔

Copy link
Copy Markdown
Contributor Author

@Wetitpig Wetitpig Apr 30, 2025

Choose a reason for hiding this comment

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

I suggest to replace base with bfee, because base doesn't reflect the base fee and exec with time

The unit s is probably already indicative of time?

Comment thread .../hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java
final StringBuilder message = new StringBuilder();
final int nbTransactions = block.getBody().getTransactions().size();
message.append("Imported #%,d (%s)|%5d tx");
message.append("Imported #%,d (%s)| %4d tx");
Copy link
Copy Markdown
Contributor

@ahamlat ahamlat Apr 30, 2025

Choose a reason for hiding this comment

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

👍

Comment thread .../hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java
}
if (block.getBody().getWithdrawals().isPresent()) {
message.append("|%3d ws");
message.append("| %2d ws");
Copy link
Copy Markdown
Contributor

@ahamlat ahamlat Apr 30, 2025

Choose a reason for hiding this comment

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

👍

Wetitpig added 3 commits April 30, 2025 20:56
@Wetitpig
Standardise log content in AbstractEngineNewPayload and AbstractEngin…
aa3867b 
…eForkchoiceUpdated

Signed-off-by: Wetitpig <winsto003@hotmail.com>
@Wetitpig
Update CHANGELOG.md
b96359b 
Signed-off-by: Wetitpig <winsto003@hotmail.com>
@Wetitpig
Ascending finality
cbdc5d6 
Signed-off-by: Wetitpig <winsto003@hotmail.com>
@Wetitpig
For Fusaka compatibility
d442e6e 
Signed-off-by: Wetitpig <winsto003@hotmail.com>
@Wetitpig Wetitpig requested a review from ahamlat April 30, 2025 12:58
ahamlat
ahamlat approved these changes Apr 30, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahamlat ahamlat left a comment

Choose a reason for hiding this comment

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

🚢

@ahamlat ahamlat enabled auto-merge (squash) April 30, 2025 15:18
@ahamlat ahamlat merged commit 578805d into besu-eth:main Apr 30, 2025
48 checks passed
@Wetitpig Wetitpig deleted the shortLog branch May 1, 2025 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahamlat ahamlat ahamlat approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Wetitpig @ahamlat