Add svp spend tx signature #2804

julia-zack · 2024-10-14T20:16:51Z

For the validation protocol to be successful, we need the spend transaction to be signed and broadcasted by the proposed pegnatories.

Requirements
-Create a new addSvpSpendTxSignature method that would add the signatures from the proposed pegnatories to the svp spend transaction.

This method should also remove the spend transaction from the map once is fully signed, and emit the release_btc event so the federators can proceed with the broadcast.

…e it more clear and distinguishable

julia-zack · 2024-10-14T20:18:31Z

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

            return;
        }

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

-    private boolean hasEnoughSignatures(BtcTransaction releaseTx, List<byte[]> signatures) {
+    private boolean areSignaturesEnoughToSignAllTxInputs(BtcTransaction releaseTx, List<byte[]> signatures) {


this method name was change to be distinguishable from BridgeUtils method thats also being used

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

marcos-iov

Looking nice and clean, well done!

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

marcos-iov · 2024-10-15T18:03:12Z

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

@@ -1578,19 +1578,32 @@ public void addSignature(BtcECKey federatorBtcPublicKey, List<byte[]> signatures

        Context.propagate(btcContext);

+        if (svpIsOngoing()) {


Maybe we could wrap everything in a single svp method?

if (svpIsOngoing()) { addSignatureForSvp(); return; } ... // Normal addSignature logic

To keep simple and focused, while at the same time re-using addSignature method. What do you think?

I agree with @marcos-iov 's proposal. I think separating these two logic into two methods makes much sense.

that way you won't sign a normal pegout when svp is ongoing, right?

i thought about doing something like that with a continue instead of a return, but i discarded it since (correct me if im wrong) it will make the flow continue after signing the spend tx.
Anyways doing that is no big deal, because it will return early enough here

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

nathanieliov · 2024-10-15T18:36:08Z

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

+                    logger.trace("Going to add federator public key {} to svp spend transaction", federatorBtcPublicKey);
+                    BtcTransaction svpSpendTx = svpSpendTxWFS.getValue();
+                    if (!areSignaturesEnoughToSignAllTxInputs(svpSpendTx, signatures)) {
+                        return;
+                    }
+                    addSvpSpendTxSignatures(federatorBtcPublicKey, signatures, rskTxHash, svpSpendTx);
+                });
+        }
+
        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 (!areSignaturesEnoughToSignAllTxInputs(releaseTx, signatures)) {
            return;
        }

        addReleaseSignatures(federatorBtcPublicKey, signatures, rskTxHash, releaseTx);


I think this whole block can be moved into its own method. Wdyt?

not sure, i think it makes more sense this way. If we move this block, we wont be doing much in addSignature method right?

The idea is the method name expresses by itself what you doing in those lines of code and your functional code looks more elegant. See this

Also , see this one:

return Stream.iterate(0, i -> i + 1) .limit(btcTx.getInputs().size()) .map(i -> generateSigHashForP2SHTransactionInput(btcTx, i)) .collect(Collectors.toList());

The method generateSigHashForP2SHTransactionInput abstracts the block of code on how to generate the sig hash. You can do the same thing by abstracting how the signatures are added.

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

rskj-core/src/test/java/co/rsk/peg/bitcoin/BitcoinTestUtils.java

nathanieliov · 2024-10-15T19:37:04Z

rskj-core/src/test/java/co/rsk/peg/bitcoin/BitcoinTestUtils.java

+        return sigHashes;
+    }
+
+    public static List<byte[]> generateSignerEncodedSignatures(BtcECKey signingKey, List<Sha256Hash> sigHashes) {


Suggested change

public static List<byte[]> generateSignerEncodedSignatures(BtcECKey signingKey, List<Sha256Hash> sigHashes) {

public static List<byte[]> signInputSigHashes(BtcECKey signingKey, List<Sha256Hash> sigHashes) {

I would prefer this name. Wdyt?

This process(sign a transaction) is called and known as "signing" not generating.

but im not just signing, i want the list of the encoded signatures.

rskj-core/src/test/java/co/rsk/peg/bitcoin/BitcoinTestUtils.java

rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java

nathanieliov · 2024-10-15T19:50:03Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java

+            // act & assert
+            for (BtcECKey key : wrongKeys) {
+                List<byte[]> signerEncodedSignatures = generateSignerEncodedSignatures(key, svpSpendTxSigHashes);
+                assertThrows(IllegalStateException.class,


Please assert the exception message as well.

rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java

… Add explaining comment. Make code more functional

…gh signatures.

marcos-iov · 2024-10-16T18:07:50Z

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

+
+        BtcTransaction releaseTx = provider.getPegoutsWaitingForSignatures().get(releaseCreationRskTxHash);
+        if (releaseTx == null) {
+            logger.warn("No tx waiting for signature for hash {}. Probably fully signed already.", releaseCreationRskTxHash);


method name missing in the log :)

marcos-iov · 2024-10-16T18:08:01Z

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

+            return;
+        }
+        if (!areSignaturesEnoughToSignAllTxInputs(releaseTx, signatures)) {
+            return;


Maybe log this?

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);

marcos-iov · 2024-10-16T18:09:01Z

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

+                BtcTransaction svpSpendTx = svpSpendTxWFS.getValue();
+
+                if (!areSignaturesEnoughToSignAllTxInputs(svpSpendTx, signatures)) {
+                    return;


marcos-iov · 2024-10-16T18:09:37Z

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

-        }
-        if (!hasEnoughSignatures(releaseTx, signatures)) {
+        if (svpIsOngoing() && isSvpSpendTx(releaseCreationRskTxHash)) {
+            addSvpSpendTxSignatures(federatorBtcPublicKey, signatures);


Should we maybe log going to sign svp spend tx?

my badddd, deleted it when refactored

marcos-iov · 2024-10-16T18:10:24Z

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

+    ) {
+        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."));


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

yes! It does

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

Line 631 in 0bc0589

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

marcos-iov · 2024-10-16T18:11:14Z

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

+        int signaturesSize = signatures.size();
+
+        if (inputsSize != signaturesSize) {
+            logger.warn("Expected {} signatures but received {}.", inputsSize, signaturesSize);


method name :)

marcos-iov · 2024-10-16T18:11:40Z

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

@@ -1762,7 +1807,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("Input {} of tx {} already signed by this federator.", i, releaseCreationRskTxHash);


marcos-iov · 2024-10-16T18:11:45Z

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

@@ -1778,7 +1823,7 @@ 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("Tx input {} for tx {} signed.", i, releaseCreationRskTxHash);


sonarqubecloud · 2024-10-16T18:38:50Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
97.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

julia-zack added 2 commits October 14, 2024 17:14

Create addSvpSpendTxSignature method. Add tests. Rename method to mak…

c9847d4

…e it more clear and distinguishable

Rename method to make it more clear and distinguishable

f2b740f

julia-zack commented Oct 14, 2024

View reviewed changes

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

julia-zack marked this pull request as ready for review October 14, 2024 20:25

julia-zack requested a review from a team as a code owner October 14, 2024 20:25

Rename local variable to fix sonar complain

6c2db8c

marcos-iov reviewed Oct 15, 2024

View reviewed changes

nathanieliov reviewed Oct 15, 2024

View reviewed changes

rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java Outdated Show resolved Hide resolved

nathanieliov reviewed Oct 15, 2024

View reviewed changes

julia-zack added 5 commits October 16, 2024 11:08

Rename to more meaningful or accurate names. Correct logging message.…

f3777a2

… Add explaining comment. Make code more functional

Rename rskTxHash

02f13c1

Refactor to improve readability. Add test for when there are not enou…

bd17cd4

…gh signatures.

Add missing return statement

0cf1adf

Fix sonar complain

0bc0589

marcos-iov reviewed Oct 16, 2024

View reviewed changes

Add missing logs and method name to logs.

2352597

marcos-iov approved these changes Oct 16, 2024

View reviewed changes

marcos-iov merged commit 4ab88bb into feature/powpeg_validation_protocol-phase3 Oct 16, 2024
7 checks passed

marcos-iov deleted the add_spend_tx_signature branch October 16, 2024 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add svp spend tx signature #2804

Add svp spend tx signature #2804

julia-zack commented Oct 14, 2024

julia-zack Oct 14, 2024 •

edited

Loading

marcos-iov left a comment

marcos-iov Oct 15, 2024

nathanieliov Oct 15, 2024

julia-zack Oct 16, 2024 •

edited

Loading

julia-zack Oct 16, 2024 •

edited

Loading

nathanieliov Oct 15, 2024

julia-zack Oct 16, 2024

nathanieliov Oct 16, 2024

nathanieliov Oct 16, 2024

nathanieliov Oct 15, 2024

julia-zack Oct 16, 2024

nathanieliov Oct 16, 2024

julia-zack Oct 16, 2024

nathanieliov Oct 15, 2024

marcos-iov Oct 16, 2024

julia-zack Oct 16, 2024

marcos-iov Oct 16, 2024

julia-zack Oct 16, 2024

marcos-iov Oct 16, 2024

julia-zack Oct 16, 2024

marcos-iov Oct 16, 2024

julia-zack Oct 16, 2024

julia-zack Oct 16, 2024

marcos-iov Oct 16, 2024

julia-zack Oct 16, 2024

marcos-iov Oct 16, 2024

julia-zack Oct 16, 2024

marcos-iov Oct 16, 2024

julia-zack Oct 16, 2024

marcos-iov Oct 16, 2024

julia-zack Oct 16, 2024

sonarqubecloud bot commented Oct 16, 2024

		@@ -1578,19 +1578,32 @@ public void addSignature(BtcECKey federatorBtcPublicKey, List<byte[]> signatures

		Context.propagate(btcContext);

		if (svpIsOngoing()) {

	public static List<byte[]> generateSignerEncodedSignatures(BtcECKey signingKey, List<Sha256Hash> sigHashes) {
	public static List<byte[]> signInputSigHashes(BtcECKey signingKey, List<Sha256Hash> sigHashes) {

Add svp spend tx signature #2804

Add svp spend tx signature #2804

Conversation

julia-zack commented Oct 14, 2024

julia-zack Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

marcos-iov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julia-zack Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

julia-zack Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 16, 2024

Quality Gate passed

julia-zack Oct 14, 2024 •

edited

Loading

julia-zack Oct 16, 2024 •

edited

Loading

julia-zack Oct 16, 2024 •

edited

Loading