Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create processValidationFailure method #2817

Merged
merged 11 commits into from
Oct 30, 2024
+296 −26
Merged

Create processValidationFailure method #2817

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bca315c
Make logs a global variable and remove it from methods arguments. Cre…
julia-zack Oct 21, 2024
5f5c63f
Correct log
julia-zack Oct 21, 2024
57bff23
Correct test class name
julia-zack Oct 21, 2024
b5e801a
Modify clearSvpValues to know and log which stage failed. Add tests f…
julia-zack Oct 22, 2024
227c718
Add fund tx signed hash log
julia-zack Oct 22, 2024
53170bf
Make events vbles static
julia-zack Oct 22, 2024
d9199e2
Refactor clearSvpValues to make code more functional
julia-zack Oct 22, 2024
085a14e
Add unit test for new clearProposedFederation method
julia-zack Oct 23, 2024
298fbbb
Improve naming and logs. Create clearSvpValues method in provider to …
julia-zack Oct 29, 2024
0016675
Assert any of svp values are present when clearing value. Add missing…
julia-zack Oct 29, 2024
e45e32e
Make logging more accurate when clearing svp values in provider. Mino…
julia-zack Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions rskj-core/src/main/java/co/rsk/peg/BridgeStorageProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import org.ethereum.config.blockchain.upgrades.ActivationConfig;
import org.ethereum.core.Repository;
import org.ethereum.vm.DataWord;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.spongycastle.util.encoders.Hex;

/**
Expand All @@ -40,6 +42,7 @@
* @author Oscar Guindzberg
*/
public class BridgeStorageProvider {
private static final Logger logger = LoggerFactory.getLogger(BridgeStorageProvider.class);

// Dummy value to use when saving key only indexes
private static final byte TRUE_VALUE = (byte) 1;
Expand Down Expand Up @@ -679,6 +682,33 @@ private void saveSvpSpendTxWaitingForSignatures() {
);
}

public void clearSvpValues() {
if (svpFundTxHashUnsigned != null) {
logger.warn("[clearSvpValues] Fund tx change {} was never registered.", svpFundTxHashUnsigned);
setSvpFundTxHashUnsigned(null);
}

if (svpFundTxSigned != null) {
logger.warn("[clearSvpValues] Spend tx was never created. Fund tx hash: {}", svpFundTxSigned.getHash());
setSvpFundTxSigned(null);
}

if (svpSpendTxWaitingForSignatures != null) {
Keccak256 rskCreationHash = svpSpendTxWaitingForSignatures.getKey();
BtcTransaction svpSpendTx = svpSpendTxWaitingForSignatures.getValue();

logger.warn("[clearSvpValues] Spend tx {} was not fully signed. Rsk creation hash: {}.",
svpSpendTx.getHash(), rskCreationHash);
setSvpSpendTxWaitingForSignatures(null);
setSvpSpendTxHashUnsigned(null);
}

if (svpSpendTxHashUnsigned != null) {
logger.warn("[clearSvpValues] Spend tx {} was not registered.", svpSpendTxHashUnsigned);
setSvpSpendTxHashUnsigned(null);
}
}

public void save() {
saveBtcTxHashesAlreadyProcessed();

Expand Down
43 changes: 8 additions & 35 deletions rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1017,47 +1017,20 @@ private void logUpdateCollections(Transaction rskTx) {
eventLogger.logUpdateCollections(sender);
}

protected void processValidationFailure(Federation proposedFederation) {
protected void processSVPFailure(Federation proposedFederation) {
eventLogger.logCommitFederationFailure(rskExecutionBlock, proposedFederation);
logger.warn("[processValidationFailure] Proposed federation validation failed so svp values will be cleared.");
logger.warn("[processSVPFailure] Proposed federation validation failed at block {}, so svp values will be cleared.", rskExecutionBlock.getNumber());

clearProposedFederation();
clearSvpValues();
}

private void clearSvpValues() {
private void clearProposedFederation() {
federationSupport.clearProposedFederation();
}

provider.getSvpFundTxHashUnsigned().ifPresent(
svpFundTxHashUnsigned -> {
logger.warn("[clearSvpValues] Fund tx change {} was never registered.", svpFundTxHashUnsigned);
provider.setSvpFundTxHashUnsigned(null);
}
);

provider.getSvpFundTxSigned().ifPresent(
svpFundTxSigned -> {
logger.warn("[clearSvpValues] Spend tx was never created. Fund tx hash: {}", svpFundTxSigned.getHash());
provider.setSvpFundTxSigned(null);
}
);

provider.getSvpSpendTxWaitingForSignatures().ifPresent(
svpSpendTxWFS -> {
Keccak256 rskCreationHash = svpSpendTxWFS.getKey();
BtcTransaction svpSpendTx = svpSpendTxWFS.getValue();

logger.warn("[clearSvpValues] Spend tx {} was not fully signed. Rsk creation hash: {}.",
svpSpendTx.getHash(), rskCreationHash);
provider.setSvpSpendTxWaitingForSignatures(null);
provider.setSvpSpendTxHashUnsigned(null);
}
);

provider.getSvpSpendTxHashUnsigned().ifPresent(
svpSpendTxHashUnsigned -> {
logger.warn("[clearSvpValues] Spend tx {} was not registered.", svpSpendTxHashUnsigned);
provider.setSvpSpendTxHashUnsigned(null);
}
);
private void clearSvpValues() {
provider.clearSvpValues();
}

private boolean svpIsOngoing() {
Expand Down
70 changes: 70 additions & 0 deletions rskj-core/src/test/java/co/rsk/peg/BridgeStorageProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1162,6 +1162,76 @@ void getSvpSpendTxWaitingForSignatures_whenEntryIsCached_shouldReturnTheCachedEn
}
}

@Nested
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
@Tag("clear svp values tests")
class ClearSvpValuesTests {
private BridgeStorageProvider bridgeStorageProvider;

@BeforeEach
void setup() {
Repository repository = createRepository();
bridgeStorageProvider = createBridgeStorageProvider(repository, mainnetBtcParams, activationsAllForks);
}

@Test
void clearSvpValues_whenFundTxHashUnsigned_shouldClearValue() {
// arrange
Sha256Hash svpFundTxHashUnsigned = BitcoinTestUtils.createHash(1);
bridgeStorageProvider.setSvpFundTxHashUnsigned(svpFundTxHashUnsigned);

// act
bridgeStorageProvider.clearSvpValues();

// assert
assertFalse(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent());
Copy link
Contributor

Choose a reason for hiding this comment

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

I would assert that no svp value is present, in all tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as in BridgeSupportSvpTest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

}

@Test
void clearSvpValues_whenFundTxSigned_shouldClearValue() {
// arrange
BtcTransaction svpFundTxSigned = new BtcTransaction(mainnetBtcParams);
bridgeStorageProvider.setSvpFundTxSigned(svpFundTxSigned);

// act
bridgeStorageProvider.clearSvpValues();

// assert
assertFalse(bridgeStorageProvider.getSvpFundTxSigned().isPresent());
}

@Test
void clearSvpValues_whenSpendTxWFS_shouldClearSpendTxValues() {
// arrange
Keccak256 svpSpendTxCreationHash = RskTestUtils.createHash(1);
BtcTransaction svpSpendTx = new BtcTransaction(mainnetBtcParams);
Map.Entry<Keccak256, BtcTransaction> svpSpendTxWFS = new AbstractMap.SimpleEntry<>(svpSpendTxCreationHash, svpSpendTx);
bridgeStorageProvider.setSvpSpendTxWaitingForSignatures(svpSpendTxWFS);

// act
bridgeStorageProvider.clearSvpValues();

// assert
assertFalse(bridgeStorageProvider.getSvpSpendTxWaitingForSignatures().isPresent());
assertFalse(bridgeStorageProvider.getSvpSpendTxHashUnsigned().isPresent());
}

@Test
void clearSvpValues_whenSpendTxHashUnsigned_shouldClearValue() {
// arrange
Keccak256 svpSpendTxCreationHash = RskTestUtils.createHash(1);
BtcTransaction svpSpendTx = new BtcTransaction(mainnetBtcParams);
Map.Entry<Keccak256, BtcTransaction> svpSpendTxWFS = new AbstractMap.SimpleEntry<>(svpSpendTxCreationHash, svpSpendTx);
bridgeStorageProvider.setSvpSpendTxWaitingForSignatures(svpSpendTxWFS);

// act
bridgeStorageProvider.clearSvpValues();

// assert
assertFalse(bridgeStorageProvider.getSvpSpendTxHashUnsigned().isPresent());
}
}

@Test
void getReleaseRequestQueue_before_rskip_146_activation() throws IOException {
Repository repositoryMock = mock(Repository.class);
Expand Down
41 changes: 23 additions & 18 deletions rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,8 @@ void setUp() {

@Nested
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
@Tag("Validation failure tests")
class ValidationFailureTests {
@Tag("SVP failure tests")
class SVPFailureTests {
@BeforeEach
void setUp() {
logs = new ArrayList<>();
Expand Down Expand Up @@ -158,45 +158,44 @@ void processValidationFailure_whenSvpFundTxHashUnsigned_shouldLogValidationFailu
bridgeStorageProvider.setSvpFundTxHashUnsigned(svpFundTxHashUnsigned);

// act
bridgeSupport.processValidationFailure(proposedFederation);
bridgeSupport.processSVPFailure(proposedFederation);

// assert
assertLogCommitFederationFailed();
assertProposedFederationWasCleared();
assertFalse(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent());
assertNoProposedFederation();
assertNoSVPValues();
}

@Test
void processValidationFailure_whenSvpFundTxSigned_shouldLogValidationFailureAndClearValue() {
// arrange
BtcTransaction svpFundTx = mock(BtcTransaction.class);
BtcTransaction svpFundTx = new BtcTransaction(btcMainnetParams);
bridgeStorageProvider.setSvpFundTxSigned(svpFundTx);

// act
bridgeSupport.processValidationFailure(proposedFederation);
bridgeSupport.processSVPFailure(proposedFederation);

// assert
assertLogCommitFederationFailed();
assertProposedFederationWasCleared();
assertFalse(bridgeStorageProvider.getSvpFundTxSigned().isPresent());
assertNoProposedFederation();
assertNoSVPValues();
}

@Test
void processValidationFailure_whenSvpSpendTxWFS_shouldLogValidationFailureAndClearSpendTxValues() {
// arrange
Keccak256 svpSpendTxCreationHash = RskTestUtils.createHash(1);
BtcTransaction svpSpendTx = mock(BtcTransaction.class);
BtcTransaction svpSpendTx = new BtcTransaction(btcMainnetParams);
Map.Entry<Keccak256, BtcTransaction> svpSpendTxWFS = new AbstractMap.SimpleEntry<>(svpSpendTxCreationHash, svpSpendTx);
bridgeStorageProvider.setSvpSpendTxWaitingForSignatures(svpSpendTxWFS);

// act
bridgeSupport.processValidationFailure(proposedFederation);
bridgeSupport.processSVPFailure(proposedFederation);

// assert
assertLogCommitFederationFailed();
assertProposedFederationWasCleared();
assertFalse(bridgeStorageProvider.getSvpSpendTxWaitingForSignatures().isPresent());
assertFalse(bridgeStorageProvider.getSvpSpendTxHashUnsigned().isPresent());
assertNoProposedFederation();
assertNoSVPValues();
}

@Test
Expand All @@ -206,12 +205,12 @@ void processValidationFailure_whenSvpSpendTxHashUnsigned_shouldLogValidationFail
bridgeStorageProvider.setSvpSpendTxHashUnsigned(svpSpendTxHash);

// act
bridgeSupport.processValidationFailure(proposedFederation);
bridgeSupport.processSVPFailure(proposedFederation);

// assert
assertLogCommitFederationFailed();
assertProposedFederationWasCleared();
assertFalse(bridgeStorageProvider.getSvpSpendTxHashUnsigned().isPresent());
assertNoProposedFederation();
assertNoSVPValues();
}

private void assertLogCommitFederationFailed() {
Expand All @@ -224,9 +223,15 @@ private void assertLogCommitFederationFailed() {
assertEventWasEmittedWithExpectedData(encodedData);
}

private void assertProposedFederationWasCleared() {
private void assertNoProposedFederation() {
assertFalse(federationSupport.getProposedFederation().isPresent());
}
private void assertNoSVPValues() {
assertFalse(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent());
assertFalse(bridgeStorageProvider.getSvpFundTxSigned().isPresent());
assertFalse(bridgeStorageProvider.getSvpSpendTxWaitingForSignatures().isPresent());
assertFalse(bridgeStorageProvider.getSvpSpendTxHashUnsigned().isPresent());
}
}

@Nested
Expand Down