TransactionOutput: divorce from Message

It is never sent on its own, so it doesn't need to be a `Message`.

* A static constructur `read()` replaces the native constructor that deserialized
  from a payload.
* A `write()` helper replaces `bitcoinSerializeToStream()`.
* A `serialize()` helper replaces `bitcoinSerialize()`.

This comes with tests.

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

@@ -694,7 +694,7 @@ public class Transaction extends Message {
         int numOutputs = numOutputsVarInt.intValue();
         outputs = new ArrayList<>(Math.min((int) numOutputs, Utils.MAX_INITIAL_ARRAY_LENGTH));
         for (long i = 0; i < numOutputs; i++) {
-            TransactionOutput output = new TransactionOutput(this, payload.slice());
+            TransactionOutput output = TransactionOutput.read(payload.slice(), this);
             outputs.add(output);
             // intentionally read again, due to the slice above
             Buffers.skipBytes(payload, 8); // value
@@ -1521,7 +1521,7 @@ public class Transaction extends Message {
         // txout_count, txouts
         stream.write(VarInt.of(outputs.size()).serialize());
         for (TransactionOutput out : outputs)
-            out.bitcoinSerializeToStream(stream);
+            stream.write(out.serialize());
         // script_witnisses
         if (useSegwit) {
             for (TransactionInput in : inputs)

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

@@ -35,15 +35,13 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import javax.annotation.Nullable;
-import java.io.IOException;
-import java.io.OutputStream;
+import java.nio.BufferOverflowException;
 import java.nio.BufferUnderflowException;
 import java.nio.ByteBuffer;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Objects;
 
-import static org.bitcoinj.base.internal.Preconditions.check;
 import static org.bitcoinj.base.internal.Preconditions.checkArgument;
 import static org.bitcoinj.base.internal.Preconditions.checkState;
 
@@ -53,7 +51,7 @@ import static org.bitcoinj.base.internal.Preconditions.checkState;
  * 
  * <p>Instances of this class are not safe for use by multiple threads.</p>
  */
-public class TransactionOutput extends Message {
+public class TransactionOutput {
     private static final Logger log = LoggerFactory.getLogger(TransactionOutput.class);
 
     @Nullable protected Transaction parent;
@@ -76,15 +74,18 @@ public class TransactionOutput extends Message {
     @Nullable private TransactionInput spentBy;
 
     /**
-     * Deserializes a transaction output message. This is usually part of a transaction message.
+     * Deserialize this transaction output from a given payload.
      *
-     * @param payload Bitcoin protocol formatted byte array containing message content.
-     * @throws ProtocolException
+     * @param payload           payload to deserialize from
+     * @param parentTransaction parent transaction of the output
+     * @return read message
+     * @throws BufferUnderflowException if the read message extends beyond the remaining bytes of the payload
      */
-    public TransactionOutput(@Nullable Transaction parent, ByteBuffer payload) throws ProtocolException {
-        super(payload);
-        setParent(parent);
-        availableForSpending = true;
+    public static TransactionOutput read(ByteBuffer payload, Transaction parentTransaction) throws BufferUnderflowException, ProtocolException {
+        Objects.requireNonNull(parentTransaction);
+        Coin value = Coin.valueOf(ByteUtils.readInt64(payload));
+        byte[] scriptBytes = Buffers.readLengthPrefixedBytes(payload);
+        return new TransactionOutput(parentTransaction, value, scriptBytes);
     }
 
     /**
@@ -111,6 +112,7 @@ public class TransactionOutput extends Message {
         // SIGHASH_SINGLE signatures, so unfortunately we have to allow that here.
         checkArgument(value.signum() >= 0 || value.equals(Coin.NEGATIVE_SATOSHI), () ->
                 "negative values not allowed");
+        Objects.requireNonNull(scriptBytes);
         this.value = value.value;
         this.scriptBytes = scriptBytes;
         setParent(parent);
@@ -124,31 +126,46 @@ public class TransactionOutput extends Message {
         return scriptPubKey;
     }
 
-    @Override
-    protected void parse(ByteBuffer payload) throws BufferUnderflowException, ProtocolException {
-        value = ByteUtils.readInt64(payload);
-        // 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.
-        check(value >= 0 || value == -1, () -> new ProtocolException("value out of range: " + value));
-        scriptBytes = Buffers.readLengthPrefixedBytes(payload);
+    /**
+     * Write this transaction output into the given buffer.
+     *
+     * @param buf buffer to write into
+     * @return the buffer
+     * @throws BufferOverflowException if the output doesn't fit the remaining buffer
+     */
+    public ByteBuffer write(ByteBuffer buf) throws BufferOverflowException {
+        ByteUtils.writeInt64LE(value, buf);
+        Buffers.writeLengthPrefixedBytes(buf, scriptBytes);
+        return buf;
     }
 
-    @Override
+    /**
+     * Allocates a byte array and writes this transaction output into it.
+     *
+     * @return byte array containing the transaction output
+     */
+    public byte[] serialize() {
+        return write(ByteBuffer.allocate(getMessageSize())).array();
+    }
+
+    /** @deprecated use {@link #serialize()} */
+    @Deprecated
+    public byte[] bitcoinSerialize() {
+        return serialize();
+    }
+
+    /**
+     * Return the size of the serialized message. Note that if the message was deserialized from a payload, this
+     * size can differ from the size of the original payload.
+     *
+     * @return size of the serialized message in bytes
+     */
     public int getMessageSize() {
         int size = 8; // value
         size += VarInt.sizeOf(scriptBytes.length) + scriptBytes.length;
         return size;
     }
 
-    @Override
-    protected void bitcoinSerializeToStream(OutputStream stream) throws IOException {
-        Objects.requireNonNull(scriptBytes);
-        ByteUtils.writeInt64LE(value, stream);
-        // TODO: Move script serialization into the Script class, where it belongs.
-        stream.write(VarInt.of(scriptBytes.length).serialize());
-        stream.write(scriptBytes);
-    }
-
     /**
      * Returns the value of this output. This is the amount of currency that the destination address
      * receives.

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

@@ -5282,8 +5282,8 @@ public class Wallet extends BaseTaggableObject
                 result.updatedOutputValues = new ArrayList<>();
             }
             for (int i = 0; i < req.tx.getOutputs().size(); i++) {
-                TransactionOutput output = new TransactionOutput(tx,
-                        ByteBuffer.wrap(req.tx.getOutputs().get(i).bitcoinSerialize()));
+                TransactionOutput output = TransactionOutput.read(
+                        ByteBuffer.wrap(req.tx.getOutputs().get(i).bitcoinSerialize()), tx);
                 if (req.recipientsPayFees) {
                     // Subtract fee equally from each selected recipient
                     output.setValue(output.getValue().subtract(fee.divide(req.tx.getOutputs().size())));

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

@@ -16,6 +16,8 @@
 
 package org.bitcoinj.core;
 
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
 import org.bitcoinj.base.Address;
 import org.bitcoinj.base.BitcoinNetwork;
 import org.bitcoinj.base.Coin;
@@ -31,15 +33,21 @@ import org.hamcrest.CoreMatchers;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 
+import java.nio.ByteBuffer;
 import java.util.Arrays;
+import java.util.Iterator;
 import java.util.List;
+import java.util.Random;
+import java.util.stream.Stream;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
+@RunWith(JUnitParamsRunner.class)
 public class TransactionOutputTest extends TestWithWallet {
 
     @Before
@@ -117,4 +125,33 @@ public class TransactionOutputTest extends TestWithWallet {
                 BitcoinNetwork.TESTNET));
         p2wpkh.toString();
     }
+
+    @Test
+    @Parameters(method = "randomOutputs")
+    public void write(TransactionOutput output) {
+        ByteBuffer buf = ByteBuffer.allocate(output.getMessageSize());
+        output.write(buf);
+        assertFalse(buf.hasRemaining());
+    }
+
+    private Iterator<TransactionOutput> randomOutputs() {
+        Random random = new Random();
+        Transaction parent = new Transaction();
+        return Stream.generate(() -> {
+            byte[] randomBytes = new byte[100];
+            random.nextBytes(randomBytes);
+            return new TransactionOutput(parent, Coin.ofSat(Math.abs(random.nextLong())), randomBytes);
+        }).limit(10).iterator();
+    }
+
+    @Test
+    public void negativeValue_minusOne() {
+        // -1 is allowed because it is used as a sentinel value
+        new TransactionOutput(new Transaction(), Coin.ofSat(-1), new byte[0]);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void negativeValue() {
+        new TransactionOutput(new Transaction(), Coin.ofSat(-2), new byte[0]);
+    }
 }

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

@@ -740,7 +740,7 @@ public class TransactionTest {
             long outputsSize = hackOutputsSize ? Integer.MAX_VALUE : getOutputs().size();
             stream.write(VarInt.of(outputsSize).serialize());
             for (TransactionOutput out : getOutputs())
-                out.bitcoinSerializeToStream(stream);
+                stream.write(out.serialize());
             // script_witnisses
             if (useSegwit) {
                 for (TransactionInput in : getInputs()) {