TransactionOutput: make field value immutable

Because tweaking is necessary for fee calculation logic, these usages
have been changed to produce new TransactionOutputs instead and
replace them in transactions as needed.

core/src/main/java/org/bitcoinj/core/Transaction.java

@@ -1011,6 +1011,21 @@ public TransactionOutput addOutput(TransactionOutput to) {
         return to;
     }
 
+    /**
+     * Replaces an already added output. This is meant to amend a transaction before it's committed to a wallet.
+     *
+     * @param index  index of output to replace
+     * @param output output to replace with
+     */
+    public void replaceOutput(int index, TransactionOutput output) {
+        TransactionOutput oldOutput = outputs.remove(index);
+        checkState(oldOutput.isAvailableForSpending(), () ->
+                "output to be replaced is buried in a wallet: " + oldOutput);
+        oldOutput.setParent(null);
+        output.setParent(this);
+        outputs.add(index, output);
+    }
+
     /**
      * Creates an output based on the given address and value, adds it to this transaction, and returns the new output.
      */

core/src/main/java/org/bitcoinj/core/TransactionOutput.java

@@ -57,7 +57,7 @@ public class TransactionOutput {
     @Nullable protected Transaction parent;
 
     // The output's value is kept as a native type in order to save class instances.
-    private long value;
+    private final long value;
 
     // A transaction output has a script used for authenticating that the redeemer is allowed to spend
     // this output.
@@ -169,14 +169,14 @@ public Coin getValue() {
     }
 
     /**
-     * Sets the value of this output.
+     * Returns a clone of this output, with given value. The typical use case is fee calculation.
+     *
+     * @param value value for the clone
+     * @return clone of output, with given value
      */
-    public void setValue(Coin value) {
+    public TransactionOutput withValue(Coin value) {
         Objects.requireNonNull(value);
-        // Negative values obviously make no sense, except for -1 which is used as a sentinel value when calculating
-        // SIGHASH_SINGLE signatures, so unfortunately we have to allow that here.
-        checkArgument(value.signum() >= 0 || value.equals(Coin.NEGATIVE_SATOSHI), () -> "value out of range: " + value);
-        this.value = value.value;
+        return new TransactionOutput(this.parent, value, this.scriptBytes);
     }
 
     /**

core/src/main/java/org/bitcoinj/wallet/Wallet.java

@@ -4439,7 +4439,7 @@ public void completeTx(SendRequest req) throws InsufficientMoneyException, Trans
                 CoinSelector selector = req.coinSelector == null ? coinSelector : req.coinSelector;
                 bestCoinSelection = selector.select((Coin) network.maxMoney(), candidates);
                 candidates = null;  // Selector took ownership and might have changed candidates. Don't access again.
-                req.tx.getOutput(0).setValue(bestCoinSelection.totalValue());
+                req.tx.replaceOutput(0, req.tx.getOutput(0).withValue(bestCoinSelection.totalValue()));
                 log.info("  emptying {}", bestCoinSelection.totalValue().toFriendlyString());
             }
 
@@ -4453,7 +4453,7 @@ public void completeTx(SendRequest req) throws InsufficientMoneyException, Trans
 
             if (updatedOutputValues != null) {
                 for (int i = 0; i < updatedOutputValues.size(); i++) {
-                    req.tx.getOutput(i).setValue(updatedOutputValues.get(i));
+                    req.tx.replaceOutput(i, req.tx.getOutput(i).withValue(updatedOutputValues.get(i)));
                 }
             }
 
@@ -4591,7 +4591,8 @@ private boolean adjustOutputDownwardsForFee(Transaction tx, CoinSelection coinSe
             boolean ensureMinRequiredFee) {
         Coin fee = estimateFees(tx, coinSelection, feePerKb, ensureMinRequiredFee);
         TransactionOutput output = tx.getOutput(0);
-        output.setValue(output.getValue().subtract(fee));
+        output = output.withValue(output.getValue().subtract(fee));
+        tx.replaceOutput(0, output);
         return !output.isDust();
     }
 
@@ -5269,11 +5270,12 @@ private FeeCalculation calculateFee(SendRequest req, Coin value, boolean needAtL
                         ByteBuffer.wrap(req.tx.getOutput(i).serialize()), tx);
                 if (req.recipientsPayFees) {
                     // Subtract fee equally from each selected recipient
-                    output.setValue(output.getValue().subtract(fee.divide(req.tx.getOutputs().size())));
+                    output = output.withValue(output.getValue().subtract(fee.divide(req.tx.getOutputs().size())));
                     // first receiver pays the remainder not divisible by output count
                     if (i == 0) {
-                        output.setValue(
-                                output.getValue().subtract(fee.divideAndRemainder(req.tx.getOutputs().size())[1])); // Subtract fee equally from each selected recipient
+                        // Subtract fee equally from each selected recipient
+                        Coin feeRemainder = fee.divideAndRemainder(req.tx.getOutputs().size())[1];
+                        output = output.withValue(output.getValue().subtract(feeRemainder));
                     }
                     result.updatedOutputValues.add(output.getValue());
                     Coin nonDustValue = output.getMinNonDustValue();
@@ -5304,9 +5306,10 @@ private FeeCalculation calculateFee(SendRequest req, Coin value, boolean needAtL
                     // This would be against the purpose of the all-inclusive feature.
                     // So instead we raise the change and deduct from the first recipient.
                     Coin missingToNotBeDust = changeOutput.getMinNonDustValue().subtract(changeOutput.getValue());
-                    changeOutput.setValue(changeOutput.getValue().add(missingToNotBeDust));
+                    changeOutput = changeOutput.withValue(changeOutput.getValue().add(missingToNotBeDust));
                     TransactionOutput firstOutput = tx.getOutput(0);
-                    firstOutput.setValue(firstOutput.getValue().subtract(missingToNotBeDust));
+                    firstOutput = firstOutput.withValue(firstOutput.getValue().subtract(missingToNotBeDust));
+                    tx.replaceOutput(0, firstOutput);
                     result.updatedOutputValues.set(0, firstOutput.getValue());
                     if (firstOutput.isDust()) {
                         throw new CouldNotAdjustDownwards();

core/src/test/java/org/bitcoinj/core/TransactionTest.java

@@ -116,15 +116,15 @@ public void duplicateOutPoint() {
     @Test(expected = VerificationException.NegativeValueOutput.class)
     public void negativeOutput() {
         Transaction tx = FakeTxBuilder.createFakeTx(TESTNET.network());
-        tx.getOutput(0).setValue(Coin.NEGATIVE_SATOSHI);
+        tx.replaceOutput(0, tx.getOutput(0).withValue(Coin.NEGATIVE_SATOSHI));
         Transaction.verify(TESTNET.network(), tx);
     }
 
     @Test(expected = VerificationException.ExcessiveValue.class)
     public void exceedsMaxMoney2() {
         Transaction tx = FakeTxBuilder.createFakeTx(TESTNET.network());
         Coin half = BitcoinNetwork.MAX_MONEY.divide(2).add(Coin.SATOSHI);
-        tx.getOutput(0).setValue(half);
+        tx.replaceOutput(0, tx.getOutput(0).withValue(half));
         tx.addOutput(half, ADDRESS);
         Transaction.verify(TESTNET.network(), tx);
     }