BesuCommand.isRevertReasonEnabled belongs in EVMConfiguration

[jflo]

While doing some refactoring I had to provide isRevertReasonEnabled from BesuCommand through the BesuController, and it struck me that this is more obviously an EVM configuration option. This should be refactored and folded into EVMConfiguration.

[shemnon]

Isn't this a data storage question? Revert Reason is in the output of the root message frame, wether or not it is recorded. No code in the EVM module uses it, the flag is part of TransactionReceiptFactory, switching between these two functions -

besu/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetProtocolSpecs.java

Lines 940 to 958 in 8eee569

	private static TransactionReceipt byzantiumTransactionReceiptFactory(
	// ignored because it's always FRONTIER
	final TransactionType __,
	final TransactionProcessingResult result,
	final WorldState worldState,
	final long gasUsed) {
	return new TransactionReceipt(
	result.isSuccessful() ? 1 : 0, gasUsed, result.getLogs(), Optional.empty());
	}
	private static TransactionReceipt byzantiumTransactionReceiptFactoryWithReasonEnabled(
	// ignored because it's always FRONTIER
	final TransactionType __,
	final TransactionProcessingResult result,
	final WorldState worldState,
	final long gasUsed) {
	return new TransactionReceipt(
	result.isSuccessful() ? 1 : 0, gasUsed, result.getLogs(), result.getRevertReason());
	}

.

When writing it to trie you already have the flag

besu/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/TransactionReceipt.java

Lines 191 to 192 in 15d54af

	public void writeToForReceiptTrie(
	final RLPOutput rlpOutput, final boolean withRevertReason, final boolean compacted) {

, so we could simply drop it from the protocol spec factory instead.

[siladu]

@jflo is this blocking anything else? Should we prioritise on a team's board or happy to leave for anyone to pick up?

[jflo]

Anyone can pick this up, and should consider @shemnon 's suggestions above. I'm not married to any particular approach, as long as this option ends up being grouped in somewhere more specific and relevant.

[bomanaps]

Hello @jflo & @shemnon ,

I've reviewed this issue and would like to confirm my understanding of the steps required to address it:

1. Move isRevertReasonEnabled to EVMConfiguration:

   • Add a new field isRevertReasonEnabled in the EVMConfiguration class
   • Implement getter and setter methods for this field

2. Update BesuCommand and BesuController:

   • Remove direct handling of isRevertReasonEnabled in BesuCommand
   • Modify BesuController to initialize and manage this flag using EVMConfiguration

3. Modify TransactionReceiptFactory:

   • Update methods to use EVMConfiguration for checking the isRevertReasonEnabled flag
   • Combine the two factory methods into one, where the revert reason is included or excluded based on EVMConfiguration

4. Update MainnetProtocolSpecs:

   • Change logic to pass EVMConfiguration to the TransactionReceiptFactory when creating transaction receipts

If these steps accurately reflect the requirements, I would like to request assignment to this issue. Thank you for your consideration.

[shemnon]

@bomanaps Do not, under any condition, add it to EVMConfiguration. Do some more studying of where and how the flag is used. EVMConfiguration should only be for options used by the :evm module, and it isn't used there. In fact, you will discover that the flag is only ever used in the ProtocolSpec

Adding it to DataStorageConfiguration would be a better place. Then MainnetProtocolSpecs and ClassicProtocolSpecs can replace the two fields enableRevertReason and isParallelTxProcessingEnabled parameters being passed around with the single DataStorageConfiguration Object.

We could move chainId into EVMConfiguration, that refactoring would make sense. Probably in a different PR as we would also want to update ChainIdOperation as well as any other place ChainId and EVMConfiguration are passed along together. But that seems like a separate PR

[bomanaps]

Sorry for the misunderstanding. I will move isRevertReasonEnabled to DataStorageConfiguration, include this flag (along with isParallelTxProcessingEnabled), and remove it from BesuCommand and BesuController. I'll also update MainnetProtocolSpecs and ClassicProtocolSpecs accordingly.