Document & test the semantics of GetRawTransaction, and fix platform-dependent parsing.

CHANGELOG.md

@@ -0,0 +1,28 @@
+# Changelog
+All notable changes to this library will be documented in this file.
+
+The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
+and this library adheres to Rust's notion of
+[Semantic Versioning](https://semver.org/spec/v2.0.0.html).
+
+## [Unreleased]
+
+### Changed
+
+- The `RawTransaction` values returned from a call to `GetMempoolStream`
+  now report a `Height` value of `0`, in order to be consistent with
+  the results of calls to `GetTransaction`. See the documentation of
+  `RawTransaction` in `walletrpc/service.proto` for more details on
+  the semantics of this field.
+
+### Fixed
+
+- Parsing of `getrawtransaction` results is now platform-independent.
+  Previously, values of `-1` returned for the transaction height would
+  be converted to different `RawTransaction.Height` values depending
+  upon whether `lightwalletd` was being run on a 32-bit or 64-bit 
+  platform.
+
+## [Prior Releases]
+
+This changelog was not created until after the release of v0.4.17

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"
@@ -522,6 +522,43 @@ func GetBlockRange(cache *BlockCache, blockOut chan<- *walletrpc.CompactBlock, e
 	errOut <- nil
 }
 
+// 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 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
+}
+
 func displayHash(hash []byte) string {
 	return hex.EncodeToString(parser.Reverse(hash))
 }

common/common_test.go

@@ -9,6 +9,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"math"
 	"os"
 	"strings"
 	"testing"
@@ -604,7 +605,7 @@ func mempoolStub(method string, params []json.RawMessage) (json.RawMessage, erro
 		if txid != "mempooltxid-1" {
 			testT.Fatal("unexpected txid")
 		}
-		r, _ := json.Marshal("aabb")
+		r, _ := json.Marshal(map[string]string{"hex":"aabb"})
 		return r, nil
 	case 5:
 		// Simulate that still no new block has arrived ...
@@ -636,7 +637,7 @@ func mempoolStub(method string, params []json.RawMessage) (json.RawMessage, erro
 		if txid != "mempooltxid-2" {
 			testT.Fatal("unexpected txid")
 		}
-		r, _ := json.Marshal("ccdd")
+		r, _ := json.Marshal(map[string]string{"hex":"ccdd"})
 		return r, nil
 	case 8:
 		// A new block arrives, this will cause these two tx to be returned
@@ -668,7 +669,7 @@ func TestMempoolStream(t *testing.T) {
 		return nil
 	})
 	if err != nil {
-		t.Fatal("GetMempool failed")
+		t.Errorf("GetMempool failed: %v", err)
 	}
 
 	// This should return two transactions.
@@ -677,7 +678,7 @@ func TestMempoolStream(t *testing.T) {
 		return nil
 	})
 	if err != nil {
-		t.Fatal("GetMempool failed")
+		t.Errorf("GetMempool failed: %v", err)
 	}
 	if len(replies) != 2 {
 		t.Fatal("unexpected number of tx")
@@ -687,13 +688,13 @@ func TestMempoolStream(t *testing.T) {
 	if !bytes.Equal([]byte(replies[0].GetData()), []byte{0xaa, 0xbb}) {
 		t.Fatal("unexpected tx contents")
 	}
-	if replies[0].GetHeight() != 200 {
+	if replies[0].GetHeight() != 0 {
 		t.Fatal("unexpected tx height")
 	}
 	if !bytes.Equal([]byte(replies[1].GetData()), []byte{0xcc, 0xdd}) {
 		t.Fatal("unexpected tx contents")
 	}
-	if replies[1].GetHeight() != 200 {
+	if replies[1].GetHeight() != 0 {
 		t.Fatal("unexpected tx height")
 	}
 
@@ -710,3 +711,61 @@ func TestMempoolStream(t *testing.T) {
 	sleepCount = 0
 	sleepDuration = 0
 }
+
+func TestZcashdRpcReplyUnmarshalling(t *testing.T) {
+		var txinfo0 ZcashdRpcReplyGetrawtransaction
+		err0 := json.Unmarshal([]byte("{\"hex\": \"deadbeef\", \"height\": 123456}"), &txinfo0)
+		if err0 != nil {
+			t.Fatal("Failed to unmarshal tx with known height.")
+		}
+		if txinfo0.Height != 123456 {
+			t.Errorf("Unmarshalled incorrect height: got: %d, want: 123456.", txinfo0.Height)
+		}
+
+		var txinfo1 ZcashdRpcReplyGetrawtransaction
+		err1 := json.Unmarshal([]byte("{\"hex\": \"deadbeef\", \"height\": -1}"), &txinfo1)
+		if err1 != nil {
+			t.Fatal("failed to unmarshal tx not in main chain")
+		}
+		if txinfo1.Height != -1 {
+			t.Errorf("Unmarshalled incorrect height: got: %d, want: -1.", txinfo1.Height)
+		}
+
+		var txinfo2 ZcashdRpcReplyGetrawtransaction
+		err2 := json.Unmarshal([]byte("{\"hex\": \"deadbeef\"}"), &txinfo2)
+		if err2 != nil {
+			t.Fatal("failed to unmarshal reply lacking height data")
+		}
+		if txinfo2.Height != 0 {
+			t.Errorf("Unmarshalled incorrect height: got: %d, want: 0.", txinfo2.Height)
+		}
+}
+
+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)
+		}
+}

common/mempool.go

@@ -1,7 +1,6 @@
 package common
 
 import (
-	"encoding/hex"
 	"encoding/json"
 	"sync"
 	"time"
@@ -105,35 +104,33 @@ func refreshMempoolTxns() error {
 			// We've already fetched this transaction
 			continue
 		}
-		g_txidSeen[txid(txidstr)] = struct{}{}
+
 		// We haven't fetched this transaction already.
+		g_txidSeen[txid(txidstr)] = struct{}{}
 		txidJSON, err := json.Marshal(txidstr)
 		if err != nil {
 			return err
 		}
-		// The "0" is because we only need the raw hex, which is returned as
-		// just a hex string, and not even a json string (with quotes).
-		params := []json.RawMessage{txidJSON, json.RawMessage("0")}
+
+		params := []json.RawMessage{txidJSON, json.RawMessage("1")}
 		result, rpcErr := RawRequest("getrawtransaction", params)
 		if rpcErr != nil {
 			// Not an error; mempool transactions can disappear
 			continue
 		}
-		// strip the quotes
-		var txStr string
-		err = json.Unmarshal(result, &txStr)
-		if err != nil {
-			return err
-		}
-		txBytes, err := hex.DecodeString(txStr)
+
+		rawtx, err := ParseRawTransaction(result)
 		if err != nil {
 			return err
 		}
-		newRtx := &walletrpc.RawTransaction{
-			Data:   txBytes,
-			Height: uint64(g_lastBlockChainInfo.Blocks),
+
+		// Skip any transaction that has been mined since the list of txids
+		// was retrieved.
+		if (rawtx.Height != 0) {
+			continue;
 		}
-		g_txList = append(g_txList, newRtx)
+
+		g_txList = append(g_txList, rawtx)
 	}
 	return nil
 }

frontend/service.go

@@ -314,34 +314,19 @@ func (s *lwdStreamer) GetTransaction(ctx context.Context, txf *walletrpc.TxFilte
 		if len(txf.Hash) != 32 {
 			return nil, errors.New("transaction ID has invalid length")
 		}
-		leHashStringJSON, err := json.Marshal(hex.EncodeToString(parser.Reverse(txf.Hash)))
+		txidJSON, err := json.Marshal(hex.EncodeToString(parser.Reverse(txf.Hash)))
 		if err != nil {
 			return nil, err
 		}
-		params := []json.RawMessage{
-			leHashStringJSON,
-			json.RawMessage("1"),
-		}
-		result, rpcErr := common.RawRequest("getrawtransaction", params)
 
-		// For some reason, the error responses are not JSON
+		params := []json.RawMessage{txidJSON, json.RawMessage("1")}
+		result, rpcErr := common.RawRequest("getrawtransaction", params)
 		if rpcErr != nil {
+			// For some reason, the error responses are not JSON
 			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 common.ParseRawTransaction(result)
 	}
 
 	if txf.Block != nil && txf.Block.Hash != nil {

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