Skip to content

Commit dd8e4be

Browse files
marcos-iovmarcos-iov
authored
Merge pull request #412 from rsksmart/get-rid-of-hsm-version-integration
Fix block selection. Remove HSM version 3
2 parents 0116def + 23b76f9 commit dd8e4be

File tree

13 files changed

+243
-155
lines changed

13 files changed

+243
-155
lines changed

‎src/main/java/co/rsk/federate/FedNodeRunner.java‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import co.rsk.NodeRunner;
2323
import co.rsk.bitcoinj.core.BtcECKey;
2424
import co.rsk.core.RskAddress;
25+
import co.rsk.federate.signing.hsm.HSMUnsupportedVersionException;
2526
import co.rsk.federate.signing.hsm.HSMVersion;
2627
import co.rsk.peg.constants.BridgeConstants;
2728
import co.rsk.federate.adapter.ThinConverter;
@@ -162,15 +163,22 @@ private void startBookkeepingServices() throws SignerException, HSMClientExcepti
162163
HSMClientProtocol protocol =
163164
hsmClientProtocolFactory.buildHSMClientProtocolFromConfig(powHsmConfig);
164165

165-
int hsmVersion = protocol.getVersion();
166-
logger.debug("[run] Using HSM version {}", hsmVersion);
166+
int version = protocol.getVersion();
167+
if (!HSMVersion.isValidHSMVersion(version)) {
168+
throw new HSMUnsupportedVersionException("Unsupported HSM version " + version);
169+
}
170+
171+
logger.debug("[run] Using HSM version {}", version);
167172

168-
if (HSMVersion.isPowHSM(hsmVersion)) {
169-
hsmBookkeepingClient = buildBookKeepingClient(
170-
protocol, powHsmConfig);
171-
hsmBookkeepingService = buildBookKeepingService(
172-
hsmBookkeepingClient, powHsmConfig);
173+
// Only start bookkeeping services for PoW HSMs. HSM version 1 doesn't support bookkeeping.
174+
if (!HSMVersion.isPowHSM(version)) {
175+
return;
173176
}
177+
178+
hsmBookkeepingClient = buildBookKeepingClient(
179+
protocol, powHsmConfig);
180+
hsmBookkeepingService = buildBookKeepingService(
181+
hsmBookkeepingClient, powHsmConfig);
174182
}
175183

176184
private void configureFederatorSupport() throws SignerException {

‎src/main/java/co/rsk/federate/signing/ECDSAHSMSigner.java‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@
4646
*/
4747
public class ECDSAHSMSigner implements ECDSASigner {
4848

49-
private static final Logger LOGGER = LoggerFactory.getLogger(ECDSAHSMSigner.class);
49+
private static final Logger logger = LoggerFactory.getLogger(ECDSAHSMSigner.class);
5050

5151
private final HSMSigningClientProvider clientProvider;
5252
private final Map<KeyId, String> keyIdMapping;
@@ -78,7 +78,8 @@ public ECDSASignerCheckResult check() {
7878
try {
7979
ensureHsmClient();
8080
} catch (HSMClientException e) {
81-
String keysIds = keyIdMapping.keySet().stream().map(keyId -> keyId.toString()).collect(Collectors.joining(", "));
81+
logger.error("[check] Unable to create HSM client", e);
82+
String keysIds = keyIdMapping.keySet().stream().map(KeyId::toString).collect(Collectors.joining(", "));
8283
return new ECDSASignerCheckResult(Collections.singletonList("HSM "+ keysIds + " Signer: " + e.getMessage()));
8384
}
8485

@@ -90,7 +91,7 @@ public ECDSASignerCheckResult check() {
9091
client.getPublicKey(mapping.getValue());
9192
} catch (HSMClientException e) {
9293
messages.add(e.getMessage());
93-
LOGGER.error("[check] Unable to retrieve public key", e);
94+
logger.error("[check] Unable to retrieve public key", e);
9495
}
9596
}
9697

@@ -107,10 +108,11 @@ public ECPublicKey getPublicKey(KeyId keyId) throws SignerException {
107108

108109
@Override
109110
public int getVersionForKeyId(KeyId keyId) throws SignerException {
110-
int version = 0;
111111
if (!canSignWith(keyId)) {
112112
throw new SignerException(String.format("Can't find version for this key for the requested signing key: %s", keyId));
113113
}
114+
115+
int version;
114116
try {
115117
ensureHsmClient();
116118
version = client.getVersion();
@@ -130,7 +132,7 @@ public ECKey.ECDSASignature sign(KeyId keyId, SignerMessage message) throws Sign
130132

131133
@Override
132134
public String getVersionString() throws SignerException {
133-
String keysIds = keyIdMapping.keySet().stream().map(keyId -> keyId.toString()).collect(Collectors.joining(", "));
135+
String keysIds = keyIdMapping.keySet().stream().map(KeyId::toString).collect(Collectors.joining(", "));
134136
try {
135137
ensureHsmClient();
136138
return "HSM " + keysIds + " Signer Version:" + client.getVersion();
@@ -158,6 +160,7 @@ private <T> T invokeWithVersionRetry(KeyId keyId, SignerCall<T> call) throws Sig
158160
ensureHsmClient();
159161
return call.invoke(client);
160162
} catch(HSMChangedVersionException e) {
163+
logger.debug("[invokeWithVersionRetry] HSM client version changed, retrying", e);
161164
client = null;
162165
return invokeWithVersionRetry(keyId, call);
163166
} catch (HSMClientException e) {

‎src/main/java/co/rsk/federate/signing/hsm/HSMVersion.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
public enum HSMVersion {
77
V1(1),
88
V2(2),
9-
V3(3),
109
V4(4),
1110
V5(5),
1211
;
@@ -22,14 +21,14 @@ public int getNumber() {
2221
}
2322

2423
public static List<HSMVersion> getPowHSMVersions() {
25-
return Arrays.asList(V2, V3, V4, V5);
24+
return Arrays.asList(V2, V4, V5);
2625
}
2726

2827
public static boolean isPowHSM(int version) {
2928
return getPowHSMVersions().stream().anyMatch(hsmVersion -> hsmVersion.number == version);
3029
}
3130

32-
public static boolean isPowHSM(HSMVersion version){
33-
return getPowHSMVersions().contains(version);
31+
public static boolean isValidHSMVersion(int version) {
32+
return Arrays.stream(values()).anyMatch(hsmVersion -> hsmVersion.number == version);
3433
}
3534
}

‎src/main/java/co/rsk/federate/signing/hsm/advanceblockchain/ConfirmedBlocksProvider.java‎

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import java.util.ArrayList;
77
import java.util.List;
88
import org.ethereum.core.Block;
9+
import org.ethereum.core.BlockHeader;
910
import org.ethereum.db.BlockStore;
1011
import org.slf4j.Logger;
1112
import org.slf4j.LoggerFactory;
@@ -92,24 +93,28 @@ public List<Block> getConfirmedBlocks(Keccak256 startingPoint) {
9293
return confirmedBlocks;
9394
}
9495

95-
private BigInteger getBlockDifficultyToConsider(Block block) {
96-
BigInteger blockDifficulty = block.getDifficulty().asBigInteger();
97-
BigInteger difficultyToConsider = blockDifficulty;
98-
if (hsmVersion >= HSMVersion.V3.getNumber()) {
99-
BigInteger unclesDifficulty = block.getUncleList().stream()
100-
.map(uncle -> uncle.getDifficulty().asBigInteger())
101-
.reduce(BigInteger.ZERO, BigInteger::add);
102-
blockDifficulty = blockDifficulty.add(unclesDifficulty);
103-
difficultyToConsider = difficultyCap.min(blockDifficulty);
104-
}
96+
protected BigInteger getBlockDifficultyToConsider(Block block) {
10597
logger.trace(
106-
"[getBlockDifficultyToConsider] Block {} (height {}), total difficulty {}, considering {}",
107-
block.getHash(),
108-
block.getNumber(),
109-
blockDifficulty,
110-
difficultyToConsider
98+
"[getBlockDifficultyToConsider] Get difficulty for block {} at height {}", block.getHash(), block.getNumber()
11199
);
112100

113-
return difficultyToConsider;
101+
BigInteger blockTotalDifficulty = block.getDifficulty().asBigInteger();
102+
if (hsmVersion < HSMVersion.V4.getNumber()) {
103+
logger.trace("[getBlockDifficultyToConsider] Considering block total difficulty {}", blockTotalDifficulty);
104+
return blockTotalDifficulty;
105+
}
106+
107+
// Considering uncles difficulty and cap
108+
BigInteger blockDifficultyToConsider = difficultyCap.min(blockTotalDifficulty);
109+
BigInteger unclesDifficultyToConsider = block.getUncleList().stream()
110+
.map(uncle -> difficultyCap.min(uncle.getDifficulty().asBigInteger()))
111+
.reduce(BigInteger.ZERO, BigInteger::add);
112+
113+
logger.trace(
114+
"[getBlockDifficultyToConsider] Block difficulty {}, considering {}",
115+
blockTotalDifficulty,
116+
blockDifficultyToConsider
117+
);
118+
return blockDifficultyToConsider.add(unclesDifficultyToConsider);
114119
}
115120
}

‎src/main/java/co/rsk/federate/signing/hsm/advanceblockchain/HsmBookkeepingClientImpl.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ public void advanceBlockchain(List<Block> blocks) throws HSMClientException {
167167
ObjectNode payload = this.hsmClientProtocol.buildCommand(ADVANCE_BLOCKCHAIN.getCommand(), getVersion());
168168
addBlocksToPayload(payload, blockHeaderChunk);
169169

170-
if (getVersion() >= HSMVersion.V3.getNumber()) {
170+
if (getVersion() >= HSMVersion.V4.getNumber()) {
171171
List<String[]> brothers = getBrothers(blockHeaderChunk, message);
172172
addBrothersToPayload(payload, brothers);
173173
}

‎src/test/java/co/rsk/federate/FedNodeRunnerTest.java‎

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
import co.rsk.NodeRunner;
1919
import co.rsk.bitcoinj.core.NetworkParameters;
20+
import co.rsk.federate.signing.hsm.HSMUnsupportedVersionException;
2021
import co.rsk.federate.signing.hsm.HSMVersion;
2122
import co.rsk.peg.constants.BridgeConstants;
2223
import co.rsk.federate.btcreleaseclient.BtcReleaseClient;
@@ -50,6 +51,7 @@
5051
import java.util.Collections;
5152
import java.util.List;
5253
import org.ethereum.config.Constants;
54+
import org.junit.jupiter.api.Assertions;
5355
import org.junit.jupiter.api.BeforeEach;
5456
import org.junit.jupiter.api.Test;
5557
import org.junit.jupiter.api.io.TempDir;
@@ -82,7 +84,7 @@ void setUp() throws IOException, HSMClientException {
8284
when(constants.getBridgeConstants()).thenReturn(bridgeConstants);
8385
when(bridgeConstants.getBtcParamsString()).thenReturn(NetworkParameters.ID_REGTEST);
8486

85-
int hsmVersion = HSMVersion.V3.getNumber();
87+
int hsmVersion = HSMVersion.V4.getNumber();
8688
HSMClientProtocol protocol = mock(HSMClientProtocol.class);
8789
when(protocol.getVersion()).thenReturn(hsmVersion);
8890
HSMClientProtocolFactory hsmClientProtocolFactory = mock(HSMClientProtocolFactory.class);
@@ -189,6 +191,36 @@ void test_with_hsm_v1_config() throws Exception {
189191
assertNull(bookkeepingService);
190192
}
191193

194+
@Test
195+
void hsm_v3_config_shouldFail() throws Exception {
196+
SignerConfigBuilder configBuilder = SignerConfigBuilder.builder()
197+
.withHsmSigner("m/44'/0'/0'/0/0");
198+
SignerConfig btcSignerConfig = configBuilder.build(BTC);
199+
200+
when(fedNodeSystemProperties.signerConfig(BTC.getId())).thenReturn(btcSignerConfig);
201+
HSMClientProtocol protocol = mock(HSMClientProtocol.class);
202+
when(protocol.getVersion()).thenReturn(3);
203+
HSMClientProtocolFactory hsmClientProtocolFactory = mock(HSMClientProtocolFactory.class);
204+
when(hsmClientProtocolFactory.buildHSMClientProtocolFromConfig(any())).thenReturn(protocol);
205+
206+
fedNodeRunner = new FedNodeRunner(
207+
mock(BtcToRskClient.class),
208+
mock(BtcToRskClient.class),
209+
mock(BtcReleaseClient.class),
210+
mock(FederationWatcher.class),
211+
mock(FederatorSupport.class),
212+
mock(FederateLogger.class),
213+
mock(RskLogMonitor.class),
214+
mock(NodeRunner.class),
215+
fedNodeSystemProperties,
216+
hsmClientProtocolFactory,
217+
mock(HSMBookKeepingClientProvider.class),
218+
mock(FedNodeContext.class)
219+
);
220+
221+
Assertions.assertThrows(HSMUnsupportedVersionException.class, fedNodeRunner::run, "Unsupported HSM version 3");
222+
}
223+
192224
@Test
193225
void test_with_hsm_config_without_btc() throws Exception {
194226
SignerConfig rskSignerConfig = getHSMRSKSignerConfig();
@@ -486,12 +518,12 @@ private SignerConfig getHSMBTCSignerConfig(HSMVersion version) throws HSMClientE
486518
new BigInteger("4405500"), 500000L, 1000, 100, true);
487519
}
488520

489-
if (version.getNumber() >= 3) {
521+
if (version.getNumber() >= 4) {
490522
when(hsmBookkeepingClient.getBlockchainParameters()).thenReturn(
491523
new PowHSMBlockchainParameters(
492524
createHash(1).toHexString(),
493525
new BigInteger("4405500"),
494-
NetworkParameters.ID_UNITTESTNET.toString()));
526+
NetworkParameters.ID_UNITTESTNET));
495527
}
496528

497529
return configBuilder.build(PowPegNodeKeyId.BTC);

‎src/test/java/co/rsk/federate/signing/ECDSAHSMSignerTest.java‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -257,18 +257,11 @@ void getVersionForKeyId_HSMClientException() throws HSMClientException {
257257
@Test
258258
void getVersionForKeyId_SignerException() throws HSMClientException {
259259
KeyId key = new KeyId("keyAB");
260-
int version = HSMVersion.V3.getNumber();
261-
when(providerMock.getSigningClient()).thenReturn(clientMock);
262-
when(clientMock.getVersion()).thenReturn( version);
263260

264-
try {
265-
signer.getVersionForKeyId(key);
266-
fail();
267-
} catch (SignerException e) {
268-
assertEquals(e.getMessage(),String.format("Can't find version for this key for the requested signing key: %s", key));
269-
}
261+
when(providerMock.getSigningClient()).thenReturn(clientMock);
270262

271-
verify(clientMock,times(0)).getVersion();
263+
assertThrows(SignerException.class, () -> signer.getVersionForKeyId(key), "No mapped HSM key id found");
264+
verify(clientMock,never()).getVersion();
272265
}
273266

274267
@Test

0 commit comments

Comments
 (0)