Refactor to avoid code duplication. Add javadocs to methods and remove comments

Author: julia-zack

src/main/java/co/rsk/bitcoinj/wallet/Wallet.java

@@ -654,25 +654,22 @@ public void completeTx(SendRequest req) throws InsufficientMoneyException {
             if (req.shuffleOutputs)
                 req.tx.shuffleOutputs();
 
-            // Now sign the inputs, thus proving that we are entitled to redeem the connected outputs.
+            // Now sign the legacy inputs, thus proving that we are entitled to redeem the connected outputs.
             if (req.signInputs)
-                signTransaction(req);
+                signLegacyTransaction(req);
 
             // Check virtual size.
             int baseSize = req.tx.unsafeBitcoinSerialize().length;
-            if (req.isSegwitCompatible) {
-                // in segwit-compatible, the scriptSig is a 36-bytes-fixed-size hash,
-                // so we should count its bytes manually.
-                int segwitCompatibleScriptSigSize = 36;
-                baseSize += req.tx.getInputs().size() * segwitCompatibleScriptSigSize;
-            }
             int totalSize = baseSize;
+            // if the tx was signed, there's nothing else to consider
             if (!req.signInputs) {
-                if (!req.isSegwitCompatible) {
-                    baseSize += estimateBytesForSigning(bestCoinSelection);
+                if (req.isSegwitCompatible) {
+                    baseSize += calculateSegwitScriptSigSize(req.tx);
                     totalSize = baseSize;
-                } else {
                     totalSize += estimateBytesForSigning(bestCoinSelection);
+                } else {
+                    baseSize += estimateBytesForSigning(bestCoinSelection);
+                    totalSize = baseSize;
                 }
             }
 
@@ -693,12 +690,12 @@ public void completeTx(SendRequest req) throws InsufficientMoneyException {
     }
 
     /**
-     * <p>Given a send request containing transaction, attempts to sign it's inputs. This method expects transaction
+     * <p>Given a send request containing a legacy transaction, attempts to sign it's inputs. This method expects transaction
      * to have all necessary inputs connected or they will be ignored.</p>
      * <p>Actual signing is done by pluggable {@link #signers} and it's not guaranteed that
      * transaction will be complete in the end.</p>
      */
-    public void signTransaction(SendRequest req) {
+    public void signLegacyTransaction(SendRequest req) {
         try {
             BtcTransaction tx = req.tx;
             List<TransactionInput> inputs = tx.getInputs();
@@ -748,7 +745,7 @@ private boolean adjustOutputDownwardsForFee(
         Coin feePerKb,
         boolean ensureMinRequiredFee
     ) {
-        final int size = calculateTxSizeBeforeSigning(tx, isSegwit, coinSelection);
+        final int size = calculateTxSizeForFees(tx, isSegwit, coinSelection);
         Coin fee = feePerKb.multiply(size).divide(1000);
         if (ensureMinRequiredFee && fee.compareTo(BtcTransaction.REFERENCE_DEFAULT_MIN_TX_FEE) < 0)
             fee = BtcTransaction.REFERENCE_DEFAULT_MIN_TX_FEE;
@@ -1024,7 +1021,7 @@ private FeeCalculation calculateFee(
                 checkState(input.getScriptBytes().length == 0);
             }
 
-            int size = calculateTxSizeBeforeSigning(tx, req.isSegwitCompatible, selection);
+            int size = calculateTxSizeForFees(tx, req.isSegwitCompatible, selection);
 
             Coin feePerKb = req.feePerKb;
             if (needAtLeastReferenceFee && feePerKb.compareTo(BtcTransaction.REFERENCE_DEFAULT_MIN_TX_FEE) < 0) {
@@ -1043,26 +1040,51 @@ private FeeCalculation calculateFee(
         return result;
     }
 
-    private int calculateTxSizeBeforeSigning(BtcTransaction tx, boolean isSegwitCompatible, CoinSelection bestCoinSelection) {
+    /**
+     * Calculates the virtual size of a Bitcoin transaction for fee estimation purposes.
+     * When estimating fees, we assume the transaction is not yet signed, so we need to consider
+     * the expected size of the signatures.
+     * - If the transaction is legacy (non-SegWit), signature data is part of the base size,
+     *       and the total size is equal to the base size.
+     * - If the transaction is SegWit-compatible, the scriptSig is a 36-byte fixed-size hash
+     *       located in the input, so its part of the base size. Signatures are located in the
+     *       witness, so they are just part of the total size.
+     * @param tx the unsigned Bitcoin transaction
+     * @param isSegwitCompatible whether the transaction is SegWit-compatible
+     * @param bestCoinSelection the selected UTXOs for the transaction
+     * @return the estimated virtual size of the transaction
+     */
+    private int calculateTxSizeForFees(BtcTransaction tx, boolean isSegwitCompatible, CoinSelection bestCoinSelection) {
         int baseSize = tx.unsafeBitcoinSerialize().length;
-        if (isSegwitCompatible) {
-            // in segwit-compatible, the scriptSig is a 36-bytes-fixed-size hash,
-            // so we should count its bytes manually.
-            int segwitCompatibleScriptSigSize = 36;
-            baseSize += tx.getInputs().size() * segwitCompatibleScriptSigSize;
-        }
-        int totalSize = baseSize;
+        int totalSize;
         if (!isSegwitCompatible) {
             baseSize += estimateBytesForSigning(bestCoinSelection);
             totalSize = baseSize;
         } else {
+            baseSize += calculateSegwitScriptSigSize(tx);
+            totalSize = baseSize;
             totalSize += estimateBytesForSigning(bestCoinSelection);
         }
         return calculateVirtualSize(baseSize, totalSize);
     }
 
+    private int calculateSegwitScriptSigSize(BtcTransaction tx) {
+        int segwitCompatibleScriptSigSize = 36;
+        return tx.getInputs().size() * segwitCompatibleScriptSigSize;
+    }
+
+    /**
+     * Calculates the virtual size of a Bitcoin transaction as defined in BIP141.
+     * The virtual size is a weighted size used to properly account for the discount SegWit
+     * provides on witness data.
+     * - For legacy transactions, {@code baseSize == totalSize}, so the virtual size equals both.
+     * - For SegWit transactions, {@code baseSize} excludes witness data, and {@code totalSize} includes it.
+     *
+     * @param baseSize the size of the transaction excluding witness data
+     * @param totalSize the full size of the transaction including witness data
+     * @return the virtual size in vbytes
+     */
     private int calculateVirtualSize(int baseSize, int totalSize) {
-        // As described in BIP141
         int txWeight = totalSize + (3 * baseSize);
         return txWeight / 4;
     }

src/test/java/co/rsk/bitcoinj/wallet/WalletTest.java

@@ -9,9 +9,7 @@
 import java.nio.charset.StandardCharsets;
 import java.util.*;
 
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.*;
 
 public class WalletTest {
     // data from tx https://mempool.space/testnet/tx/1744459aeaf7369aadc9fc40de9ab2bf575b14e35029b35a7ee4bbd3de65af7f
@@ -123,11 +121,12 @@ public void completeTx_legacy() throws InsufficientMoneyException {
         wallet.completeTx(sr);
 
         // assert
-        double expectedSize = 375;
+        double realSize = 375;
+        int expectedCalculatedSize = 340;
         // for legacy tx, the calculation has a 10% diff approx,
         // but we have to keep the same implementation for backwards compatibility
         double allowedPercentageError = 10.0;
-        assertCalculatedSizeIsCloseToExpectedSize(expectedSize, allowedPercentageError);
+        assertCalculatedSizeIsCloseToExpectedSize(realSize, allowedPercentageError, expectedCalculatedSize);
     }
 
     @Test
@@ -148,10 +147,11 @@ public void completeTx_segwit() throws InsufficientMoneyException {
         wallet.completeTx(sr);
 
         // assert
-        double expectedSize = 183.75;
+        double realSize = 183.75;
+        int expectedCalculatedSize = 184;
         // for segwit, the calculation seems to be pretty accurate
         double allowedPercentageError = 1.0;
-        assertCalculatedSizeIsCloseToExpectedSize(expectedSize, allowedPercentageError);
+        assertCalculatedSizeIsCloseToExpectedSize(realSize, allowedPercentageError, expectedCalculatedSize);
     }
 
     @Test
@@ -176,17 +176,18 @@ public void completeTx_forTxThatExceedsMaximumStandardSize_throwsExceededMaxTran
         assertThrows(Wallet.ExceededMaxTransactionSize.class, () -> wallet.completeTx(sr));
     }
 
-    private void assertCalculatedSizeIsCloseToExpectedSize(double expectedSize, double allowedPercentageError) {
-        double allowedError = Math.ceil(expectedSize * allowedPercentageError / 100);
+    private void assertCalculatedSizeIsCloseToExpectedSize(double realSize, double allowedPercentageError, int expectedCalculatedSize) {
+        double allowedError = Math.ceil(realSize * allowedPercentageError / 100);
 
         Coin availableAmount = AMOUNT_IN_EACH_UTXO.multiply(AMOUNT_OF_UTXOS_TO_USE);
         Coin actualValueSent = tx.getOutputs().get(0).getValue();
         Coin fee = availableAmount.minus(actualValueSent);
         // fee = (tx size * feePerKb) / 1000 => fee = (tx size * 10_000) / 1000
         // => fee = tx size * 10 => tx size = fee / 10
         long size = fee.divide(10L).getValue();
+        assertEquals(expectedCalculatedSize, size); // to make the test deterministic
 
-        double diff = Math.abs(expectedSize - size);
+        double diff = Math.abs(realSize - size);
         assertTrue(diff <= allowedError);
     }
 }