Document & test the conversion from getrawtransaction response to `…

…RawTransaction`

This also modifies the internal `common.ZcashdRpcReplyGetrawtransaction`
type to ensure that the reinterpretation of the `-1` height value from
a `getrawtransaction` response is not platform-dependent.

common/common.go

@@ -127,7 +127,7 @@ type (
 	// many more fields but these are the only ones we current need.
 	ZcashdRpcReplyGetrawtransaction struct {
 		Hex    string
-		Height int
+		Height int64
 	}
 
 	// zcashd rpc "getaddressbalance"

frontend/service.go

@@ -307,6 +307,43 @@ func (s *lwdStreamer) GetLatestTreeState(ctx context.Context, in *walletrpc.Empt
 	return s.GetTreeState(ctx, &walletrpc.BlockID{Height: uint64(latestHeight)})
 }
 
+// ParseRawTransaction converts between the JSON result of a `zcashd`
+// `getrawtransaction` call and the `RawTransaction` protobuf type.
+//
+// Due to an error in the original protobuf definition, it is necessary to
+// reinterpret the result of the `getrawtransaction` RPC call. Zcashd will
+// return the int64 value `-1` for the height of transactions that appear in
+// the block index, but which are not mined in the main chain. `service.proto`
+// defines the height field of `RawTransaction` to be a `uint64`, and as such
+// we must map the response from the zcashd RPC API to be representable within
+// this space. Additionally, the `height` field will be absent for transactions
+// in the mempool, resulting in the default value of `0` being set. Therefore,
+// the meanings of the `Height` field of the `RawTransaction` type are as
+// follows:
+//
+// * height 0: the transaction is in the mempool
+// * height 0xffffffffffffffff: the transaction has been mined on a fork that
+//   is not currently the main chain
+// * any other height: the transaction has been mined in the main chain at the
+//   given height
+func ParseRawTransaction(message json.RawMessage) (*walletrpc.RawTransaction, error) {
+		// Many other fields are returned, but we need only these two.
+		var txinfo common.ZcashdRpcReplyGetrawtransaction
+		err := json.Unmarshal(message, &txinfo)
+		if err != nil {
+			return nil, err
+		}
+		txBytes, err := hex.DecodeString(txinfo.Hex)
+		if err != nil {
+			return nil, err
+		}
+
+		return &walletrpc.RawTransaction{
+			Data:   txBytes,
+			Height: uint64(txinfo.Height),
+		}, nil
+}
+
 // GetTransaction returns the raw transaction bytes that are returned
 // by the zcashd 'getrawtransaction' RPC.
 func (s *lwdStreamer) GetTransaction(ctx context.Context, txf *walletrpc.TxFilter) (*walletrpc.RawTransaction, error) {
@@ -328,20 +365,7 @@ func (s *lwdStreamer) GetTransaction(ctx context.Context, txf *walletrpc.TxFilte
 		if rpcErr != nil {
 			return nil, rpcErr
 		}
-		// Many other fields are returned, but we need only these two.
-		var txinfo common.ZcashdRpcReplyGetrawtransaction
-		err = json.Unmarshal(result, &txinfo)
-		if err != nil {
-			return nil, err
-		}
-		txBytes, err := hex.DecodeString(txinfo.Hex)
-		if err != nil {
-			return nil, err
-		}
-		return &walletrpc.RawTransaction{
-			Data:   txBytes,
-			Height: uint64(txinfo.Height),
-		}, nil
+		return ParseRawTransaction(result)
 	}
 
 	if txf.Block != nil && txf.Block.Hash != nil {

frontend/service_test.go

@@ -3,6 +3,7 @@ package frontend
 import (
 	"encoding/json"
 	"testing"
+	"math"
 
 	"github.com/zcash/lightwalletd/common"
 )
@@ -36,3 +37,32 @@ func TestZcashdRpcReplyUnmarshalling(t *testing.T) {
 		}
 }
 
+func TestParseRawTransaction(t *testing.T) {
+		rt0, err0 := ParseRawTransaction([]byte("{\"hex\": \"deadbeef\", \"height\": 123456}"))
+		if err0 != nil {
+			t.Fatal("Failed to parse raw transaction response with known height.")
+		}
+		if rt0.Height != 123456 {
+			t.Errorf("Unmarshalled incorrect height: got: %d, expected: 123456.", rt0.Height)
+		}
+
+		rt1, err1 := ParseRawTransaction([]byte("{\"hex\": \"deadbeef\", \"height\": -1}"))
+		if err1 != nil {
+			t.Fatal("Failed to parse raw transaction response for a known tx not in the main chain.")
+		}
+		// We expect the int64 value `-1` to have been reinterpreted as a uint64 value in order
+		// to be representable as a uint64 in `RawTransaction`. The conversion from the twos-complement
+		// signed representation should map `-1` to `math.MaxUint64`.
+		if rt1.Height != math.MaxUint64 {
+			t.Errorf("Unmarshalled incorrect height: got: %d, want: 0x%X.", rt1.Height, uint64(math.MaxUint64))
+		}
+
+		rt2, err2 := ParseRawTransaction([]byte("{\"hex\": \"deadbeef\"}"))
+		if err2 != nil {
+			t.Fatal("Failed to parse raw transaction response for a tx in the mempool.")
+		}
+		if rt2.Height != 0 {
+			t.Errorf("Unmarshalled incorrect height: got: %d, expected: 0.", rt2.Height)
+		}
+}
+

walletrpc/service.proto

@@ -31,12 +31,31 @@ message TxFilter {
      bytes hash = 3;        // transaction ID (hash, txid)
 }
 
-// RawTransaction contains the complete transaction data. It also optionally includes 
-// the block height in which the transaction was included, or, when returned
-// by GetMempoolStream(), the latest block height.
+// RawTransaction contains the complete transaction data. It also includes the
+// height for the block in which the transaction was included in the main
+// chain, if any (as detailed below).
 message RawTransaction {
-    bytes data = 1;     // exact data returned by Zcash 'getrawtransaction'
-    uint64 height = 2;  // height that the transaction was mined (or -1)
+    // The serialized representation of the Zcash transaction.
+    bytes data = 1;
+    // The height at which the transaction is mined, or a sentinel value.
+    //
+    // Due to an error in the original protobuf definition, it is necessary to
+    // reinterpret the result of the `getrawtransaction` RPC call. Zcashd will
+    // return the int64 value `-1` for the height of transactions that appear
+    // in the block index, but which are not mined in the main chain. Here, the
+    // height field of `RawTransaction` was erroneously created as a `uint64`,
+    // and as such we must map the response from the zcashd RPC API to be
+    // representable within this space. Additionally, the `height` field will
+    // be absent for transactions in the mempool, resulting in the default
+    // value of `0` being set. Therefore, the meanings of the `height` field of
+    // the `RawTransaction` type are as follows:
+    //
+    // * height 0: the transaction is in the mempool
+    // * height 0xffffffffffffffff: the transaction has been mined on a fork that
+    //   is not currently the main chain
+    // * any other height: the transaction has been mined in the main chain at the
+    //   given height
+    uint64 height = 2;
 }
 
 // A SendResponse encodes an error code and a string. It is currently used