From fb0aa98a4def9346afde3a636449d927a4360ffe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Cordier?= Date: Tue, 9 May 2023 12:18:46 +0200 Subject: [PATCH 1/6] Add check for ContributeAndFinalize in ReplicatesService --- CHANGELOG.md | 1 + .../com/iexec/core/chain/IexecHubService.java | 7 ++ .../core/replicate/ReplicatesService.java | 3 + .../core/replicate/ReplicateServiceTests.java | 79 +++++++++++++++++++ 4 files changed, 90 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36875ad6..c2b533b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to this project will be documented in this file. ### New Features - Add ContributeAndFinalize to `ReplicateWorkflow`. (#574) +- Add check for ContributeAndFinalize in `ReplicatesService`. ### Bug Fixes - Prevent race condition on replicate update. (#568) ### Quality diff --git a/src/main/java/com/iexec/core/chain/IexecHubService.java b/src/main/java/com/iexec/core/chain/IexecHubService.java index d9978bb5..cf63c0a3 100644 --- a/src/main/java/com/iexec/core/chain/IexecHubService.java +++ b/src/main/java/com/iexec/core/chain/IexecHubService.java @@ -33,6 +33,7 @@ import java.util.concurrent.ThreadPoolExecutor; import static com.iexec.common.utils.DateTimeUtils.now; +import static com.iexec.commons.poco.chain.ChainContributionStatus.CONTRIBUTED; import static com.iexec.commons.poco.utils.BytesUtils.stringToBytes; @Slf4j @@ -74,6 +75,12 @@ public boolean isTaskInUnsetStatusOnChain(String chainDealId, int taskIndex) { || ChainTaskStatus.UNSET.equals(chainTask.get().getStatus()); } + + public boolean isTaskInCompletedStatusOnChain(String chainTaskId) { + Optional oTask = getChainTask(chainTaskId); + return oTask.filter(chainTask -> ChainTaskStatus.COMPLETED.equals(chainTask.getStatus())).isPresent(); + } + /** * Check if a deal's contribution deadline * is still not reached. diff --git a/src/main/java/com/iexec/core/replicate/ReplicatesService.java b/src/main/java/com/iexec/core/replicate/ReplicatesService.java index bd4e6499..4a181f22 100644 --- a/src/main/java/com/iexec/core/replicate/ReplicatesService.java +++ b/src/main/java/com/iexec/core/replicate/ReplicatesService.java @@ -203,6 +203,7 @@ public ReplicateStatusUpdateError canUpdateReplicateStatus(String chainTaskId, case REVEAL_FAILED: canUpdate = false; break; + case CONTRIBUTE_AND_FINALIZE_DONE: case RESULT_UPLOAD_FAILED: canUpdate = verifyStatus(chainTaskId, walletAddress, newStatus, updateReplicateStatusArgs); break; @@ -537,6 +538,8 @@ private boolean verifyStatus(String chainTaskId, return isResultUploaded(updateReplicateStatusArgs.getTaskDescription()); case RESULT_UPLOAD_FAILED: return !isResultUploaded(updateReplicateStatusArgs.getTaskDescription()); + case CONTRIBUTE_AND_FINALIZE_DONE: + return iexecHubService.repeatIsRevealedTrue(chainTaskId,walletAddress) && iexecHubService.isTaskInCompletedStatusOnChain(chainTaskId); default: return true; } diff --git a/src/test/java/com/iexec/core/replicate/ReplicateServiceTests.java b/src/test/java/com/iexec/core/replicate/ReplicateServiceTests.java index 282a970c..1cd769d5 100644 --- a/src/test/java/com/iexec/core/replicate/ReplicateServiceTests.java +++ b/src/test/java/com/iexec/core/replicate/ReplicateServiceTests.java @@ -19,6 +19,8 @@ import com.iexec.common.replicate.*; import com.iexec.commons.poco.chain.ChainContribution; import com.iexec.commons.poco.chain.ChainContributionStatus; +import com.iexec.commons.poco.chain.ChainTask; +import com.iexec.commons.poco.chain.ChainTaskStatus; import com.iexec.commons.poco.notification.TaskNotificationType; import com.iexec.commons.poco.task.TaskDescription; import com.iexec.commons.poco.utils.BytesUtils; @@ -1255,6 +1257,83 @@ void shouldNotAuthorizeUpdateOnResultUploadedSinceResultNotUploaded() { .isEqualTo(ReplicateStatusUpdateError.GENERIC_CANT_UPDATE); } + @Test + void shouldAuthorizeUpdateOnContributeAndFinalizeOnGoing() { + final Replicate replicate = new Replicate(WALLET_WORKER_1, CHAIN_TASK_ID); + replicate.updateStatus(COMPUTED, ReplicateStatusModifier.WORKER); + + final ReplicatesList replicatesList = new ReplicatesList(CHAIN_TASK_ID, Collections.singletonList(replicate)); + final ReplicateStatusUpdate statusUpdate = ReplicateStatusUpdate.builder() + .modifier(WORKER) + .status(CONTRIBUTE_AND_FINALIZE_ONGOING) + .build(); + + when(replicatesRepository.findByChainTaskId(CHAIN_TASK_ID)).thenReturn(Optional.of(replicatesList)); + + assertThat(replicatesService.canUpdateReplicateStatus(CHAIN_TASK_ID, WALLET_WORKER_1, statusUpdate, null)) + .isEqualTo(ReplicateStatusUpdateError.NO_ERROR); + } + + @Test + void shouldAuthorizeUpdateOnContributeAndFinalizeDone() { + final Replicate replicate = new Replicate(WALLET_WORKER_1, CHAIN_TASK_ID); + replicate.updateStatus(CONTRIBUTE_AND_FINALIZE_ONGOING, ReplicateStatusModifier.WORKER); + + final ReplicatesList replicatesList = new ReplicatesList(CHAIN_TASK_ID, Collections.singletonList(replicate)); + final ReplicateStatusUpdate statusUpdate = ReplicateStatusUpdate.builder() + .modifier(WORKER) + .status(CONTRIBUTE_AND_FINALIZE_DONE) + .build(); + + when(replicatesRepository.findByChainTaskId(CHAIN_TASK_ID)).thenReturn(Optional.of(replicatesList)); + when(iexecHubService.repeatIsRevealedTrue(CHAIN_TASK_ID, WALLET_WORKER_1)) + .thenReturn(true); + when(iexecHubService.isTaskInCompletedStatusOnChain(CHAIN_TASK_ID)).thenReturn(true); + + assertThat(replicatesService.canUpdateReplicateStatus(CHAIN_TASK_ID, WALLET_WORKER_1, statusUpdate, null)) + .isEqualTo(ReplicateStatusUpdateError.NO_ERROR); + } + + @Test + void shouldNotAuthorizeUpdateOnContributeAndFinalizeDoneWhenNotRevealed() { + final Replicate replicate = new Replicate(WALLET_WORKER_1, CHAIN_TASK_ID); + replicate.updateStatus(CONTRIBUTE_AND_FINALIZE_ONGOING, ReplicateStatusModifier.WORKER); + + final ReplicatesList replicatesList = new ReplicatesList(CHAIN_TASK_ID, Collections.singletonList(replicate)); + final ReplicateStatusUpdate statusUpdate = ReplicateStatusUpdate.builder() + .modifier(WORKER) + .status(CONTRIBUTE_AND_FINALIZE_DONE) + .build(); + + when(replicatesRepository.findByChainTaskId(CHAIN_TASK_ID)).thenReturn(Optional.of(replicatesList)); + when(iexecHubService.repeatIsRevealedTrue(CHAIN_TASK_ID, WALLET_WORKER_1)) + .thenReturn(false); + when(iexecHubService.isTaskInCompletedStatusOnChain(CHAIN_TASK_ID)).thenReturn(true); + + assertThat(replicatesService.canUpdateReplicateStatus(CHAIN_TASK_ID, WALLET_WORKER_1, statusUpdate, null)) + .isEqualTo(ReplicateStatusUpdateError.GENERIC_CANT_UPDATE); + } + + @Test + void shouldNotAuthorizeUpdateOnContributeAndFinalizeDoneWhenTaskNotCompleted() { + final Replicate replicate = new Replicate(WALLET_WORKER_1, CHAIN_TASK_ID); + replicate.updateStatus(CONTRIBUTE_AND_FINALIZE_ONGOING, ReplicateStatusModifier.WORKER); + + final ReplicatesList replicatesList = new ReplicatesList(CHAIN_TASK_ID, Collections.singletonList(replicate)); + final ReplicateStatusUpdate statusUpdate = ReplicateStatusUpdate.builder() + .modifier(WORKER) + .status(CONTRIBUTE_AND_FINALIZE_DONE) + .build(); + + when(replicatesRepository.findByChainTaskId(CHAIN_TASK_ID)).thenReturn(Optional.of(replicatesList)); + when(iexecHubService.repeatIsRevealedTrue(CHAIN_TASK_ID, WALLET_WORKER_1)) + .thenReturn(true); + when(iexecHubService.isTaskInCompletedStatusOnChain(CHAIN_TASK_ID)).thenReturn(false); + + assertThat(replicatesService.canUpdateReplicateStatus(CHAIN_TASK_ID, WALLET_WORKER_1, statusUpdate, null)) + .isEqualTo(ReplicateStatusUpdateError.GENERIC_CANT_UPDATE); + } + // computeUpdateReplicateStatusArgs @Test From f5a900820d1126b06f013296287a5ea62494faac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Cordier?= Date: Tue, 9 May 2023 12:21:47 +0200 Subject: [PATCH 2/6] update changelog with PR ID --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2b533b2..13f1a253 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. ### New Features - Add ContributeAndFinalize to `ReplicateWorkflow`. (#574) -- Add check for ContributeAndFinalize in `ReplicatesService`. +- Add check for ContributeAndFinalize in `ReplicatesService`. (#576) ### Bug Fixes - Prevent race condition on replicate update. (#568) ### Quality From d7bfb9345eccbf8a190ac35955cbe0b415b5d3b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Cordier?= Date: Tue, 9 May 2023 13:41:06 +0200 Subject: [PATCH 3/6] fix format --- .../com/iexec/core/chain/IexecHubService.java | 21 ++++++++++--------- .../core/replicate/ReplicatesService.java | 20 ++++++++++-------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/iexec/core/chain/IexecHubService.java b/src/main/java/com/iexec/core/chain/IexecHubService.java index cf63c0a3..83dc9cc5 100644 --- a/src/main/java/com/iexec/core/chain/IexecHubService.java +++ b/src/main/java/com/iexec/core/chain/IexecHubService.java @@ -62,7 +62,7 @@ public IexecHubService(CredentialsService credentialsService, /** * Check if the task is defined onchain and * has the status {@link ChainTaskStatus#UNSET}. - * + * * @param chainDealId * @param taskIndex * @return true if the task is found with the status UNSET, false otherwise. @@ -77,14 +77,15 @@ public boolean isTaskInUnsetStatusOnChain(String chainDealId, int taskIndex) { public boolean isTaskInCompletedStatusOnChain(String chainTaskId) { - Optional oTask = getChainTask(chainTaskId); - return oTask.filter(chainTask -> ChainTaskStatus.COMPLETED.equals(chainTask.getStatus())).isPresent(); + return getChainTask(chainTaskId) + .filter(chainTask -> ChainTaskStatus.COMPLETED == chainTask.getStatus()) + .isPresent(); } /** * Check if a deal's contribution deadline * is still not reached. - * + * * @param chainDealId * @return true if deadline is not reached, false otherwise. */ @@ -97,7 +98,7 @@ public boolean isBeforeContributionDeadline(String chainDealId) { /** * Check if a deal's contribution deadline * is still not reached. - * + * * @param chainDeal * @return true if deadline is not reached, false otherwise. */ @@ -110,13 +111,13 @@ public boolean isBeforeContributionDeadline(ChainDeal chainDeal) { *

Get deal's contribution deadline date. The deadline * is calculated as follow: * start + maxCategoryTime * maxNbOfPeriods. - * + * *

    *
  • start: the start time of the deal. *
  • maxCategoryTime: duration of the deal's category. *
  • nbOfCategoryUnits: number of category units dedicated * for the contribution phase. - * + * * @param chainDeal * @return */ @@ -132,13 +133,13 @@ public Date getChainDealContributionDeadline(ChainDeal chainDeal) { *

    Get deal's final deadline date. The deadline * is calculated as follow: * start + maxCategoryTime * 10. - * + * *

      *
    • start: the start time of the deal. *
    • maxCategoryTime: duration of the deal's category. *
    • 10: number of category units dedicated * for the hole execution. - * + * * @param chainDeal * @return */ @@ -242,7 +243,7 @@ public boolean hasEnoughGas() { private ChainReceipt buildChainReceipt(TransactionReceipt receipt) { return ChainReceipt.builder() .txHash(receipt.getTransactionHash()) - .blockNumber(receipt.getBlockNumber() != null? + .blockNumber(receipt.getBlockNumber() != null ? receipt.getBlockNumber().longValue() : 0) .build(); } diff --git a/src/main/java/com/iexec/core/replicate/ReplicatesService.java b/src/main/java/com/iexec/core/replicate/ReplicatesService.java index 4a181f22..bb265aa6 100644 --- a/src/main/java/com/iexec/core/replicate/ReplicatesService.java +++ b/src/main/java/com/iexec/core/replicate/ReplicatesService.java @@ -350,15 +350,16 @@ public Either updateReplicateS * 3) if worker did succeed onChain when CONTRIBUTED/REVEALED. * 4) if worker did upload when RESULT_UPLOADING. */ + /** * This method updates a replicate while caring about thread safety. * A single replicate can then NOT be updated twice at the same time. * This method should be preferred to * {@link ReplicatesService#updateReplicateStatusWithoutThreadSafety(String, String, ReplicateStatusUpdate, UpdateReplicateStatusArgs)}! * - * @param chainTaskId Chain task id of the task whose replicate should be updated. - * @param walletAddress Wallet address of the worker whose replicate should be updated. - * @param statusUpdate Info about the status update - new status, date of update, ... + * @param chainTaskId Chain task id of the task whose replicate should be updated. + * @param walletAddress Wallet address of the worker whose replicate should be updated. + * @param statusUpdate Info about the status update - new status, date of update, ... * @param updateReplicateStatusArgs Optional args used to update the status. * @return Either a {@link ReplicateStatusUpdateError} if the status can't be updated, * or a next action for the worker. @@ -396,9 +397,9 @@ public Either updateReplicateS * This method has to be used with a synchronization mechanism, e.g. * {@link ReplicatesService#updateReplicateStatus(String, String, ReplicateStatus, ReplicateStatusDetails)} * - * @param chainTaskId Chain task id of the task whose replicate should be updated. - * @param walletAddress Wallet address of the worker whose replicate should be updated. - * @param statusUpdate Info about the status update - new status, date of update, ... + * @param chainTaskId Chain task id of the task whose replicate should be updated. + * @param walletAddress Wallet address of the worker whose replicate should be updated. + * @param statusUpdate Info about the status update - new status, date of update, ... * @param updateReplicateStatusArgs Optional args used to update the status. * @return Either a {@link ReplicateStatusUpdateError} if the status can't be updated, * or a next action for the worker. @@ -539,7 +540,8 @@ private boolean verifyStatus(String chainTaskId, case RESULT_UPLOAD_FAILED: return !isResultUploaded(updateReplicateStatusArgs.getTaskDescription()); case CONTRIBUTE_AND_FINALIZE_DONE: - return iexecHubService.repeatIsRevealedTrue(chainTaskId,walletAddress) && iexecHubService.isTaskInCompletedStatusOnChain(chainTaskId); + return iexecHubService.repeatIsRevealedTrue(chainTaskId, walletAddress) + && iexecHubService.isTaskInCompletedStatusOnChain(chainTaskId); default: return true; } @@ -586,7 +588,7 @@ private String getStatusUpdateLogs(String chainTaskId, Replicate replicate, Repl public boolean isResultUploaded(String chainTaskId) { Optional task = iexecHubService.getTaskDescriptionFromChain(chainTaskId); - if (task.isEmpty()){ + if (task.isEmpty()) { return false; } @@ -595,7 +597,7 @@ public boolean isResultUploaded(String chainTaskId) { public boolean isResultUploaded(TaskDescription task) { // Offchain computing - basic & tee - if (task.containsCallback()){ + if (task.containsCallback()) { return true; } From f34adb8b7e0f493e18c286c97ce62e448a3287cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Cordier?= Date: Tue, 9 May 2023 14:55:43 +0200 Subject: [PATCH 4/6] add some tests for iexecHubService and remove unused import --- .../core/chain/IexecHubServiceTests.java | 75 +++++++++++++++++++ .../core/replicate/ReplicateServiceTests.java | 2 - 2 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 src/test/java/com/iexec/core/chain/IexecHubServiceTests.java diff --git a/src/test/java/com/iexec/core/chain/IexecHubServiceTests.java b/src/test/java/com/iexec/core/chain/IexecHubServiceTests.java new file mode 100644 index 00000000..953c554c --- /dev/null +++ b/src/test/java/com/iexec/core/chain/IexecHubServiceTests.java @@ -0,0 +1,75 @@ +package com.iexec.core.chain; + +import com.iexec.commons.poco.chain.ChainTask; +import com.iexec.commons.poco.chain.ChainTaskStatus; +import com.iexec.commons.poco.contract.generated.IexecHubContract; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mock; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; +import org.web3j.crypto.Credentials; +import org.web3j.crypto.Keys; +import org.web3j.protocol.core.RemoteFunctionCall; +import org.web3j.tx.TransactionManager; + +import java.math.BigInteger; +import java.util.Optional; + +import static com.iexec.commons.poco.utils.TestUtils.CHAIN_TASK_ID; +import static org.assertj.core.api.AssertionsForClassTypes.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.*; + +public class IexecHubServiceTests { + + @Mock + private CredentialsService credentialsService; + + @Mock + private Web3jService web3jService; + + @Mock + private ChainConfig chainConfig; + + private IexecHubService iexecHubService; + + @BeforeEach + void init() throws Exception { + MockitoAnnotations.openMocks(this); + + final Credentials credentials = Credentials.create(Keys.createEcKeyPair()); + + when(credentialsService.getCredentials()).thenReturn(credentials); + when(web3jService.hasEnoughGas(any())).thenReturn(true); + when(chainConfig.getHubAddress()).thenReturn("0x748e091bf16048cb5103E0E10F9D5a8b7fBDd860"); + + try (MockedStatic iexecHubContract = Mockito.mockStatic(IexecHubContract.class)) { + final IexecHubContract mockIexecContract = mock(IexecHubContract.class); + final RemoteFunctionCall mockRemoteFunctionCall = mock(RemoteFunctionCall.class); + iexecHubContract.when(() -> IexecHubContract.load(any(), any(), (TransactionManager) any(), any())) + .thenReturn(mockIexecContract); + when(mockIexecContract.contribution_deadline_ratio()).thenReturn(mockRemoteFunctionCall); + when(mockRemoteFunctionCall.send()).thenReturn(BigInteger.ONE); + iexecHubService = spy(new IexecHubService(credentialsService, web3jService, chainConfig)); + } + } + + + @Test + void shouldTaskBeInCompletedStatusOnChain() { + final ChainTask task = ChainTask.builder().status(ChainTaskStatus.COMPLETED).build(); + doReturn(Optional.of(task)).when(iexecHubService).getChainTask(CHAIN_TASK_ID); + + assertThat(iexecHubService.isTaskInCompletedStatusOnChain(CHAIN_TASK_ID)).isTrue(); + } + + @Test + void shouldTaskNotBeInCompletedStatusOnChain() { + final ChainTask task = ChainTask.builder().status(ChainTaskStatus.REVEALING).build(); + doReturn(Optional.of(task)).when(iexecHubService).getChainTask(CHAIN_TASK_ID); + + assertThat(iexecHubService.isTaskInCompletedStatusOnChain(CHAIN_TASK_ID)).isFalse(); + } +} diff --git a/src/test/java/com/iexec/core/replicate/ReplicateServiceTests.java b/src/test/java/com/iexec/core/replicate/ReplicateServiceTests.java index 1cd769d5..7fa51110 100644 --- a/src/test/java/com/iexec/core/replicate/ReplicateServiceTests.java +++ b/src/test/java/com/iexec/core/replicate/ReplicateServiceTests.java @@ -19,8 +19,6 @@ import com.iexec.common.replicate.*; import com.iexec.commons.poco.chain.ChainContribution; import com.iexec.commons.poco.chain.ChainContributionStatus; -import com.iexec.commons.poco.chain.ChainTask; -import com.iexec.commons.poco.chain.ChainTaskStatus; import com.iexec.commons.poco.notification.TaskNotificationType; import com.iexec.commons.poco.task.TaskDescription; import com.iexec.commons.poco.utils.BytesUtils; From a541f3787699c016c23635195446cee734be91e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Cordier?= Date: Tue, 9 May 2023 14:57:48 +0200 Subject: [PATCH 5/6] remove unused import --- src/main/java/com/iexec/core/chain/IexecHubService.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/com/iexec/core/chain/IexecHubService.java b/src/main/java/com/iexec/core/chain/IexecHubService.java index 83dc9cc5..94b582fc 100644 --- a/src/main/java/com/iexec/core/chain/IexecHubService.java +++ b/src/main/java/com/iexec/core/chain/IexecHubService.java @@ -33,7 +33,6 @@ import java.util.concurrent.ThreadPoolExecutor; import static com.iexec.common.utils.DateTimeUtils.now; -import static com.iexec.commons.poco.chain.ChainContributionStatus.CONTRIBUTED; import static com.iexec.commons.poco.utils.BytesUtils.stringToBytes; @Slf4j From 92977d733c40baad1b73a85deb09a2366d57d71c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Cordier?= Date: Tue, 9 May 2023 15:01:05 +0200 Subject: [PATCH 6/6] Remove public modifier in tests --- src/test/java/com/iexec/core/chain/IexecHubServiceTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/com/iexec/core/chain/IexecHubServiceTests.java b/src/test/java/com/iexec/core/chain/IexecHubServiceTests.java index 953c554c..f4f92921 100644 --- a/src/test/java/com/iexec/core/chain/IexecHubServiceTests.java +++ b/src/test/java/com/iexec/core/chain/IexecHubServiceTests.java @@ -22,7 +22,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; -public class IexecHubServiceTests { +class IexecHubServiceTests { @Mock private CredentialsService credentialsService;