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

Use FederationChangeFunctions enum when voting #2897

Merged
merged 3 commits into from
Jan 6, 2025
+36 −47
Merged

Use FederationChangeFunctions enum when voting #2897

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b48d38
Use FederationChangeFunctions enum when voting
julia-zack Dec 18, 2024
f6530c5
Refactor to use new java17 switch
julia-zack Dec 19, 2024
98a1d89
Remove wrong comment. Store call spec public keys arguments in a vari…
julia-zack Jan 6, 2025
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
51 changes: 25 additions & 26 deletions rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,14 +425,16 @@ public void clearProposedFederation() {
@Override
public int voteFederationChange(Transaction tx, ABICallSpec callSpec, SignatureCache signatureCache, BridgeEventLogger eventLogger) {
String calledFunction = callSpec.getFunction();
// Must be on one of the allowed functions
if (unknownFederationChangeFunction(calledFunction)) {
// Must be one of the allowed functions
Optional<FederationChangeFunction> federationChangeFunction = Arrays.stream(FederationChangeFunction.values())
.filter(function -> function.getKey().equals(calledFunction))
.findAny();
if (federationChangeFunction.isEmpty()) {
logger.warn("[voteFederationChange] Federation change function \"{}\" does not exist.", StringUtils.trim(calledFunction));
return FederationChangeResponseCode.NON_EXISTING_FUNCTION_CALLED.getCode();
}

AddressBasedAuthorizer authorizer = constants.getFederationChangeAuthorizer();

// Must be authorized to vote (checking for signature)
if (!authorizer.isAuthorized(tx, signatureCache)) {
RskAddress voter = tx.getSender(signatureCache);
Expand All @@ -444,7 +446,7 @@ public int voteFederationChange(Transaction tx, ABICallSpec callSpec, SignatureC
// call would be successful
ABICallVoteResult result;
try {
result = executeVoteFederationChangeFunction(true, callSpec, eventLogger);
result = executeVoteFederationChangeFunction(true, callSpec.getArguments(), federationChangeFunction.get(), eventLogger);
} catch (BridgeIllegalArgumentException e) {
logger.warn("[voteFederationChange] Unexpected federation change vote exception: {}", e.getMessage());
result = new ABICallVoteResult(false, FederationChangeResponseCode.GENERIC_ERROR.getCode());
Expand All @@ -468,7 +470,7 @@ public int voteFederationChange(Transaction tx, ABICallSpec callSpec, SignatureC
if (winnerSpecOptional.isPresent()) {
ABICallSpec winnerSpec = winnerSpecOptional.get();
try {
result = executeVoteFederationChangeFunction(false, winnerSpec, eventLogger);
result = executeVoteFederationChangeFunction(false, winnerSpec.getArguments(), federationChangeFunction.get(), eventLogger);
} catch (BridgeIllegalArgumentException e) {
logger.warn("[voteFederationChange] Unexpected federation change vote exception: {}", e.getMessage());
return FederationChangeResponseCode.GENERIC_ERROR.getCode();
Expand All @@ -481,25 +483,22 @@ public int voteFederationChange(Transaction tx, ABICallSpec callSpec, SignatureC
return (int) result.getResult();
}

private boolean unknownFederationChangeFunction(String calledFunction) {
return Arrays.stream(FederationChangeFunction.values()).noneMatch(fedChangeFunction -> fedChangeFunction.getKey().equals(calledFunction));
}

private ABICallVoteResult executeVoteFederationChangeFunction(boolean dryRun, ABICallSpec callSpec, BridgeEventLogger eventLogger) throws BridgeIllegalArgumentException {
private ABICallVoteResult executeVoteFederationChangeFunction(boolean dryRun, byte[][] callSpecArguments, FederationChangeFunction federationChangeFunction, BridgeEventLogger eventLogger) throws BridgeIllegalArgumentException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of receiving a byte[][] parameter. I think it let's you pass whatever. Wouldn't it be better to keep passing the ABICallSpec obejct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, you right. I'll rollback the change

// Try to do a dry-run and only register the vote if the
// call would be successful
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this comment belongs here, the dryRun is a parameter. It's on voteFederationChange function where the dryRun is first attempted

ABICallVoteResult result;
Integer executionResult;
switch (callSpec.getFunction()) {
case "create":
int executionResult;

switch (federationChangeFunction) {
case CREATE:
executionResult = createPendingFederation(dryRun);
result = new ABICallVoteResult(executionResult == 1, executionResult);
break;
case "add":
if(activations.isActive(RSKIP123)) {
case ADD:
if (activations.isActive(RSKIP123)) {
throw new IllegalStateException("The \"add\" function is disabled.");
}
byte[] publicKeyBytes = callSpec.getArguments()[0];
byte[] publicKeyBytes = callSpecArguments[0];
BtcECKey publicKey;
ECKey publicKeyEc;
try {
Expand All @@ -511,36 +510,36 @@ private ABICallVoteResult executeVoteFederationChangeFunction(boolean dryRun, AB
executionResult = addFederatorPublicKeyMultikey(dryRun, publicKey, publicKeyEc, publicKeyEc);
result = new ABICallVoteResult(executionResult == 1, executionResult);
break;
case "add-multi":
case ADD_MULTI:
BtcECKey btcPublicKey;
ECKey rskPublicKey;
ECKey mstPublicKey;
try {
btcPublicKey = BtcECKey.fromPublicOnly(callSpec.getArguments()[0]);
btcPublicKey = BtcECKey.fromPublicOnly(callSpecArguments[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like callSpecArguments[] is read twice usually, inside the try and then inside the catch. Perhaps we should store in a variable to a avoid possible errors by mixing up the indexes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch!

} catch (Exception e) {
throw new BridgeIllegalArgumentException("BTC public key could not be parsed " + Bytes.of(callSpec.getArguments()[0]), e);
throw new BridgeIllegalArgumentException("BTC public key could not be parsed " + Bytes.of(callSpecArguments[0]), e);
}

try {
rskPublicKey = ECKey.fromPublicOnly(callSpec.getArguments()[1]);
rskPublicKey = ECKey.fromPublicOnly(callSpecArguments[1]);
} catch (Exception e) {
throw new BridgeIllegalArgumentException("RSK public key could not be parsed " + Bytes.of(callSpec.getArguments()[1]), e);
throw new BridgeIllegalArgumentException("RSK public key could not be parsed " + Bytes.of(callSpecArguments[1]), e);
}

try {
mstPublicKey = ECKey.fromPublicOnly(callSpec.getArguments()[2]);
mstPublicKey = ECKey.fromPublicOnly(callSpecArguments[2]);
} catch (Exception e) {
throw new BridgeIllegalArgumentException("MST public key could not be parsed " + Bytes.of(callSpec.getArguments()[2]), e);
throw new BridgeIllegalArgumentException("MST public key could not be parsed " + Bytes.of(callSpecArguments[2]), e);
}
executionResult = addFederatorPublicKeyMultikey(dryRun, btcPublicKey, rskPublicKey, mstPublicKey);
result = new ABICallVoteResult(executionResult == 1, executionResult);
break;
case "commit":
Keccak256 pendingFederationHash = new Keccak256(callSpec.getArguments()[0]);
case COMMIT:
Keccak256 pendingFederationHash = new Keccak256(callSpecArguments[0]);
executionResult = commitFederation(dryRun, pendingFederationHash, eventLogger).getCode();
result = new ABICallVoteResult(executionResult == 1, executionResult);
break;
case "rollback":
case ROLLBACK:
executionResult = rollbackFederation(dryRun);
result = new ABICallVoteResult(executionResult == 1, executionResult);
break;
Expand Down
Loading