Fix RevertTest tests (#10063)

Fixed some failing tests in the `RevertTest` suite. Added an implementation to read the error messages from the returned transaction records. There are 2 types of messages: - Encoded messages such as: "0x08c379a000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000013536f6d6520726576657274206d65737361676500000000000000000000000000" - function selector + reason - error statuses such as: `INVALID_TOKEN_ID` For the second case the `HederaEvmTransactionProcessingResult` class needed to be extended to handle messages that are not in byte format as the additional byte encoding and decoding was not needed. --------- Signed-off-by: Bilyana Gospodinova <[email protected]>
hashgraph · Jan 13, 2025 · 4461e05 · 4461e05
1 parent c84f142
commit 4461e05
Show file tree

Hide file tree

Showing 6 changed files with 160 additions and 31 deletions.
diff --git a/hedera-mirror-web3/src/main/java/com/hedera/mirror/web3/service/ContractCallService.java b/hedera-mirror-web3/src/main/java/com/hedera/mirror/web3/service/ContractCallService.java
@@ -118,7 +118,7 @@ protected HederaEvmTransactionProcessingResult doProcessCall(
             if (!mirrorNodeEvmProperties.isModularizedServices()) {
                 result = mirrorEvmTxProcessor.execute(params, estimatedGas);
             } else {
-                result = transactionExecutionService.execute(params, estimatedGas);
+                result = transactionExecutionService.execute(params, estimatedGas, gasUsedCounter);
             }
             if (!restoreGasToThrottleBucket) {
                 return result;

diff --git a/...mirror-web3/src/main/java/com/hedera/mirror/web3/service/TransactionExecutionService.java b/...mirror-web3/src/main/java/com/hedera/mirror/web3/service/TransactionExecutionService.java
@@ -16,13 +16,14 @@
 
 package com.hedera.mirror.web3.service;
 
+import static com.hedera.mirror.web3.convert.BytesDecoder.maybeDecodeSolidityErrorStringToReadableMessage;
 import static com.hedera.mirror.web3.state.Utils.isMirror;
+import static com.hedera.mirror.web3.validation.HexValidator.HEX_PREFIX;
 
 import com.hedera.hapi.node.base.AccountID;
 import com.hedera.hapi.node.base.ContractID;
 import com.hedera.hapi.node.base.Duration;
 import com.hedera.hapi.node.base.FileID;
-import com.hedera.hapi.node.base.ResponseCodeEnum;
 import com.hedera.hapi.node.base.Timestamp;
 import com.hedera.hapi.node.base.TransactionID;
 import com.hedera.hapi.node.contract.ContractCallTransactionBody;
@@ -36,7 +37,9 @@
 import com.hedera.mirror.web3.common.ContractCallContext;
 import com.hedera.mirror.web3.evm.contracts.execution.traceability.OpcodeTracer;
 import com.hedera.mirror.web3.evm.properties.MirrorNodeEvmProperties;
+import com.hedera.mirror.web3.exception.MirrorEvmTransactionException;
 import com.hedera.mirror.web3.service.model.CallServiceParameters;
+import com.hedera.mirror.web3.service.model.CallServiceParameters.CallType;
 import com.hedera.mirror.web3.state.AliasesReadableKVState;
 import com.hedera.node.app.config.ConfigProviderImpl;
 import com.hedera.node.app.service.evm.contracts.execution.HederaEvmTransactionProcessingResult;
@@ -47,13 +50,16 @@
 import com.hedera.node.config.data.EntitiesConfig;
 import com.swirlds.config.api.Configuration;
 import com.swirlds.state.State;
+import io.micrometer.core.instrument.Counter;
+import io.micrometer.core.instrument.Meter.MeterProvider;
 import jakarta.annotation.Nullable;
 import jakarta.inject.Named;
 import java.time.Instant;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import lombok.CustomLog;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.tuweni.bytes.Bytes;
 import org.hyperledger.besu.datatypes.Address;
 import org.hyperledger.besu.evm.tracing.OperationTracer;
@@ -79,15 +85,16 @@ protected TransactionExecutionService(
         this.opcodeTracer = opcodeTracer;
     }
 
-    public HederaEvmTransactionProcessingResult execute(final CallServiceParameters params, final long estimatedGas) {
+    public HederaEvmTransactionProcessingResult execute(
+            final CallServiceParameters params, final long estimatedGas, final MeterProvider<Counter> gasUsedCounter) {
         final var isContractCreate = params.getReceiver().isZero();
         final var maxLifetime =
                 DEFAULT_CONFIG.getConfigData(EntitiesConfig.class).maxLifetime();
         var executor =
                 ExecutorFactory.newExecutor(mirrorNodeState, mirrorNodeEvmProperties.getTransactionProperties(), null);
 
         TransactionBody transactionBody;
-        HederaEvmTransactionProcessingResult result;
+        HederaEvmTransactionProcessingResult result = null;
         if (isContractCreate) {
             if (params.getCallData().size() < INITCODE_SIZE_KB) {
                 transactionBody = buildContractCreateTransactionBodyWithInitBytecode(params, estimatedGas, maxLifetime);
@@ -118,14 +125,23 @@ public HederaEvmTransactionProcessingResult execute(final CallServiceParameters
 
         var receipt = executor.execute(transactionBody, Instant.now(), getOperationTracers());
         var transactionRecord = receipt.getFirst().transactionRecord();
-        if (transactionRecord.receiptOrThrow().status() == ResponseCodeEnum.SUCCESS) {
+        if (transactionRecord.receiptOrThrow().status() == com.hedera.hapi.node.base.ResponseCodeEnum.SUCCESS) {
             result = buildSuccessResult(isContractCreate, transactionRecord, params);
         } else {
-            result = buildFailedResult(transactionRecord, isContractCreate);
+            handleFailedResult(transactionRecord, isContractCreate, gasUsedCounter);
         }
         return result;
     }
 
+    // Duplicated code as in ContractCallService class - it will be removed from there when switch to the modularized
+    // implementation entirely.
+    private void updateErrorGasUsedMetric(
+            final MeterProvider<Counter> gasUsedCounter, final long gasUsed, final int iterations) {
+        gasUsedCounter
+                .withTags("type", CallType.ERROR.toString(), "iteration", String.valueOf(iterations))
+                .increment(gasUsed);
+    }
+
     private ContractFunctionResult getTransactionResult(
             final TransactionRecord transactionRecord, boolean isContractCreate) {
         return isContractCreate
@@ -148,17 +164,30 @@ private HederaEvmTransactionProcessingResult buildSuccessResult(
                 params.getReceiver());
     }
 
-    private HederaEvmTransactionProcessingResult buildFailedResult(
-            final TransactionRecord transactionRecord, final boolean isContractCreate) {
-        var result = getTransactionResult(transactionRecord, isContractCreate);
-        var status = transactionRecord.receipt().status();
+    private void handleFailedResult(
+            final TransactionRecord transactionRecord,
+            final boolean isContractCreate,
+            final MeterProvider<Counter> gasUsedCounter)
+            throws MirrorEvmTransactionException {
+        var result =
+                isContractCreate ? transactionRecord.contractCreateResult() : transactionRecord.contractCallResult();
+        var status = transactionRecord.receiptOrThrow().status();
+        if (result == null) {
+            // No result - the call did not reach the EVM and probably failed at pre-checks. No metric to update in this
+            // case.
+            throw new MirrorEvmTransactionException(status.protoName(), StringUtils.EMPTY, StringUtils.EMPTY);
+        } else {
+            var errorMessage = getErrorMessage(result).orElse(Bytes.EMPTY);
+            var detail = maybeDecodeSolidityErrorStringToReadableMessage(errorMessage);
+            updateErrorGasUsedMetric(gasUsedCounter, result.gasUsed(), 1);
+            throw new MirrorEvmTransactionException(status.protoName(), detail, errorMessage.toHexString());
+        }
+    }
 
-        return HederaEvmTransactionProcessingResult.failed(
-                result.gasUsed(),
-                0L,
-                0L,
-                Optional.of(Bytes.wrap(status.protoName().getBytes())),
-                Optional.empty());
+    private Optional<Bytes> getErrorMessage(final ContractFunctionResult result) {
+        return result.errorMessage().startsWith(HEX_PREFIX)
+                ? Optional.of(Bytes.fromHexString(result.errorMessage()))
+                : Optional.empty(); // If it doesn't start with 0x, the message is already decoded and readable.
     }
 
     private TransactionBody.Builder defaultTransactionBodyBuilder(final CallServiceParameters params) {

diff --git a/...test/java/com/hedera/mirror/web3/service/AbstractContractCallServiceOpcodeTracerTest.java b/...test/java/com/hedera/mirror/web3/service/AbstractContractCallServiceOpcodeTracerTest.java
@@ -37,6 +37,8 @@
 import com.hedera.mirror.web3.service.model.ContractDebugParameters;
 import com.hedera.mirror.web3.utils.ContractFunctionProviderRecord;
 import com.hedera.node.app.service.evm.contracts.execution.HederaEvmTransactionProcessingResult;
+import io.micrometer.core.instrument.Counter;
+import io.micrometer.core.instrument.Meter.MeterProvider;
 import jakarta.annotation.Resource;
 import java.util.List;
 import java.util.Optional;
@@ -68,6 +70,9 @@ abstract class AbstractContractCallServiceOpcodeTracerTest extends AbstractContr
     private HederaEvmTransactionProcessingResult resultCaptor;
     private ContractCallContext contextCaptor;
 
+    @Captor
+    private ArgumentCaptor<MeterProvider<Counter>> gasUsedCounter;
+
     @Resource
     private EntityDatabaseAccessor entityDatabaseAccessor;
 
@@ -92,7 +97,7 @@ void setUpArgumentCaptors() {
                         return transactionProcessingResult;
                     })
                     .when(transactionExecutionService)
-                    .execute(paramsCaptor.capture(), gasCaptor.capture());
+                    .execute(paramsCaptor.capture(), gasCaptor.capture(), gasUsedCounter.capture());
         }
     }
 

diff --git a/...java/com/hedera/mirror/web3/service/ContractCallServiceERCTokenReadOnlyFunctionsTest.java b/...java/com/hedera/mirror/web3/service/ContractCallServiceERCTokenReadOnlyFunctionsTest.java
@@ -32,9 +32,16 @@
 import com.hedera.mirror.common.domain.token.Token;
 import com.hedera.mirror.common.domain.token.TokenTypeEnum;
 import com.hedera.mirror.web3.exception.MirrorEvmTransactionException;
+import com.hedera.mirror.web3.state.MirrorNodeState;
 import com.hedera.mirror.web3.web3j.generated.ERCTestContract;
 import com.hedera.mirror.web3.web3j.generated.RedirectTestContract;
+import jakarta.annotation.PostConstruct;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.math.BigInteger;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
 import lombok.RequiredArgsConstructor;
 import org.hyperledger.besu.datatypes.Address;
 import org.junit.jupiter.api.Test;
@@ -965,6 +972,40 @@ void decimalsNegative() {
         assertThatThrownBy(functionCall::send).isInstanceOf(MirrorEvmTransactionException.class);
     }
 
+    // Temporary test to increase test coverage
+    @Test
+    void decimalsNegativeModularizedServices() throws InvocationTargetException, IllegalAccessException {
+        // Given
+        final var modularizedServicesFlag = mirrorNodeEvmProperties.isModularizedServices();
+        mirrorNodeEvmProperties.setModularizedServices(true);
+
+        final var backupProperties = mirrorNodeEvmProperties.getProperties();
+        final Map<String, String> propertiesMap = new HashMap<>();
+        propertiesMap.put("contracts.maxRefundPercentOfGasLimit", "100");
+        propertiesMap.put("contracts.maxGasPerSec", "15000000");
+        mirrorNodeEvmProperties.setProperties(propertiesMap);
+
+        Method postConstructMethod = Arrays.stream(MirrorNodeState.class.getDeclaredMethods())
+                .filter(method -> method.isAnnotationPresent(PostConstruct.class))
+                .findFirst()
+                .orElseThrow(() -> new IllegalStateException("@PostConstruct method not found"));
+
+        postConstructMethod.setAccessible(true); // Make the method accessible
+        postConstructMethod.invoke(state);
+
+        final var tokenEntity = nftPersist();
+        final var tokenAddress = toAddress(tokenEntity.getTokenId());
+        final var contract = testWeb3jService.deploy(ERCTestContract::deploy);
+        // When
+        final var functionCall = contract.send_decimals(tokenAddress.toHexString());
+        // Then
+        assertThatThrownBy(functionCall::send).isInstanceOf(MirrorEvmTransactionException.class);
+
+        // Restore changed property values.
+        mirrorNodeEvmProperties.setModularizedServices(modularizedServicesFlag);
+        mirrorNodeEvmProperties.setProperties(backupProperties);
+    }
+
     @Test
     void ownerOfNegative() {
         // Given

diff --git a/hedera-mirror-web3/src/test/java/com/hedera/mirror/web3/service/RevertTest.java b/hedera-mirror-web3/src/test/java/com/hedera/mirror/web3/service/RevertTest.java
@@ -34,6 +34,7 @@ public class RevertTest extends AbstractContractCallServiceTest {
     void testRevertPayable() {
         final var contract = testWeb3jService.deploy(Reverter::deploy);
         meterRegistry.clear();
+        testWeb3jService.setSender(TREASURY_ADDRESS);
 
         final var functionCall = contract.send_revertPayable(BigInteger.ONE);
         assertThatThrownBy(functionCall::send)

diff --git a/...or-web3/src/test/java/com/hedera/mirror/web3/service/TransactionExecutionServiceTest.java b/...or-web3/src/test/java/com/hedera/mirror/web3/service/TransactionExecutionServiceTest.java
@@ -18,7 +18,9 @@
 
 import static com.hedera.mirror.web3.state.Utils.isMirror;
 import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.mockStatic;
 import static org.mockito.Mockito.when;
@@ -32,6 +34,7 @@
 import com.hedera.mirror.web3.evm.contracts.execution.traceability.OpcodeTracer;
 import com.hedera.mirror.web3.evm.contracts.execution.traceability.OpcodeTracerOptions;
 import com.hedera.mirror.web3.evm.properties.MirrorNodeEvmProperties;
+import com.hedera.mirror.web3.exception.MirrorEvmTransactionException;
 import com.hedera.mirror.web3.service.TransactionExecutionService.ExecutorFactory;
 import com.hedera.mirror.web3.service.model.CallServiceParameters;
 import com.hedera.mirror.web3.service.model.CallServiceParameters.CallType;
@@ -46,6 +49,8 @@
 import com.swirlds.state.State;
 import com.swirlds.state.spi.ReadableKVState;
 import com.swirlds.state.spi.ReadableStates;
+import io.micrometer.core.instrument.Counter;
+import io.micrometer.core.instrument.Meter.MeterProvider;
 import java.time.Instant;
 import java.util.List;
 import java.util.stream.Stream;
@@ -56,6 +61,7 @@
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.junit.jupiter.params.provider.MethodSource;
 import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.Mock;
@@ -81,6 +87,9 @@ class TransactionExecutionServiceTest {
     @Mock
     private ContractCallContext contractCallContext;
 
+    @Mock
+    private MeterProvider<Counter> gasUsedCounter;
+
     private TransactionExecutionService transactionExecutionService;
 
     private static Stream<Arguments> provideCallData() {
@@ -151,7 +160,7 @@ void testExecuteContractCallSuccess(String senderAddressHex) {
                     buildServiceParams(false, org.apache.tuweni.bytes.Bytes.EMPTY, senderAddress);
 
             // When
-            var result = transactionExecutionService.execute(callServiceParameters, DEFAULT_GAS);
+            var result = transactionExecutionService.execute(callServiceParameters, DEFAULT_GAS, gasUsedCounter);
 
             // Then
             assertThat(result).isNotNull();
@@ -160,8 +169,14 @@ void testExecuteContractCallSuccess(String senderAddressHex) {
         }
     }
 
-    @Test
-    void testExecuteContractCallFailure() {
+    @ParameterizedTest
+    @CsvSource({
+        "0x08c379a000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000013536f6d6520726576657274206d65737361676500000000000000000000000000,CONTRACT_REVERT_EXECUTED,Some revert message",
+        "INVALID_TOKEN_ID,CONTRACT_REVERT_EXECUTED,''",
+        "0x,INVALID_TOKEN_ID,''"
+    })
+    void testExecuteContractCallFailureWithErrorMessage(
+            final String errorMessage, final ResponseCodeEnum responseCode, final String detail) {
         // Given
         try (MockedStatic<ExecutorFactory> executorFactoryMock = mockStatic(ExecutorFactory.class);
                 MockedStatic<ContractCallContext> contractCallContextMock = mockStatic(ContractCallContext.class)) {
@@ -179,15 +194,15 @@ void testExecuteContractCallFailure() {
             TransactionRecord transactionRecord = mock(TransactionRecord.class);
             TransactionReceipt transactionReceipt = mock(TransactionReceipt.class);
 
-            // Simulate CONTRACT_REVERT_EXECUTED status in the receipt
-            when(transactionReceipt.status()).thenReturn(ResponseCodeEnum.CONTRACT_REVERT_EXECUTED);
+            when(transactionReceipt.status()).thenReturn(responseCode);
             when(transactionRecord.receiptOrThrow()).thenReturn(transactionReceipt);
-            when(transactionRecord.receipt()).thenReturn(transactionReceipt);
             when(singleTransactionRecord.transactionRecord()).thenReturn(transactionRecord);
 
             ContractFunctionResult contractFunctionResult = mock(ContractFunctionResult.class);
-            when(transactionRecord.contractCallResultOrThrow()).thenReturn(contractFunctionResult);
-            when(contractFunctionResult.gasUsed()).thenReturn(DEFAULT_GAS);
+            when(transactionRecord.contractCallResult()).thenReturn(contractFunctionResult);
+            when(contractFunctionResult.errorMessage()).thenReturn(errorMessage);
+            when(gasUsedCounter.withTags(anyString(), anyString(), anyString(), anyString()))
+                    .thenReturn(mock(Counter.class));
 
             // Mock the executor to return a List with the mocked SingleTransactionRecord
             when(transactionExecutor.execute(
@@ -197,13 +212,51 @@ void testExecuteContractCallFailure() {
             CallServiceParameters callServiceParameters =
                     buildServiceParams(false, org.apache.tuweni.bytes.Bytes.EMPTY, Address.ZERO);
 
-            // When
-            var result = transactionExecutionService.execute(callServiceParameters, DEFAULT_GAS);
+            // Then
+            assertThatThrownBy(() ->
+                            transactionExecutionService.execute(callServiceParameters, DEFAULT_GAS, gasUsedCounter))
+                    .isInstanceOf(MirrorEvmTransactionException.class)
+                    .hasMessageContaining(responseCode.name())
+                    .hasFieldOrPropertyWithValue("detail", detail);
+        }
+    }
+
+    @Test
+    void testExecuteContractCallFailureOnPreChecks() {
+        // Given
+        try (MockedStatic<ExecutorFactory> executorFactoryMock = mockStatic(ExecutorFactory.class);
+                MockedStatic<ContractCallContext> contractCallContextMock = mockStatic(ContractCallContext.class)) {
+
+            // Set up mock behaviors for ExecutorFactory
+            executorFactoryMock
+                    .when(() -> ExecutorFactory.newExecutor(any(), any(), any()))
+                    .thenReturn(transactionExecutor);
+
+            // Set up mock behaviors for ContractCallContext
+            contractCallContextMock.when(ContractCallContext::get).thenReturn(contractCallContext);
+
+            // Mock the SingleTransactionRecord and TransactionRecord
+            SingleTransactionRecord singleTransactionRecord = mock(SingleTransactionRecord.class);
+            TransactionRecord transactionRecord = mock(TransactionRecord.class);
+            TransactionReceipt transactionReceipt = mock(TransactionReceipt.class);
+
+            when(transactionRecord.receiptOrThrow()).thenReturn(transactionReceipt);
+            when(transactionReceipt.status()).thenReturn(ResponseCodeEnum.INVALID_ACCOUNT_ID);
+            when(singleTransactionRecord.transactionRecord()).thenReturn(transactionRecord);
+
+            // Mock the executor to return a List with the mocked SingleTransactionRecord
+            when(transactionExecutor.execute(
+                            any(TransactionBody.class), any(Instant.class), any(OperationTracer[].class)))
+                    .thenReturn(List.of(singleTransactionRecord));
+
+            CallServiceParameters callServiceParameters =
+                    buildServiceParams(false, org.apache.tuweni.bytes.Bytes.EMPTY, Address.ZERO);
 
             // Then
-            assertThat(result).isNotNull();
-            assertThat(result.getGasUsed()).isEqualTo(DEFAULT_GAS);
-            assertThat(result.getRevertReason()).isPresent();
+            assertThatThrownBy(() ->
+                            transactionExecutionService.execute(callServiceParameters, DEFAULT_GAS, gasUsedCounter))
+                    .isInstanceOf(MirrorEvmTransactionException.class)
+                    .hasMessageContaining(ResponseCodeEnum.INVALID_ACCOUNT_ID.name());
         }
     }
 
@@ -248,7 +301,7 @@ void testExecuteContractCreateSuccess(org.apache.tuweni.bytes.Bytes callData) {
             CallServiceParameters callServiceParameters = buildServiceParams(true, callData, Address.ZERO);
 
             // When
-            var result = transactionExecutionService.execute(callServiceParameters, DEFAULT_GAS);
+            var result = transactionExecutionService.execute(callServiceParameters, DEFAULT_GAS, gasUsedCounter);
 
             // Then
             assertThat(result).isNotNull();