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

Improve newPayload and FCU logs #7961

Merged
merged 34 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
9e558e5
Change the output log on newPayload execution
ahamlat Nov 27, 2024
ca3962d
Update the log format by adding padding to have a better ux
ahamlat Nov 27, 2024
b8a84da
Add the number of parallelized executed transactions in the log (if any)
ahamlat Nov 28, 2024
2aaf354
Initialize isProcessedInParallel in isProcessedInParallel
ahamlat Nov 28, 2024
3fff1c5
Initialize isProcessedInParallel in isProcessedInParallel
ahamlat Nov 28, 2024
454c59d
Initialize isProcessedInParallel in isProcessedInParallel
ahamlat Nov 28, 2024
0d82b32
Fix error: Conversion = 'P'
ahamlat Nov 28, 2024
eb0321b
Fix error: f != java.lang.Integer
ahamlat Nov 28, 2024
1755514
shorten the block hash in the output
ahamlat Nov 28, 2024
8ff711c
add final
ahamlat Nov 28, 2024
88353ac
FCU log message + spotless
ahamlat Nov 28, 2024
ad6eb05
Change the format of the base fee
ahamlat Nov 29, 2024
04d6521
Change the format of the base fee
ahamlat Nov 29, 2024
bf26d50
refactor + fix unit tests
ahamlat Nov 29, 2024
5d3786d
Migrate to Prometheus lib 1.x (#7880)
fab-10 Nov 27, 2024
1dbc590
Add histogram to metrics system (#7944)
fab-10 Nov 27, 2024
812bafe
Shutdown MetricsSystem when stopping MetricsService (#7958)
fab-10 Nov 28, 2024
ab86b6c
[MINOR] (privacy deprecation) correct target to 24.12.0 (#7953)
macfarla Nov 29, 2024
a18e6d5
PoW mining - deprecation (#7905)
macfarla Nov 29, 2024
6be3e28
Remove P2P TLS (experimental) feature (#7942)
macfarla Nov 29, 2024
05d5cde
fix TransactionLocation in DefaultBlockchain unsafeImportBlock() (#7…
pinges Nov 29, 2024
6b9672b
Fast sync - deprecation warning (#7906)
macfarla Nov 29, 2024
297893d
TransactionSimulator can be a singleton (#7925)
fab-10 Nov 29, 2024
20f714e
Update ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/j…
ahamlat Dec 2, 2024
01cf738
Add a space
ahamlat Dec 2, 2024
68a056c
Merge branch 'main' into change-log-newpayload
ahamlat Dec 2, 2024
a7534e7
Make shortenHexString part of Hash class.
ahamlat Dec 3, 2024
50e8f0b
typo
ahamlat Dec 3, 2024
7415e38
Merge branch 'main' into change-log-newpayload
ahamlat Dec 3, 2024
e1d2395
Change shortenHexString to respect the same signature as existing sim…
ahamlat Dec 4, 2024
4f58822
Merge branch 'main' into change-log-newpayload
ahamlat Dec 4, 2024
0c903c7
Add changelog.
ahamlat Dec 4, 2024
07ce1a6
Merge branch 'main' into change-log-newpayload
ahamlat Dec 5, 2024
69d4564
Merge branch 'main' into change-log-newpayload
ahamlat Dec 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion datatypes/src/main/java/org/hyperledger/besu/datatypes/Wei.java
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,31 @@ public static Wei fromQuantity(final Quantity quantity) {
* @return the string
*/
public String toHumanReadableString() {
return toHumanReadableStringWithPadding(1);
}

/**
* Wei to human-readable string, with padding
*
* @return the string
*/
public String toHumanReadablePaddedString() {
return toHumanReadableStringWithPadding(6);
}

/**
* Returns a human-readable String, the number of returned characters depends on the width
* parameter
*
* @param width the number of digits to use
* @return a human-readable String
*/
private String toHumanReadableStringWithPadding(final int width) {
final BigInteger amount = toBigInteger();
final int numOfDigits = amount.toString().length();
final Unit preferredUnit = Unit.getPreferred(numOfDigits);
final double res = amount.doubleValue() / preferredUnit.divisor;
return String.format("%1." + preferredUnit.decimals + "f %s", res, preferredUnit);
return String.format("%" + width + "." + preferredUnit.decimals + "f %s", res, preferredUnit);
}

/** The enum Unit. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,28 @@ public void toHumanReadableString() {
assertThat(Wei.of(new BigInteger("1" + String.valueOf(manyZeros))).toHumanReadableString())
.isEqualTo("100.00 tether");
}

@Test
public void toHumanReadablePaddedString() {
assertThat(Wei.ZERO.toHumanReadablePaddedString()).isEqualTo(" 0 wei");
assertThat(Wei.ONE.toHumanReadablePaddedString()).isEqualTo(" 1 wei");

assertThat(Wei.of(999).toHumanReadablePaddedString()).isEqualTo(" 999 wei");
assertThat(Wei.of(1000).toHumanReadablePaddedString()).isEqualTo(" 1.00 kwei");

assertThat(Wei.of(1009).toHumanReadablePaddedString()).isEqualTo(" 1.01 kwei");
assertThat(Wei.of(1011).toHumanReadablePaddedString()).isEqualTo(" 1.01 kwei");

assertThat(Wei.of(new BigInteger("1000000000")).toHumanReadablePaddedString())
.isEqualTo(" 1.00 gwei");

assertThat(Wei.of(new BigInteger("1000000000000000000")).toHumanReadablePaddedString())
.isEqualTo(" 1.00 ether");

final char[] manyZeros = new char[32];
Arrays.fill(manyZeros, '0');
assertThat(
Wei.of(new BigInteger("1" + String.valueOf(manyZeros))).toHumanReadablePaddedString())
.isEqualTo("100.00 tether");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -389,8 +389,7 @@ protected RpcErrorType getInvalidPayloadAttributesError() {

// fcU calls are synchronous, no need to make volatile
private long lastFcuInfoLog = System.currentTimeMillis();
private static final String logMessage =
"{} for fork-choice-update: head: {}, finalized: {}, safeBlockHash: {}";
private static final String logMessage = "FCU({}) | head: {} | finalized: {} | safeBlockHash: {}";

private void logForkchoiceUpdatedCall(
final EngineStatus status, final EngineForkchoiceUpdatedParameter forkChoice) {
Expand All @@ -413,9 +412,9 @@ private void logAtInfo(
LOG.info(
logMessage,
status.name(),
forkChoice.getHeadBlockHash(),
forkChoice.getFinalizedBlockHash(),
forkChoice.getSafeBlockHash());
AbstractEngineNewPayload.shortenString(forkChoice.getHeadBlockHash().toHexString()),
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment about making etherscan debugging more difficult, less important for FCU though I think since can cross ref with Imported log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same as for newPayload call.

AbstractEngineNewPayload.shortenString(forkChoice.getFinalizedBlockHash().toHexString()),
AbstractEngineNewPayload.shortenString(forkChoice.getSafeBlockHash().toHexString()));
}

private void logAtDebug(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,8 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
.flatMap(Optional::stream)
.mapToInt(List::size)
.sum(),
(System.currentTimeMillis() - startTimeMs) / 1000.0);
(System.currentTimeMillis() - startTimeMs) / 1000.0,
executionResult.getNbParallelizedTransations());
return respondWith(reqId, blockParam, newBlockHeader.getHash(), VALID);
} else {
if (executionResult.causedBy().isPresent()) {
Expand Down Expand Up @@ -564,26 +565,52 @@ private Optional<List<Request>> extractRequests(final Optional<List<String>> may
.collect(Collectors.toList()));
}

private void logImportedBlockInfo(final Block block, final int blobCount, final double timeInS) {
private void logImportedBlockInfo(
final Block block,
final int blobCount,
final double timeInS,
final Optional<Integer> nbParallelizedTransations) {
final StringBuilder message = new StringBuilder();
message.append("Imported #%,d / %d tx");
final int nbTransactions = block.getBody().getTransactions().size();
message.append("Imported #%,d (%s)|%5d tx");
final List<Object> messageArgs =
new ArrayList<>(
List.of(block.getHeader().getNumber(), block.getBody().getTransactions().size()));
List.of(
block.getHeader().getNumber(),
shortenString(block.getHash().toHexString()),
ahamlat marked this conversation as resolved.
Show resolved Hide resolved
nbTransactions));
if (block.getBody().getWithdrawals().isPresent()) {
message.append(" / %d ws");
message.append("|%3d ws");
ahamlat marked this conversation as resolved.
Show resolved Hide resolved
messageArgs.add(block.getBody().getWithdrawals().get().size());
}
message.append(" / %d blobs / base fee %s / %,d (%01.1f%%) gas / (%s) in %01.3fs. Peers: %d");
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");
messageArgs.addAll(
List.of(
blobCount,
block.getHeader().getBaseFee().map(Wei::toHumanReadableString).orElse("N/A"),
block.getHeader().getBaseFee().map(Wei::toHumanReadablePaddedString).orElse("N/A"),
block.getHeader().getGasUsed(),
(block.getHeader().getGasUsed() * 100.0) / block.getHeader().getGasLimit(),
block.getHash().toHexString(),
timeInS,
ethPeers.peerCount()));
mgasPerSec));
if (nbParallelizedTransations.isPresent()) {
double parallelizedTxPercentage =
(double) (nbParallelizedTransations.get() * 100) / nbTransactions;
message.append("| parallel txs %5.1f%%");
messageArgs.add(parallelizedTxPercentage);
}
message.append("| peers: %2d");
messageArgs.add(ethPeers.peerCount());
LOG.info(String.format(message.toString(), messageArgs.toArray()));
}

public static String shortenString(final String input) {
if (input == null || input.length() <= 10) {
throw new IllegalArgumentException("Input string must be longer than 10 characters");
}
String firstPart = input.substring(0, 6);
String lastPart = input.substring(input.length() - 5);
return firstPart + "....." + lastPart;
ahamlat marked this conversation as resolved.
Show resolved Hide resolved
ahamlat marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public class BlockProcessingResult extends BlockValidationResult {

private final Optional<BlockProcessingOutputs> yield;
private final boolean isPartial;
private Optional<Integer> nbParallelizedTransations = Optional.empty();

/** A result indicating that processing failed. */
public static final BlockProcessingResult FAILED = new BlockProcessingResult("processing failed");
Expand All @@ -40,6 +41,21 @@ public BlockProcessingResult(final Optional<BlockProcessingOutputs> yield) {
this.isPartial = false;
}

/**
* A result indicating that processing was successful but incomplete.
*
* @param yield the outputs of processing a block
* @param nbParallelizedTransations potential number of parallelized transactions during block
* processing
*/
public BlockProcessingResult(
final Optional<BlockProcessingOutputs> yield,
final Optional<Integer> nbParallelizedTransations) {
this.yield = yield;
this.isPartial = false;
this.nbParallelizedTransations = nbParallelizedTransations;
}

/**
* A result indicating that processing was successful but incomplete.
*
Expand Down Expand Up @@ -144,4 +160,13 @@ public List<TransactionReceipt> getReceipts() {
public Optional<List<Request>> getRequests() {
return yield.flatMap(BlockProcessingOutputs::getRequests);
}

/**
* Returns an optional that contains the number of parallelized transactions (if there is any)
*
* @return Optional of parallelized transactions during the block execution
*/
public Optional<Integer> getNbParallelizedTransations() {
return nbParallelizedTransations;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ public BlockProcessingResult validateAndProcessBlock(
}

return new BlockProcessingResult(
Optional.of(new BlockProcessingOutputs(worldState, receipts, maybeRequests)));
Optional.of(new BlockProcessingOutputs(worldState, receipts, maybeRequests)),
result.getNbParallelizedTransations());
}
} catch (MerkleTrieException ex) {
context.getWorldStateArchive().heal(ex.getMaybeAddress(), ex.getLocation());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ public BlockProcessingResult processBlock(
blockHashLookup,
blobGasPrice);

boolean parallelizedTxFound = false;
int nbParallelTx = 0;
for (int i = 0; i < transactions.size(); i++) {
final Transaction transaction = transactions.get(i);
if (!hasAvailableBlockBudget(blockHeader, transaction, currentGasUsed)) {
Expand Down Expand Up @@ -181,6 +183,13 @@ public BlockProcessingResult processBlock(
transactionReceiptFactory.create(
transaction.getType(), transactionProcessingResult, worldState, currentGasUsed);
receipts.add(transactionReceipt);
if (!parallelizedTxFound
&& transactionProcessingResult.getIsProcessedInParallel().isPresent()) {
parallelizedTxFound = true;
nbParallelTx = 1;
} else if (transactionProcessingResult.getIsProcessedInParallel().isPresent()) {
nbParallelTx++;
}
}
if (blockHeader.getBlobGasUsed().isPresent()
&& currentBlobGasUsed != blockHeader.getBlobGasUsed().get()) {
Expand Down Expand Up @@ -243,7 +252,8 @@ public BlockProcessingResult processBlock(
}

return new BlockProcessingResult(
Optional.of(new BlockProcessingOutputs(worldState, receipts, maybeRequests)));
Optional.of(new BlockProcessingOutputs(worldState, receipts, maybeRequests)),
parallelizedTxFound ? Optional.of(nbParallelTx) : Optional.empty());
}

protected Optional<PreprocessingContext> runBlockPreProcessing(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,10 @@ public Optional<TransactionProcessingResult> applyParallelizedTransactionResult(

blockAccumulator.importStateChangesFromSource(transactionAccumulator);

if (confirmedParallelizedTransactionCounter.isPresent())
if (confirmedParallelizedTransactionCounter.isPresent()) {
confirmedParallelizedTransactionCounter.get().inc();
transactionProcessingResult.setIsProcessedInParallel(Optional.of(Boolean.TRUE));
}
return Optional.of(transactionProcessingResult);
} else {
blockAccumulator.importPriorStateFromSource(transactionAccumulator);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ public enum Status {

private final Bytes output;

private Optional<Boolean> isProcessedInParallel = Optional.empty();

private final ValidationResult<TransactionInvalidReason> validationResult;
private final Optional<Bytes> revertReason;

Expand Down Expand Up @@ -194,6 +196,25 @@ public ValidationResult<TransactionInvalidReason> getValidationResult() {
return validationResult;
}

/**
* Set isProcessedInParallel to the value in parameter
*
* @param isProcessedInParallel new value of isProcessedInParallel
*/
public void setIsProcessedInParallel(final Optional<Boolean> isProcessedInParallel) {
this.isProcessedInParallel = isProcessedInParallel;
}
Comment on lines +204 to +206
Copy link
Contributor

Choose a reason for hiding this comment

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

can we avoid to make this class mutable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that currently, when a transaction executes, it doesn't know itself if it si being executed in parallel or not, so the instance of TransactionProcessingResult is not aware if it was parallel or not. This information is only accessible later on. that's why we need to make the result mutable at this level only.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok I see, fine for me, but maybe this is a sign that some refactor is needed, to postponed the creation of the result, of the data should be kept elsewhere


/**
* Returns a flag that indicates if the transaction was executed in parallel
*
* @return Optional of Boolean, the value of the boolean is true if the transaction was executed
* in parallel
*/
public Optional<Boolean> getIsProcessedInParallel() {
return isProcessedInParallel;
}

/**
* Returns the reason why a transaction was reverted (if applicable).
*
Expand Down