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.

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

Use empty data Incremental Exporter when gvr does not exist #574

Merged
merged 12 commits into from
May 26, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.hamcrest.CoreMatchers.is;
import static tech.pegasys.web3signer.tests.keymanager.SlashingProtectionDataChoice.WITHOUT_SLASHING_PROTECTION_DATA;
import static tech.pegasys.web3signer.tests.keymanager.SlashingProtectionDataChoice.WITH_SLASHING_PROTECTION_DATA;

import tech.pegasys.web3signer.core.service.http.ArtifactType;
import tech.pegasys.web3signer.core.service.http.handlers.keymanager.delete.DeleteKeystoresRequestBody;
Expand All @@ -35,7 +37,6 @@
import org.junit.jupiter.api.Test;

public class DeleteKeystoresAcceptanceTest extends KeyManagerTestBase {

private static final String BLS_PRIVATE_KEY_1 =
"3ee2224386c82ffea477e2adf28a2929f5c349165a4196158c7f3a2ecca40f35";

Expand Down Expand Up @@ -68,7 +69,7 @@ public class DeleteKeystoresAcceptanceTest extends KeyManagerTestBase {

@Test
public void invalidRequestBodyReturnsError() throws URISyntaxException {
setupSignerWithKeyManagerApi(true);
setupSignerWithKeyManagerApi(WITH_SLASHING_PROTECTION_DATA);
final Response response = callDeleteKeystores("{\"invalid\": \"json body\"}");
response.then().assertThat().statusCode(400);
}
Expand All @@ -79,7 +80,7 @@ public void deletingExistingKeyWithNoSlashingProtectionDataTwiceReturnsNotFound(
final String pubKey =
"0xa46bf94016af71e55ca0518fe6a8bd3852e01b3f959780a4faf3bbe461ac553c0a83f232cc5f2a4b827d8d3455b706e4";
createBlsKey("eth2/bls_keystore_2.json", "otherpassword");
setupSignerWithKeyManagerApi(true);
setupSignerWithKeyManagerApi(WITH_SLASHING_PROTECTION_DATA);
callDeleteKeystores(composeRequestBody(pubKey))
.then()
.contentType(ContentType.JSON)
Expand All @@ -103,7 +104,7 @@ public void deletingExistingKeyWithNoSlashingProtectionDataTwiceReturnsNotFound(
@Test
public void deletingExistingKeyReturnDeleted() throws URISyntaxException {
createBlsKey("eth2/bls_keystore.json", "somepassword");
setupSignerWithKeyManagerApi(true);
setupSignerWithKeyManagerApi(WITH_SLASHING_PROTECTION_DATA);
callDeleteKeystores(composeRequestBody())
.then()
.contentType(ContentType.JSON)
Expand All @@ -117,7 +118,7 @@ public void deletingExistingKeyReturnDeleted() throws URISyntaxException {
@Test
public void deletingExistingTwiceReturnsNotActive() throws URISyntaxException {
createBlsKey("eth2/bls_keystore.json", "somepassword");
setupSignerWithKeyManagerApi(true);
setupSignerWithKeyManagerApi(WITH_SLASHING_PROTECTION_DATA);
callDeleteKeystores(composeRequestBody())
.then()
.contentType(ContentType.JSON)
Expand All @@ -143,7 +144,7 @@ public void deletingExistingTwiceReturnsNotActive() throws URISyntaxException {
public void deletingRemovesSignerFromActiveSigners() throws URISyntaxException {
final String firstPubkey = createBlsKey("eth2/bls_keystore.json", "somepassword");
final String secondPubKey = createBlsKey("eth2/bls_keystore_2.json", "otherpassword");
setupSignerWithKeyManagerApi(true);
setupSignerWithKeyManagerApi(WITH_SLASHING_PROTECTION_DATA);

callListKeys()
.then()
Expand Down Expand Up @@ -176,7 +177,7 @@ public void deletingRemovesSignerFromActiveSigners() throws URISyntaxException {
@Test
public void deletingReadOnlyKeyReturnError() throws URISyntaxException {
final String readOnlyPubkey = createRawPrivateKeyFile(BLS_PRIVATE_KEY_1);
setupSignerWithKeyManagerApi(true);
setupSignerWithKeyManagerApi(WITH_SLASHING_PROTECTION_DATA);
callDeleteKeystores(composeRequestBody(readOnlyPubkey))
.then()
.contentType(ContentType.JSON)
Expand All @@ -192,7 +193,7 @@ public void deletingDisablesSigningForAllWeb3Signers()
throws URISyntaxException, JsonProcessingException {
final String firstPubkey = createBlsKey("eth2/bls_keystore.json", "somepassword");
final String secondPubKey = createBlsKey("eth2/bls_keystore_2.json", "otherpassword");
setupSignerWithKeyManagerApi(true);
setupSignerWithKeyManagerApi(WITH_SLASHING_PROTECTION_DATA);

final SignerConfiguration signer2Configuration =
new SignerConfigurationBuilder()
Expand Down Expand Up @@ -257,6 +258,23 @@ public void testRequestBodyParsing() throws IOException {
"0x98d083489b3b06b8740da2dfec5cc3c01b2086363fe023a9d7dc1f907633b1ff11f7b99b19e0533e969862270061d884");
}

@Test
public void deletingExistingKeyWithNoSlashingProtectionReturnDeleted() throws URISyntaxException {

createBlsKey("eth2/bls_keystore.json", "somepassword");

setupSignerWithKeyManagerApi(WITHOUT_SLASHING_PROTECTION_DATA);

callDeleteKeystores(composeRequestBody())
.then()
.contentType(ContentType.JSON)
.assertThat()
.statusCode(200)
.body("data[0].status", is("deleted"))
.and()
.body("slashing_protection", is(""));
}

private String composeRequestBody() {
return composeRequestBody(
"0x98d083489b3b06b8740da2dfec5cc3c01b2086363fe023a9d7dc1f907633b1ff11f7b99b19e0533e969862270061d884");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static tech.pegasys.web3signer.dsl.utils.WaitUtils.waitFor;
import static tech.pegasys.web3signer.signing.KeyType.BLS;
import static tech.pegasys.web3signer.tests.keymanager.SlashingProtectionDataChoice.WITHOUT_SLASHING_PROTECTION_DATA;

import tech.pegasys.signers.bls.keystore.KeyStore;
import tech.pegasys.signers.bls.keystore.KeyStoreLoader;
Expand Down Expand Up @@ -53,11 +54,11 @@ public class KeyManagerTestBase extends AcceptanceTestBase {
@TempDir protected Path testDirectory;

protected void setupSignerWithKeyManagerApi() throws URISyntaxException {
setupSignerWithKeyManagerApi(false);
setupSignerWithKeyManagerApi(WITHOUT_SLASHING_PROTECTION_DATA);
}

protected void setupSignerWithKeyManagerApi(final boolean insertSlashingProtectionData)
throws URISyntaxException {
protected void setupSignerWithKeyManagerApi(
final SlashingProtectionDataChoice slashingProtectionDataChoice) throws URISyntaxException {
final SignerConfigurationBuilder builder = new SignerConfigurationBuilder();
builder
.withKeyStoreDirectory(testDirectory)
Expand All @@ -70,22 +71,24 @@ protected void setupSignerWithKeyManagerApi(final boolean insertSlashingProtecti
.withKeyManagerApiEnabled(true);
startSigner(builder.build());

if (insertSlashingProtectionData) {
final SignerConfigurationBuilder importBuilder = new SignerConfigurationBuilder();
importBuilder
.withMode("eth2")
.withSlashingEnabled(true)
.withSlashingProtectionDbUrl(signer.getSlashingDbUrl())
.withSlashingProtectionDbUsername(DB_USERNAME)
.withSlashingProtectionDbPassword(DB_PASSWORD)
.withKeyStoreDirectory(testDirectory)
.withSlashingImportPath(getResourcePath("slashing/slashingImport_two_entries.json"))
.withHttpPort(12345); // prevent wait for Ports file in AT

final Signer importSigner = new Signer(importBuilder.build(), null);
importSigner.start();
waitFor(() -> assertThat(importSigner.isRunning()).isFalse());
if (slashingProtectionDataChoice == WITHOUT_SLASHING_PROTECTION_DATA) {
return;
}

final SignerConfigurationBuilder importBuilder = new SignerConfigurationBuilder();
importBuilder
.withMode("eth2")
.withSlashingEnabled(true)
.withSlashingProtectionDbUrl(signer.getSlashingDbUrl())
.withSlashingProtectionDbUsername(DB_USERNAME)
.withSlashingProtectionDbPassword(DB_PASSWORD)
.withKeyStoreDirectory(testDirectory)
.withSlashingImportPath(getResourcePath("slashing/slashingImport_two_entries.json"))
.withHttpPort(12345); // prevent wait for Ports file in AT

final Signer importSigner = new Signer(importBuilder.build(), null);
importSigner.start();
waitFor(() -> assertThat(importSigner.isRunning()).isFalse());
}

public Response callListKeys() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright 2022 ConsenSys AG.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*/
package tech.pegasys.web3signer.tests.keymanager;

public enum SlashingProtectionDataChoice {
WITH_SLASHING_PROTECTION_DATA,
WITHOUT_SLASHING_PROTECTION_DATA
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import tech.pegasys.web3signer.signing.util.IdentifierUtils;
import tech.pegasys.web3signer.slashingprotection.SlashingProtection;
import tech.pegasys.web3signer.slashingprotection.interchange.IncrementalExporter;
import tech.pegasys.web3signer.slashingprotection.interchange.NoOpIncrementalExporter;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
Expand Down Expand Up @@ -97,7 +98,7 @@ private IncrementalExporter createIncrementalExporter(final ByteArrayOutputStrea
return slashingProtection.isPresent()
? slashingProtection.get().createIncrementalExporter(outputStream)
// Using no-op exporter instead of returning an optional so can use try with for closing
: new NoOpIncrementalExporter();
: new NoOpIncrementalExporter(outputStream);
}

private DeleteKeystoreResult processKeyToDelete(
Expand Down Expand Up @@ -151,15 +152,4 @@ private DeleteKeystoreResult attemptToExportWithSlashingData(
DeleteKeystoreStatus.ERROR, "Error exporting slashing data: " + e.getMessage());
}
}

private static class NoOpIncrementalExporter implements IncrementalExporter {
@Override
public void export(final String publicKey) {}

@Override
public void finalise() {}

@Override
public void close() throws Exception {}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import tech.pegasys.web3signer.slashingprotection.interchange.InterchangeManager;
import tech.pegasys.web3signer.slashingprotection.interchange.InterchangeModule;
import tech.pegasys.web3signer.slashingprotection.interchange.InterchangeV5Manager;
import tech.pegasys.web3signer.slashingprotection.interchange.NoOpIncrementalExporter;
import tech.pegasys.web3signer.slashingprotection.validator.AttestationValidator;
import tech.pegasys.web3signer.slashingprotection.validator.BlockValidator;
import tech.pegasys.web3signer.slashingprotection.validator.GenesisValidatorRootValidator;
Expand Down Expand Up @@ -146,9 +147,13 @@ public void exportDataWithFilter(final OutputStream output, final List<String> p

@Override
public IncrementalExporter createIncrementalExporter(final OutputStream out) {
if (!gvrValidator.genesisValidatorRootExists()) {
return new NoOpIncrementalExporter(out);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might be nice to create the NoOpIncrementalExporter at the same level as abstraction as before, e.g. by exposing genesisValidatorRootExists to the level above.

The reader would also benefit from the existing comment that explains why we use the noop exporter:

// Using no-op exporter instead of returning an optional so can use try with for closing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a comment. However, exposing checkGVR at SlashingProtection interface would add two level of abstraction i.e. DBSlashing.checkGVR -> gvrValidator.checkGVR. T


try {
return interchangeManager.createIncrementalExporter(out);
} catch (IOException e) {
} catch (final IOException e) {
throw new RuntimeException(
"Failed to initialise incremental exporter for slashing protection data", e);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright 2022 ConsenSys AG.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*/
package tech.pegasys.web3signer.slashingprotection.interchange;

import java.io.OutputStream;

public class NoOpIncrementalExporter implements IncrementalExporter {
private final OutputStream outputStream;

public NoOpIncrementalExporter(final OutputStream outputStream) {
this.outputStream = outputStream;
}

@Override
public void export(final String publicKey) {}

@Override
public void finalise() {}

@Override
public void close() throws Exception {
outputStream.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

nice spot!

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ public boolean checkGenesisValidatorsRootAndInsertIfEmpty(Bytes32 genesisValidat
READ_COMMITTED, handle -> validateGvr(handle, genesisValidatorsRoot)));
}

public boolean genesisValidatorRootExists() {
return failsafeExecutor.get(
() ->
jdbi.inTransaction(
READ_COMMITTED,
handle -> metadataDao.findGenesisValidatorsRoot(handle).isPresent()));
}

private boolean validateGvr(final Handle handle, final Bytes32 genesisValidatorsRoot) {
final Optional<Bytes32> dbGvr = metadataDao.findGenesisValidatorsRoot(handle);
final boolean isValidGvr = dbGvr.map(gvr -> gvr.equals(genesisValidatorsRoot)).orElse(true);
Expand Down