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

Add svp spend tx signature #2804

Merged
merged 9 commits into from
Oct 16, 2024
Merged

Add svp spend tx signature #2804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c9847d4
Create addSvpSpendTxSignature method. Add tests. Rename method to mak…
julia-zack Oct 14, 2024
f2b740f
Rename method to make it more clear and distinguishable
julia-zack Oct 14, 2024
6c2db8c
Rename local variable to fix sonar complain
julia-zack Oct 14, 2024
f3777a2
Rename to more meaningful or accurate names. Correct logging message.…
julia-zack Oct 16, 2024
02f13c1
Rename rskTxHash
julia-zack Oct 16, 2024
bd17cd4
Refactor to improve readability. Add test for when there are not enou…
julia-zack Oct 16, 2024
0cf1adf
Add missing return statement
julia-zack Oct 16, 2024
0bc0589
Fix sonar complain
julia-zack Oct 16, 2024
2352597
Add missing logs and method name to logs.
julia-zack Oct 16, 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
147 changes: 97 additions & 50 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 @@ -1567,50 +1567,46 @@ private void adjustBalancesIfChangeOutputWasDust(BtcTransaction btcTx, Coin sent
* The hash for the signature must be calculated with Transaction.SigHash.ALL and anyoneCanPay=false. The signature must be canonical.
* If enough signatures were added, ask federators to broadcast the btc release tx.
*
* @param federatorBtcPublicKey Federator who is signing
* @param signatures 1 signature per btc tx input
* @param rskTxHash The hash of the rsk tx
* @param federatorBtcPublicKey Federator who is signing
* @param signatures 1 signature per btc tx input
* @param releaseCreationRskTxHash The hash of the release creation rsk tx
*/
public void addSignature(BtcECKey federatorBtcPublicKey, List<byte[]> signatures, Keccak256 rskTxHash) throws IOException {
public void addSignature(BtcECKey federatorBtcPublicKey, List<byte[]> signatures, Keccak256 releaseCreationRskTxHash) throws IOException {
if (signatures == null || signatures.isEmpty()) {
return;
}

Context.propagate(btcContext);

BtcTransaction releaseTx = provider.getPegoutsWaitingForSignatures().get(rskTxHash);
if (releaseTx == null) {
logger.warn("No tx waiting for signature for hash {}. Probably fully signed already.", rskTxHash);
return;
}
if (!hasEnoughSignatures(releaseTx, signatures)) {
if (svpIsOngoing() && isSvpSpendTx(releaseCreationRskTxHash)) {
logger.trace("[addSignature] Going to sign svp spend transaction with federator public key {}", federatorBtcPublicKey);
addSvpSpendTxSignatures(federatorBtcPublicKey, signatures);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe log going to sign svp spend tx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my badddd, deleted it when refactored

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

return;
}

addReleaseSignatures(federatorBtcPublicKey, signatures, rskTxHash, releaseTx);
}

private boolean hasEnoughSignatures(BtcTransaction releaseTx, List<byte[]> signatures) {
int inputsSize = releaseTx.getInputs().size();
int signaturesSize = signatures.size();

if (inputsSize != signaturesSize) {
logger.warn("Expected {} signatures but received {}.", inputsSize, signaturesSize);
return false;
}
return true;
logger.trace("[addSignature] Going to sign release transaction with federator public key {}", federatorBtcPublicKey);
addReleaseSignatures(federatorBtcPublicKey, signatures, releaseCreationRskTxHash);
}

private void addReleaseSignatures(
BtcECKey federatorPublicKey,
List<byte[]> signatures,
Keccak256 rskTxHash,
BtcTransaction btcTx
Keccak256 releaseCreationRskTxHash
) throws IOException {

BtcTransaction releaseTx = provider.getPegoutsWaitingForSignatures().get(releaseCreationRskTxHash);
if (releaseTx == null) {
logger.warn("[addReleaseSignatures] No tx waiting for signature for hash {}. Probably fully signed already.", releaseCreationRskTxHash);
return;
}
if (!areSignaturesEnoughToSignAllTxInputs(releaseTx, signatures)) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe log this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is being logged :)

rskj/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java

Line 1699 in 0bc0589

logger.warn("Expected {} signatures but received {}.", inputsSize, signaturesSize);

}

Optional<Federation> optionalFederation = getFederationFromPublicKey(federatorPublicKey);
if (optionalFederation.isEmpty()) {
logger.warn(
"[addSignature] Supplied federator btc public key {} does not belong to any of the federators.",
"[addReleaseSignatures] Supplied federator btc public key {} does not belong to any of the federators.",
federatorPublicKey
);
return;
Expand All @@ -1620,27 +1616,27 @@ private void addReleaseSignatures(
Optional<FederationMember> federationMember = federation.getMemberByBtcPublicKey(federatorPublicKey);
if (federationMember.isEmpty()){
logger.warn(
"[addSignature] Supplied federator btc public key {} doest not match any of the federator member btc public keys {}.",
"[addReleaseSignatures] Supplied federator btc public key {} doest not match any of the federator member btc public keys {}.",
federatorPublicKey, federation.getBtcPublicKeys()
);
return;
}
FederationMember signingFederationMember = federationMember.get();

byte[] rskTxHashSerialized = rskTxHash.getBytes();
byte[] releaseCreationRskTxHashSerialized = releaseCreationRskTxHash.getBytes();
if (!activations.isActive(ConsensusRule.RSKIP326)) {
eventLogger.logAddSignature(signingFederationMember, btcTx, rskTxHashSerialized);
eventLogger.logAddSignature(signingFederationMember, releaseTx, releaseCreationRskTxHashSerialized);
}

processSigning(signingFederationMember, signatures, rskTxHash, btcTx);
processSigning(signingFederationMember, signatures, releaseCreationRskTxHash, releaseTx);

if (!BridgeUtils.hasEnoughSignatures(btcContext, btcTx)) {
logMissingSignatures(btcTx, rskTxHash, federation);
if (!BridgeUtils.hasEnoughSignatures(btcContext, releaseTx)) {
logMissingSignatures(releaseTx, releaseCreationRskTxHash, federation);
return;
}

logReleaseBtc(btcTx, rskTxHashSerialized);
provider.getPegoutsWaitingForSignatures().remove(rskTxHash);
logReleaseBtc(releaseTx, releaseCreationRskTxHashSerialized);
provider.getPegoutsWaitingForSignatures().remove(releaseCreationRskTxHash);
}

private Optional<Federation> getFederationFromPublicKey(BtcECKey federatorPublicKey) {
Expand All @@ -1657,24 +1653,75 @@ private Optional<Federation> getFederationFromPublicKey(BtcECKey federatorPublic
return Optional.empty();
}

private void logMissingSignatures(BtcTransaction btcTx, Keccak256 rskTxHash, Federation federation) {
private boolean isSvpSpendTx(Keccak256 releaseCreationRskTxHash) {
return provider.getSvpSpendTxWaitingForSignatures()
.map(Map.Entry::getKey)
.filter(key -> key.equals(releaseCreationRskTxHash))
.isPresent();
}

private void addSvpSpendTxSignatures(
BtcECKey proposedFederatorPublicKey,
List<byte[]> signatures
) {
Federation proposedFederation = federationSupport.getProposedFederation()
// This flow should never be reached. There should always be a proposed federation if svpIsOngoing.
.orElseThrow(() -> new IllegalStateException("Proposed federation must exist when trying to sign the svp spend transaction."));
nathanieliov marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the one below could also be logged, but not sure. Maybe the Bridge catches this exception and logs it? If that's the case then we are probably good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! It does

rskj/rskj-core/src/main/java/co/rsk/peg/Bridge.java

Line 631 in 0bc0589

logger.warn("Exception in addSignature", e);

FederationMember federationMember = proposedFederation.getMemberByBtcPublicKey(proposedFederatorPublicKey)
.orElseThrow(() -> new IllegalStateException("Federator must belong to proposed federation to sign the svp spend transaction."));

provider.getSvpSpendTxWaitingForSignatures()
// The svpSpendTxWFS should always be present at this point, since we already checked isTheSvpSpendTx.
.ifPresent(svpSpendTxWFS -> {

Keccak256 svpSpendTxCreationRskTxHash = svpSpendTxWFS.getKey();
BtcTransaction svpSpendTx = svpSpendTxWFS.getValue();

if (!areSignaturesEnoughToSignAllTxInputs(svpSpendTx, signatures)) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here

}

processSigning(federationMember, signatures, svpSpendTxCreationRskTxHash, svpSpendTx);

if (!BridgeUtils.hasEnoughSignatures(btcContext, svpSpendTx)) {
logMissingSignatures(svpSpendTx, svpSpendTxCreationRskTxHash, proposedFederation);
return;
}

logReleaseBtc(svpSpendTx, svpSpendTxCreationRskTxHash.getBytes());
provider.setSvpSpendTxWaitingForSignatures(null);
});
}

private boolean areSignaturesEnoughToSignAllTxInputs(BtcTransaction releaseTx, List<byte[]> signatures) {
int inputsSize = releaseTx.getInputs().size();
int signaturesSize = signatures.size();

if (inputsSize != signaturesSize) {
logger.warn("[areSignaturesEnoughToSignAllTxInputs] Expected {} signatures but received {}.", inputsSize, signaturesSize);
return false;
}
return true;
}

private void logMissingSignatures(BtcTransaction btcTx, Keccak256 releaseCreationRskTxHash, Federation federation) {
int missingSignatures = BridgeUtils.countMissingSignatures(btcContext, btcTx);
int neededSignatures = federation.getNumberOfSignaturesRequired();
int signaturesCount = neededSignatures - missingSignatures;

logger.debug("Tx {} not yet fully signed. Requires {}/{} signatures but has {}",
rskTxHash, neededSignatures, federation.getSize(), signaturesCount);
logger.debug("[logMissingSignatures] Tx {} not yet fully signed. Requires {}/{} signatures but has {}",
releaseCreationRskTxHash, neededSignatures, federation.getSize(), signaturesCount);
}

private void logReleaseBtc(BtcTransaction btcTx, byte[] rskTxHashSerialized) {
logger.info("Tx fully signed {}. Hex: {}", btcTx, Bytes.of(btcTx.bitcoinSerialize()));
eventLogger.logReleaseBtc(btcTx, rskTxHashSerialized);
private void logReleaseBtc(BtcTransaction btcTx, byte[] releaseCreationRskTxHashSerialized) {
logger.info("[logReleaseBtc] Tx fully signed {}. Hex: {}", btcTx, Bytes.of(btcTx.bitcoinSerialize()));
eventLogger.logReleaseBtc(btcTx, releaseCreationRskTxHashSerialized);
}

private void processSigning(
FederationMember federatorMember,
List<byte[]> signatures,
Keccak256 rskTxHash,
Keccak256 releaseCreationRskTxHash,
BtcTransaction btcTx) {

// Build input hashes for signatures
Expand All @@ -1695,10 +1742,10 @@ private void processSigning(
}

// All signatures are correct. Proceed to signing
boolean signed = sign(federatorBtcPublicKey, txSigs, sigHashes, rskTxHash, btcTx);
boolean signed = sign(federatorBtcPublicKey, txSigs, sigHashes, releaseCreationRskTxHash, btcTx);

if (signed && activations.isActive(ConsensusRule.RSKIP326)) {
eventLogger.logAddSignature(federatorMember, btcTx, rskTxHash.getBytes());
eventLogger.logAddSignature(federatorMember, btcTx, releaseCreationRskTxHash.getBytes());
}
}

Expand All @@ -1712,7 +1759,7 @@ private List<TransactionSignature> getTransactionSignatures(BtcECKey federatorBt

if (!federatorBtcPublicKey.verify(sigHash, decodedSignature)) {
logger.warn(
"Signature {} {} is not valid for hash {} and public key {}",
"[getTransactionSignatures] Signature {} {} is not valid for hash {} and public key {}",
i,
Bytes.of(decodedSignature.encodeToDER()),
sigHash,
Expand All @@ -1723,7 +1770,7 @@ private List<TransactionSignature> getTransactionSignatures(BtcECKey federatorBt

TransactionSignature txSig = new TransactionSignature(decodedSignature, BtcTransaction.SigHash.ALL, false);
if (!txSig.isCanonical()) {
logger.warn("Signature {} {} is not canonical.", i, Bytes.of(decodedSignature.encodeToDER()));
logger.warn("[getTransactionSignatures] Signature {} {} is not canonical.", i, Bytes.of(decodedSignature.encodeToDER()));
throw new SignatureException();
}
txSigs.add(txSig);
Expand All @@ -1738,7 +1785,7 @@ private List<BtcECKey.ECDSASignature> getDecodedSignatures(List<byte[]> signatur
decodedSignatures.add(BtcECKey.ECDSASignature.decodeFromDER(signature));
} catch (RuntimeException e) {
int index = signatures.indexOf(signature);
logger.warn("Malformed signature for input {} : {}", index, Bytes.of(signature));
logger.warn("[getDecodedSignatures] Malformed signature for input {} : {}", index, Bytes.of(signature));
throw new SignatureException();
}
}
Expand All @@ -1749,7 +1796,7 @@ private boolean sign(
BtcECKey federatorBtcPublicKey,
List<TransactionSignature> txSigs,
List<Sha256Hash> sigHashes,
Keccak256 rskTxHash,
Keccak256 releaseCreationRskTxHash,
BtcTransaction btcTx) {

boolean signed = false;
Expand All @@ -1762,7 +1809,7 @@ private boolean sign(
BridgeUtils.isInputSignedByThisFederator(federatorBtcPublicKey, sigHash, input);

if (alreadySignedByThisFederator) {
logger.warn("Input {} of tx {} already signed by this federator.", i, rskTxHash);
logger.warn("[sign] Input {} of tx {} already signed by this federator.", i, releaseCreationRskTxHash);
break;
}

Expand All @@ -1778,14 +1825,14 @@ private boolean sign(

Script inputScriptWithSignature = outputScript.getScriptSigWithSignature(inputScript, txSigs.get(i).encodeToBitcoin(), sigIndex);
input.setScriptSig(inputScriptWithSignature);
logger.debug("Tx input {} for tx {} signed.", i, rskTxHash);
logger.debug("[sign] Tx input {} for tx {} signed.", i, releaseCreationRskTxHash);
signed = true;
} catch (IllegalStateException e) {
Federation retiringFederation = getRetiringFederation();
if (getActiveFederation().hasBtcPublicKey(federatorBtcPublicKey)) {
logger.debug("A member of the active federation is trying to sign a tx of the retiring one");
logger.debug("[sign] A member of the active federation is trying to sign a tx of the retiring one");
} else if (retiringFederation != null && retiringFederation.hasBtcPublicKey(federatorBtcPublicKey)) {
logger.debug("A member of the retiring federation is trying to sign a tx of the active one");
logger.debug("[sign] A member of the retiring federation is trying to sign a tx of the active one");
}
return false;
}
Expand Down
Loading