Remove signatures from svp spend tx before searching it while running HSM #390
Conversation
src/main/java/co/rsk/federate/btcreleaseclient/BtcReleaseClient.java
Outdated
Show resolved
Hide resolved
src/main/java/co/rsk/federate/btcreleaseclient/BtcReleaseClient.java
Outdated
Show resolved
Hide resolved
apancorb
changed the base branch from
lovell-700-rc-alphanet
to
LOVELL-7.0.0-rc
src/main/java/co/rsk/federate/btcreleaseclient/BtcReleaseClient.java
Outdated
Show resolved
Hide resolved
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
apancorb
force-pushed
the
fix/remove-sigs-svp-spend
branch
from
731329b to
a368f73
Compare
| * @return {@code true} if the transaction has the required number of confirmations and is ready to be signed; | ||
| * {@code false} otherwise | ||
| */ | ||
| private boolean isSVPSpendTxReadyToSign(long currentBlockNumber, Map.Entry<Keccak256, BtcTransaction> svpSpendTx) { | ||
| try { | ||
|
|
||
| BtcTransaction svpSpendTxWithoutSignatures = svpSpendTx.getValue(); |
There was a problem hiding this comment.
I wouldn't call it that way here, it still has the signatures
There was a problem hiding this comment.
I renamed it back to btcTx.
| Federation spendingFed = getSpendingFederation(svpSpendTxWithoutSignatures); | ||
|
|
||
| logger.debug("[isSvpSpendTxReadyToSign] SVP spend tx before removing signatures [{}]", svpSpendTxWithoutSignatures); | ||
| removeSignaturesFromTransaction(svpSpendTxWithoutSignatures, spendingFed); |
There was a problem hiding this comment.
Could make use of BitcoinUtils:: removeSignaturesFromTransactionWithP2shMultiSigInputs
There was a problem hiding this comment.
Agree. Maybe we should deprecate co.rsk.federate.btcreleaseclient.BtcReleaseClient#removeSignaturesFromTransaction and suggest use removeSignaturesFromTransactionWithP2shMultiSigInputs in the comment.
Btw, removeSignaturesFromTransactionWithP2shMultiSigInputs throws RuntimeExceptions. Should we catch them if so? I think we should, if not, it could result in pegouts not being able to be signed.
There was a problem hiding this comment.
Let's not change more than we need right now, just fix the problem and move on with Lovell. Future refactors can come later.
By the way, instead of removeSignaturesFromTransactionWithP2shMultiSigInputs we should actually use getMultiSigTransactionHashWithoutSignatures that makes a copy of the transaction and returns it without signatures. That makes it a lot clearer
| BtcTransaction svpSpendTxWithoutSignatures = svpSpendTx.getValue(); | ||
| Federation spendingFed = getSpendingFederation(svpSpendTxWithoutSignatures); | ||
|
|
||
| logger.debug("[isSvpSpendTxReadyToSign] SVP spend tx before removing signatures [{}]", svpSpendTxWithoutSignatures); |
There was a problem hiding this comment.
Probably want to log just the hash of the transaction
There was a problem hiding this comment.
|
|
||
| logger.debug("[isSvpSpendTxReadyToSign] SVP spend tx before removing signatures [{}]", svpSpendTxWithoutSignatures); | ||
| removeSignaturesFromTransaction(svpSpendTxWithoutSignatures, spendingFed); | ||
| logger.debug("[isSvpSpendTxReadyToSign] SVP spend tx after removing signatures [{}]", svpSpendTxWithoutSignatures); |
src/main/java/co/rsk/federate/btcreleaseclient/BtcReleaseClient.java
Outdated
Show resolved
Hide resolved
src/test/java/co/rsk/federate/btcreleaseclient/BtcReleaseClientTest.java
Outdated
Show resolved
Hide resolved
src/test/java/co/rsk/federate/btcreleaseclient/BtcReleaseClientTest.java
Outdated
Show resolved
Hide resolved
|
Description
When running with an HSM, the node is unable to find the related event since it is taking account the hash of the transaction with the singnatures on it. It must remove the signatures from the bitcoin transaction before comparing with the tx hash of the event.
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist:
rskj:fix/remove-sigs-svp-spendfed:fix/remove-sigs-svp-spend