From 272f561805c1281c497f30d44141e9cd08aea564 Mon Sep 17 00:00:00 2001 From: julia-zack Date: Thu, 31 Oct 2024 17:10:11 -0300 Subject: [PATCH 1/2] Create updateSvpState method and make all svp-related-methods private Minor refactors to avoid duplication code Rewrite comment Fix sonar complains Handle exception when processing fund transaction. Add logs for when updating svp state Add debug level log for when proposed fed does not exist Not allow validation period end block to be part of validation ongoing period Make updateSvpState private. Call it from updateCollections Add missing import Fix test --- .../main/java/co/rsk/peg/BridgeSupport.java | 99 ++- .../java/co/rsk/peg/BridgeSupportSvpTest.java | 583 ++++++++---------- 2 files changed, 325 insertions(+), 357 deletions(-) diff --git a/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java b/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java index 6c9a001dbb..570ca8c815 100644 --- a/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java +++ b/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java @@ -1010,6 +1010,8 @@ public void updateCollections(Transaction rskTx) throws IOException { processConfirmedPegouts(rskTx); updateFederationCreationBlockHeights(); + + updateSvpState(rskTx); } private void logUpdateCollections(Transaction rskTx) { @@ -1017,13 +1019,51 @@ private void logUpdateCollections(Transaction rskTx) { eventLogger.logUpdateCollections(sender); } - protected void processSVPFailure(Federation proposedFederation) { + private void updateSvpState(Transaction rskTx) { + Optional proposedFederationOpt = federationSupport.getProposedFederation(); + if (proposedFederationOpt.isEmpty()) { + logger.debug("[updateSvpState] Proposed federation does not exist, so there's no svp going on."); + return; + } + + // if the proposed federation exists and the validation period ended, + // we can conclude that the svp failed + Federation proposedFederation = proposedFederationOpt.get(); + if (!validationPeriodIsOngoing(proposedFederation)) { + logger.info( + "[updateSvpState] Proposed federation validation failed at block {}. SVP failure will be processed", + rskExecutionBlock.getNumber() + ); + processSVPFailure(proposedFederation); + return; + } + + Keccak256 rskTxHash = rskTx.getHash(); + // if the fund tx signed is present, then the fund transaction change was registered, + // meaning we can create the spend tx. + Optional svpFundTxSigned = provider.getSvpFundTxSigned(); + if (svpFundTxSigned.isPresent()) { + logger.info( + "[updateSvpState] Fund tx signed was found, so spend tx creation will be processed." + ); + processSvpSpendTransactionUnsigned(rskTxHash, proposedFederation, svpFundTxSigned.get()); + return; + } + + // if the unsigned fund tx hash is not present, meaning we can proceed with the fund tx creation + Optional svpFundTxHashUnsigned = provider.getSvpFundTxHashUnsigned(); + if (svpFundTxHashUnsigned.isEmpty()) { + logger.info( + "[updateSvpState] Fund tx hash unsigned was not found, so fund tx creation will be processed." + ); + processSvpFundTransactionUnsigned(rskTxHash, proposedFederation); + } + } + + private void processSVPFailure(Federation proposedFederation) { eventLogger.logCommitFederationFailure(rskExecutionBlock, proposedFederation); - logger.warn( - "[processSVPFailure] Proposed federation validation failed at block {}, so federation election will be allowed again.", - rskExecutionBlock.getNumber() - ); + logger.info("[processSVPFailure] Federation election will be allowed again."); allowFederationElectionAgain(); } @@ -1045,21 +1085,24 @@ private boolean validationPeriodIsOngoing(Federation proposedFederation) { return rskExecutionBlock.getNumber() < validationPeriodEndBlock; } - protected void processSvpFundTransactionUnsigned(Transaction rskTx) throws IOException, InsufficientMoneyException { - Optional proposedFederationOpt = federationSupport.getProposedFederation(); - if (proposedFederationOpt.isEmpty()) { - String message = "Proposed federation should be present when processing SVP fund transaction."; - logger.warn(message); - throw new IllegalStateException(message); - } - Federation proposedFederation = proposedFederationOpt.get(); - + private void processSvpFundTransactionUnsigned(Keccak256 rskTxHash, Federation proposedFederation) { Coin spendableValueFromProposedFederation = bridgeConstants.getSpendableValueFromProposedFederation(); - BtcTransaction svpFundTransactionUnsigned = createSvpFundTransaction(proposedFederation, spendableValueFromProposedFederation); - - provider.setSvpFundTxHashUnsigned(svpFundTransactionUnsigned.getHash()); - PegoutsWaitingForConfirmations pegoutsWaitingForConfirmations = provider.getPegoutsWaitingForConfirmations(); - settleReleaseRequest(pegoutsWaitingForConfirmations, svpFundTransactionUnsigned, rskTx.getHash(), spendableValueFromProposedFederation); + try { + BtcTransaction svpFundTransactionUnsigned = createSvpFundTransaction(proposedFederation, spendableValueFromProposedFederation); + provider.setSvpFundTxHashUnsigned(svpFundTransactionUnsigned.getHash()); + PegoutsWaitingForConfirmations pegoutsWaitingForConfirmations = provider.getPegoutsWaitingForConfirmations(); + settleReleaseRequest(pegoutsWaitingForConfirmations, svpFundTransactionUnsigned, rskTxHash, spendableValueFromProposedFederation); + } catch (InsufficientMoneyException e) { + logger.error( + "[processSvpFundTransactionUnsigned] Insufficient funds for creating the fund transaction. Error message: {}", + e.getMessage() + ); + } catch (IOException e) { + logger.error( + "[processSvpFundTransactionUnsigned] IOException getting the pegouts waiting for confirmations. Error message: {}", + e.getMessage() + ); + } } private BtcTransaction createSvpFundTransaction(Federation proposedFederation, Coin spendableValueFromProposedFederation) throws InsufficientMoneyException { @@ -1092,19 +1135,13 @@ private SendRequest createSvpFundTransactionSendRequest(BtcTransaction transacti return sendRequest; } - protected void processSvpSpendTransactionUnsigned(Transaction rskTx) { - federationSupport.getProposedFederation() - .ifPresent(proposedFederation -> provider.getSvpFundTxSigned() - .ifPresent(svpFundTxSigned -> { - BtcTransaction svpSpendTransactionUnsigned = createSvpSpendTransaction(svpFundTxSigned, proposedFederation); - - Keccak256 rskTxHash = rskTx.getHash(); - updateSvpSpendTransactionValues(rskTxHash, svpSpendTransactionUnsigned); + private void processSvpSpendTransactionUnsigned(Keccak256 rskTxHash, Federation proposedFederation, BtcTransaction svpFundTxSigned) { + BtcTransaction svpSpendTransactionUnsigned = createSvpSpendTransaction(svpFundTxSigned, proposedFederation); + updateSvpSpendTransactionValues(rskTxHash, svpSpendTransactionUnsigned); - Coin amountSentToActiveFed = svpSpendTransactionUnsigned.getOutput(0).getValue(); - logReleaseRequested(rskTxHash, svpSpendTransactionUnsigned, amountSentToActiveFed); - logPegoutTransactionCreated(svpSpendTransactionUnsigned); - })); + Coin amountSentToActiveFed = svpSpendTransactionUnsigned.getOutput(0).getValue(); + logReleaseRequested(rskTxHash, svpSpendTransactionUnsigned, amountSentToActiveFed); + logPegoutTransactionCreated(svpSpendTransactionUnsigned); } private BtcTransaction createSvpSpendTransaction(BtcTransaction svpFundTxSigned, Federation proposedFederation) { diff --git a/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java b/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java index 36abb4ccad..40efac33e9 100644 --- a/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java +++ b/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java @@ -9,6 +9,7 @@ import static co.rsk.peg.bitcoin.BitcoinUtils.*; import static co.rsk.peg.bitcoin.UtxoUtils.extractOutpointValues; import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -50,6 +51,7 @@ public class BridgeSupportSvpTest { private static final Coin spendableValueFromProposedFederation = bridgeMainNetConstants.getSpendableValueFromProposedFederation(); private static final Coin feePerKb = Coin.valueOf(1000L); + private static final Keccak256 svpSpendTxCreationHash = RskTestUtils.createHash(1); private static final CallTransaction.Function releaseRequestedEvent = BridgeEvents.RELEASE_REQUESTED.getEvent(); private static final CallTransaction.Function pegoutTransactionCreatedEvent = BridgeEvents.PEGOUT_TRANSACTION_CREATED.getEvent(); @@ -60,6 +62,7 @@ public class BridgeSupportSvpTest { private final BridgeSupportBuilder bridgeSupportBuilder = BridgeSupportBuilder.builder(); private List logs; + private BridgeEventLogger bridgeEventLogger; private Block rskExecutionBlock; private Transaction rskTx; @@ -74,6 +77,11 @@ public class BridgeSupportSvpTest { private Federation activeFederation; private Federation proposedFederation; + private Sha256Hash svpFundTransactionHashUnsigned; + private BtcTransaction svpFundTransaction; + private Sha256Hash svpSpendTransactionHashUnsigned; + private BtcTransaction svpSpendTransaction; + @BeforeEach void setUp() { long rskExecutionBlockNumber = 1000L; @@ -108,21 +116,39 @@ void setUp() { btcMainnetParams, allActivations ); + + logs = new ArrayList<>(); + bridgeEventLogger = new BridgeEventLoggerImpl( + bridgeMainNetConstants, + allActivations, + logs + ); + + ECKey key = RskTestUtils.getEcKeyFromSeed("key"); + RskAddress address = new RskAddress(key.getAddress()); + when(rskTx.getSender(any())).thenReturn(address); // to not throw when logging update collections after calling it + + bridgeSupport = bridgeSupportBuilder + .withBridgeConstants(bridgeMainNetConstants) + .withProvider(bridgeStorageProvider) + .withEventLogger(bridgeEventLogger) + .withActivations(allActivations) + .withFederationSupport(federationSupport) + .withFeePerKbSupport(feePerKbSupport) + .withExecutionBlock(rskExecutionBlock) + .build(); } @Nested @TestInstance(TestInstance.Lifecycle.PER_CLASS) @Tag("SVP failure tests") class SVPFailureTests { + @BeforeEach void setUp() { - logs = new ArrayList<>(); - BridgeEventLogger bridgeEventLogger = new BridgeEventLoggerImpl( - bridgeMainNetConstants, - allActivations, - logs - ); + arrangeExecutionBlockIsAfterValidationPeriodEnded(); + // this is needed to really check proposed federation was cleared InMemoryStorage storageAccessor = new InMemoryStorage(); FederationStorageProvider federationStorageProvider = new FederationStorageProviderImpl(storageAccessor); federationStorageProvider.setProposedFederation(proposedFederation); @@ -146,13 +172,13 @@ void setUp() { } @Test - void processValidationFailure_whenSvpFundTxHashUnsigned_shouldLogValidationFailureAndClearValue() { + void updateCollections_whenSvpFundTxHashUnsigned_shouldLogValidationFailureAndClearValue() throws IOException { // arrange - Sha256Hash svpFundTxHashUnsigned = BitcoinTestUtils.createHash(1); - bridgeStorageProvider.setSvpFundTxHashUnsigned(svpFundTxHashUnsigned); + svpFundTransactionHashUnsigned = BitcoinTestUtils.createHash(1); + bridgeStorageProvider.setSvpFundTxHashUnsigned(svpFundTransactionHashUnsigned); // act - bridgeSupport.processSVPFailure(proposedFederation); + bridgeSupport.updateCollections(rskTx); // assert assertLogCommitFederationFailed(); @@ -161,13 +187,13 @@ void processValidationFailure_whenSvpFundTxHashUnsigned_shouldLogValidationFailu } @Test - void processValidationFailure_whenSvpFundTxSigned_shouldLogValidationFailureAndClearValue() { + void updateCollections_whenSvpFundTxSigned_shouldLogValidationFailureAndClearValue() throws IOException { // arrange - BtcTransaction svpFundTx = new BtcTransaction(btcMainnetParams); - bridgeStorageProvider.setSvpFundTxSigned(svpFundTx); + svpFundTransaction = new BtcTransaction(btcMainnetParams); + bridgeStorageProvider.setSvpFundTxSigned(svpFundTransaction); // act - bridgeSupport.processSVPFailure(proposedFederation); + bridgeSupport.updateCollections(rskTx); // assert assertLogCommitFederationFailed(); @@ -176,15 +202,15 @@ void processValidationFailure_whenSvpFundTxSigned_shouldLogValidationFailureAndC } @Test - void processValidationFailure_whenSvpSpendTxWFS_shouldLogValidationFailureAndClearSpendTxValues() { + void updateCollections_whenSvpSpendTxWFS_shouldLogValidationFailureAndClearSpendTxValues() throws IOException { // arrange Keccak256 svpSpendTxCreationHash = RskTestUtils.createHash(1); - BtcTransaction svpSpendTx = new BtcTransaction(btcMainnetParams); - Map.Entry svpSpendTxWFS = new AbstractMap.SimpleEntry<>(svpSpendTxCreationHash, svpSpendTx); + svpSpendTransaction = new BtcTransaction(btcMainnetParams); + Map.Entry svpSpendTxWFS = new AbstractMap.SimpleEntry<>(svpSpendTxCreationHash, svpSpendTransaction); bridgeStorageProvider.setSvpSpendTxWaitingForSignatures(svpSpendTxWFS); // act - bridgeSupport.processSVPFailure(proposedFederation); + bridgeSupport.updateCollections(rskTx); // assert assertLogCommitFederationFailed(); @@ -193,13 +219,13 @@ void processValidationFailure_whenSvpSpendTxWFS_shouldLogValidationFailureAndCle } @Test - void processValidationFailure_whenSvpSpendTxHashUnsigned_shouldLogValidationFailureAndClearValue() { + void updateCollections_whenSvpSpendTxHashUnsigned_shouldLogValidationFailureAndClearValue() throws IOException { // arrange - Sha256Hash svpSpendTxHash = BitcoinTestUtils.createHash(2); - bridgeStorageProvider.setSvpSpendTxHashUnsigned(svpSpendTxHash); + svpSpendTransactionHashUnsigned = BitcoinTestUtils.createHash(2); + bridgeStorageProvider.setSvpSpendTxHashUnsigned(svpSpendTransactionHashUnsigned); // act - bridgeSupport.processSVPFailure(proposedFederation); + bridgeSupport.updateCollections(rskTx); // assert assertLogCommitFederationFailed(); @@ -222,112 +248,58 @@ private void assertNoProposedFederation() { } private void assertNoSVPValues() { - assertFalse(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent()); - assertFalse(bridgeStorageProvider.getSvpFundTxSigned().isPresent()); - assertFalse(bridgeStorageProvider.getSvpSpendTxWaitingForSignatures().isPresent()); - assertFalse(bridgeStorageProvider.getSvpSpendTxHashUnsigned().isPresent()); + assertNoSvpFundTxHashUnsigned(); + assertNoSvpFundTxSigned(); + assertNoSvpSpendTxWFS(); + assertNoSvpSpendTxHash(); } } @Nested @TestInstance(TestInstance.Lifecycle.PER_CLASS) - @Tag("svp fund transaction creation and processing tests") - class SvpFundTxCreationAndProcessingTests { - private BtcTransaction svpFundTransactionUnsigned; - private Sha256Hash svpFundTransactionHashUnsigned; - - @BeforeEach - void setUp() { - logs = new ArrayList<>(); - BridgeEventLogger bridgeEventLogger = new BridgeEventLoggerImpl( - bridgeMainNetConstants, - allActivations, - logs - ); - - bridgeSupport = bridgeSupportBuilder - .withBridgeConstants(bridgeMainNetConstants) - .withProvider(bridgeStorageProvider) - .withEventLogger(bridgeEventLogger) - .withActivations(allActivations) - .withFederationSupport(federationSupport) - .withFeePerKbSupport(feePerKbSupport) - .withExecutionBlock(rskExecutionBlock) - .build(); - } - + @Tag("Fund transaction creation and processing tests") + class FundTxCreationAndProcessingTests { @Test - void processSvpFundTransactionWithoutSignatures_whenProposedFederationDoesNotExist_throwsIllegalStateException() { + void updateCollections_whenProposedFederationDoesNotExist_shouldNotCreateFundTransaction() throws IOException { // arrange when(federationSupport.getProposedFederation()).thenReturn(Optional.empty()); - // act & assert - IllegalStateException exception = assertThrows(IllegalStateException.class, - () -> bridgeSupport.processSvpFundTransactionUnsigned(rskTx)); + // act + bridgeSupport.updateCollections(rskTx); - String expectedMessage = "Proposed federation should be present when processing SVP fund transaction."; - assertEquals(expectedMessage, exception.getMessage()); + // assert + assertNoSvpFundTxHashUnsigned(); } @Test - void processSvpFundTransactionWithoutSignatures_whenThereAreNoEnoughUTXOs_throwsInsufficientMoneyException() { + void updateCollections_whenThereAreNoEnoughUTXOs_shouldNotCreateFundTransaction() throws IOException { // arrange List insufficientUtxos = new ArrayList<>(); when(federationSupport.getActiveFederationBtcUTXOs()).thenReturn(insufficientUtxos); - // act & assert - InsufficientMoneyException exception = assertThrows(InsufficientMoneyException.class, - () -> bridgeSupport.processSvpFundTransactionUnsigned(rskTx)); + // act + bridgeSupport.updateCollections(rskTx); - String totalNeededValueInBtc = spendableValueFromProposedFederation.multiply(2).toFriendlyString(); - String expectedMessage = String.format("Insufficient money, missing %s", totalNeededValueInBtc); - assertEquals(expectedMessage, exception.getMessage()); + // assert + assertNoSvpFundTxHashUnsigned(); } @Test - void processSvpFundTransactionWithoutSignatures_createsExpectedTransactionAndSavesTheHashInStorageEntryAndPerformsPegoutActions() throws Exception { + void updateCollections_whenFundTxCanBeCreated_createsExpectedFundTxAndSavesTheHashInStorageEntryAndPerformsPegoutActions() throws Exception { // act - bridgeSupport.processSvpFundTransactionUnsigned(rskTx); + bridgeSupport.updateCollections(rskTx); bridgeStorageProvider.save(); // to save the tx sig hash // assert assertSvpFundTxHashUnsignedWasSavedInStorage(); - assertSvpReleaseWasSettled(); + assertSvpFundTxReleaseWasSettled(); assertSvpFundTransactionHasExpectedInputsAndOutputs(); } - private void assertSvpFundTxHashUnsignedWasSavedInStorage() { - Optional svpFundTransactionHashUnsignedOpt = bridgeStorageProvider.getSvpFundTxHashUnsigned(); - assertTrue(svpFundTransactionHashUnsignedOpt.isPresent()); - - svpFundTransactionHashUnsigned = svpFundTransactionHashUnsignedOpt.get(); - } - - private void assertSvpReleaseWasSettled() throws IOException { - PegoutsWaitingForConfirmations pegoutsWaitingForConfirmations = bridgeStorageProvider.getPegoutsWaitingForConfirmations(); - assertPegoutWasAddedToPegoutsWaitingForConfirmations(pegoutsWaitingForConfirmations, svpFundTransactionHashUnsigned, rskTx.getHash()); - - svpFundTransactionUnsigned = getSvpFundTransactionFromPegoutsMap(pegoutsWaitingForConfirmations); - - assertPegoutTxSigHashWasSaved(svpFundTransactionUnsigned); - assertLogReleaseRequested(rskTx.getHash(), svpFundTransactionHashUnsigned, spendableValueFromProposedFederation); - assertLogPegoutTransactionCreated(svpFundTransactionUnsigned); - } - - private BtcTransaction getSvpFundTransactionFromPegoutsMap(PegoutsWaitingForConfirmations pegoutsWaitingForConfirmations) { - Set pegoutEntries = pegoutsWaitingForConfirmations.getEntries(); - assertEquals(1, pegoutEntries.size()); - // we now know that the only present pegout is the fund tx - Iterator iterator = pegoutEntries.iterator(); - PegoutsWaitingForConfirmations.Entry pegoutEntry = iterator.next(); - - return pegoutEntry.getBtcTransaction(); - } - private void assertSvpFundTransactionHasExpectedInputsAndOutputs() { assertInputsAreFromActiveFederation(); - List svpFundTransactionUnsignedOutputs = svpFundTransactionUnsigned.getOutputs(); + List svpFundTransactionUnsignedOutputs = svpFundTransaction.getOutputs(); int svpFundTransactionUnsignedOutputsExpectedSize = 3; assertEquals(svpFundTransactionUnsignedOutputsExpectedSize, svpFundTransactionUnsignedOutputs.size()); assertOneOutputIsChange(svpFundTransactionUnsignedOutputs); @@ -337,7 +309,7 @@ private void assertSvpFundTransactionHasExpectedInputsAndOutputs() { private void assertInputsAreFromActiveFederation() { Script activeFederationScriptSig = PegTestUtils.createBaseInputScriptThatSpendsFromTheFederation(activeFederation); - List inputs = svpFundTransactionUnsigned.getInputs(); + List inputs = svpFundTransaction.getInputs(); for (TransactionInput input : inputs) { assertEquals(activeFederationScriptSig, input.getScriptSig()); @@ -363,49 +335,91 @@ private void assertOneOutputIsToProposedFederationWithFlyoverPrefixWithExpectedA assertOutputWasSentToExpectedScriptWithExpectedAmount(svpFundTransactionUnsignedOutputs, proposedFederationWithFlyoverPrefixScriptPubKey, spendableValueFromProposedFederation); } + } - private void assertPegoutWasAddedToPegoutsWaitingForConfirmations(PegoutsWaitingForConfirmations pegoutsWaitingForConfirmations, Sha256Hash pegoutTransactionHash, Keccak256 releaseCreationTxHash) { - Set pegoutEntries = pegoutsWaitingForConfirmations.getEntries(); - Optional pegoutEntry = pegoutEntries.stream() - .filter(entry -> entry.getBtcTransaction().getHash().equals(pegoutTransactionHash) && - entry.getPegoutCreationRskBlockNumber().equals(rskExecutionBlock.getNumber()) && - entry.getPegoutCreationRskTxHash().equals(releaseCreationTxHash)) - .findFirst(); - assertTrue(pegoutEntry.isPresent()); - } + private void arrangeSvpFundTransactionUnsigned() { + recreateSvpFundTransactionUnsigned(); + addOutputChange(svpFundTransaction); + saveSvpFundTransactionUnsigned(); + } + + private void saveSvpFundTransactionUnsigned() { + savePegoutIndex(svpFundTransaction); + saveSvpFundTransactionHashUnsigned(svpFundTransaction.getHash()); + } - private void assertPegoutTxSigHashWasSaved(BtcTransaction pegoutTransaction) { - Optional pegoutTxSigHashOpt = BitcoinUtils.getFirstInputSigHash(pegoutTransaction); - assertTrue(pegoutTxSigHashOpt.isPresent()); + private void savePegoutIndex(BtcTransaction pegout) { + BitcoinUtils.getFirstInputSigHash(pegout) + .ifPresent(inputSigHash -> bridgeStorageProvider.setPegoutTxSigHash(inputSigHash)); + } - Sha256Hash pegoutTxSigHash = pegoutTxSigHashOpt.get(); - assertTrue(bridgeStorageProvider.hasPegoutTxSigHash(pegoutTxSigHash)); - } + private void saveSvpFundTransactionHashUnsigned(Sha256Hash svpFundTransactionHashUnsigned) { + bridgeStorageProvider.setSvpFundTxHashUnsigned(svpFundTransactionHashUnsigned); + bridgeSupport.save(); + } + + private void assertSvpFundTxHashUnsignedWasSavedInStorage() { + Optional svpFundTransactionHashUnsignedOpt = bridgeStorageProvider.getSvpFundTxHashUnsigned(); + assertTrue(svpFundTransactionHashUnsignedOpt.isPresent()); + + svpFundTransactionHashUnsigned = svpFundTransactionHashUnsignedOpt.get(); + } + + private void assertSvpFundTxReleaseWasSettled() throws IOException { + PegoutsWaitingForConfirmations pegoutsWaitingForConfirmations = bridgeStorageProvider.getPegoutsWaitingForConfirmations(); + assertPegoutWasAddedToPegoutsWaitingForConfirmations(pegoutsWaitingForConfirmations, svpFundTransactionHashUnsigned, rskTx.getHash()); + + svpFundTransaction = getSvpFundTransactionFromPegoutsMap(pegoutsWaitingForConfirmations); + + assertPegoutTxSigHashWasSaved(svpFundTransaction); + assertLogReleaseRequested(rskTx.getHash(), svpFundTransactionHashUnsigned, spendableValueFromProposedFederation); + assertLogPegoutTransactionCreated(svpFundTransaction); + } + + private void assertPegoutWasAddedToPegoutsWaitingForConfirmations(PegoutsWaitingForConfirmations pegoutsWaitingForConfirmations, Sha256Hash pegoutTransactionHash, Keccak256 releaseCreationTxHash) { + Set pegoutEntries = pegoutsWaitingForConfirmations.getEntries(); + Optional pegoutEntry = pegoutEntries.stream() + .filter(entry -> entry.getBtcTransaction().getHash().equals(pegoutTransactionHash) && + entry.getPegoutCreationRskBlockNumber().equals(rskExecutionBlock.getNumber()) && + entry.getPegoutCreationRskTxHash().equals(releaseCreationTxHash)) + .findFirst(); + assertTrue(pegoutEntry.isPresent()); + } + + private BtcTransaction getSvpFundTransactionFromPegoutsMap(PegoutsWaitingForConfirmations pegoutsWaitingForConfirmations) { + Set pegoutEntries = pegoutsWaitingForConfirmations.getEntries(); + assertEquals(1, pegoutEntries.size()); + // we now know that the only present pegout is the fund tx + Iterator iterator = pegoutEntries.iterator(); + PegoutsWaitingForConfirmations.Entry pegoutEntry = iterator.next(); + + return pegoutEntry.getBtcTransaction(); + } + + private void assertPegoutTxSigHashWasSaved(BtcTransaction pegoutTransaction) { + Optional pegoutTxSigHashOpt = BitcoinUtils.getFirstInputSigHash(pegoutTransaction); + assertTrue(pegoutTxSigHashOpt.isPresent()); + + Sha256Hash pegoutTxSigHash = pegoutTxSigHashOpt.get(); + assertTrue(bridgeStorageProvider.hasPegoutTxSigHash(pegoutTxSigHash)); + } + + private void assertSvpFundTransactionValuesWereNotUpdated() { + assertTrue(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent()); + assertNoSvpFundTxSigned(); } @Nested @TestInstance(TestInstance.Lifecycle.PER_CLASS) - @Tag("svp fund transaction registration tests") - class SvpFundTxRegistrationTests { + @Tag("Fund transaction registration tests") + class FundTxRegistrationTests { private PartialMerkleTree pmtWithTransactions; private int btcBlockWithPmtHeight; - @BeforeEach - void setUp() { - bridgeSupport = bridgeSupportBuilder - .withBridgeConstants(bridgeMainNetConstants) - .withProvider(bridgeStorageProvider) - .withActivations(allActivations) - .withFederationSupport(federationSupport) - .withFeePerKbSupport(feePerKbSupport) - .withExecutionBlock(rskExecutionBlock) - .build(); - } - @Test void registerBtcTransaction_forSvpFundTransactionChange_whenProposedFederationDoesNotExist_shouldRegisterTransactionButNotUpdateSvpFundTransactionValues() throws Exception { // arrange - BtcTransaction svpFundTransaction = arrangeSvpFundTransactionUnsignedWithChange(); + arrangeSvpFundTransactionUnsigned(); signInputs(svpFundTransaction); // a transaction trying to be registered should be signed setUpForTransactionRegistration(svpFundTransaction); @@ -428,17 +442,12 @@ void registerBtcTransaction_forSvpFundTransactionChange_whenProposedFederationDo assertSvpFundTransactionValuesWereNotUpdated(); } - private void assertSvpFundTransactionValuesWereNotUpdated() { - assertTrue(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent()); - assertFalse(bridgeStorageProvider.getSvpFundTxSigned().isPresent()); - } - @Test void registerBtcTransaction_forSvpFundTransactionChange_whenValidationPeriodEnded_shouldRegisterTransactionButNotUpdateSvpFundTransactionValues() throws Exception { // arrange arrangeExecutionBlockIsAfterValidationPeriodEnded(); - BtcTransaction svpFundTransaction = arrangeSvpFundTransactionUnsignedWithChange(); + arrangeSvpFundTransactionUnsigned(); signInputs(svpFundTransaction); // a transaction trying to be registered should be signed setUpForTransactionRegistration(svpFundTransaction); @@ -461,7 +470,7 @@ void registerBtcTransaction_forSvpFundTransactionChange_whenValidationPeriodEnde @Test void registerBtcTransaction_forNormalPegout_whenSvpPeriodIsOngoing_shouldRegisterTransactionButNotUpdateSvpFundTransactionValues() throws Exception { // Arrange - arrangeSvpFundTransactionUnsignedWithChange(); + arrangeSvpFundTransactionUnsigned(); BtcTransaction pegout = createPegout(proposedFederation.getRedeemScript()); savePegoutIndex(pegout); @@ -487,8 +496,8 @@ void registerBtcTransaction_forNormalPegout_whenSvpPeriodIsOngoing_shouldRegiste @Test void registerBtcTransaction_forSvpFundTransactionWithoutChangeOutput_whenSvpPeriodIsOngoing_shouldJustUpdateSvpFundTransactionValues() throws Exception { // Arrange - BtcTransaction svpFundTransaction = recreateSvpFundTransactionUnsigned(); - saveSvpFundTransactionUnsigned(svpFundTransaction); + recreateSvpFundTransactionUnsigned(); + saveSvpFundTransactionUnsigned(); signInputs(svpFundTransaction); // a transaction trying to be registered should be signed setUpForTransactionRegistration(svpFundTransaction); @@ -512,8 +521,7 @@ void registerBtcTransaction_forSvpFundTransactionWithoutChangeOutput_whenSvpPeri @Test void registerBtcTransaction_forSvpFundTransactionChange_whenSvpPeriodIsOngoing_shouldRegisterTransactionAndUpdateSvpFundTransactionValues() throws Exception { // Arrange - BtcTransaction svpFundTransaction = arrangeSvpFundTransactionUnsignedWithChange(); - + arrangeSvpFundTransactionUnsigned(); signInputs(svpFundTransaction); // a transaction trying to be registered should be signed setUpForTransactionRegistration(svpFundTransaction); @@ -533,29 +541,6 @@ void registerBtcTransaction_forSvpFundTransactionChange_whenSvpPeriodIsOngoing_s assertSvpFundTransactionValuesWereUpdated(); } - private BtcTransaction arrangeSvpFundTransactionUnsignedWithChange() { - BtcTransaction svpFundTransaction = recreateSvpFundTransactionUnsigned(); - addOutputChange(svpFundTransaction); - saveSvpFundTransactionUnsigned(svpFundTransaction); - - return svpFundTransaction; - } - - private void saveSvpFundTransactionUnsigned(BtcTransaction svpFundTransaction) { - savePegoutIndex(svpFundTransaction); - saveSvpFundTransactionHashUnsigned(svpFundTransaction.getHash()); - } - - private void saveSvpFundTransactionHashUnsigned(Sha256Hash svpFundTransactionHashUnsigned) { - bridgeStorageProvider.setSvpFundTxHashUnsigned(svpFundTransactionHashUnsigned); - bridgeSupport.save(); - } - - private void savePegoutIndex(BtcTransaction pegout) { - BitcoinUtils.getFirstInputSigHash(pegout) - .ifPresent(inputSigHash -> bridgeStorageProvider.setPegoutTxSigHash(inputSigHash)); - } - private void setUpForTransactionRegistration(BtcTransaction transaction) throws BlockStoreException { // recreate a valid chain that has the tx, so it passes the previous checks in registerBtcTransaction BtcBlockStoreWithCache.Factory btcBlockStoreFactory = new RepositoryBtcBlockStoreWithCache.Factory(btcMainnetParams, 100, 100); @@ -597,127 +582,59 @@ private void assertSvpFundTransactionValuesWereUpdated() { Optional svpFundTransactionSignedOpt = bridgeStorageProvider.getSvpFundTxSigned(); assertTrue(svpFundTransactionSignedOpt.isPresent()); - assertFalse(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent()); + assertNoSvpFundTxHashUnsigned(); } } @Nested @TestInstance(TestInstance.Lifecycle.PER_CLASS) - @Tag("svp spend transaction creation and processing tests") - class SvpSpendTxCreationAndProcessingTests { - private BtcTransaction svpFundTransaction; - private Sha256Hash svpSpendTransactionHashUnsigned; - private BtcTransaction svpSpendTransactionUnsigned; - - @BeforeEach - void setUp() { - logs = new ArrayList<>(); - BridgeEventLogger bridgeEventLogger = new BridgeEventLoggerImpl( - bridgeMainNetConstants, - allActivations, - logs - ); - - bridgeSupport = bridgeSupportBuilder - .withBridgeConstants(bridgeMainNetConstants) - .withProvider(bridgeStorageProvider) - .withEventLogger(bridgeEventLogger) - .withActivations(allActivations) - .withFederationSupport(federationSupport) - .withFeePerKbSupport(feePerKbSupport) - .withExecutionBlock(rskExecutionBlock) - .build(); - } + @Tag("Spend transaction creation and processing tests") + class SpendTxCreationAndProcessingTests { @Test - void processSvpSpendTransaction_whenThereIsNoProposedFederation_shouldNotCreateNorProcessSpendTx() { + void updateCollections_whenThereIsNoFundTxSigned_shouldNotCreateNorProcessSpendTx() throws IOException { // act - when(federationSupport.getProposedFederation()).thenReturn(Optional.empty()); - bridgeSupport.processSvpSpendTransactionUnsigned(rskTx); + bridgeSupport.updateCollections(rskTx); bridgeStorageProvider.save(); // assert - assertSvpSpendTransactionValuesWereNotSaved(); - assertSvpSpendTransactionReleaseWasNotLogged(); + assertNoSvpSpendTxHash(); + assertNoSvpSpendTxWFS(); } @Test - void processSvpSpendTransaction_whenThereIsNoFundTxSigned_shouldNotCreateNorProcessSpendTx() { - // act - bridgeSupport.processSvpSpendTransactionUnsigned(rskTx); - bridgeStorageProvider.save(); - - // assert - assertSvpSpendTransactionValuesWereNotSaved(); - assertSvpSpendTransactionReleaseWasNotLogged(); - } - - private void assertSvpSpendTransactionValuesWereNotSaved() { - Optional svpSpendTxHashUnsigned = bridgeStorageProvider.getSvpSpendTxHashUnsigned(); - assertFalse(svpSpendTxHashUnsigned.isPresent()); - - Optional> svpSpendTxWaitingForSignatures = bridgeStorageProvider.getSvpSpendTxWaitingForSignatures(); - assertFalse(svpSpendTxWaitingForSignatures.isPresent()); - } - - private void assertSvpSpendTransactionReleaseWasNotLogged() { - assertEquals(0, logs.size()); - } - - @Test - void processSvpSpendTransaction_createsExpectedTransactionAndSavesTheValuesAndLogsExpectedEvents() { + void updateCollections_whenSpendTxCanBeCreated_createsExpectedSpendTxAndSavesTheValuesAndLogsExpectedEvents() throws IOException { // arrange - svpFundTransaction = arrangeSvpFundTransactionSigned(); + arrangeSvpFundTransactionSigned(); // act - bridgeSupport.processSvpSpendTransactionUnsigned(rskTx); + bridgeSupport.updateCollections(rskTx); bridgeStorageProvider.save(); // assert - assertSvpSpendTxHashUnsignedWasSavedInStorage(); - assertSvpSpendTxIsWaitingForSignatures(); + svpSpendTransactionHashUnsigned = assertSvpSpendTxHashUnsignedWasSavedInStorage(); + svpSpendTransaction = assertSvpSpendTxIsWaitingForSignatures(); assertSvpSpendTxHasExpectedInputsAndOutputs(); assertSvpFundTxSignedWasRemovedFromStorage(); - assertLogPegoutTransactionCreated(svpSpendTransactionUnsigned); - - TransactionOutput outputToActiveFed = svpSpendTransactionUnsigned.getOutput(0); + assertLogPegoutTransactionCreated(svpSpendTransaction); + TransactionOutput outputToActiveFed = svpSpendTransaction.getOutput(0); assertLogReleaseRequested(rskTx.getHash(), svpSpendTransactionHashUnsigned, outputToActiveFed.getValue()); } - private void assertSvpSpendTxHashUnsignedWasSavedInStorage() { - Optional svpSpendTransactionHashUnsignedOpt = bridgeStorageProvider.getSvpSpendTxHashUnsigned(); - assertTrue(svpSpendTransactionHashUnsignedOpt.isPresent()); - - svpSpendTransactionHashUnsigned = svpSpendTransactionHashUnsignedOpt.get(); - } - - private void assertSvpSpendTxIsWaitingForSignatures() { - Optional> svpSpendTxWaitingForSignaturesOpt = bridgeStorageProvider.getSvpSpendTxWaitingForSignatures(); - assertTrue(svpSpendTxWaitingForSignaturesOpt.isPresent()); - Map.Entry svpSpendTxWaitingForSignatures = svpSpendTxWaitingForSignaturesOpt.get(); - - Keccak256 svpSpendTxWaitingForSignaturesRskTxHash = svpSpendTxWaitingForSignatures.getKey(); - assertEquals(rskTx.getHash(), svpSpendTxWaitingForSignaturesRskTxHash); - - BtcTransaction svpSpendTxWaitingForSignaturesSpendTx = svpSpendTxWaitingForSignatures.getValue(); - assertEquals(svpSpendTransactionHashUnsigned, svpSpendTxWaitingForSignaturesSpendTx.getHash()); - svpSpendTransactionUnsigned = svpSpendTxWaitingForSignaturesSpendTx; - } - private void assertSvpFundTxSignedWasRemovedFromStorage() { Optional svpFundTxSigned = bridgeStorageProvider.getSvpFundTxSigned(); assertFalse(svpFundTxSigned.isPresent()); } private void assertSvpSpendTxHasExpectedInputsAndOutputs() { - List inputs = svpSpendTransactionUnsigned.getInputs(); + List inputs = svpSpendTransaction.getInputs(); assertEquals(2, inputs.size()); assertInputsHaveExpectedScriptSig(inputs); assertInputsOutpointHashIsFundTxHash(inputs, svpFundTransaction.getHash()); - List outputs = svpSpendTransactionUnsigned.getOutputs(); + List outputs = svpSpendTransaction.getOutputs(); assertEquals(1, outputs.size()); long calculatedTransactionSize = 1762L; // using calculatePegoutTxSize method @@ -756,40 +673,43 @@ private void assertInputHasExpectedScriptSig(TransactionInput input, Script rede } } + private Sha256Hash assertSvpSpendTxHashUnsignedWasSavedInStorage() { + Optional svpSpendTransactionHashUnsignedOpt = bridgeStorageProvider.getSvpSpendTxHashUnsigned(); + assertTrue(svpSpendTransactionHashUnsignedOpt.isPresent()); + + svpSpendTransactionHashUnsigned = svpSpendTransactionHashUnsignedOpt.get(); + return svpSpendTransactionHashUnsigned; + } + + private BtcTransaction assertSvpSpendTxIsWaitingForSignatures() { + Optional> svpSpendTxWaitingForSignaturesOpt = bridgeStorageProvider.getSvpSpendTxWaitingForSignatures(); + assertTrue(svpSpendTxWaitingForSignaturesOpt.isPresent()); + Map.Entry svpSpendTxWaitingForSignatures = svpSpendTxWaitingForSignaturesOpt.get(); + + Keccak256 svpSpendTxWaitingForSignaturesRskTxHash = svpSpendTxWaitingForSignatures.getKey(); + assertEquals(rskTx.getHash(), svpSpendTxWaitingForSignaturesRskTxHash); + + BtcTransaction svpSpendTxWaitingForSignaturesSpendTx = svpSpendTxWaitingForSignatures.getValue(); + assertEquals(svpSpendTransactionHashUnsigned, svpSpendTxWaitingForSignaturesSpendTx.getHash()); + + return svpSpendTxWaitingForSignaturesSpendTx; + } + @Nested @TestInstance(TestInstance.Lifecycle.PER_CLASS) - @Tag("svp spend transaction signing tests") - class SvpSpendTxSigning { - private static final List PROPOSED_FEDERATION_SIGNERS_KEYS = + @Tag("Spend transaction signing tests") + class SpendTxSigning { + private final List proposedFederationSignersKeys = BitcoinTestUtils.getBtcEcKeysFromSeeds(new String[]{"member01", "member02", "member03", "member04", "member05"}, true); // this is needed to have the private keys too - private static final Keccak256 svpSpendTxCreationHash = RskTestUtils.createHash(1); + private final Keccak256 svpSpendTxCreationHash = RskTestUtils.createHash(1); - private BtcTransaction svpSpendTx; private List svpSpendTxSigHashes; - private BridgeEventLogger bridgeEventLogger; @BeforeEach void setUp() { - logs = new ArrayList<>(); - bridgeEventLogger = new BridgeEventLoggerImpl( - bridgeMainNetConstants, - allActivations, - logs - ); - - bridgeSupport = bridgeSupportBuilder - .withBridgeConstants(bridgeMainNetConstants) - .withProvider(bridgeStorageProvider) - .withActivations(allActivations) - .withFederationSupport(federationSupport) - .withFeePerKbSupport(feePerKbSupport) - .withEventLogger(bridgeEventLogger) - .withExecutionBlock(rskExecutionBlock) - .build(); - arrangeSvpSpendTransaction(); - svpSpendTxSigHashes = generateTransactionInputsSigHashes(svpSpendTx); + svpSpendTxSigHashes = generateTransactionInputsSigHashes(svpSpendTransaction); } @Test @@ -807,7 +727,7 @@ void addSignature_forSvpSpendTx_withWrongKeys_shouldThrowIllegalStateExceptionAn // assert for (BtcECKey wrongSigningKey : wrongSigningKeys) { - assertFederatorDidNotSignInputs(svpSpendTx.getInputs(), svpSpendTxSigHashes, wrongSigningKey); + assertFederatorDidNotSignInputs(svpSpendTransaction.getInputs(), svpSpendTxSigHashes, wrongSigningKey); } assertAddSignatureWasNotLogged(); @@ -820,14 +740,14 @@ void addSignature_forSvpSpendTx_whenProposedFederationDoesNotExist_shouldNotAddP when(federationSupport.getProposedFederation()).thenReturn(Optional.empty()); // act - for (BtcECKey proposedFederatorSignerKey : PROPOSED_FEDERATION_SIGNERS_KEYS) { + for (BtcECKey proposedFederatorSignerKey : proposedFederationSignersKeys) { List signatures = generateSignerEncodedSignatures(proposedFederatorSignerKey, svpSpendTxSigHashes); bridgeSupport.addSignature(proposedFederatorSignerKey, signatures, svpSpendTxCreationHash); } // assert - for (BtcECKey key : PROPOSED_FEDERATION_SIGNERS_KEYS) { - assertFederatorDidNotSignInputs(svpSpendTx.getInputs(), svpSpendTxSigHashes, key); + for (BtcECKey key : proposedFederationSignersKeys) { + assertFederatorDidNotSignInputs(svpSpendTransaction.getInputs(), svpSpendTxSigHashes, key); } assertAddSignatureWasNotLogged(); @@ -849,14 +769,14 @@ void addSignature_forSvpSpendTx_whenValidationPeriodEnded_shouldNotAddProposedFe .build(); // act - for (BtcECKey proposedFederatorSignerKeys : PROPOSED_FEDERATION_SIGNERS_KEYS) { + for (BtcECKey proposedFederatorSignerKeys : proposedFederationSignersKeys) { List signatures = generateSignerEncodedSignatures(proposedFederatorSignerKeys, svpSpendTxSigHashes); bridgeSupport.addSignature(proposedFederatorSignerKeys, signatures, svpSpendTxCreationHash); } // assert - for (BtcECKey proposedFederatorSignerKeys : PROPOSED_FEDERATION_SIGNERS_KEYS) { - assertFederatorDidNotSignInputs(svpSpendTx.getInputs(), svpSpendTxSigHashes, proposedFederatorSignerKeys); + for (BtcECKey proposedFederatorSignerKeys : proposedFederationSignersKeys) { + assertFederatorDidNotSignInputs(svpSpendTransaction.getInputs(), svpSpendTxSigHashes, proposedFederatorSignerKeys); } assertAddSignatureWasNotLogged(); @@ -866,15 +786,15 @@ void addSignature_forSvpSpendTx_whenValidationPeriodEnded_shouldNotAddProposedFe @Test void addSignature_forSvpSpendTx_withoutEnoughSignatures_shouldNotAddProposedFederatorsSignatures() throws Exception { // act - for (BtcECKey proposedFederatorSignerKey : PROPOSED_FEDERATION_SIGNERS_KEYS) { + for (BtcECKey proposedFederatorSignerKey : proposedFederationSignersKeys) { List signatures = generateSignerEncodedSignatures(proposedFederatorSignerKey, svpSpendTxSigHashes); List notEnoughSignatures = signatures.subList(0, signatures.size() - 1); bridgeSupport.addSignature(proposedFederatorSignerKey, notEnoughSignatures, svpSpendTxCreationHash); } // assert - for (BtcECKey proposedFederatorSignerKeys : PROPOSED_FEDERATION_SIGNERS_KEYS) { - assertFederatorDidNotSignInputs(svpSpendTx.getInputs(), svpSpendTxSigHashes, proposedFederatorSignerKeys); + for (BtcECKey proposedFederatorSignerKeys : proposedFederationSignersKeys) { + assertFederatorDidNotSignInputs(svpSpendTransaction.getInputs(), svpSpendTxSigHashes, proposedFederatorSignerKeys); } assertAddSignatureWasNotLogged(); @@ -899,24 +819,24 @@ private void assertSvpSpendTxWFSWasNotRemoved() { @Test void addSignature_forSvpSpendTx_shouldAddProposedFederatorsSignatures() throws Exception { // act - for (BtcECKey proposedFederatorSignerKey : PROPOSED_FEDERATION_SIGNERS_KEYS) { + for (BtcECKey proposedFederatorSignerKey : proposedFederationSignersKeys) { List signatures = generateSignerEncodedSignatures(proposedFederatorSignerKey, svpSpendTxSigHashes); bridgeSupport.addSignature(proposedFederatorSignerKey, signatures, svpSpendTxCreationHash); } // assert - for (BtcECKey proposedFederatorSignerKey : PROPOSED_FEDERATION_SIGNERS_KEYS) { + for (BtcECKey proposedFederatorSignerKey : proposedFederationSignersKeys) { assertFederatorSigning( svpSpendTxCreationHash.getBytes(), - svpSpendTx.getInputs(), + svpSpendTransaction.getInputs(), svpSpendTxSigHashes, proposedFederation, proposedFederatorSignerKey ); } - assertLogReleaseBtc(svpSpendTxCreationHash, svpSpendTx); - assertLogsSize(PROPOSED_FEDERATION_SIGNERS_KEYS.size() + 1); // proposedFedSigners size for addSignature, +1 for release btc - assertFalse(bridgeStorageProvider.getSvpSpendTxWaitingForSignatures().isPresent()); + assertLogReleaseBtc(svpSpendTxCreationHash, svpSpendTransaction); + assertLogsSize(proposedFederationSignersKeys.size() + 1); // proposedFedSigners size for addSignature, +1 for release btc + assertNoSvpSpendTxWFS(); } @Test @@ -953,7 +873,7 @@ void addSignature_forNormalPegout_whenSvpIsOngoing_shouldAddJustActiveFederators assertLogReleaseBtc(rskTxHash, pegout); assertLogsSize(activeFedSignersKeys.size() + 1); // activeFedSignersKeys size for addSignature, +1 for release btc - for (BtcECKey key : PROPOSED_FEDERATION_SIGNERS_KEYS) { + for (BtcECKey key : proposedFederationSignersKeys) { assertFederatorDidNotSignInputs(pegoutInputs, pegoutTxSigHashes, key); } assertSvpSpendTxWFSWasNotRemoved(); @@ -1006,38 +926,6 @@ private void assertFederatorSignedInputs(List inputs, List svpSpendTxWaitingForSignatures = new AbstractMap.SimpleEntry<>(svpSpendTxCreationHash, svpSpendTx); - bridgeStorageProvider.setSvpSpendTxWaitingForSignatures(svpSpendTxWaitingForSignatures); - bridgeStorageProvider.save(); - } } private void arrangeExecutionBlockIsAfterValidationPeriodEnded() { @@ -1048,18 +936,16 @@ private void arrangeExecutionBlockIsAfterValidationPeriodEnded() { rskExecutionBlock = createRskBlock(validationPeriodEndBlock, rskExecutionBlockTimestamp); } - private BtcTransaction arrangeSvpFundTransactionSigned() { - BtcTransaction svpFundTransaction = recreateSvpFundTransactionUnsigned(); + private void arrangeSvpFundTransactionSigned() { + recreateSvpFundTransactionUnsigned(); signInputs(svpFundTransaction); bridgeStorageProvider.setSvpFundTxSigned(svpFundTransaction); bridgeStorageProvider.save(); - - return svpFundTransaction; } - private BtcTransaction recreateSvpFundTransactionUnsigned() { - BtcTransaction svpFundTransaction = new BtcTransaction(btcMainnetParams); + private void recreateSvpFundTransactionUnsigned() { + svpFundTransaction = new BtcTransaction(btcMainnetParams); Sha256Hash parentTxHash = BitcoinTestUtils.createHash(1); addInput(svpFundTransaction, parentTxHash, proposedFederation.getRedeemScript()); @@ -1068,8 +954,6 @@ private BtcTransaction recreateSvpFundTransactionUnsigned() { Address flyoverProposedFederationAddress = PegUtils.getFlyoverAddress(btcMainnetParams, bridgeMainNetConstants.getProposedFederationFlyoverPrefix(), proposedFederation.getRedeemScript()); svpFundTransaction.addOutput(spendableValueFromProposedFederation, flyoverProposedFederationAddress); - - return svpFundTransaction; } private BtcTransaction createPegout(Script redeemScript) { @@ -1105,6 +989,53 @@ private void signInputs(BtcTransaction transaction) { ); } + private void arrangeSvpSpendTransaction() { + recreateSvpSpendTransactionUnsigned(); + saveSvpSpendTransactionWFSValues(); + } + + private void recreateSvpSpendTransactionUnsigned() { + svpSpendTransaction = new BtcTransaction(btcMainnetParams); + svpSpendTransaction.setVersion(BTC_TX_VERSION_2); + + arrangeSvpFundTransactionSigned(); + // add inputs + addInputFromMatchingOutputScript(svpSpendTransaction, svpFundTransaction, proposedFederation.getP2SHScript()); + Script proposedFederationRedeemScript = proposedFederation.getRedeemScript(); + svpSpendTransaction.getInput(0) + .setScriptSig(createBaseP2SHInputScriptThatSpendsFromRedeemScript(proposedFederationRedeemScript)); + + Script flyoverRedeemScript = getFlyoverRedeemScript(bridgeMainNetConstants.getProposedFederationFlyoverPrefix(), proposedFederationRedeemScript); + addInputFromMatchingOutputScript(svpSpendTransaction, svpFundTransaction, ScriptBuilder.createP2SHOutputScript(flyoverRedeemScript)); + svpSpendTransaction.getInput(1) + .setScriptSig(createBaseP2SHInputScriptThatSpendsFromRedeemScript(flyoverRedeemScript)); + + // add output + svpSpendTransaction.addOutput(Coin.valueOf(1762), federationSupport.getActiveFederationAddress()); + } + + private void saveSvpSpendTransactionWFSValues() { + Map.Entry svpSpendTxWaitingForSignatures = new AbstractMap.SimpleEntry<>(svpSpendTxCreationHash, svpSpendTransaction); + bridgeStorageProvider.setSvpSpendTxWaitingForSignatures(svpSpendTxWaitingForSignatures); + bridgeStorageProvider.save(); + } + + private void assertNoSvpFundTxHashUnsigned() { + assertFalse(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent()); + } + + private void assertNoSvpFundTxSigned() { + assertFalse(bridgeStorageProvider.getSvpFundTxSigned().isPresent()); + } + + private void assertNoSvpSpendTxWFS() { + assertFalse(bridgeStorageProvider.getSvpSpendTxWaitingForSignatures().isPresent()); + } + + private void assertNoSvpSpendTxHash() { + assertFalse(bridgeStorageProvider.getSvpSpendTxHashUnsigned().isPresent()); + } + private void assertOutputWasSentToExpectedScriptWithExpectedAmount(List transactionOutputs, Script expectedScriptPubKey, Coin expectedAmount) { Optional expectedOutput = searchForOutput(transactionOutputs, expectedScriptPubKey); assertTrue(expectedOutput.isPresent()); From 40832340f460102eaddf8e865ada88423068d99b Mon Sep 17 00:00:00 2001 From: julia-zack Date: Wed, 13 Nov 2024 10:54:42 -0300 Subject: [PATCH 2/2] Correct and make updateSvpState easier to follow --- .../main/java/co/rsk/peg/BridgeSupport.java | 32 +++++--- .../java/co/rsk/peg/BridgeSupportSvpTest.java | 81 ++++++++++++++++--- 2 files changed, 92 insertions(+), 21 deletions(-) diff --git a/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java b/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java index 570ca8c815..9660b59b41 100644 --- a/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java +++ b/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java @@ -1022,7 +1022,7 @@ private void logUpdateCollections(Transaction rskTx) { private void updateSvpState(Transaction rskTx) { Optional proposedFederationOpt = federationSupport.getProposedFederation(); if (proposedFederationOpt.isEmpty()) { - logger.debug("[updateSvpState] Proposed federation does not exist, so there's no svp going on."); + logger.trace("[updateSvpState] Proposed federation does not exist, so there's no svp going on."); return; } @@ -1034,11 +1034,19 @@ private void updateSvpState(Transaction rskTx) { "[updateSvpState] Proposed federation validation failed at block {}. SVP failure will be processed", rskExecutionBlock.getNumber() ); - processSVPFailure(proposedFederation); + processSvpFailure(proposedFederation); return; } Keccak256 rskTxHash = rskTx.getHash(); + + if (shouldCreateAndProcessSvpFundTransaction()) { + logger.info( + "[updateSvpState] No svp values were found, so fund tx creation will be processed." + ); + processSvpFundTransactionUnsigned(rskTxHash, proposedFederation); + } + // if the fund tx signed is present, then the fund transaction change was registered, // meaning we can create the spend tx. Optional svpFundTxSigned = provider.getSvpFundTxSigned(); @@ -1047,20 +1055,22 @@ private void updateSvpState(Transaction rskTx) { "[updateSvpState] Fund tx signed was found, so spend tx creation will be processed." ); processSvpSpendTransactionUnsigned(rskTxHash, proposedFederation, svpFundTxSigned.get()); - return; } + } - // if the unsigned fund tx hash is not present, meaning we can proceed with the fund tx creation + private boolean shouldCreateAndProcessSvpFundTransaction() { + // the fund tx will be created when the svp starts, + // so we must ensure all svp values are clear to proceed with its creation Optional svpFundTxHashUnsigned = provider.getSvpFundTxHashUnsigned(); - if (svpFundTxHashUnsigned.isEmpty()) { - logger.info( - "[updateSvpState] Fund tx hash unsigned was not found, so fund tx creation will be processed." - ); - processSvpFundTransactionUnsigned(rskTxHash, proposedFederation); - } + Optional svpFundTxSigned = provider.getSvpFundTxSigned(); + Optional svpSpendTxHashUnsigned = provider.getSvpSpendTxHashUnsigned(); // spendTxHash will be removed the last, after spendTxWFS, so is enough checking just this value + + return svpFundTxHashUnsigned.isEmpty() + && svpFundTxSigned.isEmpty() + && svpSpendTxHashUnsigned.isEmpty(); } - private void processSVPFailure(Federation proposedFederation) { + private void processSvpFailure(Federation proposedFederation) { eventLogger.logCommitFederationFailure(rskExecutionBlock, proposedFederation); logger.info("[processSVPFailure] Federation election will be allowed again."); diff --git a/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java b/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java index 40efac33e9..c2414dc678 100644 --- a/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java +++ b/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java @@ -141,8 +141,8 @@ void setUp() { @Nested @TestInstance(TestInstance.Lifecycle.PER_CLASS) - @Tag("SVP failure tests") - class SVPFailureTests { + @Tag("Failure tests") + class FailureTests { @BeforeEach void setUp() { @@ -259,6 +259,7 @@ private void assertNoSVPValues() { @TestInstance(TestInstance.Lifecycle.PER_CLASS) @Tag("Fund transaction creation and processing tests") class FundTxCreationAndProcessingTests { + @Test void updateCollections_whenProposedFederationDoesNotExist_shouldNotCreateFundTransaction() throws IOException { // arrange @@ -288,10 +289,10 @@ void updateCollections_whenThereAreNoEnoughUTXOs_shouldNotCreateFundTransaction( void updateCollections_whenFundTxCanBeCreated_createsExpectedFundTxAndSavesTheHashInStorageEntryAndPerformsPegoutActions() throws Exception { // act bridgeSupport.updateCollections(rskTx); - bridgeStorageProvider.save(); // to save the tx sig hash + bridgeStorageProvider.save(); // assert - assertSvpFundTxHashUnsignedWasSavedInStorage(); + assertSvpFundTxHashUnsignedIsSavedInStorage(); assertSvpFundTxReleaseWasSettled(); assertSvpFundTransactionHasExpectedInputsAndOutputs(); } @@ -335,6 +336,59 @@ private void assertOneOutputIsToProposedFederationWithFlyoverPrefixWithExpectedA assertOutputWasSentToExpectedScriptWithExpectedAmount(svpFundTransactionUnsignedOutputs, proposedFederationWithFlyoverPrefixScriptPubKey, spendableValueFromProposedFederation); } + + @Test + void updateCollections_whenWaitingForFundTxToBeRegistered_shouldNotCreateFundTransactionAgain() throws Exception { + // arrange + arrangeSvpFundTransactionUnsigned(); + assertSvpFundTxHashUnsignedIsSavedInStorage(); + + // act + bridgeSupport.updateCollections(rskTx); + bridgeStorageProvider.save(); + + // assert + assertJustUpdateCollectionsWasLogged(); + } + + @Test + void updateCollections_whenFundTxSignedIsSaved_shouldNotCreateFundTransaction() throws Exception { + // arrange + arrangeSvpFundTransactionSigned(); + + // act + bridgeSupport.updateCollections(rskTx); + bridgeStorageProvider.save(); + + // assert + assertNoSvpFundTxHashUnsigned(); + } + + @Test + void updateCollections_whenWaitingForSpendTxToBeRegistered_shouldNotCreateFundTransaction() throws Exception { + // arrange + arrangeSvpSpendTransaction(); + + // act + bridgeSupport.updateCollections(rskTx); + bridgeStorageProvider.save(); + + // assert + assertNoSvpFundTxHashUnsigned(); + assertJustUpdateCollectionsWasLogged(); + } + + private void assertJustUpdateCollectionsWasLogged() { + assertEquals(1, logs.size()); + + CallTransaction.Function updateCollectionsEvent = BridgeEvents.UPDATE_COLLECTIONS.getEvent(); + List encodedTopics = getEncodedTopics(updateCollectionsEvent); + assertEventWasEmittedWithExpectedTopics(encodedTopics); + + String senderData = rskTx.getSender(mock(SignatureCache.class)).toHexString(); + byte[] encodedData = getEncodedData(updateCollectionsEvent, senderData); + assertEventWasEmittedWithExpectedData(encodedData); + } } private void arrangeSvpFundTransactionUnsigned() { @@ -358,7 +412,7 @@ private void saveSvpFundTransactionHashUnsigned(Sha256Hash svpFundTransactionHas bridgeSupport.save(); } - private void assertSvpFundTxHashUnsignedWasSavedInStorage() { + private void assertSvpFundTxHashUnsignedIsSavedInStorage() { Optional svpFundTransactionHashUnsignedOpt = bridgeStorageProvider.getSvpFundTxHashUnsigned(); assertTrue(svpFundTransactionHashUnsignedOpt.isPresent()); @@ -937,13 +991,17 @@ private void arrangeExecutionBlockIsAfterValidationPeriodEnded() { } private void arrangeSvpFundTransactionSigned() { - recreateSvpFundTransactionUnsigned(); - signInputs(svpFundTransaction); + recreateSvpFundTransactionSigned(); bridgeStorageProvider.setSvpFundTxSigned(svpFundTransaction); bridgeStorageProvider.save(); } + private void recreateSvpFundTransactionSigned() { + recreateSvpFundTransactionUnsigned(); + signInputs(svpFundTransaction); + } + private void recreateSvpFundTransactionUnsigned() { svpFundTransaction = new BtcTransaction(btcMainnetParams); @@ -991,14 +1049,14 @@ private void signInputs(BtcTransaction transaction) { private void arrangeSvpSpendTransaction() { recreateSvpSpendTransactionUnsigned(); - saveSvpSpendTransactionWFSValues(); + saveSvpSpendTransactionValues(); } private void recreateSvpSpendTransactionUnsigned() { svpSpendTransaction = new BtcTransaction(btcMainnetParams); svpSpendTransaction.setVersion(BTC_TX_VERSION_2); - arrangeSvpFundTransactionSigned(); + recreateSvpFundTransactionSigned(); // add inputs addInputFromMatchingOutputScript(svpSpendTransaction, svpFundTransaction, proposedFederation.getP2SHScript()); Script proposedFederationRedeemScript = proposedFederation.getRedeemScript(); @@ -1014,9 +1072,12 @@ private void recreateSvpSpendTransactionUnsigned() { svpSpendTransaction.addOutput(Coin.valueOf(1762), federationSupport.getActiveFederationAddress()); } - private void saveSvpSpendTransactionWFSValues() { + private void saveSvpSpendTransactionValues() { + bridgeStorageProvider.setSvpSpendTxHashUnsigned(svpSpendTransaction.getHash()); + Map.Entry svpSpendTxWaitingForSignatures = new AbstractMap.SimpleEntry<>(svpSpendTxCreationHash, svpSpendTransaction); bridgeStorageProvider.setSvpSpendTxWaitingForSignatures(svpSpendTxWaitingForSignatures); + bridgeStorageProvider.save(); }